openid / OpenID4VP

48 stars 13 forks source link

`client_id_scheme` security considerations #124

Open danielfett opened 5 months ago

danielfett commented 5 months ago

When client_id_scheme is used, there can be multiple client_ids in the same ecosystem that belong to different clients. One of those clients could be malicious, compromised or the client_id scheme could allow for spoofing/impersonating client ids. One such insecure/spoofable client must not endanger the security of other clients in the same ecosystem.

In the protocol, since the client_id_scheme parameter namespaces the client_id, it should appear everywhere where client_id appears, including in aud values and in the sub value of the Verifier Attestation JWT.

In the credentials, the client_id_scheme should be included besides the client_id as the audience; e.g., in an SD-JWT KB-JWT, there should be a separate claim besides aud.

In Section 12.1, the following sentences need to be adapted as well:

The Wallet MUST link every Verifiable Presentation returned to the Verifier in the VP Token to the client_id and the nonce values of the respective Authentication Request.

The Verifier MUST validate every individual Verifiable Presentation in an Authorization Response and ensure that it is linked to the values of the client_id and the nonce parameter it had used for the respective Authorization Request.

The client_id is used to detect the presentation of Verifiable Credentials to a party other than the one intended.

In the security considerations there should be considerations about confusing two client IDs with different client ID schemes.

Everywhere where a party checks a client id (especially the AS and the client), it must check the tuple (client_id, client_id_scheme) instead. This also applies if client_id_scheme is not used by one of the parties (in which case client_id_scheme must be replaced by, e.g., null).

Edit 2023-05-17: Fixed a mistake in the third paragraph.

Sakurann commented 5 months ago

sorry, I am probably missing something, if client_id belongs to a compromised or malicious client that is part of the ecosystem using the same client_id_scheme, how does including client_id_scheme help detect that.

jogu commented 5 months ago

sorry, I am probably missing something, if client_id belongs to a compromised or malicious client that is part of the ecosystem using the same client_id_scheme, how does including client_id_scheme help detect that.

It's the equivalent of suggesting instead of "aud": "https://www.example.com" in a JWT for a client we instead have "aud": "redirect_uri:https://www.example.com" if the client_id uses the redirect_uri client_id_scheme (but instead of having a structured aud value Daniel is suggesting having a separate claim that recipients of the JWT will need to check).

I'm not sure if there is actually a practical issue unless a client_id_scheme that allows the attacker to select a completely arbitrary client_id is supported (which I don't think any of the currently defined client_id_schemes do, I think they pretty much all require a url or hostname or an AS-assigned client_id).

It does feel like there might be an issue when client_id_schemes result in what is nominally the same client having the same client_id but with different security levels. So for exactly where a client intends to have client_id_scheme=entity_id&client_id=https://www.example.com but the attacker instead uses client_id_scheme=redirect_uri&client_id=https://www.example.com and is then able to obtain a VC with an aud of https://www.example.com that it can attempt to inject into a flow for the legitimate client.

awoie commented 5 months ago

In the credentials, the client_id should be included as the audience; e.g., in an SD-JWT KB-JWT, there should be a separate claim besides aud.

Each credential format has to define how to use client_id and client_id_scheme in the response.

W3C VCDM with Data Integrity:

SD-JWT VC:

W3C VCDM with JWT:

ISO mdoc:

awoie commented 5 months ago

This also applies if client_id_scheme is not used by one of the parties (in which case client_id_scheme must be replaced by, e.g., null).

IMO, if client_id_scheme is not present, it is pre-registered (default value if omitted).

danielfett commented 5 months ago

I'm not sure if there is actually a practical issue unless a client_id_scheme that allows the attacker to select a completely arbitrary client_id is supported (which I don't think any of the currently defined client_id_schemes do, I think they pretty much all require a url or hostname or an AS-assigned client_id).

The problem is that this security is accidental and neither guaranteed in all deployments nor for any future extensions that define new client id schemes.

jogu commented 5 months ago

@danielfett yes, exactly. For clarity whilst I can't identify a practical attack with the current client id schemes I do agree there's a real issue here we need to look at fixing.

Sakurann commented 4 months ago

in-person WG:

peppelinux commented 4 months ago

I've come to believe that the security concerns discussed in this thread primarily pertain to the redirect_uri client_id_scheme. If other client id schemes do not share this vulnerability, my recommendation - even if it might sound very unpopular - is to discontinue support for the redirect_uri scheme and remove it from the specifications. Personally, I have always perceived the redirect_uri scheme as a security risk and have been critical of its implementation.

Regarding the potential security issues with new client id schemes mentioned by @danielfett and @jogu, I believe such concerns should be addressed by those future extensions that have not yet identified their security weaknesses. Something that seems pretty out of scope for openid4vci.

I agree with @awoie with the approach that if the parameter is absent, the Wallet MUST follow the behavior outlined in RFC6749.

In the context of OpenID Federation, the client id by itself is sufficient to initiate trust discovery, unless it is an HTTPS URL. While the client id scheme serves as a useful indicator for determining the pattern of client id evaluation.

It's important to note that even with namespaced client ids, an attacker could potentially replace the entire client_id value, redirecting to any values.

Therefore, I suggest that our focus should be more on addressing the root of the problem—the redirect_uri—rather than the representation of the client identifier itself.

jogu commented 4 months ago

I've come to believe that the security concerns discussed in this thread primarily pertain to the redirect_uri client_id_scheme. If other client id schemes do not share this vulnerability

There is a similar ambiguity between entity_id and x509_san_uri

It's important to note that even with namespaced client ids, an attacker could potentially replace the entire client_id value, redirecting to any values.

That would result in client authentication failing as far as I can see.

Therefore, I suggest that our focus should be more on addressing the root of the problem—the redirect_uri—rather than the representation of the client identifier itself.

The root cause of the problem is the introduction of client_id_scheme in the VCI spec, hence I believe the issue needs to be addressed in the VCI spec.

peppelinux commented 4 months ago

@jogu formally you are completely right, as usual. At the same time, from my perspective, a trust discovery using federation or x509 can be achieved by using the client_id alone.

I don't have any expectations about not taking this in the scope of openid4vci, therefore I hope that my comment won't block the conversation about the solution we may have to improve the current specs.

tplooker commented 4 months ago

In the protocol, since the client_id_scheme parameter namespaces the client_id, it should appear everywhere where client_id appears, including in aud values and in the sub value of the Verifier Attestation JWT.

This is interesting, I wouldn't have described the client_id_scheme as namespacing the client_id. I would have described the purpose of the client_id_scheme parameter as telling you how to interpret the client_id. I can see however the security of client authentication becomes tied to the weakest client_id_scheme supported by the party receiving the request is that a correct way to interpret it @danielfett? e.g you could potentially have the same client_id across two different client_id_schemes?

tplooker commented 4 months ago

Just a note related to @Sakurann's comment above, I will provide another proposal related to this issue soon which could help simplify things.

tplooker commented 4 months ago

As said above, here is my proposal

While we consider this issue, I think we should also decide whether all of the client_id_schemes we currently have defined are worth persisting with. IMO, if we consider the original intent of the client_id_scheme it was really about devising a way for a client to identify itself and communicate its metadata with the wallet in a way that doesn't require pre-registration. For a variety of reasons we've ended up with perhaps more client_id_schemes then are practically needed, for instance below is the current set of client id schemes supported and used

One possible proposal here would be to do the following

How would this look, a standard request would look like

HTTP/1.1 302 Found Location: https://client.example.org/universal-link? response_type=vp_token &client_id=https%3A%2F%2Fclient.example.org &client_id_scheme=uri &redirect_uri=https%3A%2F%2Fclient.example.org%2Fcb &presentation_definition=... &nonce=n-0S6_WzA2Mj &client_metadata=...

In a scenario where the requested is signed and the client/verifier's signing keys need to be traced via a chain of x509 certificates (e.g how x509_san_dns works), the signed request would look as it does now, for example decoded to

