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

Detached JWS do not match #1

Open
OR13 opened this issue Dec 4, 2019 · 6 comments
Open

Detached JWS do not match #1

OR13 opened this issue Dec 4, 2019 · 6 comments

Comments

@OR13
Copy link
Member

OR13 commented Dec 4, 2019

transmute-industries/linked-data-signature-starter-kit#1

I believe there may be an error with encoding before the data is passed to sign.

Its also possible that b64 option is not implemented correctly.

@OR13
Copy link
Member Author

OR13 commented Dec 4, 2019

I created a test which attempts to sign the same data, with the same keys:

https://github.com/transmute-industries/json-ld-sig-detached-jws/blob/master/src/__tests__/Interop.spec.js#L46

@OR13
Copy link
Member Author

OR13 commented Dec 4, 2019

Per the cookbook @panva/jose requires the input be base64url encoded:

https://tools.ietf.org/html/rfc7520#section-4.5.1

However, https://tools.ietf.org/html/rfc7797#section-8

[JWS] base64url-encodes the JWS Payload to restrict the set of
   characters used to represent it so that the representation does not
   contain characters used for delimiters in JWS representations.  Those
   delimiters are the period ('.') character for the JWS Compact
   Serialization and the double-quote ('"') character for the JWS JSON
   Serialization.  When the "b64" (base64url-encode payload) value is
   "false", these properties are lost.  It then becomes the
   responsibility of the application to ensure that payloads only
   contain characters that will not cause parsing problems for the
   serialization used, as described in Section 5.  The application also
   incurs the responsibility to ensure that the payload will not be
   modified during transmission.

I think the issue is with the crypto-ld sign library.... although the need to call .toString is a bit suspicious...

For example:


const header = {
  alg: "EdDSA",
  b64: false,
  crit: ["b64"]
};

const toBeSigned = Buffer.from("1223123123123123123", "hex");

describe("EdDSA", () => {
  it("sign and verify", async () => {
    const flat = jose.JWS.sign.flattened(
      toBeSigned,
      jose.JWK.asKey(fixtures.didKey.privateKeyJwk),
      header
    );
    const verified = await jose.JWS.verify(
      {
        ...flat,
        payload: toBeSigned.toString()
      },
      jose.JWK.asKey(fixtures.didKey.publicKeyJwk),
      {
        crit: ["b64"]
      }
    );
    expect(verified).toBe(toBeSigned.toString());
  });
});

@OR13
Copy link
Member Author

OR13 commented Dec 5, 2019

After testing with:

from jwcrypto import jwk
from jwcrypto import jws
from jwcrypto.common import json_decode, json_encode


keys = {
    "publicKeyJwk": {
        "crv": "Ed25519",
        "x": "h83ufcOAO9zVigHCgOOTp8waN_ycH4xnPRvn45yu6gw",
        "kty": "OKP",
        "kid": "8R_gUPjBoJ_nf39_G7VLGWNuhL5etuW7zvS46kwYN6Q"
    },
    "privateKeyJwk": {
        "crv": "Ed25519",
        "x": "h83ufcOAO9zVigHCgOOTp8waN_ycH4xnPRvn45yu6gw",
        "d": "Mqr_E4EQ51DxzVHh74HEy6F0JIykqDnrwgeJOxiLeTU",
        "kty": "OKP",
        "kid": "8R_gUPjBoJ_nf39_G7VLGWNuhL5etuW7zvS46kwYN6Q"
    }
}


header = {
    "alg": "EdDSA",
    "b64": False,
    "crit": ["b64"]
}


payload = bytes.fromhex('4a4b4c')

s = jws.JWS(payload)
s.add_signature(jwk.JWK.from_json(json_encode(keys['privateKeyJwk'])),
                'EdDSA', json_encode(header))

jws = s.serialize(compact=True)


print(jws)

I'm pretty sure the detached JWS implementation we have here is good, and the issue lies in the jsonld-signatures implementation.

@dlongley
Copy link

dlongley commented Dec 5, 2019

There's a bug in the panva/jose library. I don't have time to discuss much further or do a PR over at panva/jose for this so I'm just dropping this here:

This code makes an incorrect assumption that the JWS payload is a string that has been utf-8 encoded into a buffer:

https://github.com/panva/jose/blob/master/lib/jws/sign.js#L100

That base64url.decode function will decode the base64url encoded payload to get a buffer of bytes and then it assumes those bytes are utf8-encoded characters as it decodes it to a string using toString('utf8'):

https://github.com/panva/jose/blob/master/lib/help/base64url.js#L30

This is an incorrect assumption -- and node.js's Buffer.toString('utf8') produces garbage output as a result, without throwing an error. Note that if new util.TextDecoder were used instead, it would throw an error revealing this problem. The code then takes this garbage string and concatenates it here:

https://github.com/panva/jose/blob/master/lib/jws/sign.js#L106

To correct this, instead of doing this:

      if (!joseHeader.protected.b64) {
        i(this).payload = base64url.decode(i(this).payload)
      }
// ...

recipient.signature = base64url.encodeBuffer(sign(alg, key, Buffer.from(`${recipient.protected}.${i(this).payload}`)))

It should be doing something like this:

      if (!joseHeader.protected.b64) {
        i(this).payload = base64url.decodeToBuffer(i(this).payload)
      }
// ...

recipient.signature = base64url.encodeBuffer(sign(alg, key, Buffer.concat([
  Buffer.from(`${recipient.protected}.`),
  ${i(this).payload}`)]))

Of course, some other minor changes might be necessary to handle the case where joseHeader.protected.b64 is true since this runs through the same code path.

You can see the problem by putting a 128 into the data in the gist I sent before:

https://gist.github.com/dlongley/b19f2723e11d99d2e39d36b6f8ae7e17#file-gistfile1-txt-L15

const data = new Uint8Array([128]);

The signature won't match when you do this. If you use 127 (which will be valid utf-8) the signature will match. I assume a unit test of this sort could be added to panva/jose.

@OR13
Copy link
Member Author

OR13 commented Dec 5, 2019

I have opened an issue here: panva/jose#57

@OR13
Copy link
Member Author

OR13 commented Dec 5, 2019

Proof the raw signatures match and the issue is one of encoding:

https://gist.github.com/dlongley/b19f2723e11d99d2e39d36b6f8ae7e17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants