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

Fix, derToRaw does not handle r/s with leading 0s. #117

Merged
merged 9 commits into from Dec 31, 2022

Conversation

codydfns
Copy link
Contributor

@codydfns codydfns commented Dec 30, 2022

Fixes: 118

What Changed
Updating the derToRaw function to handle signature parts that have leading (excluded) zeros.

Details of the Issue
The R and S in ASN.1/DER are 32 byte signed numbers. There are two properties that need to hold true:

  1. The number needs to be a positive number, so if the first byte of the number is a negative value, ASN.1/DER will pad the number with a zero. For example, abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123000000 will be represented as 00abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123000000.
  2. The number is minimized to remove leading zeros. For example, 000000abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123 will be represented as 00abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123 (leading 00 because ab is negative).

When converting the number, you need to be mindful of the actual length of the number, to avoid overflowing into the other number.

Raw/IEEE P1363 requires the r/s values to be non-minimized unsigned values (always 32 bytes), so we need to add back the leading zeros to the number to make it 32 bytes when we return the value.

A good explanation of ASN.1/Der can be found here:

When encoded in DER, this becomes the following sequence of bytes:

0x30 b1 0x02 b2 (vr) 0x02 b3 (vs)
where:

b1 is a single byte value, equal to the length, in bytes, of the remaining list of bytes (from the first 0x02 to the end of the encoding);
b2 is a single byte value, equal to the length, in bytes, of (vr);
b3 is a single byte value, equal to the length, in bytes, of (vs);
(vr) is the signed big-endian encoding of the value "𝑟", of minimal length;
(vs) is the signed big-endian encoding of the value "𝑠", of minimal length.

@codecov-commenter
Copy link

codecov-commenter commented Dec 30, 2022

Codecov Report

Base: 92.94% // Head: 92.95% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (6fd3d1e) compared to base (2ebe5fa).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #117   +/-   ##
=======================================
  Coverage   92.94%   92.95%           
=======================================
  Files          16       16           
  Lines        5984     5992    +8     
=======================================
+ Hits         5562     5570    +8     
  Misses        422      422           
Impacted Files Coverage Δ
lib/toolbox.js 90.05% <100.00%> (+0.23%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@codydfns codydfns changed the title derToRaw does not handle r/s with leading 0s. Fix, derToRaw does not handle r/s with leading 0s. Dec 30, 2022
lib/toolbox.js Outdated Show resolved Hide resolved
deno.jsonc Outdated Show resolved Hide resolved
test/asn1.test.js Outdated Show resolved Hide resolved
@JamesCullum
Copy link
Member

Thanks for your contribution! It'd be great if you could consider my comments and bring the changes and tests more in line with the existing codebase 👍

@JamesCullum JamesCullum marked this pull request as draft December 30, 2022 08:51
…ript native types for padding the array in derToRaw.
@codydfns codydfns marked this pull request as ready for review December 30, 2022 19:03
test/asn1.test.js Outdated Show resolved Hide resolved
test/asn1.test.js Outdated Show resolved Hide resolved
@JamesCullum
Copy link
Member

Thanks a lot for the updates - really clean and good solution! 💯

Do you want to do the celebratory version bump (to 3.3.5) in the package.json for the automated deployment? Happy new year to you, by the way!

@codydfns
Copy link
Contributor Author

Sounds good. Thanks @JamesCullum! Happy new year to you as well!

@JamesCullum JamesCullum merged commit 7598e31 into webauthn-open-source:master Dec 31, 2022
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

Successfully merging this pull request may close these issues.

In some cases ECDSA signatures are not validated properly.
3 participants