What the developer wrote
Authored by Mike Tolkachev
url: improve bounds checking
What changed, and why it matters
This commit tightens bounds checks in URL encoding/decoding functions used in Blockstream Jade, a hardware wallet. The changes fix potential off-by-one/out-of-bounds memory reads and writes when handling percent-encoded characters. Because Jade processes untrusted data (e.g., QR codes, USB messages, or companion-app URLs), a malformed URL fragment could previously trigger a buffer overread or, in encoding, a truncated/overwritten output buffer. The patch is defensive and partial: it improves checks but does not fully restructure the API, so residual risks from caller misuse remain possible.
Treat as a security fix and include in the next firmware release. Review callers of urldecode()/urlencode() to ensure src_len and dest_len are always passed correctly and that return-value failures are handled rather than silently using truncated output. Add unit tests covering edge cases: empty input, input ending with '%' or '%X', oversized hex sequences, and minimum-size output buffers.
Security signals we found
Bounds-check rewrite in URL decoder to prevent out-of-bounds reads before percent-decoding
Cast to unsigned char moved into isxdigit() to avoid undefined behavior with signed char inputs
Destination bounds checks in URL encoder changed from pointer-subtraction form to remaining-space form, mitigating possible underflow/wrap
Functions process externally supplied URL-encoded data, a common attack surface
Commit title explicitly says 'improve bounds checking', indicating security-relevant hardening
Evidence from the diff
In urldecode(), the old code read src[1] and src[2] before verifying the source pointer was at least two bytes away from src_end, and it cast to unsigned char only after the read. The new check (src_end - src > 2) is evaluated before indexing src[1] and src[2], and the casts to unsigned char are applied inside isxdigit() to avoid UB on negative char values. In urlencode(), the destination bounds checks were rewritten from dest > dest_end - N (which can underflow if dest_end <= dest or if pointer arithmetic wraps) to dest_end - dest < N, a safer idiom. The patch is small (+5/-7) and does not add new tests or change function signatures; it is best characterized as a hardening fix for memory-safety bugs that could be reachable from external input.
Changed components
main/utils/urldecode.curldecode()urlencode()Inspect captured patch +5 / −7
diff --git a/main/utils/urldecode.c b/main/utils/urldecode.c
index 7adb9c9..d94fe09 100644
--- a/main/utils/urldecode.c
+++ b/main/utils/urldecode.c
@@ -45,12 +45,10 @@ bool urldecode(const char* src, const size_t src_len, char* dest, const size_t d
return false;
}
- const unsigned char c1 = src[1];
- const unsigned char c2 = src[2];
-
- if ((*src == '%') && (src < src_end - 2) && isxdigit(c1) && isxdigit(c2)) {
+ if ((*src == '%') && (src_end - src > 2) && isxdigit((unsigned char)src[1])
+ && isxdigit((unsigned char)src[2])) {
// Encoded hex character
- *dest++ = (16 * map_char(c1)) + map_char(c2);
+ *dest++ = (16 * map_char(src[1])) + map_char(src[2]);
src += 3;
} else if (*src == '+') {
// Encoded <space>
@@ -81,7 +79,7 @@ bool urlencode(const char* src, const size_t src_len, char* dest, const size_t d
const char* dest_end = dest + dest_len;
while (src < src_end) {
- if (dest > dest_end - 2) {
+ if (dest_end - dest < 2) {
// Destination insufficient - need at least 1 char for encoding and 1 for nul-terminator.
// Truncate (terminate) here and return false.
*dest = '\0';
@@ -96,7 +94,7 @@ bool urlencode(const char* src, const size_t src_len, char* dest, const size_t d
*dest++ = '+';
++src;
} else {
- if (dest > dest_end - 4) {
+ if (dest_end - dest < 4) {
// Destination insufficient - need 3 chars for encoding and 1 for nul-terminator.
// Truncate (terminate) here and return false.
*dest = '\0';
Why this scored 61/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.