From bb3d6d5326518b36598ff0a0b8f4b334c953e0fe Mon Sep 17 00:00:00 2001 From: Nick <79813408+niccellular@users.noreply.github.com> Date: Sun, 8 Feb 2026 12:20:15 -0500 Subject: [PATCH] Fix embedded null byte truncation in ATAK strings (#9570) --- src/meshUtils.cpp | 11 ++ src/meshUtils.h | 3 + src/modules/AtakPluginModule.cpp | 69 +++++----- test/TestUtil.cpp | 16 +++ test/TestUtil.h | 5 +- test/test_atak/test_main.cpp | 216 +++++++++++++++++++++++++++++++ 6 files changed, 289 insertions(+), 31 deletions(-) create mode 100644 test/test_atak/test_main.cpp diff --git a/src/meshUtils.cpp b/src/meshUtils.cpp index ea2ba641b..1a4497101 100644 --- a/src/meshUtils.cpp +++ b/src/meshUtils.cpp @@ -106,4 +106,15 @@ const std::string vformat(const char *const zcFormat, ...) std::vsnprintf(zc.data(), zc.size(), zcFormat, vaArgs); va_end(vaArgs); return std::string(zc.data(), iLen); +} + +size_t pb_string_length(const char *str, size_t max_len) +{ + size_t len = 0; + for (size_t i = 0; i < max_len; i++) { + if (str[i] != '\0') { + len = i + 1; + } + } + return len; } \ No newline at end of file diff --git a/src/meshUtils.h b/src/meshUtils.h index 9fcf6f8a8..67446f91f 100644 --- a/src/meshUtils.h +++ b/src/meshUtils.h @@ -35,4 +35,7 @@ bool isOneOf(int item, int count, ...); const std::string vformat(const char *const zcFormat, ...); +// Get actual string length for nanopb char array fields. +size_t pb_string_length(const char *str, size_t max_len); + #define IS_ONE_OF(item, ...) isOneOf(item, sizeof((int[]){__VA_ARGS__}) / sizeof(int), __VA_ARGS__) diff --git a/src/modules/AtakPluginModule.cpp b/src/modules/AtakPluginModule.cpp index a51ef54c3..bddb6276b 100644 --- a/src/modules/AtakPluginModule.cpp +++ b/src/modules/AtakPluginModule.cpp @@ -6,6 +6,7 @@ #include "configuration.h" #include "main.h" #include "mesh/compression/unishox2.h" +#include "meshUtils.h" #include "meshtastic/atak.pb.h" AtakPluginModule *atakPluginModule; @@ -70,16 +71,17 @@ void AtakPluginModule::alterReceivedProtobuf(meshtastic_MeshPacket &mp, meshtast auto compressed = cloneTAKPacketData(t); compressed.is_compressed = true; if (t->has_contact) { - auto length = unishox2_compress_lines(t->contact.callsign, strlen(t->contact.callsign), compressed.contact.callsign, - sizeof(compressed.contact.callsign) - 1, USX_PSET_DFLT, NULL); + auto length = unishox2_compress_lines( + t->contact.callsign, pb_string_length(t->contact.callsign, sizeof(t->contact.callsign)), + compressed.contact.callsign, sizeof(compressed.contact.callsign) - 1, USX_PSET_DFLT, NULL); if (length < 0) { LOG_WARN("Compress overflow contact.callsign. Revert to uncompressed packet"); return; } LOG_DEBUG("Compressed callsign: %d bytes", length); - length = unishox2_compress_lines(t->contact.device_callsign, strlen(t->contact.device_callsign), - compressed.contact.device_callsign, sizeof(compressed.contact.device_callsign) - 1, - USX_PSET_DFLT, NULL); + length = unishox2_compress_lines( + t->contact.device_callsign, pb_string_length(t->contact.device_callsign, sizeof(t->contact.device_callsign)), + compressed.contact.device_callsign, sizeof(compressed.contact.device_callsign) - 1, USX_PSET_DFLT, NULL); if (length < 0) { LOG_WARN("Compress overflow contact.device_callsign. Revert to uncompressed packet"); return; @@ -87,9 +89,11 @@ void AtakPluginModule::alterReceivedProtobuf(meshtastic_MeshPacket &mp, meshtast LOG_DEBUG("Compressed device_callsign: %d bytes", length); } if (t->which_payload_variant == meshtastic_TAKPacket_chat_tag) { - auto length = unishox2_compress_lines(t->payload_variant.chat.message, strlen(t->payload_variant.chat.message), - compressed.payload_variant.chat.message, - sizeof(compressed.payload_variant.chat.message) - 1, USX_PSET_DFLT, NULL); + auto length = unishox2_compress_lines( + t->payload_variant.chat.message, + pb_string_length(t->payload_variant.chat.message, sizeof(t->payload_variant.chat.message)), + compressed.payload_variant.chat.message, sizeof(compressed.payload_variant.chat.message) - 1, USX_PSET_DFLT, + NULL); if (length < 0) { LOG_WARN("Compress overflow chat.message. Revert to uncompressed packet"); return; @@ -98,9 +102,9 @@ void AtakPluginModule::alterReceivedProtobuf(meshtastic_MeshPacket &mp, meshtast if (t->payload_variant.chat.has_to) { compressed.payload_variant.chat.has_to = true; - length = unishox2_compress_lines(t->payload_variant.chat.to, strlen(t->payload_variant.chat.to), - compressed.payload_variant.chat.to, - sizeof(compressed.payload_variant.chat.to) - 1, USX_PSET_DFLT, NULL); + length = unishox2_compress_lines( + t->payload_variant.chat.to, pb_string_length(t->payload_variant.chat.to, sizeof(t->payload_variant.chat.to)), + compressed.payload_variant.chat.to, sizeof(compressed.payload_variant.chat.to) - 1, USX_PSET_DFLT, NULL); if (length < 0) { LOG_WARN("Compress overflow chat.to. Revert to uncompressed packet"); return; @@ -110,9 +114,11 @@ void AtakPluginModule::alterReceivedProtobuf(meshtastic_MeshPacket &mp, meshtast if (t->payload_variant.chat.has_to_callsign) { compressed.payload_variant.chat.has_to_callsign = true; - length = unishox2_compress_lines(t->payload_variant.chat.to_callsign, strlen(t->payload_variant.chat.to_callsign), - compressed.payload_variant.chat.to_callsign, - sizeof(compressed.payload_variant.chat.to_callsign) - 1, USX_PSET_DFLT, NULL); + length = unishox2_compress_lines( + t->payload_variant.chat.to_callsign, + pb_string_length(t->payload_variant.chat.to_callsign, sizeof(t->payload_variant.chat.to_callsign)), + compressed.payload_variant.chat.to_callsign, sizeof(compressed.payload_variant.chat.to_callsign) - 1, + USX_PSET_DFLT, NULL); if (length < 0) { LOG_WARN("Compress overflow chat.to_callsign. Revert to uncompressed packet"); return; @@ -134,18 +140,18 @@ void AtakPluginModule::alterReceivedProtobuf(meshtastic_MeshPacket &mp, meshtast auto uncompressed = cloneTAKPacketData(t); uncompressed.is_compressed = false; if (t->has_contact) { - auto length = - unishox2_decompress_lines(t->contact.callsign, strlen(t->contact.callsign), uncompressed.contact.callsign, - sizeof(uncompressed.contact.callsign) - 1, USX_PSET_DFLT, NULL); + auto length = unishox2_decompress_lines( + t->contact.callsign, pb_string_length(t->contact.callsign, sizeof(t->contact.callsign)), + uncompressed.contact.callsign, sizeof(uncompressed.contact.callsign) - 1, USX_PSET_DFLT, NULL); if (length < 0) { LOG_WARN("Decompress overflow contact.callsign. Bailing out"); return; } LOG_DEBUG("Decompressed callsign: %d bytes", length); - length = unishox2_decompress_lines(t->contact.device_callsign, strlen(t->contact.device_callsign), - uncompressed.contact.device_callsign, - sizeof(uncompressed.contact.device_callsign) - 1, USX_PSET_DFLT, NULL); + length = unishox2_decompress_lines( + t->contact.device_callsign, pb_string_length(t->contact.device_callsign, sizeof(t->contact.device_callsign)), + uncompressed.contact.device_callsign, sizeof(uncompressed.contact.device_callsign) - 1, USX_PSET_DFLT, NULL); if (length < 0) { LOG_WARN("Decompress overflow contact.device_callsign. Bailing out"); return; @@ -153,9 +159,11 @@ void AtakPluginModule::alterReceivedProtobuf(meshtastic_MeshPacket &mp, meshtast LOG_DEBUG("Decompressed device_callsign: %d bytes", length); } if (uncompressed.which_payload_variant == meshtastic_TAKPacket_chat_tag) { - auto length = unishox2_decompress_lines(t->payload_variant.chat.message, strlen(t->payload_variant.chat.message), - uncompressed.payload_variant.chat.message, - sizeof(uncompressed.payload_variant.chat.message) - 1, USX_PSET_DFLT, NULL); + auto length = unishox2_decompress_lines( + t->payload_variant.chat.message, + pb_string_length(t->payload_variant.chat.message, sizeof(t->payload_variant.chat.message)), + uncompressed.payload_variant.chat.message, sizeof(uncompressed.payload_variant.chat.message) - 1, USX_PSET_DFLT, + NULL); if (length < 0) { LOG_WARN("Decompress overflow chat.message. Bailing out"); return; @@ -164,9 +172,9 @@ void AtakPluginModule::alterReceivedProtobuf(meshtastic_MeshPacket &mp, meshtast if (t->payload_variant.chat.has_to) { uncompressed.payload_variant.chat.has_to = true; - length = unishox2_decompress_lines(t->payload_variant.chat.to, strlen(t->payload_variant.chat.to), - uncompressed.payload_variant.chat.to, - sizeof(uncompressed.payload_variant.chat.to) - 1, USX_PSET_DFLT, NULL); + length = unishox2_decompress_lines( + t->payload_variant.chat.to, pb_string_length(t->payload_variant.chat.to, sizeof(t->payload_variant.chat.to)), + uncompressed.payload_variant.chat.to, sizeof(uncompressed.payload_variant.chat.to) - 1, USX_PSET_DFLT, NULL); if (length < 0) { LOG_WARN("Decompress overflow chat.to. Bailing out"); return; @@ -176,10 +184,11 @@ void AtakPluginModule::alterReceivedProtobuf(meshtastic_MeshPacket &mp, meshtast if (t->payload_variant.chat.has_to_callsign) { uncompressed.payload_variant.chat.has_to_callsign = true; - length = - unishox2_decompress_lines(t->payload_variant.chat.to_callsign, strlen(t->payload_variant.chat.to_callsign), - uncompressed.payload_variant.chat.to_callsign, - sizeof(uncompressed.payload_variant.chat.to_callsign) - 1, USX_PSET_DFLT, NULL); + length = unishox2_decompress_lines( + t->payload_variant.chat.to_callsign, + pb_string_length(t->payload_variant.chat.to_callsign, sizeof(t->payload_variant.chat.to_callsign)), + uncompressed.payload_variant.chat.to_callsign, sizeof(uncompressed.payload_variant.chat.to_callsign) - 1, + USX_PSET_DFLT, NULL); if (length < 0) { LOG_WARN("Decompress overflow chat.to_callsign. Bailing out"); return; diff --git a/test/TestUtil.cpp b/test/TestUtil.cpp index b470b8ce8..a8262238f 100644 --- a/test/TestUtil.cpp +++ b/test/TestUtil.cpp @@ -4,6 +4,13 @@ #include "TestUtil.h" +#if defined(ARDUINO) +#include +#else +#include +#include +#endif + void initializeTestEnvironment() { concurrency::hasBeenSetup = true; @@ -15,4 +22,13 @@ void initializeTestEnvironment() perhapsSetRTC(RTCQualityNTP, &tv); #endif concurrency::OSThread::setup(); +} + +void testDelay(unsigned long ms) +{ +#if defined(ARDUINO) + ::delay(ms); +#else + std::this_thread::sleep_for(std::chrono::milliseconds(ms)); +#endif } \ No newline at end of file diff --git a/test/TestUtil.h b/test/TestUtil.h index ce021e459..fb634184b 100644 --- a/test/TestUtil.h +++ b/test/TestUtil.h @@ -1,4 +1,7 @@ #pragma once // Initialize testing environment. -void initializeTestEnvironment(); \ No newline at end of file +void initializeTestEnvironment(); + +// Portable delay for tests (Arduino or host). +void testDelay(unsigned long ms); \ No newline at end of file diff --git a/test/test_atak/test_main.cpp b/test/test_atak/test_main.cpp new file mode 100644 index 000000000..84078b300 --- /dev/null +++ b/test/test_atak/test_main.cpp @@ -0,0 +1,216 @@ +#include +#include + +#include "TestUtil.h" +#include "meshUtils.h" + +void setUp(void) +{ + // set stuff up here +} + +void tearDown(void) +{ + // clean stuff up here +} + +/** + * Test normal string without embedded nulls + * Should behave the same as strlen() for regular strings + */ +void test_normal_string(void) +{ + char test_str[32] = "Hello World"; + size_t expected = 11; // strlen("Hello World") + size_t result = pb_string_length(test_str, sizeof(test_str)); + TEST_ASSERT_EQUAL_size_t(expected, result); +} + +/** + * Test empty string + * Should return 0 for empty string + */ +void test_empty_string(void) +{ + char test_str[32] = ""; + size_t expected = 0; + size_t result = pb_string_length(test_str, sizeof(test_str)); + TEST_ASSERT_EQUAL_size_t(expected, result); +} + +/** + * Test string with only trailing nulls + * Common case - string followed by null padding + */ +void test_trailing_nulls(void) +{ + char test_str[32] = {0}; + strcpy(test_str, "Test"); + // test_str is now: "Test\0\0\0\0..." (4 chars + 28 nulls) + size_t expected = 4; + size_t result = pb_string_length(test_str, sizeof(test_str)); + TEST_ASSERT_EQUAL_size_t(expected, result); +} + +/** + * Test string with embedded null byte + * This is the critical bug case - strlen() would truncate at first null + */ +void test_embedded_null(void) +{ + char test_str[32] = {0}; + // Create string "ABC\0XYZ" (embedded null after C) + test_str[0] = 'A'; + test_str[1] = 'B'; + test_str[2] = 'C'; + test_str[3] = '\0'; // embedded null + test_str[4] = 'X'; + test_str[5] = 'Y'; + test_str[6] = 'Z'; + // Rest is already null from initialization + + // strlen would return 3, but pb_string_length should return 7 + size_t strlen_result = strlen(test_str); + size_t pb_result = pb_string_length(test_str, sizeof(test_str)); + + TEST_ASSERT_EQUAL_size_t(3, strlen_result); // strlen stops at first null + TEST_ASSERT_EQUAL_size_t(7, pb_result); // pb_string_length finds last non-null +} + +/** + * Test Android UID with embedded null bytes + * Real-world case from bug report: ANDROID-e7e455b40002429d + * The "00" in the UID represents 0x00 bytes that were truncating the string + */ +void test_android_uid_pattern(void) +{ + char test_str[32] = {0}; + // Simulate "ANDROID-e7e455b4" + 0x00 + 0x00 + "2429d" + const char part1[] = "ANDROID-e7e455b4"; + strcpy(test_str, part1); + size_t pos = strlen(part1); + test_str[pos] = '\0'; // embedded null + test_str[pos + 1] = '\0'; // another embedded null + strcpy(test_str + pos + 2, "2429d"); + + // The full UID should be 24 characters + size_t strlen_result = strlen(test_str); + size_t pb_result = pb_string_length(test_str, sizeof(test_str)); + + TEST_ASSERT_EQUAL_size_t(16, strlen_result); // strlen truncates to "ANDROID-e7e455b4" + TEST_ASSERT_EQUAL_size_t(23, pb_result); // pb_string_length gets full length +} + +/** + * Test string with multiple embedded nulls + * Edge case with several null bytes scattered through the string + */ +void test_multiple_embedded_nulls(void) +{ + char test_str[32] = {0}; + // Create "A\0B\0C\0D" (3 embedded nulls) + test_str[0] = 'A'; + test_str[1] = '\0'; + test_str[2] = 'B'; + test_str[3] = '\0'; + test_str[4] = 'C'; + test_str[5] = '\0'; + test_str[6] = 'D'; + + size_t strlen_result = strlen(test_str); + size_t pb_result = pb_string_length(test_str, sizeof(test_str)); + + TEST_ASSERT_EQUAL_size_t(1, strlen_result); // strlen stops at first null + TEST_ASSERT_EQUAL_size_t(7, pb_result); // pb_string_length finds all chars +} + +/** + * Test buffer completely filled with non-null characters + * Edge case where string uses entire buffer + */ +void test_full_buffer(void) +{ + char test_str[8]; + // Fill entire buffer with 'X' + memset(test_str, 'X', sizeof(test_str)); + + size_t result = pb_string_length(test_str, sizeof(test_str)); + TEST_ASSERT_EQUAL_size_t(8, result); +} + +/** + * Test buffer with all nulls + * Should return 0 + */ +void test_all_nulls(void) +{ + char test_str[32] = {0}; + size_t result = pb_string_length(test_str, sizeof(test_str)); + TEST_ASSERT_EQUAL_size_t(0, result); +} + +/** + * Test single character followed by nulls + * Minimal non-empty case + */ +void test_single_char(void) +{ + char test_str[32] = {0}; + test_str[0] = 'X'; + + size_t result = pb_string_length(test_str, sizeof(test_str)); + TEST_ASSERT_EQUAL_size_t(1, result); +} + +/** + * Test callsign field typical size + * Test with typical ATAK callsign field size (64 bytes) + */ +void test_callsign_field_size(void) +{ + char test_str[64] = {0}; + strcpy(test_str, "CALLSIGN-123"); + + size_t result = pb_string_length(test_str, sizeof(test_str)); + TEST_ASSERT_EQUAL_size_t(12, result); +} + +/** + * Test with data at end of buffer + * String with embedded null and data at very end + */ +void test_data_at_buffer_end(void) +{ + char test_str[10] = {0}; + test_str[0] = 'A'; + test_str[1] = '\0'; + test_str[8] = 'Z'; // Data near end + test_str[9] = 'X'; // Data at end + + size_t result = pb_string_length(test_str, sizeof(test_str)); + TEST_ASSERT_EQUAL_size_t(10, result); // Should find the 'X' at position 9 +} + +void setup() +{ + // NOTE!!! Wait for >2 secs + // if board doesn't support software reset via Serial.DTR/RTS + testDelay(10); + testDelay(2000); + + UNITY_BEGIN(); + RUN_TEST(test_normal_string); + RUN_TEST(test_empty_string); + RUN_TEST(test_trailing_nulls); + RUN_TEST(test_embedded_null); + RUN_TEST(test_android_uid_pattern); + RUN_TEST(test_multiple_embedded_nulls); + RUN_TEST(test_full_buffer); + RUN_TEST(test_all_nulls); + RUN_TEST(test_single_char); + RUN_TEST(test_callsign_field_size); + RUN_TEST(test_data_at_buffer_end); + exit(UNITY_END()); +} + +void loop() {}