OWASP / ASVS

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

Add requirement about usage of claims other than subject and issuer as an identifier for OpenID Connect #1826

Closed jsherm-fwdsec closed 1 month ago

jsherm-fwdsec commented 10 months ago

Usage of claims other than the subject and issuer identifier to uniquely identify an end user in OpenID Connect is non-compliant with the framework. As per a recent Microsoft report, there is a false identifier anti-pattern being followed and exploited in the wild resulting in account takeover.

To keep it simple, the sub (subject) and iss (issuer) claims, when used together, are considered to be a unique primary identifier for OIDC, as the uniqueness across users is guaranteed. Other claims such as email, username or phone number should not be used, as they can change over time and an attacker can falsify these claims.

I'm interested in contributing and happy to create a PR for this, as well as adding other OIDC requirements. Perhaps this is something I can help with as part of the OAuth2 changes being discussed here ?

jmanico commented 10 months ago

Super excited about this. A dedicated OIDC section would be fantastic!

elarlang commented 10 months ago

Yes, that is correct - we have OAuth PR to be discussed and merged and that is why we don't have sepparate issues per requirement so far on the topic.

The branch for that is located at https://github.com/OWASP/ASVS/blob/master-v51-oauth/5.0/en/0x51-V51-OAuth2.md

If you have in mind some wording for the proposed requirement, please share it that (we can fine-tune and help with that) and matching section from the OAuth category.

ImanSharaf commented 10 months ago

@jsherm-fwdsec great idea!

tghosth commented 9 months ago

@csfreak92 did you see this? Can you work with @jsherm-fwdsec on this?

csfreak92 commented 9 months ago

For sure @tghosth, will work with @jsherm-fwdsec on this one! I think some parts of it were covered by OAuth 2.0 requirements that I wrote, but I will double/triple check to ensure we got them nailed to good detail to include OpenID stuff.

elarlang commented 9 months ago

I'm interested in contributing and happy to create a PR for this

our usual process is, that we first have agreement on the requirement wording in an issue and then we go for PR. This way it is easier to follow later, why some change was made and what where the arguments behind the change were.

@jsherm-fwdsec - do you have a wording or an idea for the point for additional requirement(s)?

jmanico commented 9 months ago

How about…

Verify that applications utilizing OpenID Connect identify and utilize at least one additional claim, beyond sub and iss. This could include claims such as email, preferred_username, or a custom claim agreed upon with the identity provider (IdP).

elarlang commented 9 months ago

How about… Verify that applications utilizing OpenID Connect identify and utilize at least one additional claim, beyond sub and iss. This could include claims such as email, preferred_username, or a custom claim agreed upon with the identity provider (IdP).

For me it seems it is not correct, see https://openid.net/specs/openid-connect-core-1_0.html#ClaimStability.

Therefore, the only guaranteed unique identifier for a given End-User is the combination of the iss Claim and the sub Claim.

jsherm-fwdsec commented 9 months ago

Hi @jmanico @elarlang! Agreed I think we need to be more specific about what the unique identifier should be and it should conform to the spec.

How about something such as this: _Verify that the unique identifier in the ID token for an end-user is a combination of the sub Claim and the iss Claim. Avoid usage of other claims such as email, phone_number, preferredusername, or name , as they may be mutable and can be potentially falsified.

elarlang commented 9 months ago
jsherm-fwdsec commented 9 months ago

That works here's what I would suggest: _Verify that the unique identifier in the ID/access token for an end-user is a combination of the sub claim and the iss claim, if a unique identifier is needed. Avoid usage of other claims such as email, phone_number, preferredusername, or name , as they may be mutable and can be potentially falsified.

Also agreed this should go under https://github.com/OWASP/ASVS/blob/master-v51-oauth/5.0/en/0x51-V51-OAuth2.md#v513-resource-server.

Since we're touching on ID tokens that are specific to OIDC, we might also want to add and update some terminology under this section for ID token, access token, and "Authorization Server" to include info about the OpenID Provider: https://github.com/OWASP/ASVS/blob/master-v51-oauth/5.0/en/0x51-V51-OAuth2.md#terminology.

Here's my attempt:

elarlang commented 9 months ago

Just for info, we need to solve some other open issues for the OAuth paragraph before moving this one further.

csfreak92 commented 9 months ago

Yeah, I'm working on the OAuth section on my local copy. I will push commits soon.

csfreak92 commented 8 months ago

