Security release/candidate 1#4109
Draft
lionakhnazarov wants to merge 184 commits into
Draft
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)
…into feat/byzantine-signing-harness
…into feat/byzantine-coordination-harness
…ess' into feat/byzantine-f008-corroboration
…tss-lib hardening (#8) Documents the protocol-fork breaking changes (bound GG20 session nonces, changed DKG/signing session-ID formats, fail-closed minimum-length contract) and their coordinated-upgrade requirement.
…fixes (#2) Documents the wire-breaking ECDH/HKDF and G1HashToPoint changes (F-02/F-03), the RandomBeacon reentrancy guard, and the deduplicator TOCTOU fix.
…' into epic-consolidation
…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 skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
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.
No description provided.