fix: defensive buffer-write hardening (sprintf, mt_sprintf, NMEA checksum) - #11236
fix: defensive buffer-write hardening (sprintf, mt_sprintf, NMEA checksum)#11236ndoo wants to merge 1 commit into
Conversation
…ksum)
None of these are demonstrated as exploitable today, but are latent
landmines worth closing while auditing this area:
- DropzoneModule.cpp: 4 raw sprintf() calls into fixed-size buffers,
safe today only because of current nanopb field-size invariants.
Switched to snprintf().
- STM32_LittleFS.cpp: dbg_strerr_lfs's sprintf("%ld", err) into a
10-byte buffer - INT32_MIN needs 12. Switched to snprintf() into a
13-byte buffer.
- RedirectablePrint.cpp: mt_sprintf() had a dead strcpy() (immediately
overwritten by the vsnprintf() right after) that underflows on an
empty format string, writing 1 byte past a 0-size allocation. Removed
it - vsnprintf() alone already produces the correct result.
- NMEAWPL.cpp: printWPL()/printGGA() use snprintf()'s return value
(which can exceed bufsz on truncation) directly as an offset/size for
a second snprintf() call and a checksum loop, both of which need it
clamped first or bufsz-len underflows into a huge size_t. Safe today
only because of current field-size bounds; clamped defensively.
Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Andrew Yong <me@ndoo.sg>
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
snprintf returns the length it would have written, so a truncated NMEA sentence made buf + len point past the buffer and bufsz - len underflow into a huge size for the checksum append. Clamp after each write. Also pulls in the rest of #11236: the two remaining Dropzone sprintf calls, the dead strcpy in mt_sprintf that wrote one byte past a zero-size allocation for an empty format, and the 10-byte errcode buffer that INT32_MIN overflows. Co-Authored-By: Andrew Yong <me@ndoo.sg>
|
Thanks for this @ndoo, and sorry to close it out from under you. #11293 started as the NMEA checksum offset fix from #11242, which lands on the same three lines in You are credited as co-author on the commit. The two Your analysis was right on every point, including the empty-format case where |
No worries. Tbh most of the groundwork was Claude analysis. Nonetheless just happy to see the fix ship. Cheers |
snprintf returns the length it would have written, so a truncated NMEA sentence made buf + len point past the buffer and bufsz - len underflow into a huge size for the checksum append. Clamp after each write. Also pulls in the rest of #11236: the two remaining Dropzone sprintf calls, the dead strcpy in mt_sprintf that wrote one byte past a zero-size allocation for an empty format, and the 10-byte errcode buffer that INT32_MIN overflows. Co-Authored-By: Andrew Yong <me@ndoo.sg>
snprintf returns the length it would have written, so a truncated NMEA sentence made buf + len point past the buffer and bufsz - len underflow into a huge size for the checksum append. Clamp after each write. Also pulls in the rest of #11236: the two remaining Dropzone sprintf calls, the dead strcpy in mt_sprintf that wrote one byte past a zero-size allocation for an empty format, and the 10-byte errcode buffer that INT32_MIN overflows. Co-Authored-By: Andrew Yong <me@ndoo.sg>
snprintf returns the length it would have written, so a truncated NMEA sentence made buf + len point past the buffer and bufsz - len underflow into a huge size for the checksum append. Clamp after each write. Also pulls in the rest of #11236: the two remaining Dropzone sprintf calls, the dead strcpy in mt_sprintf that wrote one byte past a zero-size allocation for an empty format, and the 10-byte errcode buffer that INT32_MIN overflows. Co-Authored-By: Andrew Yong <me@ndoo.sg>
…eshtastic#11293) * Checksum NMEA sentences from the $ delimiter The PositionLite printWPL() format begins with a CRLF, so the fixed start offset of 1 folded the newline and the $ into the checksum and every sentence went out with a wrong value. Locate the $ instead and stop at the terminator or a \*. * Clamp truncated writes and harden the remaining fixed buffers snprintf returns the length it would have written, so a truncated NMEA sentence made buf + len point past the buffer and bufsz - len underflow into a huge size for the checksum append. Clamp after each write. Also pulls in the rest of meshtastic#11236: the two remaining Dropzone sprintf calls, the dead strcpy in mt_sprintf that wrote one byte past a zero-size allocation for an empty format, and the 10-byte errcode buffer that INT32_MIN overflows. Co-Authored-By: Andrew Yong <me@ndoo.sg> * Bail out on a zero-sized buffer and cast err for %ld snprintf writes nothing at all when bufsz is 0, not even a terminator, so the checksum helper would run strchr over whatever the buffer already held. Return before touching it. int32_t is not long on every target, so cast before formatting with %ld. Co-Authored-By: Andrew Yong <me@ndoo.sg> * Add NMEA sentence regression tests Covers checksum computation from the $ delimiter for both printWPL overloads and printGGA, zero-sized buffers, and truncated buffers down to one byte. Co-Authored-By: Andrew Yong <me@ndoo.sg> * Tighten checksum parsing and pin the WPL fixture checksum Require exactly two hex digits followed by the sentence terminator, and assert both WPL overloads against a known checksum instead of comparing them to each other. * Bump native suite count to 43 --------- Co-authored-by: Andrew Yong <me@ndoo.sg>
2 participants
Problem
A full-tree sweep for unsafe buffer writes (as part of an ongoing STM32WL memory-safety audit) found the codebase in good shape overall — this is the small remaining handful of loose ends. None are demonstrated as exploitable today; all are latent landmines worth closing while in this area.
src/modules/DropzoneModule.cpp: 4 rawsprintf()calls into fixed-size buffers, safe today only because of current nanopb field-size invariants (no defense-in-depth if those change).src/platform/stm32wl/STM32_LittleFS.cpp:dbg_strerr_lfs'ssprintf(errcode, "%ld", err)into a 10-byte buffer —INT32_MINneeds 12 bytes (debug-build-only code path).src/RedirectablePrint.cpp:mt_sprintf()had astrcpy()immediately overwritten by thevsnprintf()right after it — pure dead code, and on an empty format string it underflows, writing 1 byte past a 0-size allocation.src/gps/NMEAWPL.cpp:printWPL()/printGGA()usesnprintf()'s return value (which can exceedbufszon truncation) directly as an offset/size for a secondsnprintf()call and a checksum loop — both need the length clamped first, orbufsz-lenunderflows into a hugesize_t. Safe today only because of current field-size bounds.Fix
sprintf()calls forsnprintf().strcpy()inmt_sprintf()—vsnprintf()alone already produces the correct result.lentobufsz(or 0) after eachsnprintf()call inNMEAWPL.cppbefore using it as an offset or in the checksum loop.Test plan
pio run -e wio-e5/pio run -e rak3172— build clean, confirms all four touched files actually compile on the memory-constrained target these were audited for.🤝 Attestations