w3c / vc-data-model

W3C Verifiable Credentials v2.0 Specification
https://w3c.github.io/vc-data-model/
Other
293 stars 106 forks source link

Protected term errors when supporting v1 and v2 #1150

Closed OR13 closed 1 year ago

OR13 commented 1 year ago

Consider:

{
  "@context": ["https://www.w3.org/ns/credentials/v2"],
  "type": ["VerifiablePresentation"],
  "verifiableCredential": [
    {
      "@context": ["https://www.w3.org/2018/credentials/v1"],
      "type": ["VerifiableCredential"]
    }
  ]
}

jsonld.SyntaxError: Invalid JSON-LD syntax; tried to redefine a protected term.

mprorock commented 1 year ago

excellent catch @OR13

OR13 commented 1 year ago

I was able to work around this issue, by removing the v2 context from the outer object:

const multiPresentationSupport = {
        '@context': {'@vocab': 'https://www.w3.org/2018/credentials#'},
        type: ['VerifiablePresentation', 'VerifiablePresentations'],
        id: `urn:uuid:${uuid.v4()}`,
        holder: process.env.API_BASE_URL as string,
        verifiablePresentation: verifiablePresentations,
      }

As you can see, most of the terms that get defined related to a presentation of a presentation of a mill test report are controlled by origins other than W3C...

Screen Shot 2023-06-12 at 12 17 45 PM

Green are terms defined by w3c.

Red are terms defined by schema.org and w3id.org.

OR13 commented 1 year ago

🔥 Don't be evil. We need to discuss some security issuers here.

Probably a better resolution to the above error

const multiPresentationSupport = {
        '@context': {'@vocab': 'https://www.w3.org/2018/credentials#'},
        type: ['VerifiablePresentation'],
        id: `urn:uuid:${uuid.v4()}`,
        holder: holder,
        verifiableCredential: verifiablePresentations,
      }
Screen Shot 2023-06-12 at 12 28 03 PM

Notice this time the graphs are joined properly, because https://www.w3.org/2018/credentials#verifiableCredential is a known property of a VerifiablePresentation where as verifiablePresentation IS NOT.

Also notice that "issuer dependent terms" are defined relative to w3c, not w3id.org.

This is important since those terms might leak claim vocabulary metadata into the network logs of w3id.org.

For example, users who click https://w3id.org/security#proof leak interest of a particular credential vocabulary being relevant to a referrer, to w3id.org...

Goto to microsoft.com, open console, past:

(async ()=>{
    // comment this code out to remove an undefined term error 
    // and demonstrate that w3id.org sees traffic from referring websites
    // and might be attacked for facilitating term definitions by threat actors that don't like the term being defined
    var term = property.undefined
    await fetch('https://w3id.org/security#proof')
})()
Screen Shot 2023-06-12 at 12 35 12 PM

Notice the referrer in the request headers, and the location in the response headers:

Screen Shot 2023-06-12 at 12 38 31 PM Screen Shot 2023-06-12 at 12 38 22 PM Screen Shot 2023-06-12 at 12 40 01 PM

Because of w3id.org, GitHub learns of microsoft.com interest in security vocabulary... and w3id.org learns term /vocabulary resolution timing related to microsoft.com and github.

You can click this link https://w3id.org/security#proof to make w3id.org care even more about this particular term.

Consider also the dangers, and costs of:

(async ()=>{
    // comment this code out to remove an undefined term error 
    // and demonstrate that w3id.org sees traffic from referring websites
    // and might be attacked for facilitating term definitions by threat actors that don't like the term being defined
    var term = property.undefined
    while(true){   
      await fetch('https://w3id.org/security#proof')
    }
})()

