Bound CommonCLI arguments before scratch copies

.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

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];

test/fixtures/common_cli_argument_bounds/main.cpp

@@ -0,0 +1,179 @@
+#include <array>
+#include <cassert>
+#include <cstdint>
+#include <cstdio>
+#include <cstring>
+#include <initializer_list>
+#include <string>
+#include <helpers/CLICommandUtils.h>
+#include <helpers/GpsPowerPolicy.h>
+
+@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<uint8_t, 3> 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<char, 160> 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<uint8_t, 3>{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");
+}

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()