fix(deps): lift btcd off the vulnerable v0.22.3 downgrade pin - #4219
fix(deps): lift btcd off the vulnerable v0.22.3 downgrade pin#4219mswilkison wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change updates Bitcoin dependency resolution, vendors the btcec secp256k1 implementation, adds transaction-size and witness sighash safeguards, and introduces cryptographic key, signature, and encryption functionality with comprehensive tests and examples. ChangesBitcoin dependency and transaction handling
Vendored btcec implementation
Key and signature APIs
Encryption and examples
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant Encrypt
participant Recipient
participant Decrypt
Sender->>Encrypt: plaintext and recipient public key
Encrypt->>Recipient: ephemeral public key, IV, ciphertext, and HMAC
Recipient->>Decrypt: ciphertext and private key
Decrypt->>Recipient: authenticated plaintext
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
245eb9f to
8bbf009
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/bitcoin/electrum/transaction.go (1)
21-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the size guard into a shared
pkg/bitcoinhelper.The check and error text are byte-for-byte duplicates of
Transaction.Deserialize(pkg/bitcoin/transaction.goLines 166-173). A single exported validator keeps the limit and message in sync if either changes.♻️ Sketch
In
pkg/bitcoin/transaction.go:// ValidateTransactionByteLength returns an error if data exceeds the maximum // transaction byte length accepted by the decoder. func ValidateTransactionByteLength(length int) error { if length > MaxTransactionByteLength { return fmt.Errorf( "transaction byte length [%v] exceeds the maximum of [%v]", length, MaxTransactionByteLength, ) } return nil }Then here:
- if len(headerBytes) > bitcoin.MaxTransactionByteLength { - return nil, fmt.Errorf( - "transaction byte length [%v] exceeds the maximum of [%v]", - len(headerBytes), - bitcoin.MaxTransactionByteLength, - ) - } + if err := bitcoin.ValidateTransactionByteLength(len(headerBytes)); err != nil { + return nil, err + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/bitcoin/electrum/transaction.go` around lines 21 - 31, Extract the duplicated transaction-size check into an exported ValidateTransactionByteLength helper in the bitcoin package, preserving the existing limit and error text. Update both Transaction.Deserialize and the Electrum deserialization path to call this shared validator and return its error unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@third_party/btcsuite/btcec/example_test.go`:
- Around line 7-13: Add an explicit go test ./third_party/btcsuite/btcec/...
step to the applicable Go test workflow(s), ensuring the vendored btcec examples
run under CI alongside the existing root-module gotestsum coverage. Reuse the
current workflow setup and apply this to every release/client Go test workflow
that needs the coverage.
In `@third_party/btcsuite/btcec/VENDOR.md`:
- Around line 43-65: Add and maintain the root go.sum entry for the locally
replaced module github.com/btcsuite/btcd/btcec at the synthetic version declared
by its go.mod. Ensure the entry is generated or preserved when running go mod
tidy with the local module replacement, while leaving the existing btcec/v2
entries unchanged.
- Around line 5-13: Update the introductory file-count statement in VENDOR.md to
say four files are added for vendoring only, matching the existing “four added
files” wording later in the document.
---
Nitpick comments:
In `@pkg/bitcoin/electrum/transaction.go`:
- Around line 21-31: Extract the duplicated transaction-size check into an
exported ValidateTransactionByteLength helper in the bitcoin package, preserving
the existing limit and error text. Update both Transaction.Deserialize and the
Electrum deserialization path to call this shared validator and return its error
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d679d6b-6bed-4ad4-990c-9eefde39c7b0
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumthird_party/btcsuite/btcec/go.sumis excluded by!**/*.sum
📒 Files selected for processing (32)
Dockerfilego.modpkg/bitcoin/electrum/block.gopkg/bitcoin/electrum/transaction.gopkg/bitcoin/estimator.gopkg/bitcoin/script.gopkg/bitcoin/transaction.gopkg/bitcoin/transaction_builder.gopkg/bitcoin/transaction_test.gothird_party/btcsuite/btcec/LICENSEthird_party/btcsuite/btcec/README.mdthird_party/btcsuite/btcec/VENDOR.mdthird_party/btcsuite/btcec/bench_test.gothird_party/btcsuite/btcec/btcec.gothird_party/btcsuite/btcec/btcec_test.gothird_party/btcsuite/btcec/ciphering.gothird_party/btcsuite/btcec/ciphering_test.gothird_party/btcsuite/btcec/doc.gothird_party/btcsuite/btcec/example_test.gothird_party/btcsuite/btcec/field.gothird_party/btcsuite/btcec/field_test.gothird_party/btcsuite/btcec/genprecomps.gothird_party/btcsuite/btcec/gensecp256k1.gothird_party/btcsuite/btcec/go.modthird_party/btcsuite/btcec/precompute.gothird_party/btcsuite/btcec/privkey.gothird_party/btcsuite/btcec/privkey_test.gothird_party/btcsuite/btcec/pubkey.gothird_party/btcsuite/btcec/pubkey_test.gothird_party/btcsuite/btcec/secp256k1.gothird_party/btcsuite/btcec/signature.gothird_party/btcsuite/btcec/signature_test.go
| import ( | ||
| "encoding/hex" | ||
| "fmt" | ||
|
|
||
| "github.com/btcsuite/btcd/btcec" | ||
| "github.com/btcsuite/btcd/chaincfg/chainhash" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the vendored module manifest and root replace directives.
fd -t f 'go.mod' | while read -r f; do echo "=== $f ==="; cat "$f"; done
# Check root module's replace/require wiring for btcec.
rg -n 'btcec|btcd' go.mod go.sum 2>/dev/null | head -50
# Does CI run tests inside third_party?
fd -t f -e yml -e yaml . .github 2>/dev/null --exec sh -c 'echo "=== $1 ==="; cat "$1"' sh {}
rg -n 'go test|third_party' Makefile 2>/dev/nullRepository: threshold-network/keep-core
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== github workflows mentioning go test/third_party =="
fd -t f -e yml -e yaml . .github | xargs rg -n 'go test|gotestsum|third_party|/btcec|btcec' || true
echo "== Dockerfiles mentioning go modules/tests == "
fd -t f 'Dockerfile*' . | while read -r f; do
echo "--- $f"
rg -n 'WORKDIR|go test|gotestsum|go mod|third_party|/btcec|btcec' "$f" || true
done
echo "== candidate files under btcec =="
fd -t f . third_party/btcsuite/btcec | sort
echo "== btcec module imports/requirements relevant lines =="
sed -n '1,80p' third_party/btcsuite/btcec/go.mod
sed -n '1,40p' third_party/btcsuite/btcec/example_test.go
sed -n '1,40p' third_party/btcsuite/btcec/VENDOR.mdRepository: threshold-network/keep-core
Length of output: 5882
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Dockerfile build dependencies section =="
sed -n '1,100p' Dockerfile
echo "== go.mod tidy/replace dependency section =="
sed -n '60,105p' go.mod
echo "== check presence of btcec go.sum checksums =="
rg -n 'github.com/btcsuite/btcd/btcec|github.com/btcsuite/btcd/chaincfg/chainhash|github.com/davecgh/go-spew' third_party/btcsuite/btcec/go.sum go.sumRepository: threshold-network/keep-core
Length of output: 6936
Run the vendored btcec examples in CI.
The module wiring is already correct: github.com/btcsuite/btcd/btcec is explicitly required and replaced with ./third_party/btcsuite/btcec, and go.mod also requires github.com/btcsuite/btcd/chaincfg/chainhash with matching checksums. The remaining CI gap is that release/client gotestsum -- ./... runs from the root module, so the standalone btcec sub-module examples remain uncovered. Add an explicit go test ./third_party/btcsuite/btcec/... step in the Go test workflow(s).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@third_party/btcsuite/btcec/example_test.go` around lines 7 - 13, Add an
explicit go test ./third_party/btcsuite/btcec/... step to the applicable Go test
workflow(s), ensuring the vendored btcec examples run under CI alongside the
existing root-module gotestsum coverage. Reuse the current workflow setup and
apply this to every release/client Go test workflow that needs the coverage.
go.mod replaced the whole btcd module with v0.22.3 — below the v0.23.2 the module itself requires — solely because the tss-lib fork and first-party key-handling code import the pre-split btcd/btcec package that btcd stopped shipping in v0.23. That downgrade kept six fixed security advisories open against the Bitcoin consensus, wire, and script packages of the bridge (GO-2022-1098 / GHSA-2chg-86hq-7w38, GO-2024-2818 / GHSA-3jgf-r68h-xfqm, GO-2024-3189 / GHSA-27vh-h6mc-q6g8). Serve the pre-split btcec package from an in-tree copy of the btcd v0.22.3 sources instead (third_party/btcsuite/btcec, gofmt-only delta, see VENDOR.md for provenance and verification), and raise the main btcd module to v0.24.2. v0.24.2 is an exact pin — it cannot go up or down. It is the lowest release fixing every advisory above (the last lands in 0.24.2-beta.rc1), and both higher options are closed: v0.25.0 requires btcec/v2 v2.3.5, but btcec/v2 v2.3.4 dropped the error return from ecdsa.SignCompact, and go-ethereum v1.13.15 calls the two-value form in crypto/signature_nocgo.go, so pulling btcec/v2 that far forward breaks every cgo-less build including the cross-compiled release binaries; v0.26 additionally moved wire, txscript and chaincfg into separate /v2 modules and dropped the plain import paths the go-electrum and tss-lib forks use. Raising btcd further requires upgrading go-ethereum first. The v0.23.4 btcd/v2 module alias is dropped along with the downgrade replace: it only isolated leaf packages, and any aliased package that imported sibling btcd packages silently linked back into the vulnerable v0.22.3 code. Fallout handled in first-party code: - txscript.NewTxSigHashes now takes a PrevOutputFetcher and requires the previous outputs of all inputs; TransactionBuilder registers each input's locking script and value as inputs are added, and ComputeSignatureHashes asserts the set is complete rather than letting a future gap reach a panicking path. - pkg/bitcoin moves from the archived github.com/btcsuite/btcutil to github.com/btcsuite/btcd/btcutil. - pkg/bitcoin/electrum returns from the /v2 alias to plain btcd import paths. Deserialization is capped at 4,000,000 bytes. btcd v0.24.2 rewrote MsgTx.btcDecode to slice every script of a transaction out of one fixed-size 4 MiB buffer, advancing it after each read; readScriptBuf bounds each individual script against maxWitnessItemSize but never against the remaining buffer, so once the cumulative script length of a single transaction passes 4 MiB the next slice panics instead of returning an error. Both decoders replaced here (v0.22.3 and the v0.23.4 alias) allocate per script and decode the same input cleanly, so this would otherwise be a regression on a path fed by the untrusted Electrum backend: decodeTransaction runs before GetTransaction's txid check, GetTransactionConfirmations performs no hash check at all, and no recover() covers the tbtc signing paths. Upstream fixed it only in the split-out wire/v2 module, part of the v0.26 layout the plain-path forks make unreachable. A transaction's weight is 3*base_size + total_size and cannot exceed the maximum block weight of 4,000,000 weight units, so a consensus-valid transaction is always strictly shorter than the cap — nothing legitimate is rejected and the buffer overflow becomes unreachable. Finally, the Dockerfile resolved the module graph from go.mod and go.sum alone, before any source was copied. The directory replace added for the vendored btcec needs its target present, so go mod download failed in the image build while local builds passed. Copy third_party alongside the module files. The tss-lib fork pin is untouched: it compiles against byte-identical btcec sources and the stable chaincfg surface of v0.24.2. Signing vectors in pkg/bitcoin tests confirm DER signatures, sighashes, and compressed public keys are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8bbf009 to
4b35de7
Compare
|
CI is green (21/21). One correction to the version choice above, worth recording since it constrains any future btcd bump. The first push used btcd v0.25.0 and v0.25.0 requires Now pinned to v0.24.2, which requires That makes the pin exact in both directions: it can't go lower without reopening an advisory, or higher without upgrading go-ethereum first. Both constraints are in the Note for whoever rebases |
MaxTransactionByteLength only bounds the total wire length of the incoming payload. The panic it was added for is actually triggered by the cumulative declared (varint-read) script/witness length within a single transaction exceeding btcd's shared, fixed-size 4,194,304-byte script buffer; readScriptBuf checks a declared count only against the flat 4,000,000-byte maxWitnessItemSize constant, never against that buffer's actual remaining capacity. A declared length does not need to be backed by that many delivered bytes for the panicking slice expression to be evaluated -- it is built as an argument to io.ReadFull, before any read is attempted -- so a transaction that first delivers ~194KB of real script data (enough to shrink the shared buffer below 4,000,000 bytes remaining) and then merely declares, but does not deliver, one more 4,000,000-byte item still panics, with a total wire length far below the cap this guard checks. Both guarded call sites, bitcoin.Transaction.Deserialize and electrum.decodeTransaction, share this pattern and sit on the path from the untrusted Electrum server to the process, with nothing recovering from a panic anywhere between them. Wrap both call sites in a recover() that converts any panic from the underlying btcd decoder into a returned error, and correct the MaxTransactionByteLength doc comment, which implied the length check alone closed this panic. Add regression tests in both packages that build the minimal "declare, don't deliver" payload and assert a clean error comes back instead of a crash.
|
Reviewed the btcd downgrade pin and found one issue, now fixed: MaxTransactionByteLength cap does not close the panic it was added for (Critical) — the cap only bounds a transaction's total wire length, but the underlying panic is triggered by the cumulative declared script/witness length within a single transaction exceeding btcd's shared, fixed-size 4,194,304-byte script buffer. A payload that delivers roughly 194 KB of real script data and then merely declares, without delivering, one more large item still panics — well under the byte-length cap. Fixed by wrapping both untrusted-input decode paths ( No other issues found. |
lrsaturnino
left a comment
There was a problem hiding this comment.
1 finding fixed and pushed; no items requiring manual follow-up.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@third_party/btcsuite/btcec/README.md`:
- Around line 8-14: Correct the documentation typos in the btcec README: change
“suite of test” to “suite of tests,” fix “signficantly” to “significantly,”
revise “developers original” to the grammatically correct possessive wording,
and correct “parsed form raw bytes” to “parsed from raw bytes” in the additional
affected section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 34aeeb4c-8491-4742-87af-22afaef932ed
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumthird_party/btcsuite/btcec/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
Dockerfilego.modpkg/bitcoin/electrum/block.gopkg/bitcoin/electrum/transaction.gopkg/bitcoin/electrum/transaction_test.gopkg/bitcoin/estimator.gopkg/bitcoin/script.gopkg/bitcoin/transaction.gopkg/bitcoin/transaction_builder.gopkg/bitcoin/transaction_test.gothird_party/btcsuite/btcec/LICENSEthird_party/btcsuite/btcec/README.mdthird_party/btcsuite/btcec/VENDOR.mdthird_party/btcsuite/btcec/bench_test.gothird_party/btcsuite/btcec/btcec.gothird_party/btcsuite/btcec/btcec_test.gothird_party/btcsuite/btcec/ciphering.gothird_party/btcsuite/btcec/ciphering_test.gothird_party/btcsuite/btcec/doc.gothird_party/btcsuite/btcec/example_test.gothird_party/btcsuite/btcec/field.gothird_party/btcsuite/btcec/field_test.gothird_party/btcsuite/btcec/genprecomps.gothird_party/btcsuite/btcec/gensecp256k1.gothird_party/btcsuite/btcec/go.modthird_party/btcsuite/btcec/precompute.gothird_party/btcsuite/btcec/privkey.gothird_party/btcsuite/btcec/privkey_test.gothird_party/btcsuite/btcec/pubkey.gothird_party/btcsuite/btcec/pubkey_test.gothird_party/btcsuite/btcec/secp256k1.gothird_party/btcsuite/btcec/signature.gothird_party/btcsuite/btcec/signature_test.go
🚧 Files skipped from review as they are similar to previous changes (23)
- third_party/btcsuite/btcec/go.mod
- third_party/btcsuite/btcec/privkey_test.go
- Dockerfile
- third_party/btcsuite/btcec/doc.go
- third_party/btcsuite/btcec/pubkey_test.go
- pkg/bitcoin/transaction.go
- third_party/btcsuite/btcec/gensecp256k1.go
- third_party/btcsuite/btcec/precompute.go
- third_party/btcsuite/btcec/VENDOR.md
- third_party/btcsuite/btcec/genprecomps.go
- pkg/bitcoin/estimator.go
- third_party/btcsuite/btcec/ciphering_test.go
- pkg/bitcoin/script.go
- third_party/btcsuite/btcec/example_test.go
- third_party/btcsuite/btcec/signature_test.go
- third_party/btcsuite/btcec/pubkey.go
- third_party/btcsuite/btcec/ciphering.go
- third_party/btcsuite/btcec/btcec.go
- third_party/btcsuite/btcec/btcec_test.go
- third_party/btcsuite/btcec/signature.go
- third_party/btcsuite/btcec/privkey.go
- go.mod
- third_party/btcsuite/btcec/field_test.go
| Package btcec implements elliptic curve cryptography needed for working with | ||
| Bitcoin (secp256k1 only for now). It is designed so that it may be used with the | ||
| standard crypto/ecdsa packages provided with go. A comprehensive suite of test | ||
| is provided to ensure proper functionality. Package btcec was originally based | ||
| on work from ThePiachu which is licensed under the same terms as Go, but it has | ||
| signficantly diverged since then. The btcsuite developers original is licensed | ||
| under the liberal ISC license. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the documentation typos.
“suite of test,” “signficantly,” “developers original,” and “parsed form raw bytes” reduce clarity.
Proposed fix
- standard crypto/ecdsa packages provided with go. A comprehensive suite of test
+ standard crypto/ecdsa packages provided with Go. A comprehensive suite of tests
is provided to ensure proper functionality. Package btcec was originally based
on work from ThePiachu which is licensed under the same terms as Go, but it has
- signficantly diverged since then. The btcsuite developers original is licensed
+ significantly diverged since then. The btcsuite developers' original is licensed
under the liberal ISC license.
...
- parsed form raw bytes and serializing the generated signature.
+ parsed from raw bytes and serializing the generated signature.Also applies to: 28-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@third_party/btcsuite/btcec/README.md` around lines 8 - 14, Correct the
documentation typos in the btcec README: change “suite of test” to “suite of
tests,” fix “signficantly” to “significantly,” revise “developers original” to
the grammatically correct possessive wording, and correct “parsed form raw
bytes” to “parsed from raw bytes” in the additional affected section.
Source: Linters/SAST tools
- M3: narrow the 'cannot crash the process' overclaim in MaxTransactionByteLength and electrum.decodeTransaction comments; the recover() turns slice-bounds panics into errors but does NOT cover fatal OOM from attacker-declared varint counts that btcd's wire decoder honors before reading backing bytes. - M4: pre-check len(rawTx)/2 against MaxTransactionByteLength in electrum.decodeTransaction before hex.DecodeString allocates the full decoded buffer. - M10: drop the exact 'recovered from a panic' string assertion in both declared-not-delivered tests; the invariant is process survival plus err != nil, not the specific recovered-panic message (which depends on today's btcd readScriptBuf bound). - M11: add the under-cap precondition guard to TestDecodeTransactionDeclaredNotDelivered so a future cap change cannot silently misdirect the test into a different code path. - M12: document why the prevOuts field is typed as the concrete *txscript.MultiPrevOutFetcher rather than the PrevOutputFetcher interface (the latter would silently turn the completeness check into a no-op). - M13: tighten the ComputeSignatureHashes guard comment to note that the completeness check is not exercised for coinbase-shaped inputs (which NewTxSigHashes skips). - M22: correct the decodeTransaction guard comment to refer to the pre-decode length guard, not a wire-vs-script conflation. - M18, M19, M20, M23: fix VENDOR.md (three -> four files, //go:build line insertions, missing first-party consumers, six-vs-three advisories).
- M7: add TestTransactionBuilder_ComputeSignatureHashesMissingPrevOut that bypasses the Add* methods via tb.internal.AddTxIn and asserts the exact 'missing previous output for input [0]' error, exercising the defensive branch that replaced an unhandled panic and is otherwise unreachable through any public API. - M9: extend TestTransactionBuilder_AddPublicKeyHashInput and TestTransactionBuilder_AddScriptHashInput to assert the Add* method also registered the UTXO in builder.prevOuts (FetchPrevOutput + value), not just in sigHashArgs and internal.TxIn. Closes the gap where the completeness check was exercised only indirectly via downstream sighash-fixture comparisons. - M14: add a VENDOR.md note about the v0.22.3-to-v0.24.2 shared script pool ceiling change (6.4 MB to roughly 524 MB per concurrent decode; per-decode worst case unchanged at 4 MiB). RSS can grow under high concurrency - worth knowing for future capacity triage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@third_party/btcsuite/btcec/VENDOR.md`:
- Around line 53-54: Update the multiplication markers in the retention-ceiling
text of VENDOR.md so they do not trigger MD037; replace each `*` between numeric
operands with `×` or an escaped asterisk, preserving the displayed calculations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aa735ae6-3661-4fe2-9ffd-6b6417b97ddc
📒 Files selected for processing (7)
pkg/bitcoin/electrum/transaction.gopkg/bitcoin/electrum/transaction_test.gopkg/bitcoin/transaction.gopkg/bitcoin/transaction_builder.gopkg/bitcoin/transaction_builder_test.gopkg/bitcoin/transaction_test.gothird_party/btcsuite/btcec/VENDOR.md
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/bitcoin/transaction_builder.go
- pkg/bitcoin/transaction.go
| global retention ceiling therefore rises from 6.4 MB (12,500 * 512 B) | ||
| to 524 MB (125 * 4 MiB), about an 82x increase. The per-script cap |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the multiplication markers.
The * characters trigger MD037 warnings; use × or escape them.
Proposed fix
- to 524 MB (125 * 4 MiB), about an 82x increase.
+ to 524 MB (125 × 4 MiB), about an 82x increase.🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 53-53: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 54-54: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@third_party/btcsuite/btcec/VENDOR.md` around lines 53 - 54, Update the
multiplication markers in the retention-ceiling text of VENDOR.md so they do not
trigger MD037; replace each `*` between numeric operands with `×` or an escaped
asterisk, preserving the displayed calculations.
Source: Linters/SAST tools
The previous note conflated bytes-per-entry with bytes-per-slab and said the 4 MiB shared slab 'stayed' across the version jump. It did not: v0.22.3 pooled 12,500 individual 512-byte buffers (freeListMaxScriptSize = 512; Borrow falls back to make for larger scripts); v0.24.2 introduces a single fixed-size 4 MiB slab (scriptSlabSize = 1 << 22) checked out per decode. That slab- architecture change is what introduces the readScriptBuf slice-bounds panic guarded against in bitcoin.MaxTransactionByteLength and electrum.decodeTransaction. Also tighten the per-script cap note: it tightens from 32 MiB (MaxMessagePayload) to 4,000,000 (maxWitnessItemSize), strictly safer.
69da1aa to
4144f8b
Compare
Summary
go.modreplaces the wholebtcdmodule with v0.22.3 - below thev0.23.2the module itself requires - which keeps three already-fixed security advisories open against the Bitcoin consensus, wire, and script packages the bridge actually compiles. (Three vulnerabilities, six identifiers counting the GHSA aliases.)This is long-standing on
mainand not specific to any release branch. It surfaced during a dependency sweep of #4109; that PR's integration branch inherits it, and this fix is deliberately targeted here rather than stacked on the release candidate so it gets CI and normal review.Unlike an unused-code advisory, this code is in the build —
go list -deps ./...returns 16btcsuite/btcdpackages includingblockchain,blockchain/indexers,btcjson,btcutil,txscript, andwire.wireblockchain,txscriptFindAndDeletere-implementationtxscriptWhy the downgrade was there, and why it can go
btcd v0.23 extracted
btcd/btcecinto the separate modulebtcd/btcec/v2, and later versions don't ship the pre-split package at all. Two consumers still import the pre-split path and can't simply be edited: the pinned tss-lib fork (crypto/ckd,tss/curve) and first-party key-handling code (pkg/crypto/ephemeral,pkg/crypto/secp256k1,pkg/net/...,pkg/chain/local_v1,pkg/internal/pbutils), which relies on v1btcectypes being aliases of thecrypto/ecdsatypes.So one crypto package was holding the entire module — consensus code included - a minor version back.
This PR serves just that package from an in-tree copy of the btcd v0.22.3
btcecsources (third_party/btcsuite/btcec) and moves the module itself to v0.24.2. The vendored copy is byte-identical to upstream apart from a mechanicalgofmtpass;VENDOR.mdrecords provenance, a reproduciblediff -rverification recipe, and the exit path (delete the directory once tss-lib migrates tobtcec/v2). The upstream btcec test suite ships with it and passes.Why v0.24.2 and not v0.26.2
v0.24.2 is the lowest version that fixes all three advisories (the last one lands in
0.24.2-beta.rc1), and going higher isn't currently possible:btcec/v2v2.3.5, andbtcec/v2v2.3.4 changedecdsa.SignCompactfrom([]byte, error)to[]byte. go-ethereum v1.13.15 calls the two-value form incrypto/signature_nocgo.go, so pullingbtcec/v2that far forward breaks every cgo-less build - including the cross-compiled release binaries thatmake releaseproduces. Raising btcd past v0.24.2 requires upgrading go-ethereum first, which is out of scope here.wire,txscript,chaincfg,chainhash, andbtcutilmoved to separate/v2modules and the plain-path packages no longer exist. The go-electrum and tss-lib forks import the plain paths with import strings we don't control, so v0.26.x isn't reachable without forking both.So this is a compatibility ceiling, not a security gap - v0.24.2 is advisory-clean. Both constraints are recorded in the
go.modcomment so the next person to touch this pin doesn't have to rediscover them.The
btcd/v2alias goes tooreplace github.com/btcsuite/btcd/v2 => github.com/btcsuite/btcd v0.23.4only isolates self-contained packages. Any aliased package that imports a sibling btcd package does so by plain path, which resolved back to the v0.22.3 module - so the "patched" alias was linking vulnerablewire/txscriptunderneath it. With the downgrade gone the alias has no purpose, and the twopkg/bitcoin/electrumfiles using it return to plain paths.First-party fallout
txscript.NewTxSigHashestakes aPrevOutputFetchersince v0.23 and (as of v0.24.2) requires the previous outputs of all inputs.TransactionBuildernow registers each input's locking script and value in aMultiPrevOutFetcheras inputs are added - it already fetched exactly that data for its sighash args.ComputeSignatureHashesalso asserts the set is complete and returns an error, rather than letting a future gap reach the panicking path.mempool.GetTxVirtualSize/btcutil.Hash160/btcutil.NewTxmove from the archivedbtcsuite/btcutiltobtcd/btcutil(same semantics).Deserialization cap and recover - required by the version jump
btcd v0.24.2 rewrote
MsgTx.btcDecodeto slice every script of a transaction out of one fixed-size 4 MiB buffer (scriptSlabSize = 1 << 22), advancing it after each read.readScriptBufbounds each individual script againstmaxWitnessItemSizebut never against the remaining buffer, so once the cumulative script length of a single transaction passes 4 MiB the next slice panics instead of returning an error:Reproduced directly: 4 × 900 KB output scripts decode cleanly, 5 panic. Both decoders this PR replaces (v0.22.3 and the v0.23.4 alias) allocate per script and handle the same bytes fine, so this would be a regression introduced by the upgrade - on a path fed by the untrusted Electrum backend.
decodeTransactionruns beforeGetTransaction's txid check,GetTransactionConfirmationsperforms no hash check at all, and norecover()covers the tbtc signing paths, so it would terminate the node. Upstream fixed it only in the split-outwire/v2module, part of the v0.26 layout the plain-path forks make unreachable.The fix is two layers, and only the combination closes the panic: a
MaxTransactionByteLength = 4_000_000cap applied inbitcoin.Transaction.Deserializeandelectrum.decodeTransaction(a transaction's weight is3*base_size + total_sizeand cannot exceed the 4,000,000 WU maximum block weight, so a consensus-valid transaction is always strictly shorter than the cap - nothing legitimate is rejected), plus arecover()that turns the slice-bounds panic that can still fire below the cap into a plain error. Two regression tests (TestTransaction_DeserializeDeclaredNotDeliveredandTestDecodeTransactionDeclaredNotDelivered, both added in626538d2) construct a 194,378-byte (189.8 KiB, 4.86% of the cap) transaction that panics in unpatched btcd and confirm therecover()returns the error. The cap alone is not sufficient - without therecover(), payloads of that size terminate the process. The cap bounds grossly oversized input; therecover()is what actually turns the under-cap panic into an error.A subsequent commit also pre-checks
len(rawTx)/2againstMaxTransactionByteLengthinelectrum.decodeTransactionbeforehex.DecodeStringallocates, so a malicious multi-GB hex string can't pay the full allocation before the guard runs.Note: the
recover()covers slice-bounds panics fromreadScriptBufonly. It does not cover fatal runtime errors such as out-of-memory from attacker-declared varint counts that btcd's wire decoder honors before reading backing bytes. Closing that class of input requires either a pre-decode budget check on declared input/output counts or a future btcd release; both are out of scope here.The v0.22.3-to-v0.24.2 jump also changes btcd's script-decoding internals beyond the slab panic. Two material changes worth knowing for future triage:
MaxMessagePayload) to 4,000,000 (maxWitnessItemSize) - strictly safer, no legitimate transaction is affected.Neither is a security issue; both are documented in the corresponding
MaxTransactionByteLengthdoc comment and inVENDOR.md.Docker
third_party/is copied beforeRUN go mod download. The directoryreplacetarget must exist for the module graph to resolve, so the image build would otherwise fail at that step even though local builds pass.Verification
go build ./...,gofmt -l .clean; fullgo test ./...green.CGO_ENABLED=0 go buildfordarwin/amd64,darwin/arm64,linux/amd64,linux/arm64,windows/amd64- the cgo-less cross-compile pathmake releaseuses, which selects go-ethereum'ssignature_nocgo.goand is where thebtcec/v2incompatibility above shows up.go vet ./...reports only the pre-existingpkg/tecdsa/signing/protocol.gocopylock warning, byte-identical on unmodifiedmain.TestTransactionBuilder_Signingbyte-vector fixtures pass: DER signatures, sighashes, and compressed public keys are byte-identical across the version jump. The signing path still uses v1 btcec (now vendored, byte-identical), so its implicit BIP-62 low-S normalization is preserved.VENDOR.mdrecipe: only the four added files (go.mod,go.sum,LICENSE,VENDOR.md) differ from gofmt'd upstreambtcec@v0.22.3.docker build --target build-sourcespasses, including thego mod downloadstep that fails without thethird_partyCOPY.Known residuals
github.com/btcsuite/btcutil(2019) is still linked, because the tss-lib fork'scrypto/ckdimports itsbase58package - the same unchangeable-import-string constraint that motivates the vendoring. One pure codec package, no advisories against it; first-party code no longer touches it. MVS now selects an ~20-months-olderbtcsuite/btcutilpseudo-version thanmaindid (the archivedv1.0.3-0.20201208143702direct require is gone, replaced by the tss-lib fork'sv0.0.0-20190425235716-9e5f4b9a998d); the only compiled package (base58) is functionally identical between the two commits, so this is benign.go.sumkeeps/go.mod-only hash lines for two old btcd pseudo-versions reached through transitive requires that MVS overrides. Noh1:hashes, so no source is compiled; the vulnerablev0.22.3/v0.23.4full hashes are gone. Naive graph-level SCA tools may still surface them.btcecsits at a module path OSV doesn't index, so a hypothetical future advisory against pre-split btcec code wouldn't be flagged automatically. None of the current three are inbtcec. The vendored dir is also invisible to root-modulego test ./...and has no automated advisory monitor; byte-identity relies on a human running theVENDOR.mdrecipe.replacemeans external modules importing keep-core packages can't resolve the btcec requirement. Not a new blast radius:mainalready carries the same unresolvable-with-replace pattern forgoogle.golang.org/protobuf/dev, which downstream consumers cannot resolve either. The btcec pseudo-version is new on this branch, but the underlying pattern is pre-existing.Test plan
go build ./.../go vet ./.../gofmt -l .go test ./...(full suite)VENDOR.mdbyte-identity verification recipedocker build --target build-sourcesNote for #4109
The security-release integration branch carries the same downgrade. Once this lands, its next rebase picks the fix up; the
go.modresolution there is mechanical (that branch has a much newer dependency set, so the conflict is in surrounding lines rather than the btcd ones).Summary by CodeRabbit
Bug Fixes
Security
Tests