Very quickly this becomes a cost center for the vocabulary service provider.... and the redirection service... (don't run the above code, it will denial of service attack the operators of https://github.com/perma-id/w3id.org

The problem with URLs, is that developers use them... and not always per the specs, despite our best efforts.

We could mitigate some of these issues by using term definitions that are not resolvable, but that will create a different set of usability challenges, since someone would need to look up where the URNs were defined to understand what they mean, for example:

urn:ietf:params:oauth:jwk-thumbprint:sha-256:NzbLsXh8uDCcd-6MNwXF4W_7noWXFZAfHkxZsRGC9Xs

Is not resolvable, but but:

https://w3id.org/security#jwk-thumbprint

is... even though the link above provides no human readable definitions.

dlongley commented 1 year ago

Yeah, we made breaking changes in v2. However, I think we can address this by adding "@context": null as a property-scoped context under the verifiableCredential term in the VerifiablePresentation definition.

This approach works in the JSON-LD playground: https://tinyurl.com/29yye6vc

TallTed commented 1 year ago

@OR13 — don't run the above code

Please put this and similar warnings before the code that should not be run, and optimally break such code making it unexecutable without some fix(es), and include some comment(s) saying that it has been broken, and if you must include some comment(s) explicitly describing how a bad actor could change the code to make it executable.

Note that various crawlers may act on any URLs they encounter, whether or not they're "live" on the page where they're encountered, so you may already be causing trouble for various entities.

OR13 commented 1 year ago

@TallTed how would you suggest disabling the above code?

Should I make the URL invalid so fetch doesn't run properly?

OR13 commented 1 year ago

Related to https://github.com/w3c/vc-data-model/pull/1158

OR13 commented 1 year ago

Must be done before CR, because of the context being normative, see also https://github.com/w3c/vc-data-model/pull/1149

TallTed commented 1 year ago

[@OR13] @TallTed how would you suggest disabling the above code?

By inserting characters or words or comment delimiters or the like, such that the code won't run, optimally at all, but minimally before it does the thing causing you to warn people not to run the code.

[@OR13] Should I make the URL invalid so fetch doesn't run properly?

Sure, that might work. Making it invalid early, such that no GET or similar request will reach a place where you've raised flags about potentially confidentiality-breaking log entries.

iherman commented 1 year ago

The issue was discussed in a meeting on 2023-06-28

View the transcript #### 2.15. Protected term errors when supporting v1 and v2 (issue vc-data-model#1150) _See github issue [vc-data-model#1150](https://github.com/w3c/vc-data-model/issues/1150)._ **Brent Zundel:** # 1150 Orie raised this a few weeks ago. Needs someone assigned and a label. **Orie Steele:** If you make v2 presentation and use a v1 you get an error. … need to address in the normative context. > *Phillip Long:* dlongley - should be addressed in the context - it's one line change to be made to the v2 context. **Brent Zundel:** assigned to dllongley. … it's a wrap. … all of the issues that we have left we have 69 opene issues. **Orie Steele:** correction on scribes part if you make a v2 presentation and us a v1 context you'll get an error. **Brent Zundel:** pending close issues be aware! ---
OR13 commented 1 year ago

@TallTed thanks! I updated the examples to throw undefined term errors that halt execution, in honor of how JSON-LD processors also throw errors when terms are not properly defined in a context.

dlongley commented 1 year ago

This solution to this is in this comment. We should apply this when we make other context changes and rev the normative context hash.

Related PRs for potentially adding or modifying context terms:

w3c/vc-data-model#938 w3c/vc-data-model#1214 w3c/vc-data-integrity#193 https://github.com/w3c/vc-status-list-2021/pull/69

There may be more than that. We might be able to save some effort by making the context updates at once.

iherman commented 1 year ago

The issue was discussed in a meeting on 2023-08-16

View the transcript #### 2.3. Protected term errors when supporting v1 and v2 (issue vc-data-model#1150) _See github issue [vc-data-model#1150](https://github.com/w3c/vc-data-model/issues/1150)._ **Kristina Yasuda:** protected term errors when doing v1 and v2, dave has pointed to multiple PRs in his latest comment, what's plan here? **Dave Longley:** I don't plan to do PR unless it's done as a final pass before CR over all of the other issues we have -- we have a number of things that will need tomake changes to v2 context (other specs, etc) -- we need to make pass at context at the end, tagged this item and others to make sure we get everything in there. **Kristina Yasuda:** waiting for v2 context to be "final final". **Dave Longley:** Not "final final", just right before we go to CR.
iherman commented 1 year ago

The issue was discussed in a meeting on 2023-08-16

View the transcript #### 2.3. Protected term errors when supporting v1 and v2 (issue vc-data-model#1150) _See github issue [vc-data-model#1150](https://github.com/w3c/vc-data-model/issues/1150)._ **Kristina Yasuda:** protected term errors when doing v1 and v2, dave has pointed to multiple PRs in his latest comment, what's plan here? **Dave Longley:** I don't plan to do PR unless it's done as a final pass before CR over all of the other issues we have -- we have a number of things that will need tomake changes to v2 context (other specs, etc) -- we need to make pass at context at the end, tagged this item and others to make sure we get everything in there. **Kristina Yasuda:** waiting for v2 context to be "final final". **Dave Longley:** Not "final final", just right before we go to CR.
dlongley commented 1 year ago

This will be addressed by w3c/vc-data-model#1259.

msporny commented 1 year ago

PR w3c/vc-data-model#1259 has been raised to address this issue. This issue will be closed once w3c/vc-data-model#1259 is merged.

msporny commented 1 year ago

PR https://github.com/w3c/vc-data-model/pull/1259 is merged, closing.