Harden against crafted-packet crashes + adversarial fuzzing (#10862)
Audit and fuzzing of the RF-packet decode -> dispatch -> display/phone paths for the "crash a node or phone with a crafted packet" surface, beyond the XEdDSA authenticity work. Crash fixes (reproduced under AddressSanitizer / UBSan): - GeoCoord::latLongToUTM/latLongToMGRS read fixed letter tables out of bounds on extreme latitude_i/longitude_i from a received Position, and narrowed out-of-range easting/northing doubles to unsigned (float-cast-overflow UB). Clamp the UTM zone, the easting/northing narrowing, and the band/col/row indices. Regression: test_geocoord_extreme_coords_no_oob. - EnvironmentTelemetry/AirQualityTelemetry render attacker floats via String(float), which on nRF52/RP2040/STM32/portduino formats into a fixed char[33] (dtostrf) and overflows near FLT_MAX. Clamp the rendered metrics via UnitConversions::displaySafeFloat (finite + magnitude <= 1e9), unit-tested in test_type_conversions. Defense-in-depth + robustness: - TraceRouteModule::printRoute: fix an snr_back[-1] OOB read (wrong count in the guard) and stop formatting the INT8_MIN "unknown SNR" sentinel as a dB value. - WaypointModule/NodeDB: sanitize untrusted strings before the OLED renderer and the phone-facing ClientNotification (belt-and-suspenders vs PB_VALIDATE_UTF8). - MeshService::sendToPhone: withhold NODEINFO/WAYPOINT packets whose nested string won't cleanly decode, protecting strict phone protobuf decoders without affecting mesh relay. Tests: new test_fuzz_decode (protobuf decode + UTF-8 sanitizer fuzz) and test_fuzz_packets (perhapsDecode / module-handler / traceroute / phone-gate fuzz), all under AddressSanitizer; native-suite-count 25 -> 27. Full suite 515/515 green.
This commit is contained in:
+40
-15
@@ -1,4 +1,14 @@
|
||||
#include "GeoCoord.h"
|
||||
#include <cmath>
|
||||
|
||||
// Narrow a UTM meter value to its unsigned field, clamping non-finite/negative/oversized inputs: an
|
||||
// extreme (crafted) lat/lon can drive these out of range, and an overflowing double->unsigned cast is UB.
|
||||
static uint32_t clampMeters(double m)
|
||||
{
|
||||
if (!std::isfinite(m) || m < 0.0)
|
||||
return 0;
|
||||
return m > 4.0e9 ? 4000000000u : (uint32_t)m;
|
||||
}
|
||||
|
||||
GeoCoord::GeoCoord()
|
||||
{
|
||||
@@ -124,8 +134,13 @@ void GeoCoord::latLongToUTM(const double lat, const double lon, UTM &utm)
|
||||
{
|
||||
|
||||
const std::string latBands = "CDEFGHJKLMNPQRSTUVWXX";
|
||||
utm.zone = int((lon + 180) / 6 + 1);
|
||||
utm.band = latBands[int(lat / 8 + 10)];
|
||||
// A received Position carries raw int32 latitude_i/longitude_i with no range validation, so lat/lon
|
||||
// here can be far outside real geographic bounds. Clamp the derived UTM zone (valid 1..60) and the
|
||||
// latitude-band index so the lookups below cannot read out of bounds (GeoCoord.cpp:128 stack over/
|
||||
// under-read on e.g. latitude_i = INT32_MAX/INT32_MIN).
|
||||
utm.zone = std::min(std::max(int((lon + 180) / 6 + 1), 1), 60);
|
||||
int bandIdx = std::min(std::max(int(lat / 8 + 10), 0), int(latBands.length()) - 1);
|
||||
utm.band = latBands[bandIdx];
|
||||
double a = 6378137; // WGS84 - equatorial radius
|
||||
double k0 = 0.9996; // UTM point scale on the central meridian
|
||||
double eccSquared = 0.00669438; // eccentricity squared
|
||||
@@ -160,17 +175,22 @@ void GeoCoord::latLongToUTM(const double lat, const double lon, UTM &utm)
|
||||
sin(2 * latRad) +
|
||||
(15 * eccSquared * eccSquared / 256 + 45 * eccSquared * eccSquared * eccSquared / 1024) * sin(4 * latRad) -
|
||||
(35 * eccSquared * eccSquared * eccSquared / 3072) * sin(6 * latRad));
|
||||
utm.easting = (double)(k0 * N *
|
||||
(A + (1 - T + C) * pow(A, 3) / 6 +
|
||||
(5 - 18 * T + T * T + 72 * C - 58 * eccPrimeSquared) * A * A * A * A * A / 120) +
|
||||
500000.0);
|
||||
utm.northing =
|
||||
(double)(k0 * (M + N * tan(latRad) *
|
||||
(A * A / 2 + (5 - T + 9 * C + 4 * C * C) * A * A * A * A / 24 +
|
||||
(61 - 58 * T + T * T + 600 * C - 330 * eccPrimeSquared) * A * A * A * A * A * A / 720)));
|
||||
double eastingMeters =
|
||||
k0 * N *
|
||||
(A + (1 - T + C) * pow(A, 3) / 6 + (5 - 18 * T + T * T + 72 * C - 58 * eccPrimeSquared) * A * A * A * A * A / 120) +
|
||||
500000.0;
|
||||
double northingMeters =
|
||||
k0 * (M + N * tan(latRad) *
|
||||
(A * A / 2 + (5 - T + 9 * C + 4 * C * C) * A * A * A * A / 24 +
|
||||
(61 - 58 * T + T * T + 600 * C - 330 * eccPrimeSquared) * A * A * A * A * A * A / 720));
|
||||
|
||||
if (lat < 0)
|
||||
utm.northing += 10000000.0; // 10000000 meter offset for southern hemisphere
|
||||
northingMeters += 10000000.0; // 10000000 meter offset for southern hemisphere
|
||||
|
||||
// Clamp before narrowing to the unsigned UTM fields (see clampMeters): extreme lat/lon can drive
|
||||
// these negative or past UINT32, and the raw double->unsigned cast would be UB.
|
||||
utm.easting = clampMeters(eastingMeters);
|
||||
utm.northing = clampMeters(northingMeters);
|
||||
}
|
||||
|
||||
// Converts lat long coordinates to an MGRS.
|
||||
@@ -182,10 +202,15 @@ void GeoCoord::latLongToMGRS(const double lat, const double lon, MGRS &mgrs)
|
||||
latLongToUTM(lat, lon, utm);
|
||||
mgrs.zone = utm.zone;
|
||||
mgrs.band = utm.band;
|
||||
double col = floor(utm.easting / 100000);
|
||||
mgrs.east100k = e100kLetters[(mgrs.zone - 1) % 3][col - 1];
|
||||
double row = (int32_t)floor(utm.northing / 100000.0) % 20;
|
||||
mgrs.north100k = n100kLetters[(mgrs.zone - 1) % 2][row];
|
||||
// utm.zone is clamped to 1..60 above, but guard every index defensively: the column/row derived
|
||||
// from easting/northing can fall outside the 100km-grid letter tables when lat/lon are extreme.
|
||||
int zoneIdx3 = ((mgrs.zone - 1) % 3 + 3) % 3;
|
||||
int zoneIdx2 = ((mgrs.zone - 1) % 2 + 2) % 2;
|
||||
int colIdx = std::min(std::max(int(floor(utm.easting / 100000)) - 1, 0), int(e100kLetters[zoneIdx3].length()) - 1);
|
||||
mgrs.east100k = e100kLetters[zoneIdx3][colIdx];
|
||||
int rowIdx = ((int32_t)floor(utm.northing / 100000.0) % 20 + 20) % 20;
|
||||
rowIdx = std::min(std::max(rowIdx, 0), int(n100kLetters[zoneIdx2].length()) - 1);
|
||||
mgrs.north100k = n100kLetters[zoneIdx2][rowIdx];
|
||||
mgrs.easting = (int32_t)utm.easting % 100000;
|
||||
mgrs.northing = (int32_t)utm.northing % 100000;
|
||||
}
|
||||
|
||||
@@ -302,10 +302,39 @@ bool MeshService::trySendPosition(NodeNum dest, bool wantReplies)
|
||||
return false;
|
||||
}
|
||||
|
||||
// Re-decode nested string-bearing payloads before local phone delivery so PB_VALIDATE_UTF8 rejects
|
||||
// malformed NodeInfo/Waypoint data a strict phone decoder could crash on. Mesh relay is unaffected.
|
||||
bool MeshService::phonePayloadIsDecodable(const meshtastic_Data &d)
|
||||
{
|
||||
// User/Waypoint are all-static nanopb messages (no PB_ENABLE_MALLOC/callback fields), so the
|
||||
// decoded scratch owns no heap and needs no pb_release.
|
||||
switch (d.portnum) {
|
||||
case meshtastic_PortNum_NODEINFO_APP: {
|
||||
meshtastic_User u = meshtastic_User_init_zero;
|
||||
return pb_decode_from_bytes(d.payload.bytes, d.payload.size, &meshtastic_User_msg, &u);
|
||||
}
|
||||
case meshtastic_PortNum_WAYPOINT_APP: {
|
||||
meshtastic_Waypoint w = meshtastic_Waypoint_init_zero;
|
||||
return pb_decode_from_bytes(d.payload.bytes, d.payload.size, &meshtastic_Waypoint_msg, &w);
|
||||
}
|
||||
default:
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
void MeshService::sendToPhone(meshtastic_MeshPacket *p)
|
||||
{
|
||||
perhapsDecode(p);
|
||||
|
||||
// Withhold decoded nested payloads a strict phone decoder would reject; still-encrypted packets
|
||||
// pass through (the phone may hold the key).
|
||||
if (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag && !phonePayloadIsDecodable(p->decoded)) {
|
||||
LOG_WARN("Dropping undecodable portnum=%d payload from phone delivery (from=0x%08x)", p->decoded.portnum, p->from);
|
||||
releaseToPool(p);
|
||||
fromNum++; // notify observers so the phone can resync
|
||||
return;
|
||||
}
|
||||
|
||||
#ifdef ARCH_ESP32
|
||||
#if !MESHTASTIC_EXCLUDE_STOREFORWARD
|
||||
if (moduleConfig.store_forward.enabled && storeForwardModule->isServer() &&
|
||||
|
||||
@@ -100,6 +100,11 @@ class MeshService
|
||||
p->decoded.portnum == meshtastic_PortNum_DETECTION_SENSOR_APP ||
|
||||
p->decoded.portnum == meshtastic_PortNum_ALERT_APP;
|
||||
}
|
||||
|
||||
/// Returns false when a decoded NodeInfo/Waypoint payload fails nested protobuf decode (invalid
|
||||
/// UTF-8 under PB_VALIDATE_UTF8, etc.); other portnums pass through. Callers gate on the variant.
|
||||
static bool phonePayloadIsDecodable(const meshtastic_Data &decoded);
|
||||
|
||||
/// Called when some new packets have arrived from one of the radios
|
||||
Observable<uint32_t> fromNumChanged;
|
||||
|
||||
|
||||
+9
-3
@@ -1024,7 +1024,7 @@ void NodeDB::installDefaultConfig(bool preserveKey = false)
|
||||
strncpy(config.network.ntp_server, "meshtastic.pool.ntp.org", 32);
|
||||
|
||||
#if (defined(T_DECK) || defined(T_WATCH_S3) || defined(UNPHONE) || defined(PICOMPUTER_S3) || defined(SENSECAP_INDICATOR) || \
|
||||
defined(ELECROW_PANEL) || defined(HELTEC_V4_TFT) || defined(HELTEC_V4_R8_TFT) || defined(RAK_WISMESH_TAP_V2)) && \
|
||||
defined(ELECROW_PANEL) || defined(HELTEC_V4_TFT) || defined(HELTEC_V4_R8_TFT) || defined(RAK_WISMESH_TAP_V2)) && \
|
||||
HAS_TFT
|
||||
// switch BT off by default; use TFT programming mode or hotkey to enable
|
||||
config.bluetooth.enabled = false;
|
||||
@@ -3264,14 +3264,20 @@ bool NodeDB::updateUser(uint32_t nodeId, meshtastic_User &p, uint8_t channelInde
|
||||
if (owner.public_key.size == 32 && memcmp(p.public_key.bytes, owner.public_key.bytes, 32) == 0) {
|
||||
if (!duplicateWarned) {
|
||||
duplicateWarned = true;
|
||||
// Sanitize before embedding long_name in the phone-facing ClientNotification string
|
||||
// (defense-in-depth vs PB_VALIDATE_UTF8).
|
||||
char safeName[sizeof(p.long_name)];
|
||||
strncpy(safeName, p.long_name, sizeof(safeName));
|
||||
safeName[sizeof(safeName) - 1] = '\0';
|
||||
sanitizeUtf8(safeName, sizeof(safeName));
|
||||
char warning[] =
|
||||
"Remote device %s has advertised your public key. This may indicate a compromised key. You may need "
|
||||
"to regenerate your public keys.";
|
||||
LOG_WARN(warning, p.long_name);
|
||||
LOG_WARN(warning, safeName);
|
||||
meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed();
|
||||
cn->level = meshtastic_LogRecord_Level_WARNING;
|
||||
cn->time = getValidTime(RTCQualityFromNet);
|
||||
snprintf(cn->message, sizeof(cn->message), warning, p.long_name);
|
||||
snprintf(cn->message, sizeof(cn->message), warning, safeName);
|
||||
service->sendClientNotification(cn);
|
||||
}
|
||||
return false;
|
||||
|
||||
@@ -210,6 +210,11 @@ void AirQualityTelemetryModule::drawFrame(OLEDDisplay *display, OLEDDisplayUiSta
|
||||
return;
|
||||
}
|
||||
|
||||
// Same String(float) stack-overflow guard as EnvironmentTelemetry: form_formaldehyde is the only
|
||||
// float rendered here, and its raw bytes are unvalidated on decode.
|
||||
telemetry.variant.air_quality_metrics.form_formaldehyde =
|
||||
UnitConversions::displaySafeFloat(telemetry.variant.air_quality_metrics.form_formaldehyde);
|
||||
|
||||
const auto &m = telemetry.variant.air_quality_metrics;
|
||||
|
||||
// Check if any telemetry field has valid data
|
||||
|
||||
@@ -375,6 +375,22 @@ void EnvironmentTelemetryModule::drawFrame(OLEDDisplay *display, OLEDDisplayUiSt
|
||||
return;
|
||||
}
|
||||
|
||||
// Bound the float metrics before String(float) renders them (see UnitConversions::displaySafeFloat);
|
||||
// the stored packet is always the environment variant.
|
||||
{
|
||||
auto &e = telemetry.variant.environment_metrics;
|
||||
e.temperature = UnitConversions::displaySafeFloat(e.temperature);
|
||||
e.relative_humidity = UnitConversions::displaySafeFloat(e.relative_humidity);
|
||||
e.barometric_pressure = UnitConversions::displaySafeFloat(e.barometric_pressure);
|
||||
e.voltage = UnitConversions::displaySafeFloat(e.voltage);
|
||||
e.current = UnitConversions::displaySafeFloat(e.current);
|
||||
e.lux = UnitConversions::displaySafeFloat(e.lux);
|
||||
e.white_lux = UnitConversions::displaySafeFloat(e.white_lux);
|
||||
e.weight = UnitConversions::displaySafeFloat(e.weight);
|
||||
e.distance = UnitConversions::displaySafeFloat(e.distance);
|
||||
e.radiation = UnitConversions::displaySafeFloat(e.radiation);
|
||||
}
|
||||
|
||||
const auto &m = telemetry.variant.environment_metrics;
|
||||
|
||||
// Check if any telemetry field has valid data
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
#pragma once
|
||||
|
||||
#include <cmath>
|
||||
|
||||
class UnitConversions
|
||||
{
|
||||
public:
|
||||
@@ -7,4 +9,13 @@ class UnitConversions
|
||||
static float MetersPerSecondToKnots(float metersPerSecond);
|
||||
static float MetersPerSecondToMilesPerHour(float metersPerSecond);
|
||||
static float HectoPascalToInchesOfMercury(float hectoPascal);
|
||||
|
||||
// Bound a float before Arduino String(float) renders it: its fixed char[33] + dtostrf overflow
|
||||
// near FLT_MAX (stack smash). Clamp to +/-1e9 (<=10 digits) and drop non-finite values.
|
||||
static inline float displaySafeFloat(float v)
|
||||
{
|
||||
if (!std::isfinite(v))
|
||||
return 0.0f;
|
||||
return v < -1e9f ? -1e9f : (v > 1e9f ? 1e9f : v);
|
||||
}
|
||||
};
|
||||
|
||||
@@ -452,7 +452,7 @@ void TraceRouteModule::printRoute(meshtastic_RouteDiscovery *r, uint32_t origin,
|
||||
// If there's a route back (or we are the destination as then the route is complete), print it
|
||||
if (r->route_back_count > 0 || origin == nodeDB->getNodeNum()) {
|
||||
route += "\n";
|
||||
if (r->snr_towards_count > 0 && origin == nodeDB->getNodeNum())
|
||||
if (origin == nodeDB->getNodeNum() && r->snr_back_count > 0 && r->snr_back[r->snr_back_count - 1] != INT8_MIN)
|
||||
route += vformat("(%.2fdB) 0x%x <-- ", (float)r->snr_back[r->snr_back_count - 1] / 4, origin);
|
||||
else
|
||||
route += "...";
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
#include "configuration.h"
|
||||
#include "graphics/SharedUIDisplay.h"
|
||||
#include "graphics/draw/CompassRenderer.h"
|
||||
#include "meshUtils.h"
|
||||
|
||||
#if HAS_SCREEN
|
||||
#include "gps/RTC.h"
|
||||
@@ -92,6 +93,10 @@ void WaypointModule::drawFrame(OLEDDisplay *display, OLEDDisplayUiState *state,
|
||||
return;
|
||||
}
|
||||
|
||||
// Sanitize before these reach the OLED renderer (defense-in-depth vs PB_VALIDATE_UTF8).
|
||||
sanitizeUtf8(wp.name, sizeof(wp.name));
|
||||
sanitizeUtf8(wp.description, sizeof(wp.description));
|
||||
|
||||
// Get timestamp info. Will pass as a field to drawColumns
|
||||
char lastStr[20];
|
||||
getTimeAgoStr(sinceReceived(&mp), lastStr, sizeof(lastStr));
|
||||
|
||||
Reference in New Issue
Block a user