Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Value of a verification method property? #315

Open
iherman opened this issue Oct 16, 2024 · 13 comments
Open

Value of a verification method property? #315

iherman opened this issue Oct 16, 2024 · 13 comments
Assignees
Labels
CR1 This item was processed during the first Candidate Recommendation phase. pending close (7 days) This issue will be closed after 7 days. question Further information is requested

Comments

@iherman
Copy link
Member

iherman commented Oct 16, 2024

In https://www.w3.org/TR/vc-data-integrity/#proofs the definition of verficationMethod says:

verificationMethod
A verification method is the means and information needed to verify the proof. If included, the value MUST be a string that maps to a [URL].

This means that the following is not acceptable:

 "proof": {
    "type": "DataIntegrityProof",
    "cryptosuite": "eddsa-jcs-2022",
    "verificationMethod": {
        "id": "https://controller.example/123456789abcdefghi#keys-1",
       "type": "Multikey", 
       "controller": "https://controller.example/123456789abcdefghi",
       "publicKeyMultibase": "z6MkmM42vxfqZQsv4ehtTjFFxQ4sQKS2w6WR7emozFAn5cxu"
   }
}

This is in contradiction with the definition of the same property for controller documents in https://www.w3.org/TR/controller-document/#verification-methods:

If present, the value MUST be a set of verification methods, where each verification method is expressed using a map.

Which, interestingly, though would make the example valid if used on a controller document, would make this version invalid:

"verificationMethod": "https://controller.example/123456789abcdefghi#keys-1"

which is the only acceptable usage of the property for a proof.

Is this difference intentional? If so, why? Or are these both specification bugs?

Personally, I tend to believe these are both bugs.

@msporny
Copy link
Member

msporny commented Oct 19, 2024

Is this difference intentional?

Yes. :)

If so, why?

Because you cannot trust a proof to contain the the cryptographic material. If you allowed for that, then you could trivially compromise any proof. Here's the attack:

  1. I want to impersonate you.
  2. I generate a proof with your ID (I set the issuer field to you) and embed the public key information for one of your keys (I do as you say above in the proof, but I inject a public key value for a private key that I control).
  3. The verifier has no need to go anywhere else to verify the proof, it just uses the verificationMethod in the proof and BAM, they're compromised and I've just stolen your identity. :)

Or are these both specification bugs?

Neither are specification bugs.

That's what these sets of text are about:

https://w3c.github.io/vc-data-integrity/#verification-method-binding
https://w3c.github.io/controller-document/#retrieve-verification-method

@msporny msporny added question Further information is requested pending close (7 days) This issue will be closed after 7 days. CR1 This item was processed during the first Candidate Recommendation phase. labels Oct 19, 2024
@iherman
Copy link
Member Author

iherman commented Oct 20, 2024

@msporny

Neither are specification bugs.

I understand the argument for proofs. But you answered only half of my question: why do we require the same property to use a map as a value in the case of controller document, then? Doesn't the same argument holds, i.e., that I can impersonate a controller document using my own key? Even if there is a good argumentation in favor of today's status quo, this apparent inconsistency in our specs should be explained, I will surely not be the only person seeing it...

@iherman
Copy link
Member Author

iherman commented Oct 20, 2024

Another point, though, that does not have a bearing on the specification: the differentiation that you describe in very JSON specific. It does not mean anything when you reason about RDF graphs directly: a resource is either a "real" one with a URL or a blank node, and that is it...

@filip26
Copy link

filip26 commented Oct 20, 2024

did:key as a verification method (as plain @id - which is valid) suffers the same "attack" as described in comment 315 if a verifier does not have a policy/process to decide whenever to trust a key provided by the method or not - embedded or not, it does not matter.

@msporny
Copy link
Member

msporny commented Oct 20, 2024

@iherman wrote:

But you answered only half of my question: why do we require the same property to use a map as a value in the case of controller document, then?

Because the verification method has to be defined /somewhere/ and that place is in the controller document.

Doesn't the same argument holds, i.e., that I can impersonate a controller document using my own key?

No, because the "Retrieve Verification Method" algorithm prevents that sort of attack (in steps 2, 4, 6, 9, and 10):

https://w3c.github.io/controller-document/#retrieve-verification-method

and this is explained here (read everything after Example 19):

https://w3c.github.io/controller-document/#example-minimum-conformant-controller-document

Even if there is a good argumentation in favor of today's status quo, this apparent inconsistency in our specs should be explained, I will surely not be the only person seeing it...

We do spend an entire algorithm and three follow-up paragraphs explaining it at the link above (granted, that language just got merged yesterday!)... and each cryptosuite algorithm points back to the retrieval algorithm in their "Proof Verification" subalgorithms:

https://w3c.github.io/vc-di-ecdsa/#proof-verification-ecdsa-rdfc-2019

Do you feel like we need more than that, and if so, what would you suggest we add/change and where?

@msporny
Copy link
Member

msporny commented Oct 20, 2024

@iherman wrote:

Another point, though, that does not have a bearing on the specification: the differentiation that you describe in very JSON specific. It does not mean anything when you reason about RDF graphs directly: a resource is either a "real" one with a URL or a blank node, and that is it...

Hmm, I'm going to assert that it's not JSON specific at all. :)

What matters is how that information got into your information space (in the most abstract sense of that word). That is, if you let bad data in, you are compromised no matter whether you're processing "as JSON" or "as RDF" or anything else. The algorithm is about not letting bad data in, that is, the order in which you dereference the URLs and process their information is vitally important to prevent bad data being injected into your information space. The algorithm highlights how you should dereference that data safely. Even if you were operating in TURTLE, the general algorithm wouldn't change AFAICT.

