From b03e8e02e3d41e55beb6087e9ddcd7a7f44e3c77 Mon Sep 17 00:00:00 2001 From: Andrew Yong Date: Mon, 27 Jul 2026 00:23:32 +0800 Subject: [PATCH] fix: defensive buffer-write hardening (sprintf, mt_sprintf, NMEA checksum) 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 Signed-off-by: Andrew Yong --- src/RedirectablePrint.cpp | 1 - src/gps/NMEAWPL.cpp | 24 ++++++++++++++++++++++++ src/modules/DropzoneModule.cpp | 10 +++++----- src/platform/stm32wl/STM32_LittleFS.cpp | 4 ++-- 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/RedirectablePrint.cpp b/src/RedirectablePrint.cpp index e53806ac77a..f6641fdbdce 100644 --- a/src/RedirectablePrint.cpp +++ b/src/RedirectablePrint.cpp @@ -386,7 +386,6 @@ std::string RedirectablePrint::mt_sprintf(const std::string fmt_str, ...) va_list ap; while (1) { formatted.reset(new char[n]); /* Wrap the plain char array into the unique_ptr */ - strcpy(&formatted[0], fmt_str.c_str()); va_start(ap, fmt_str); int final_n = vsnprintf(&formatted[0], n, fmt_str.c_str(), ap); va_end(ap); diff --git a/src/gps/NMEAWPL.cpp b/src/gps/NMEAWPL.cpp index 5259427e635..b1ce50e69a0 100644 --- a/src/gps/NMEAWPL.cpp +++ b/src/gps/NMEAWPL.cpp @@ -27,11 +27,19 @@ uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_PositionLite &pos, c (abs(geoCoord.getLatitude()) - geoCoord.getDMSLatDeg() * 1e+7) * 6e-6, geoCoord.getDMSLatCP(), geoCoord.getDMSLonDeg(), (abs(geoCoord.getLongitude()) - geoCoord.getDMSLonDeg() * 1e+7) * 6e-6, geoCoord.getDMSLonCP(), name); + // snprintf() returns the length that *would* have been written, which can exceed bufsz if + // truncated - clamp before using it as an index/size, or the checksum loop reads out of + // bounds and bufsz-len underflows into a huge size for the next snprintf(). + if (len >= bufsz) + len = bufsz > 0 ? bufsz - 1 : 0; + uint32_t chk = 0; for (uint32_t i = 1; i < len; i++) { chk ^= buf[i]; } len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk); + if (len >= bufsz) + len = bufsz > 0 ? bufsz - 1 : 0; return len; } @@ -43,11 +51,19 @@ uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_Position &pos, const (abs(geoCoord.getLatitude()) - geoCoord.getDMSLatDeg() * 1e+7) * 6e-6, geoCoord.getDMSLatCP(), geoCoord.getDMSLonDeg(), (abs(geoCoord.getLongitude()) - geoCoord.getDMSLonDeg() * 1e+7) * 6e-6, geoCoord.getDMSLonCP(), name); + // snprintf() returns the length that *would* have been written, which can exceed bufsz if + // truncated - clamp before using it as an index/size, or the checksum loop reads out of + // bounds and bufsz-len underflows into a huge size for the next snprintf(). + if (len >= bufsz) + len = bufsz > 0 ? bufsz - 1 : 0; + uint32_t chk = 0; for (uint32_t i = 1; i < len; i++) { chk ^= buf[i]; } len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk); + if (len >= bufsz) + len = bufsz > 0 ? bufsz - 1 : 0; return len; } /* ------------------------------------------- @@ -91,11 +107,19 @@ uint32_t printGGA(char *buf, size_t bufsz, const meshtastic_Position &pos) (abs(geoCoord.getLongitude()) - geoCoord.getDMSLonDeg() * 1e+7) * 6e-6, geoCoord.getDMSLonCP(), pos.fix_quality, pos.sats_in_view, pos.HDOP, geoCoord.getAltitude(), 'M', pos.altitude_geoidal_separation, 'M', 0, 0); + // snprintf() returns the length that *would* have been written, which can exceed bufsz if + // truncated - clamp before using it as an index/size, or the checksum loop reads out of + // bounds and bufsz-len underflows into a huge size for the next snprintf(). + if (len >= bufsz) + len = bufsz > 0 ? bufsz - 1 : 0; + uint32_t chk = 0; for (uint32_t i = 1; i < len; i++) { chk ^= buf[i]; } len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk); + if (len >= bufsz) + len = bufsz > 0 ? bufsz - 1 : 0; return len; } diff --git a/src/modules/DropzoneModule.cpp b/src/modules/DropzoneModule.cpp index 2340f31f260..5f06268e51b 100644 --- a/src/modules/DropzoneModule.cpp +++ b/src/modules/DropzoneModule.cpp @@ -32,13 +32,13 @@ ProcessMessage DropzoneModule::handleReceived(const meshtastic_MeshPacket &mp) auto &p = mp.decoded; char matchCompare[54]; auto incomingMessage = reinterpret_cast(p.payload.bytes); - sprintf(matchCompare, "%s conditions", owner.short_name); + snprintf(matchCompare, sizeof(matchCompare), "%s conditions", owner.short_name); if (strncasecmp(incomingMessage, matchCompare, strlen(matchCompare)) == 0) { LOG_DEBUG("Received dropzone conditions request"); startSendConditions = millis(); } - sprintf(matchCompare, "%s conditions", owner.long_name); + snprintf(matchCompare, sizeof(matchCompare), "%s conditions", owner.long_name); if (strncasecmp(incomingMessage, matchCompare, strlen(matchCompare)) == 0) { LOG_DEBUG("Received dropzone conditions request"); startSendConditions = millis(); @@ -79,11 +79,11 @@ meshtastic_MeshPacket *DropzoneModule::sendConditions() auto windDirection = telemetry.variant.environment_metrics.wind_direction; auto temp = telemetry.variant.environment_metrics.temperature; auto baro = UnitConversions::HectoPascalToInchesOfMercury(telemetry.variant.environment_metrics.barometric_pressure); - sprintf(replyStr, "%s @ %02d:%02d:%02dz\nWind %.2f kts @ %d°\nBaro %.2f inHg %.2f°C", dropzoneStatus, hour, min, sec, - windSpeed, windDirection, baro, temp); + snprintf(replyStr, sizeof(replyStr), "%s @ %02d:%02d:%02dz\nWind %.2f kts @ %d°\nBaro %.2f inHg %.2f°C", dropzoneStatus, + hour, min, sec, windSpeed, windDirection, baro, temp); } else { LOG_ERROR("No sensor found"); - sprintf(replyStr, "%s @ %02d:%02d:%02d\nNo sensor found", dropzoneStatus, hour, min, sec); + snprintf(replyStr, sizeof(replyStr), "%s @ %02d:%02d:%02d\nNo sensor found", dropzoneStatus, hour, min, sec); } LOG_DEBUG("Conditions reply: %s", replyStr); reply->decoded.payload.size = strlen(replyStr); // You must specify how many bytes are in the reply diff --git a/src/platform/stm32wl/STM32_LittleFS.cpp b/src/platform/stm32wl/STM32_LittleFS.cpp index 97e79e61e7e..9c7d7fc4a6b 100644 --- a/src/platform/stm32wl/STM32_LittleFS.cpp +++ b/src/platform/stm32wl/STM32_LittleFS.cpp @@ -272,8 +272,8 @@ const char *dbg_strerr_lfs(int32_t err) return "LFS_ERR_NOMEM"; default: - static char errcode[10]; - sprintf(errcode, "%ld", err); + static char errcode[13]; // "-2147483648\0" (INT32_MIN) is the longest possible value + snprintf(errcode, sizeof(errcode), "%ld", err); return errcode; }