descriptor: validate registered descriptors when re-loading them
What the developer wrote
Authored by Mike Tolkachev
descriptor: validate registered descriptors when re-loading them
What changed, and why it matters
This commit tightens validation when a Bitcoin hardware wallet reloads saved 'descriptors' (recipe-like data that describes how coins can be spent). It changes several length checks from 'greater than' to 'greater than or equal to', and adds a cap on the number of stored values. The change prevents a stored descriptor from claiming a length exactly equal to the fixed buffer size, which could leave the buffer without a terminating zero byte and cause memory corruption or crashes when the data is later used. Because the data is reloaded from the device's own storage, exploitation likely requires an attacker who can already tamper with stored data or trick the user into registering a malicious descriptor.
Treat this as a security hardening fix and include it in the next firmware release. Review other deserialization paths in descriptor.c and elsewhere for similar '>' vs '>=' off-by-one issues, and confirm that all string fields are either null-terminated or handled as length-prefixed blobs. If a CVE is desired, request one from MITRE or the vendor's security contact; the commit alone does not assign one.
Security signals we found
Off-by-one length validation allowing buffer-filling input
Missing null-terminator safeguard on fixed-size string buffers
Untrusted persisted data parsed without sufficient bounds checks
Potential out-of-bounds access in descriptor values array
Patch hardens deserialization but does not add new feature
Evidence from the diff
In main/descriptor.c, descriptor_from_bytes() deserializes registered descriptors from persistent storage. The patch changes three boundary checks from ‘>’ to ‘>=’: script_len vs sizeof(descriptor->script), key_len vs sizeof(map_entry->key), and value_len vs sizeof(map_entry->value). It also adds an upper bound on descriptor->num_values against MAX_ALLOWED_SIGNERS. The original ‘>’ checks allowed a length exactly equal to the destination buffer size. For C-string fields, that would leave no room for a null terminator, so subsequent string operations could read or write past the buffer. The num_values check prevents an out-of-bounds write into descriptor->values[]. The commit message says this is validation of registered descriptors when re-loading them, implying prior trust assumptions were insufficient.
Changed components
main/descriptor.cdescriptor_from_bytes()registered descriptor storage and reload pathInspect captured patch +7 / −3
diff --git a/main/descriptor.c b/main/descriptor.c
index 2a45e21..f235580 100644
--- a/main/descriptor.c
+++ b/main/descriptor.c
@@ -661,7 +661,7 @@ bool descriptor_from_bytes(const uint8_t* bytes, const size_t bytes_len, descrip
// Descriptor script
memcpy(&descriptor->script_len, read_ptr, sizeof(descriptor->script_len));
- if (descriptor->script_len > sizeof(descriptor->script)) {
+ if (descriptor->script_len >= sizeof(descriptor->script)) {
JADE_LOGE("Bad script_len stored registered descriptor data");
return false;
}
@@ -672,13 +672,17 @@ bool descriptor_from_bytes(const uint8_t* bytes, const size_t bytes_len, descrip
// Any data values
memcpy(&descriptor->num_values, read_ptr, sizeof(descriptor->num_values));
+ if (descriptor->num_values > MAX_ALLOWED_SIGNERS) {
+ JADE_LOGE("Bad num_values in stored registered descriptor data");
+ return false;
+ }
read_ptr += sizeof(descriptor->num_values);
for (uint8_t i = 0; i < descriptor->num_values; ++i) {
string_value_t* const map_entry = descriptor->values + i;
memcpy(&map_entry->key_len, read_ptr, sizeof(map_entry->key_len));
- if (map_entry->key_len > sizeof(map_entry->key)) {
+ if (map_entry->key_len >= sizeof(map_entry->key)) {
JADE_LOGE("Bad key_len stored registered descriptor data");
return false;
}
@@ -688,7 +692,7 @@ bool descriptor_from_bytes(const uint8_t* bytes, const size_t bytes_len, descrip
read_ptr += map_entry->key_len;
memcpy(&map_entry->value_len, read_ptr, sizeof(map_entry->value_len));
- if (map_entry->value_len > sizeof(map_entry->value)) {
+ if (map_entry->value_len >= sizeof(map_entry->value)) {
JADE_LOGE("Bad value_len stored registered descriptor data");
return false;
}
Why this scored 63/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.