I think this is one of those things that the RDF community hasn't spent much time on. Many RDF datasets are fully trusted, that is, all the information is dumped together because it's all "true". The problem space we're in w/ VCs and DIDs, however, cannot make that assumption. In fact, it makes the opposite assumption -- you can't trust any of this information unless you painstakingly verify it. IOW, nothing gets into your information space without explicit vetting. You presume the default mode of operation is that adversaries are trying to poison your RDF data, which is quite different from many other usages of RDF where you presume you're operating with trusted data sets.

@msporny
Copy link
Member

msporny commented Oct 20, 2024

@filip26 wrote:

if a verifier does not have a policy/process to decide whenever to trust a key provided by the method or not - embedded or not, it does not matter.

Yes, and in addition to that part of the policy/process for retrieving a verification method is specifically defined in this algorithm:

https://w3c.github.io/controller-document/#retrieve-verification-method

@iherman
Copy link
Member Author

iherman commented Oct 20, 2024

Yep, I have not seen that note before, and it clarifies things.

Hm. Almost 😅. The note says:

Verification method identifiers are expressed as strings that are URLs, or via the id property, whose value is a URL.

which sounds good. But the specification of the Verification Method says:

verificationMethod
The verificationMethod property is OPTIONAL. If present, the value MUST be a set of verification methods, where each verification method is expressed using a map.

In my reading, this makes the following not conforming:

{
   
  "verificationMethod": "https://example.org/whatever"
  
}

And this contradicts your note.

@msporny
Copy link
Member

msporny commented Oct 20, 2024

@iherman wrote:

And this contradicts your note.

It doesn't... :) because the language says: "verification method identifiers" ... and the thing you pointed to is a "verification method definition" :)

Yes, I know, it's so subtle it's easily missed... :) and I wouldn't be surprised if some part of the spec doesn't call it the right thing. I haven't yet done a full editorial pass on the document with the intention to make the language easier to understand. Some of these security concerns are so subtle that they're difficult to capture in language... and perhaps better "enforced" in a test suite.

@iherman
Copy link
Member Author

iherman commented Oct 20, 2024

@iherman wrote:

Another point, though, that does not have a bearing on the specification: the differentiation that you describe in very JSON specific. It does not mean anything when you reason about RDF graphs directly: a resource is either a "real" one with a URL or a blank node, and that is it...

Hmm, I'm going to assert that it's not JSON specific at all. :)

Ok. Let us fight! 😀 (Knowing that this is a side-issue with respec to this specification.)

What matters is how that information got into your information space (in the most abstract sense of that word). That is, if you let bad data in, you are compromised no matter whether you're processing "as JSON" or "as RDF" or anything else. The algorithm is about not letting bad data in, that is, the order in which you dereference the URLs and process their information is vitally important to prevent bad data being injected into your information space. The algorithm highlights how you should dereference that data safely. Even if you were operating in TURTLE, the general algorithm wouldn't change AFAICT.

I understand what you are saying. And that means that such standard features must be part of an algorithmic specification on how one uses a specific set of triples (the DI ones in this space). And that is fine. But, in this respect, you make a specification (or, shall we say, a standard restriction) at two different places in this spec. You say something in the algorithmic part that defines how a particular data is to be used by the application, but you also put a restriction on the particular serialization of the data, making use of the specificities of that particular serialization. You could not do that if the DI spec was translated for general RDF use (while you could translate the algorithmic part).

@msporny
Copy link
Member

msporny commented Oct 20, 2024

@iherman wrote:

Ok. Let us fight! 😀

:P

You could not do that if the DI spec was translated for general RDF use (while you could translate the algorithmic part).

Ah, ok, I see what you're saying. We'd have to re-write the "Retrieve Verification Method" algorithm to purely deal in quads. So, instead of saying things like "If controllerDocument.id does not match the controllerDocumentUrl", we'd have to say something like: "If the subject of the triples in the controllerDocument graph does not match the controllerDocumentUrl", right?

@iherman
Copy link
Member Author

iherman commented Oct 20, 2024

@iherman wrote:

Ok. Let us fight! 😀

:P

You could not do that if the DI spec was translated for general RDF use (while you could translate the algorithmic part).

Ah, ok, I see what you're saying. We'd have to re-write the "Retrieve Verification Method" algorithm to purely deal in quads. So, instead of saying things like "If controllerDocument.id does not match the controllerDocumentUrl", we'd have to say something like: "If the subject of the triples in the controllerDocument graph does not match the controllerDocumentUrl", right?

Yes, and this points at something more general: this spec refers to the 'id' property as if it was just another property, which is an artifact of JSON-LD. From a quad point of view, this does not really make sense; that would need rewriting.

But, also, from RDF point of view, these two (JSON-LD) extracts are undistinguishable, so you cannot make a statement about them in the standard:

"property" : "https://a.b.c.d"
"property": {
    "@id": "https://a.b.c.d"
}

which the current specs does.

@iherman
Copy link
Member Author

iherman commented Oct 20, 2024

@iherman wrote:

And this contradicts your note.

It doesn't... :) because the language says: "verification method identifiers" ... and the thing you pointed to is a "verification method definition" :)

Ah.

Yes, I know, it's so subtle it's easily missed... :)

That is an understatement...😀 I think it is worth making things a little bit more explicit in the specification of the property.

and I wouldn't be surprised if some part of the spec doesn't call it the right thing.

That I do not know, but that subtlety is, well, too subtle for my taste (though you are right about it).

I haven't yet done a full editorial pass on the document with the intention to make the language easier to understand. Some of these security concerns are so subtle that they're difficult to capture in language... and perhaps better "enforced" in a test suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR1 This item was processed during the first Candidate Recommendation phase. pending close (7 days) This issue will be closed after 7 days. question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants