Repository navigation
fix(crypto): complete input-bound hardening - #28
Merged
Merged
Conversation
This PR addresses the four follow-up findings from the review of PR #12 (agent-docs/pr-integration/12.md): 12-F1: KGRound3Message.UnmarshalProofInts now validates exact arity (paillier.ProofIters = 13 parts) and returns an error instead of panicking on short/long/empty slices. 12-F2: KGRound2Message1.UnmarshalFactorProof / UnmarshalFactorProofTilde now guard against nil sub-message pointers and return a descriptive error instead of dereferencing nil. 12-F3 (P2): Backports upstream e65fb36's verifyMaxModulusBitLen=65536 ceiling to crypto/paillier ModProof/ModVerify. An exported API caller can no longer supply an arbitrarily large modulus that triggers the O(bitLen) session sampler. Regression tests pin the boundary at 65536 (passes ceiling, rejected at oddness) and 65537 (rejected at ceiling with allocation < 256 KiB). 12-F4: RangeProofAlice.Bytes and ValidateBasic now fail-loud on nil or partially-nil receivers, consistent with the sibling ProofBob/ProofBobWC encoders. All tests are deterministic, fast, and fail before/pass after the fixes.
…ording/allocation Review follow-ups on the input-hardening-followups PR: 1. crypto/paillier/paillier.go: verifyMaxModulusBitLen's comment claimed sampleYModN tags blocks with a single byte and wraps at 257 blocks. This fork's sampler actually tags blocks with a 4-byte uint32 index (does not wrap until 2^32 blocks), so that rationale was factually wrong here. The comment now documents the ceiling honestly as an upstream-compatible allocation/operational cap, not a sampler correctness bound, and says so explicitly. 2. crypto/paillier/mod_proof.go: extracted the ceiling comparison into an unexported exceedsModulusBitLenCeiling(N) predicate used by ModVerify. mod_proof_test.go now pins the exact boundary (65536 bit-length passes, 65537 fails) by calling this predicate directly, and separately preserves an end-to-end ModVerify behavior check at the same widths -- neither depends on ModVerify's error wording or on measuring allocation. Dropped the runtime.MemStats allocation assertion and the strings.Contains wording assertions along with their imports. 3. crypto/mta/range_proof_test.go: TestRangeProofAliceBytesInvalidReceiver no longer pins panic message text. It now asserts ValidateBasic() returns false on the invalid receiver directly, then recovers the Bytes() panic and checks the recovered value is an error that is NOT a runtime.Error -- which fails before the fix (nil-pointer dereference panics are runtime.Error) and passes after (the guard panics with a plain fmt.Errorf). Removed the inaccurate round-trip claim from the doc comment since this test does not exercise a round-trip. 4. ecdsa/keygen/messages_test.go: TestKGRound2UnmarshalFactorProofNil now also exercises the symmetric successful FacproofTilde decode once it is populated, matching the existing Facproof positive-path coverage. gofmt run on all touched files; no build/test/lint executed per the delivery contract.
Author
|
Integration verification completed before merge into
|
This was referenced Sep 29, 2026
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.
Summary
Focused follow-up to the review of #12 (
agent-docs/pr-integration/12.md), addressing the four validated gaps that were explicitly called out as non-blocking follow-ups in that review (not the separate broad fuzz-harness PR).ecdsa/keygen/messages.goKGRound3Message.UnmarshalProofIntsindexedproofBzs[0..12]with no arity guard; a direct exported-decoder call panicked on fewer than 13 parts. Now validates exact arity (paillier.ProofIters = 13) viacommon.NonEmptyMultiBytesand returns an error.KGRound2Message1.UnmarshalFactorProof/UnmarshalFactorProofTildedereferenced a nilFacproof/FacproofTildesubmessage; a direct call panicked. Both now return a descriptive error on a nil submessage.e65fb36("crypto: bound the declared length before reserving it, in gob decode and ModProof.Verify"). AddsverifyMaxModulusBitLen = 65536tocrypto/paillier, checked inModProof.ModVerifybeforecommon.IsUsableUnknownOrderModulus(i.e. before the O(bitLen) primality check and the session-tagged samplersampleYModN), so the exported API can no longer allocate/hash proportional to an arbitrarily large caller-supplied modulus. Placement and derivation mirror the upstream commit'scrypto/modproof/proof.gohunk (256-bit sampler blocks, one-byte-equivalent block tag, boundary atbitLen <= 65536). The shipped keygen wire path already pins moduli to 2048 bits, so this closes the gap only in the exported API surface.RangeProofAlice.Bytes/ValidateBasiclacked the nil/ValidateBasicfail-loud symmetry that siblingProofBob/ProofBobWCalready have.ValidateBasicnow short-circuits on a nil receiver, andBytes()panics with a descriptive"RangeProofAlice.Bytes: invalid receiver"error instead of segfaulting inside a nil*big.Intfield.Regression tests (intrinsic to each fix, deterministic & fast)
ecdsa/keygen/messages_test.go:TestKGRound3UnmarshalProofIntsArity(exact-13, short 0..12, extra 14th part, empty part — all rejected without panicking);TestKGRound2UnmarshalFactorProofNil(nil submessage rejected on both decoders, valid submessage round-trips).crypto/paillier/mod_proof_test.go:TestModProofVerifyAcceptsCeilingBoundary(a modulus at exactly 65536 bits passes the ceiling, rejected only at the later oddness check — guards the off-by-one);TestModProofVerifyRejectsOversizedModulus(a 65537-bit odd composite is rejected with the ceiling error and allocates under 256 KiB, measured viaruntime.MemStats).crypto/mta/range_proof_test.go:TestRangeProofAliceBytesInvalidReceiver(nil receiver and nil-field receiver both fail loud with the descriptive message instead of a raw nil-pointer panic).Notes