Skip to content

Use asn1js.Integer.fromBigInt instead of asn1js.Integer#456

Open
SWGovernikus wants to merge 15 commits intoPeculiarVentures:masterfrom
Governikus:master
Open

Use asn1js.Integer.fromBigInt instead of asn1js.Integer#456
SWGovernikus wants to merge 15 commits intoPeculiarVentures:masterfrom
Governikus:master

Conversation

@SWGovernikus
Copy link

The r and s values returned by the WebCryptoAPI are assigned as byte buffers to the asn1js.Integer class (new asn1js.Integer({ valueHex: rBuffer });).
The WebCryptoAPI does not apply any formatting to these values; it outputs byte arrays whose lengths correspond to the bit length (order) of the elliptic curve. As a result, smaller numerical values are left-padded with 0x00 bytes, which can lead to invalid or misinterpreted ASN.1 encodings. See asn1IntTest.spec.ts.
As a solution we propose to convert to BigInt and use the asn1js.Integer.fromBigInt method.

This should solve #385

Copy link
Contributor

@microshine microshine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, looks correct and should resolve the P‑521 encoding issue. I left a few small remarks in the review.

@SWGovernikus SWGovernikus reopened this Nov 6, 2025
@SWGovernikus
Copy link
Author

SWGovernikus commented Dec 1, 2025

@microshine i hope the new test works. If there is anything else to fix, please let me know. :)

@coveralls
Copy link

coveralls commented Dec 9, 2025

Coverage Status

coverage: 74.554% (+0.02%) from 74.539%
when pulling 7983d0d on Governikus:master
into 82ece66 on PeculiarVentures:master.

@gtjarks
Copy link

gtjarks commented Jan 20, 2026

Hello @microshine,
is there anything else needed for the ticket, or are there still adjustments to be made to the code?

@SWGovernikus
Copy link
Author

Hello @microshine , sorry to bother you :)
Please let me know if anything can be done on my side to merge this. Thank You!

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.

4 participants