Fix ECDSA Signature Unwrapping #594

nlordell · 2024-07-19T22:52:43Z

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¹ that:

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.

Convert r to an octet string of length n and append this sequence of bytes to result.

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²:

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:

shall not all be ones; and

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¹).

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 · 2024-07-20T02:39:21Z

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.

packages/server/src/helpers/iso/isoCrypto/unwrapEC2Signature.ts

MasterKale · 2024-07-20T03:39:58Z

packages/server/src/helpers/iso/isoCrypto/unwrapEC2Signature.ts

+  const iBytes = new Uint8Array(i);
+
+  const normalizedBytes = new Uint8Array(n);
+  if (iBytes.length <= n) {
+    normalizedBytes.set(iBytes, n - iBytes.length);
+  } else if (iBytes.length === n + 1 && iBytes[0] === 0 && (iBytes[1] & 0x80) === 0x80) {
+    normalizedBytes.set(iBytes.slice(1));


I think it's possible to reduce the number of copies of the input bytes that are made if there's more consistent use of .slice() (I'm assuming the method arguments are renamed as I requested above):

Suggested change

const iBytes = new Uint8Array(i);

const normalizedBytes = new Uint8Array(n);

if (iBytes.length <= n) {

normalizedBytes.set(iBytes, n - iBytes.length);

} else if (iBytes.length === n + 1 && iBytes[0] === 0 && (iBytes[1] & 0x80) === 0x80) {

normalizedBytes.set(iBytes.slice(1));

const normalizedBytes = new Uint8Array(componentLength);

if (bytes.length <= componentLength) {

normalizedBytes.set(bytes.slice(0), componentLength - bytes.length);

} else if (bytes.length === componentLength + 1 && bytes[0] === 0 && (bytes[1] & 0x80) === 0x80) {

normalizedBytes.set(bytes.slice(1));

I implemented something I'm happy with here, where the underlying buffer only gets copied if the signature component needs to be extended. In the more common cases with the signature components are already the correct length (or have a leading 0), we reuse the underlying buffer without copying.

Note that instead of using slice here, I opted for subarray so that the underlying buffer does not get copied.

MasterKale · 2024-07-20T03:46:21Z

packages/server/src/helpers/iso/isoCrypto/unwrapEC2Signature.ts

+  } else if (iBytes.length === n + 1 && iBytes[0] === 0 && (iBytes[1] & 0x80) === 0x80) {
+    normalizedBytes.set(iBytes.slice(1));
+  } else {
+    throw new Error("invalid signature component length");


Let's add a bit more debugging info to the error message:

Suggested change

throw new Error("invalid signature component length");

throw new Error(`invalid signature length: ${bytes.length} bytes (component length: ${componentLength}`);

I would still say "invalid signature component length" since this is the length of the r or s value and not the whole signature.

Adding more debugging info makes sense!

Ah, r and s are called "components", one day I'll finish this Real-World Cryptography book I swear 🫠

I'll defer to you on exact verbiage here, my intent here was to add lengths to the error message to help library users better troubleshoot this error 😅

Ah, r and s are called "components"

Just for transparency, this isn't a technical term I found in literature, just a more general term that fits here... I tried to find what people actually call r and s individually, but couldn't find any real consensus.

I added a bit more text to the comments so the meaning of "component" hopefully clearer

nlordell · 2024-07-20T08:25:59Z

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 · 2024-07-20T08:47:59Z

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 · 2024-07-21T07:37:49Z

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.

Co-authored-by: Matthew Miller <[email protected]>

MasterKale · 2024-07-21T23:03:52Z

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:

SimpleWebAuthn/deno.jsonc

Lines 1 to 6 in a169def

    
           { 
        
             "fmt": { 
        
               "singleQuote": true, 
        
               "lineWidth": 100 
        
             } 
        
           }

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.

Formatted with `deno fmt --config deno.jsonc`

nlordell · 2024-07-22T07:40:54Z

Thanks for the tip! Changes should be formatted now.

MasterKale · 2024-07-23T04:18:58Z

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

MasterKale · 2024-07-23T04:46:55Z

@nlordell I've released @simplewebauthn/[email protected] that has this fix in it. Thank you again! 🚀

nlordell added 2 commits July 19, 2024 22:17

Fix ECDSA Signature Unwrapping

8702b2c

Add Unit Tests

2e96308

nlordell mentioned this pull request Jul 19, 2024

Ambigious behaviour with WebAuthn shim safe-global/safe-modules#369

Closed

Stricter Leading 0 Checks

6e56a1c

MasterKale reviewed Jul 20, 2024

View reviewed changes

packages/server/src/helpers/iso/isoCrypto/unwrapEC2Signature.ts Outdated Show resolved Hide resolved

MasterKale reviewed Jul 20, 2024

View reviewed changes

nlordell requested a review from MasterKale July 21, 2024 07:37

Implement PR Review suggestions

215dc5d

Co-authored-by: Matthew Miller <[email protected]>

nlordell force-pushed the fix-ecdsa-extraction branch from 83d7161 to 215dc5d Compare July 21, 2024 07:40

Format Source Files

8411b7e

Formatted with `deno fmt --config deno.jsonc`

MasterKale approved these changes Jul 23, 2024

View reviewed changes

MasterKale merged commit aa947c2 into MasterKale:master Jul 23, 2024
1 check passed

MasterKale added the package:server @simplewebauthn/server label Jul 23, 2024

MasterKale added this to the v10.0.1 milestone Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ECDSA Signature Unwrapping #594

Fix ECDSA Signature Unwrapping #594

nlordell commented Jul 19, 2024 •

edited

Loading

MasterKale commented Jul 20, 2024

MasterKale Jul 20, 2024 •

edited

Loading

nlordell Jul 21, 2024

MasterKale Jul 20, 2024 •

edited

Loading

nlordell Jul 20, 2024

MasterKale Jul 20, 2024

nlordell Jul 21, 2024

nlordell Jul 21, 2024

nlordell commented Jul 20, 2024 •

edited

Loading

nlordell commented Jul 20, 2024

nlordell commented Jul 21, 2024

MasterKale commented Jul 21, 2024 •

edited

Loading

nlordell commented Jul 22, 2024

MasterKale commented Jul 23, 2024

MasterKale commented Jul 23, 2024

	throw new Error("invalid signature component length");
	throw new Error(`invalid signature length: ${bytes.length} bytes (component length: ${componentLength}`);

Fix ECDSA Signature Unwrapping #594

Fix ECDSA Signature Unwrapping #594

Conversation

nlordell commented Jul 19, 2024 • edited Loading

Footnotes

MasterKale commented Jul 20, 2024

MasterKale Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

nlordell Jul 21, 2024

Choose a reason for hiding this comment

MasterKale Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

nlordell Jul 20, 2024

Choose a reason for hiding this comment

MasterKale Jul 20, 2024

Choose a reason for hiding this comment

nlordell Jul 21, 2024

Choose a reason for hiding this comment

nlordell Jul 21, 2024

Choose a reason for hiding this comment

nlordell commented Jul 20, 2024 • edited Loading

nlordell commented Jul 20, 2024

nlordell commented Jul 21, 2024

MasterKale commented Jul 21, 2024 • edited Loading

nlordell commented Jul 22, 2024

MasterKale commented Jul 23, 2024

MasterKale commented Jul 23, 2024

nlordell commented Jul 19, 2024 •

edited

Loading

MasterKale Jul 20, 2024 •

edited

Loading

MasterKale Jul 20, 2024 •

edited

Loading

nlordell commented Jul 20, 2024 •

edited

Loading

MasterKale commented Jul 21, 2024 •

edited

Loading