From b149580c19299af115e5c7304780ac7117ef3a85 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Tue, 19 May 2026 23:13:19 +0200 Subject: [PATCH] fix(ding): clamp sample rate against int -> uint32 truncation CodeQL flagged the writeWAV cast of strconv.Atoi's result to uint32 (alert 148). Two-layer defence: the handler rejects sample-rate query params outside [8000, 192000] before parsing ever reaches Render, and WithDefaults snaps any out-of-range caller-supplied SampleRate back to the default before renderChirp allocates buffers sized by it. Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/service/ding/ding.go | 37 +++++++++++++++++++--- pkg/service/ding/ding_test.go | 35 ++++++++++++++++++++ pkg/service/handlers/handlers_ding.go | 16 ++++++++-- pkg/service/handlers/handlers_ding_test.go | 29 +++++++++++++++++ 4 files changed, 109 insertions(+), 8 deletions(-) diff --git a/pkg/service/ding/ding.go b/pkg/service/ding/ding.go index 2fec754..7a7dbaf 100644 --- a/pkg/service/ding/ding.go +++ b/pkg/service/ding/ding.go @@ -79,7 +79,7 @@ func DefaultOptions() Options { func (o Options) WithDefaults() Options { d := DefaultOptions() - if o.SampleRate <= 0 { + if o.SampleRate <= 0 || o.SampleRate > int(maxSampleRate) { o.SampleRate = d.SampleRate } @@ -151,7 +151,13 @@ func Render(opts Options) []byte { var buf bytes.Buffer - _ = writeWAV(&buf, left, right, opts.SampleRate) + // Defensive bound check: clamp before the conversion to + // uint32 so even a buggy caller (or one that bypassed the + // handler-side bound check on the query param) can't trigger + // integer truncation in the WAV header fields. + sampleRate32 := safeSampleRate(opts.SampleRate) + + _ = writeWAV(&buf, left, right, sampleRate32) return buf.Bytes() } @@ -225,7 +231,28 @@ const ( wavBitsPer = 16 ) -func writeWAV(w io.Writer, left, right []float64, sampleRate int) error { +// maxSampleRate is the largest sample rate writeWAV will accept +// before clamping. Generous enough to allow studio-quality 192 +// kHz; well below the uint32 ceiling the WAV header field can +// represent, and far below anything the byte-rate multiplication +// downstream could overflow. +const maxSampleRate uint32 = 192_000 + +// safeSampleRate converts the operator-supplied int sample rate +// into the uint32 the WAV header needs, clamping anything +// out-of-range to the default. Defence-in-depth: the +// handler-side sampleRateParam already rejects unreasonable +// inputs, but Render is exported so other callers (tests, +// scripts) could pass anything. +func safeSampleRate(in int) uint32 { + if in <= 0 || in > int(maxSampleRate) { + return uint32(DefaultOptions().SampleRate) + } + + return uint32(in) +} + +func writeWAV(w io.Writer, left, right []float64, sampleRate uint32) error { if len(left) != len(right) { return fmt.Errorf("channel length mismatch: %d vs %d", len(left), len(right)) } @@ -262,11 +289,11 @@ func writeWAV(w io.Writer, left, right []float64, sampleRate int) error { return err } - if err := binary.Write(w, binary.LittleEndian, uint32(sampleRate)); err != nil { + if err := binary.Write(w, binary.LittleEndian, sampleRate); err != nil { return err } - byteRate := uint32(sampleRate * wavChannels * (wavBitsPer / 8)) + byteRate := sampleRate * uint32(wavChannels) * uint32(wavBitsPer/8) if err := binary.Write(w, binary.LittleEndian, byteRate); err != nil { return err } diff --git a/pkg/service/ding/ding_test.go b/pkg/service/ding/ding_test.go index bcc2f3f..5f60d45 100644 --- a/pkg/service/ding/ding_test.go +++ b/pkg/service/ding/ding_test.go @@ -77,6 +77,41 @@ func TestRender_OverrideSampleRateChangesByteRate(t *testing.T) { } } +func TestSafeSampleRate_ClampsOutOfRange(t *testing.T) { + cases := []struct { + in int + want uint32 + }{ + {22050, 22050}, + {44100, 44100}, + {192000, 192000}, + {0, 22050}, // zero → default + {-1, 22050}, // negative → default + {200000, 22050}, // above max → default + {1 << 31, 22050}, // way beyond uint32 → default (the original CodeQL concern) + {1 << 33, 22050}, // wraps to a different value on int→uint32; default protects us + {int(maxSampleRate) + 1, 22050}, + } + + for _, c := range cases { + if got := safeSampleRate(c.in); got != c.want { + t.Errorf("safeSampleRate(%d) = %d, want %d", c.in, got, c.want) + } + } +} + +func TestRender_HugeSampleRateDoesNotTruncateOrPanic(t *testing.T) { + // Regression for the int→uint32 truncation CodeQL flagged. + // A caller (test, future SDK user) bypassing the handler's + // sampleRateParam guard with an int well above uint32 used + // to silently wrap. The defensive clamp now substitutes the + // default sample rate instead. + data := Render(Options{SampleRate: 1 << 33}.WithDefaults()) + if sr := binary.LittleEndian.Uint32(data[24:28]); sr != uint32(DefaultOptions().SampleRate) { + t.Errorf("expected clamp to default sample rate, got %d", sr) + } +} + func TestWithDefaults_FillsZeroFields(t *testing.T) { got := Options{PitchHigh: 1000}.WithDefaults() if got.PitchHigh != 1000 { diff --git a/pkg/service/handlers/handlers_ding.go b/pkg/service/handlers/handlers_ding.go index e1eb31f..2f1c092 100644 --- a/pkg/service/handlers/handlers_ding.go +++ b/pkg/service/handlers/handlers_ding.go @@ -114,7 +114,7 @@ func parseDingOptions(r *http.Request) (ding.Options, bool) { touched = true } - if v, ok := intParam(q.Get("sample-rate")); ok { + if v, ok := sampleRateParam(q.Get("sample-rate")); ok { opts.SampleRate = v touched = true } @@ -157,13 +157,23 @@ func millisecondsParam(raw string) (float64, bool) { return float64(v) / 1000.0, true } -func intParam(raw string) (int, bool) { +// dingMinSampleRate / dingMaxSampleRate gate the operator-supplied +// sample-rate against int→uint32 truncation and against values +// outside any realistic audio range. The upper bound is generous +// (192 kHz is studio-quality) but well below the uint32 ceiling +// the WAV header field can hold. +const ( + dingMinSampleRate = 8000 + dingMaxSampleRate = 192000 +) + +func sampleRateParam(raw string) (int, bool) { if raw == "" { return 0, false } v, err := strconv.Atoi(raw) - if err != nil || v <= 0 { + if err != nil || v < dingMinSampleRate || v > dingMaxSampleRate { return 0, false } diff --git a/pkg/service/handlers/handlers_ding_test.go b/pkg/service/handlers/handlers_ding_test.go index fed91ca..b8f9cb8 100644 --- a/pkg/service/handlers/handlers_ding_test.go +++ b/pkg/service/handlers/handlers_ding_test.go @@ -90,6 +90,35 @@ func TestHandleDing_InvalidParamFallsBackToDefault(t *testing.T) { } } +func TestHandleDing_OutOfRangeSampleRateFallsBackToDefault(t *testing.T) { + // CodeQL flagged the int→uint32 truncation; reject values + // outside the sane audio range at the parser, so neither + // the WAV header nor the int→uint32 cast can be tricked. + cases := []string{ + "1", // below dingMinSampleRate + "7999", // just under the floor + "500000", // above dingMaxSampleRate + "4294967300", // > uint32 — the truncation source + } + + ts := newDingTestServer(t) + + for _, sr := range cases { + t.Run(sr, func(t *testing.T) { + res, err := http.Get(ts.URL + "/media/aftertouch-ding.wav?sample-rate=" + sr) + if err != nil { + t.Fatalf("GET: %v", err) + } + defer res.Body.Close() + + body := readAll(t, res) + if got := binary.LittleEndian.Uint32(body[24:28]); got != 22050 { + t.Errorf("sample-rate=%s should clamp to default 22050, got %d", sr, got) + } + }) + } +} + func TestHandleDing_DefaultIsCached(t *testing.T) { // Reset the cache by simulating a fresh process. dingDefaultCache.once = sync.Once{}