Repository navigation
security: a credential's proof must be made by a key of its issuer - #36
Merged
Merged
Conversation
`DTGCredential::verify_proof_with_public_key` verified the Data Integrity proof under the key it was handed, but never checked that the proof's `verificationMethod` belongs to the credential's `issuer`. A caller that resolves the method the proof names and then trusts `issuer()` therefore accepted a credential naming any issuer, signed by any key. Its sibling `verify_grant_with_public_key` already made this check. Both public verification entry points now run one shared check after the signature and return `DTGCredentialError::ProofNotFromIssuer` on a mismatch. A method belongs to the issuer when the DID before its `#` is exactly the issuer, when it is the issuer itself, or when it is a bare `#fragment`, read against the issuer. Comparison is verbatim, and an empty issuer matches nothing. `authority::verify_chain` and `delegation::verify_chain` check no signatures themselves and rely on each link having been verified with `verify_proof_with_public_key`, so they gain the binding through it. Not a breaking API change: the error variant already existed and the enum is `#[non_exhaustive]`. It is a behaviour change: credentials signed with a key whose id is not a method of the issuer no longer verify. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defect
DTGCredential::verify_proof_with_public_keyverified the Data Integrity proof under the key the caller passed, but never checked that the proof'sverificationMethodbelongs to the credential'sissuer. A caller that resolves the method the proof names and then trustsissuer()accepted a credential naming any issuer, signed by any key.verify_grant_with_public_keyalready made this check; the credential-level entry point did not.Fix
One private helper,
check_proof_from_issuer, now runs after the signature in both public verification entry points:DTGCredential::verify_proof_with_public_keyverify_grant_with_public_keyauthority::verify_chainanddelegation::verify_chaincheck no signatures themselves (by design, documented); they rely on each link having gone throughverify_proof_with_public_key, so they gain the binding through it. No other public function verifies a proof.A method belongs to the issuer when:
#is exactly the issuer (did:x:a#key-1), or#fragment, read against the issuer (previously refused by the grant path).Comparison is verbatim (a DID URL with a query or path before the fragment is not the issuer); an empty issuer matches nothing. On the grant path the issuer may be a string or an object with an
id.Compatibility
DTGCredentialError::ProofNotFromIssueralready existed and the enum is#[non_exhaustive]. No version bump here; CHANGELOG entry under[Unreleased].Tests
tests/issuer_binding.rs, each case against both entry points: another DID refused; a DID that merely extends the issuer's refused; a DID URL with a query refused; a method of the issuer accepted; the issuer itself as the method accepted; a relative#fragmentaccepted; object-form issuer bound by itsid(accept and refuse).Locally:
cargo fmt --check,cargo clippy --all-targets --all-features -D warnings,cargo test --all-features,cargo test --no-default-features,cargo checkon 1.95.0,cargo package, and both examples run.