Repository navigation
create: Draw randomness from Core and the OS - #145
BenWestgate wants to merge 4 commits into
Conversation
02e1f72 to
06ff2d3
Compare
|
@codex security-review Generated by Claude Code |
Summary
Testing
|
|
@codex we don't see the changes you speak of. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
cACK # Hashing keeps the plaintext xpub in the wallet file from checking guesses of a share. The question then becomes, how much entropy is in an xprv and why SHA256 instead of another hash function for example SHA512? And why just one round? |
Summary
Testing
|
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Security review of 40fb74f (PR #145; 13 changed files).
[P2] Treat failure to unload the entropy wallet as a creation failure. In CoreEntropy.close(), unloadwallet errors are reduced to a warning, _folder/_disk are cleared before cleanup succeeds, and rmtree(..., ignore_errors=True) hides filesystem removal failures. _create() can consequently report a successful ceremony (or proceed to wallet initialization) while the entropy wallet remains loaded in Bitcoin Core; callers also lose the state needed to retry cleanup. The source xprvs held in that wallet determine all the generated entropy. Although the wallet is normally relocked after each draw, combined failures of walletlock and unloadwallet can leave the keys RPC-accessible during the 60-second unlock window. Propagate unload failure after best-effort cleanup, preserve failed resource handles for retries, and test this at the CLI boundary. The current unit test explicitly expects a warning and successful close() on unload failure.
Other security boundaries reviewed: Bitcoin Core selection and stdin-only passphrase handling; 256-bit passphrase generation; SHA-256 extraction from each fresh xprv to prevent stored public xpubs from testing guesses of shares; explicit source injection and byte-count checks; unbiased index rejection sampling; confirmation-gated shares; OS-specific RAM/disk paths and signal cleanup. SHA-256 is appropriate here: one invocation already provides 256-bit preimage resistance, and SHA-512 or repeated hashing cannot increase entropy beyond the source. No other actionable vulnerability was identified in these paths.
CI: 10 Python test-matrix jobs and 1 Bitcoin Core fixture job passed. The managed local diff scanner could not start because head 40fb74f is not available in the local Git object database; this review inspected GitHub's exact PR patches and head-revision source. Recommendation: revise before merge and re-review wallet teardown.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d20d70dd00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenWestgate
changed the title
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head security/code review.
Concept ACK bea2914; not a security ACK yet. Seed, share, index and identifier draws now require injected entropy; each SHA-256 draw combines fresh Core xprv material with independent OS bytes, with no silent fallback on Core failure. Wallet cleanup and interruption handling have tests. Before human approval: independent real-Core 32.0rc3 verification of repeated draws, interruption/unload, disk-fallback warning and secret-file removal, plus security review of the new CoreEntropy boundary.
|
This is the real-Core check the review above asks for, run on
Not covered: the macOS RAM disk and Windows against real Core. CI covers those paths only with fakes. I wrote both the change and this check, so it isn't independent. The script is about 250 lines and could go under Generated by Claude Code |
|
cACK: The script is about 250 lines and could go under tools/ if you want it rerunnable here. Yes. |
|
Do the commits follow contributing.md? |
Require an injected entropy source throughout master-seed and share generation, including random identifiers, indices and rejection redraws. The CLI supplies CoreEntropy, which hashes each fresh Bitcoin Core xprv together with 32 OS random bytes. Core failure stops creation instead of falling back. Use a new encrypted temporary wallet for each draw and finish relocking, unloading and deleting it before returning bytes. Convert supported termination signals into interruptions during creation, and report incomplete cleanup before another card can be emitted. Document RAM-backed paths, the disk fallback and the limits of leaked wallet files. Include generation, CLI, cleanup and interruption regressions together with the existing real-Core regtest coverage and matching API/security documentation. Validation: the complete Git tree is identical to bea2914, whose Python matrix, Core fixture and CodeQL checks passed. git diff --check passes. This consolidates the reviewed implementation without changing any file. Refs #145.
bea2914 to
7cb2ae6
Compare
BenWestgate
changed the title
|
The 14-commit history contained repeated fixups and reversals, so it needed consolidation under CONTRIBUTING.md's squashing guidance. It is now one atomic commit, Verified All six inline threads are resolved. The approved standalone verification script is still absent from the branch, so the PR description now explicitly tracks that request and independent security review as remaining work. This history cleanup does not supply either. |
Unit tests fake Core, so nothing showed that a real node really unloads and forgets each draw's wallet. The new check runs CoreEntropy against regtest Core on Linux: repeated draws, a real SIGINT while each Core call is in flight, `ms32 create` in a terminal stopped by SIGTERM, SIGHUP and SIGINT, and the disk fallback without /dev/shm. After each case no wallet may stay loaded or on disk, and Core's debug.log must hold no key or passphrase. The Bitcoin Core fixtures workflow runs it next to the existing regtest check. Claude-Session: https://claude.ai/code/session_01T233rKgZqE5wzDm3EVTHL1
The entropy check runs the ms32 CLI, which reads its version from the installed package's metadata, so PYTHONPATH alone fails with PackageNotFoundError. Claude-Session: https://claude.ai/code/session_01T233rKgZqE5wzDm3EVTHL1
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted incremental review at 753eded: Code ACK for the follow-up test/CI changes. The standalone 22-check Core entropy harness is committed, the workflow installs codex32 before calling its CLI, and both current-head CI jobs passed. Independent security review of CoreEntropy and real-Core macOS/Windows coverage remain outstanding. Not a security/merge ACK.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted independent security review at 753eded: 22/22 entropy harness checks passed on real Bitcoin Core 32.0rc4 (Linux regtest), including fresh draws, per-draw wallet cleanup, interruptions, disk fallback and log checks. No confirmed vulnerability in the 15-file diff. Linux Core boundary verified; full Tails creation/restore and macOS/Windows runtime coverage remain open. Not yet a merge ACK.
Fresh creation now takes all seed, share, identifier, index and redraw randomness through an injected entropy source. The CLI combines each fresh Bitcoin Core xprv with 32 OS random bytes using SHA-256; a Core failure stops creation rather than falling back. This uses Core's generator while retaining OS protection if a temporary wallet's public data is later recovered.
CoreEntropycreates an encrypted temporary wallet for each draw, relocks and unloads it, and removes its files before returning bytes. Linux uses/dev/shm, macOS uses a RAM disk, and other systems warn before using disk-backed temporary storage. Passphrases travel through stdin. Creation handles supported termination signals and reports incomplete cleanup before another card is emitted. The API, CLI, unit tests, real-Core regtest and security documentation change together.Review the injected-source boundary in
generation.py, the mixed draw and cleanup in_bitcoin_core.py, and interruption handling incli.py. PRs #159 and #160 build on this change.Validation: commit
7cb2ae6has exactly the same complete Git tree asbea2914, which passed the Python matrix, Bitcoin Core fixture job and CodeQL.git diff --checkpasses. The reported 25-check Linux Core 32.0rc3 run was performed by the implementation's author; macOS and Windows were covered with fakes rather than real Core.Remaining before merge:
tools/, with reproducible invocation instructions, and verify that committed tool.tools/bitcoin_core_entropy.py(31ef005); its docstring says how to run it (753eded). Overlapping checks from the 25 were merged, leaving 22.