Question for the leaders: @jmanico, @tghosth, @elarlang, Will OpenID Connect section be under the OAuth 2.0 chapter that we were working on? Or will it be on a separate chapter on its own? If it was the latter part, I would suggest @jsherm-fwdsec to create a PR on a separate branch, but if someone can point him to where to push it so that he won't be waiting too long for me on the OAuth section to be merged.

elarlang commented 8 months ago

At the moment we put all OAuth and OpenID-related requirements into one chapter.

Based on the current direction, we will not have a separate section for OpenID flow. But let's see, how many OpenID-related requirements we will have, we may reorganize them into separate/different chapters.

For current requirement we agreed to add it to the Resource Server chapter: https://github.com/OWASP/ASVS/issues/1826#issuecomment-1936948199

category/section choice - does it fit to the "Resource Server" (as "Relying Party") section in our current structure (https://github.com/OWASP/ASVS/blob/master-v51-oauth/5.0/en/0x51-V51-OAuth2.md)

https://github.com/OWASP/ASVS/issues/1826#issuecomment-1941874604

Also agreed this should go under https://github.com/OWASP/ASVS/blob/master-v51-oauth/5.0/en/0x51-V51-OAuth2.md#v513-resource-server.

I can see the procedure, that we get the current OAuth chapter merged to the main branch, and then we go issue-by-issue with the modifications. At the moment it can be a slow-downer, as there are too many "opened issues" as separate comments in different places and it's complicated to get an overview, what is the situation and what must be done.

jmanico commented 8 months ago

I suggest that OAuth2 and OIDC are two separate chapters. They are quite different even though they are both based on the OAuth2 Framework.

tghosth commented 8 months ago

Are they going to be different enough to merit two chapters @csfreak92 ?

tghosth commented 8 months ago

They should probably be in different sections at least

csfreak92 commented 8 months ago

I haven't looked at OpenID Connect as closely with OAuth. Maybe separate sections at least for now. Can you draft something for it @jsherm-fwdsec?

elarlang commented 8 months ago

Gentlemen, please, re-read my comment: https://github.com/OWASP/ASVS/issues/1826#issuecomment-1968307400 :)

Let's get the OAuth chapter in, let's get the changes done, let's get OIDC requirements in and then we can shake everything to correct or better place if needed. One step at a time.

jsherm-fwdsec commented 8 months ago

Sounds good @elarlang. I'd be happy to help contribute to the OIDC requirements when the time comes.

randomstuff commented 5 months ago

Other claims such as email, username or phone number should not be used, as they can change over time and an attacker can falsify these claims.

Provision should probably be added to allow for using some other claims if this requirement is overridden by another specification (for a specific issuer). Some issuers could provide custom claims with identifiers which are intended to be stable and reliable for user identification (for this issuer).

For example ProSanteConnect (a French SSO service for medical practitioners) uses a SubjectNameID claim for declaring the user national identifier and its usage is required:

Le Fournisseur de Service DOIT utiliser le champ SubjectNameID pour récupérer l'identifiant unique de l'identité […]

Roughly translated as:

The Service Provider [i.e. the Relying party] MUST use the SubjectNameID field [i.e. claim] in order to fetch the identity unique identifier

AgentConnect, a French SSO service for national agents states:

Il ne faut pas effectuer une réconciliation sur le SUB, car le SUB est calculé en fonction du FI utilisé. Nous vous recommandons l'utilisation de l'email professionnel pour cet usage.

Translated as:

SUB shall not be used for [identity] reconciliation because the SUB is computed depending on the identity provider [i.e. OpenID provider]. We recommend using the professional email [address] for this usage.

elarlang commented 5 months ago

Thank you, Gabriel (@randomstuff) for reaching out!

Proposal from Jordan https://github.com/OWASP/ASVS/issues/1826#issuecomment-1941874604

Verify that the unique identifier in the ID/access token for an end-user is a combination of the sub claim and the iss claim, if a unique identifier is needed. Avoid usage of other claims such as email, phone_number, preferred_username, or name , as they may be mutable and can be potentially falsified.

In general, it would be nice to set the requirement against the attack vector, or defining the principle that must be achieved, not saying directly "use this configuration". Then it's potentially more clear, why such a requirement exists.

I think we can rephrase the proposal from Jordan to achieve this.

csfreak92 commented 3 months ago

Hmmmm, what about Verify that the unique identifier in the ID/access token for an end-user is a combination of and composed of non-mutable attributes (e.g. sub claim and iss claim)? I do like the idea of mention the sub claim and iss claim as they are indeed importantly some of those attributes. I don't want implementors to get lost in translation then next thing they use is something more hilarious, if you know what I mean.

randomstuff commented 1 month ago

Verify that the unique identifier in the ID/access token for an end-user is a combination of and composed of non-mutable attributes (e.g. sub claim and iss claim)

Could we go further and say:

Verify that the unique identifier in the ID/access token for an end-user is a combination of and composed of non-mutable attributes. This will typically be the combination of the 'sub' and 'iss' claims.

randomstuff commented 1 month ago

non-mutable

(Actually the important thing is that the attribute tuple must not be reattribuable to another user. Mutable attributes might be OK if there is a mechanism in place in order to transition to the newer values. Not sure we need to go into this level of details however.)

elarlang commented 1 month ago

Just some wordings to clarify/recheck I can understand the addressed problem correctly:

Verify that if the application builds a unique identifier for the end-user based on data in the ID/access token, the identifier is a combination of and composed of non-mutable attributes. This will typically be the combination of the 'sub' and 'iss' claims.

.. or roughly the message is, if you build a unique key based on data from the token, you need to take into account that a token from one issuer can not produce the same unique key as a token from another issuer.

randomstuff commented 1 month ago

you need to take into account that a token from one issuer can not produce the same unique key as a token from another issuer.

Actually that's not the problem being addresses, I think.

The problem is if the RP uses for example, the email claim in order to identify the user. If the user foo@example.com remove his account on the IdP/email-provider, someone might register a new account later take over the email address foo@example.com and take over the original user's account on the RP.

Same is true for example for a phone_number claim.

randomstuff commented 1 month ago

On the topic of the iss claim, it should always be taken into account for identifying the account.

elarlang commented 1 month ago

Ok, got the idea. Back to proposal (from https://github.com/OWASP/ASVS/issues/1826#issuecomment-2374679998):

Verify that the unique identifier in the ID/access token for an end-user is a combination of and composed of non-mutable attributes. This will typically be the combination of the 'sub' and 'iss' claims.

I think the wording is misleading:

Verify that the unique identifier in the X token

We don't verify something what is in the token, but we need to build / use something from the token, that is unique identifier for the user.

randomstuff commented 1 month ago

Maybe something like,

Verify that the users are scoped by issuer ("iss" claim) and that the claims used for identifying the users cannot be reattributed to another user. Typically the "sub" claim will be used but other claims could be used instead. The "email" and "phone_number" claims are not suitable for identifying the user because they might be reattributed to other users.

elarlang commented 1 month ago

Moving forward :)

I think the requirement needs to contain the information, for whom this applies (OIDC Client, OAuth RS) and whether is it valid for ID token or also for access_token.

jmanico commented 1 month ago

I think that’s a good idea Elar. Maybe a new column for this section specifying the protocol or protocols it’s relevant for.

randomstuff commented 1 month ago

or also for access_token

Yes, the case of OIDC is quite clear. This should apply yo the ID token and the UserInfo response.

For OAuth, it depends. The access token could have many different formats. It might not be tied to a user identity at all (from the point of view of the protected resource server).

TobiasAhnoff commented 1 month ago

For OAuth "it depends" so a suggestion is to have a more generic requirement, perhaps like this:

Verify that users and clients can be uniquely identified from the access token. Typically for a JWT by using the iss and sub claims, where subject is a unique identifier for the context of the issuer, but note that other token attributes (claims) can be used as well (depending on context and token format, in example client-id claim).

For OIDC ID Tokens this is given by the specs, see https://openid.net/specs/openid-connect-core-1_0.html#IDToken which states that sub is "A locally unique and never reassigned identifier within the Issuer for the End-User, which is intended to be consumed by the Client...". So the requirement for OIDC could be something like:

Verify that users can be uniquely identified from the ID token, where subject is a unique identifier for the context of the issuer.

elarlang commented 1 month ago

Let's try to get the requirement for "OIDC Client" first. I made some changes to the proposal:

Verify that the Client uniquely identifies the user from ID token claims, where the subject claim is the unique identifier for the issuer context.

ping @TobiasAhnoff @randomstuff

randomstuff commented 1 month ago

Verify that the Client uniquely identifies the user from ID token claims, where the subject claim is the unique identifier for the issuer context.

I think there are two missing things in this formulation:

Therefore, I'd use something like:

Verify that the Client uniquely identifies the user from ID token claim or claims, usually the "sub" claim, which are not reassigned to other users (for the scope of an IdP).

elarlang commented 1 month ago

which are not reassigned to other users (for the scope of an IdP).

"are not reassigned" > "can not be reassigned"?

Verify that the Client uniquely identifies the user from ID token claims, usually the 'sub' claim, which can not be reassigned to other users (for the scope of an IdP).

elarlang commented 1 month ago

The requirement to V51.5 OIDC Client goes in via #2124

# Description L1 L2 L3
51.5.2 [ADDED] Verify that the Client uniquely identifies the user from ID token claims, usually the 'sub' claim, which can not be reassigned to other users (for the scope of an identity provider).
elarlang commented 1 month ago

Now, let's move on with the requirement for the OAuth Resource Server:

Verify that users and clients can be uniquely identified from the access token. Typically for a JWT by using the iss and sub claims, where subject is a unique identifier for the context of the issuer, but note that other token attributes (claims) can be used as well (depending on context and token format, in example client-id claim).

We should take into account that the access token information may come from Introspection (https://datatracker.ietf.org/doc/html/rfc7662)

randomstuff commented 1 month ago

Verify that users and clients can be uniquely identified from the access token. Typically for a JWT by using the iss and sub claims, where subject is a unique identifier for the context of the issuer, but note that other token attributes (claims) can be used as well (depending on context and token format, in example client-id claim).

Would it be worth mentioning that this is only if user identification is required (or is that clear enough as it is)? The resource server could very well receive (by design) an anonymous access token.

elarlang commented 1 month ago

Would it be worth mentioning that this is only if user identification is required (or is that clear enough as it is)? The resource server could very well receive (by design) an anonymous access token.

Yes, conditional requirement makes sense.

TobiasAhnoff commented 1 month ago

A suggestion is

Verify that, if authorization is based on user or client identity, users and clients must be uniquely identified from the access token. Typically for a JWT by using the iss and sub claims, where subject is a unique identifier for the context of the issuer, but note that other token attributes (claims) can be used as well (depending on context and token format, in example client-id claim).

Or just

Verify that, if authorization is based on user or client identity, users and clients must be uniquely identified from the access token.

randomstuff commented 1 month ago

@TobiasAhnoff, both wordings use "uniquely identified" which I think is not clear enough (the important feature being "not reattributable").

randomstuff commented 1 month ago

[ADDED] Verify that the Client uniquely identifies the user from ID token claims, usually the 'sub' claim, which can not be reassigned to other users (for the scope of an identity provider).

I am wondering whether it it would be worth adding a requirement stating that if multiple identity providers are used, we should verify that each the identities of each issuer are properly partitioned from each other i.e. that IdP1 cannot spoof the identity of a user from IdP2 (unless this is desired as a consequence of some functional requirement).

elarlang commented 1 month ago

I am wondering whether it it would be worth adding a requirement stating that if multiple identity providers are used, we should verify that each the identities of each issuer are properly partitioned from each other i.e. that IdP1 cannot spoof the identity of a user from IdP2 (unless this is desired as a consequence of some functional requirement).

What if the same user uses 2 different IdP's but those should give the same user in the client?

elarlang commented 1 month ago

For the access token requirement, I propose one alternative direction:

Verify that if an access control decision requires identifying a unique user from an access token (JWT or related token introspection response), the resource server identifies the user from claims that can not be reassigned to other users. Typically it means using a combination of 'iss' and 'sub' claims.

randomstuff commented 1 month ago

What if the same user uses 2 different IdP's but those should give the same user in the client?

Yes, that's why there was "unless this is desired as a consequence of some functional requirement". The goal being to say that this must not happen "by mistake" but only by design. Something in the line of:

Verify that, if multiple identity providers are used by a RP, the identity of a user from an identity provider cannot be spoofed by another identity provider [by using the same user identifier].

randomstuff commented 1 month ago

I am wondering whether it it would be worth adding a requirement stating that if multiple identity providers are used, we should verify that each the identities of each issuer are properly partitioned from each other i.e. that IdP1 cannot spoof the identity of a user from IdP2 (unless this is desired as a consequence of some functional requirement).

On the other hand, this is not specific to OAuth but applicable to other SSO / authentication systems. I'll check which verifications (if any) in the authentication chapter (or somewhere else) already covers this.

elarlang commented 1 month ago

I feel like here are 3 topics mixed:

I there is need to improve requirement 1, let's open a new issue.

Let's have agreement - PR-ready for requirement 2.

If there is material for requirement 3, it is material for a separate issue.