Aspnetcore: 2FA Tokens do not expire after 30 seconds, but after 90!

[duki994]

Describe the bug

Just look at line 59:
// Allow codes from 90s in each direction (we could make this configurable?)

https://github.com/aspnet/AspNetCore/blob/master/src/Identity/Extensions.Core/src/AuthenticatorTokenProvider.cs

To Reproduce

Steps to reproduce the behavior:

1. Using all .NET Core 2.0+
2. Using any AspNetCore.Identity 2.0+ version

1. Try 2FA authentication
2. Read code from authenticator app on your phone
3. Remeber code
4. When it expires wait for 40s. In total that is 70s after expiration of token
5. Enter token
6. Token passes validation

It shouldn't pass validation.

And 90s in each direction is very dangerous.

Expected behavior

TOTP tokens expire after 30 seconds (+ maybe clock skew that should be configurable

Screenshots

If applicable, add screenshots to help explain your problem.

Additional context

.NET Core 2.2.5

[pranavkm]

/cc @blowdart @HaoK

[brockallen]

This is how TOTP works since code expire after 30 seconds. Imagine the code is generated and as soon as it's generated it expires, and then it's sent SMS or whatever. That means the code is already invalid by the time it arrives at the user. So this 90 second skew is to accommodate for those edge cases.

[blowdart]

What @brockallen said, it's by design. Pretty much all implementations give you a buffer/skew time.

[blowdart]

@duki994 Would you like it to be configurable? It may frustrate your users.

[duki994]

@brockallen
Sure. For SMS as TOTP code delivery sytem this clock skew is ok. Mobile network latency to end user can be quite an issue depending on region.

For 2FA authenticator apps 90 seconds clock-skew is too much in lots of implementations.

@blowdart
Configurable clock-skew would be nice. Or both clock-skew and time expiration.

I was just very confused because in documentation it's written that TOTP code is valid for 30 seconds.
But there was no mention about clock-skew and default value about clock skew. I assumed it was 15 seconds, but it was 90.

This was breaking issue for my customers and they were annoyed by this, but it was easy to change behavior by subclassing AuthenticatorTokenProvider<TUser> and overriding default implementation of ValidateAsync, plus providing custom IOptions so expiry time can be configured via startup (so customers can use appsettings.json, environment or whatever they want)

```
public class MyAuthenticatorTokenProvider : AuthenticatorTokenProvider
where TUser : class
{
private static readonly DateTime _unixEpoch = new DateTime(1970, 1, 1, 0, 0, 0);

    private readonly IOptionsSnapshot<MyAuthenticatorTokenProviderOptions> _options;

    public MyAuthenticatorTokenProvider(IOptionsSnapshot<MyAuthenticatorTokenProviderOptions> options) : base()
    {
        _options = options;
    }

    private long GetTimeStep() => (DateTime.UtcNow - _unixEpoch).Ticks / _options.Value.AuthenticatorTokenExpiryTime.Ticks;

    public override async Task<bool> ValidateAsync(string purpose, string token, UserManager<TUser> manager, TUser user)
    {
        var key = await manager.GetAuthenticatorKeyAsync(user);
        if (!int.TryParse(token, out int code))
        {
            return false;
        }

        var encodedKey = Base32Encoding.Standard.ToBytes(key);
        using (var hash = new HMACSHA1(encodedKey))
        {
            var expectedCode = Rfc6238AuthenticationService.ComputeTotp(hash, (ulong)(GetTimeStep()), modifier: null);
            return expectedCode == code;
        }
    }

}

````

@blowdart @pranavkm @HaoK @brockallen
I also have another question about line 56 in https://github.com/aspnet/AspNetCore/blob/master/src/Identity/Extensions.Core/src/AuthenticatorTokenProvider.cs

var hash = new HMACSHA1(Base32.FromBase32(key));

Why is this not wrapped in using statement?
It's using unmanaged SHA1CryptoServiceProvider underneath and it needs to dispose SafeHashHandle instance(s) used underneath>

[blowdart]

90 second skew is the defacto default for even TOTP apps.

As for having no using, there's no real unmanaged resources that have a long lifetime. Sure it'd be more correct, but it doesn't actually make a difference,

[blowdart]

And the TOTP values are valid for 30 seconds, that's when they rotate, but we accept values either side because of skew and UX reasons.

[duki994]

@blowdart

#

It's ok.

1. This skew should be configurable.
2. Or maybe just change docs to say that skew is 90s in each direction and provide sample implementation.

Also, _Rfc6238AuthenticationService_ has internal modifier
https://github.com/aspnet/AspNetCore/blob/master/src/Identity/Extensions.Core/src/Rfc6238AuthenticationService.cs

Maybe switch it to public so other consumers can use it?
In my example I had to copy that class and make it public (yes, reflection would have done same job, but it maynot obvious for other consultant programmers on project initially).

RFC6238 and RFC4226 standards that are used in _Rfc6238AuthenticationService_ did not change for several years.
That class needs to be public, so other consumers can use it.

And this class is internal too? I don't see reasoning behind making _Base32_ encoding class internal.
https://github.com/aspnet/AspNetCore/blob/master/src/Identity/Extensions.Core/src/Base32.cs

I do understand hiding _Base32_ from public usage since this is internal dependency, but for _Rfc6238AuthenticationService_ i don't get why internal modifier.

Exposing _Rfc6238AuthenticationService_ would allow users to use it in for specific features like mine. To compute TOTP and use it to override/create their custom authentication handlers with existing RFC6238 logic.

[blowdart]

Frankly we don't want to make it public because it was written for identity and not as a general purpose implementation, We'd have to move it's namespace, and that increases the support burden.,

[duki994]

@blowdart

Then at least clock-skew should be configurable. Adnd default would stay at 90s in each direction.

Proposed ideas for configurable clock-skew

Configurable by IOptions

My initial idea was to make it configurable via IOptions and to follow Options pattern.
But then, it makes typos or wrong configurations quite easy. Especially if values are read via appsettings.json or any other external source.
That would allow possibly very wrong configurations by ops teams.

Configurable by protected property - explicit subclassing of AuthenticatorTokenProvider

This allows configuration to be in code, in one place.
Just override property and set clock skew.

Plus manually add token provider and set it up to be provider for 2FA tokens in Identity builder in Startup.cs.

It's more explicit, and there is intention shown in code for overriding default behavior.
For simplicity, it would be one property for each time direction (past and future).

If clock-skew is wrongly configured, then it's easy to see where the error was made.
And it's easy to see that developer had to do a bit more job to override default behavior.

[dnxit]

That value should be configurable, Mobile network latency can be a problem sometimes and user gets already expired codes and then they're frustrated to get another one.

Some related issues.
https://github.com/aspnet/AspNetIdentity/issues/15
https://github.com/aspnet/Identity/issues/1633