Moderate 59 Bitcoin Committed Jul 31, 2026 · 12:44 UTC

Type consistency in psbt_parse_rawtx, and some other hardenings

1af86279 View source ↗

Public commit record

What the developer wrote

Authored by Salvatore Ingala

73/100 · Adequate

Type consistency in psbt_parse_rawtx, and some other hardenings

Change key_len and output_index to size_t for consistency with
other APIs, add some extra bounds checks.

Also adds:
- missing return value check on parser_consolidate_buffers
- reject trailing data during parsing
- zero txid_parser_outputs_t before starting parsing

✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context

The short version

What changed, and why it matters

This commit hardens a Bitcoin transaction parser in Ledger's app. It changes some numeric fields to safer types, adds bounds checks on transaction counts and sizes, rejects extra trailing bytes after a transaction, checks a previously-ignored internal buffer operation, and zeroes out result memory before use. These are defensive fixes that could prevent memory corruption, incorrect parsing, or information leakage, but the commit does not describe a specific active exploit.

Recommended action

Treat as a security hardening patch. Review whether any prior firmware version without these checks could be exposed to malformed PSBTs or non-witness UTXO data; consider a security advisory if a concrete vulnerability (e.g., parser state confusion or information leak) can be demonstrated. Otherwise, include in normal release notes as defensive hardening.

Security signals we found

Integer type narrowing and sentinel change from -1 to SIZE_MAX

Bounds checks before uint64_t to unsigned int casts

Return value of parser_consolidate_buffers now checked

Trailing data after parsed transaction now rejected

Output structure zero-initialized before parsing

Static linkage added to internal step-count constants

Technical analysis

Evidence from the diff

The patch modifies psbt_parse_rawtx and its callers. Key changes: (1) output_index and key_len changed from signed int to size_t, using SIZE_MAX as the sentinel for ‘no output queried’; (2) added UINT32_MAX bounds checks on scriptsig_size, n_inputs, n_outputs, cur_wit_stack_elements and cur_wit_elem_len before casting to unsigned int; (3) parser_consolidate_buffers return value is now checked and failures set parser_error; (4) after successful parsing, any trailing bytes remaining in the buffer cause parser_error; (5) outputs struct is zeroed at the start of call_psbt_parse_rawtx; (6) several step-count arrays are made static. These are hardening measures against integer/truncation issues, buffer mishandling and information disclosure from uninitialised fields.

Changed components

src/handler/lib/psbt_parse_rawtx.csrc/handler/lib/psbt_parse_rawtx.hsrc/handler/sign_psbt/amount_from_psbt.c

Inspect captured patch +66 / −17

diff --git a/src/handler/lib/psbt_parse_rawtx.c b/src/handler/lib/psbt_parse_rawtx.c
index 46a0816..cc9dd4f 100644
--- a/src/handler/lib/psbt_parse_rawtx.c
+++ b/src/handler/lib/psbt_parse_rawtx.c
@@ -62,7 +62,7 @@ typedef struct parse_rawtx_state_s {
         };
     };
 
-    int output_index;  // index of queried output, or -1
+    size_t output_index;  // index of queried output, or SIZE_MAX if no output is queried
 
     txid_parser_outputs_t *parser_outputs;
 
