-
-
Notifications
You must be signed in to change notification settings - Fork 673
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
51.2.15 - OAuth - ask to be transaction-specific #2092
Comments
In the way I understood it, No, Although, I believe you are correct that the way |
Ack, the focus is "transaction-specific". Additionally, @randomstuff asked a question in #2041 (comment)
I think we need to reword it what problem the requirement solves and avoid duplication with other requirements. |
@randomstuff @TobiasAhnoff - any ideas how to solve this? |
Yes, I agree. They are redundant. |
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:
"sufficient entropy" is somewhat left to interpretation. OAuth requires at least 128 bits and recommends 160 bits of entropy for tokens:
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:
|
I think the randomness is well covered in V6 (and if it is not, for general requirements it should be covered there)
I think the main focus should be "transaction-specific" to mitigate replay attacks. |
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
For OIDC,, given that it is clear that all OAuth requirements also applies to OIDC it could be:
or if we mix, like suggested above
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. |
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. |
They way I see it (based on https://github.com/OWASP/ASVS/pull/2122/files) this is addressed by
And for OIDC
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?
Or have I confused latest numbering? |
We need to do de-duplication round later, I have not opened issues for those yet, as the first goal is to get all requirements in. For this, the goal seems to be "transaction-specific", all other parts are too mixed here and are covered somewhere else. |
Ok, then I think this issue needs to suggest one requirement for OAuth and one for OIDC, perhaps this could work? OAuth
OIDC (given that it is clear that all OAuth requirements also applies to OIDC)
|
As we have some renumbering and modifications, then I'll recheck everything and make an update. The requirement is now moved to 51.2.15:
I tried to understand, from where the "replay codes into the authorization response" comes, because for me it does not make too much sense. Potential resource. The "authorization code response" comes from the AS, but in reality, it is a request from AS to the client. Authorization code replay can happen:
Against "replay" the actual defense is that the authorization code is "one time use", and it is covered by 51.2.1. From the defined attack vector to mitigate "authroization code replay" point of view, the PKCE seems offtopic here. The PKCE is against authorization code interception/injection.
This part is talking about ID token and the 'nonce' claim, that is defense against ID token replay attack and it is covered by 51.4.1. From the authorization code replay attack point of view, it seems also offtopic.
This is most likely from here and is also meant against authorization code interception/injection attacks. From the authorization code replay attack point of view, being transaction-specific provides defense against replay, but it then requires, that AS keeps the list of already used PKCE and nonce values (that is actually recommended to do for AS: Authorization servers are encouraged to make a reasonable effort at detecting and preventing the use of constant PKCE challenge or OpenID Connect nonce values.). "transaction-specific" part is not covered as a separate requirement. So, TLDR - the requirement was actually against authorization code interception/injection attacks. The proposal
|
|
Note that it was meant to be the source, from where the confusing wording is coming from. |
The fact that the authorization code is single use indeed prevent against authorization code replay (implemented AS-side).
I think In other words,
I think a similar argument can be made about PKCE. |
See for reference on this topic 4.5.3 from the Oauth Security topics draft. And:
|
I agree that I applied PKCE
|
The first sentence says:
but then:
Shouldn't the first sentence be instead something like:
I would use
I feel we miss the notion that they are bound to some sort of user session as well (i.e. that a transaction is bound to a given user session). See for example OAuth 2.1 draft section 7.9:
Or OAuth Security topics draft 4.5.3.2:
|
All valid points, do you want to propose full requirement as well? |
Just for one more direction how to set up the first part.
The "user session" or "user agent session" need to be written the way that is valid for public clients and confidential clients. Even here, only session is enough, it must be transaction specific - it means you can not use the same value many times for the same session. My intent is to cover the transaction-specific part with the requirement, but the first part, which is used to explain the need for transaction-specific behavior, is a bit too wide and overlaps already with OAuth client CSRF protection (51.3.2) and OIDC client ID token replay protection (51.5.1). |
With these modifications, that would be:
|
Small suggestion here: Verify that the client validates the values (such as the authorization code or ID token) sent to or received by the client to ensure they result from the authorization flow was initiated by the same user agent session and transaction. This requires that client-generated secrets, such as the PKCE 'code_verifier,' 'state,' or OIDC 'nonce,' are not guessable, are specific to the transaction, and are securely bound to both the client and the user agent session in which the transaction was started. |
@jmanico, yes this is much better! Two minor grammar fixes (I think):
|
I explain changes proposed here #2092 (comment)
First change is important, other is more question of a point of view. |
OK that would give us:
Small questions about the wording:
Isn't "authorization flow" and "transaction" redundant here? In which cases should we talk about "authorization flow" or about "transaction"? Another thing which is not very clean is that the main requirement says:
but in the next sentence, the scope of the requirement is expanded into what I think is another consideration:
Another thing which is somewhat weird is:
The client cannot really verify the authorization code directly. It can check the I'm wondering the current propositions are still somewhat weird or at least not very very precise even if they convey the general idea. |
Please propose correction.
Verify that the client accepts values only if (what must be done) > it can be achieved by using not guessable value (how it must be done). Personally I can not see a problem here.
This was my reason to change "validate" to "accept" in #2092 (comment) |
What about something such as:
|
I prefer to develop our previous version. |
For me it seems that one important part ("accept" + "only if" got lost from #2092 (comment)). So updated proposal:
As the 2nd part feels a bit duplication, there is potential for improvements, but if there are no mistakes or errors to point out, I'm ok to go in with that. |
The proposed requirement is valid for OAuth Client and OIDC Client... So the only valid move seems to be to add it into to V51.1 Generic OAuth and OIDC security. |
Update: skip the long issue and jump to #2092 (comment)
Update: the requirement is moved to 51.2.15
Current requirement
51.2.251.2.15: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:
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):
For me the requirement 51.2.15 seems to mix different things. Or did I miss the actual point for the requirement?
The text was updated successfully, but these errors were encountered: