Fix signature serialization, non-terminating parse, address length and pubkey validation

[SachinMeier]

Security fixes to signature and public key handling, each shipped with a test that fails against the unfixed code.

Fix 1: serialize_signature/1 emitted short, invalid signatures (SIG-005)

It used :binary.encode_unsigned, which drops leading zero bytes, so any signature whose r or s has a leading zero byte (~0.8% of real signatures) serialized to fewer than 64 bytes. BIP340 test vector 4 (whose r has ten leading zero bytes) re-serialized to 53 bytes, which the library's own parse_signature/1 then rejected. r and s are now each padded to exactly 32 bytes, making serialize_signature/1 the exact inverse of parse_signature/1.

Fix 2: parse_signature/1 never terminated on empty input (HIGH)

Base.decode16("") returns {:ok, ""}, so the hex fallback clause tail-called itself on "" forever — a silent busy loop reachable from untrusted input. Concretely, Invoice.decode("lnbc1qh65qct") hung a wallet permanently, since BOLT11 decoding split off an empty signature and fed it to ecdsa_recover_compact/3; a slightly longer input like "lnbc1w4pnfm" raised a MatchError instead. Two layers fixed:

• The hex fallback clause now only accepts 128-character input (a 64-byte compact signature in hex); everything else falls through to {:error, "invalid signature size"}.
• Invoice.decode/1 now checks the data part is at least 111 words (7-word timestamp + 104-word signature) before splitting, returning {:error, :invoice_data_too_short}.

The regression tests run each call under a Task with a 5s timeout so a regression fails CI instead of hanging it.

Fix 3: Address never checked the 20-byte payload (HIGH)

Address.encode/3 accepted a hash of any length, and is_valid? accepted any payload length behind a valid checksum and version byte. Since OP_HASH160 always yields 20 bytes, a P2PKH/P2SH output built around any other length can never be spent — passing a SHA-256 or a 33-byte pubkey where a hash160 belongs minted a valid-looking burn address. encode/3 now requires byte_size(hash) == 20 and validation requires exactly 21 decoded bytes.

Fix 4: uncompressed public keys were never validated (MEDIUM)

Point.parse_public_key/1 accepted any 65-byte 04||x||y input with no field-range or on-curve check — parse_public_key(<<0x04>> <> <<1::256>> <> <<1::256>>) returned {:ok, %Point{x: 1, y: 1}}, an off-curve point that then flowed into Math.add/Math.multiply (invalid-curve attack surface) or into unspendable addresses. The uncompressed branch now requires x < p, y < p, and y^2 == x^3 + 7 (mod p). The compressed branch now rejects x >= p (previously get_y silently reduced mod p, so 02||(p+1) was accepted as x = 1), matching Point.lift_x/1.

Two callers were hardened alongside it:

• ExtendedKey.check_point/1 hard-matched {:ok, pubkey} = parse_public_key(key). Rejecting x >= p newly turned an attacker-supplied xpub into a MatchError; it now returns {:error, "invalid public key"}.
• The SEC-hex fallback clause called Base.decode16!/2 on any binary that wasn't 33 or 65 bytes — including 65-byte keys with a bad prefix — so Script.is_multi?/1 raised ArgumentError on attacker script bytes. It is now length-guarded and routed through Utils.hex_to_bin/1.

Fix 5: der_parse_signature/1 was non-strict (SIG-003)

It enforced only the outer length and the 0x02 markers; parse_sig_key/1 then did a bare :binary.decode_unsigned with no minimal-encoding, zero-value, sign-bit, or [1, n-1] check. All five PoC cases were accepted:

[A] non-minimal DER (33-byte r with leading 0x00) => {:ok, %Signature{r: 1, s: 1}}
[B] DER with high-bit 'negative' r                => {:ok, %Signature{r: 128, s: 1}}
[C] DER r = 2^256 (> n)                           => accepted
[D] DER (0,0)                                     => parses, and verify_signature => true
[E] DER with empty INTEGERs                       => {:ok, %Signature{r: 0, s: 0}}

[D] is the (0,0) forgery reachable from attacker-serialized bytes; [A]/[B] are byte-distinct encodings of the same (r,s), i.e. malleability at the encoding layer.

DER INTEGERs are now validated per BIP66 — non-empty, minimally encoded, high bit clear, at most 33 bytes — and both parse paths run the decoded scalars through Signature.new/2, which requires r, s ∈ [1, n-1]. All five cases now return {:error, _}.

Follow-on: PSBT stores parsed Signatures

Because strict DER parsing accepts only canonical encodings and der_serialize_signature/1 reproduces exactly that encoding, the two are now exact inverses — so the reason partial_sig stored raw DER bytes (preserving non-canonical encodings verbatim) no longer applies. In.partial_sig now holds a Signature.t(), and still round-trips byte-for-byte. add_field/3 also accepts raw DER and normalizes it; a hand-built struct is revalidated through Signature.new/2.

This means a PSBT carrying a non-canonical partial signature is rejected at decode rather than at finalize — earlier than Core, but never rejecting a PSBT Core considers valid overall, since BIP66 has been consensus since 2015.

API notes

Address.encode/3, Signature.serialize_signature/1 and der_serialize_signature/1 raise ArgumentError on invalid input rather than returning {:error, _}, keeping their String.t() / binary return types. A union return would have been a trap for callers like Script.to_address/2, which wraps the result in {:ok, _} unconditionally and would have yielded {:ok, {:error, msg}}. Signature.new/2 (formerly the private new_signature/2) is now public.

Coordination note

PR #125 also touches der_parse_signature/1 and Ecdsa.verify_signature/3 in this module, adding the same [1, n-1] range checks plus the point-at-infinity check in verify_signature/3. The two overlap on the range check but not on the DER strictness or the verify_signature/3 hardening, so both are still needed; whichever lands second needs a small rebase in lib/secp256k1/secp256k1.ex.

🤖 Generated with Claude Code