Fix overflow check in parse_unsigned_decimal
What the developer wrote
Authored by Salvatore Ingala
Fix overflow check in parse_unsigned_decimal
Not all overflows were detected by the overflow check.
Fixed and added a regression test.
Also fixed the type discrepancy between uint32_t and size_t.
What changed, and why it matters
This commit fixes a bug in how the Ledger Bitcoin app reads large numbers from wallet policy strings. The old overflow check could miss some values that are too large to fit in a 32-bit unsigned integer, causing the app to silently accept and truncate an oversized number. This could let an attacker craft a wallet policy that passes validation when it should have been rejected, potentially changing the meaning of a spending condition such as a time lock. The fix corrects the math used to detect overflow and adds a regression test.
Treat this as a security-relevant bug fix. Review whether the fixed parser is used in any code path that accepts untrusted input, such as wallet policies received from a host. Ensure the regression test is run in CI and consider fuzzing parse_unsigned_decimal with boundary values.
Security signals we found
Integer overflow in a parser that validates wallet policies
Silent truncation could alter the semantics of a parsed policy
Affects miniscript/policy expressions such as `older(...)`
Regression test added for the overflow case
Evidence from the diff
In src/common/wallet.c, parse_unsigned_decimal parses a base-10 unsigned integer into a uint32_t. The original code stored the intermediate value in a size_t and checked overflow with if (10 * result + next_digit < result). Because size_t is typically 64 bits on the host and the multiplication/addition could wrap differently than a 32-bit uint32_t, some inputs that overflow uint32_t were not caught. The patch changes the accumulator to uint32_t and uses a safe pre-check result > (UINT32_MAX - next_digit) / 10 before multiplying. A unit test verifies that wsh(older(5368709120)) is rejected; the old code would have truncated it to 0x40000000.
Changed components
src/common/wallet.c:parse_unsigned_decimalwallet policy parserminiscript descriptor parsingInspect captured patch +17 / −5
diff --git a/src/common/wallet.c b/src/common/wallet.c
index 668ecb9..be6cad2 100644
--- a/src/common/wallet.c
+++ b/src/common/wallet.c
@@ -249,7 +249,7 @@ static PolicyNodeType parse_token(buffer_t *buffer) {
*/
static int parse_unsigned_decimal(buffer_t *buffer, uint32_t *out) {
uint8_t c;
- size_t result = 0;
+ uint32_t result = 0;
int digits_read = 0;
while (buffer_peek(buffer, &c) && is_digit(c)) {
++digits_read;
@@ -260,20 +260,20 @@ static int parse_unsigned_decimal(buffer_t *buffer, uint32_t *out) {
return -1;
}
- if (10 * result + next_digit < result) {
- return -1; // overflow, integer too large
+ if (result > (UINT32_MAX - next_digit) / 10) {
+ return -1;
}
result = 10 * result + next_digit;
buffer_seek_cur(buffer, 1);
}
- *out = result;
-
if (digits_read == 0) {
return -1;
}
+ *out = result;
+
return 0;
}
diff --git a/unit-tests/test_wallet.c b/unit-tests/test_wallet.c
index e3de279..f49efd9 100644
--- a/unit-tests/test_wallet.c
+++ b/unit-tests/test_wallet.c
@@ -365,6 +365,17 @@ static void test_get_policy_segwit_version(void **state) {
assert(get_policy_segwit_version(policy) == 1);
}
+// Regression test: parse_unsigned_decimal must reject values that overflow uint32_t.
+// 5368709120 == 10 * 0x20000000 == 0x1_4000_0000 which does not fit in 32 bits;
+// the old overflow check silently truncated it to 0x40000000, failing to detect
+// the overflow.
+static void test_parse_unsigned_decimal_overflow(void **state) {
+ (void) state;
+
+ uint8_t out[MAX_WALLET_POLICY_MEMORY_SIZE];
+ assert_true(0 > parse_policy("wsh(older(5368709120))", out, sizeof(out)));
+}
+
static void test_failures(void **state) {
(void) state;
@@ -897,6 +908,7 @@ int main() {
cmocka_unit_test(test_parse_policy_tr_musig_scriptpath),
cmocka_unit_test(test_parse_policy_tr_musig_keypath),
cmocka_unit_test(test_get_policy_segwit_version),
+ cmocka_unit_test(test_parse_unsigned_decimal_overflow),
cmocka_unit_test(test_failures),
cmocka_unit_test(test_miniscript_types),
cmocka_unit_test(test_traverse_single_leaf),
Why this scored 62/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.