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

oauth2 filter: nonce implementation refinements #36276

Open
denniskniep opened this issue Sep 21, 2024 · 4 comments
Open

oauth2 filter: nonce implementation refinements #36276

denniskniep opened this issue Sep 21, 2024 · 4 comments
Labels
enhancement Feature requests. Not bugs or questions.

Comments

@denniskniep
Copy link
Contributor

I just want to open a discussion about the newly introduced nonce cookie in the oauth2 filter.
There are several aspects I want to highlight.

1. Generation

The current implementation sets a timestamp as nonce and encodes it into the state param of the oidc request.

std::string generateFixedLengthNonce(TimeSource& time_source) {
constexpr size_t length = 16;
std::string nonce = fmt::format("{}", time_source.systemTime().time_since_epoch().count());

As far as I understood, the current implementation wants to mitigate CSRF. Which is considered a best practice (see here: OAuth 2.0 Threat Model and Security Considerations)

From my point of view setting the nonce to the current timestamp creates an opportunity for attackers to predict or guess the nonce value. It would be better to use a randomly generated value combined with the current timestamp. The randomly generated value makes it non-guessable while the timestamp ensures uniqueness. If we apply a hash on the combined values then we get an opaque value.

See the following as references:

The binding value used for CSRF protection MUST contain a non-guessable value

https://datatracker.ietf.org/doc/html/rfc6749#section-10.12

e.g., a hash of the session cookie used to authenticate the user agent

https://datatracker.ietf.org/doc/html/rfc6819#section-5.3.5

RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie.

https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest

using a unique and non-guessable value associated with each authentication request about to be initiated

https://auth0.com/docs/secure/attack-protection/state-parameters#csrf-attacks

Arbitrary (often random or pseudo-random) number

https://auth0.com/docs/glossary?term=nonce

2. One-time use

Furthermore the nonce cookie expires after 10 minutes.

std::string expire_in = std::to_string(10 * 60);

Meaning there is a chance that the nonce is used for multiple authentication requests, which is not recommended.

See the following as references:

Clients MUST prevent CSRF. One-time use CSRF tokens carried in the "state" parameter, which are securely bound to the user agent, SHOULD be used for that purpose.
https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-13#section-3.1

The state value SHOULD be invalidated by the client after its first use at the redirection endpoint
https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#section-4.2.4

In other words, the nonce is only issued once
https://auth0.com/docs/glossary?term=nonce

3. Use also oidc's nonce parameter

The current implementation uses the RECOMMENDED state parameter to mitigate CSRF, but there is also an OPTIONAL nonce parameter defined in the spec which mitigates replay attacks.

state
RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie.
https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest

nonce
OPTIONAL. String value used to associate a Client session with an ID Token, and to mitigate replay attacks. The value is passed through unmodified from the Authentication Request to the ID Token. Sufficient entropy MUST be present in the nonce values used to prevent attackers from guessing values. For implementation notes, see Section 15.5.2.
https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest

Because we are using the nonce cookie already for the state parameter, we could easily use it for the nonce parameter. We must add another validation when we receive the ID Token. Validate that the ID Token contains a nonce claim, which must match the value of the nonce cookie. This would mitigate replay attacks.

see also:
https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes
https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#section-4.5.3.2

4. Include Nonce into the HMAC cookie

The entropy of the HMAC-Cookie could be enhanced by adding the nonce value. It would improve security especially in scenarios where the tokens are not stored into cookies. And therefore also not part of the HMAC-Cookie. Adding the Nonce value would ensure that the HMAC-Cookie incorporates always and in every scenario a unique and non-guessable value, which enhances security significantly.

Summary

In summary from my point of view we should do the following:

  • Nonce should be generated via hash(randomValue + timestamp) which would fulfill the requirements: unique, non-guessable & opaque
  • The Nonce should be invalidated after usage. A fresh Nonce should be generated for each and every authentication request.
  • Use the oidc nonce parameter itself (not only state parameter)
  • Include Nonce into the HMAC cookie generation

Would be happy to discuss this and I'm looking forward to your feedback!
cc: @zhaohuabing @mattklein123 @derekargueta

@denniskniep denniskniep added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Sep 21, 2024
@zhaohuabing
Copy link
Member

zhaohuabing commented Sep 22, 2024

@denniskniep Thanks for opening this discussion. I appreciate the thorough review of the nonce implementation in the OAuth2 filter and agree that there are several improvements we can consider. Below are my thoughts:

1. Nonce Generation
I fully agree that relying solely on the timestamp for nonce generation may introduce predictability. We can combine the timestamp with the HMAC secret to generate a more secure, unpredictable nonce.

2. One-time Use of Nonce
The Nonce is stored in a cookie because:

  • The nonce needs to be sent along with the callback request so that the OAuth2 filter can validate it against the nonce in the redirect response.
  • The browser often sends multiple requests before the first one gets authenticated, resulting in multiple callbacks from the OAuth2 server, which may not always arrive in the same order as the requests were sent. The same nonce is used for these requests to ensure successful nonce validation.

The 10-minute validity period was chosen as a reasonable window, as users typically complete the authentication process within that time frame. However, I agree that this doesn’t strictly enforce single-use. This could be improved by deleting the nonce cookie after the first use.

3. Utilization of OIDC Nonce Parameter
Could we track this in a separate issue? This seems like a new feature rather than an enhancement of the existing implementation. Additionally, it should likely be opt-in, as not all OAuth2 providers may support it.

4. Including Nonce in the HMAC Cookie
Nonce can't be used to generate the HMAC cookie if the nonce cookie is deleted after the first use.

Thanks again for bringing these points to. Looking forward to your thoughts!

@jmarantz jmarantz removed the triage Issue requires triage label Sep 23, 2024
@denniskniep
Copy link
Contributor Author

Happy to hear your positive feedback.

2. One-time Use of Nonce

You are right, we need to solve the problem, that multiple authentication requests could be sent, which results in multiple callbacks

What about generating a unique nonce cookie linked to the authentication request? The cookie containing this nonce would have a unique identifier related to that specific request. This way, multiple requests won't cause issues because each callback will only delete the cookie related to its own request. To prevent the browser from being filled with unused authentication request nonce cookies (for example, if the callback never happens), we can restrict the cookie's validity period to 10 minutes.

Example:

  1. Authentication Request Nr. 1 issues Cookie nonce-t473jhu:<UniqueNonceValueA> and send request with queryparam state=nonce-t473jhu=<UniqueNonceValueA>
  2. Authentication Request Nr. 2 issues Cookie nonce-fu7ddgt:<UniqueNonceValueB> and send request with queryparam state=nonce-fu7ddgt=<UniqueNonceValueB>
  3. Received Authentication Callback for Request Nr. 2 with Queryparameter state=nonce-fu7ddgt=<UniqueNonceValueB>. Then we need to validate corresponding nonce and clear cookie nonce-fu7ddgt:<UniqueNonceValueB>
  4. Received Authentication Callback for Request Nr. 1 with Queryparameter state=nonce-t473jhu=<UniqueNonceValueA>. Then we need to validate corresponding nonce and clear cookie nonce-t473jhu:<UniqueNonceValueA>

3. Utilization of OIDC Nonce Parameter

I fully agree to create new issue for topic 3 and to model it as an opt-in feature (also to not break existing adoptions)

4. Including Nonce in the HMAC Cookie

Nonce can't be used to generate the HMAC cookie if the nonce cookie is deleted after the first use.

That's actually true. And even more difficult if we might have multiple nonce cookies. Therefore I propose to implement this mechanism independent of the nonce. We could simply generate a secure, httpOnly, unique and non-guessable session cookie. That is always generated, when the OauthHMAC Cookie is generated. My proposal for a name is OauthSessionId

Maybe we should also create this as a separate ticket, if we are not using the nonce value for this.

Copy link

github-actions bot commented Nov 2, 2024

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 2, 2024
@denniskniep
Copy link
Contributor Author

not stale

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions.
Projects
None yet
Development

No branches or pull requests

3 participants