auth0 / node-jws

JSON Web Signatures
http://self-issued.info/docs/draft-ietf-jose-json-web-signature.html
MIT License
709 stars 108 forks source link

Failure to correctly serialize ECDSA signatures #20

Closed dlg-yahoo closed 9 years ago

dlg-yahoo commented 9 years ago

When serializing an ECDSA signature, the correct procedure is to form two fixed-length bigints, each of octet length equal to the length of the representation of the prime which the curve is defined modulo to.

This package does not do so. E.g., try doing:

> jws.sign({header:{alg:'ES256'},payload:'testing',privateKey:key}).split('.')[2].length
94
> jws.sign({header:{alg:'ES256'},payload:'testing',privateKey:key}).split('.')[2].length
95

where

key = '-----BEGIN EC PRIVATE KEY-----\nMHcCAQEEIN8L9YcwuTY6DotH/gc8xP+oemVRj/uTQO6GjnLjSlgIoAoGCCqGSM49\nAwEHoUQDQgAE4nnUjvSzANAZ4l4wrWgpCPqEMDGoCPnEpm8093dDh6Uoww62quMA\nnLCpUSU56HuL5F0dHwuRXOdbQqo9P3hR6g==\n-----END EC PRIVATE KEY-----\n';

The length of the final part should always be the same, regardless of the numerical value of r or s. (In this case, ((4 * 64) + 2) // 3 == 86, so...I'm fairly confused by what this package is doing exactly.

timmclean commented 9 years ago

Isn't this a side effect of ASN.1 encoding? It outputs an ASN.1-encoded sequence consisting of two integers. ASN.1 integers use two's complement, which would cause the length to vary.

dlg-yahoo commented 9 years ago

See JWS A.3.1. JWS does not ASN.1 encode r and s. In fact, I'm not aware of any standardized serialization of ECDSA signatures that encodes them in this way.

Drawing from the example on page 42:

> len('''     DtEhU3ljbEg8L38VWAfUAqOyKAM6-Xx-F4GawxaepmXFCgfTjDxw5djxLa8ISlSA
     pmWQxfKTUJqPP3-Kg6NU1Q'''.replace(' ', '').replace('\n', ''))
86

(As a note: Two's complement is irrelevant, since these integers are always positive. But ASN.1 integers are, indeed, variable-length. They just aren't used by JWS.)

timmclean commented 9 years ago

Ah, good point, so this library is returning ASN.1 sigs against spec. That seems to be the default behaviour of Node's crypto library. I think signature creation happens here.

timmclean commented 9 years ago

Minor problem, but it would cause compatibility problems with other implementations.

coruus commented 9 years ago

It is not a 'minor problem'. This library purports to implement ECDSA JWS tokens. It implements something else and calls them ECDSA JWS tokens. It is perfectly fine for a library to define an arbitrary serialization format, but it is not okay for it to re-use the name or something it doesn't implement.

(@dlg-yahoo c'est moi)

timmclean commented 9 years ago

OK then.

coruus commented 9 years ago

@timmclean: And, sorry for my irritability: Thank you for digging up where the problem actually is!

omsmith commented 9 years ago

See https://github.com/brianloveswords/node-jwa/issues/3 and a proposed fix at https://github.com/brianloveswords/node-jwa/pull/5

omsmith commented 9 years ago

@coruus Should be fixed with the release of node-jwa@1.0.1

coruus commented 9 years ago

Awesome! I'll give it a try. (Sorry, I didn't manage to find the time to fix it nicely enough to submit a PR...) Thank you; some former co-workers will really appreciate this!