enhance the fp data checking logic
What the developer wrote
Authored by aaron
enhance the fp data checking logic
What changed, and why it matters
This commit tightens how the Keystone hardware wallet's fingerprint sensor driver checks incoming data lengths. Before the change, the code could be tricked into reading or writing past the end of a fixed-size buffer if a malformed fingerprint message claimed a bogus length, or it could misread the length bytes and act too early. The patch adds bounds checks, resets state on bad input, and fixes a timing issue when deciding whether both length bytes have arrived. This is a defensive hardening fix for a likely buffer overflow / out-of-bounds access in interrupt-driven fingerprint UART traffic.
Treat this commit as a security hardening fix. Review whether prior firmware versions without these checks are exposed to out-of-bounds access via crafted fingerprint sensor traffic, and consider issuing a firmware update or advisory if the device accepts fingerprint commands from a reachable bus. Audit nearby fingerprint command handlers for additional missing length checks.
Security signals we found
Buffer bounds check added before indexing intrRecvBuffer[rcvByteCount]
Length-field validation added (totalLen > 3 && totalLen <= RCV_MSG_MAX_LEN)
State reset (memset_s + counters zeroed) on malformed / oversized input
Frame-complete comparison hardened from == to >=
Off-by-one/timing fix: length now computed after both length bytes received (rcvcByteCount == 3)
Buffer-name mismatch corrected in timeout reset path
Evidence from the diff
In FingerprintIsrRecvProcess(), an interrupt handler consuming bytes from the fingerprint module, the patch: (1) fixes a buffer-name typo (g_intrRecvBuffer -> intrRecvBuffer) in the timeout reset path; (2) adds an explicit defensive bound preventing rcvByteCount from indexing intrRecvBuffer[] when it is negative or >= RCV_MSG_MAX_LEN; (3) changes the length-parsed test from rcvByteCount == 2 to rcvByteCount == 3 so totalLen is computed only after both length bytes plus the header are stored; (4) validates totalLen (must be > 3 and <= RCV_MSG_MAX_LEN) and resets state on junk lengths; and (5) changes the frame-complete test from == to >= totalLen. These changes prevent out-of-bounds writes/reads and state confusion caused by malformed or truncated fingerprint frames.
Changed components
src/managers/fingerprint_process.cFingerprint ISR receive state machineFingerprint sensor UART/command parserInspect captured patch +16 / −3
diff --git a/src/managers/fingerprint_process.c b/src/managers/fingerprint_process.c
index 36973af..0ae6e2f 100644
--- a/src/managers/fingerprint_process.c
+++ b/src/managers/fingerprint_process.c
@@ -1166,11 +1166,19 @@ void __inline FingerprintIsrRecvProcess(uint8_t byte)
if (rcvByteCount != 0) {
if (tick - lastTick > 200) {
rcvByteCount = 0;
- memset_s(g_intrRecvBuffer, RCV_MSG_MAX_LEN, 0, RCV_MSG_MAX_LEN);
+ memset_s(intrRecvBuffer, RCV_MSG_MAX_LEN, 0, RCV_MSG_MAX_LEN);
}
}
lastTick = tick;
+ // Defensive bound: never index past the buffer, whatever the length field claims.
+ if (rcvByteCount < 0 || rcvByteCount >= RCV_MSG_MAX_LEN) {
+ memset_s(intrRecvBuffer, RCV_MSG_MAX_LEN, 0, RCV_MSG_MAX_LEN);
+ rcvByteCount = 0;
+ totalLen = 0;
+ return;
+ }
+
if (rcvByteCount == 0) { // frame head
if (byte == 0xAA) {
intrRecvBuffer[rcvByteCount++] = byte;
@@ -1180,12 +1188,17 @@ void __inline FingerprintIsrRecvProcess(uint8_t byte)
}
} else if (rcvByteCount == 1 || rcvByteCount == 2) { //frame len
intrRecvBuffer[rcvByteCount++] = byte;
- if (rcvByteCount == 2) {
+ if (rcvByteCount == 3) { // both length bytes are now present
totalLen = (intrRecvBuffer[2] << 8) + intrRecvBuffer[1] + 3;
+ if (totalLen <= 3 || totalLen > RCV_MSG_MAX_LEN) { // reject junk lengths
+ memset_s(intrRecvBuffer, RCV_MSG_MAX_LEN, 0, RCV_MSG_MAX_LEN);
+ rcvByteCount = 0;
+ totalLen = 0;
+ }
}
} else {
intrRecvBuffer[rcvByteCount++] = byte;
- if (rcvByteCount == totalLen) {
+ if (rcvByteCount >= totalLen) {
if (g_delayCmd == FINGERPRINT_CMD_LOW_POWER && totalLen == 0x27) {
uint8_t passwd = 0;
memcpy_s(g_fpRandomKey, sizeof(g_fpRandomKey), &intrRecvBuffer[15], 16);
Why this scored 71/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.