fix(core): validate length in the nRF service callbacks
What the developer wrote
Authored by tychovrahe
fix(core): validate length in the nRF service callbacks
`nrf_management_rx_cb()` and `nrf_test_cb()` select on `data[0]` without
checking that a byte is present, and the two management handlers went further
on the same assumption, computing `len - 1` and clamping the result with
MIN(). A zero-length frame turned that subtraction into 4294967295, so the
clamp always picked the full struct size.
The reads stayed inside the SPI frame buffer, so nothing was out of bounds.
The defect is that a truncated response was accepted as a complete one:
- a one-byte MGMT_RESP_INFO set `info_valid` while leaving `drv->info`
holding whatever a previous exchange left there,
- a one-byte MGMT_RESP_AUTH_RESPONSE set `auth_data_valid` with `auth_data`
still zeroed, which `nrf_authenticate()` then fed to the MAC comparison.
Both fail closed, but `nrf_authenticate()` gates boot in production builds and
has no business deciding anything from a frame the nRF never filled in.
Require the whole payload before marking the state valid, and set the flag
after the copy rather than before. The minimums match what the nRF sends:
`send_info()` transmits `1 + sizeof(nrf_info_t)` bytes and
`mgmt_process_challenge()` more than the digest we read. An older nRF that
sent less would now fail `nrf_get_info()` and fall through to the SMP path,
which forces an update - the outcome we want.
This is the same defect class as the recent fix to
`ble_process_rx_msg_mac()`; these were the remaining callbacks.
[no changelog]
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What changed, and why it matters
This commit fixes a bug in the Trezor hardware wallet's code that talks to the nRF wireless chip over SPI. The code read the first byte of a received message to decide what kind of response it was, without first checking whether any bytes had actually been received. For two important response types—device info and authentication data—it then marked the data as 'valid' before copying it. A zero-length or truncated message could therefore make the device believe it had received valid authentication data when it had not. The device 'fails closed' (it does not unlock anything from bad data), but the bug could still let an empty or truncated frame influence the boot-time authentication check, which is not a safe design. The fix requires the full expected payload length before marking anything valid and moves the 'valid' flag to after the copy.
Apply the patch. Ensure all nRF/Bluetooth SPI callbacks validate minimum frame length before indexing `data[0]` and before marking any parsed state as valid. Audit remaining callbacks for the same pattern (set-valid-then-copy). Consider adding static analysis rules to catch `len - 1` on unsigned lengths and validity-flag ordering issues.
Security signals we found
Integer underflow in length calculation: `len - 1` with `uint32_t len == 0` produces `UINT32_MAX`
Use of untrusted length in `MIN()` without minimum-length validation
Validity flag set before data copy, allowing truncated frames to be treated as valid
Boot-gating authentication decision influenced by unvalidated nRF frame
Same defect class as a prior fix to `ble_process_rx_msg_mac()`
Evidence from the diff
In core/embed/io/nrf/stm32u5/nrf.c, nrf_management_rx_cb() previously switched on data[0] and, for MGMT_RESP_INFO and MGMT_RESP_AUTH_RESPONSE, set drv->info_valid / drv->auth_data_valid before copying with memcpy(..., MIN(len - 1, sizeof(...))). Because len is a uint32_t, a zero-length frame made len - 1 underflow to UINT32_MAX, so MIN() selected the full struct size and the copy stayed within the SPI frame buffer. The real defect was acceptance of a truncated response as complete: a one-byte MGMT_RESP_INFO frame set info_valid while leaving stale drv->info, and a one-byte MGMT_RESP_AUTH_RESPONSE set auth_data_valid with auth_data still zeroed. nrf_authenticate() then used that zeroed buffer in a MAC comparison and gates boot in production builds. The patch adds an explicit len < 1 guard, requires len >= 1 + sizeof(...) for each management response, copies the full payload, and only then sets the validity flag. A similar len < 1 guard is added to nrf_test_cb() in nrf_test.c.
Changed components
core/embed/io/nrf/stm32u5/nrf.ccore/embed/io/nrf/stm32u5/nrf_test.cnrf_management_rx_cb()nrf_test_cb()nrf_authenticate() (consumer of auth_data_valid)Inspect captured patch +20 / −2
### core/embed/io/nrf/stm32u5/nrf.c
@@ -105,14 +105,27 @@ void nrf_management_rx_cb(const uint8_t *data, uint32_t len) {
return;
}
+ if (len < 1) {
+ // insufficient data length
+ return;
+ }
+
switch (data[0]) {
case MGMT_RESP_INFO:
+ if (len < 1 + sizeof(nrf_info_t)) {
+ // insufficient data length
+ break;
+ }
+ memcpy(&drv->info, &data[1], sizeof(nrf_info_t));
drv->info_valid = true;
- memcpy(&drv->info, &data[1], MIN(len - 1, sizeof(nrf_info_t)));
break;
case MGMT_RESP_AUTH_RESPONSE:
+ if (len < 1 + sizeof(drv->auth_data)) {
+ // insufficient data length
+ break;
+ }
+ memcpy(&drv->auth_data, &data[1], sizeof(drv->auth_data));
drv->auth_data_valid = true;
- memcpy(&drv->auth_data, &data[1], MIN(len - 1, sizeof(drv->auth_data)));
break;
default:
break;
### core/embed/io/nrf/stm32u5/nrf_test.c
@@ -54,6 +54,11 @@ typedef struct {
static nrf_test_t g_nrf_test;
void nrf_test_cb(const uint8_t *data, uint32_t len) {
+ if (len < 1) {
+ // insufficient data length
+ return;
+ }
+
switch (data[0]) {
case PRODTEST_RESP_SPI:
g_nrf_test.answered_spi = true;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.