search

OWASP / ASVS

Application Security Verification Standard
Creative Commons Attribution Share Alike 4.0 International
2.76k stars 671 forks source link

Add token requirement for intented audience #2363

Closed TobiasAhnoff closed 2 days ago

TobiasAhnoff commented 1 week ago

Add access token requirement to mitigate the risk of accepting tokens that was not intended to be valid for a particular service (API).

This is part of "cleaning up 3.5" see https://github.com/OWASP/ASVS/issues/1917#issuecomment-2466143871

Verify that the access token is intended for the service (API). For JWTs this can be achieved by validating the claims 'aud' and 'typ'. If audience is present, it must be validated to match against an allow list for the given API. If the typ claim is present, it should be equal to 'at-jwt'.

elarlang commented 1 week ago

If this requirement will be added as general requirement for tokens, we need to recheck OAuth-specific requirement:

V51.4.2 Verify that the resource server validates the access token to be made for that resource server (audience), for example by checking the 'aud' claim from the access token to be an expected value.

elarlang commented 1 week ago

To have an general requirement, we should not limit it to access tokens. It is required to check the aud for other tokens as well, at the moment the proposal seems unnecessary limitation. I think the outcome is part of spliting 3.5.6 to requirements "per task" https://github.com/OWASP/ASVS/issues/1967#issuecomment-2351456688.

elarlang commented 1 week ago

Are the title and proposal in conflict? scope vs audience?

TobiasAhnoff commented 1 week ago

Yes, a better title would be "Add requirement to mitigate the risk of accepting tokens that was not intended to be valid for a particular service" and rewrite in a more general way, maybe like this

Verify that the token is intended for the service. For JWTs this can be achieved by validating the claims 'aud' and 'typ'. If audience is present, it must be validated to match against an allow list for the given API. If the typ claim is present and it is an access token, it should be equal to 'at-jwt'.

elarlang commented 1 week ago

I think the typ goes to the "intended usage" field and it is a separate topic than intended audience/service.

ryarmst commented 1 week ago

Proposed refinement:

Verify that the token is intended for the service. For JWTs, this can be achieved by validating the 'aud' claim against an allow list for the service.

ryarmst commented 1 week ago

Actually, thinking about this further, and in relation to #1967, could we not also wrap up 'iss' into this as a service could have a relationship with an issuer such that all tokens from the issuer are valid for the service?

randomstuff commented 1 week ago

such that all tokens from the issuer are valid for the service?

(If I understand what you are saying,) that should be frowned upon in general because it makes way for token abuse (from one service to another, etc.).

The aud claims could have a per-iss meaning however.

elarlang commented 6 days ago

'iss' is a separate topic - who made ('iss') vs for what service it is made ('aud').

Problem to solve - tokens from one issuer made for different services must not be cross-usable for each other (if it not clearly intended so by the 'aud' definition).

Based on the latest proposal https://github.com/OWASP/ASVS/issues/2363#issuecomment-2479991939

Proposed change:

For both cases, I think the word "service" may be misinterpreted, especially if the context is not too well known.

Verify that the token is intended for the service (audience). For JWTs, this can be achieved by validating the 'aud' claim against an allowlist defined in the service.

elarlang commented 6 days ago

If we are happy with the general requirement, it is ready for PR.

We have similar requirement for OAuth scenario, this deduplication will be handled in https://github.com/OWASP/ASVS/issues/2182#issuecomment-2480476484.

TobiasAhnoff commented 6 days ago

I think the typ goes to the "intended usage" field and it is a separate topic than intended audience/service.

Should we add a new general requirement for "intended usage"?

We also have this in 51.1.1 where it is OIDC/OAuth specific for different token types (e g OIDC ID Tokens), but I think it is good to have a general requirement for "intended usage" since it is not just important for OAuth/OIDC flows.

elarlang commented 6 days ago

Should we add a new general requirement for "intended usage"?

Please one problem per issue, I opened a separate issue for that: https://github.com/OWASP/ASVS/issues/2379

randomstuff commented 5 days ago

The wording of the current proposition is not correct: we don't want to "verify that the token is intended". Instead, we want to "verify that the service checks that the token is intended".

So it should looks somewhat like:

Verify that the service receiving a token as a proof of authentication or authorization makes sure that the token is intended for the service (audience) before accepting it. For JWTs, this can be achieved by validating the 'aud' claim against an allowlist defined in the service.

elarlang commented 4 days ago

I agree with the problem statement, but not with the new proposal (we need to have general requirement into V3.5). Updated proposal:

Verify that the service receiving a token validates the token to be intended for the service (audience) before accepting the token's contents. For JWTs, this can be achieved by validating the 'aud' claim against an allowlist defined in the service.

elarlang commented 4 days ago

If no further comments and feedback, I'll PR it tomorrow.

tghosth commented 2 days ago

My proposal

# Description L1 L2 L3 CWE
3.5.8 [ADDED] Verify that the service only accepts tokens which are intended for use with that service. For JWTs, this can be achieved by validating the 'aud' claim against an allowlist defined in the service. ✓ ✓ ✓

The first sentence was a little clunky using the word token 3 times. I have tried to make it smoother, I don't think it changes the meaning of the first sentence when supported by the 2nd sentence.

elarlang commented 2 days ago

If would like to get the "audience" back (as it was proposed and explained in https://github.com/OWASP/ASVS/issues/2363#issuecomment-2480455586) - it is called "audience restriction" and it makes it clearer why the requirement exists and what is the main goal.

Waiting for #2182 to be in sync.

tghosth commented 2 days ago

So how about:

# Description L1 L2 L3 CWE
3.5.8 [ADDED] Verify that the service only accepts tokens which are intended for use with that service (usually defined as the intended 'audience'). For JWTs, this can be achieved by validating the 'aud' claim against an allowlist defined in the service. ✓ ✓ ✓
elarlang commented 2 days ago

I'm going to use just "(audience)". "Usually defined" feels like some fact based on stats, Using ' feels like some technical term.