w3c / vc-data-integrity

W3C Data Integrity Specification
https://w3c.github.io/vc-data-integrity/
Other
42 stars 20 forks source link

Specify how relative URLs should be processed #77

Closed msporny closed 1 year ago

msporny commented 1 year ago

It is possible to perform a key confusion attack if relative URLs are put together in the wrong way from a VC or a controller document. This issue is to track this concern and make sure there is language on how to process verification method URLs such that it is not possible to have a key confusion attack. This algorithm should go in at least the vc-data-integrity spec and possibly the vc-jwt specification. /cc @OR13 @tplooker

One way to address this concern is to specify what to do when an implementer uses both absolute and relative URLs (that is, we count on some implementers doing the wrong thing, and when they do the wrong thing, we make sure implementations eliminate the possibility of a key confusion attack).

So, we could say something to the effect of: "Implementations MUST use absolute URLs everywhere." and then "If you come across a relative URL, then you can construct an absolute URL using and then ensure that the absolute URL resolves to a key in the controller document." I'll note that this eliminates a DID Document's ability to refer to a key that is external to a controller document, but that might be a agility/security trade-off that we're willing to make to ensure that there won't be key confusion attacks related to DID Documents.

OR13 commented 1 year ago

There are several open issues related, I will cross link.

OR13 commented 1 year ago

There are data integrity equivalents to this to consider:

"proof": {
  "type": "Ed25519Signature2018",
  "created": "2022-05-07T15:30:56Z",
  "verificationMethod": "did:example:123#kid-123", // absolute and pretty much what we see today.
  "proofPurpose": "assertionMethod",
  "jws": "eyJhbGciOiJFZERTQ..."
}

vs

"proof": {
  "type": "Ed25519Signature2018",
  "created": "2022-05-07T15:30:56Z",
  "verificationMethod": "#kid-123", // relative to.... issuer / issuer.id ?
  "proofPurpose": "assertionMethod",
  "jws": "eyJhbGciOiJFZERTQ..."
}
OR13 commented 1 year ago

Related to DID Document side of this:

{
// didDocument.id
    "iss": "did:web:example.nz", 
    "nbf": 1516239022,
    "exp": 1516239922,
    "jti": "urn:uuid:cc599d04-0d51-4f7e-8ef5-d7b5f8461c5f",
    "vc": {
        "@context": [ "https://www.w3.org/2018/credentials/v1", "https://nzcp.covid19.health.nz/contexts/v1" ],
        "version": "1.0.0",
        "type": [ "VerifiableCredential", "PublicCovidPass" ],
        "credentialSubject": {
            "givenName": "John Andrew",
            "familyName": "Doe",
            "dob": "1979-04-14"
        }
    }
}
{
    "@context": ["https://www.w3.org/ns/did/v1", "https://w3id.org/security/suites/jws-2020/v1"],
    "iss": "did:web:example.nz"
    "verificationMethod": [{
// proof.verificationMethod || header.kid || header.iss + header.kid || payload.iss + header.kid
        "id": "did:web:example.nz#key1", 
        "type": "JsonWebKey2020",
        "controller": "did:web:example.com",
        "publicKeyJwk": {
          "kty":"EC",
          "crv":"P-256",
          "x":"MKBCTNIcKUSDii11ySs3526iDZ8AiTo7Tu6KPAqv7D4",
          "y":"4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM",
        },
    }],
    "assertionMethod": ["did:web:example.com#key1"],
    "authentication": ["did:web:example.com#key1"],
}
OR13 commented 1 year ago

Safest path forward is to concretely describe how you get to a public key... all the ways you can get to one... better to define them all up front and then mark the "less desirable ones" not required to implement.

tplooker commented 1 year ago

IMO as a general rule, if the verification method is ever suppose to be checked against another property (e.g the issuer field in a VC) to resolve the key required to verify the VC AND establish the identity of the signer (e.g who is the issuer). Then the verification method should never be allowed to contain an absolute URL because doing so means the system fails open if the verifiers logic around comparing the verification method controller to the VC issuer is flawed. How popular JWT profiles work like id_tokens and how we designed NZCP as Orie referenced above uses an approach that makes it much much harder to ever fail like this because the key resolution process requires you to combine who the issuer is and the key id (conceptually similar to verificationMethod here) to get the required key to verify.

OR13 commented 1 year ago

We've updated iss and kid header parameters to have this property pretty much everywhere.

