Add Absolution-based fuzzing#502
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
|
elf sizes
Stack consumption summary
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #502 +/- ##
===========================================
- Coverage 72.36% 72.36% -0.01%
===========================================
Files 71 71
Lines 10347 10348 +1
Branches 1876 1877 +1
===========================================
Hits 7488 7488
Misses 2113 2113
- Partials 746 747 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e1b41ec to
dd5cfbc
Compare
acffb98 to
65657f0
Compare
iartemov-ledger
left a comment
There was a problem hiding this comment.
Cool work!
1st round of the review, I played a bit with the fuzzer locally:
Bitcoin fuzzing — coverage & UBSan cleanup
1. What was run
- Edited
fuzzing/harness/fuzz_dispatcher.c: traded oneSIGN_PSBTstructured-lane
slot for aGET_EXTENDED_PUBKEYslot (still 16 slots,% 16routing unchanged).
diff --git a/fuzzing/harness/fuzz_dispatcher.c b/fuzzing/harness/fuzz_dispatcher.c
index 2fb48fd7..54866214 100644
--- a/fuzzing/harness/fuzz_dispatcher.c
+++ b/fuzzing/harness/fuzz_dispatcher.c
@@ -86,7 +86,7 @@ static const size_t btc_raw_n_commands =
/* Structured-lane command map (16 slots): slot count == weight. */
const fuzz_command_spec_t fuzz_commands[16] = {
- /* SIGN_PSBT x 8 (50%) */
+ /* SIGN_PSBT x 7 (43.75%) */
{.cla = CLA_APP,
.ins = SIGN_PSBT,
.p2_max = CURRENT_PROTOCOL_VERSION,
@@ -115,8 +115,10 @@ const fuzz_command_spec_t fuzz_commands[16] = {
.ins = SIGN_PSBT,
.p2_max = CURRENT_PROTOCOL_VERSION,
.flags = FUZZ_CMD_HAS_DATA},
+ /* GET_EXTENDED_PUBKEY x 1 (6.25%) — also picked in the raw lane; a
+ * structured slot drives the display/derivation path more often. */
{.cla = CLA_APP,
- .ins = SIGN_PSBT,
+ .ins = GET_EXTENDED_PUBKEY,
.p2_max = CURRENT_PROTOCOL_VERSION,
.flags = FUZZ_CMD_HAS_DATA},
/* REGISTER_WALLET x 2 (12.5%) */- Regenerated seeds:
generate-seed-corpus.py→fuzzing/base-corpus/
(needsBOLOS_SDK+APP_DIR). - Re-ran the campaign with a real budget:
WARMUP_SEC=120 MAIN_SEC=1800 WORKERS=8
(was the 30 s / 60 s / 2-worker smoke test).
Reproduce
# --- host: build the CFL image and enter it with the live tree mounted ---
# inside app-bitcoin-new folder
docker build -t btc-fuzz -f .clusterfuzzlite/Dockerfile .
docker run --rm -it \
-v <PATH_TO_BTC_APP>:/app \
-v <PATH_TO_SDK>:/ledger-secure-sdk \
btc-fuzz bash
# --- inside the container ---
export BOLOS_SDK=/ledger-secure-sdk
export APP_DIR=/app
unset LIB_FUZZING_ENGINE # else cmake links the missing /usr/lib/libFuzzingEngine.a
python3 /app/fuzzing/scripts/generate-seed-corpus.py /app/fuzzing/base-corpus
rm -rf /app/build/fast /app/build/cov /app/.fuzz-artifacts/cov-run
WARMUP_SEC=120 MAIN_SEC=1800 WORKERS=$(nproc) \
"$BOLOS_SDK"/fuzzing/scripts/app-campaign.sh --app-dir /app cov-run
# --- inspect (artifacts appear on host via the mount) ---
cat /app/.fuzz-artifacts/cov-run/key-files-coverage.txt
grep -rhc "runtime error" /app/.fuzz-artifacts/cov-run/targets/fuzz_app/*/worker-*/fuzz.log2. Coverage improvement (bitcoin-run → cov-run)
| Metric | Before | After | Δ |
|---|---|---|---|
| Lines | 61.34% | 76.21% | +14.9 |
| Regions | 53.19% | 66.21% | +13.0 |
| Functions executed | 82.10% | 90.74% | +8.6 |
| Branches | 52.86% | 66.56% | +13.7 |
Biggest per-file gains (lines):
| File | Before → After |
|---|---|
get_extended_pubkey.c |
20.0% → 77.4% (harness slot) |
stream_merkleized_map_value.c |
0.0% → 83.3% (was reachable after all) |
common/script.c |
24.3% → 65.1% |
sign_psbt/txhashes.c |
38.6% → 72.3% |
lib/policy.c |
68.4% → 86.7% |
common/wallet.c |
71.3% → 85.1% |
3. Issues found & fixes
| Site | UBSan check | Verdict | Action |
|---|---|---|---|
lib/policy.c:1959 older->n & ~SEQUENCE_LOCKTIME_TYPE_FLAG |
implicit-integer-sign-change (int -4194305 → uint32_t 4290772991) |
benign result (mask is correct), real consensus code | fix — ~(uint32_t) SEQUENCE_LOCKTIME_TYPE_FLAG |
fuzzing/mock/fuzz_sha256.c |
unsigned overflow + unsigned-shift-base | benign (SHA wraps by design) | mask — __attribute__((no_sanitize("integer"))) |
sign_psbt.h:37 address_index |
implicit-integer-sign-change | benign (BIP32 index, no neg sentinel) | fix — field int → uint32_t (kills 2 sites: preprocess_outputs.c:78, process_in_outs.c:122) |
lib/check_merkle_tree_sorted.c:27 |
implicit truncation | benign (bounded size) | fix — explicit (uint32_t) casts |
sign_psbt/preprocess_inputs.c:178,212 |
uint64 overflow | real ordering bug — BITCOIN_TOTAL_SUPPLY guard ran after the add |
fix — validate before each accumulation; dropped redundant trailing check |
Mask = mock only. Fixes = src. Only behavioral change is the amount-overflow ordering
(rejects an impossible over-supply input slightly earlier; valid PSBTs unaffected).
diff --git a/fuzzing/mock/fuzz_sha256.c b/fuzzing/mock/fuzz_sha256.c
index 58eb0316..4b5375aa 100644
--- a/fuzzing/mock/fuzz_sha256.c
+++ b/fuzzing/mock/fuzz_sha256.c
@@ -34,6 +34,11 @@ static inline void write_be32(uint8_t *p, uint32_t v)
p[3] = (uint8_t) v;
}
+// SHA-256 is defined over mod-2^32 arithmetic and 32-bit rotates, so every
+// integer op here wraps / shifts by design. Silence UBSan's opt-in integer
+// checks (unsigned overflow, unsigned-shift-base, truncation) for this
+// function only, so they don't drown out genuine findings elsewhere.
+__attribute__((no_sanitize("integer")))
static void fuzz_sha256_transform(uint32_t state[8], const uint8_t block[64])
{
uint32_t W[64];diff --git a/src/handler/lib/check_merkle_tree_sorted.c b/src/handler/lib/check_merkle_tree_sorted.c
index 39467045..b09519d4 100644
--- a/src/handler/lib/check_merkle_tree_sorted.c
+++ b/src/handler/lib/check_merkle_tree_sorted.c
@@ -24,10 +24,12 @@ int call_check_merkle_tree_sorted_with_callback(dispatcher_context_t *dispatcher
for (size_t cur_el_idx = 0; cur_el_idx < size; cur_el_idx++) {
uint8_t cur_el[MAX_CHECK_MERKLE_TREE_SORTED_PREIMAGE_SIZE];
+ // tree size and leaf index are bounded well below 2^32 in practice;
+ // cast to the callee's uint32_t parameters explicitly.
int cur_el_len = call_get_merkle_leaf_element(dispatcher_context,
root,
- size,
- cur_el_idx,
+ (uint32_t) size,
+ (uint32_t) cur_el_idx,
cur_el,
sizeof(cur_el));
diff --git a/src/handler/lib/policy.c b/src/handler/lib/policy.c
index 28ea7a7c..89f08f3c 100644
--- a/src/handler/lib/policy.c
+++ b/src/handler/lib/policy.c
@@ -1956,7 +1956,7 @@ static int check_older_node_cb(const policy_node_t *node, void *callback_state)
(void) callback_state;
if (node->type == TOKEN_OLDER) {
const policy_node_with_uint32_t *older = (const policy_node_with_uint32_t *) node;
- uint32_t n = older->n & ~SEQUENCE_LOCKTIME_TYPE_FLAG;
+ uint32_t n = older->n & ~(uint32_t) SEQUENCE_LOCKTIME_TYPE_FLAG;
if (n < 1 || n > 65535) {
return -1;
}
diff --git a/src/handler/sign_psbt.h b/src/handler/sign_psbt.h
index 6e149aa0..cf19a614 100644
--- a/src/handler/sign_psbt.h
+++ b/src/handler/sign_psbt.h
@@ -34,7 +34,7 @@ typedef struct {
// matched with the current key expression in the signing flow
bool is_change;
- int address_index;
+ uint32_t address_index;
// For an output, its scriptPubKey
// for an input, the prevout's scriptPubKey (either from the non-witness-utxo, or from the
diff --git a/src/handler/sign_psbt/preprocess_inputs.c b/src/handler/sign_psbt/preprocess_inputs.c
index 59bfc562..a789f483 100644
--- a/src/handler/sign_psbt/preprocess_inputs.c
+++ b/src/handler/sign_psbt/preprocess_inputs.c
@@ -175,6 +175,12 @@ bool __attribute__((noinline)) preprocess_inputs(
return false;
}
+ // sanity check before accumulating, to avoid overflowing the total
+ if (input.prevout_amount > BITCOIN_TOTAL_SUPPLY) {
+ PRINTF("Input amount exceeds Bitcoin total supply!\n");
+ SEND_SW(dc, SW_INCORRECT_DATA);
+ return false;
+ }
st->inputs_total_amount += input.prevout_amount;
}
@@ -209,6 +215,12 @@ bool __attribute__((noinline)) preprocess_inputs(
}
} else {
// we extract the scriptPubKey and prevout amount from the witness utxo
+ // sanity check before accumulating, to avoid overflowing the total
+ if (wit_utxo_prevout_amount > BITCOIN_TOTAL_SUPPLY) {
+ PRINTF("Input amount exceeds Bitcoin total supply!\n");
+ SEND_SW(dc, SW_INCORRECT_DATA);
+ return false;
+ }
st->inputs_total_amount += wit_utxo_prevout_amount;
input.prevout_amount = wit_utxo_prevout_amount;
@@ -217,13 +229,6 @@ bool __attribute__((noinline)) preprocess_inputs(
}
}
- if (input.prevout_amount > BITCOIN_TOTAL_SUPPLY) {
- // sanity check to avoid overflows in amounts
- PRINTF("Input amount exceed Bitcoin total supply!\n");
- SEND_SW(dc, SW_INCORRECT_DATA);
- return false;
- }
-
// check if the input is internal; if not, continue
int is_internal = is_in_out_internal(dc, st, sign_psbt_cache, &input.in_out, true);cfc10b1 to
91976c2
Compare
91976c2 to
8712113
Compare
8712113 to
c97f96a
Compare
Code coverage reportPer-file coverage
|
Description
Integrates the Secure SDK's Absolution-based fuzzing framework into the
Bitcoin app. The app opts in with a small
fuzzing/subtree andcmake/LedgerAppFuzz.cmakeThe fuzzer input is split into
[ prefix | tail ]: the prefix restores theapp's globals to a target state from a declarative model, and the tail is fed to
the real APDU dispatcher. This reaches deep, multi-APDU PSBT/wallet logic
(merkleized PSBTs, wallet-policy registration, swap checks) at near-stateless
speed.
Changes
fuzzing/— app harness, semantic host/model layer, mocks, invariants, and achecked-in base corpus.
.clusterfuzzlite/+.github/workflows/clusterfuzzlite.yml— ClusterFuzzLite CI.common/merkle: boundceil_lgand use unsigned shifts.common/parser_ext: guard zero-lengthmemmove.swap_checks/policyare gated behindFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION— no change to production builds.Opt-in, additive tooling: no on-device/firmware behavior change. Requires the
SDK's
aro/fuzzing-frameworkbranch (pinned in.clusterfuzzlite/Dockerfile).