From a7828d5342794a90982b2326fe707b59b2f7e235 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Thu, 15 Oct 2026 23:02:05 -0700 Subject: [PATCH] Bound CommonCLI arguments before scratch copies --- .github/workflows/run-unit-tests.yml | 1 + src/helpers/CommonCLI.cpp | 23 ++- .../common_cli_argument_bounds/main.cpp | 179 ++++++++++++++++++ test/test_common_cli_argument_bounds.py | 105 ++++++++++ 4 files changed, 305 insertions(+), 3 deletions(-) create mode 100644 test/fixtures/common_cli_argument_bounds/main.cpp create mode 100644 test/test_common_cli_argument_bounds.py diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index 544e5c1dc..8aa6994d2 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -177,6 +177,7 @@ jobs: run: | python3 -B test/test_protocol_parser_bounds.py -v python3 -B test/test_cli_error_bounds.py -v + python3 -B test/test_common_cli_argument_bounds.py -v python3 -B test/test_ota_security.py -v python3 -B test/test_companion_offline_delivery.py -v python3 -B test/test_room_receive_admission.py -v diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index 18ce5ab1a..999b496c2 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -207,6 +207,14 @@ static const size_t LEGACY_MQTT_GAP_6SLOT = 306 + 6 * 186; // 1422 static const size_t LEGACY_MQTT_GAP_3SLOT = 306 + 3 * 186; // 864 static const size_t LEGACY_OBS_TAIL_MAX = 124; // rx_boosted(1) + flood(2) + snmp(25) + watchdog(1) + alert block(95) +static bool copyCLIArgument(char* dest, size_t capacity, const char* argument) { + size_t len = 0; + while (len < capacity && argument[len] != 0) len++; + if (len == capacity) return false; + memcpy(dest, argument, len + 1); + return true; +} + static bool looksNumeric(const char* s) { if (s == NULL) return false; while (*s == ' ') s++; @@ -3323,7 +3331,10 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, char* command, char* re strcpy(reply, "null"); } } else if (memcmp(command, "sensor set ", 11) == 0) { - strcpy(tmp, &command[11]); + if (!copyCLIArgument(tmp, sizeof(tmp), &command[11])) { + strcpy(reply, "Error, argument too long"); + return; + } const char *parts[2]; int num = mesh::Utils::parseTextParts(tmp, parts, 2, ' '); const char *key = (num > 0) ? parts[0] : ""; @@ -4890,6 +4901,10 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep (unsigned int)FLOOD_RETRY_BUCKET_PREFIXES); } } else if (memcmp(config, "direct.retry.cr ", 16) == 0) { + if (!copyCLIArgument(tmp, sizeof(tmp), &config[16])) { + strcpy(reply, "Error, argument too long"); + return; + } if (strcmp(&config[16], "off") == 0) { _prefs->direct_retry_cr_enabled = 0; savePrefs(); @@ -4903,7 +4918,6 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep strcpy(reply, "Error, use on or off on this role"); } } else { - strcpy(tmp, &config[16]); const char *parts[4]; int num = mesh::Utils::parseTextParts(tmp, parts, 4, ','); if (num == 4 && looksNumeric(parts[0]) && looksNumeric(parts[1]) && looksNumeric(parts[2]) && looksNumeric(parts[3])) { @@ -4927,7 +4941,10 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep } } } else if (memcmp(config, "recent.repeater ", 16) == 0) { - strcpy(tmp, &config[16]); + if (!copyCLIArgument(tmp, sizeof(tmp), &config[16])) { + strcpy(reply, "Error, argument too long"); + return; + } const char *parts[2]; int num = mesh::Utils::parseTextParts(tmp, parts, 2, ' '); uint8_t prefix[MAX_HASH_SIZE]; diff --git a/test/fixtures/common_cli_argument_bounds/main.cpp b/test/fixtures/common_cli_argument_bounds/main.cpp new file mode 100644 index 000000000..44a0a4c11 --- /dev/null +++ b/test/fixtures/common_cli_argument_bounds/main.cpp @@ -0,0 +1,179 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include + +@CONSTANTS@ + +namespace mesh { +struct Utils { + static int parseTextParts(char*, const char*[], int, char = ','); + static void toHex(char*, const uint8_t*, size_t); + static bool isHexChar(char); + static bool fromHex(uint8_t*, int, const char*); +}; +@UTILS@ +} + +@PARSERS@ + +struct Prefs { + uint32_t gps_interval = 7; + uint8_t gps_enabled = 0, bridge_uart = 1, direct_retry_cr_enabled = 0; + int8_t direct_retry_cr4_snr_x4 = 40, direct_retry_cr5_snr_x4 = 30; + int8_t direct_retry_cr7_snr_x4 = 0, direct_retry_cr8_snr_x4 = -10; +}; +struct Sensors { + unsigned calls = 0; + std::string key, value; + bool setSettingValue(const char* k, const char* v) { + ++calls; key = k; value = v; return true; + } + bool gpsUsesSerialUart(uint8_t) { return true; } +}; +struct Callbacks { + bool advanced = true, bridge = false; + unsigned support_calls = 0, recent_calls = 0; + std::array prefix{}; + uint8_t prefix_len = 0; + int8_t snr = 0; + bool supportsAdvancedRetryConfig() { ++support_calls; return advanced; } + bool isRs232BridgeRunning() { return bridge; } + bool setRecentRepeater(const uint8_t* p, uint8_t n, int8_t s) { + ++recent_calls; memcpy(prefix.data(), p, n); prefix_len = n; snr = s; return true; + } +}; +struct CommonCLI { + // The production branches see the real array capacity. Keeping the backing + // array external lets ASan detect its exact end independently of other fields. + char (&tmp)[COMMON_CLI_TMP_LEN]; + Prefs prefs; + Prefs* _prefs = &prefs; + Sensors sensors; + Sensors* _sensors = &sensors; + Callbacks callbacks; + Callbacks* _callbacks = &callbacks; + unsigned saves = 0; + explicit CommonCLI(char (&scratch)[COMMON_CLI_TMP_LEN]) : tmp(scratch) {} + void savePrefs() { ++saves; } + void run(const char* command, char* reply) { + const char* config = command; + @SENSOR_BRANCH@ + else @RETRY_BRANCH@ + else @RECENT_BRANCH@ + else assert(false); + } +}; + +static std::string argument(const std::string& kind, size_t size) { + const std::string base = kind == "sensor" ? "custom " : + kind == "gps" ? "gps_interval " : + kind == "retry" ? "0,0,0," : "AABBCC "; + assert(size >= base.size()); + return base + std::string(size - base.size(), kind == "sensor" ? 'x' : '0'); +} +static std::string command(const std::string& kind, const std::string& args) { + return (kind == "sensor" || kind == "gps" ? "sensor set " : + kind == "retry" ? "direct.retry.cr " : "recent.repeater ") + args; +} +static void untouched(const CommonCLI& cli) { + assert(cli.saves == 0 && cli.sensors.calls == 0); + assert(cli.callbacks.support_calls == 0 && cli.callbacks.recent_calls == 0); + assert(cli.prefs.gps_interval == 7 && cli.prefs.gps_enabled == 0); + assert(cli.prefs.direct_retry_cr_enabled == 0); + assert(cli.prefs.direct_retry_cr4_snr_x4 == 40 && cli.prefs.direct_retry_cr5_snr_x4 == 30); + assert(cli.prefs.direct_retry_cr7_snr_x4 == 0 && cli.prefs.direct_retry_cr8_snr_x4 == -10); +} +static void exercise(const std::string& kind, size_t size, bool advanced, bool adjacent) { + char exact[COMMON_CLI_TMP_LEN]; + struct Buffers { + uint8_t before[16]; + char scratch[COMMON_CLI_TMP_LEN]; + uint8_t after[16]; + } buffers; + memset(exact, 0x55, sizeof(exact)); + memset(&buffers, 0x55, sizeof(buffers)); + CommonCLI cli(adjacent ? buffers.scratch : exact); + cli.callbacks.advanced = advanced; + std::array reply; + reply.fill(char(0x55)); + const auto args = argument(kind, size); + const auto input = command(kind, args); + cli.run(input.c_str(), reply.data()); + if (size >= COMMON_CLI_TMP_LEN) { + assert(strcmp(reply.data(), "Error, argument too long") == 0); + untouched(cli); + for (char value : cli.tmp) assert(value == char(0x55)); + } else if (kind == "sensor" || kind == "gps") { + assert(strcmp(reply.data(), "ok") == 0 && cli.sensors.calls == 1); + assert(cli.sensors.value == args.substr(args.find(' ') + 1)); + assert(cli.saves == (kind == "gps" ? 1U : 0U)); + if (kind == "gps") assert(cli.prefs.gps_interval == 0); + } else if (kind == "retry") { + assert(cli.callbacks.support_calls == 1); + assert(cli.saves == (advanced ? 1U : 0U)); + assert(strcmp(reply.data(), advanced ? "OK" : "Error, use on or off on this role") == 0); + if (advanced) { + assert(cli.prefs.direct_retry_cr_enabled == 1); + assert(cli.prefs.direct_retry_cr4_snr_x4 == 0 && cli.prefs.direct_retry_cr5_snr_x4 == 0); + assert(cli.prefs.direct_retry_cr7_snr_x4 == 0 && cli.prefs.direct_retry_cr8_snr_x4 == 0); + } + } else { + assert(strncmp(reply.data(), "OK - set AABBCC", 15) == 0); + assert(cli.callbacks.recent_calls == 1 && cli.callbacks.prefix_len == 3); + assert((cli.callbacks.prefix == std::array{0xAA, 0xBB, 0xCC})); + assert(cli.callbacks.snr == 0); + } + for (size_t i = strlen(reply.data()) + 1; i < reply.size(); ++i) + assert(reply[i] == char(0x55)); + for (uint8_t value : buffers.before) assert(value == 0x55); + for (uint8_t value : buffers.after) assert(value == 0x55); +} +static void unchangedModes() { + char scratch[COMMON_CLI_TMP_LEN]; + char reply[160]; + CommonCLI cli(scratch); + for (bool advanced : {false, true}) { + cli.callbacks.advanced = advanced; + cli.run("direct.retry.cr off", reply); + assert(strcmp(reply, "OK") == 0 && cli.prefs.direct_retry_cr_enabled == 0); + if (!advanced) { + cli.run("direct.retry.cr on", reply); + assert(strcmp(reply, "OK") == 0 && cli.prefs.direct_retry_cr_enabled == 1); + } + } + cli.run("sensor set gps 1", reply); + assert(strcmp(reply, "ok") == 0 && cli.prefs.gps_enabled == 1); + cli.run("recent.repeater AA", reply); + assert(cli.callbacks.prefix_len == 1 && cli.callbacks.snr == 12); +} +int main(int argc, char** argv) { + assert(argc == 2); + const std::string scenario(argv[1]); + if (scenario != "production") { + exercise(scenario, 179, true, false); + return 0; + } + // Exercise the helper itself using exact arrays, including the terminator. + char one[1] = {'x'}; + assert(!copyCLIArgument(one, 0, "") && one[0] == 'x'); + assert(copyCLIArgument(one, sizeof(one), "") && one[0] == 0); + assert(!copyCLIArgument(one, sizeof(one), "x") && one[0] == 0); + for (const std::string kind : {"sensor", "gps", "retry", "recent"}) { + for (bool adjacent : {false, true}) { + for (bool advanced : {false, true}) { + for (size_t size : {size_t(COMMON_CLI_TMP_LEN - 1), size_t(COMMON_CLI_TMP_LEN), + size_t(159) - command(kind, "").size(), + size_t(179) - command(kind, "").size(), size_t(4096)}) + exercise(kind, size, advanced, adjacent); + } + } + } + unchangedModes(); + puts("PASS: production CLI argument bounds"); +} diff --git a/test/test_common_cli_argument_bounds.py b/test/test_common_cli_argument_bounds.py new file mode 100644 index 000000000..d0d762a1f --- /dev/null +++ b/test/test_common_cli_argument_bounds.py @@ -0,0 +1,105 @@ +#!/usr/bin/env python3 +"""Exercise production scratch-copy branches with exact bounds and side effects.""" + +from pathlib import Path +import os +import re +import shutil +import subprocess +import tempfile +import unittest + +from cpp_source import body + + +ROOT = Path(__file__).resolve().parents[1] +FIXTURE = ROOT / "test/fixtures/common_cli_argument_bounds/main.cpp" + + +class CommonCLIArgumentBoundsTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + compiler = os.environ.get("CXX") or shutil.which("g++") or shutil.which("clang++") + if not compiler: + raise RuntimeError("CLI argument bounds require a C++17 sanitizer compiler") + common = (ROOT / "src/helpers/CommonCLI.cpp").read_text() + header = (ROOT / "src/helpers/CommonCLI.h").read_text() + core = (ROOT / "src/MeshCore.h").read_text() + utils = (ROOT / "src/Utils.cpp").read_text() + constants = header[header.index("#ifndef FLOOD_RETRY_PREFIX_SLOTS"): + header.index("#ifndef FLOOD_CHANNEL_KEY_PREFIX_LEN")] + for name in ("PRV_KEY_SIZE", "MAX_HASH_SIZE"): + constants += re.search(r"^#define " + name + r"\s+.+$", core, re.MULTILINE).group() + "\n" + constants += re.search(r"^#define RECENT_REPEATER_PREFIX_MAX_BYTES\s+.+$", + common, re.MULTILINE).group() + parsers = "\n".join(body(common, signature) for signature in ( + "static bool copyCLIArgument(", "static bool looksNumeric(", + "static bool looksUnsignedInteger(", "static const char* skipSpacesConst(", + "static bool parseUint32Strict(", "static int16_t parseSnrDbX4(", + "static void formatSnrDbX4(", "static bool parseHashPrefix(", + "static void formatSnrDbX4Short(")) + utils_code = re.search(r"static const char hex_chars\[\][^;]+;", utils).group() + utils_code += "\n" + "\n".join(body(utils, signature) for signature in ( + "static uint8_t hexVal(", "void Utils::toHex(", "bool Utils::isHexChar(", + "bool Utils::fromHex(", "int Utils::parseTextParts(")) + source = FIXTURE.read_text(encoding="ascii") + replacements = { + "CONSTANTS": constants, "PARSERS": parsers, "UTILS": utils_code, + "SENSOR_BRANCH": body(common, 'if (memcmp(command, "sensor set ", 11) == 0)'), + "RETRY_BRANCH": body(common, 'if (memcmp(config, "direct.retry.cr ", 16) == 0)'), + "RECENT_BRANCH": body(common, 'if (memcmp(config, "recent.repeater ", 16) == 0)'), + } + for key, value in replacements.items(): + source = source.replace("@" + key + "@", value) + cls.work = tempfile.TemporaryDirectory(prefix="meshcore-cli-argument-bounds-") + cls.binaries = {} + for name, defines in (("default", []), ("larger", ["-DCOMMON_CLI_TMP_LEN=192"]), + ("legacy", [])): + generated = source + if name == "legacy": + # Restore all three audited unchecked copies as a negative control. + pattern = (r'if \(!copyCLIArgument\(tmp, sizeof\(tmp\), (&(?:command|config)\[\d+\])\)\) ' + r'\{\s*strcpy\(reply, "Error, argument too long"\);\s*return;\s*\}') + generated, count = re.subn(pattern, r"strcpy(tmp, \1);", generated) + assert count == 3, "review the negative controls after branch changes" + directory = Path(cls.work.name) + path, binary = directory / (name + ".cpp"), directory / name + path.write_text(generated, encoding="ascii") + result = subprocess.run([ + compiler, "-std=c++17", "-O1", "-g", "-Wall", "-Wextra", "-Werror", + "-fsanitize=address,undefined", "-fno-sanitize-recover=all", + "-fno-omit-frame-pointer", "-fno-pie", "-no-pie", *defines, + "-I" + str(ROOT / "src"), str(path), "-o", str(binary), + ], capture_output=True, text=True, timeout=60) + if result.returncode: + cls.work.cleanup() + raise AssertionError(result.stdout + result.stderr) + cls.binaries[name] = binary + + @classmethod + def tearDownClass(cls): + cls.work.cleanup() + + def run_case(self, variant, scenario): + return subprocess.run([str(self.binaries[variant]), scenario], capture_output=True, + text=True, timeout=15, + env=dict(os.environ, ASAN_OPTIONS="detect_leaks=0:abort_on_error=0")) + + def test_production_boundaries_and_rejection_without_effects(self): + for variant in ("default", "larger"): + with self.subTest(variant=variant): + result = self.run_case(variant, "production") + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("PASS: production CLI argument bounds", result.stdout) + + def test_all_three_previous_copies_fail_address_sanitizer(self): + for command in ("sensor", "retry", "recent"): + with self.subTest(command=command): + result = self.run_case("legacy", command) + self.assertNotEqual(result.returncode, 0) + self.assertIn("AddressSanitizer", result.stderr) + self.assertIn("buffer-overflow", result.stderr) + + +if __name__ == "__main__": + unittest.main()