otp: be more strict parsing counter/period.
What the developer wrote
Authored by Jon Griffiths
otp: be more strict parsing counter/period.
Suggested-by: Jordan Mecom (@jmecom) via https://github.com/jmecom/odo
Adds a parse_uint64 to parse decimal unsigned integers.
What changed, and why it matters
This commit tightens how Blockstream Jade handles the numbers inside OTP setup URLs (the 'counter' for HOTP codes and the 'period' for TOTP codes). Before the change, the code copied the number into a small temporary buffer and converted it with standard C library functions, which could silently accept non-digit characters, ignore overflow, or be fed an oversized input. The patch adds a strict decimal parser that only accepts digits, rejects values too large for the variable, and enforces length limits. The commit message credits the change to an external suggestion, but does not call it a security fix.
Treat as a defensive hardening patch. Review whether the prior lenient parsing could have led to unexpected OTP behavior or denial of service, and consider whether additional OTP URI fields (e.g., digits, algorithm) need similar strict parsing. No urgent exploit mitigation is evident from the diff alone.
Security signals we found
Replaces unsafe strtoull/strtoul parsing of untrusted URI query parameters with strict bounded decimal parser
Adds explicit length and overflow checks for numeric OTP parameters
Adds regression tests for malformed counter/period values
Suggested-by external contributor, indicating independent review
Evidence from the diff
The patch replaces ad-hoc strtoull/strtoul parsing of the ‘counter’ and ‘period’ query parameters in otpauth.c with a new parse_uint64() helper in util.c. The helper validates that every character is a decimal digit, caps input length at 20 characters, and checks for uint64_t overflow during multiplication. For TOTP period, the parsed value is additionally constrained to <= 0xff before being stored as an 8-bit field. Tests are added covering empty, too-long, overflow, and non-numeric inputs. The change is defensive hardening rather than a clear exploit fix: the prior code already limited HOTP counter length to 20 and TOTP period length to 3, so overflow paths were constrained, but strtoul-family functions still permit trailing/leading non-digit characters and do not signal parse failures.
Changed components
main/otpauth.cmain/utils/util.cmain/utils/util.htest_jade.pyInspect captured patch +76 / −18
diff --git a/main/otpauth.c b/main/otpauth.c
index 7d2e61a..1260a52 100644
--- a/main/otpauth.c
+++ b/main/otpauth.c
@@ -176,33 +176,23 @@ bool otp_uri_to_ctx(const char* uri, size_t uri_len, otpauth_ctx_t* otp_ctx)
// Get counter(hotp) or period(totp)
if (otp_ctx->type_len == 4 && !strncmp("hotp", otp_ctx->type, otp_ctx->type_len)) {
- otp_ctx->otp_type = OTPTYPE_HOTP;
-
// 'counter' is mandatory for hotp
OTP_CHECK_BOOL_RETURN(get_query_argument(query, query_len, "counter", &tmp, &tmp_len));
- OTP_CHECK_BOOL_RETURN(tmp && tmp_len > 0 && tmp_len <= 20);
-
- // Needs copying to nul-terminated buffer before converting
- char buf[20];
- memcpy(buf, tmp, tmp_len);
- buf[tmp_len] = '\0';
- otp_ctx->counter = strtoull(buf, NULL, 10);
+ OTP_CHECK_BOOL_RETURN(tmp && tmp_len > 0);
+ OTP_CHECK_BOOL_RETURN(parse_uint64(tmp, tmp_len, &otp_ctx->counter));
+ otp_ctx->otp_type = OTPTYPE_HOTP;
} else if (otp_ctx->type_len == 4 && strncmp("totp", otp_ctx->type, otp_ctx->type_len) == 0) {
- otp_ctx->otp_type = OTPTYPE_TOTP;
-
// Period can be specified, but defaults to 30s
get_query_argument(query, query_len, "period", &tmp, &tmp_len);
if (!tmp) {
otp_ctx->period = 30;
} else {
- OTP_CHECK_BOOL_RETURN(tmp_len > 0 && tmp_len <= 3);
-
- // Needs copying to nul-terminated buffer before converting
- char buf[4];
- memcpy(buf, tmp, tmp_len);
- buf[tmp_len] = '\0';
- otp_ctx->period = strtoul(buf, NULL, 10);
+ uint64_t value64;
+ OTP_CHECK_BOOL_RETURN(parse_uint64(tmp, tmp_len, &value64));
+ OTP_CHECK_BOOL_RETURN(value64 <= 0xff);
+ otp_ctx->period = value64 & 0xff;
}
+ otp_ctx->otp_type = OTPTYPE_TOTP;
} else {
JADE_LOGE("Unknown OTP type: %.*s", otp_ctx->type_len, otp_ctx->type);
return false;
diff --git a/main/utils/util.c b/main/utils/util.c
index fe5218d..22d5f9f 100644
--- a/main/utils/util.c
+++ b/main/utils/util.c
@@ -175,4 +175,28 @@ bool bin_to_base32(const uint8_t* bin, const size_t bin_len, char* b32_str, cons
*out = '\0';
return true;
}
+
+bool parse_uint64(const char* str, const size_t str_len, uint64_t* value_out)
+{
+ const uint64_t max_mul = 0xffffffffffffffffull / 10;
+ const uint64_t max_mod = 0xffffffffffffffffull % 10;
+ JADE_ASSERT(str && value_out);
+ if (!str_len || str_len > 20) {
+ return false; // Empty or too long to fit in uint64_t
+ }
+ uint64_t value = 0;
+ for (size_t i = 0; i < str_len; ++i) {
+ char ch = str[i];
+ if (ch < '0' || ch > '9') {
+ return false;
+ }
+ ch -= '0';
+ if (value > max_mul || (value == max_mul && ch > max_mod)) {
+ return false; // Value too large
+ }
+ value = value * 10 + ch;
+ }
+ *value_out = value;
+ return true;
+}
#endif // AMALGAMATED_BUILD
diff --git a/main/utils/util.h b/main/utils/util.h
index 4028224..f45301b 100644
--- a/main/utils/util.h
+++ b/main/utils/util.h
@@ -98,6 +98,9 @@ static inline void map_string(char* s, int (*fnmap)(int))
void split_text(
const char* src, size_t len, size_t wordlen, char* output, size_t output_len, size_t* num_words, size_t* written);
+// Parse a uint64 from a string. Allows leading zeros but no non-digit chars
+bool parse_uint64(const char* str, size_t str_len, uint64_t* value_out);
+
// Bip32 path utils
static inline bool ishardened(const uint32_t n) { return n & 0x80000000; }
static inline uint32_t harden(const uint32_t n) { return n | 0x80000000; }
diff --git a/test_jade.py b/test_jade.py
index 26b6c78..7af6791 100644
--- a/test_jade.py
+++ b/test_jade.py
@@ -3573,6 +3573,27 @@ def test_hotp(jadeapi):
rslt = jadeapi.get_otp_code(hotp_name)
assert rslt == expected
+ if not args.libjade:
+ return # Only test bad parameters on libjade
+
+ # Test bad uri parameters
+ hotp_uri = '&'.join(hotp_uri.split('&')[:-1])
+ bad_params = [
+ '', # counter not given
+ '&counter=', # counter empty
+ '&counter=000000000000000000001', # counter too long
+ '&counter=18446744073709551616', # counter too large
+ '&counter=abc', # counter not a number
+ ]
+ for params in bad_params:
+ try:
+ jadeapi.register_otp(hotp_name, hotp_uri + params)
+ assert False, f'hotp error not raised for "{params}"'
+ except JadeError as err:
+ assert err.code == JadeError.BAD_PARAMETERS
+ assert 'Failed to parse otp record' in err.message
+ continue
+
# Test according to otp spec (rfc6238)
def test_totp(jadeapi):
@@ -3608,6 +3629,26 @@ def test_totp(jadeapi):
rslt = jadeapi.get_otp_code(totp_name, value_override=timestamp)
assert rslt == expected[i]
+ if not args.libjade:
+ return # Only test bad parameters on libjade
+
+ # Test bad uri parameters
+ totp_uri = '&'.join(totp_uri.split('&')[:-2])
+ bad_params = [
+ '&digits=', # digits not given
+ '&digits=7', # digits not valid (6 or 8)
+ '&period=', # period empty
+ '&period=256', # period too large
+ ]
+ for params in bad_params:
+ try:
+ jadeapi.register_otp(totp_name, totp_uri + params)
+ assert False, f'totp error not raised for "{params}"'
+ except JadeError as err:
+ assert err.code == JadeError.BAD_PARAMETERS
+ assert 'Failed to parse otp record' in err.message
+ continue
+
# NOTE:
# There is some uncertainty around secrets padding when shorter than the hash size.
Why this scored 59/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.