https://transmute-industries.github.io/vc-jwt-test-suite/#report

msporny commented 1 year ago

Safest path forward is to concretely describe how you get to a public key

For Data Integrity, this is now normatively defined here:

https://www.w3.org/TR/vc-data-integrity/#retrieve-verification-method

The normative algorithm for retrieving a public key is defined such that relative URLs are not allowed in Data Integrity proofs in the verificationMethod property. If a relative URL is attempted to be used, the dereferencing operation in the Retrieve Verification Method algorithm will fail, and the algorithm will error out (failing closed).

VC-JWT can have different rules, which can be defined in that specification.

The original question asked in this issue has been answered (usage of a relative URL will result in an error). Marking as pending 7 day close. If anyone believes that the issue has not been addressed, please let us know.

OR13 commented 1 year ago

The normative algorithm for retrieving a public key is defined such that relative URLs are not allowed in Data Integrity proofs in the verificationMethod property.

I wonder if we should do the same for kid in vc-jwt?

If a relative URL is attempted to be used, the dereferencing operation in the Retrieve Verification Method algorithm will fail, and the algorithm will error out (failing closed).

What about if an absolute URL that is different from issuer is used?

From a quick glance, this would succeed:

issuer: did:good.guy.example:123
proof: {
 verificationMethod: did:bag.guy.example:456#key-42
OR13 commented 1 year ago

Please remove the pending close until the binding to issuer is addressed.

msporny commented 1 year ago

What about if an absolute URL that is different from issuer is used?

That is a business rule that is outside the base Data Integrity specification. Remember, there is no such thing as an issuer in the Data Integrity specification; it's supposed to be oblivious to VC data structures.

That rule probably belongs in the vc-data-model, given that it's a part of validation, not verification... and it's not clear that you'd always want to enforce it (such as if there is an M-of-N signatures requirement associated with a particular issuer, like a Board of Directors)... or if you're dealing with a Notary use case, where you have a chained signature where the signatory and the notary are clearly not going to be the same issuer.

All that said, I'm not entirely averse to adding something in the Data Integrity specification about it, but then we're going to have to repeat the language across all the securing specifications. We should probably say something about it, without making it a MUST (given the two use cases above that demonstrate that ALWAYS doing that sort of binding would not allow for identified use cases).

Let me know how you want to proceed. I propose we add something to vc-data-model about cross-referencing the signing key with the issuer (in some way), and put that text in the validation section.

OR13 commented 1 year ago

@msporny I agree with what you wrote.

But I don't think we are chartered to apply data integrity proofs to data models that are different from w3c verifiable credentials.

It sounds to me like the clarification that is needed is something like this:

verificationMethod is always absolute URL that resolves to a controller document (what we have today) and in the context of "verifying" a data integrity proof... no additional information is needed (I think we have this already).

In the context of "validating" a document secured by one or more data integrity proofs, additional processing might occur. (what is needed in this spec).

It won't surprise you to hear that I object to filling the core data model with DataIntegrityProof specific validation logic or examples.

That text should be in this work item, not the core data model... but it's possible that it's not relevant to implementers, of either spec, in which case, it can be omitted from both.

msporny commented 1 year ago

But I don't think we are chartered to apply data integrity proofs to data models that are different from w3c verifiable credentials.

We are chartered to make Data Integrity generally useful, but our scope is not to extend beyond Verifiable Credentials. For example, if someone wants to work on how DI is applicable to graphs of arbitrary sizes, or streamed-signing of RDF Quads that are coming out of a quadstore, those things are out of scope (this is why the work was placed into this WG, to give us a set of limited use cases, and then if successful, expand the scope of DI in future WGs that might not be the VCWG). Hard coding DI to VCs was never a goal... all that said...

That text should be in this work item, not the core data model... but it's possible that it's not relevant to implementers, of either spec, in which case, it can be omitted from both.

I can add some text to the Data Integrity spec that talks about usage of Data Integrity with VCs and validation and issuer (and use that as an example of "further processing" that some applications might do), since we're chartered to speak to that. The guidance might not be useful for everyone using DataIntegrityProofs, but that's fine... if it's not useful to an implementer, they don't have to pay attention to the section.

msporny commented 1 year ago

PR #119 has been raised to address this issue. This issue will be closed once PR #119 has been merged.

msporny commented 1 year ago

PR #119 has been merged to address this issue, closing.