From edf3f85199d495f57d9704c723402f21adea20b4 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Fri, 16 Oct 2026 00:15:55 -0700 Subject: [PATCH] Reject ambiguous management firmware versions without scanf --- src/helpers/ManagementReporter.cpp | 37 ++++++++++++--- test/fixtures/management/CommonCLI.h | 3 +- test/fixtures/management/runtime_test.cpp | 55 ++++++++++++++++++++++- test/test_management_report.py | 37 +++++++++++++++ 4 files changed, 125 insertions(+), 7 deletions(-) diff --git a/src/helpers/ManagementReporter.cpp b/src/helpers/ManagementReporter.cpp index 5ff817362..c84d7fe5e 100644 --- a/src/helpers/ManagementReporter.cpp +++ b/src/helpers/ManagementReporter.cpp @@ -34,6 +34,35 @@ static unsigned parseDays(const char* text) { } while (*text); return days; } + +// MGR2 has three or four decimal version bytes, followed only by an optional +// release/build label. Keep that label opaque for existing tagged builds. +static bool parseManagementFirmwareVersion(const char* text, uint32_t& version) { + if (!text) return false; + if (*text == 'v') ++text; + uint32_t packed = 0; + unsigned components = 0; + while (true) { + if (*text < '0' || *text > '9') return false; + unsigned component = 0; + do { + const unsigned digit = *text++ - '0'; + if (component > (255U - digit) / 10U) return false; + component = component * 10U + digit; + } while (*text >= '0' && *text <= '9'); + packed = (packed << 8) | component; + ++components; + if (*text != '.') break; + if (components == 4) return false; + ++text; + } + if (components < 3) return false; + if (*text && ((*text != '-' && *text != '+') || !text[1])) return false; + if (components == 3) packed <<= 8; + version = packed; + return true; +} + struct ManagementReporter::Working { History history; Extrema during_report; @@ -201,11 +230,9 @@ void ManagementReporter::snapshot() { #endif // If EndF is absent, only publish a version we can parse unambiguously. if (!(valid & FIRMWARE)) { - unsigned major = 0, minor = 0, patch = 0, pre = 0; - const char* version = callbacks.getFirmwareVer(); if (*version == 'v') ++version; - const int n = sscanf(version, "%u.%u.%u.%u", &major, &minor, &patch, &pre); - if (n >= 3 && major <= 255 && minor <= 255 && patch <= 255 && pre <= 255) { - write32(p + 28, major << 24 | minor << 16 | patch << 8 | pre); valid |= FIRMWARE; + uint32_t version; + if (parseManagementFirmwareVersion(callbacks.getFirmwareVer(), version)) { + write32(p + 28, version); valid |= FIRMWARE; } } write16(p + 76, valid); diff --git a/test/fixtures/management/CommonCLI.h b/test/fixtures/management/CommonCLI.h index 86517e792..b003154ef 100644 --- a/test/fixtures/management/CommonCLI.h +++ b/test/fixtures/management/CommonCLI.h @@ -29,7 +29,8 @@ public: }; class CommonCLICallbacks { public: - const char* getFirmwareVer() { return "v1.17.1.5"; } + const char* firmware_version = "v1.17.1.5"; + const char* getFirmwareVer() { return firmware_version; } const char* getRole() { return "repeater"; } }; struct LocationProvider { bool isEnabled() { return false; } }; diff --git a/test/fixtures/management/runtime_test.cpp b/test/fixtures/management/runtime_test.cpp index d379c70e4..8b4acbf11 100644 --- a/test/fixtures/management/runtime_test.cpp +++ b/test/fixtures/management/runtime_test.cpp @@ -58,7 +58,60 @@ struct Fixture { return legacy; } }; -int main() { +static void versionSnapshot(const char* version, bool valid_version, uint32_t packed = 0) { + Fixture f; + f.callbacks.firmware_version = version; + f.configure(); f.advance(5 * DAY + 60); + assert(f.mesh.packets.size() == 1); + const auto* header = f.mesh.packets[0].payload; + assert(!memcmp(header, "MGR2", 4)); + const bool reported_valid = (read16(header + 76) & FIRMWARE) != 0; + const uint32_t reported_version = read32(header + 28); + if (reported_valid != valid_version || reported_version != packed) + fprintf(stderr, "version %s: expected valid=%u packed=%08x; got valid=%u packed=%08x\n", + version ? version : "(null)", unsigned(valid_version), unsigned(packed), + unsigned(reported_valid), unsigned(reported_version)); + assert(reported_valid == valid_version && reported_version == packed); +} + +static void versionSnapshots() { + for (const char* version : {"1.2.3", "v1.2.3", "v001.002.003", + "v1.2.3-release", "v1.2.3+build_42", "v1.2.3-rc.1+build_42"}) + versionSnapshot(version, true, 0x01020300); + for (const char* version : {"1.2.3.4", "v1.2.3.004", "v1.2.3.4-preview1", + "v1.2.3.4+build_42", "v1.2.3.4-label_with_underscores"}) + versionSnapshot(version, true, 0x01020304); + versionSnapshot("v1.18.0", true, 0x01120000); // Direct-build example default. + versionSnapshot("v1.17.1.9-halo-keymind-cascade-dev-5f4d242f", true, 0x01110109); + versionSnapshot("v1.17.1.9-halo-keymind-cascade-dev.42-observer-dev-5f4d242f", true, 0x01110109); + versionSnapshot("v1.18.0.42-5f4d242f", true, 0x0112002a); // Three-part base + build number. + versionSnapshot("255.255.255.255", true, UINT32_MAX); + versionSnapshot("0.0.0", true); + versionSnapshot("v0.0.0.0+build_0", true); + for (const char* version : {"", "v", "1", "1.2", "1..3", "1.2.", "1.2.3.", + "1.2.3.4.5", "1.2.3.xyz", "1.2.3garbage", "1.2.3.4garbage", "1.2.3 arbitrary text", + "1.2.3-", "1.2.3+", "1.2.3.4-", "1.2.3.4+", "release-v1.2.3", + " 1.2.3", "1. 2.3", "1.2.3 ", "1.2.3. 4", "+1.2.3", "1.+2.3", "1.2.-3", + "1.2.3.+4", "1.2.3.-4", "256.0.0", "0.256.0", "0.0.256", "0.0.0.256", + "v4294967297.2.3", "v1.2.3.4294967297", "v-4294967295.2.3", "dev-5f4d242f"}) + versionSnapshot(version, false); + const std::string huge_component(4096, '9'); + versionSnapshot(("v" + huge_component + ".2.3").c_str(), false); + versionSnapshot(("v1.2.3." + huge_component).c_str(), false); + versionSnapshot(nullptr, false); +} + +int main(int argc, char** argv) { + if (argc == 2 && !strcmp(argv[1], "versions")) { + versionSnapshots(); + puts("PASS production MGR2 firmware version snapshots"); + return 0; + } + if (argc == 2 && !strcmp(argv[1], "version-malformed")) { + versionSnapshot("v1.2.3garbage", false); + return 0; + } + assert(argc == 1); { Fixture f; assert(f.cmd("get mgmt").find("direct=5d") != std::string::npos); diff --git a/test/test_management_report.py b/test/test_management_report.py index e50850703..7a2bb24df 100644 --- a/test/test_management_report.py +++ b/test/test_management_report.py @@ -18,6 +18,18 @@ spec = importlib.util.spec_from_file_location("management", ROOT / "tools/manage report = importlib.util.module_from_spec(spec) spec.loader.exec_module(report) +# Audited old snapshot fallback: the real libc conversion accepts a malformed +# numeric prefix as a valid-looking MGR2 version. Keep a negative runtime +# control so the snapshot regression proves it detects that bug. +LEGACY_VERSION_FALLBACK = r'''if (!(valid & FIRMWARE)) { + unsigned major = 0, minor = 0, patch = 0, pre = 0; + const char* version = callbacks.getFirmwareVer(); if (*version == 'v') ++version; + const int n = sscanf(version, "%u.%u.%u.%u", &major, &minor, &patch, &pre); + if (n >= 3 && major <= 255 && minor <= 255 && patch <= 255 && pre <= 255) { + write32(p + 28, major << 24 | minor << 16 | patch << 8 | pre); valid |= FIRMWARE; + } + }''' + class ManagementTests(unittest.TestCase): @classmethod @@ -76,6 +88,17 @@ class ManagementTests(unittest.TestCase): runtime_cmd += [str(cls.crypto / p) for p in sources] + ["-o", str(runtime)] subprocess.run(runtime_cmd, check=True, capture_output=True, text=True) cls.runtime = runtime + runtime_source = Path(cls.work.name) / "ManagementReporter.cpp" + runtime_text = runtime_source.read_text(encoding="ascii") + fallback = extract_braced(runtime_text, "if (!(valid & FIRMWARE))") + legacy_source = Path(cls.work.name) / "ManagementReporter-legacy.cpp" + legacy_source.write_text(runtime_text.replace(fallback, LEGACY_VERSION_FALLBACK, 1), + encoding="ascii") + cls.legacy_runtime = Path(cls.work.name) / "runtime-legacy-version" + legacy_cmd = list(runtime_cmd) + legacy_cmd[legacy_cmd.index(str(runtime_source))] = str(legacy_source) + legacy_cmd[-1] = str(cls.legacy_runtime) + subprocess.run(legacy_cmd, check=True, capture_output=True, text=True) @classmethod def tearDownClass(cls): @@ -437,6 +460,20 @@ process.stdin.on('end', async () => { def test_runtime_storage_scheduling_rollover_and_failures(self): subprocess.run([str(self.runtime)], check=True) + def test_production_snapshot_versions_are_bounded_and_release_compatible(self): + result = subprocess.run([str(self.runtime), "versions"], capture_output=True, + text=True, timeout=30) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("PASS production MGR2 firmware version snapshots", result.stdout) + + def test_previous_snapshot_parser_reports_malformed_input_as_a_valid_version(self): + result = subprocess.run([str(self.legacy_runtime), "version-malformed"], capture_output=True, + text=True, timeout=30) + self.assertNotEqual(result.returncode, 0) + self.assertIn("version v1.2.3garbage: expected valid=0", result.stderr) + self.assertIn("got valid=1", result.stderr) + self.assertIn("Assertion", result.stderr) + def test_actual_legacy_route_adoption_persistence_and_precedence(self): source = (ROOT / "src/helpers/CommonCLI_Management.cpp").read_text(encoding="utf-8") state = source[source.index("static constexpr char DATA_ROUTE_FILE[]"):