Security release/candidate 1#4109
Open
lionakhnazarov wants to merge 192 commits into
Open
Conversation
- Remove stale comment in KeepRandomBeaconOperator that incorrectly described the gas cap as a protection against compromised service contracts; the require now enforces success instead - Add missing enabled-path test for clientinfo.Initialize - Add breaking-change callout to docs warning operators that the metrics server is now opt-in (port 0 by default)
… default
Closes residual references to port 9601 as a default in operator-facing
samples and docs so operators copying the canonical configs do not silently
re-enable the now-disabled-by-default metrics/diagnostics HTTP server.
- configs/config.toml.SAMPLE: comment the [clientInfo] section and
Port = 9601 with an inline note that the section is opt-in.
- docs/resources/client-start-help: regenerate the help capture to
"Disabled by default." matching the new flag default in cmd/flags.go.
- docs/resources/docker-start-{mainnet,testnet}-sample: drop the
-p 9601:9601 host-port mapping that re-exposed the diagnostics port.
- docs-v1/run-random-beacon.adoc, bundle-guide.adoc: replace literal
localhost:9601 example with a configurable-port placeholder.
Also resolves the docs/run-keep-node.adoc contradiction with the new
trusted-network guidance: the Ports section instructed operators to
expose the Diagnostics Port publicly for Rewards Allocation, which
conflicts with the Client Info section's "trusted network only"
requirement. Rewritten to require a trusted network path and added a
cross-reference to the Client Info section.
Adds infrastructure/kube/keep-dev/eth-tx-rpc-ws-networkpolicy.yaml
restricting ingress to the dev geth StatefulSet (8545/8546) to
in-namespace pods, defense-in-depth against future cross-namespace
exposure given the StatefulSet binds 0.0.0.0 with --rpcvhosts=* and
--wsorigins=*.
Adds security/ directory with structured analysis for external pentesters covering the keep-core Go client and Solidity contracts. Files: - security/README.md: index, scope, and quick orientation - security/architecture.md: components, trust boundaries, actor roles - security/attack-surface.md: P2P, chain events, RPC, key ingestion, CLI - security/critical-paths.md: DKG, signing, beacon, tBTC minting/redemption - security/crypto-review.md: primitives, custom constructions, flagged issues - security/smart-contracts.md: contract inventory, proxies, privilege functions - security/threat-model.md: assets, threat actors, STRIDE mapping Also adds a security/ callout to the directory structure in README.adoc.
- Add 15 individual finding files (F-01 through F-15) covering Critical, High, Medium, and Low/Informational severity issues - Add 4 detailed strix vuln reports (vuln-0001 through vuln-0004) with PoC scripts and exploit walkthroughs - Add vulnerabilities.csv summary - Add .envrc and .envrc.* to .gitignore
- Merge strix vuln reports into the analyst-written findings - Promote F-07/vuln-0004 (WalletRegistry upgrade) to High (CVSS 8.2, confirmed exploit) and renumber as F-05 - Promote F-12/vuln-0002 (unauthenticated metrics) to Medium (CVSS 5.3) with full technical detail absorbed from strix report - Add F-13 (tBTC dedup race, TOCTOU, CVSS 6.5) and F-14 (legacy beacon reward withdrawal, CVSS 5.3) from strix unique findings - Drop vulnerabilities.csv (redundant with individual files)
Each finding now has a ## Verification section with: - STATUS: CONFIRMED / REQUIRES_EXTERNAL_REVIEW - Evidence from actual source code at the referenced location - Notes on mitigations, intent, or caveats where relevant 16/17 CONFIRMED. F-10 (encryption.Box) requires review of the threshold-network/keep-common fork which is not in this repo.
F-04: marked Invalid -- tss-lib fork is a known internal fork, not unreviewed third-party code. F-05: updated to Valid/Mitigated by Design -- intentional bytecode tradeoff documented in-code as Audit ISSUE #2; onlyGovernance cannot be used due to ProxyAdmin call-chain; reinitializer(2) + atomic upgradeToAndCall + Timelock provide equivalent protection.
…oss-codebase pattern
…-092 governance design
…(NaCl secretbox), sound
…ontext, note operational design
…utable, zero balance
…o production callers
…ctural constraint
F-02: correct timing claim -- counter-based approach bounds but does not normalize timing (loop exits on first valid point) F-03: note test helper fix (nil info -> labeled) and new regression test TestEcdhNilInfoDiffersFromLabeled in symmetric_key_test.go F-05: mark as known issue, link tracking issue tlabs-xyz/keep-core-security#6
dorny/paths-filter@v2 calls the GitHub API to list changed PR files. The GITHUB_TOKEN default scope was tightened; explicit permission is required to avoid 'Resource not accessible by integration' errors.
- architecture.md: fix cache-expiry wording (negative -> positive) and add language tags / blank lines around tables for markdownlint - attack-surface.md: correct V3 keystore wrong-password behavior (returns ErrDecrypt via MAC check, not silent wrong key material); add language tag to BroadcastNetworkMessage block - F-04: align severity formatting with other invalidated findings - F-05: restructure Remediation so the onlyGovernance diff is framed as why the obvious fix fails (msg.sender is ProxyAdmin under upgradeToAndCall); operational controls listed as primary mitigation - F-06: reword description so it does not contradict the on-chain safety-net conclusion - F-07: canonicalize on approveResult() (matches the cited library location); add challengeResult range to Verified-against - F-09: label pre-fix narrative explicitly; note callback execution now runs under the reentrancy guard - F-12: add CVSS v3.1 vector strings for original (5.3) and revised (3.1) scores; revised vector arithmetically matches 3.1 - F-14: add verification date and reproduction hint for the TokenStakingEscrow zero-balance claim
… bounded counter-based approach G1HashToPoint previously used try-and-increment (compute SHA-256(m), then increment x until a quadratic residue is found). Iteration count varied with the hash output, creating a measurable timing side channel. Replace with a counter-based hash-and-try: SHA-256(m || counter) for counter 0..63. Each attempt does identical work (one SHA-256 + one modular sqrt), bounding and normalising timing. Failure probability is (1/2)^64 ≈ 5e-20. NOTE: this changes the output of G1HashToPoint for the same input. Deployment requires a coordinated network upgrade. Follow-up: implement constant-time RFC 9380 SWU. See: tlabs-xyz/keep-core-security#4 Adds three determinism, distinctness, and on-curve validity tests. Closes: F-02 (partial -- bounded, not constant-time)
…twork-tss-lib-digest' into epic-consolidation
…nd tss-lib (threshold-network#19) bumps into epic
…pools-2.x-lockfile' into epic-consolidation
…n-replacement' into epic-consolidation
…t' into epic-consolidation
…cal-networks-config-0.x-lockfile' into epic-consolidation
…scan-3.x-lockfile' into epic-consolidation
…n' into epic-consolidation
…nsolidation # Conflicts: # CHANGELOG.md
…o epic-consolidation # Conflicts: # CHANGELOG.md # go.mod # go.sum
Integration fix: threshold-network#38's signing harness used message.Text(16) as the session ID, which is <16 bytes for small test messages. Combined with the hardened tss-lib session-nonce enforcement landed in #8, this panicked ("session ID must be at least 16 bytes"). Prefix the ID with a fixed label so it always clears the floor while preserving per-message uniqueness and cross-member agreement.
…erials' into epic-consolidation # Conflicts: # CHANGELOG.md
…nsolidation # Conflicts: # CHANGELOG.md
…in epic Combine the per-PR Keep a Changelog entries (threshold-network#36, threshold-network#37, threshold-network#34, threshold-network#38, threshold-network#39, threshold-network#40, #8, #2, threshold-network#14) into shared Added/Changed/Fixed/Security sections under [Unreleased].
…from epic The threshold-network#26 lockfile refresh floated @ethersproject/abi from 5.4.1 to 5.8.0 (via hardhat-etherscan@3.1.8's ^5.1.2 range). The newer ABI coder decodes uint256[] event args into BigNumbers that fail the chai deep-equal in random-beacon test/utils/dkg.ts against the pinned-ethers expected values, deterministically failing 15 'RandomBeacon - Group Creation' tests. hardhat-etherscan is a deploy/verify-only plugin with no functional value for this security consolidation; exclude threshold-network#26 like the other broken bumps (threshold-network#20/threshold-network#21/threshold-network#22). This restores solidity/random-beacon/yarn.lock to pre-threshold-network#26.
Merge tlabs-xyz/keep-core-security @ epic-consolidation (ac63328) onto threshold-network/main for security-release/candidate-1. Conflict resolution: - Keep main's libp2p handshake deadline (threshold-network#4008) in transport.go - Take epic hardening for bitcoin/spv/tbtc/tecdsa paths - Retain main's removal of legacy solidity-v1/dashboard and token-stakedrop - tss-lib pin: 86bd1a375cc0 Co-authored-by: Cursor <cursoragent@cursor.com>
getProofInfo assumed every proof header carries the relay epoch difficulty, so with txProofDifficultyFactor=1 it assembled single-header proofs. On testnet4 (BIP94), sweeps mined in minimum-difficulty blocks produced proofs containing only a DIFF1 header, which the Bridge rejects with "Not at current or previous difficulty". Mirror the Bridge's BitcoinTx logic instead: skip leading DIFF1 headers when both relay epochs are above minimum, bind the requested difficulty to the first decisive header matching the relay's current or previous epoch difficulty, and accumulate headers until their total observed difficulty covers requestedDifficulty * txProofDifficultyFactor.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedToo many files! This PR contains 177 files, which is 27 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (177)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
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.
…wups fix: review follow-ups for security-release/candidate-1 (CI unblock + verified fixes)
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
4 participants
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
Integration candidate for the coordinated security release (
sr-candidate-1).Merges:
tlabs-xyz/keep-core-security@epic-consolidation(ac63328) — private security epic (#41)main@4137b34be(testnet4 stack, chore(ci): align Yarn 4 workflows and solidity workspace tooling #3999–feat(go): add testnet4 mapping and harden runtime integration behavior #4002, fix(libp2p): bound Keep authentication handshake with SetDeadline #4008, Remove bootstrap nodes and replace with operator peers #3909, …)getProofInfomirrors Bridge difficulty logic (d07eb41)dd2c873)Head:
dd2c87360tss-lib pin:
github.com/threshold-network/tss-lib@v0.0.0-20260615180949-86bd1a375cc0(86bd1a3)Sepolia smoke on this commit: PASS (SR-1, SR-2 full @ groupSize=3, SR-4 live SPV proof, SR-5 rollback). Details below.
All operators in a DKG or signing ceremony must run the same build. Mixed old/new nodes will fail TSS rounds (session IDs, proof transcripts, wire formats differ).
Do not roll out node-by-node during an in-flight ceremony. Upgrade when idle; rollback only when idle.
Breaking changes (operator-visible)
G1HashToPoint,PrepareForSigningAPI changesconnected_wellknown_peers_count[clientinfo] Portexplicitly if scraping/metricsconnected_bootstrap_count→connected_wellknown_peers_count--network.bootstrap=truedeprecatedFull table: see epic
CHANGELOG.mdandSECURITY-BREAKING-CHANGES.mdonkeep-core-security.Conflict resolution (vs
main+ epic)applyHandshakeDeadline/ SetDeadline)getProofInfofix (testnet4 minimum-difficulty blocks)solidity-v1,dashboard,token-stakedrop86bd1a3viago.modreplaceDependencies
Local regression (pre-merge)
Sepolia validation (
security-candidate-1@dd2c87360)Droplet:
162.243.170.240· Image:keep-client:security-candidate-1[clientinfo]enabledWalletCreated@ Sepolia block 11130388; wallet PKH0xa8abdbaa…123c75b8…) →submitDepositSweepProof0x0d5de901…@ block 11136453 (status=1)Baseline replay: prior image
sepolia-dev-forklev-btcdiff-idle-flagvs candidate — no mesh/metrics/rollback regression; candidate proves new crypto + live SPV.Docker image
docker build --platform linux/amd64 -f Dockerfile --target runtime-docker \ --build-arg ENVIRONMENT=sepolia \ --build-arg VERSION=security-candidate-1 \ --build-arg REVISION=dd2c87360 \ -t keep-client:security-candidate-1 .Important: default Dockerfile final stage (
output-bins) is not runnable — use--target runtime-docker.Operator upgrade checklist
connected_wellknown_peers_count).LightRelayMaintainerProxy.authorize+authorizeSpvMaintainerafter fresh deploy; fund maintainer gas for btcdiff catch-up.Rollback: revert image tag when chain/ceremony is idle only.
Review focus
keep-core-security#41CHANGELOG86bd1a3matches intended releasegetProofInfoDIFF1 logic matches BridgeRelated
86bd1a3Test plan
go teston changed packages[Unreleased]vs epic