Open csfreak92 opened 3 years ago
Hi, @TobiasAhnoff - I agree with your point of view and (y) for the attitude to understand the situation first.
The situation at the moment:
* The entire OAuth paragraph is the first draft. It is published to get feedback and based on that to validate and improve requirements. * If ASVS already has requirements to address certain issues, such as validating JWT, then those requirements are not duplicated to the OAuth section.
The action plan here can be - if you feel, that the OAuth paragraph is missing some requirement, then just point it out. If it is covered somewhere else, we can then say it. For me it's probably faster to say is it covered than for you to investigate the entire ASVS for each potential proposal.
👍 I will structure my feedback and add some issues during this week, some of it might be covered in other parts of ASVS, but faster if you note if it is covered or not, thank you!
I have added two Github issues, hope this will be a good way to provide feedback on the OAuth chapter https://github.com/OWASP/ASVS/issues/1924 https://github.com/OWASP/ASVS/issues/1925
I have added two Github issues, hope this will be a good way to provide feedback on the OAuth chapter #1924 #1925
Thank you, I'll take a look in few days.
@csfreak92 would be good to get your feedback as well!
Hi @tghosth, will get back to this thread once I got back home from traveling. For sure will review all the feedback from the community for the OAuth 2.0 section. Cheers!
Updates from my end evaluating @jmanico's comments with my comments. Will push a PR with the OAuth 2.1 draft together with updates and notes from these comments :) @elarlang, @TobiasAhnoff, @jsherm-fwdsec, @tghosth.
I had some of my OAuth2 friends review this, this morning. Here are a few suggestions:
PKCE Requirement: Originally, PKCE was advised for public clients. Now, it's a best practice for all OAuth clients, including confidential clients, to prevent authorization code interception attacks. This recommendation needs clear emphasis. ---> [Covered with 51.2.2, but adding this for more emphasis]
Refresh Token Strategy: Implement rotating and expiring refresh tokens to avoid long-term misuse. This should be done in conjunction with sender-constraining mechanisms for enhanced security. ---> [Covered with 51.1.4]
Avoid Resource Owner Password Credentials Grant: This grant type is not recommended due to security concerns. Instead, use the authorization code grant with PKCE. ---> [Covered and added to the ### Resource Owner Password Credentials Grant paragraph explanation at the bottom]
Consistent Terminology: Use "Authorization Server" and "Resource Server" consistently throughout the document to avoid confusion, especially for newcomers. ---> [Covered in the whole chapter unless I am missing something?? Can someone point anything that was not consistent so that I can fix it with an updated PR :) ]
Clarify OAuth vs. OpenID Connect: Emphasize that OAuth 2.0 focuses on authorization delegation, while OpenID Connect deals with authentication federation. Expand on this to help readers understand their distinct purposes. ---> [This is ongoing work with other folks during the meeting regarding OAuth 2.0 and OpenID Connect. Noted and will be kept in mind]
Prevent Authorization Server Mix-Up Attacks: Include up-to-date strategies for mitigating mix-up attacks in environments with multiple Authorization Servers. Detail the use of the "iss" parameter and recommend additional checks like verifying the "aud" claim. ---> [Covered with 51.2.1 and reference links at the bottom. Noted about the "aud" claim and will keep it in mind in the updated PR that is on the works with the other folks during the meeting :) ]
Detailed Scope and Audience Implementation: Provide concrete examples of how to implement and validate access token restrictions based on scope and audience to offer actionable guidance. ---> [This is ongoing work with other folks during the meeting regarding OAuth 2.0 and OpenID Connect. Noted and will be kept in mind for creating a further concrete example and guidance for this in the updated PR. :) ]
Hey all 👋 Great work on this piece. As I have interest and expertise in OAuth and OIDC, I'll give this a look in the coming week(s)
My first question if you don't mind, why is everything L3? Is there a basis for how we are setting them? I can see several of those can be split between L1->3 based on priority and conformance with the protocols at hand and profile required.
Looking forward to better understand the work set here behind this :)
Hey @ThunderSon - we are waiting for your input :) As it is valid for every opened issue, but on the topic, all OAuth/OIDC related issues are with label https://github.com/OWASP/ASVS/labels/V51
Levels - I can not see L3, everything is L1 at the moment. But anyhow, we still don't have levels defined and time should not be wasted at the moment for the discussion of levels. In the big picture, the same is for chapter texts - let's get necessary requirements in first, rest of it comes later (as a result of requirements and how those are divided into chapters).
Update from @jmanico's comments on OAuth 2.1:
Should we really be limited with OAuth 2.0 or should take direction to OAuth 2.1:
For sure. Here are the main differences between OAuth 2.0 and 2.1 and topics we should cover to address 2.1.
PKCE is required for all OAuth clients using the authorization code flow ---> [Covered and added some verbiage for OAuth 2.1 coverage in 51.2.2]
Redirect URIs must be compared using exact string-matching ---> [Added new requirement numbers 51.2.5 and 51.1.7 to cover for this ]
ADDED on my PR #1971 51.2.5 Verify that Clients do not expose URLs that forward the user's browser to arbitrary URIs obtained from a query parameter ("open redirectors") and any URI redirects must be compared using exact string-matching.
51.1.7 Verify that the Authorization Server does not expose URLs that forward the user's browser to arbitrary URIs obtained from a query parameter ("open redirectors") which can enable exfiltration of authorization codes and access tokens. Redirect URIs must be compared using exact string-matching.
The Implicit grant (responsetype=token) is omitted from this specification ---> [Should we add some language to say this is a big no, no thing to use? In my initial pull requests, it was pointed out by Josh that we should have language as "use this instead of avoid using this..." I can always add that to my pull request, but just want to make sure we are all on the same page. What are everyone's thoughts about this? ]_
The Resource Owner Password Credentials grant is omitted from this specification ---> [Covered by adding some sentence at the end of the ### Resource Owner Password Credentials Grant]
Bearer token usage omits the use of bearer tokens in the query string of URIs ---> [NEED HELP to research on how this helps and what it prevents... I am not sure about this part]
Refresh tokens for public clients must either be sender-constrained or one-time use ---> [Covered already with 51.1.4]
Ping @elarlang, @TobiasAhnoff, @tghosth and @jsherm-fwdsec
As related PR was 2 months old, outdated and causing conflicts, I closed it.
Proposed change for "Resource Owner Password Credentials Grant":
Aside from this grant type can leak credentials in more places than just the Authorization Server, adapting the Resource Owner password credentials grant to two-factor authentication, authentication with cryptographic credentials (e.g. WebCrypto, WebAuthn) and authentication processes that require multiple steps can be hard or impossible. This grant type is not recommended in general due to security concerns. Instead, use the authorization code grant with PKCE. This grant type is omitted from OAuth 2.1 specification.
Please let's find agreement on the issues before making PR and leaving it hanging there. It wastes quite some time to re-work it again and again to understand, what is the latest state.
Ed note: This issue was originally called "v3.5 Token-based Session Management addition"
Ed note 2: If you are just joining us, the draft chapter is currently here: V51 OAuth 2.0 Protocol
Before you review it, read through this issue thread to collect the context and then feel free to open separate issues if you have specific questions or suggestions.
Original text starts here:
I think we need to add a control that validates the incoming HTTP request in an API endpoint which issues new access and refresh tokens. Usually this is with the refresh endpoint, but in some weirdly built applications, there were some implementations that every response would contain the new tokens that could be used for the next authenticated and authorized calls to the API. The requirement would be for the refresh token endpoints to have a validation of the requests, URL and request body must be validated before issuing the new set of tokens.
The rationale about this thought came to me when I was testing a few recent applications that either have refresh endpoints or a few ones that do not have refresh endpoints but the next authenticated tokens are coming from the HTTP responses. This is a bit of an old way of doing it as I understood and a bit uncommon. However, I noticed a pattern of accepting the request even if it was malformed or missing the object id or any reference for a resource (e.g. http://www.example.com/12345 but I the request was http://www.example.com/ralph; second URL is invalid and should be rejected). The application returns no valuable data except the new set of tokens which can be used for the succeeding calls to the API. I think it was a bad design, but for defense in depth purposes and preventing an attacker for prolonging their usage of the compromised account (which would be cut short had the application been following proper OIDC flow), the application should check for incoming HTTP requests to be correct in format, parameters, etc. and if anything is wrong then reject the request and never send back new tokens.
I would place this under v3.5 Token-based Session Management. I don't know whether this is still an acceptable thing to worry about and whether the threat model is too low to bother about this control, but I think it just seems like a basic thing that developers should have placed some sort of validation for requests before allowing the scenario described to happen. If this is too low risk, then we could scrap it.