Ported various bug fixes from the develop branch to nanos branch#429
Ported various bug fixes from the develop branch to nanos branch#429iartemov-ledger wants to merge 10 commits intonanosfrom
Conversation
(cherry picked from commit 4b1f0e0)
(cherry picked from commit 193ff56)
…ting prematurely for some PSBTs. Added explanatory comment (cherry picked from commit aea89fd)
The function would cause a buffer overflow passed a buffer longer than 32 bytes. This is not an issue today as it was only ever used for a fixed short string (WALLET_SLIP0021_LABEL). The workaround to copy into a local buffer is still needed, so we defensively forbid longer strings. (cherry picked from commit c63f73c)
…throw() SDK function (cherry picked from commit e289daf)
(cherry picked from commit aeeeaf0)
- get_merkle_preimage failed to update the data_ptr pointer - fpt_der_data_callback would read corrupted data if a zero-length chunk (or one starting with a very large varint that does not fit in the first chunk) is received. (cherry picked from commit 0586ab2)
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
b35cf6d to
161fcd3
Compare
5a1501b to
dc0f2e5
Compare
There was a problem hiding this comment.
Pull request overview
This pull request ports various bug fixes from the develop branch to the main branch, focusing on correctness, error handling, and infrastructure updates.
Changes:
- Fixed multiple critical bugs in C code including memset argument error, buffer handling, and type mismatches
- Updated Bitcoin RPC test calls from deprecated
getwalletinfo()togetbalances()API - Pinned speculos to version 0.25.5 and updated build infrastructure to use CMake for Bitcoin Core
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| unit-tests/test_script.c | Added test for 0x00 byte sequence push and fixed loop to test values 1-16 instead of 0-16 |
| src/handler/sign_psbt/extract_bip32_derivation.c | Fixed varint reading to single byte, corrected memmove offset calculation, added empty chunk handling |
| src/handler/sign_psbt.c | Changed return value from -1 to 0 for invalid derivation path length (continue vs abort) |
| src/handler/register_wallet.c | Added error handling for compute_wallet_hmac return value |
| src/handler/lib/psbt_parse_rawtx.c | Fixed type mismatch (bool to int) and added buffer overflow check |
| src/handler/lib/policy.c | Fixed memset bug (sizeof(memset) to sizeof(used)) |
| src/handler/lib/get_merkle_preimage.c | Refactored pointer calculation for clarity |
| src/crypto.c | Added buffer length validation and proper 64-byte temporary buffer for SDK function |
| src/crypto.h | Updated documentation to specify 32-byte label length limit |
| src/common/script.c | Fixed condition to exclude 0 from non-minimal push check (only 1-16 need special opcodes) |
| tests/test_e2e_tapscripts.py | Updated to use getbalances() instead of deprecated getwalletinfo() |
| tests/test_e2e_multisig.py | Updated to use getbalances() instead of deprecated getwalletinfo() |
| tests/test_e2e_miniscript.py | Updated to use getbalances() instead of deprecated getwalletinfo() |
| tests/conftest.py | Changed bitcoind path from hardcoded to use PATH |
| tests/requirements.txt | Pinned speculos to 0.25.5, removed from ragger extras |
| .github/workflows/builder-image-workflow.yml | Updated checkout action to v4, changed tag to NanoS_Blue_baseline |
| .github/workflows/build_and_functional_tests.yml | Added custom container image and test options configuration |
| .github/workflows/Dockerfile | Updated to use speculos 0.25.5, switched from autotools to CMake for Bitcoin Core build |
| CHANGELOG.md | Added entry for version 2.2.6 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmake -B build -DENABLE_IPC=OFF && \ | ||
| cmake --build build && \ | ||
| cmake --install build |
There was a problem hiding this comment.
This Docker build stage clones the external repository https://github.com/bitcoin/bitcoin.git from an unpinned, mutable branch and then runs its CMake build and install steps via cmake -B build ..., cmake --build build, and cmake --install build. If the upstream repo or its CMake scripts are ever compromised, arbitrary code can execute during the GitHub Actions build and poison the published speculos-bitcoin image, potentially exfiltrating GITHUB_TOKEN or injecting backdoored binaries used in CI. To mitigate this supply-chain risk, pin the Bitcoin Core source to a specific commit or signed release and verify its integrity (e.g., via signatures or checksums) before building and installing it, instead of tracking the moving default branch.
There was a problem hiding this comment.
We do it already in the develop branch, but I agree, something to do with it in another PR.
Test run with physical Nano S (OS version 2.1.0)
--backend ledgerwalettestsSizes comparison
(
make -C . -j DEBUG=0 COIN=bitcoin TARGET=nanos BOLOS_SDK=$NANOS_SDK)Elf sections size before (
nanos) branch:Elf sections sizes after:
=> so no changes
CI test results
Call Ledger guidelines_enforcer / Dispatch check / Check APP_LOAD_PARAMS is expected to fail as we've removed Nano S and changed derivation paths parameters.