IMPORTANT: Tampered with VC documents still pass validation

[anvabr]

Problem description

Our project heavily relies on transmute libraries to operate with VCs and related concepts. The standard mandate that VC documents contain "credentialSubject" properties, which are populated with data and some 'service' attributes, the example is shown below:

"credentialSubject": [
  {
    "field1": 123123,
    "@context": [
        "https://ipfs.io/ipfs/bafkreidfufvsbnreh6mitnzleSmyltnxwikik51zbayfrm2j3qdstuw34j"
     ],
    "id": "3841453-234a-425e-924c-820c2904eff4",
    "type":"Test",
  }
]

The id element is required by the standard to have values in URI format, however this requirement is not followed by some of our users as in the example above. It appears that such mis-formatting of the content of id field makes the validation library to create signatures which then always accepted as 'valid' regardless of the changes in the content of the element.

Step to reproduce

Steps to reproduce the behavior:

1. Create a VC document with mis-formatted id element as per above
2. Generate signature
3. Change value of the field1 attribute (or any other attribute in your document)
4. Call validation function in the transmute library, observe the 'valid' status reported

Expected behaviour

The library needs to report signature as invalid when the document has changed, regardless of the correctness of formatting of the original document.

Tooling and/or screenshots

Please see this open source repo where one of our developers created a tool demonstrating the problem. https://github.com/artembuslaev/vc-signature-problem

[ipbyrne]

Hello @anvabr !

Thank you for bringing this issue to our attention. We will investigate this further and post an update here as soon we have one.

[anvabr]

@ipbyrne thank you for the quick note! I've changed the title a bit - to reflect the severity of the issue from our perspective. And only then noticed your note. I hope you don't mind, it's serious and urgent for us! There is another issue in the works too, our dev is saying that both are possibly in a 3rd party library your are using internally.

[Neurone]

@ipbyrne I think the problem originates inside the jsonld.js library at this line:

// skip relative IRI subjects (not valid RDF)
if(!_isAbsoluteIri(id)) {
    continue;
}

This makes the dataset variable in the canonize function to be void:

  const dataset = await jsonld.toRDF(input, opts);

  // do canonicalization
  return canonize.canonize(dataset, options);

That void value is passed to another library (rdf-canonize) that again return void with a void dataset parameter.

Subsequently, you use the wannabe canonized value in the createVerifyData function to create the payload to be verified later, hashing a void value:

const c14nDocument = await this.canonize(document, {
   documentLoader,
   expansionMap
});
return Buffer.concat([sha256(c14nProofOptions), sha256(c14nDocument)]);

And that payload is always valid with the test signature we provided in the example, regardless of its actual content.

I tried to upgrade to the latest jsonld - v8.1.0, you are currently using the v5.2.0 - but it does not seem to solve the problem.

A quick and dirty solution could be to check for that value to be not null, throwing an exception otherwise (and, in that case, the validation is returned as false)

[ipbyrne]

Hello, @Neurone and @anvabr I just wanted to confirm that I have been able to successfully replicate the issue and am currently working on a fix now. I will update you both on this ticket once we have released the fix so you can do a version bump to resolve the issue on your end.

Thanks again for taking the time to report in detail what you both were seeing, we really appreciate it!

[ipbyrne]

I have confirmed that the jsonld.toRDF is the culprit as you mentioned since it returns an empty array ([]) resulting in dropped nquads when you canonize the credential.