Header

{
    "typ": "oauth-authz-req+jwt",
    "alg": "RS256",
    "x5c": ["...", "..."]
}

Payload

{
   "client_id": "https://client.example.org",
   "client_id_scheme": "uri",
   "response_type": "vp_token",
   "redirect_uri": "https://client.example.org/cb",
   "nonce":"n-0S6_WzA2Mj",
   "presentation_definition": { ... },
   "client_metadata": { ... }
}

When the client wants to use the verifier_attestation style behaviour the signed request would look as follows

Header

{
    "typ": "oauth-authz-req+jwt",
    "alg": "RS256",
    "jwt": "..."
}

Payload

{
   "client_id": "https://client.example.org",
   "client_id_scheme": "uri",
   "response_type": "vp_token",
   "redirect_uri": "https://client.example.org/cb",
   "nonce":"n-0S6_WzA2Mj",
   "presentation_definition": { ... },
   "client_metadata": { ... }
}
David-Chadwick commented 3 months ago

If we go back in time to when the client_id_scheme parameter was introduced, it was as a result of issue 1551 that I introduced. This was essentially "How can the wallet trust the verifier". I wanted this new parameter to define the trust scheme being used, so that the wallet could look up the client_id in its trust infrastructure to determine if the client_id was trusted. But this is not how the parameter has evolved since then. However if we return to the original issue, then redefining client_id_scheme to indicate the trust scheme in operation (which in fact some of the values already do e.g. entity_id indicates OpenID Federation) then we might have a solution to this issue.

jogu commented 3 months ago

@David-Chadwick

then redefining client_id_scheme to indicate the trust scheme in operation (which in fact some of the values already do e.g. entity_id indicates OpenID Federation) then we might have a solution to this issue.

I don't think I'm following how this solves the problem, can you expand please? It seems like the fundamental issue of client id's not being unique to a trust scheme and a client id alone not being sufficient to indicate how the client wants to be / was authenticated would remain.

jogu commented 3 months ago

@tplooker

  • Simplify to have one client ID scheme of uri where when used, the URI needs to have a scheme of either https or did.
  • To cater for the current behaviour of the verifier_attestation x509_san_dns and x509_san_uri client id schemes, which IMO are more about how the client authenticates rather then how it identifies itself we shift to a model which relies on the presence of certain parameters in the signed request object header, such as x5c.

Can you expand on how this scheme would solve the "In the credentials, the client_id_scheme should be included besides the client_id as the audience; e.g., in an SD-JWT KB-JWT, there should be a separate claim besides aud." part of Daniel's original problem statement?

David-Chadwick commented 3 months ago

@jogu Indicating the trust scheme that should be used to validate the client_id solves the problem because the wallet can determine if the RP is trusted or not by communicating with its trust scheme and root(s) of trust. The assumption is that every trust scheme will not have duplicate client_ids (because if it did have, then no-one could determine if a specific client_id was the intended trusted one or not.) It does not mater if a different trust scheme use the same client_ids (as another trust scheme) to refer to different RPs, because the wallet is operating in its own trust scheme and determines if the RP is trusted under its own trust scheme rules. So if RP1 and RP2 have the same client_id but these are issued under different trust schemes, TS1 and TS2 respectively, the wallet will know which RP is trusted dependent upon the asserted trust scheme. It is very important that the trust scheme provides all the important parameters of the RP (the ones provided in the protocol should not be trusted because they do not need to be signed and they could be fraudulent). So say RP2 fraudulently says its client-id has been allocated by TS1, the wallet will nevertheless believe it is talking to RP1 and will proceed on that basis. RP2 will never receive any credentials from the wallet because the trust scheme will have provided RP1's URL. The worst thing that can happen to the user is that RP1 (which is trusted) receives unexpected credentials from the wallet when it had not requested them. Alternatively the wallet may detect anomalies between what the trust scheme says about RP1 and what RP2 said about itself, and therefore decides to terminate the connection.

