-
-
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
V51 OAuth: Consider adding more general OAuth verifications #1925
Comments
Thank you @TobiasAhnoff for your input! It is a nice "validation round", do we have pointed out proposals and requirements covered in one way or another in the ASVS. A general point of view - OAuth is "logic" based on a list of other technologies, and if some requirement is clearly related to the underlying technology, it should be covered there. proposal 1
Not OAuth specific, covered by section V14.1 Build and Deploy proposal 2
Not OAuth specific, covered by section V3.4 Cookie-based Session Management. One option is to move JWT part away from session management, but this discussion you can follow in the issue #1917 proposal 3
This is something to address. How much detail or level of abstract we can use, is good question. I opened the same topic in #1916 "Discussion/Proposal 2". Probably it makes sense to open a separate issue for this to keep the focus. edit: I opened separate issue: #1964 - if it matches with your problem statement, we can move discussion there, if it does not, then we handle it separately. proposal 4
Not OAuth specific, general JWT. I think we should add
Also note, that the access token does not need to be a JWT. First random response from related search proposal 5
I don't think it is OAuth specific. I'm not sure we have it addressed, need to anaylze chapters: ... but those are in the authentication section. proposal 6
Manually it is anyway doable. How do you verify that or what the requirement gives extra? proposal 7
The refresh_token topic requires more attention. But there is more than one point of view. It is different attitude, is the OAuth used as first-party or 3rd party solution. For the first-party and "session management replacement" (which should be disallowed or not recommended) I would say we can apply V3.3 Session Timeout and V3.8 Session Termination requirements.
For 3rd party solution, we have a requirement currently located in 3.5.1 (it is subject to change via #1917 (comment))
I think we need to be really precise, about what architecture and solution we address with the requirement. One extra issue to cover with the refresh_token topic expiration is that with new refresh_token the AS must keep the proposal 8
It requires quite a strong change to widespread attitude, as with using SPA architecture, often the browser uses directly OAuth service, including token endpoint.
edit: I opened separate issue: #1963 proposal 9
Can you find the matching requirement from [V6 Stored Cryptography](V6 Stored Cryptography)? proposal 10
Yes, with many requirements we took the direction of that style. We could save months of @csfreak92 time :) But now we are here and we go with the list of OAuth-related requirements to v5.0. Before any further action (open mentioned separate issues) I wait "first wave" of feedback :) |
I'll take a look @elarlang and @tghosth. Thanks for the suggestions too @TobiasAhnoff! I'll work on this when I get back home from traveling. :) Pardon the delayed responses. |
Hi @TobiasAhnoff - I would like to hear your comments on my feedback. Then we can open separate issues to discuss some topics further and "close" topics, that are already covered or do not require further discussion. |
I agree that this is could be part of other chapters, but this is perhaps not a Build/Deploy issue, more of a design/architecture decision or best practice like 6.2.2 - "Verify that industry proven or government approved cryptographic algorithms, modes, and libraries are used, instead of custom coded cryptography". You should avoid implementing your own custom code for token-endpoints (AS). It is hard to implement an AS according to BCPs and I believe it would be good if ASVS recommends using hardened secure libraries, products or services (implemented according to BCPs) for OAuth/OIDC solutions.
I agree, probably good to move, in example when using JWTs (or other kinds of self-contained tokens) in a service-to-service scenario (OAuth client credentials) there is no session, just token-based request authentication and authorization. It will be good to continue this in #1917
I agree, we continue this in #1964
I agree, this could be part of 3.5.6
I agree, this is better as a more general verification, it is always important to rotate key material, not just when signing access tokens, but is is good to mention AS as an example of when it is important and sometimes forgotten. 2.10.1 captures this in a way, but it needs some works since it is focused on service authentication.
This is also related to 2.10.1, perhaps part of the #1032 discussion?
Refresh-tokens and sessions etc are need elaboration, I need to get back on this later this week...
I agree, it will be good to continue this in #1963
No, as I understand this is not present in V6. Besides using crypto properly, this is also a least privilege issue, a mistake made when configuring an AS is to use a symmetric signing key (HMACs) without realizing that it will be shared with all RS (who now also will be able to create valid tokens). Depending on the organization, trust boundaries etc this might be a critical issue, leaking a very sensitive key...which might be good to point out in ASVS
I believe it would be good to reference both https://oauth.net/2.1/ and https://datatracker.ietf.org/doc/html/draft-ietf-oauth-browser-based-apps in the OAuth chapter, and https://openid.bitbucket.io/fapi/fapi-2_0-security-profile.html from the OIDC chapter, perhaps not as verifications, but as part of the text at the beginning of the chapters. Since these documents are important for building secure OAuth/OIDC applications and it is important that ASVS verifications are aligned with them. |
For proposal 1 I suggest to not add this aspect as a separate verification, it could be part of suggested verfication for proposal 10 (below) and (in my opinion) it is also covered by verifications 1.2.3 and 1.4.4, which might be clearer if the OAuth chapter also had the same text as chapter 13: For proposal 5 I suggest to make it part of 6.4 and add this new verification: For proposal 9 I suggest to alter it a bit, make that part of 6.4 and add this new verification: For proposal 10 (and 1) I suggest to add a new verification (or maybe address this in text for the new OAuth/OIDC chapter): |
@TobiasAhnoff - if you think those proposals are not covered yet but should be, please open issues for each proposal separately. |
All proposals are now covered by other issues, so this can be closed |
There are of course many candidates, it is hard to summarize all relevant OAuth and OIDC specs in just a small set of verifications for V51 OAuth.
Here are a few suggestions on some more general verifications that, based on my experience with security reviews and penetration tests, would be interesting to discuss (maybe add as individual items with motivation etc, if that would be a good way to contribute):
(edit: added numbers for organizing the feedback)
Perhaps even have a general verification that simply states:
10 Verify recommendations for OAuth2 from https://oauth.net/2.1/ and OIDC FAPI 2.0 from https://openid.net/wg/fapi/specifications/ according to your threat model and business requirements.
Or just recommend to follow this guidance in the in the text before the verifications (not expressing this as a verification).
The text was updated successfully, but these errors were encountered: