Repository navigation
Conversation
libbitcoinpqc v0.3.0 returns from slh_dsa_keygen, slh_dsa_sign and slh_dsa_secret_key_validate without clearing the stack frames that held secret-key material, and DerivePQCKey kept the HD master seed and HMAC state in its own frame. Each operation now runs in a non-inlined function, after which the public CPQCKey and DerivePQCKey functions overwrite PQC_STACK_BURN_BYTES (32 KiB) of the released stack, on success and failure paths. In the integrated (optimized) library build the library calls use at most about 12 KiB of stack. The burn is sized from that measurement and kept small, so the operations still fit in a 128 KiB thread stack, the musl default. Tests check that the burn covers all stack the operations write, that no secret key, master seed or HKDF state remains in the released stack, and that the operations run on a 128 KiB thread. Refs #260 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SnapshotReleasedStack() copied the released stack through a 192 KiB local array. With -fstack-clash-protection the compiler probes such a frame as it allocates it, and on most targets the probes store to it: Clang on x86 stores zeros every 4 KiB, GCC on AArch64 every 64 KiB, and GCC on 32-bit ARM stores a register value every 4 KiB. The checks read those stores as stack written below the burned region, or as a burned region cut into pieces, and failed on correct builds. GCC on x86 probes with `or $0`, which leaves memory unchanged. The copy now runs in a leaf function with a small frame, which reads the region below it into a buffer allocated before the stack is painted. Refs #260 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba49dd92da
ℹ️ 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".
The released-stack probes read the stack below the stack pointer, which Valgrind's Memcheck reports as invalid reads, and the copy includes bytes of the copying function's own frame, which Memcheck treats as undefined, so the scans of the copy reported uninitialised-value errors. When <valgrind/memcheck.h> is available, SnapshotReleasedStack() now disables address-error reporting for the range it reads and marks the copy defined afterwards. Without the header the client requests are compiled out and nothing changes. Refs #260 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
Summary
libbitcoinpqc v0.3.0 returns from
slh_dsa_keygen,slh_dsa_signandslh_dsa_secret_key_validatewithout clearing the stack frames that held secret-key material (SK_SEED,SK_PRFand values derived from them), andDerivePQCKeykept the HD master seed and HMAC state in its own frame. That memory stayed on the released stack after the call returned.What changed:
src/crypto/pqc.cpp: each operation now runs in a non-inlined function (GenerateKeyImpl,ValidateSecretKeyImpl,SignImpl,DeriveKeyImpl). After it returns, the publicCPQCKey::MakeNewKey,CPQCKey::Set,CPQCKey::SignandDerivePQCKeyoverwritePQC_STACK_BURN_BYTESof the released stack withBurnReleasedStack(), on success and failure paths.src/crypto/pqc.h:PQC_STACK_BURN_BYTES = 32 KiB, so the tests check the value the code uses.-O2: keygen 12,168 B, validate 12,008 B, sign 12,232 B), and theImplframes add less than 1 KiB. 32 KiB is about 2.5 times the deepest use. It is kept small because the burn adds its frame to the deepest signing call chain, and the operations must still fit in a 128 KiB thread stack, the musl default. The deepest measured entry on an RPC thread (DerivePQCKey, about 41.6 KiB from the thread's stack top) plus the burn is about 74 KiB.src/libbitcoinpqcstays at tagv0.3.0, sotest/lint/libbitcoinpqc-subtree-check.shis unaffected.Refs #260
Testing
Built with
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_GUI=OFF -DBUILD_BENCH=OFF -DBUILD_FUZZ_BINARY=OFF(targetstest_qbit qbitd qbit-cli).New unit tests in
src/test/pqc_tests.cpp:pqc_secret_key_operations_clear_released_stack: paints 192 KiB of stack, runsMakeNewKey,SetandSign, and copies the released region. For each, it finds the burn's zero run, checks that nothing below it was written (1 KiB is allowed formemory_cleanse's own frames), and checks that no copy of the key'sSK_SEEDremains.pqc_key_derivation_clears_released_stack: the same depth check forDerivePQCKey; the halves of the master seed, the HKDF inner hash,prkandSK_SEEDmust all be absent.pqc_secret_key_operations_fit_small_thread_stack: all four operations run on a pthread with a 128 KiB stack.DerivePQCKeyframe layout, 4 checks fail (stack written 8414 bytes below the burned region, both master-seed halves, the HKDF inner hash). With a 128 KiB burn, the small-thread test crashes (SIGSEGV).Test-harness fix (second commit): the released-stack copy now runs in a small leaf frame. Copying through a 192 KiB local array let
-fstack-clash-protectionprobes write into the copied region (Clang on x86 stores zeros every 4 KiB; GCC on AArch64 and 32-bit ARM also store), so the checks failed on correct builds. With the earlier copy, a Clang 21 Release build failed 4 depth checks; with this commit it passes. A standalone copy of the helpers passes on GCC and Clang x86-64 and AArch64, GCC 32-bit ARM and RISC-V 64 (QEMU user mode), at-O2and-O0. With 40 KiB of extra stack use injected into keygen, the Clang build still reportsstack written 18494 bytes below the burned regionand theSK_SEEDcopy.Unit tests:
build/bin/test_qbit --run_test='pqc_tests:pqc_sigcache_tests:txvalidationcache_tests:script_tests:net_tests:net_peer_connection_tests:net_peer_eviction_tests:netbase_tests:denialofservice_tests:peerman_tests:headers_sync_chainwork_tests:blockencodings_tests'(GCC): 136 cases, 571,351 assertions, no errors.pqc_testson the Clang 21 build: 40 cases, no errors. The three new tests passed 8 of 8 repeated runs on each compiler.Functional tests:
build/test/functional/test_runner.py --jobs=4: 22 of 22 runs passed.feature_p2mr,wallet_p2mr,wallet_p2mr_psbt(sign on RPC worker threads)feature_auxpow,feature_auxpow_commitment_activation,feature_minchainwork,feature_minchainwork_requested_blockp2p_block_sync(v1, v2),p2p_compactblocks,p2p_compactblocks_blocksonly,p2p_compactblocks_hb(v1, v2),p2p_fingerprint,p2p_getdatap2p_headers_sync_with_minchainwork,p2p_ibd_stalling(v1, v2),p2p_initial_headers_sync,p2p_invalid_messages,p2p_sendheaders,p2p_unrequested_blocksEarlier runs on the first commit's tree also passed the PQC and wallet P2MR unit suites (
pqc_tests,pqc_sigcache_tests,key_io_p2mr_tests,descriptor_p2mr_tests,psbt_p2mr_tests,p2mr_datapqchash_tests,wallet_p2mr_*; 160 cases, 7,072 assertions) and the functional testswallet_p2mr_multisig_restartandwallet_pqc_usage.Lint:
lint-includes,lint-tests,lint-locale-dependenceandgit diff --checkare clean.Not run: musl (see the open items), MSVC, and a real AArch64 or 32-bit ARM build of
test_qbit.Target Branch
mainor a maintainer-requested release branch such as0.1.x.This targets
1.x.xfor v1.0.1 (#181 prepares that release), and then 1.1.0-rc2.Risk / Review Notes
Notes:
memory_cleanseper key generation, key import, signature or derivation, small next to an SLH-DSA operation.pqc_testsruns on every CI target that runs unit tests.Open items:
doc/build/build-unix.md, then runbuild/bin/test_qbit --run_test='pqc_tests:wallet_p2mr_parallel_signing_tests'andbuild/test/functional/test_runner.py wallet_p2mr.py wallet_p2mr_psbt.py feature_p2mr.py. These sign on threads with musl's default 128 KiB stack.Qbit-Org/qbit-libbitcoinpqctag (proposedv0.3.1) carrying the same released-stack clearing inside the library, ideally without unrelated API changes, so the subtree can be bumped output-neutrally. The caller-side burn can stay as defence in depth or be dropped after that bump.Docs / Process Impact
Choose exactly one:
libbitcoinpqc Subtree Checklist (if
src/libbitcoinpqcchanged)Not applicable:
src/libbitcoinpqcis unchanged.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.