jogu commented 3 months ago

@David-Chadwick

The worst thing that can happen to the user is that RP1 (which is trusted) receives unexpected credentials from the wallet when it had not requested them.

I don't think that's the worst thing that can happen. The verifier RP1 can potentially receive credentials for one user in a session started by a different user.

David-Chadwick commented 3 months ago

You would have to explain to me how this could possibly happen. Wouldn't it require two different users to be colluding with a rogue RP (RP2) in order to gain access to a trustworthy RP (RP1)? In which case the two users could collude much more easily than needing to involve RP2. But if the two users are not colluding, then the user with the high value credentials would need to be tricked into sending their credentials to RP1 when the user with no or low value credentials is trying to access it. So it sounds highly improbably to me, unless I have missed something.

jogu commented 3 months ago

Wouldn't it require two different users to be colluding with a rogue RP (RP2) in order to gain access to a trustworthy RP (RP1)? In which case the two users could collude much more easily than needing to involve RP2.

An example would be one user is an attacker, the second user is being phished.

We're probably veering of the main point as we don't have a full attack that works with what's defined today. The problem is that both the spec and in particular client_id_scheme have extension points, so we're leaving a potential foot gun that might result in unintended consequences when people add/change things in the future.

What we do know is that in general signed messages that are received are normally validated to be to and from the expected parties, and by not including & checking client_id_scheme (whilst also having existing and potential future client id schemes that will assign client ids that have the same value but are either different clients or have been authenticated in different potentially much less trustworthy ways) in those messages we have broken that property.

David-Chadwick commented 3 months ago

This is precisely why I am suggesting that client_id_scheme should refer to the trust scheme and the client_id should be evaluation with respect to this trust mechanism (which it already does for X.509 for example, so make it do this for all the other schemes as well. Currently it has imprecise semantics).

peppelinux commented 3 months ago

@tplooker

for instance below is the current set of client id schemes supported and used

x509_san_dns
x509_san_uri
redirect_uri
verifier_attestation
did
pre-registered

entity_id should be taken in consideration and therefore included in the list.

selfissued commented 3 months ago

I've been thinking about this for a while. It seems to me that introducing client_id_scheme may have created more problems than it solves. I would therefore support proposals to remove it.

Some have advocated in this thread that if client_id_scheme isn't present, that the default should be that the Client ID is allocated by the Authorization Server. I think the answer should be closer to "the default is defined by the ecosystem rules".

For instance, in OpenID Federation, we have years of experience with Client IDs that are Entity Identifier URLs. They work great. In those deployments, no client_id_scheme is needed or used, and they're not allocated by ASs. In this case, the default is that the Client ID is an Entity ID.

jogu commented 3 months ago

This is precisely why I am suggesting that client_id_scheme should refer to the trust scheme and the client_id should be evaluation with respect to this trust mechanism (which it already does for X.509 for example, so make it do this for all the other schemes as well. Currently it has imprecise semantics).

Yes, it might sort of solve messages from the verifier to the wallet that include client_id_scheme.

I don't understand how that solves the fundamental problem of signed messages in both ways not being properly authenticated without descending into "it's okay for everything we know about today" which as I mentioned in my previous message isn't a robust approach.

David-Chadwick commented 3 months ago

"it's okay for everything we know about today"

Isn't that how all security works? and patches are introduced when new facts and bugs come to light

David-Chadwick commented 3 months ago

x509_san_dns - indicates the trust scheme x509_san_uri - indicates the trust scheme redirect_uri - does not say anything about the trust scheme verifier_attestation - not sure what this indicates did - some dids might indicate the trust scheme and others do not pre-registered - indicates the trust scheme entity_id - indicates the trust scheme but might be better named "federation"

tplooker commented 3 months ago

Can you expand on how this scheme would solve the "In the credentials, the client_id_scheme should be included besides the client_id as the audience; e.g., in an SD-JWT KB-JWT, there should be a separate claim besides aud." part of Daniel's original problem statement?

