sign: disallow overlong der encodings and zero r or s values
What the developer wrote
Authored by Jon Griffiths
sign: disallow overlong der encodings and zero r or s values
libsecp will happily parse and return such signatures without error,
however they can never be valid (in the overlong case, because libsecp
internally sets R to zero). Reject them up-front when parsing rather
than failing when attempting to use them later.
What changed, and why it matters
This commit tightens signature validation in a cryptographic library. It now rejects malformed DER-encoded signatures that are longer than allowed, or where one of the signature numbers (R or S) is zero. Previously, the underlying secp256k1 library would accept these invalid forms silently, which could lead to later failures or unexpected behavior when the signatures are used in Bitcoin/Elements transactions.
Review callers of wally_ec_sig_from_der() to ensure they handle parse failures correctly, and verify that EC_SIGNATURE_DER_MAX_LEN is consistent with other DER length limits in the project. Consider adding test vectors for overlong DER and zero R/S signatures.
Security signals we found
Cryptographic input validation hardening
Rejection of overlong DER-encoded ECDSA signatures
Rejection of zero R or S signature components
Defense-in-depth against invalid signatures being accepted silently
Evidence from the diff
The patch updates wally_ec_sig_from_der() in src/sign.c. It adds an upper-bound check on bytes_len (EC_SIGNATURE_DER_MAX_LEN) and, after parsing/serializing, verifies that neither the R nor S half of the compact signature is all-zeroes. libsecp256k1’s DER parser accepts overlong encodings and zero-valued components without error, but such signatures are cryptographically invalid (overlong DER causes libsecp to internally set R=0). The change rejects them at parse time and clears the output buffer on failure.
Changed components
src/sign.cwally_ec_sig_from_der()ECDSA signature parsingInspect captured patch +6 / −1
diff --git a/src/sign.c b/src/sign.c
index e803b09..0399102 100644
--- a/src/sign.c
+++ b/src/sign.c
@@ -295,10 +295,15 @@ int wally_ec_sig_from_der(const unsigned char *bytes, size_t bytes_len,
const secp256k1_context *ctx = secp256k1_context_static;
bool ok;
- ok = bytes && bytes_len && bytes_out && len == EC_SIGNATURE_LEN &&
+ ok = bytes && bytes_len && bytes_len <= EC_SIGNATURE_DER_MAX_LEN &&
+ bytes_out && len == EC_SIGNATURE_LEN &&
secp256k1_ecdsa_signature_parse_der(ctx, &sig_secp, bytes, bytes_len) &&
secp256k1_ecdsa_signature_serialize_compact(ctx, bytes_out, &sig_secp);
+ if (ok && (mem_is_zero(bytes_out, EC_SIGNATURE_LEN / 2) ||
+ mem_is_zero(bytes_out + EC_SIGNATURE_LEN / 2, EC_SIGNATURE_LEN / 2)))
+ ok = false; /* R or S are 0: this signature is invalid */
+
if (!ok && bytes_out)
wally_clear(bytes_out, len);
wally_clear(&sig_secp, sizeof(sig_secp));
Why this scored 62/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.