oauth-wg / oauth-selective-disclosure-jwt

https://datatracker.ietf.org/doc/draft-ietf-oauth-selective-disclosure-jwt/
Other
56 stars 27 forks source link

Validate that disclosures match the issuer JWT #373

Closed bifurcation closed 7 months ago

bifurcation commented 8 months ago

SD-JWT validation processes should require that the validating party (Holder or Verifier) validate that the hash of each disclosure object appears in the Issuer JWT claims. If this validation is not performed, then the SD-JWT format is malleable, in the sense that arbitrary additional disclosure objects can be added beyond those that the issuer intended. This creates the opportunity for mischief, from bugs to covert channels.

This is not a difficult property to enforce. The validation logic already has the validating party compute the lists of (1) hashes of disclosure objects and (2) disclosure hashes in the Issuer JWT. So the only additional step required is to check that (1) is a subset of (2).

bc-pi commented 8 months ago

That requirement that the hash of each disclosure must appear in the Issuer-singed JWT is in the current draft text - near the top of Verification of the SD-JWT has "a Holder or a Verifier MUST ensure that ... all Disclosures are correct, i.e., their digests are referenced in the Issuer-signed JWT".

Seems it could be made more explicit or clear?

But it is there.

bifurcation commented 8 months ago

Ah, thanks, I had missed that. Even given that though:

If we're going to eliminate recursive disclosure, I think the current text probably suffices (modulo "correct"). But if we're going to keep it, we need to elaborate the algorithm. Maybe it can be folded into the existing recursive algorithm as a termination condition. If you're adding the hashes inside disclosed values as you restore them, then if you get to the point where none of the claim hashes are in the set of disclosure hashes, then if the set of disclosure hashes is non-empty, the object is invalid.

bc-pi commented 8 months ago

Assuming here in this ticket at least that we are not going to eliminate recursive disclosure.

The use of "correct" is indeed awkward and that whole bit of text could be improved. There's room for a lot of improvement there.

I've always understood text like "referenced in the Issuer-signed JWT" as intending to mean included directly in that JWT but also included recursively. But being more explicit is probably worthwhile.

I think it'd be sufficient to say somewhere something like "all disclosures must correspond to a hash value that appears (either directly or recursively) in the Issuer-signed JWT". That somewhere could be in place of the "all Disclosures are correct" text and/or a step in the validation steps near the check that digests not be repeated.

To you earlier point - it's not a difficult property to enforce. Even with the recursion. A validating party needs to compute the lists of (1) hashes of the disclosure objects and has to look at (2) all disclosure hashes in the Issuer JWT (including by recursive inclusion). The only additional step is still just to check that (1) is a subset of (2). Which is basically just checking that there were no unused disclosures at the end of processing.

bifurcation commented 8 months ago

Agreed, if we can update the algorithm to reject if any hashes are left over, then that would fix this issue.

bc-pi commented 8 months ago

Agreed, if we can update the algorithm to reject if any hashes are left over, then that would fix this issue.

Great, I think we can update the the algorithm to reject if any disclosures are left over. It'd probably go near the check to ensure that no digest value is repeated in the whole of the SD-JWT - either in the payload directly or recursively in any disclosure (that check about duplicate digests #355 needs a bit of improvement too).

Also we should tidy up that text with "correct".