passwordless-lib / fido2-net-lib

FIDO2 .NET library for FIDO2 / WebAuthn Attestation and Assertion using .NET
https://fido2-net-lib.passwordless.dev/
MIT License
1.18k stars 168 forks source link

possible issue with AndroidSafetyNet timestampMs issue #93

Closed apurvabhansali closed 5 years ago

apurvabhansali commented 5 years ago

I think I found a possible issue here. We are calling with attestation type direct because we want to get the aaguid of the authenticator for later display in our UI. On the android phone when we call RequestNewCredential in the library, we are getting randomingly the

Fido2VerificationException("Android SafetyNet timestampMs must be between one minute ago and now")

that is thrown. I believe the reason is that because it may not be the case that the phone and the server are perfectly synced in time. If the android phone is, for example, .5 sec behind the server, then it is possible, they may show the same time, but the timestamp received from the Android phone will actually be less than DateTime.UtcNow on the server. This is then throwing the exception.

I think this logic needs to be thought through, for now in our code, we have just commented out this check.

The 2nd "possible" issue with this check is that you are giving an upper bound on time at 1 min (i.e. it should not be longer than a minute from effectively the credential create on the client to the requestNewCredential api on the server). But it is not clear when the client generates the timestamp. On the android phone, I get the dialogs from Android which ask the user to first click getstarted, then to select the type of authenticator, and then to do the authentication itself. This process could take over a minute, but it is okay if the timestamp is generated at the end of the process instead of the beginning. Any insight on this?

aseigler commented 5 years ago

That timestamp check is there to pass FIDO conformance test checks. What we should probably do is rework that check to allow user configurable thresholds when not conformance testing, and the strict time constraints required by the conformance tests when testing is underway.

Does that make sense?

apurvabhansali commented 5 years ago

ok I did not know that was a conformance test issue. I feel a typical server administrator may not know how to set all those kind of thresholds in real world scenarios. But that maybe the best answer.

Maybe a better solution would be to provide some timesyncing API between client and server that would automatically set the "threshold". So if I call the api (which would take in a timestamp from the client) and then compare that to UtcNow when the server receives the api call, then the delta can be "threshold" (with a little additional fudge factor), and this amount is used later when the requestnewcredential call is done.

As far the conformance test, they would not call the api, so threshold would remain 0. But this api could be called by the client whenever it is starting an attestation sequence to set the "real world" threshold Thoughts?

aseigler commented 5 years ago

We can offer some sane defaults that are more reasonable than the conformance testing requirements, and let the user configure them if they want to. There is precedence for this, check out the Timeout and ChallengeSize in the Configuration class.

apurvabhansali commented 5 years ago

ok, this can work, and I guess 3rd parties like ourselves can extend this capability differently as we desire.

aseigler commented 5 years ago

You can add something like

"TimestampDriftTolerance": 1000000

to the appSettings.json or a local environment variable to set how much drift you wish to allow to make your scenario work.

abergs commented 5 years ago

@aseigler To make sure develoeprs who run into the exception understand it, we should adjust the exceptionmessage to include the actual drift instead of "between one minute ago and now".

aseigler commented 5 years ago

Totally agree, message should be "got {timestamp}, not within bounds of {upper} {lower}"