From 999883a9a393d6dde7021a1229783abcf2da795b Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Wed, 29 Jul 2026 08:26:48 -0700 Subject: [PATCH] fix: reject/clamp LDF signal bit_width > 64 to prevent UB in decode() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lin::ldf::DB::decode() shifts a uint64_t by up to bit_width - 1 bits (`1ULL << i`) with no upper bound on the LDF-declared bit_width. LDF signals with bit_width > 64 (unconstrained int, no range check in parse_signals()) trigger a shift-by->=64, which is undefined behavior and aborts under UBSan. Reaching the actual UB requires decode()'s data argument to exceed 8 bytes, which is a normal, unbounded std::vector& — a malformed or crafted .ldf file can therefore crash any process that parses it and later decodes an oversized frame against it. Fixes: - parse_signals() now rejects (skips) any signal whose parsed bit_width falls outside [1, 64], consistent with how the parser already skips other malformed signal lines rather than storing bad data. - decode() additionally clamps its own loop bound to [0, 64] as defense in depth, since DB's storage members are public and a Signal can be constructed directly by a caller that bypasses the text parser entirely. Closes #18 Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --- src/ldf/parser.cpp | 15 +++++++++- tests/test_ldf.cpp | 70 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/src/ldf/parser.cpp b/src/ldf/parser.cpp index 3a97471..2123f78 100644 --- a/src/ldf/parser.cpp +++ b/src/ldf/parser.cpp @@ -60,7 +60,13 @@ DB::decode(uint8_t id, const std::vector& data) const { // LSB-first (Intel) bit extraction — REQ-LDF-009 uint64_t val = 0; - int bit_width = sit->second.bit_width; + // Defense-in-depth clamp: parse_signals() already rejects + // out-of-range bit_width at parse time, but decode() must not rely + // solely on that — a Signal can also be constructed directly by + // callers who bypass the LDF text parser. `1 << i` for i >= 64 is + // undefined behavior (aborts under UBSan); clamp the loop bound + // itself rather than trusting the stored value. + int bit_width = std::clamp(sit->second.bit_width, 0, 64); for (int i = 0; i < bit_width; ++i) { int byte_idx = (ref.bit_offset + i) / 8; int bit_idx = (ref.bit_offset + i) % 8; @@ -193,6 +199,13 @@ struct Parser { Signal sig; sig.name = name; try { sig.bit_width = static_cast(parse_int(parts[0])); } catch (...) {} + // A bit_width outside [1, 64] cannot be decoded (decode()'s bit + // extraction loop shifts a uint64_t by `i` bits) and LIN frames + // are at most kLINMaxDataLen (8) bytes = 64 bits wide anyway, so + // treat it the same as any other malformed signal line: skip it + // rather than storing a Signal that would later cause undefined + // behavior (shift-by->=64) in decode(). fusa:req REQ-LDF-008 + if (sig.bit_width < 1 || sig.bit_width > 64) continue; try { sig.init_value = parse_uint(parts[1]); } catch (...) {} sig.publisher = trim(parts[2]); for (std::size_t i = 3; i < parts.size(); ++i) { diff --git a/tests/test_ldf.cpp b/tests/test_ldf.cpp index 4b9e0ae..b449063 100644 --- a/tests/test_ldf.cpp +++ b/tests/test_ldf.cpp @@ -190,3 +190,73 @@ TEST_CASE("Frames() returns defensive copy", "[ldf][REQ-LDF-015]") { frames.clear(); CHECK(db->frame(0x10) != nullptr); } + +TEST_CASE("parse rejects signal bit_width > 64", "[ldf][REQ-LDF-007]") { + // BigSignal's declared bit_width (128) exceeds what any uint64_t-based + // decode() can represent (and exceeds 8*kLINMaxDataLen bits anyway) — + // parse_signals() must not store it, or a later decode() call against a + // >8-byte buffer would shift a uint64_t by >=64 bits (UB). + static const char* kLDF = R"( +Signals { + BigSignal : 128, 0, MotorControl, BCM ; + MotorSpeed : 8, 0, MotorControl, BCM ; +} + +Frames { + BigFrame : 0x30, MotorControl, 10 { + BigSignal, 0 ; + } +} +)"; + std::istringstream ss(kLDF); + auto db = parse(ss); + REQUIRE(db != nullptr); + CHECK(db->signal("BigSignal") == nullptr); + CHECK(db->signal("MotorSpeed") != nullptr); + + // The frame itself still parses; decoding it against a >8-byte buffer + // (the precondition needed to actually reach the unclamped shift) must + // not crash, and the unresolvable signal is simply absent from the + // result rather than UB. + std::vector data(10, 0xFF); + std::unordered_map result; + REQUIRE_NOTHROW(result = db->decode(0x30, data)); + CHECK(result.count("BigSignal") == 0); +} + +TEST_CASE("parse rejects signal bit_width == 0", "[ldf][REQ-LDF-007]") { + static const char* kLDF = R"( +Signals { + ZeroWidth : 0, 0, MotorControl, BCM ; +} +)"; + std::istringstream ss(kLDF); + auto db = parse(ss); + REQUIRE(db != nullptr); + CHECK(db->signal("ZeroWidth") == nullptr); +} + +TEST_CASE("Decode clamps an out-of-range bit_width instead of UB", "[ldf][REQ-LDF-009]") { + // Bypasses the text parser entirely (DB's storage members are public, + // populated by parse() but constructible directly by any caller) to + // exercise decode()'s own defense-in-depth clamp, independent of the + // parse-time rejection covered above. + DB db; + Signal sig; + sig.name = "Huge"; + sig.bit_width = 200; + db.signals_["Huge"] = sig; + + LDFFrame fr; + fr.name = "HugeFrame"; + fr.id = 0x31; + fr.signals.push_back(SignalRef{"Huge", 0}); + db.frames_[0x31] = fr; + + std::vector data(20, 0xFF); // > 8 bytes: reaches the clamp path + std::unordered_map result; + REQUIRE_NOTHROW(result = db.decode(0x31, data)); + REQUIRE(result.count("Huge") == 1); + // bit_width clamped to 64; with all-0xFF input every extracted bit is 1. + CHECK(result.at("Huge") == 0xFFFFFFFFFFFFFFFFULL); +}