Below is a comparison of the outputs when canonizing a good credential (uses urn:uuid in credential subject id vs a bad one that doesn't):

Good:

<urn:uuid:2cb13e3f-c3dd-4cf9-b7e2-a7378fc88173> <http://www.w3.org/1999/02/22-rdf-syntax-ns#type> <https://w3id.org/traceability/#undefinedTerm#c6f68dcd-d02d-4eb4-8787-66a57a4ce00f> .
    <urn:uuid:2cb13e3f-c3dd-4cf9-b7e2-a7378fc88173> <https://www.schema.org/text> "1" .
    <urn:uuid:2cb13e3f-c3dd-4cf9-b7e2-a7378fc88173> <https://www.schema.org/text> "2" .
    _:c14n0 <http://purl.org/dc/terms/created> "2023-01-18T09:15:54Z"^^<http://www.w3.org/2001/XMLSchema#dateTime> _:c14n1 .
    _:c14n0 <http://www.w3.org/1999/02/22-rdf-syntax-ns#type> <https://w3id.org/security#Ed25519Signature2018> _:c14n1 .
    _:c14n0 <https://w3id.org/security#jws> "eyJhbGciOiJFZERTQSIsImI2NCI6ZmFsc2UsImNyaXQiOlsiYjY0Il19..bGcoUsYebRKV_6P26SZviaiAJ4gt6qTVrIz-fWRgxvzTbTBzJJjjQzDEIXXp8ReWnIKgUy5882t_35852hC6CQ" _:c14n1 .
    _:c14n0 <https://w3id.org/security#proofPurpose> <https://w3id.org/security#assertionMethod> _:c14n1 .
    _:c14n0 <https://w3id.org/security#verificationMethod> <did:key:z6MktWjP95fMqCMrfNULcdszFeTVUCE1zcgz3Hv5bVAisHgk#z6MktWjP95fMqCMrfNULcdszFeTVUCE1zcgz3Hv5bVAisHgk> _:c14n1 .

Bad:

_:c14n1 <http://purl.org/dc/terms/created> "2023-01-18T09:15:54Z"^^<http://www.w3.org/2001/XMLSchema#dateTime> _:c14n0 .
    _:c14n1 <http://www.w3.org/1999/02/22-rdf-syntax-ns#type> <https://w3id.org/security#Ed25519Signature2018> _:c14n0 .
    _:c14n1 <https://w3id.org/security#jws> "eyJhbGciOiJFZERTQSIsImI2NCI6ZmFsc2UsImNyaXQiOlsiYjY0Il19..9oJ-3GO-KfxKZFq7GCWQ_BCAj5_bRLungBSqecGhLQWO9Qb0E2ThCB1n8LFqTof86LfuXBLPZQOs2Ux7WssQDQ" _:c14n0 .
    _:c14n1 <https://w3id.org/security#proofPurpose> <https://w3id.org/security#assertionMethod> _:c14n0 .
    _:c14n1 <https://w3id.org/security#verificationMethod> <did:key:z6MktWjP95fMqCMrfNULcdszFeTVUCE1zcgz3Hv5bVAisHgk#z6MktWjP95fMqCMrfNULcdszFeTVUCE1zcgz3Hv5bVAisHgk> _:c14n0 .

To fix this we are going to implement a wrapper around the canonize function to make it "safe" so it will throw an error when necessary.

[anvabr]

@ipbyrne Thank you for keeping us posted, and we are looking forward to deploying the solution. Please could I get some idea on the timelines for this fix?

[anvabr]

Please see also #230

[ipbyrne]

Hey @anvabr I should have this issues resolved today. I will look at the other 2 mentioned as well.

[anvabr]

HI @ipbyrne, I hope you had a good week-end. I am just checking in for updates if you have any on this and #230 and #231?

[ipbyrne]

Hey @anvabr!

I apologize about the delay in getting the fix rolled out. I am hoping to have it published this afternoon. If not then it should be published by tomorrow.

We are wrapping the jsonld library to fix the issue with a ‘safeCanonize’ that we will then implement into our Verifiable Data packages.

Once our wrapped library is published I will get this one updated and you should be able to bump the package version in project.

I will report back hear as soon as all of this is complete.

[ipbyrne]

Hey @anvabr !

So I have just pushed out a fix that resolves this issue. Using the example repo you provided, if you update all of the packages to our latest version the following tests now pass:

test vc
    ✔ create vc and verify VC with VALID id (111ms)
    2) create vc and verify VC with INVALID id
    3) create vc with bbs signature and verify VC with VALID id 
    ✔ create vc with bbs signature and verify VC with VALID id with mattrglobal library (73ms)
    ✔ remove field and verify signed VC with VALID ID
    ✔ remove field and verify signed VC with INVALID ID

For test #2: This one should fail now as we prevent signing data that is not valid JSON-LD so seeing the test fail is expected. It now should give the following error:

Error: ["Invalid JSON-LD ID at /id. Using this value would allow the data in the object to be mutable.","Invalid JSON-LD ID at /credentialSubject/0/id. Using this value would allow the data in the object to be mutable."]

For test #3: This should pass but is still failing so I believe it's related to the other BBS issues you raised. I am currently working on those and will add updates to those issues.

I am going to close this issue out since it has fixed the issue of preventing credentials from being mutable with our libraries.