fix(deps): lift btcd off the vulnerable v0.22.3 downgrade pin - #14
Closed
mswilkison wants to merge 2 commits into
Closed
fix(deps): lift btcd off the vulnerable v0.22.3 downgrade pin#14mswilkison wants to merge 2 commits into
mswilkison wants to merge 2 commits into
Conversation
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.25.0 — the newest release with the classic single-module layout, containing the fixes for every advisory open against v0.22.3. 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. - 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. The tss-lib fork pin is untouched: it compiles against byte-identical btcec sources and the stable chaincfg surface of v0.25.0. 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> Claude-Session: https://claude.ai/code/session_01PRiUN74spKNnQ9bsGaVzHM
…uild Two problems found while reviewing the btcd upgrade. 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 the upgrade replaces (v0.22.3 and the v0.23.4 alias) allocate per script and decode the same input cleanly, so this would have been 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, which belongs to the v0.26.x layout that the plain-path forks make unreachable. Reject oversized input before it reaches the decoder. 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. ComputeSignatureHashes also now asserts that every input has a registered previous output. The invariant holds today because TransactionBuilder only ever appends inputs through Add*Input, but a future gap would reach the same panicking path rather than an error. Separately, 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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PRiUN74spKNnQ9bsGaVzHM
10 tasks
Author
|
Superseded by threshold-network#4219, which targets Closing this one. The btcd downgrade turns out to be pre-existing on
|
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
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
Resolves the
btcdfinding from the dependency-sweep review comment on threshold-network/keep-core#4109: thereplace github.com/btcsuite/btcd => github.com/btcsuite/btcd v0.22.3directive pinned the whole btcd module below the requiredv0.23.2, keeping six fixed security advisories open against the Bitcoin consensus, wire, and script packages of the bridge.This PR is stacked on
security-release/candidate-1and contains only the btcd fix.What changed
replace btcd => btcd v0.22.3(whole module downgraded)replace btcd/v2 => btcd v0.23.4(module alias)require btcd v0.23.2(inert — overridden by the replace)require btcd v0.25.0btcd/btcecserved by the downgraded modulethird_party/btcsuite/btcec(in-tree copy of the btcd v0.22.3 btcec sources) via a directoryreplacepkg/bitcoinon archivedbtcsuite/btcutilbtcsuite/btcd/btcutil v1.1.5pkg/bitcoin/electrumonbtcd/v2/wirealias pathsbtcd/wireWhy the downgrade existed, and why vendoring is the fix
btcd v0.23 extracted
btcd/btcecinto the separate modulebtcd/btcec/v2; later versions don't ship the pre-split package at all. Two consumers still import the pre-split path and cannot easily migrate: the pinned tss-lib fork (audited crypto — import strings can't change without cutting a new fork release, and the86bd1a3pin is deliberately review-locked for this release) and first-party key-handling code (pkg/crypto/ephemeral,pkg/crypto/secp256k1,pkg/net/...), which relies on v1btcectypes being aliases of thecrypto/ecdsatypes.Serving that one package from an in-tree copy (gofmt-only delta vs upstream;
VENDOR.mdhas a mechanical verification recipe, and the upstream btcec test suite is included and passes) removes the only reason the whole module was held at v0.22.3.Why v0.25.0 and not v0.26.2
v0.26.x restructured btcd:
wire,txscript,chaincfg,chainhash,btcutilmoved out to/v2modules and the plain-path packages no longer exist. The go-electrum fork and the tss-lib fork import the plain paths (btcd/chaincfg,btcd/txscript) with import strings we don't control, so v0.26.x is unreachable without forking both. v0.25.0 is the newest release with the classic layout, and every advisory in the table below is fixed by v0.24.2-beta.rc1, so v0.25.0 is advisory-clean. (Bumping to v0.26.x later is purely a layout migration, not a security gap.)Why the
btcd/v2alias had to go tooThe alias only isolates self-contained packages. Any aliased package that imports sibling btcd packages does so via plain import paths, which resolved back to the v0.22.3 module — i.e. the aliased "patched" code was silently frankenlinking vulnerable v0.22.3 wire/txscript underneath. With the downgrade gone, the alias serves no purpose and the two electrum files return to plain paths.
Advisory resolution
wireblockchain/txscriptFindAndDelete)txscriptThe vendored v0.22.3
btcecis outside the advisory set — none of the six touchbtceccode. It is byte-identical to what every consumer (tss-lib fork included) compiled against before this PR, so there is zero crypto-behavior delta. Exit path (documented inVENDOR.md): migrate the tss-lib fork tobtcec/v2, then delete the directory.First-party code fallout
txscript.NewTxSigHashestakes aPrevOutputFetchersince v0.23 and (as of v0.25.0) 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 sighash-args purposes. The only production caller ofComputeSignatureHashes(pkg/tbtc/wallet.go) assembles inputs exclusively throughAdd*Input, so the fetcher is always complete;ComputeSignatureHashesnow also asserts that explicitly 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 hardening — 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 checks it 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 this PR replaces (v0.22.3 and the v0.23.4 alias) allocate per script and decode the same input cleanly, so this would have been a regression introduced by the upgrade, on a path fed by the untrusted Electrum backend (
decodeTransactionruns beforeGetTransaction's txid check, andGetTransactionConfirmationshas no hash check at all). Norecover()covers the tbtc signing paths, so it would terminate the node. Upstream fixed it only in the split-outwire/v2module, which belongs to the v0.26.x layout that is unreachable here.Fixed in first-party code with 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 its serialized length is always strictly below the cap — nothing consensus-valid is rejected, and the slab overflow becomes unreachable.TestTransaction_DeserializeOversizedlocks it in (andFuzzTransactionDeserializealready asserts the never-panic invariant, though it cannot reach these sizes).Docker build
third_party/is now copied beforeRUN go mod download. The directoryreplacetarget must exist for the module graph to resolve, so without it the image build fails at that step even though local builds pass.Verification
go build ./...,go vet ./...clean; fullgo test ./...green.pkg/bitcoinTestTransactionBuilder_Signingbyte-vector fixtures pass: DER signatures, sighashes, and compressed public keys are byte-identical across the version jump (v1 btcec's implicit BIP-62 low-S normalization is preserved because the signing path still uses the — now vendored, byte-identical — v1 btcec).VENDOR.mdrecipe (diff -rvs gofmt'd upstreambtcec@v0.22.3) reports only the three added files (go.mod,go.sum,LICENSE,VENDOR.md), and the upstream btcec test suite passes both from the main module and standalone.go list -deps ./...resolves to any btcd version with open advisories.go mod downloadreproduced failing without the Dockerfile change and passing with it.This PR was reviewed by an adversarial multi-agent pass over four lenses (consensus/signing correctness, module-graph soundness, the vendored crypto copy, and third-party fork consumers); the two blockers above were found that way and are fixed in the diff.
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. It is one pure codec package with no advisories against it; first-party code no longer touches it.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 does not index, so a hypothetical future advisory against pre-split btcec code would not be flagged automatically. None of the current six are in btcec.replacemeans external modules importing keep-core packages cannot resolve the btcec requirement. Not a regression: the previousrequirewas btcdv0.23.2, which does not contain thebtcecpackage at all, so downstream builds of the affected packages already failed.Test plan
go build ./.../go vet ./.../gofmt -l .go test ./...(full suite)go test github.com/btcsuite/btcd/btcec)VENDOR.mdbyte-identity verification recipego mod downloadagainst a go.mod + go.sum + third_party-only tree (Docker step simulation)docker build --target build-sources— the full dependency+generation stage, including thego mod downloadstep that fails without thethird_partyCOPYOut of scope
The
x/cryptofloor issue from the same review comment (bump lands below the 0.52.0 advisory floor) is a separate concern with its own conflict against PR threshold-network#4142 and is deliberately not addressed here.