diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f3e2e55..3dee859 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,9 +21,9 @@ jobs: go: ["1.25", "1.26"] steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version: ${{ matrix.go }} @@ -36,7 +36,7 @@ jobs: - name: Upload coverage if: matrix.os == 'ubuntu-latest' && matrix.go == '1.25' - uses: actions/upload-artifact@v6 + uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6 with: name: coverage path: coverage.out @@ -48,9 +48,9 @@ jobs: name: Coverage floor (library packages >= 85%) runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version: "1.25" @@ -79,9 +79,9 @@ jobs: name: Benchmark smoke (ubuntu-latest) runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version: "1.25" @@ -103,9 +103,9 @@ jobs: name: Fuzz (short, ubuntu-latest) runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version: "1.25" @@ -129,9 +129,9 @@ jobs: name: RELAY conform (--strict) runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version: "1.25" @@ -151,9 +151,9 @@ jobs: name: RELAY interop (LIN) runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version: "1.25" @@ -177,9 +177,9 @@ jobs: name: go-FuSa full lifecycle (v0.48.0) runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version: "1.25" @@ -260,7 +260,7 @@ jobs: - name: Upload safety evidence if: always() - uses: actions/upload-artifact@v6 + uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6 with: name: gofusa-safety-evidence path: gofusa-audit-pack.zip @@ -275,9 +275,9 @@ jobs: name: Standards compliance (ISO / IEC / DO / UNECE / SLSA) runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version: "1.25" diff --git a/.github/workflows/dco.yml b/.github/workflows/dco.yml index 3a3edf8..ce3fa00 100644 --- a/.github/workflows/dco.yml +++ b/.github/workflows/dco.yml @@ -9,7 +9,7 @@ jobs: name: Developer Certificate of Origin runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 with: fetch-depth: 0 diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index e747a22..4f3eeb9 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -25,16 +25,16 @@ jobs: target: [go-lin, quickstart, lintool] steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 - name: Set up QEMU (multi-arch) - uses: docker/setup-qemu-action@v3 + uses: docker/setup-qemu-action@c7c53464625b32c7a7e944ae62b3e17d2b600130 # v3 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v3 + uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3 - name: Log in to GHCR - uses: docker/login-action@v3 + uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3 with: registry: ${{ env.REGISTRY }} username: ${{ github.actor }} @@ -54,7 +54,7 @@ jobs: - name: Extract metadata id: meta - uses: docker/metadata-action@v5 + uses: docker/metadata-action@c299e40c65443455700f0fdfc63efafe5b349051 # v5 with: images: ${{ steps.image.outputs.name }} tags: | @@ -64,7 +64,7 @@ jobs: type=sha,prefix=sha-,format=short - name: Build and push - uses: docker/build-push-action@v6 + uses: docker/build-push-action@10e90e3645eae34f1e60eeb005ba3a3d33f178e8 # v6 with: context: . file: docker/Dockerfile diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a380f8b..2dc5245 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -16,12 +16,12 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 with: fetch-depth: 0 ref: main - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version: "1.25" diff --git a/CHANGELOG.md b/CHANGELOG.md index 4904ae9..33b49df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,38 @@ canonical list. Dates are release dates (UTC-7, matching tag creation). ## [Unreleased] +- fix: `ldf.parseFrameHeader` now rejects (rather than silently corrupting) + a frame whose ID is outside 0x00–0x3F or whose declared length is + outside 0–8 bytes — previously a negative length (e.g. from + `f: 0x10, MASTER, -4;`) reached `DB.Encode`'s `make([]byte, f.Length)` + and panicked (`runtime error: makeslice: len out of range`, CWE-789 DoS + on untrusted LDF input), and an out-of-range ID (e.g. `300`) was + silently truncated via a bare `uint8()` cast, corrupting whatever frame + already lived at the truncated ID. A frame rejected this way is now + properly skipped rather than swallowing every subsequent frame in the + same `Frames` section (the previous single-`continue` mistook the + rejected frame's own closing brace for the section's closing brace). + Signal-ref bit offsets with a negative value (also previously discarded + the parse error) are now rejected the same way rather than relying on + incidental Go shift/comparison semantics to avoid a panic (#76) +- fix(safety): `safety.Receiver.Unwrap` now compares the wire-transmitted + `DataID`/`SourceID` against the receiver's configured `Config` and + returns a new `ErrIDMismatch` on mismatch — previously the CRC check + alone was (incorrectly, per the code's own now-corrected comment) + treated as sufficient masquerade protection, so a frame protected under + a different `DataID`/`SourceID` than the receiver's was accepted + without error (#76) +- fix(virtual): `Bus.Publish`/`Bus.PublishClassic` now reject a non-nil, + zero-length payload the same way they already reject an over-length + one, so the virtual bus can never broadcast a 0-data-byte frame that + `lin.ValidateFrame` itself would consider malformed (LIN Specification + Package 2.2A: the data field carries 1–8 bytes); `PublishClassic` also + gained the `LINMaxDataLen` over-length guard `Publish` already had (#76) +- chore(ci): pinned all third-party and first-party GitHub Actions in + `.github/workflows/` to immutable commit SHAs (with a `# vX` comment for + readability) instead of mutable version tags, matching the repo's own + SLSA/supply-chain evidence posture (#76) + ## [1.5.0] — 2026-07-30 - chore: bump `github.com/SoundMatt/RELAY` v1.11.0 → `github.com/SoundMatt/RELAY/v2` diff --git a/ldf/parser.go b/ldf/parser.go index 711b03a..cbad841 100644 --- a/ldf/parser.go +++ b/ldf/parser.go @@ -441,6 +441,15 @@ func (p *ldfParser) parseFrames(db *DB) error { p.next() fr, err := parseFrameHeader(line) if err != nil { + // Skip past this frame's body (up to and including its + // closing brace) so a single invalid frame header does not + // swallow subsequently well-formed frames or get mistaken + // for the closing brace of the whole Frames section. + for p.pos < len(p.lines) { + if p.next() == "}" { + break + } + } continue } // parse signal refs @@ -456,7 +465,14 @@ func (p *ldfParser) parseFrames(db *DB) error { parts := strings.SplitN(inner, ",", 2) if len(parts) == 2 { sigName := strings.TrimSpace(parts[0]) - offset, _ := parseInt(strings.TrimSpace(parts[1])) + offset, err := parseInt(strings.TrimSpace(parts[1])) + if err != nil || offset < 0 { + // Malformed or negative bit offset: skip this signal + // ref rather than let extractBits/packBits receive a + // negative offset (would rely on incidental Go shift/ + // comparison semantics to avoid a panic). + continue + } fr.Signals = append(fr.Signals, SignalRef{Name: sigName, BitOffset: int(offset)}) } } @@ -486,8 +502,14 @@ func parseFrameHeader(line string) (*Frame, error) { if err != nil { return nil, fmt.Errorf("ldf: invalid frame ID in %q: %w", line, err) } + if id < 0 || id > int64(lin.LINMaxID) { + return nil, fmt.Errorf("ldf: frame ID %d out of range 0-%d in %q", id, lin.LINMaxID, line) + } publisher := strings.TrimSpace(parts[1]) - length, _ := parseInt(strings.TrimSpace(parts[2])) + length, err := parseInt(strings.TrimSpace(parts[2])) + if err != nil || length < 0 || length > int64(lin.LINMaxDataLen) { + return nil, fmt.Errorf("ldf: frame length out of range 0-%d in %q", lin.LINMaxDataLen, line) + } return &Frame{ Name: name, ID: uint8(id), diff --git a/ldf/parser_test.go b/ldf/parser_test.go index 10e45a1..018d766 100644 --- a/ldf/parser_test.go +++ b/ldf/parser_test.go @@ -339,6 +339,131 @@ func TestFrames_defensiveCopy(t *testing.T) { } } +// ── go-LIN-01: frame ID/length range validation ────────────────────────────── +// +// Regression coverage for a bug where parseFrameHeader discarded the +// length-parse error and never range-checked ID or length. A frame with a +// negative length made DB.Encode panic via make([]byte, f.Length); an +// out-of-range ID (e.g. 300) was silently truncated to a valid-looking 6-bit +// ID via uint8(id), corrupting the frame table under the wrong key. + +//fusa:test REQ-LDF-005 +//fusa:test REQ-SEC-001 + +func TestParse_rejectsNegativeFrameLength(t *testing.T) { + const input = ` +Frames { + BadFrame: 0x10, MasterNode, -4 { + EngineSpeed, 0; + } +} +` + db, err := ldf.Parse(strings.NewReader(input)) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if f := db.Frame(0x10); f != nil { + t.Fatalf("Frame(0x10) = %+v, want nil (frame with negative length must be rejected, not stored)", f) + } + // The original bug manifested here: make([]byte, f.Length) with a + // negative Length panics. Encode on the rejected ID must simply report + // "unknown frame" (nil), never panic. + if out := db.Encode(0x10, nil); out != nil { + t.Fatalf("Encode(0x10) = % X, want nil for a rejected frame", out) + } +} + +func TestParse_rejectsOutOfRangeFrameID(t *testing.T) { + const input = ` +Frames { + BadFrame: 300, MasterNode, 4 { + EngineSpeed, 0; + } +} +` + db, err := ldf.Parse(strings.NewReader(input)) + if err != nil { + t.Fatalf("Parse: %v", err) + } + // 300 truncated via a bare uint8() cast wraps to 300-256=44 (0x2C). The + // fix must reject the frame outright, not store it under the truncated + // ID, silently corrupting whatever legitimate frame lives at 0x2C. + if f := db.Frame(0x2C); f != nil { + t.Fatalf("Frame(0x2C) = %+v, want nil (out-of-range ID must not be silently truncated into the table)", f) + } + if len(db.Frames()) != 0 { + t.Fatalf("Frames() = %d entries, want 0", len(db.Frames())) + } +} + +// A frame rejected for an out-of-range ID/length must not swallow +// subsequently well-formed frames in the same Frames section — the parser +// must skip past the bad frame's body, not mistake its closing brace for the +// Frames section's own closing brace. +func TestParse_badFrameDoesNotSwallowLaterFrames(t *testing.T) { + const input = ` +Frames { + BadFrame: 300, MasterNode, 4 { + EngineSpeed, 0; + } + GoodFrame: 0x20, SlaveB, 2 { + WindowPos, 0; + } +} +` + db, err := ldf.Parse(strings.NewReader(input)) + if err != nil { + t.Fatalf("Parse: %v", err) + } + f := db.Frame(0x20) + if f == nil { + t.Fatal("Frame(0x20) = nil, want GoodFrame to still be parsed after a preceding invalid frame") + } + if f.Name != "GoodFrame" { + t.Errorf("frame Name = %q, want GoodFrame", f.Name) + } +} + +// ── go-LIN-A3: negative signal-ref bit offset ──────────────────────────────── +// +// Regression coverage for a bug where the bit offset in a frame's signal +// reference was parsed with the error discarded, allowing a negative +// BitOffset to reach extractBits/packBits and rely on incidental Go +// shift/comparison semantics to avoid a panic rather than being rejected by +// design. + +//fusa:test REQ-LDF-006 + +func TestParse_rejectsNegativeSignalBitOffset(t *testing.T) { + const input = ` +Signals { + EngineSpeed: 16, 0x0000, MasterNode, SlaveA; +} +Frames { + EngineFrame: 0x10, MasterNode, 4 { + EngineSpeed, -8; + } +} +` + db, err := ldf.Parse(strings.NewReader(input)) + if err != nil { + t.Fatalf("Parse: %v", err) + } + f := db.Frame(0x10) + if f == nil { + t.Fatal("Frame(0x10) = nil, want the frame itself to still be parsed") + } + for _, ref := range f.Signals { + if ref.Name == "EngineSpeed" { + t.Fatalf("signal ref %+v: negative BitOffset must be rejected, not stored", ref) + } + } + // Encode/Decode must remain panic-safe even though the signal ref was + // dropped (it simply won't appear in the packed/decoded output). + _ = db.Encode(0x10, map[string]uint64{"EngineSpeed": 1}) + _ = db.Decode(0x10, []byte{0, 0, 0, 0}) +} + func FuzzParse(f *testing.F) { f.Add(sampleLDF) f.Add("") @@ -354,6 +479,12 @@ func FuzzParse(f *testing.F) { _ = db.Signals() for id := uint8(0); id <= 0x3F; id++ { _ = db.Frame(id) + // Regression for go-LIN-01: a structurally-valid-but-malicious + // LDF (e.g. a negative or oversize frame length) must never make + // DB.Encode panic (makeslice with a negative/huge length) or + // over-allocate. Every frame Parse actually accepted must be + // safe to Encode. + _ = db.Encode(id, nil) } }) } diff --git a/safety/e2e.go b/safety/e2e.go index 076ca6b..f58dfeb 100644 --- a/safety/e2e.go +++ b/safety/e2e.go @@ -69,6 +69,9 @@ const ( ErrSequenceGap // ErrHeaderTooShort means the payload is shorter than the 10-byte header. ErrHeaderTooShort + // ErrIDMismatch means the transmitted DataID/SourceID did not match the + // Receiver's configured identity (masquerade / mis-addressing detection). + ErrIDMismatch ) // E2EError is returned when an E2E safety check fails. @@ -191,8 +194,19 @@ func (r *Receiver) Unwrap(data []byte) ([]byte, error) { } } - _ = dataID // validated implicitly via CRC - _ = sourceID // validated implicitly via CRC + // Masquerade / mis-addressing detection: the transmitted DataID and + // SourceID MUST match this Receiver's configured identity. The CRC only + // proves the header is self-consistent; it does not bind the frame to the + // expected logical stream, so this check is what actually protects against + // a frame from a different DataID/SourceID being accepted. + if dataID != r.cfg.DataID || sourceID != r.cfg.SourceID { + return nil, &E2EError{ + Kind: ErrIDMismatch, + Counter: seq, + Message: fmt.Sprintf("identity mismatch: wire DataID=0x%04X SourceID=0x%04X, want DataID=0x%04X SourceID=0x%04X", + dataID, sourceID, r.cfg.DataID, r.cfg.SourceID), + } + } r.mu.Lock() defer r.mu.Unlock() diff --git a/safety/e2e_test.go b/safety/e2e_test.go index 79452f4..e374f1e 100644 --- a/safety/e2e_test.go +++ b/safety/e2e_test.go @@ -210,6 +210,61 @@ func TestUnwrap_sequenceGap(t *testing.T) { } } +// ── go-LIN-02: masquerade / mis-addressing detection (ErrIDMismatch) ───────── +// +// Regression coverage for a bug where Receiver.Unwrap never compared the +// wire-transmitted DataID/SourceID against its own configured Config: the +// CRC only proved the header was self-consistent, not that it belonged to +// the expected logical stream, so a frame protected under a different +// DataID/SourceID than the receiver's was silently accepted. + +//fusa:test REQ-SAFETY-001 +//fusa:test REQ-SAFETY-002 +//fusa:test REQ-SEC-002 + +func TestUnwrap_rejectsDataIDMismatch(t *testing.T) { + // Protected under a different DataID than the receiver expects, but + // otherwise perfectly well-formed (correct CRC, correct sequence). + wrongDataID := safety.Config{DataID: cfg.DataID + 1, SourceID: cfg.SourceID} + p := safety.NewProtector(wrongDataID) + r := safety.NewReceiver(cfg) + + protected := p.Protect([]byte{0x01, 0x02}) + _, err := r.Unwrap(protected) + if err == nil { + t.Fatal("expected E2E error for DataID masquerade, got nil") + } + var e2e *safety.E2EError + if !errors.As(err, &e2e) { + t.Fatalf("expected *E2EError, got %T", err) + } + if e2e.Kind != safety.ErrIDMismatch { + t.Errorf("ErrorKind = %v, want ErrIDMismatch", e2e.Kind) + } +} + +func TestUnwrap_rejectsSourceIDMismatch(t *testing.T) { + // Protected under a different SourceID than the receiver expects — same + // masquerade class, different field, both of which the CRC alone does + // not bind to the receiver's expected identity. + wrongSourceID := safety.Config{DataID: cfg.DataID, SourceID: cfg.SourceID + 1} + p := safety.NewProtector(wrongSourceID) + r := safety.NewReceiver(cfg) + + protected := p.Protect([]byte{0x01, 0x02}) + _, err := r.Unwrap(protected) + if err == nil { + t.Fatal("expected E2E error for SourceID masquerade, got nil") + } + var e2e *safety.E2EError + if !errors.As(err, &e2e) { + t.Fatalf("expected *E2EError, got %T", err) + } + if e2e.Kind != safety.ErrIDMismatch { + t.Errorf("ErrorKind = %v, want ErrIDMismatch", e2e.Kind) + } +} + // ── REQ-SAFETY-010: Unwrap returns original payload ────────────────────────── //fusa:test REQ-SAFETY-010 diff --git a/virtual/bus.go b/virtual/bus.go index b9f8264..f361cad 100644 --- a/virtual/bus.go +++ b/virtual/bus.go @@ -100,6 +100,9 @@ func (b *Bus) Publish(id uint8, data []byte) error { if id > lin.LINMaxID { return fmt.Errorf("lin/virtual: frame ID 0x%02X exceeds maximum 0x%02X", id, lin.LINMaxID) } + if data != nil && len(data) == 0 { + return fmt.Errorf("lin/virtual: payload must not be empty: %w", lin.ErrInvalidFrame) + } if len(data) > lin.LINMaxDataLen { return fmt.Errorf("lin/virtual: payload length %d exceeds maximum %d: %w", len(data), lin.LINMaxDataLen, lin.ErrPayloadTooLarge) } @@ -124,6 +127,12 @@ func (b *Bus) PublishClassic(id uint8, data []byte) error { if id > lin.LINMaxID { return fmt.Errorf("lin/virtual: frame ID 0x%02X exceeds maximum 0x%02X", id, lin.LINMaxID) } + if data != nil && len(data) == 0 { + return fmt.Errorf("lin/virtual: payload must not be empty: %w", lin.ErrInvalidFrame) + } + if len(data) > lin.LINMaxDataLen { + return fmt.Errorf("lin/virtual: payload length %d exceeds maximum %d: %w", len(data), lin.LINMaxDataLen, lin.ErrPayloadTooLarge) + } b.mu.Lock() defer b.mu.Unlock() if b.closed { diff --git a/virtual/bus_test.go b/virtual/bus_test.go index 6ded7b0..c366b5a 100644 --- a/virtual/bus_test.go +++ b/virtual/bus_test.go @@ -92,6 +92,41 @@ func TestPublish_rejectsHighID(t *testing.T) { } } +// TestPublish_rejectsEmptyPayload is the regression test for go-LIN-A2: the +// virtual bus must never broadcast a 0-data-byte frame (LIN Specification +// Package 2.2A: the data field carries 1-8 bytes), matching what +// lin.ValidateFrame itself already rejects. Publish(id, nil) removes a +// registration and must remain accepted; Publish(id, []byte{}) — non-nil, +// zero-length — must be rejected the same way over-length data already is. +func TestPublish_rejectsEmptyPayload(t *testing.T) { + b, err := virtual.New() + if err != nil { + t.Fatal(err) + } + defer b.Close() + if err := b.Publish(0x10, []byte{}); err == nil { + t.Error("expected error for non-nil empty payload") + } + // nil must still be accepted (it removes any existing registration). + if err := b.Publish(0x10, nil); err != nil { + t.Errorf("Publish(id, nil) should still succeed, got %v", err) + } +} + +func TestPublishClassic_rejectsEmptyPayload(t *testing.T) { + b, err := virtual.New() + if err != nil { + t.Fatal(err) + } + defer b.Close() + if err := b.PublishClassic(0x10, []byte{}); err == nil { + t.Error("expected error for non-nil empty payload") + } + if err := b.PublishClassic(0x10, nil); err != nil { + t.Errorf("PublishClassic(id, nil) should still succeed, got %v", err) + } +} + func TestPublish_afterClose(t *testing.T) { b, err := virtual.New() if err != nil {