fix(sdk): bounds-check the DER ECDSA signature parser (DSPX-3397) - #988
Closed
dmihalcik-virtru wants to merge 2 commits into
Closed
fix(sdk): bounds-check the DER ECDSA signature parser (DSPX-3397)#988dmihalcik-virtru wants to merge 2 commits into
dmihalcik-virtru wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cryptoService.sign() returns DER-encoded ECDSA signatures, but JWS requires
raw IEEE P1363 (R || S) per RFC 7518 section 3.4. Both JWS emitters -- the KAS
rewrap request token (tdf3/src/crypto/jwt.ts) and the DPoP proof signer
(src/auth/dpop.ts) -- were shipping DER, so any EC-keyed token was rejected by
conformant verifiers (Keycloak, panva-jose). verifyJwt had the mirror bug: it
fed a raw JWS signature to a verifier expecting DER.
Also fixes reqSignature defaulting to RS256 for the rewrap request token.
WebCrypto rejects signing an EC private key with RSA params ("Unable to use
this key to sign"), so an EC dpop key could not produce a rewrap token at all;
the alg is now derived from the key's algorithm.
Supporting changes:
- export ieeeP1363ToDer / derToIeeeP1363 for these callers
- add isAsymmetricSigningAlgorithm() so the JWS `alg` header is validated
rather than blind-cast to the narrower set CryptoService can actually sign
with (the JWS alg space includes PS256/EdDSA, which we do not support)
The round-trip this creates (sign encodes to DER, caller decodes back to raw)
is tracked for removal in DSPX-3634.
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
derToIeeeP1363 indexed into the signature buffer without checking bounds: a truncated or malformed DER blob read `undefined` length bytes, sliced empty components, and then computed a negative offset in `result.set()`. It also only stripped a single leading zero, and only when the component was already over-long, so a validly-padded value could survive at the wrong width. ieeeP1363ToDer had the mirror gap: it derived the component length by halving the input, so an odd-length or wrong-curve input silently produced a fractional split rather than an error. Both now derive the fixed component width from the algorithm (getEcdsaComponentLength) and validate every read. Malformed input raises ConfigurationError with a specific message instead of producing a garbage signature. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
dspx-3397-1-jws-signatures
branch
from
August 10, 2026 17:47
756b4b3 to
b220a91
Compare
dmihalcik-virtru
force-pushed
the
dspx-3397-2-der-hardening
branch
from
August 10, 2026 17:47
4f53ae5 to
5722f0b
Compare
|
X-Test Failure Report |
dmihalcik-virtru
force-pushed
the
dspx-3397-1-jws-signatures
branch
from
August 11, 2026 17:54
b220a91 to
0f1935a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack 2/8, split out of #939. Base: #987.
What
derToIeeeP1363indexed into the signature buffer without checking bounds. A truncated or malformed DER blob would:undefinedas a length byte,result.set(r, componentLen - r.length).It also stripped only a single leading zero, and only when the component was already over-long — so a validly zero-padded value could survive at the wrong width and silently produce a garbage signature.
ieeeP1363ToDerhad the mirror gap: it derived the component length by halving the input, so an odd-length or wrong-curve input produced a fractional split instead of an error.Changes
Both directions now derive the fixed component width from the algorithm (
getEcdsaComponentLength: ES256→32, ES384→48, ES512→66) and validate every read. Malformed input raisesConfigurationErrorwith a message naming the specific problem instead of returning a bad signature.trimLeadingZerosis hoisted to module scope and now used by both directions.Tests
tests/mocha/unit/crypto/der-signature.spec.ts— round-trip across all three curves, plus truncated, over-long, wrong-tag, zero-length-INTEGER, and wrong-curve inputs.tests/mocha/reqsignature-jws.spec.tsgains the truncated-signature case, which asserts the specific error rather than a generic verification failure.How to test
cd lib && npm test