@@ -105,6 +105,12 @@ static int parse_rawtxinput_scriptsig_size(parse_rawtxinput_state_t *state, buff
     bool result = dbuffer_read_varint(buffers, &scriptsig_size);
 
     if (result) {
+        // The scriptSig is streamed and hashed, but never stored; still, the size must fit in the
+        // counters used to track the parsing progress.
+        if (scriptsig_size > UINT32_MAX) {
+            return -1;
+        }
+
         state->scriptsig_size = (unsigned int) scriptsig_size;
 
         crypto_hash_update_varint(&state->parent_state->hash_context->header, scriptsig_size);
@@ -165,7 +171,7 @@ static const parsing_step_t parse_rawtxinput_steps[] = {
     (parsing_step_t) parse_rawtxinput_sequence,
 };
 
-const size_t n_parse_rawtxinput_steps =
+static const size_t n_parse_rawtxinput_steps =
     sizeof(parse_rawtxinput_steps) / sizeof(parse_rawtxinput_steps[0]);
 
 /*   PARSER FOR A RAWTX OUTPUT */
@@ -178,8 +184,8 @@ static int parse_rawtxoutput_value(parse_rawtxoutput_state_t *state, buffer_t *b
 
         crypto_hash_update(&state->parent_state->hash_context->header, value_bytes, 8);
 
-        if (state->parent_state->output_index != -1) {
-            unsigned int relevant_output_index = (unsigned int) state->parent_state->output_index;
+        if (state->parent_state->output_index != SIZE_MAX) {
+            size_t relevant_output_index = state->parent_state->output_index;
             if (state->parent_state->out_counter == relevant_output_index) {
                 state->parent_state->parser_outputs->vout_value = value;
             }
@@ -203,8 +209,8 @@ static int parse_rawtxoutput_scriptpubkey_size(parse_rawtxoutput_state_t *state,
 
         crypto_hash_update_varint(&state->parent_state->hash_context->header, scriptpubkey_size);
 
-        if (state->parent_state->output_index != -1) {
-            unsigned int relevant_output_index = (unsigned int) state->parent_state->output_index;
+        if (state->parent_state->output_index != SIZE_MAX) {
+            size_t relevant_output_index = state->parent_state->output_index;
             if (state->parent_state->out_counter == relevant_output_index) {
                 if (scriptpubkey_size > MAX_PREVOUT_SCRIPTPUBKEY_LEN) {
                     // the requested output's scriptPubKey must fit in parser_outputs; such an
@@ -246,8 +252,8 @@ static int parse_rawtxoutput_scriptpubkey(parse_rawtxoutput_state_t *state, buff
 
         crypto_hash_update(&state->parent_state->hash_context->header, data, data_len);
 
-        if (state->parent_state->output_index != -1) {
-            unsigned int relevant_output_index = (unsigned int) state->parent_state->output_index;
+        if (state->parent_state->output_index != SIZE_MAX) {
+            size_t relevant_output_index = state->parent_state->output_index;
             if (state->parent_state->out_counter == relevant_output_index) {
                 unsigned int scriptpubkey_len =
                     state->parent_state->parser_outputs->vout_scriptpubkey_len;
@@ -283,7 +289,7 @@ static const parsing_step_t parse_rawtxoutput_steps[] = {
     (parsing_step_t) parse_rawtxoutput_scriptpubkey,
 };
 
-const size_t n_parse_rawtxoutput_steps =
+static const size_t n_parse_rawtxoutput_steps =
     sizeof(parse_rawtxoutput_steps) / sizeof(parse_rawtxoutput_steps[0]);
 
 /*   PARSER FOR A FULL RAWTX */
@@ -338,6 +344,10 @@ static int parse_rawtx_input_count(parse_rawtx_state_t *state, buffer_t *buffers
     uint64_t n_inputs;
     bool result = dbuffer_read_varint(buffers, &n_inputs);
     if (result) {
+        if (n_inputs > UINT32_MAX) {
+            return -1;
+        }
+
         state->n_inputs = (unsigned int) n_inputs;
 
         crypto_hash_update_varint(&state->hash_context->header, n_inputs);
@@ -381,6 +391,10 @@ static int parse_rawtx_output_count(parse_rawtx_state_t *state, buffer_t *buffer
     uint64_t n_outputs;
     bool result = dbuffer_read_varint(buffers, &n_outputs);
     if (result) {
+        if (n_outputs > UINT32_MAX) {
+            return -1;
+        }
+
         state->n_outputs = (unsigned int) n_outputs;
 
         crypto_hash_update_varint(&state->hash_context->header, n_outputs);
@@ -444,6 +458,9 @@ static int parse_rawtx_witnesses(parse_rawtx_state_t *state, buffer_t *buffers[2
             if (!dbuffer_read_varint(buffers, &cur_wit_stack_elements)) {
                 return 0;  // incomplete, read more data
             }
+            if (cur_wit_stack_elements > UINT32_MAX) {
+                return -1;
+            }
             state->is_cur_wit_stack_elements_read = true;
             state->cur_wit_stack_elements = (unsigned int) cur_wit_stack_elements;
             state->is_cur_wit_elem_len_read = false;
@@ -459,6 +476,9 @@ static int parse_rawtx_witnesses(parse_rawtx_state_t *state, buffer_t *buffers[2
                 if (!dbuffer_read_varint(buffers, &cur_wit_elem_len)) {
                     return 0;  // incomplete, read more data
                 }
+                if (cur_wit_elem_len > UINT32_MAX) {
+                    return -1;
+                }
                 state->is_cur_wit_elem_len_read = true;
                 state->cur_wit_elem_len = (unsigned int) cur_wit_elem_len;
                 state->cur_wit_el_bytes_read = 0;
@@ -510,7 +530,7 @@ static const parsing_step_t parse_rawtx_steps[] = {(parsing_step_t) parse_rawtx_
                                                    (parsing_step_t) parse_rawtx_witnesses,
                                                    (parsing_step_t) parse_rawtx_locktime};
 
-const size_t n_parse_rawtx_steps = sizeof(parse_rawtx_steps) / sizeof(parse_rawtx_steps[0]);
+static const size_t n_parse_rawtx_steps = sizeof(parse_rawtx_steps) / sizeof(parse_rawtx_steps[0]);
 
 static void cb_process_data(buffer_t *data, void *cb_state) {
     psbt_parse_rawtx_state_t *state = (psbt_parse_rawtx_state_t *) cb_state;
@@ -526,22 +546,39 @@ static void cb_process_data(buffer_t *data, void *cb_state) {
     int result =
         parser_run(parse_rawtx_steps, n_parse_rawtx_steps, &state->parser_context, buffers, pic);
     if (result == 0) {
-        parser_consolidate_buffers(buffers, sizeof(state->store));
+        // Each parsing step reads at most 32 bytes at a time, therefore the leftovers always fit
+        // in the store; still, we bail out rather than silently dropping data if that ever fails.
+        if (!parser_consolidate_buffers(buffers, sizeof(state->store))) {
+            PRINTF("Too many unparsed bytes\n");
+            state->parser_error = true;
+            return;
+        }
         state->store_data_length = store_buf.size;
     } else if (result < 0) {
         PRINTF("Parser error\n");
         state->parser_error = true;  // abort any remaining parsing
+    } else {
+        // Parsing is complete; no byte is expected after the end of the transaction.
+        if (dbuffer_get_length(buffers) > 0) {
+            PRINTF("Trailing data after the end of the transaction\n");
+            state->parser_error = true;
+            return;
+        }
+        state->store_data_length = 0;
     }
 }
 
 int call_psbt_parse_rawtx(dispatcher_context_t *dispatcher_context,
                           const merkleized_map_commitment_t *map,
                           const uint8_t *key,
-                          int key_len,
-                          int output_index,
+                          size_t key_len,
+                          size_t output_index,
                           txid_parser_outputs_t *outputs) {
     LOG_PROCESSOR(__FILE__, __LINE__, __func__);
 
+    // no field is left with an indeterminate value if a parsing step does not fill it in
+    memset(outputs, 0, sizeof(*outputs));
+
     cx_sha256_t hash_context;
     cx_sha256_init(&hash_context);
 
@@ -577,7 +614,7 @@ int call_psbt_parse_rawtx(dispatcher_context_t *dispatcher_context,
     }
 
     // If a specific output was requested, verify it was actually found
-    if (output_index >= 0 && (unsigned int) output_index >= flow_state.parser_state.n_outputs) {
+    if (output_index != SIZE_MAX && output_index >= flow_state.parser_state.n_outputs) {
         return -1;
     }
 
diff --git a/src/handler/lib/psbt_parse_rawtx.h b/src/handler/lib/psbt_parse_rawtx.h
index 0f60fc2..6d39d50 100644
--- a/src/handler/lib/psbt_parse_rawtx.h
+++ b/src/handler/lib/psbt_parse_rawtx.h
@@ -16,10 +16,17 @@ typedef struct {
  * Given a commitment to a merkleized map and a key, this flow parses it as a serialized bitcoin
  * transaction, computes the transaction id and optionally keeps track of the vout amount and
  * scriptPubkey of one of the outputs.
+ *
+ * If output_index is SIZE_MAX, no output is queried, and only the txid field of outputs is
+ * meaningful on success. Otherwise, the flow fails unless the transaction has an output with that
+ * index, and its value and scriptPubKey are returned in outputs.
+ *
+ * On failure, the content of outputs is unspecified; on success, any field that is not filled in
+ * is zeroed.
  */
 int call_psbt_parse_rawtx(dispatcher_context_t *dispatcher_context,
                           const merkleized_map_commitment_t *map,
                           const uint8_t *key,
-                          int key_len,
-                          int output_index,
+                          size_t key_len,
+                          size_t output_index,
                           txid_parser_outputs_t *outputs);
diff --git a/src/handler/sign_psbt/amount_from_psbt.c b/src/handler/sign_psbt/amount_from_psbt.c
index d91a0f7..01928ca 100644
--- a/src/handler/sign_psbt/amount_from_psbt.c
+++ b/src/handler/sign_psbt/amount_from_psbt.c
@@ -50,13 +50,18 @@ int __attribute__((noinline)) get_amount_scriptpubkey_from_psbt_nonwitness(
         return -1;
     }
 
+    // SIZE_MAX is reserved by call_psbt_parse_rawtx to mean "no output is queried"
+    if (prevout_n >= SIZE_MAX) {
+        return -1;
+    }
+
     txid_parser_outputs_t parser_outputs;
     // request non-witness utxo, and get the prevout's value and scriptpubkey
     int res = call_psbt_parse_rawtx(dc,
                                     input_map,
                                     (uint8_t[]) {PSBT_IN_NON_WITNESS_UTXO},
                                     1,
-                                    prevout_n,
+                                    (size_t) prevout_n,
                                     &parser_outputs);
     if (res < 0) {
         PRINTF("Parsing rawtx failed\n");

Risk score

Why this scored 59/100

Our methodology →

Potential impact 18/30

Exploitability 12/25

Stealth signal 8/15

Affected reach 10/15

Confidence 7/10

Evidence quality 4/5

Human-validated context

Community notes

Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.

No validated notes yet.

The AI analysis stands alone for now. Submit a note if you can add evidence or important context.

What the developer wrote

What changed, and why it matters

Security signals we found

Evidence from the diff

Changed components

Why this scored 59/100

Community notes

Related watches

Display total amount of external inputs when reliable

Trustworthy sign_psbt amount/fee display for non-default sighash: snapshots

Trustworthy sign_psbt amount/fee display for non-default sighash: integration tests