Skip to content

fix: use canonical DER encoding for WebCrypto ECDSA signatures#240

Merged
ajhodges merged 3 commits intobase:masterfrom
ajhodges:webcrypto-der
Jan 29, 2026
Merged

fix: use canonical DER encoding for WebCrypto ECDSA signatures#240
ajhodges merged 3 commits intobase:masterfrom
ajhodges:webcrypto-der

Conversation

@ajhodges
Copy link
Collaborator

@ajhodges ajhodges commented Jan 28, 2026

Summary

Fixed a bug where WebCrypto ECDSA signatures were incorrectly DER-encoded, causing Go's strict encoding/asn1 parser to reject them.

The custom asn1EncodeSignature function didn't add the required 0x00 padding byte when an INTEGER's first byte has its high bit set (≥ 0x80). In DER encoding, this padding is mandatory to indicate the value is positive—without it, parsers interpret the value as negative.

Changes:

  • Replaced custom asn1EncodeSignature with Ox's Signature.toDerBytes which produces canonical DER encoding
  • Removed the buggy asn1EncodeSignature function and its unused imports
  • Added comprehensive tests to verify correct DER encoding behavior

How did you test your changes?

Added unit tests in encoding.test.ts that verify:

  • 0x00 padding is added when r has high bit set (e.g., 0x8e...)
  • 0x00 padding is added when s has high bit set (e.g., 0xca...)
  • No padding is added when high bit is not set (e.g., 0x6e..., 0x4a...)
  • Small values with high bit set still get correct padding
  • Edge cases with small r/s values work correctly

Tests fail with the old implementation (missing required padding) and pass with the fix.

Also ran playground locally and made a few cryptokey transactions via http://localhost:3001/add-sub-account, which seemed to work fine

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Jan 28, 2026

✅ Heimdall Review Status

Requirement Status More Info
Reviews 2/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@ajhodges ajhodges requested a review from stephancill January 28, 2026 22:14
@ajhodges ajhodges marked this pull request as ready for review January 29, 2026 03:12
Copy link
Collaborator

@montycheese montycheese left a comment

Choose a reason for hiding this comment

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

verified works locally, thanks for finding this!

authenticatorData: arrayBufferToBase64Url(hexToBytes(webauthn.authenticatorData)),
clientDataJSON: arrayBufferToBase64Url(stringToBytes(webauthn.clientDataJSON)),
signature: arrayBufferToBase64Url(asn1EncodeSignature(signatureRaw.r, signatureRaw.s)),
signature: arrayBufferToBase64Url(Signature.toDerBytes(signatureRaw)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

Copy link
Collaborator

@stephancill stephancill left a comment

Choose a reason for hiding this comment

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

neat, thanks!

@ajhodges ajhodges merged commit 90f0668 into base:master Jan 29, 2026
9 checks passed
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