Fix ECDSA Signature Unwrapping

[nlordell]

We ran into some unexpected and inconsistent signature verification errors. After some digging I traced it down to what I believe is an issue in the unwrapEC2Signature method.

From what I understood, this is responsible for taking the ASN.1 encoded ECDSA signature, and encoding it into a format that the WebCrypto library expects (which is r ‖ s - where ‖ is the byte concatenation operator). In particular the Web Crypto API documents^1 that:

4. Let n be the smallest integer such that n * 8 is greater than the logarithm to base 2 of the order of the base point of the elliptic curve identified by params.
5. Convert r to an octet string of length n and append this sequence of bytes to result.
6. Convert s to an octet string of length n and append this sequence of bytes to result.

So, this means that the signature components have very specific expected byte lengths when "unwrapping" the signature for use with the Web Crypto "subtle" API. In practice, I found that it was not always producing signatures of the correct length, and in particular the rBytes and sBytes values were not always of length n as defined above. Specifically, I was working with P-256 signatures and noticed that sometimes r and/or s had a length of 31 instead of 32 (which is the smallest byte-length required to represent an integer equal to the order of the P-256 curve).

Looking into ASN.1 specification^2:

8.3.2 If the contents octets of an integer value encoding consist of more than one octet, then the bits of the first octet and bit 8 of the second octet:

1. shall not all be ones; and
2. shall not all be zero.

NOTE – These rules ensure that an integer value is always encoded in the smallest possible number of octets.

After more closely inspecting the signature I was using, it would occasionally produce signatures where the MSB of the r or s value was 0, which would cause the BER encoding of the signature component to be shorter than expected. From the P-256 test case, the signature in hex can be broken down to:

# SEQUENCE(03) of length 0x44(68)
30 44
# INTEGER(03) of length 0x1f(31)
02 1f 587f17b4cf4cc561fe4c1bfecb230f4ff653bedb6d76b3cf6bb1f8fea484b4
# INTEGER(03) of length 0x31(33)
02 21 00aeb47b3d78206408830fdf203d9a63512847c577e4449a1198d866c7a33682ef

Note that, the leading 00 byte from the signature r value is removed in the ASN.1 encoding, while an additional leading 00 byte is added to the s value (so that it isn't confused with 02 20 aeb47b3d78206408830fdf203d9a63512847c577e4449a1198d866c7a33682ef, which would represent -0x514b84c287df9bf77cf020dfc2659caed7b83a881bbb65ee672799385cc97d11).

This PR fixes the unwrapEC2Signature implementation in order such that it computes r ‖ s where the byte length of r and s are guaranteed to be the same (and specifically n as it is defined in the Web Crypto API specification^1).

I added some test vectors to make sure things work (that would fail on master). I also added a P-521 test vector which does not work (as it does not seem to be implemented by Deno). I left it in as ignored as the vector itself should be correct (as manually porting the test to run in my browser passed) and it might be nice to add once P-521 is supported.

[MasterKale]

Hello @nlordell, thank you very much for the submission. My past-self has required me to start off by saying that this project is not open to external contributions as per the README.

That said, given that I don't work with WebCrypto day-to-day, the code in the PR looks high quality to me, and your rationale is explained quite well, I'm going to make an exception. Crypto is hard to do right and it seems to be dumb luck that I've managed to get away this long with EC2 signature wrapping as it's been.

Thank you again for bringing this to my attention, and for helping improve use of WebCrypto in SimpleWebAuthn 🙇

Tests are passing so I'm going to start reviewing the PR "this weekend" (I might try for tonight, otherwise sometime before Monday.) After a quick perusal just now I have only a few nitpicks here and there, otherwise the bulk of the code looks good to me.

[nlordell]

My past-self has required me to start off by saying that this project is not open to external contributions as per the README.

Oh shoot, I'm really sorry, should have checked before making a PR 🙈. With this in mind, if you feel strongly about sticking to your rule, feel free to close this PR and implement the change as you see fit (and borrow some of the code from the PR if you like) - in a way consider this PR as a detailed issue report 😅. I definitely won't be disappointed and am happy in being able to contribute a detailed bug finding 😄.

dumb luck that I've managed to get away this long with EC2 signature wrapping as it's been.

Just some wild speculation, but maybe ECDSA credentials aren't as widely used as EdDSA ones? Also, it only happens when the leading 8 bits are 0, so a roughly 1/256 chance (well, a little less because r and s aren't in the full range of a 256-bit integer), so it is fairly intermittent. It could also be that some BER encoding implementations are including leading 0s in the ASN.1 signature encoding, so it does not cause issues with the code on master (at least when I played with online decoders, they happily accept leading 0s in ASN.1 BER encoded integers).

[nlordell]

Thanks for the thoughtful review! I will squash the suggestions into a single commit and play around with your idea of reducing input byte copies when I'm back home tonight.

[nlordell]

I implemented the suggestions from the PR review - quick question, do you have any preference for formatting for the file? I figured you were using deno fmt, but this changes the quoting style to use double quotes which is different from the neighboring source files.

Let me know if you want me to run an automated code formatter on this.

[MasterKale]

I implemented the suggestions from the PR review - quick question, do you have any preference for formatting for the file? I figured you were using deno fmt, but this changes the quoting style to use double quotes which is different from the neighboring source files.

Let me know if you want me to run an automated code formatter on this.

Looking at my command history, in the past I've run this from the root of the monorepo:

$> deno fmt packages/server/src/

It's because it's the root deno.jsonc file that defines single quotes and line length:

https://github.com/MasterKale/SimpleWebAuthn/blob/a169def3c663cb671cdc6bc6e00a4993944a61ae/deno.jsonc#L1-L6

Husky git pre-commit hooks are supposed to take care of this for you but I'm not surprised they didn't work for you - I haven't had great confidence in them in the past and it seems not much has improved about them.

[nlordell]

Thanks for the tip! Changes should be formatted now.

[MasterKale]

@nlordell This all looks good to me, thank you for lending this project some of your SubtleCrypto expertise 🙇

[MasterKale]

@nlordell I've released @simplewebauthn/server@10.0.1 that has this fix in it. Thank you again! 🚀