Well if there is really only "one" type of client id scheme which is "uri" do we even need to include the client_id_scheme parameter in the response anymore? I would posit we don't.

paulbastian commented 2 months ago

My observation is that client_id and client_id_scheme should always be used in conjunction, therefore this is a good indicator that these two parameters should be merged, i.e. client_id_scheme should be the scheme for client_id and we should find ways how to solve potentially evolving issues.

Or do we have other examples in the specs, where two parameters must always be used together and they are not merged?

Sakurann commented 2 months ago

WG discussion: there seems to be an agreement on the problem. but there still seems to be a disagreement on the solution:

  1. it is not a severe enough of a problem, keep things as-is
  2. namespace client_ids (especially, if those two things always have to be used together; would require an extra processing steps for the wallet to remove the namespace, before processing the client_id itself; this approach deviates from oauth, where client_id is not namespaced, which is also kind of due to the nature of wallet space being different)
  3. whenever client_id is used, mandate to also include client_id_scheme
selfissued commented 2 months ago

Based on the discussion on the call today, I continue to believe that we should remove client_id_scheme. It creates more problems than it solves.

c2bo commented 2 months ago

Based on the discussion on the call today, I continue to believe that we should remove client_id_scheme. It creates more problems than it solves.

How would we convey the mechanism to establish trust in the RP if we remove the client_id_scheme ? Wouldn't we require something very similar that likely shares the same problems?

My observation is that client_id and client_id_scheme should always be used in conjunction, therefore this is a good indicator that these two parameters should be merged, i.e. client_id_scheme should be the scheme for client_id and we should find ways how to solve potentially evolving issues. Or do we have other examples in the specs, where two parameters must always be used together and they are not merged?

I am with Paul on this one but I do feel I don't fully understand the problems that possibly prefixing client_id would create.

David-Chadwick commented 2 months ago

Well if there is really only "one" type of client id scheme which is "uri" do we even need to include the client_id_scheme parameter in the response anymore? I would posit we don't.

Since I started the issue over a year ago which led to client_id_scheme being adopted, I have never agreed with the resolution as it currently stands. I have always maintained that what we need is a method to indicate the trust scheme that the client_id should be used with. Client_id_scheme was the resolution that the group adopted, but as we can see, it is not a resolution of the original problem that I raised. So I propose that we remove client_id_scheme and introduce another parameter _trustscheme, which is an optional parameter that RPs may use if they need to indicate to the wallet what trust scheme the client_id should be used with. If the client_id on its own indicates the trust scheme that it is to be used with, then the parameter is not needed, but if there is no indication of the trust scheme, or it is ambiguous which trust scheme should be used, then the RP will indicate this in the trust_scheme parameter. The trust_scheme parameter should be a freeform string, and we can define certain values for it, but other users may define additional values in future.

danielfett commented 1 month ago

My proposal (if we agree to namespace) is to add the client id scheme as a URI scheme in front of the client id:

tlodderstedt commented 1 month ago

I originally was opposing against the change as I believed it would break existing OpenID federation implementations. However, I realized the way we use federation in OID4VP today, it already requires changes as federation implementation need to add processing of the client_id_scheme parameter. So it does not matter whether we have an additional parameter or add a prefix to the client id. Both require changes.

Sakurann commented 1 month ago

WG discussion

selfissued commented 1 month ago

The fact that this is acknowledged as breaking things reinforces my conclusion that client_id_scheme was a mistake in the first place. We should remove it.

David-Chadwick commented 1 month ago

@selfissued. client_id_scheme as currently defined is a mistake. We can agree on that. But a parameter is needed to indicate the trust scheme as I explained above. @danielfett 's proposal may do this if the semantics are clearly defined.

awoie commented 2 weeks ago

WG discussion

  • @awoie and @tplooker to make a proposal on august 6th DCP WG call.
  • would like to get implementation experience since it is a breaking change. especially for openid federation folks.

The proposal is to remove client_id_scheme and replace it by the following mechanism. If a client_id is a URL, one can look up metadata using a small number of selected .well-known suffixes. For example, one of them could be .well-known/openid-federation to enable trust establishment. Other .well-known can be defined by the core spec and by profiles. E.g. there might be a .well-known based on web domains, see https://github.com/openid/OpenID4VP/issues/82.

awoie commented 4 days ago

Remove client_id_scheme to solve #124

Rationale

Originally, the concept of a client ID scheme was introduced to govern the following:

However, the primary function of client_id_scheme in OID4VP seems to be:

Additional verifier metadata was a secondary use case since there are not many relevant ones pertaining to OpenID4VP. client_name might be one of the few.

The problem that client_id_scheme created:

Attempting to solve #124 by prefixing client_id values with the client_id_scheme does not solve all the problems that were described above. Furthermore, this would be a breaking change to other specifications OpenID4VP is used with and to existing deployments of OpenID4VP.

Removing the concept of client ID scheme would address all of the issues above. Note that this would still allow for verifier metadata being passed in the client_metadata parameter, or obtained via other mechanisms.

Observation

Given the small number of client ID schemes, these linkages can also be verified by inspection while being more flexible and less limiting in how to resolve verifier metadata by even allowing for combining different methods.

Proposal

I propose the following:

  1. Remove client_id_scheme and the concept of client ID scheme from OID4VP.
  2. Keep some of the language that is related to specific client ID schemes and focus on how these linkages can be verified based on inspection. Potentially, move some of the language to the Implementation or Security Consideration sections.

Regarding 2), I propose the following:

Conclusion

Removing the concept of client ID scheme specifically solves the original security issue (#124) and all the issues introduced in the rationale while also simplifying the specification, being more flexible and less limiting and not introducing any potential backwards compatibility issues with how the client_id value was previously defined and used in OAuth2 and OpenID-related specifications (e.g., OpenID Federation).

The proposal also allows the decoupling the verification of the linkage of public verification keys for request objects and the client_id value and/or verifier metadata. For example, a verifier attestation might be used with an entity ID value for the client ID and if the verifier attestation cannot be verified because the issuer is not trusted, .well-known lookup based on OpenID Federation could be performed.

David-Chadwick commented 4 days ago

It should not be mandatory to sign the request object, in which case the wallet/holder will not have a public key for the verifier. Nevertheless the holder should still be able to determine if the asserted client_id is trustworthy (or not) by interrogating its trust infrastructure. In which case it may need an indication of which trust method and/or root of trust to use, as suggested by the verifier. This is why an optional trust_scheme parameter may be needed instead of client_id_scheme.

javereec commented 4 days ago

I like the direction @awoie provided. The rationale is sound and the proposed solution is simplified and provides flexibility. One remark while reading.

Add text to Security or Implementation Considerations section how to verify a request object using DIDs, i.e., by checking the client_id is a DID and the keys from the kid of the JWT (request object). Add to Security Considerations that redirect_uri, even if the request object is signed, is not linked to the DID or client_id, which is missing today.

It could be argued that there likely IS a way to add redirect_uri to a DID Document, therefore providing a linkage between the two.

jogu commented 4 days ago

Oliver talked through his above comment made today on today's working group call.

Several working group members commented that they didn't see exactly how a wallet would be expected to process an incoming request to figure out the client authentication method being used, and there were similar comments about wanting to understand exactly what would go in the spec.

Oliver/Tobias agreed to provide more details, there was some discussion about whether it's best to provide more detail on the issue or as a PR, with Joseph/Kristina preferring it on the issue, however on reflection after the call the chairs felt a PR would be fine if Oliver/Tobias feel that's a better/easier way to express their thoughts.

I'm not marking with the 'ready-for-pr' yet as the working group has not yet reached consensus on adopting this approach or not, and any PR would just be to make the suggested approach clearer.

marcoscaceres commented 3 days ago

I tend to agree that dropping client_id_scheme would be a good idea (particularly if there's situations where client_id there's a mismatch with client_id_scheme). If we can derive everything we need just from client_id, then that would be great.