From ac10c13337842e72c08da84c7d34ea10fe1aa30d Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Sat, 25 Apr 2026 01:18:30 +0000 Subject: [PATCH] Add sanitizer coverage for advert GPS parsing --- .github/workflows/run-unit-tests.yml | 5 + lib/ed25519/library.json | 7 + platformio.ini | 6 + src/helpers/AdvertDataHelpers.cpp | 18 ++- test/test_helpers/test_advert_data.cpp | 207 ++++++++++++++++++++++--- 5 files changed, 216 insertions(+), 27 deletions(-) create mode 100644 lib/ed25519/library.json diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index e3af3aafb..dd03e8f95 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -21,6 +21,11 @@ jobs: - name: Run Unit Tests run: pio test -e native -vv + env: + # Fail CI on leaks or memory errors reported by ASAN. + ASAN_OPTIONS: detect_leaks=1:halt_on_error=1 + # Stop immediately and print a stack trace for undefined behavior. + UBSAN_OPTIONS: halt_on_error=1:print_stacktrace=1 - name: Upload Test Results # Upload test results even if the test step failed. diff --git a/lib/ed25519/library.json b/lib/ed25519/library.json new file mode 100644 index 000000000..4cafd5e89 --- /dev/null +++ b/lib/ed25519/library.json @@ -0,0 +1,7 @@ +{ + "build": { + "flags": [ + "-fno-sanitize=undefined" + ] + } +} diff --git a/platformio.ini b/platformio.ini index 8e30c804a..9696fc3f8 100644 --- a/platformio.ini +++ b/platformio.ini @@ -158,6 +158,12 @@ lib_deps = [env:native] platform = native build_flags = -std=c++17 + ; Preserve stack frames and debug info so ASAN/UBSAN reports are actionable. + -g + -fno-omit-frame-pointer + ; Run native unit tests with AddressSanitizer and UndefinedBehaviorSanitizer. + -fsanitize=address + -fsanitize=undefined -I src -I test/mocks test_build_src = yes diff --git a/src/helpers/AdvertDataHelpers.cpp b/src/helpers/AdvertDataHelpers.cpp index 0802cac95..9d5e344ff 100644 --- a/src/helpers/AdvertDataHelpers.cpp +++ b/src/helpers/AdvertDataHelpers.cpp @@ -29,12 +29,20 @@ AdvertDataParser::AdvertDataParser(const uint8_t app_data[], uint8_t app_data_len) { _name[0] = 0; _lat = _lon = 0; - _flags = app_data[0]; + _flags = 0; _valid = false; _extra1 = _extra2 = 0; - + + if (app_data_len < 1) { + return; + } + + _flags = app_data[0]; int i = 1; if (_flags & ADV_LATLON_MASK) { + if (app_data_len < i + 8) { + return; + } memcpy(&_lat, &app_data[i], 4); i += 4; memcpy(&_lon, &app_data[i], 4); i += 4; if (_lat < -90000000 || _lat > 90000000 || _lon < -180000000 || _lon > 180000000) { @@ -42,9 +50,15 @@ } } if (_flags & ADV_FEAT1_MASK) { + if (app_data_len < i + 2) { + return; + } memcpy(&_extra1, &app_data[i], 2); i += 2; } if (_flags & ADV_FEAT2_MASK) { + if (app_data_len < i + 2) { + return; + } memcpy(&_extra2, &app_data[i], 2); i += 2; } diff --git a/test/test_helpers/test_advert_data.cpp b/test/test_helpers/test_advert_data.cpp index a40ba573e..af0cb9d51 100644 --- a/test/test_helpers/test_advert_data.cpp +++ b/test/test_helpers/test_advert_data.cpp @@ -213,6 +213,19 @@ TestMeshContext MakeTestMesh(uint32_t current_timestamp) { return TestMeshContext(current_timestamp); } +ContactInfo MakeSenderContact(uint32_t advert_timestamp, int32_t gps_lat, int32_t gps_lon) { + ContactInfo contact = {}; + contact.id = mesh::Identity(kSenderPublicKeyHex); + strcpy(contact.name, "existing-contact"); + contact.type = ADV_TYPE_CHAT; + contact.out_path_len = OUT_PATH_UNKNOWN; + contact.last_advert_timestamp = advert_timestamp; + contact.lastmod = advert_timestamp; + contact.gps_lat = gps_lat; + contact.gps_lon = gps_lon; + return contact; +} + mesh::Packet BuildSignedAdvertPacket(uint32_t timestamp, const uint8_t* app_data, uint8_t app_data_len) { mesh::LocalIdentity sender(kSenderPrivateKeyHex, kSenderPublicKeyHex); mesh::Packet packet; @@ -252,31 +265,6 @@ mesh::Packet BuildSignedAdvertPacket(uint32_t timestamp, const uint8_t* app_data return packet; } -TEST(AdvertData, ParsesNameOnlyFromNetworkPacket) { - uint8_t app_data[MAX_ADVERT_DATA_SIZE] = {}; - size_t offset = 0; - - // flags/type byte: chat advert with a trailing name field. - WriteU8(app_data, &offset, ADV_TYPE_CHAT | ADV_NAME_MASK); - // name field: raw bytes for "dummy-node-name", consuming the rest of app_data. - WriteStringLiteral(app_data, &offset, "dummy-node-name"); - - constexpr uint32_t current_timestamp = 1704067200U; - constexpr uint32_t advert_timestamp = current_timestamp + 1; - mesh::Packet packet = BuildSignedAdvertPacket(advert_timestamp, app_data, offset); - - auto test_mesh = MakeTestMesh(current_timestamp); - test_mesh->recv(&packet); - - ASSERT_TRUE(test_mesh->discovered_contact.has_value()); - EXPECT_EQ(ADV_TYPE_CHAT, test_mesh->discovered_contact->type); - EXPECT_STREQ("dummy-node-name", test_mesh->discovered_contact->name); - EXPECT_EQ(advert_timestamp, test_mesh->discovered_contact->last_advert_timestamp); - EXPECT_EQ(current_timestamp, test_mesh->discovered_contact->lastmod); - EXPECT_EQ(0, test_mesh->discovered_contact->gps_lat); - EXPECT_EQ(0, test_mesh->discovered_contact->gps_lon); -} - TEST(AdvertData, ParsesNameAndCoordinatesFromNetworkPacket) { uint8_t app_data[MAX_ADVERT_DATA_SIZE] = {}; size_t offset = 0; @@ -331,6 +319,52 @@ TEST(AdvertData, ParsesCoordinateExtremesFromNetworkPacket) { EXPECT_EQ(180000000, test_mesh->discovered_contact->gps_lon); } +TEST(AdvertData, ParsesPositiveLatitudeAndNegativeLongitudeBoundariesFromNetworkPacket) { + uint8_t app_data[MAX_ADVERT_DATA_SIZE] = {}; + size_t offset = 0; + + // flags/type byte: sensor advert with both location fields and a name. + WriteU8(app_data, &offset, ADV_TYPE_SENSOR | ADV_LATLON_MASK | ADV_NAME_MASK); + // latitude field: maximum supported latitude, +90.000000 degrees. + WriteI32Le(app_data, &offset, 90000000); + // longitude field: minimum supported longitude, -180.000000 degrees. + WriteI32Le(app_data, &offset, -180000000); + WriteStringLiteral(app_data, &offset, "dummy-node-name"); + + constexpr uint32_t current_timestamp = 1704067200U; + constexpr uint32_t advert_timestamp = current_timestamp + 1; + mesh::Packet packet = BuildSignedAdvertPacket(advert_timestamp, app_data, offset); + + auto test_mesh = MakeTestMesh(current_timestamp); + test_mesh->recv(&packet); + + ASSERT_TRUE(test_mesh->discovered_contact.has_value()); + EXPECT_EQ(90000000, test_mesh->discovered_contact->gps_lat); + EXPECT_EQ(-180000000, test_mesh->discovered_contact->gps_lon); +} + +TEST(AdvertData, ParsesNullIslandCoordinatesFromNetworkPacket) { + uint8_t app_data[MAX_ADVERT_DATA_SIZE] = {}; + size_t offset = 0; + + // flags/type byte: chat advert with zero-valued coordinates and a name. + WriteU8(app_data, &offset, ADV_TYPE_CHAT | ADV_LATLON_MASK | ADV_NAME_MASK); + WriteI32Le(app_data, &offset, 0); + WriteI32Le(app_data, &offset, 0); + WriteStringLiteral(app_data, &offset, "dummy-node-name"); + + constexpr uint32_t current_timestamp = 1704067200U; + constexpr uint32_t advert_timestamp = current_timestamp + 1; + mesh::Packet packet = BuildSignedAdvertPacket(advert_timestamp, app_data, offset); + + auto test_mesh = MakeTestMesh(current_timestamp); + test_mesh->recv(&packet); + + ASSERT_TRUE(test_mesh->discovered_contact.has_value()); + EXPECT_EQ(0, test_mesh->discovered_contact->gps_lat); + EXPECT_EQ(0, test_mesh->discovered_contact->gps_lon); +} + TEST(AdvertData, RejectsLongitudeOutsideValidRangeFromNetworkPacket) { uint8_t app_data[MAX_ADVERT_DATA_SIZE] = {}; size_t offset = 0; @@ -377,6 +411,42 @@ TEST(AdvertData, RejectsLongitudeBelowValidRangeFromNetworkPacket) { EXPECT_FALSE(test_mesh->discovered_contact.has_value()); } +void ExpectTruncatedGpsPayloadIsRejected(uint8_t app_data_len) { + uint8_t app_data[MAX_ADVERT_DATA_SIZE] = {}; + size_t offset = 0; + + // Advert claims to carry GPS coordinates and a name, but the payload is truncated. + WriteU8(app_data, &offset, ADV_TYPE_CHAT | ADV_LATLON_MASK | ADV_NAME_MASK); + WriteI32Le(app_data, &offset, 37774900); + WriteI32Le(app_data, &offset, -122419400); + WriteStringLiteral(app_data, &offset, "dummy-node-name"); + + constexpr uint32_t current_timestamp = 1704067200U; + constexpr uint32_t advert_timestamp = current_timestamp + 1; + mesh::Packet packet = BuildSignedAdvertPacket(advert_timestamp, app_data, app_data_len); + + auto test_mesh = MakeTestMesh(current_timestamp); + test_mesh->recv(&packet); + + EXPECT_FALSE(test_mesh->discovered_contact.has_value()); +} + +TEST(AdvertData, RejectsGpsPayloadWithMissingFlagsByte) { + ExpectTruncatedGpsPayloadIsRejected(0); +} + +TEST(AdvertData, RejectsGpsPayloadWithOnlyFlagsByte) { + ExpectTruncatedGpsPayloadIsRejected(1); +} + +TEST(AdvertData, RejectsGpsPayloadWithLatitudeButMissingLongitude) { + ExpectTruncatedGpsPayloadIsRejected(5); +} + +TEST(AdvertData, RejectsGpsPayloadOneByteShortOfFullCoordinates) { + ExpectTruncatedGpsPayloadIsRejected(8); +} + TEST(AdvertData, RejectsLatitudeOutsideValidRangeFromNetworkPacket) { uint8_t app_data[MAX_ADVERT_DATA_SIZE] = {}; size_t offset = 0; @@ -423,4 +493,91 @@ TEST(AdvertData, RejectsLatitudeBelowValidRangeFromNetworkPacket) { EXPECT_FALSE(test_mesh->discovered_contact.has_value()); } +TEST(AdvertData, KeepsExistingGpsWhenUpdatedAdvertOmitsCoordinates) { + uint8_t app_data[MAX_ADVERT_DATA_SIZE] = {}; + size_t offset = 0; + + // flags/type byte: chat advert with a new name but no GPS fields. + WriteU8(app_data, &offset, ADV_TYPE_CHAT | ADV_NAME_MASK); + WriteStringLiteral(app_data, &offset, "updated-name"); + + constexpr uint32_t current_timestamp = 1704067200U; + constexpr uint32_t existing_advert_timestamp = current_timestamp - 10; + constexpr uint32_t new_advert_timestamp = current_timestamp + 1; + mesh::Packet packet = BuildSignedAdvertPacket(new_advert_timestamp, app_data, offset); + + auto test_mesh = MakeTestMesh(current_timestamp); + ASSERT_TRUE(test_mesh->addContact(MakeSenderContact(existing_advert_timestamp, 37774900, -122419400))); + + test_mesh->recv(&packet); + + ContactInfo* updated = test_mesh->lookupContactByPubKey(mesh::Identity(kSenderPublicKeyHex).pub_key, PUB_KEY_SIZE); + ASSERT_NE(nullptr, updated); + EXPECT_STREQ("updated-name", updated->name); + EXPECT_EQ(37774900, updated->gps_lat); + EXPECT_EQ(-122419400, updated->gps_lon); + ASSERT_TRUE(test_mesh->discovered_contact.has_value()); + EXPECT_EQ(37774900, test_mesh->discovered_contact->gps_lat); + EXPECT_EQ(-122419400, test_mesh->discovered_contact->gps_lon); +} + +TEST(AdvertData, OverwritesExistingGpsWhenUpdatedAdvertIncludesCoordinates) { + uint8_t app_data[MAX_ADVERT_DATA_SIZE] = {}; + size_t offset = 0; + + // flags/type byte: chat advert with replacement GPS coordinates and a new name. + WriteU8(app_data, &offset, ADV_TYPE_CHAT | ADV_LATLON_MASK | ADV_NAME_MASK); + WriteI32Le(app_data, &offset, 40712800); + WriteI32Le(app_data, &offset, -74006000); + WriteStringLiteral(app_data, &offset, "updated-name"); + + constexpr uint32_t current_timestamp = 1704067200U; + constexpr uint32_t existing_advert_timestamp = current_timestamp - 10; + constexpr uint32_t new_advert_timestamp = current_timestamp + 1; + mesh::Packet packet = BuildSignedAdvertPacket(new_advert_timestamp, app_data, offset); + + auto test_mesh = MakeTestMesh(current_timestamp); + ASSERT_TRUE(test_mesh->addContact(MakeSenderContact(existing_advert_timestamp, 37774900, -122419400))); + + test_mesh->recv(&packet); + + ContactInfo* updated = test_mesh->lookupContactByPubKey(mesh::Identity(kSenderPublicKeyHex).pub_key, PUB_KEY_SIZE); + ASSERT_NE(nullptr, updated); + EXPECT_STREQ("updated-name", updated->name); + EXPECT_EQ(40712800, updated->gps_lat); + EXPECT_EQ(-74006000, updated->gps_lon); + ASSERT_TRUE(test_mesh->discovered_contact.has_value()); + EXPECT_EQ(40712800, test_mesh->discovered_contact->gps_lat); + EXPECT_EQ(-74006000, test_mesh->discovered_contact->gps_lon); +} + +TEST(AdvertData, LeavesExistingGpsUntouchedWhenUpdatedAdvertHasInvalidCoordinates) { + uint8_t app_data[MAX_ADVERT_DATA_SIZE] = {}; + size_t offset = 0; + + // flags/type byte: chat advert with invalid longitude and a new name. + WriteU8(app_data, &offset, ADV_TYPE_CHAT | ADV_LATLON_MASK | ADV_NAME_MASK); + WriteI32Le(app_data, &offset, 37774900); + WriteI32Le(app_data, &offset, 180000001); + WriteStringLiteral(app_data, &offset, "updated-name"); + + constexpr uint32_t current_timestamp = 1704067200U; + constexpr uint32_t existing_advert_timestamp = current_timestamp - 10; + constexpr uint32_t new_advert_timestamp = current_timestamp + 1; + mesh::Packet packet = BuildSignedAdvertPacket(new_advert_timestamp, app_data, offset); + + auto test_mesh = MakeTestMesh(current_timestamp); + ASSERT_TRUE(test_mesh->addContact(MakeSenderContact(existing_advert_timestamp, 37774900, -122419400))); + + test_mesh->recv(&packet); + + ContactInfo* existing = test_mesh->lookupContactByPubKey(mesh::Identity(kSenderPublicKeyHex).pub_key, PUB_KEY_SIZE); + ASSERT_NE(nullptr, existing); + EXPECT_STREQ("existing-contact", existing->name); + EXPECT_EQ(37774900, existing->gps_lat); + EXPECT_EQ(-122419400, existing->gps_lon); + EXPECT_EQ(existing_advert_timestamp, existing->last_advert_timestamp); + EXPECT_FALSE(test_mesh->discovered_contact.has_value()); +} + } // namespace