From 8db16dba2f2d05e4c7cd1c26bfd287b10051465f Mon Sep 17 00:00:00 2001 From: Christopher Hicks Date: Fri, 11 Sep 2026 21:49:12 -0700 Subject: [PATCH 1/3] =?UTF-8?q?=E2=9C=85=20[main]=20add=20fuzz=20targets?= =?UTF-8?q?=20and=20fix=20negative-index=20panic=20in=20extractTimePart,?= =?UTF-8?q?=20fixes=20#47?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CLAUDE.md | 3 ++ main.go | 22 ++++----- main_test.go | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 23f5460..8b54673 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -95,6 +95,9 @@ dispatch. Dispatch tests drive the real `ffcli` tree via `newCommandTree` with `fakeConn` installed on `clockConfig.dial`; `dialClock` itself is tested against loopback UDP sockets (happy path, timeout, bad address). Coverage is ~83%. `-h` cannot be tested in-process because the flag sets use `flag.ExitOnError`. +Fuzz targets (`FuzzExtractTimePart`, `FuzzParseColorSpec`, `FuzzDisplayModeString`, +`FuzzStatusDecode`) run their seed corpora as ordinary tests under `go test ./...`; +fuzz them for real with e.g. `go test -fuzz FuzzExtractTimePart -fuzztime 30s`. ## Known Gaps diff --git a/main.go b/main.go index 1438022..5aeb200 100644 --- a/main.go +++ b/main.go @@ -333,17 +333,19 @@ func sendCommand(dial dialer, address string, timeout time.Duration, command str func extractTimePart(value string, part int) (uint8, error) { parts := strings.Split(value, ":") - if len(parts) > part { - n, err := strconv.Atoi(parts[part]) - if err != nil { - return 0, fmt.Errorf("parsing %q as a time component: %w", parts[part], err) - } - if n < 0 || n > 255 { - return 0, fmt.Errorf("time component %q out of range (must be 0-255)", parts[part]) - } - return uint8(n), nil + // a negative part index (or one past the last component) yields + // zero, mirroring the trailing-components-omitted behavior + if part < 0 || part >= len(parts) { + return uint8(0), nil + } + n, err := strconv.Atoi(parts[part]) + if err != nil { + return 0, fmt.Errorf("parsing %q as a time component: %w", parts[part], err) + } + if n < 0 || n > 255 { + return 0, fmt.Errorf("time component %q out of range (must be 0-255)", parts[part]) } - return uint8(0), nil + return uint8(n), nil } // parseHexColor turns one rrggbb hex color into its RGB components. diff --git a/main_test.go b/main_test.go index 25a777d..7d0be47 100644 --- a/main_test.go +++ b/main_test.go @@ -797,3 +797,127 @@ func TestDialClockBadAddress(t *testing.T) { t.Errorf("error %q does not mention the connection failure", err) } } + +// --- fuzz targets for the untrusted-input parsers --- +// +// These run their seed corpora as ordinary tests under `go test ./...`, +// which is how the CI exercises them. To actually fuzz (random inputs, +// minimizing engine, corpus growth in testdata/fuzz/), run e.g. +// +// go test -fuzz FuzzExtractTimePart -fuzztime 30s +// +// The targets exist partly for real hardening - every one of these +// functions parses bytes that originate off the wire or off argv - and +// partly so OpenSSF Scorecard's Fuzzing check detects the repo as +// fuzzed (it looks for `func FuzzXxx(*testing.F)` in *_test.go files). + +// FuzzExtractTimePart feeds arbitrary strings and part indexes to the +// time parser: no input may panic, and any parsed value must stay in +// the 0-255 range a uint8 component can hold. Negative part indexes +// used to panic here - the guard in extractTimePart was added after +// this target found it. +func FuzzExtractTimePart(f *testing.F) { + seeds := []struct { + value string + part int + }{ + {"1:2:3:4:5", 0}, + {"1:2:3:4:5", 4}, + {"0:30", 1}, + {"0:30", 2}, + {"255:0:0", 0}, + {"256:0:0", 0}, + {"1:2:-1", 2}, + {"1:2:x", 2}, + {"", 0}, + {"1:2:3:4:5", -1}, + {"1:2:3:4:5", 99}, + } + for _, s := range seeds { + f.Add(s.value, s.part) + } + f.Fuzz(func(t *testing.T, value string, part int) { + got, err := extractTimePart(value, part) + if err != nil { + // erroring is fine; a nonzero value alongside an + // error would mislead the caller + if got != 0 { + t.Fatalf("extractTimePart(%q, %d) = %d with error %v", value, part, got, err) + } + return + } + if got > 255 { + t.Fatalf("extractTimePart(%q, %d) = %d, want 0-255", value, part, got) + } + }) +} + +// FuzzParseColorSpec feeds arbitrary color specs to the parser: no +// input may panic, and a successful parse must yield exactly the +// MM:SS and HH triples the syntax describes +func FuzzParseColorSpec(f *testing.F) { + for _, seed := range []string{ + "ff0000", + "FFAA00", + "ff0000:00ff00", + "00ff00:ff0000", + "ff00", + "ff00000", + "gg0000", + "ff0000:00ff00:0000ff", + "ff0000:0", + "", + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, spec string) { + mmss, hh, err := parseColorSpec(spec) + if err != nil { + return + } + // a single color must apply to both digit groups + if strings.Count(spec, ":") == 0 { + if mmss != hh { + t.Fatalf("parseColorSpec(%q) single color gave mmss %+v != hh %+v", spec, mmss, hh) + } + } + }) +} + +// FuzzDisplayModeString feeds arbitrary mode bytes to the display-mode +// decoder; every byte is valid input for the real clock, so the only +// invariant is that decoding never panics +func FuzzDisplayModeString(f *testing.F) { + for _, seed := range []uint8{0x00, 0x01, 0x41, 0x02, 0x03, 0x04, 0x07, 0xC1, 0x61, 0xFF} { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, mode uint8) { + _ = displayModeString(mode) + }) +} + +// FuzzStatusDecode feeds arbitrary byte packets to the API 1.x and 2.0 +// decoders, exercising the paths the status subcommand takes when a +// clock (or something spoofing one) sends a malformed or hostile +// response: a 35-byte packet must decode as API 1.x, a 40-byte one as +// API 2.0, and neither may panic along the way +func FuzzStatusDecode(f *testing.F) { + f.Add(r10Bytes()) + f.Add(make([]byte, 40)) + f.Add([]byte{}) + f.Add(make([]byte, 37)) + f.Fuzz(func(t *testing.T, packet []byte) { + switch len(packet) { + case api1PacketSize: + var r10 Response10 + if err := binary.Read(bytes.NewReader(packet), binary.BigEndian, &r10); err != nil { + t.Fatalf("decoding %d-byte packet as API 1.x: %v", len(packet), err) + } + case api2PacketSize: + var r20 Response20 + if err := binary.Read(bytes.NewReader(packet), binary.BigEndian, &r20); err != nil { + t.Fatalf("decoding %d-byte packet as API 2.0: %v", len(packet), err) + } + } + }) +} From a2419acec119f671cb287b094a47473c2af61464 Mon Sep 17 00:00:00 2001 From: Christopher Hicks Date: Fri, 11 Sep 2026 21:49:12 -0700 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20[gha]=20add=20CodeQ?= =?UTF-8?q?L=20SAST=20workflow=20for=20Go=20analysis,=20fixes=20#47?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/workflows/codeql.yml | 46 ++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 .github/workflows/codeql.yml diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 0000000..a71aa5e --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,46 @@ +--- +name: CodeQL + +on: + push: + branches: + - main + pull_request: + +# global permissions +permissions: {} + +jobs: + analyze: + runs-on: ubuntu-latest + permissions: + contents: read + security-events: write # for github/codeql-action to upload SARIF results + timeout-minutes: 20 + steps: + - name: Harden the runner (Audit all outbound calls) + uses: step-security/harden-runner@e14015d583714f6e62063499dc959a02595150a1 # v2.21.1 + with: + egress-policy: audit + + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Setup Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + + - name: Initialize CodeQL + uses: github/codeql-action/init@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9 + with: + languages: go + + # manual build instead of autobuild so the analyzed build matches + # what go-ci.yml ships (and the pinned Go from go.mod) + - name: Build + run: go build ./... + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9 \ No newline at end of file From eca19e63d7d21ab12204278973c6b3d4ba8077f4 Mon Sep 17 00:00:00 2001 From: Christopher Hicks Date: Fri, 11 Sep 2026 21:52:16 -0700 Subject: [PATCH 3/3] =?UTF-8?q?=E2=9C=85=20[main]=20strengthen=20FuzzParse?= =?UTF-8?q?ColorSpec=20two-color=20invariant=20per=20review,=20fixes=20#47?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/workflows/codeql.yml | 2 +- main_test.go | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index a71aa5e..2d5bc15 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -43,4 +43,4 @@ jobs: run: go build ./... - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9 \ No newline at end of file + uses: github/codeql-action/analyze@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9 diff --git a/main_test.go b/main_test.go index 7d0be47..d8a9aa8 100644 --- a/main_test.go +++ b/main_test.go @@ -880,6 +880,16 @@ func FuzzParseColorSpec(f *testing.F) { if mmss != hh { t.Fatalf("parseColorSpec(%q) single color gave mmss %+v != hh %+v", spec, mmss, hh) } + return + } + // a valid two-color spec must decode the halves independently + spec2, spec3, _ := strings.Cut(spec, ":") + left, errLeft := parseHexColor(spec2) + right, errRight := parseHexColor(spec3) + if errLeft == nil && errRight == nil { + if mmss != left || hh != right { + t.Fatalf("parseColorSpec(%q) = %+v/%+v, want %+v/%+v", spec, mmss, hh, left, right) + } } }) }