OWASP / ASVS

Application Security Verification Standard
Creative Commons Attribution Share Alike 4.0 International
2.71k stars 662 forks source link

51.2.2 - what is the purpose for the requirement? #2092

Open elarlang opened 2 weeks ago

elarlang commented 2 weeks ago

Current requirement 51.2.2:

# Description L1 L2 L3
51.2.2 [ADDED] Verify that the replay of authorization codes into the authorization response is prevented either by using the PKCE flow or alternatively the OpenID Connect "nonce" parameter and the respective Claim in the ID Token. The PKCE challenge or OpenID Connect "nonce" must be transaction-specific and securely bound to the client and the user agent in which the transaction was started.

I'm quite confused, what is to goal for the requirement.

It starts with a claim to prevent authorization code replay into the authorization response - authorization code in authorization response is handled by the client and this is basically CSRF vector to prevent, it is covered now in 51.3.5:

# Description L1 L2 L3
51.3.5 [ADDED] Verify that, if the code flow is used, the OAuth Client has protection against CSRF attacks which trigger token requests, either by using PKCE functionality or checking the state parameter that was sent in the authorization request.

Further, it requires validating the nonce in ID token, but this is to mitigate ID token replay attack to (OIDC) the client, but for that we have also a separate requirement (at the moment it is in wrong section, but this is another topic to discuss):

# Description L1 L2 L3
51.3.2 [ADDED] Verify that the Client is using the PKCE flow or alternatively the OpenID Connect "nonce" parameter and the respective Claim in the ID Token.

For me the requirement 51.2.2 seems to mix different things. Or did I miss the actual point for the requirement?

csfreak92 commented 2 weeks ago

In the way I understood it, 51.2.2 is for ensuring that when you use PKCE challenge or OIDC nonce they should be transaction-specific to prevent replay of these codes as well.

No, 51.3.5 does not re-use those authorization codes as a CSRF protection. I haven't seen those authorization codes being used to prevent the CSRF vector. Maybe someone else has more knowledge about it from the community who can help us about this, but as far as I know that is not what those authorization codes help prevent.

Although, I believe you are correct that the way 51.3.2 is written it is already handled in 51.2.2. We can remove 51.3.2 then since 51.2.2 handles it.

elarlang commented 2 weeks ago

Ack, the focus is "transaction-specific".

Additionally, @randomstuff asked a question in https://github.com/OWASP/ASVS/issues/2041#issuecomment-2354018055

Is there any need to say the the nonce must be a unpredictable?

I think we need to reword it what problem the requirement solves and avoid duplication with other requirements.

elarlang commented 1 week ago

@randomstuff @TobiasAhnoff - any ideas how to solve this?

randomstuff commented 1 week ago

Although, I believe you are correct that the way 51.3.2 is written it is already handled in 51.2.2. We can remove 51.3.2 then since 51.2.2 handles it.

Yes, I agree. They are redundant.

randomstuff commented 1 week ago

Is there any need to say the the nonce must be a unpredictable?

I think, this should probably be somewhere and I don't believe it's currently really states but it is really more general than OAuth and should probably be somewhere else as well.

This is related to:

6.3.1, Verify that all random numbers, random file names, and random strings are generated using a cryptographically-secure pseudo-random number generator (CSPRNG) when these random values are intended to be not guessable by an attacker.

6.3.3, Verify that random numbers are created with proper entropy even when the application is under heavy load, or that the application degrades gracefully in such circumstances.

"sufficient entropy" is somewhat left to interpretation. OAuth requires at least 128 bits and recommends 160 bits of entropy for tokens:

The probability of an attacker guessing generated tokens (and other credentials not intended for handling by end users) MUST be less than or equal to 2^(-128) and SHOULD be less than or equal to 2^(-160).

Note that the the wording in the the OAuth 2.1 draft is more general than the requirement in 6.3.3 as it applies to random/opaque tokens and for signature/MAC tokens.

So the questions are:

elarlang commented 1 week ago

I think the randomness is well covered in V6 (and if it is not, for general requirements it should be covered there)

Ack, the focus is "transaction-specific".

I think the main focus should be "transaction-specific" to mitigate replay attacks.

TobiasAhnoff commented 1 week ago

A thing to note is that this is part of the "OAuth Authorization Server", not the "OIDC OpenID Provider" section, so OIDC details should be avoided in the requirement, split, moved to OIDC or should we mix OAuth and OIDC in this case?

For OAuth we could have

Verify that the replay of authorization codes into the authorization response is prevented by using PKCE with a transaction-specific challenge which is securely bound to the client and the user agent in which the transaction was started.

For OIDC,, given that it is clear that all OAuth requirements also applies to OIDC it could be:

Verify that the replay of authorization codes into the authorization response is prevented either by using OAuth PKCE or alternatively the OpenID Connect "nonce" parameter and the respective Claim in the ID Token. The OpenID Connect "nonce" must be transaction-specific in the same way as the PKCE challenge.

or if we mix, like suggested above

Verify that the replay of authorization codes into the authorization response is prevented either by using the PKCE flow or alternatively the OpenID Connect "nonce" parameter and the respective Claim in the ID Token. The PKCE challenge or OpenID Connect "nonce" must be transaction-specific and securely bound to the client and the user agent in which the transaction was started.

I think none of them is perfect, but I think it is good to keep OIDC details separate to make it clear that OAuth can be used without OIDC and ID Tokens.

elarlang commented 1 week ago

I think my initial comment applies to the last proposal as well. I would not like to mix and duplicate ID token replay (51.3.2 (it is in incorrect section)) and OAuth client CSRF (51.3.5), additionally general PKCE requirement.

Probably 2 directions from here:

In a way, if we have requirements like "Verify that, if the code flow is used, the OAuth Client has protection against CSRF attacks", then if the cause of not having the defense is the missing transaction-specific token, it is still the same requirement and the same problem, it is just finding the technical detail in it.

TobiasAhnoff commented 1 hour ago

They way I see it (based on https://github.com/OWASP/ASVS/pull/2122/files) this is addressed by

51.2.3 Verify that, if the code grant is used, the authorization server mitigates authorization code interception attacks by requiring PKCE. For authorization requests, the authorization server must require a valid code_challenge value and must not accept code_challenge_method 'plain'. For a token request, it must require validation of the "code_verifier" parameter.

And for OIDC

51.5.1 Verify that the Client (as the Relying Party) mitigates ID Token replay attacks. For example, by ensuring that the nonce claim in the ID Token matches the nonce value sent in the Authentication Request to the OpenID Provider (in OAuth2 refereed to as the Authorization request sent to the Authorization Server)

So 51.2.2, 51.3.5 and 51.3.2 can be removed since they are addressed/replaced by 51.2.3 and 51.5.1 (and randomness is well covered in V6), maybe add CSRF for 51.2.3?

Verify that, if the code grant is used, the authorization server mitigates authorization code interception and CSRF attacks by requiring PKCE. For authorization requests, the authorization server must require a valid code_challenge value and must not accept code_challenge_method 'plain'. For a token request, it must require validation of the "code_verifier" parameter.

Or have I confused latest numbering?