Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#14423 User can verify multiple times with the same two factor token #57728

Closed
wants to merge 2 commits into from

Conversation

andkorsh
Copy link

@andkorsh andkorsh commented Sep 6, 2024

Fixes #14423 User can verify multiple times with the same two factor token

Description

Adding the validation for used TOTP to comply with Section 5.2 of RFC 6238 as well as NIST guidelines (SP 800-63B, 5.1.4.2).

@andkorsh andkorsh requested a review from a team as a code owner September 6, 2024 17:01
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-identity Includes: Identity and providers label Sep 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 6, 2024
@andkorsh
Copy link
Author

andkorsh commented Sep 6, 2024

@dotnet-policy-service agree

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Sep 17, 2024
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this requires a breaking change to IUserAuthenticatorKeyStore. This change would require a non-breaking API proposal and a new IdentitySchemaVersion at a minimum.

Even if this PR did include changes to the EF stores, I would not accept it because I do not think it's worth the complexity of maintaining a new schema and the conditional logic that would entail just to prevent TOTP reuse. Furthermore, the code as proposed has a race condition where the TOTP could be reused anyway. However, if you feel that your application does need protection against TOTP reuse, you are welcome to implement a custom IUserTwoFactorTokenProvider<TUser>.

We have updated our documentation to explicitly warn about this:

Warning: An ASP.NET Core TOTP code should be kept secret because it can be used to authenticate successfully multiple times before it expires.

https://learn.microsoft.com/en-us/aspnet/core/security/authentication/identity-enable-qrcodes?view=aspnetcore-8.0

I acknowledge that this does not follow the RFC or NIST guidelines, but as I mentioned in the issue, the TOTP still offers a lot of additional security over just a password even if it can be used multiple times in a 30 second window. If an attacker can read arbitrary network traffic and sniff the TOTP from the form post, they'd usually be better off stealing the authentication cookie that will last a lot longer than 30 seconds.

@halter73 halter73 closed this Oct 11, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview1 milestone Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-identity Includes: Identity and providers community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASP.NET Identity Core 2.1 - User can verify multiple times with the same two factor token
2 participants