-
-
Notifications
You must be signed in to change notification settings - Fork 668
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
V51 OAuth: Add code and PKCE related verifications #2041
Comments
The "V51.1 Authorization Server" as it stands at the moment. This is just for information and we should discuss only PKCE related requirements in this issue.
What is the argumentation for 10 minutes? Is it supported by any spec? Do we have any matching requirement in "V2.8 One-Time Verifier" or is it that OAuth specific thing that is worth a separate requirement?
If should be maybe more clear wording, is it "verify that always grant type code is used" or "verify, that if grant type code is used".
It is / should replace current 51.1.5
It is / should replace current 51.1.3 |
Yes, it is from https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-11#section-4.1.2 and also https://openid.bitbucket.io/fapi/fapi-2_0-security-profile.html#section-5.3.2.2 notes that codes should have a short lifetime.
Maybe not, this is addressed in 2.8.1 and 2.8.4, but for readers of ASVS it might not be obvious to make the connection from OAuth codes to OTP? In a way this is done by the suggested scope text "Please read this chapter in combination with all other chapters...", so maybe just keep tha OAuth specific part "Verify that codes are valid for at most 10 minutes."?
Yes, maybe "Verify that if grant type 'code' is used, it is always used together with PKCE and requires S256 as the code challenge method."
Yes
Yes |
I also ask for opinions from everyone to combine all 3 PKCE-related requirements into one requirement. |
I would like to suggest we drop the discussion on the "nonce" parameter or the "state" parameter and only focus on PKCE for CSRF protection. |
I agree with @elarlang and think 51.1.1-51.1.3 can all be merged into one concise requirement like:
... and then drop 51.1.2 and 51.1.3 This kind of simplicity will have a longer "life" in ASVS. |
nonce is OIDC and ID-Token topic, 51.1.2 will be moved. ( #2002 ) |
authorization code expiration Proposed:
How to sneak in the message actually 30sec is usually more than enough... PKCE My attempt to combine 3 proposed PKCE requirements into one requirement, input for collecting feedback and for further wordsmithing:
|
That is … dense :) I think I would better have several more granular verifications. (but maybe after some ironing-out …) |
Should this allow the usage of Also, should there be a discussion about private-Use URIs? (Is this in scope?) |
For this discussion I re-opened related issue: #1965 (comment) |
But how to split them the way that they separately make sense and does not duplicate each other. I would just ask to implement PKCE correctly without providing functionality testing-guide against authorization server. Basically the idea is to ask:
At the moment it was a bit more technical. |
I agree that it is hard to put all that in one verification. I think the important thing for ASVS would be to make it clear that PKCE should be used and if motivated also address important hardening requirements. In my experience authorization servers which supports PKCE does that correctly and clients using PKCE also do that well (in general), the issue is that some AS might not support PKCE and some clients are not aware that PKCE needs to "enabled" (not all client frameworks have PKCE on by default) So, perhaps this is enough?
or even just
You could argue that today it is unlikely to use PKCE in the wrong way, given updated client frameworks and well-known authorization servers, but maybe there is an security issue with e g using weak code challenge methods or down-grade attacks that is important for ASVS to address? If so, I suggest having separate verifications to make the easy to read and apply. |
I would say PKCE-related misconfiguration is not that rare - widespread classics:
|
I agree with @elarlang here |
I agree with @elarlang, should we then have one or split? I think I agree with @randomstuff and prefer to have separate, but one will work as well |
One more point of view - from the ASVS perspective, should we care that PKCE is there or should we care, that authorization code interception is not possible. I think in general we should have focused on mitigating and pointing out, that "Verify you have defense against this kind of vulnerability" instead of "Verify that you have this configuration set to this value" without saying, why it is needed. If we focus on principles, it is more "time-proof", especially in the OAuth topic, where there are many new RFCs per year that all deprecate something from previous ones and it's hard to track what is the most up-to-date recommendation today and are different "best practices recommending the same thing". Is it one or two requirements, I don't care - it is more important, that those are logical, logically separated and make sense as independent requirements. |
@elarlang do I understand you correctly that we should have something like "Verify that code injection attacks are mitigated by always using the Code flow together with PKCE. For authorization request, authorization server requires valid code_challenge value and accepts code_challenge_method value S256 (plain is not allowed). For token request it requires code_verifier with value calculated from code_challenge. |
Yes, I think it nicely covers all. It is a bit long, but it does the job providing the reason, principle and also technical guidance. Personally I don't know what could be the logical way to split it to many requirements, as the main goal is still protection against authorization code interception. Although one can say the likelihood is taken down when BFF and confidential clients are in use. |
First part:
Any improvements? |
The other requirement. Initially proposed:
I propose update:
|
Some minor tweaks. This looks more natural this way (but is it?):
Nitpick: it's actually the other way around code_challenge is calculated from code_verifier :) Do we need include this much details, though? What about something like: “Verify that, when the code flow is used, the authorization server requires the use of PKCE to mitigate authorization code interception attacks. The plain code_challenge_method must not be accepted.“ |
The reason for this long requirement is to address the PKCE downgrade attack. |
Wouldn't hat be covered by: “Verify that, when the code flow is used, the authorization server requires the use of PKCE to mitigate authorization code interception attacks. The "plain" code_challenge_method must not be accepted.“ Moreover, this should be stated explicitly? (That it should not be vulnerable t PKCE downgrade attacks? |
The downgrade can be also the mistake on the AS side in token request, if it accepts the code_challenge value as code_verifier value. Also when code_challenge was not sent in the authorization request but code_verifier was sent in the token request.
Or we should keep and modify the separate requirement as we have/had:
|
All of this is not possible as long as the authorization server ensures that proper code_challenge and code_challenge_method (not plain) are included the authorization request (?). |
It does not cover the code_verifier verification in the token request. If it is accepted as code_challenge value or just not validated correctly. Anyway, it goes a bit to technical checklist side and it is up to us to choose the level of details to cover to goals we want to achieve. |
Note, that these requirement are now in the document:
Let's try to solve in this issue the requirement text for the PKCE requirement (added as 51.2.3) and I open a separate issue (#2090) for the authorization code requirement. |
51.2.3 is a bit long, perhaps split like this?
|
Some tweaks: "Verify that, if the code grant is used, the authorization server mitigates authorization code interception attacks by requiring a valid PKCE "code_challenge" parameter in the authorization request, with the code_challenge_method value S256 or better."
Some tweaks: "Verify that Authorization Servers are mitigating PKCE downgrade attacks by ensuring that all token requests using the authorization code grant require validation of the "code_verifier" parameter." or is it simply "Verify that the Authorization Servers expect a code verifier the token request of the code grant and validate it."? |
I like the tweaks from @randomstuff
and
Having "are mitigating PKCE downgrade attacks" is from original text from @csfreak92 and based on general input from @elarlang, that ASVS requirements preferably should be motivated with what kind of attack they mitigates. I think it is good, even if there are many requirements that currently just say what to do (not why). |
I think splitting PKCE can be better and I don't think it's correct way. I'll try to come up with a proposal. Maybe I just need to think more about it and then I can agree with the proposal :)
We have had quite many issues and most of them opened by ourselves because we can not understand, why some requirements have been made in the past in previous versions. So now I try to avoid this kind of issues in the future, so that the requirement author can understand why it was made, but others need some translation. |
Is this requirement independently incorrect, as the defense against authorization code interception is done in the token request? Based on my knowledge, currently there are 2 options for the code_challenge_method - those are plain and S256, so there is no "or better" available and it can be misleading. In short, I have not found a good way to split this requirement. If it is too long, we can just cut the details. |
"2 options" is basically true, but the RFC opens up for other methods as well (even if not commonly implemented today) So "or better" aligns with the RFC, but it could also be removed it there are no known implementations today, perhaps @randomstuff knows more about this? @elarlang Maybe a misunderstanding, but I think there are examples of requirements that depends on other requirements, like requiring mTLS (9.3.3) and strong ciphers separately (9.4.1). Without properly configured TLS (allowing downgrade or even unencrypted traffic), MTLS will be weak. Just like PKCE wont protect against code injection if downgrade attacks are possible. I might be wrong, but I think it works having the two PKCE requirements separated (but close together). Having them in one will works as well, in my opinion either will work. |
The point of my "S256 or better" comment was just to try to be forward looking in case new methods would be added in the future, similar to when we have “Verify that only the latest recommended versions of the TLS protocol are enabled, such as TLS 1.2 and TLS 1.3. The latest version of the TLS protocol should be the preferred option.” instead of “Use TLS 1.2 or 1.3”. AFAIU, there is no plan to do add new code challenge method though. |
@TobiasAhnoff - 2 separate requirements must be valid independently, one can be extra layer of security on top of another, but my question was, is the proposed requirement for PKCE valid independently, as it only describes validation during authorization request, but the defense must take in place in token request. @randomstuff - the point and idea behind that is understandable, I think in this case "negative requirement" works well and we can just say no to the "plain" value. If this is going to change in the future, it can be fixed with "patch release of ASVS", such as 5.0.1. The meaning for the requirement stays the same. |
Maybe like this? 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. |
Update (via PR #2122)
|
The following verifications are suggested to address code+PKCE, note that the last three are from #1971 where the second has a minor modification, but are included here as well.
V51.2 Authorization Server
Verify that codes are can only be used once and are valid for at most 10 minutes. (L1,L2,L3)
Verify that grant type 'code' is always used together with PKCE and requires S256 as the code challenge method. (L1,L2,L3)
Verify that if a Client sends a valid PKCE "code_challenge" parameter in the authorization request, the Authorization Server enforces the correct usage of "code_verifier" at the token endpoint. (L1,L2,L3)
Verify that Authorization Servers are mitigating PKCE Downgrade Attacks by ensuring a token request containing a "code_verifier" parameter is accepted only if a "code_challenge" parameter was present in the authorization request. (L1,L2,L3)
The text was updated successfully, but these errors were encountered: