Skip to content

fix: review follow-ups for security-release/candidate-1 (CI unblock + verified fixes)#13

Merged
lionakhnazarov merged 6 commits into
lionakhnazarov:security-release/candidate-1from
threshold-network:fix/pr4109-review-followups
Jul 3, 2026
Merged

fix: review follow-ups for security-release/candidate-1 (CI unblock + verified fixes)#13
lionakhnazarov merged 6 commits into
lionakhnazarov:security-release/candidate-1from
threshold-network:fix/pr4109-review-followups

Conversation

@lrsaturnino

@lrsaturnino lrsaturnino commented Jul 3, 2026

Copy link
Copy Markdown

Summary

Follow-up fixes for the review of threshold-network#4109 (security-release/candidate-1), stacked directly on its head dd2c87360. Clears the one merge blocker, applies the verified minor findings, and removes a second pre-existing red CI check that surfaced during validation.

What this fixes

  • Merge blocker — pkg/bitcoin red at head. TransactionBuilder.getScript moved onto the new Transaction.OutputAt accessor, which changed the out-of-range error wording to "output index [N] is out of range ..."; transaction_builder_test.go still asserted the pre-refactor wording, leaving the package red. The alignment commit fixed spv_test.go/wallet_test.go but missed this one. Assertion aligned.
  • pkg/bitcoinDifficulty() division-by-zero. A malformed zero-mantissa Bits field, reachable from attacker-supplied headers on the SPV proof walk, made Difficulty() panic (contained only by the maintainSpv recover). It now returns zero difficulty for a non-positive target, so such a header contributes nothing and fails proof checks gracefully. Adds a regression test.
  • pkg/beacon/event — incomplete TOCTOU remediation. The F-13 fix made the tbtc deduplicator atomic but left NotifyDKGStarted on the racy Has()+Add(), allowing two goroutines on the same seed to both proceed. Switched to the mutex-serialized Add, mirroring the tbtc deduplicator. Adds a concurrent regression test (passes under -race).
  • RandomBeaconGovernance — tuple-destructure bug. finalizeAuthorizationDecreaseDelayUpdate destructured authorizationParameters() into the wrong position, binding the returned delay into the change-period variable and overwriting the stored change period with the old delay on finalize. Destructuring corrected to mirror the sibling change-period finalizer; the delay-finalize test now uses a distinct change period and asserts it is preserved.
  • pkg/maintainer/spv — unused constant. The DIFF1 rewrite replaced epoch-length arithmetic with a header-difficulty walk, orphaning difficultyEpochLength, which trips the staticcheck CI job — a second pre-existing red check the review did not surface. Removed; the maxProofHeaders comment is also corrected (the beyond-cap outcome is a permanent skip, not a deferral).
  • Docs. Document that the counter-based G1HashToPoint also diverges permanently from the immutable on-chain g1HashToPoint (verifyBytes / reportUnauthorizedSigning), a path with no in-repo callers, so the operational consumer is the off-chain GJKR Pedersen H generator. Make the flag-day requirement explicit (the breaking key-derivation, session-ID, and hash-to-point changes have no version gate, so a ceremony's node set must upgrade atomically). Note the clientInfo metrics port default change 9601 → 0. Correct the altbn128 test comment that claimed G1HashToPoint is used in relay-entry signing.

Deliberately not included: version-gated / negotiated activation for the breaking key-derivation changes was raised in review as an operational flag-day, not a code change. It is documented rather than implemented — bolting version negotiation into the crypto layer on a security release was out of scope.

Tests

All CI checks were replicated locally against the branch head:

  • gofmt, go build ./..., go vet — clean
  • golangci-lint v2.12.2 (CI-pinned) — 0 issues
  • staticcheck 2025.1.1 with CI's -SA1019 — 0 issues
  • go testpkg/bitcoin, pkg/beacon/event (incl. -race), pkg/altbn128, pkg/maintainer/spv all pass
  • random-beacon yarn build + RandomBeaconGovernance.test.ts — 275 passing, including the new change-period-preservation assertion

The getScript refactor onto Transaction.OutputAt changed the out-of-range
error wording to "output index [N] is out of range ...", but this test still
expected the pre-refactor wording, leaving the pkg/bitcoin package red at the
release head. Align the expected substring so the suite builds green.
BlockHeader.Difficulty divided the maximum target by the header target without
guarding a zero target. A malformed zero-mantissa Bits field, reachable from
attacker-supplied headers on the SPV proof walk, made the division panic. Return
zero difficulty for a non-positive target so such a header contributes nothing
and fails downstream proof-difficulty checks gracefully instead of panicking.
NotifyDKGStarted performed a separate Has() check followed by Add(), so two
goroutines racing on the same seed could both observe it as absent and both
proceed, admitting a duplicate DKG execution. Use the mutex-serialized cache
Add, which returns true only for the first inserter, mirroring the tbtc event
deduplicator, to close the time-of-check-to-time-of-use race.
…elay update

finalizeAuthorizationDecreaseDelayUpdate destructured authorizationParameters()
into the wrong tuple position: it bound the returned authorization decrease
delay into the change-period variable and discarded the actual change period,
then wrote that value back, overwriting the stored change period with the old
delay on finalize. Skip the delay position and bind the change period, mirroring
the sibling change-period finalizer. The delay-finalize test used equal values
and so could not detect the swap; it now uses a distinct change period and
asserts the change period is preserved.
…ader-cap comment

The DIFF1 rewrite replaced epoch-length arithmetic with a forward walk over
actual header difficulties, leaving difficultyEpochLength unused and tripping the
staticcheck CI job. Remove it, and correct the maxProofHeaders comment, which
described the beyond-cap outcome as merely deferred when the walk is anchored to
the transaction's block and the skip is permanent absent a reorg.
…rade requirement

Document that the counter-based G1HashToPoint also diverges permanently from the
immutable on-chain g1HashToPoint (the verifyBytes / reportUnauthorizedSigning
path), which no in-repo code calls, so the operational consumer is the off-chain
GJKR Pedersen H generator. Make the flag-day requirement explicit: the breaking
key-derivation, session-ID, and hash-to-point changes have no version gate, so a
ceremony's whole node set must upgrade atomically. Note the clientInfo metrics
port default change from 9601 to 0. Correct the altbn128 test comment that
claimed G1HashToPoint participates in relay-entry signing; it does not.
@lionakhnazarov
lionakhnazarov marked this pull request as ready for review July 3, 2026 13:10
@lionakhnazarov
lionakhnazarov merged commit fde5857 into lionakhnazarov:security-release/candidate-1 Jul 3, 2026
17 of 19 checks passed
@lionakhnazarov
lionakhnazarov deleted the fix/pr4109-review-followups branch July 3, 2026 14:23
lionakhnazarov pushed a commit that referenced this pull request Jul 12, 2026
* fix(deps): bump go-ethereum v1.13.15 -> v1.17.3

Addresses the 5 High-severity CVEs in the v1.13.x line of
github.com/ethereum/go-ethereum flagged by the Sysdig scan of
thresholdnetwork/keep-client:v2.5.2:

- CVE-2026-22862 (High 7.5) -> v1.16.8
- CVE-2026-22868 (High 7.5) -> v1.16.8
- CVE-2026-26313 (High 7.5) -> v1.17.0
- CVE-2026-26314 (High 7.5) -> v1.16.9
- CVE-2026-26315 (High 7.5) -> v1.16.9

v1.17.3 is the latest patch in the v1.17 series. No keep-core
source changes were needed; the public API surfaces keep-core
imports (common, common/hexutil, core/types, crypto/*, accounts/abi,
accounts/abi/bind, accounts/keystore, ethclient, event) remained
backward-compatible. Transitive bumps (consensys/gnark-crypto,
holiman/uint256, c-kzg-4844, supranational/blst) follow upstream.

Verified with go build ./..., go vet ./... (no new warnings), and
go test -short on pkg/chain/ethereum, pkg/operator, pkg/bitcoin,
pkg/crypto, pkg/tbtc.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(deps): bump keep-common to v1.7.1-tlabs.1 for go-ethereum v1.16+ compat

go-ethereum v1.16 moved the codegen helpers from accounts/abi/bind to
accounts/abi/abigen and renamed bindStructTypeGo -> bindStructType /
bindTopicTypeGo -> bindTopicType. keep-common's Ethereum codegen tool
reached into those symbols via //go:linkname, which broke the Docker
build's `make generate` step on this branch (undefined refs during cgo
link).

threshold-network/keep-common v1.7.1-tlabs.1 (companion fix branch:
threshold-network/keep-common#fix/go-ethereum-1.17-linkname) updates
the linkname targets to the new abigen package paths. Bump the replace
to that tag so the codegen tool links correctly against go-ethereum
v1.17.3.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci(client): free disk space on runner before multi-arch build

* ci(client): pin free-disk-space action to commit SHA

The free-disk-space step runs in the same job that later authenticates
to Docker Hub, AWS, and GHCR. @main is a floating ref against a
third-party action with shell access to the runner — pin to the
immutable v1.3.1 commit SHA to close the supply-chain window.

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Piotr Rosłaniec <p.roslaniec@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants