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

Protected term errors when supporting v1 and v2 #1150

Closed
OR13 opened this issue Jun 12, 2023 · 17 comments
Closed

Protected term errors when supporting v1 and v2 #1150

OR13 opened this issue Jun 12, 2023 · 17 comments

Comments

@OR13
Copy link
Contributor

OR13 commented Jun 12, 2023

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
Copy link
Contributor

excellent catch @OR13

@OR13
Copy link
Contributor Author

OR13 commented Jun 12, 2023

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
Copy link
Contributor Author

OR13 commented Jun 12, 2023

🔥 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
Copy link
Contributor

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
Copy link
Member

TallTed commented Jun 12, 2023

@OR13don'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
Copy link
Contributor Author

OR13 commented Jun 12, 2023

@TallTed how would you suggest disabling the above code?

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

@OR13
Copy link
Contributor Author

OR13 commented Jun 28, 2023

Related to #1158

@OR13
Copy link
Contributor Author

OR13 commented Jun 28, 2023

Must be done before CR, because of the context being normative, see also #1149

@TallTed
Copy link
Member

TallTed commented Jun 28, 2023

[@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
Copy link
Member

iherman commented Jun 28, 2023

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

  • no resolutions were taken
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.

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
Copy link
Contributor Author

OR13 commented Jun 28, 2023

@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
Copy link
Contributor

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:

#938
#1214
w3c/vc-data-integrity#193
w3c/vc-bitstring-status-list#69

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

@iherman
Copy link
Member

iherman commented Aug 17, 2023

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

  • no resolutions were taken
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.

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.

1 similar comment
@iherman
Copy link
Member

iherman commented Aug 22, 2023

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

  • no resolutions were taken
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.

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
Copy link
Contributor

This will be addressed by #1259.

@msporny
Copy link
Member

msporny commented Aug 30, 2023

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

@msporny
Copy link
Member

msporny commented Sep 13, 2023

PR #1259 is merged, closing.

@msporny msporny closed this as completed Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants