Skip to content
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

proposal/discussion: OAuth - (for 1st party usage) only used (by the client) communication options must be allowed by authorization server #1964

Closed
elarlang opened this issue May 19, 2024 · 27 comments
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 6) PR awaiting review V51 Group issues related to OAuth _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

elarlang commented May 19, 2024

UPDATE: The focus for the issue is set to only handle the response_mode parameter validation, jump to #1964 (comment)


spin-off from #1916 "Discussion/Proposal 2"

For a situation, where OAuth is used as a "first-party" authorization solution and the application needs one and only way how it communicates with the authorization server, then for the OAuth client must be configured and the Authorization Server must validate, that: only the expected values are allowed, that is implemented by the client:

  • response_type
  • response_mode
  • scope - here, support for the offline_access may be worth special mention

edit: scope in mind - authorization request from OAuth Client to OAuth Authorization Server

--
Feedback from @tghosth in #1916 (comment)

If this is a simple requirement then it sounds sensible as long as it is not too detailed.

--
Overlap by recommendation from @TobiasAhnoff in #1925

3 Verify that client configuration asserts least privilege (e g scopes and flows)

Although response_type and response_mode are not directly privilege, those are all related with allowed flow.

@elarlang
Copy link
Collaborator Author

3 Verify that client configuration asserts least privilege (e g scopes and flows)

I was thinking more about it - I think it is another topic compared which I opened, so probably we have 2 different vectors to cover.

  • original issue - (at least for 1st party solution) authorization server should accept only those parameters and values for communication, that are needed by the (one and only) OAuth Client - response_mode, response_type, scope
  • additional issue, by Tobias - scope definition on the Authorization Server side for the OAuth Client should not give more permissions than needed for given client and actions.

ping @TobiasAhnoff - please confirm my understanding

@jmanico
Copy link
Member

jmanico commented May 21, 2024

Based on the typical OAuth registration and "user" workflow, I would add one more level here.

  1. Server allows a series of scopes to start with.
  2. Client registers (once) with a limited list of these scopes (or all that the server offers).
  3. A "user" goes through the OAuth flow and may only use the scopes that the server approved of during registration time and NOT NECESSARILY the complete lists of scopes that server allows. Again, only the scope the server approved of during client registration should be allowed during user flows, and I feel we need to clarify that level of strictness.

So I'd suggest two requirements around scopes since there are granted at very different parts of the OAuth lifecycle.

  1. Only let a client register for scopes that the server allows
  2. Only let a "user" use scopes the authorization server approved of during client-registration

@elarlang
Copy link
Collaborator Author

I'm not sure we are talking about the same thing here (user registration vs configured client), I modified initial issue to be more clear and added:

edit: scope in mind - authorization request from OAuth Client to OAuth Authorization Server

For the 1st party solution (if there is no "discovery" in place), when an application uses the SSO service from the same organization, there should be no client registration available for being a OAuth Client. It is actually worth a separate requirement.

@tghosth tghosth added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet _5.0 - prep This needs to be addressed to prepare 5.0 labels May 23, 2024
@TobiasAhnoff
Copy link

TobiasAhnoff commented Jul 17, 2024

A suggestion is to add something like this:

L1,L2,L3: Verify that allowed scopes for each client asserts least privilege (especially if RS authorization is based on just a valid token and scopes, not using any additional token claims like identity of the caller or group membership)

L2,L3: Verify that all clients are configured to only allow the required flow. For system integrations only allow grant type 'client-credentials' and for user interaction applications only allow grant type 'code' (Implicit and ROPC should not be supported, other additional flows like Device Authorization Grant might be supported if needed). Avoid reusing clients for different purposes (allowing e g both code and client-credentials).

L1,L2,L3: Verify that grant type 'code' is always used together with PKCE

L3: "Verify that all clients are confidential" (L1-L2 could allow for public clients)

L3: "Verify that all clients are configured to use strong client authentication, mTLS or private-key-jwt" (L1-L2 could also use client-secret, note that verification 9.3.3 mandates mTLS, but private-key-jwt is also regarded sufficient by e g FAPI)

L3: Verify that PAR with client authentication is used (for L1-L2 PAR is optional and could be used without client authentication)

L3: Verify that sender-constrained access-tokens are used either using mTLS or DPoP (note that for L1 and L2 this does not require client authentication if used by public clients)

L3: Verify that refresh-tokens are sender-constrained (by requiring client authentication for all token requests, while L1-L2 could allow one-time mitigation strategy for public clients, this is especially important if offline_access is granted, allowing long lived tokens not attached to active user sessions)

@elarlang
Copy link
Collaborator Author

Scope and response_type concerns are or will be covered by other requirements.

Here there is need to build a requirement to say, that only allowed response_mode can be accepted by the authorization server (for the authorization request).

@elarlang elarlang removed the 2) Awaiting response Awaiting a response from the original poster label Sep 30, 2024
@elarlang
Copy link
Collaborator Author

Using the style from 51.2.5 it should sounds like:

Verify that for a given client, the authorization server only allows 'response_mode' value that this client needs to use.

@elarlang elarlang added the 4) proposal for review Issue contains clear proposal for add/change something label Sep 30, 2024
@TobiasAhnoff
Copy link

I agree that 'response_mode' could be restricted, but is it possible (per client)? In my experience this not supported by some common OAuth/OIDC services, but I might be wrong...can´t find good reference documentation on this?

@elarlang
Copy link
Collaborator Author

elarlang commented Oct 3, 2024

When I was discussing the topic with "OAuth 2.0 for Browser-Based Applications" they thought it was a more general issue than browser-based apps and should belong to more general spec. Also, it should be solved when JAR/PAR is used for backend (confidential?) OAuth clients.

In case it is not suitable for 3rd party solutions and for discovery mode, then we should limit the requirement for 1st party usage. Although OAuth is not even meant to be used like that.

ping @randomstuff

@elarlang elarlang removed the 4) proposal for review Issue contains clear proposal for add/change something label Oct 3, 2024
@randomstuff
Copy link

I agree that 'response_mode' could be restricted, but is it possible (per client)?

I'm not sure the security benefit is that great. I don't believe an attacker would gain a significant advantage in changing the response_mode. If that's true and as I don't see much support for checking this (i.e. there is no associated client registration metadata), I would say we should not include this.

response_modes=form_post is somewhat safer because the response values won't be visible in the logs/history. It would probably be better for the client to use it when possible (i.e. when this is supported by the AS and, I guess, when the RP is browser-based) but I'm not sure that a requirement about this should be included.

@elarlang
Copy link
Collaborator Author

elarlang commented Oct 4, 2024

If redirect_uri is not matched with string-match and is (as an example) validated on origin level, it is enough to have one open-redirect, xss, referrer-leakage (for example you can set HTML attribute to an image, to have the effect) etc on the site, to steal the authorization code.

If redirect_uri is validated correctly using string-match, but if you can change response_mode, then the client does not read the authorization code from the response and leaves it to be read to an attacker - there are still XSS or referrer-leakage vectors to read it, or it may leak from URL to logs and so on.

I have proved stealing the authorization code many times during pentests and often I can say, that it could be not possible or it is more complicated if I (as an attacker) can not use other response_mode value than it is used by default.

@elarlang elarlang added the 4) proposal for review Issue contains clear proposal for add/change something label Oct 5, 2024
@TobiasAhnoff
Copy link

The suggested verification works, even if I think it is hard to do this with some AS services, but maybe it should be noted that there are other mitigations, perhaps?

Verify that for a given client, the authorization server only allows 'response_mode' value that this client needs to use. Or applies other mitigations like PAR or a confidential client.

@elarlang
Copy link
Collaborator Author

elarlang commented Oct 6, 2024

I think a confidential client is not that helpful here, as the response_mode value must be limited for the authorization request, but the confidential client comes into play with the token request. I don't know enough about PAR to comment on that.

@elarlang
Copy link
Collaborator Author

elarlang commented Oct 7, 2024

The suggested verification works, even if I think it is hard to do this with some AS services

Maybe AS providers should implement some features, not we need to change requirement to be more flexible? :)

Although I agree that the risk-level is different when confidential client or/or client authentication is used, but it's the same with basically everything (public client vs confidential client). I think serving the idea and principle is enough at the moment:

Verify that for a given client, the authorization server only allows 'response_mode' value that this client needs to use.

@randomstuff
Copy link

randomstuff commented Oct 7, 2024

Yes, I agree that PAR could be used to prevent tampering with this parameter (as well as other parameters) (as long as client authentication is required for the PAR endpoint i.e. for confidential clients).

@elarlang
Copy link
Collaborator Author

elarlang commented Oct 7, 2024

The problem is likely used if the OAuth client is public, but PAR comes into the play from level 3. It is suitable as "or" condition.

@TobiasAhnoff
Copy link

I think a confidential client is not that helpful here

For code flow it is (implicit should not be used), since the code can only be used by the confidential client who has the client-secret (which is a mitigation for the risk of the code leaking in a query-string or post-body).

The problem is likely used if the OAuth client is public, but PAR comes into the play from level 3. It is suitable as "or" condition.

I agree, is this good or should we remove/add anything?

Verify that for a given client, the authorization server only allows 'response_mode' value that this client needs to use. Or applies other mitigations like PAR or a confidential client.

@elarlang
Copy link
Collaborator Author

elarlang commented Oct 8, 2024

For code flow it is (implicit should not be used), since the code can only be used by the confidential client who has the client-secret (which is a mitigation for the risk of the code leaking in a query-string or post-body).

As I said, it comes into play with the token request, but we need to defend also the authorization request. If an attacker was able to get the authorization code, then in case there is CSRF on the client side (51.3.5) and PKCE is not used, you can force the client to make the request, without having direct access to the token endpoint.

@TobiasAhnoff
Copy link

I am not sure I understand...are you suggesting that the requirement should be just (this applies to all kinds of clients and flows)

Verify that for a given client, the authorization server only allows 'response_mode' value that this client needs to use.

or should we mention PAR (since it "defends the authorization request") and have

Verify that for a given client, the authorization server only allows 'response_mode' value that this client needs to use. Or applies other mitigations like PAR.

And remove the confidential client part since this is not relevant? Or is this only an issue for public clients so it could be like this?

Verify that for a given public client, the authorization server only allows 'response_mode' value that this client needs to use. Or applies other mitigations like PAR.

@elarlang elarlang added 6) PR awaiting review and removed 4) proposal for review Issue contains clear proposal for add/change something labels Oct 12, 2024
elarlang pushed a commit to elarlang/ASVS that referenced this issue Oct 12, 2024
@elarlang
Copy link
Collaborator Author

Goes in via #2140

# Description L1 L2 L3
51.2.12 [ADDED] Verify that for a given client, the authorization server only allows the 'response_mode' value that this client needs to use.

@elarlang
Copy link
Collaborator Author

I added the "keep it simple" version. If it requires some further improvement, please let me know. Otherwise we are done here.

@TobiasAhnoff
Copy link

I added the "keep it simple" version.

I think it is a good thing to have. But, in my experience it is often not possible to do this (since authorization servers in general does not support configuration of allowed response_modes per client), so my concern is that we might add a requirement for all levels that not many implementations can do...is that ok or should requirements in ASVS for L1 and L2 be required to also have "good" implementation support? If this is ok, then I think 51.2.12 is good.

@elarlang
Copy link
Collaborator Author

I watched it from my pen-test experience and I can say, if the requirement is satisfied, it gives enough security benefits to be a worthy requirement.

So half of your concern can be fixed with a feature request to Keycloak? :)

@randomstuff
Copy link

randomstuff commented Oct 14, 2024

Two things kind-of bothers me here:

  • I'm afraid this often won't be easily implementable without begging $framework devs, which means this requirement might be ignored and if too many requirements are ignored other will be ignored as well.
  • It is currently not even possible to implement this using dynamic client registration because there is no response_modes_supported client registration metadata.

What is possible with dynamic client registration is that the client can use require_pushed_authorization_requests so that the (confidential) client can make sure that the proper response_mode was used.

So could this be reworded as something such as (which focused on the goal):

[ADDED] Verify that for a given client, an attacker may not tamper with the 'response_mode' parameter.

This could be achieved:

  • by having the authentication server validate this value against the expected values;
  • or by using PAR;
  • or by using JAR.

@TobiasAhnoff
Copy link

@elarlang I agree that it is a worthy requirement and I like rewording with focus on the goal as @randomstuff suggested, perhaps add how this can be achieved to the requirement

Verify that for a given client, an attacker may not tamper with the 'response_mode' parameter. In example by having the authorization server validate this value against the expected values or by using PAR or JAR.

@elarlang
Copy link
Collaborator Author

Personally, I don't care how it is achieved. One may also put a WAF rule and block all other requests that are not with response_mode=expected.

I'll update the requirement with next area51 PR.

@elarlang elarlang added 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR and removed Will be closed if no response/opposite arguments labels Oct 15, 2024
@elarlang
Copy link
Collaborator Author

elarlang commented Oct 16, 2024

Via #2157

# Description L1 L2 L3
51.2.12 [ADDED] Verify that for a given client, an attacker may not tamper with the 'response_mode' parameter, for example by having the authorization server validate this value against the expected values or by using pushed authorization request (PAR) or JWT-secured authorization request (JAR).

elarlang pushed a commit to elarlang/ASVS that referenced this issue Oct 16, 2024
@elarlang elarlang added 6) PR awaiting review and removed 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Oct 16, 2024
@elarlang
Copy link
Collaborator Author

Done with this update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 6) PR awaiting review V51 Group issues related to OAuth _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

No branches or pull requests

6 participants