okta / okta-oidc-android

OIDC SDK for Android
https://github.com/okta/okta-oidc-android
Other
60 stars 45 forks source link

OKTA-414753 Remove time dependant checks from ID Token validation. #283

Closed JayNewstrom closed 2 years ago

JayNewstrom commented 2 years ago

Fixes #256

This isn't needed because in the flow the SDK uses, we know (from HTTPS) that we're getting the tokens from the server we're expecting.

Description:

Testing details:

Other considerations:

RESOLVES:

OKTA-414753

Primary Reviewer(s):

Additional Reviewers:
Security Reviewer(s) (@ okta/rex-team if necessary):
UX Reviewer(s) (if necessary):
JayNewstrom commented 2 years ago

supersedes https://github.com/okta/okta-oidc-android/pull/281

FeiChen-okta commented 2 years ago

Wait for @federicomuttis-okta to review. The time validation was done according to https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation

9. The current time MUST be before the time represented by the exp Claim.
10. The iat Claim can be used to reject tokens that were issued too far away from the current time, limiting the amount of time that nonces need to be stored to prevent attacks. The acceptable range is Client specific.
JayNewstrom commented 2 years ago

Will do. The reasoning for considering the change is from Aaron Parecki.

He referenced a few links from google. https://developers.google.com/identity/protocols/oauth2/openid-connect?hl=cs#obtainuserinfo

Normally, it is critical that you validate an ID token before you use it, but since you are communicating directly with Google over an intermediary-free HTTPS channel and using your client secret to authenticate yourself to Google, you can be confident that the token you receive really comes from Google and is valid.

FeiChen-okta commented 2 years ago

Will do. The reasoning for considering the change is from Aaron Parecki.

He referenced a few links from google. https://developers.google.com/identity/protocols/oauth2/openid-connect?hl=cs#obtainuserinfo

Normally, it is critical that you validate an ID token before you use it, but since you are communicating directly with Google over an intermediary-free HTTPS channel and using your client secret to authenticate yourself to Google, you can be confident that the token you receive really comes from Google and is valid.

Does that recommendation apply to PKCE too? Mobile does not use client secret.

JayNewstrom commented 2 years ago

The reason I chose this over the previous was due to this being just as secure (according to Aaron, and from what I could tell by reading the Google link), as well as it being the default to do the thing that always works.

I don't think it's ideal for someone to have to configure the SDK to work for all users (by disabling these checks).

aaronpk commented 2 years ago

👋

There's literally no point in checking for the issued/expiration timestamps at this point in the SDK because there's no possible way for the server to return an ID token that isn't valid at the time the POST request is made.

It's a totally different story if the SDK were able to accept an ID token from the developer or if it used any of the hybrid OIDC modes. But when the ID token is given to the app in the HTTPS response from the token endpoint, the only way that can happen is if the app makes the HTTPS call itself, and the token is generated by the server at that point in time, so the timestamps add no new information.

In that sense it's actually safer to trust the server since as we've seen, the device clock is something that the user can modify for various reasons.

aaronpk commented 2 years ago

Does that recommendation apply to PKCE too? Mobile does not use client secret.

and yes, this has nothing to do with PKCE. PKCE is used whether or not you have a client secret. The note from the Google docs is specifically about the fact that the response is coming via HTTPS rather than via an HTTP redirect from the browser.

hansreichenbach-okta commented 2 years ago

Less likely to break is NOT the same thing as safety. Also this entire argument of not needing to check the timestamps is premised on HTTPS never breaking and us never needing to worry about MITM attacks. If that's our base assumption then no checks are necessary and it doesn't even need to be a JWT.

Device clock skew is a very real problem on a lot of devices and something we are trying to tackle internally for Okta Verify as well. Once we release our Push SDK we will need to solve this pattern for developers once again and that is a scenario where we DEFINITELY cannot just ignore the checks. I think we should start a pattern here and now with the OIDC SDK for the broader problem that can be shared across SDKs in the future and be familiar for developers. Allowing them to solve the time drift problem independently through a simple interface is a great starting point for that.

aaronpk commented 2 years ago

I agree that other scenarios like the Push SDK will require proper solutions to the clock drift problem.

this entire argument of not needing to check the timestamps is premised on HTTPS never breaking and us never needing to worry about MITM attacks

If you are assuming that you can't trust the HTTPS connection, then the entire thing falls apart since you can't trust you've fetched the correct JWKS over HTTPS either.

If MITM of an HTTPS connection is a potential concern, then you need to do things like certificate pinning at the TLS layer, not add in extra checks of data that you got over the potentially-MITM'd connection at the OIDC layer.

mikenachbaur-okta commented 2 years ago

If the primary concern is clock skew breaking validation, could we perhaps compare against the HTTP response header's timestamp, instead of the local system time? For example, in the iOS JWT SDK, it always attempts to fetch the .well-known configuration and keys when validating a token.

If we retrieve the server's time, we can theoretically trust that it's more reliably accurate than the client's clock.

aaronpk commented 2 years ago

If the primary concern is clock skew breaking validation, could we perhaps compare against the HTTP response header's timestamp, instead of the local system time?

Clever, but that doesn't buy you any additional assurance, and it's the same as not checking the timestamps. If the HTTPS connection is MITM'd then the attacker could change the timestamp in the response just as easily as changing the ID token in the response.

mikenachbaur-okta commented 2 years ago

I agree @aaronpk, at the end of the day we either trust the HTTPS connection, or we need to require certificate pinning.

hansreichenbach-okta commented 2 years ago

While it is true that HTTPS is our first and primary line of defense and other parts of the OIDC flow quickly fall apart under MITM conditions I think checks like this are important to limit the blast radius if those defenses do fail. If you have direct device access or are an internet backbone level actor it's pretty straightforward to MITM any device. Developers do it to themselves all the time to inspect network traffic when debugging. Time boundary checks like this let effects from a MITM attack drop off relatively quickly after it's mitigated instead of the attacker having a token that can be reused for quite some time.

Fundamentally the tension we're discussing here is the old Security vs Usability tradeoff. This fix lessens security and provides developers with a default (IMO exclusive since they don't have an easy way to add the check back) that is slightly less secure but much less likely to cause Usability issues for their users. The fix I'm arguing for would default to more secure at the expense of the usability for devices that have clock drift, but allow the developer to flip that trade-off or find some middle ground they prefer at their option.

Both fixes have been coded so there is negligible cost difference (we've spent far more time discussing this than the coding time difference) so that should not be a concern. My stance is that as a security company Okta should default to the more secure experience but in the name of supporting our developers give them a toggle and great README write-up about it explaining the trade-off and allowing them to make a different decision at their discretion.