fix: bound TDS response parser allocations against malformed streams - #421
Open
Saurabh Singh (saurabh500) wants to merge 63 commits into
Open
Saurabh Singh (saurabh500) wants to merge 63 commits into
Saurabh Singh (saurabh500) wants to merge 63 commits into
Conversation
Add token_fuzz_test.go with reusable helpers to frame arbitrary token streams into one or more packReply TDS packets (with configurable fragmentation), build a tdsSession reading from the framed bytes, and run processSingleResponse while fully draining its buffered token channel. Add FuzzProcessSingleResponse seeded with valid synthetic responses (DONE, COLMETADATA/ROW, NBCROW, multiple result sets, ERROR, INFO, RETURNSTATUS, DONEPROC/DONEINPROC, ENVCHANGE, TABNAME/COLINFO/ORDER) and malformed inputs (unknown token, truncated token, trailing garbage). Add TestProcessSingleResponsePacketBoundary asserting the parser's token sequence is independent of packet fragmentation for the valid seeds. Refs #418 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…eams Several TDS token parsers allocated buffers and slices sized directly from attacker-controlled length prefixes before validating them against the available data, so a malformed server response could drive an unbounded allocation and OOM the client (a DoS). The stream recover() does not catch OOM because it is uncontrolled allocation, not a panic. Bound the wire-derived allocation sizes and reject underflows via badStreamPanic so a malformed stream fails cleanly as an error token: - parseFedAuthInfo: cap the token size and reject an option count that cannot fit within it (fixes the EE 00*8 ~4GB underflow repro). - readLongLenType: reject negative/oversized TEXT/NTEXT/IMAGE lengths. - readVariantTypeWithEncoding: reject underflowed/oversized data lengths. Add regression unit tests plus permanent fuzz seeds for the crafted inputs. Fixes #420 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
==========================================
+ Coverage 82.13% 82.70% +0.57%
==========================================
Files 35 35
Lines 7065 7148 +83
==========================================
+ Hits 5803 5912 +109
+ Misses 995 974 -21
+ Partials 267 262 -5
🚀 New features to boost your workflow:
|
This was referenced Aug 14, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
August 14, 2026 21:47
View session
- Correct frameReplyPackets doc: frag is a target fragment count (1..8), streams exceeding the per-packet limit split into additional packets. - Fix stale doneBody helper comment (was buildDone). - Set a spec-faithful Length field on ERROR/INFO seed tokens. - Restrict the packet-boundary determinism test to known-valid seeds by splitting seeds into validResponseSeeds/malformedResponseSeeds. - Skip per-token fmt.Sprintf allocations in the fuzz hot path via a collect flag on drainSingleResponse. Refs #418 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c3e268ca-c6f4-4edb-826c-3a4018cf939c
There was a problem hiding this comment.
Pull request overview
This PR hardens the TDS response parser against OOM/DoS scenarios by bounding allocations that are derived from server-controlled length prefixes, and adds targeted regression tests + fuzz seeds to keep these cases pinned.
Changes:
- Add size/count validation in
parseFedAuthInfoto prevent underflow/oversized allocations. - Add validation for
TEXT/NTEXT/IMAGEandsql_variantlength-derived allocations intypes.go. - Update the end-to-end fuzz target documentation and add permanent regression seeds plus a dedicated regression test file.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| types.go | Adds bounds/underflow checks for long-length types and sql_variant before allocating. |
| token.go | Caps FEDAUTHINFO token size and validates option count fits within advertised token size before allocating. |
| token_fuzz_test.go | Updates fuzz-target note and adds regression seeds for previously OOM-inducing malformed streams. |
| token_alloc_regression_test.go | Adds direct + end-to-end regression tests for malformed-length allocation issues. |
Suppressed comments (2)
token.go:528
- Same issue as above: badStreamPanicf produces a plain error, so this malformed-stream case won’t be classified as StreamError and may not poison the connection for pooling/retry logic. Use badStreamPanic(fmt.Errorf(...)) to ensure StreamError.
if uint64(count)*9+uint64(offset) > uint64(size) {
badStreamPanicf("federated authentication info advertised %d options that do not fit in %d bytes", count, size)
}
types.go:669
- Same StreamError classification concern: badStreamPanicf panics with a plain error, so malformed sql_variant data may not mark the connection as bad. Use badStreamPanic(fmt.Errorf(...)) so callers see a StreamError.
// size-2-propbytes is the trailing data length and is used below as an
// allocation size. It is derived from an attacker-controlled size prefix,
// so reject an underflowed (negative) or implausibly large value before any
// make() to avoid an OOM DoS (issue #420).
if datalen := size - 2 - propbytes; datalen < 0 || int64(datalen) > _MAX_PLP_LEN {
badStreamPanicf("sql_variant data length %d is invalid", datalen)
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Convert the issue #420 allocation guards from badStreamPanicf (plain error) to badStreamPanic(fmt.Errorf(...)) so a malformed stream surfaces as a StreamError. Conn.checkBadConn only marks the connection bad for StreamError, so a plain-error panic would leave the poisoned connection in the pool. Also assert the guards panic StreamError in the regression tests. Addresses Copilot review feedback on PR #421. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
August 14, 2026 21:56
View session
…ness' into saurabh500-bound-tds-response-allocations
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
token_fuzz_test.go:287
- This NOTE claims parseColMetadata72’s column-count allocation is “now bounded”, but this PR doesn’t change parseColMetadata72 (it still allocates
make([]columnStruct, count)from the wireuint16). Consider tightening the wording to only describe the allocation sites actually bounded by this PR to avoid misleading future readers.
single, _, ok := drainSingleResponse(seed, 0, true) // one packet
if !ok {
t.Fatal("failed to frame seed as a single packet")
}
for _, frag := range []byte{1, 3, 7, 255} {
token_alloc_regression_test.go:42
- The StreamError assertion is correct for the new allocation guards added for issue #420, but the comment currently reads like a general guarantee for all malformed-stream failures. Consider narrowing it to the specific intent of these tests (verifying the new guards panic StreamError so checkBadConn drops the connection).
// assertStreamError fails unless err is a StreamError. The allocation guards
// must panic StreamError (not a plain error) so Conn.checkBadConn marks the
// connection bad and drops it from the pool on a malformed stream.
token_alloc_regression_test.go:25
- The comment for recoverErr is a bit misleading: not all malformed-stream panics come from badStreamPanic (StreamError), and this helper captures panics directly rather than via processSingleResponse. Tweaking the wording here will make it clearer what’s being asserted in these tests.
This issue also appears on line 40 of the same file.
// recoverErr runs fn and returns any panic value coerced to an error. Token
// parsers signal a malformed stream by panicking (badStreamPanic) which
// processSingleResponse recovers into an error token.
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
August 14, 2026 22:01
View session
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
types.go:667
readVariantTypeWithEncodingstill allows an attacker-controlledsizeto be as large as ~2GiB (since the guard uses_MAX_PLP_LEN). Forsql_variant, SQL Server values have a much smaller maximum storage size (8016 bytes), so a malformed stream can still trigger a multi‑GiBmake([]byte, size-2-propbytes)allocation beforeReadFullfails, leading to OOM.
// size-2-propbytes is the trailing data length and is used below as an
// allocation size. It is derived from an attacker-controlled size prefix,
// so reject an underflowed (negative) or implausibly large value before any
// make() to avoid an OOM DoS (issue #420).
if datalen := size - 2 - propbytes; datalen < 0 || int64(datalen) > _MAX_PLP_LEN {
A sql_variant is capped at 8016 bytes on SQL Server and is never a (max)/ LOB type, so bounding its data length at _MAX_PLP_LEN (~2 GiB) still let an attacker-controlled size prefix drive a multi-gigabyte make(). Introduce _MAX_VARIANT_LEN and reject any larger length, and cover the oversize case in the regression test. Addresses Copilot review feedback on PR #421 (issue #420). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
August 14, 2026 22:10
View session
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
types.go:589
- The guard covers both negative lengths and lengths exceeding the LOB maximum, but the error text currently reads as though every failure is an “exceeds the maximum” case. For negative sizes this is misleading and makes diagnosing malformed streams harder. Consider a message that states the value is invalid/out of range while still mentioning the maximum LOB size.
// The advertised size is attacker-controlled; reject anything a real server
// cannot produce before using it as an allocation size (OOM DoS, issue #420).
if size < 0 || int64(size) > _MAX_PLP_LEN {
badStreamPanic(fmt.Errorf("TEXT/NTEXT/IMAGE length %d exceeds the maximum LOB size of %d bytes", size, int64(_MAX_PLP_LEN)))
}
Benchmark Results (main vs PR)Click to expand benchstat outputGenerated by CI — commit 60232fd |
Address Copilot review suggestions on the TDS fuzz harness: - Add TestProcessSingleResponseMalformedSeeds so a regression that silently accepts an unknown or truncated token is caught, instead of relying only on the fuzz target (which checks for panics) and the boundary test (which excludes malformed seeds). - Assert in TestProcessSingleResponsePacketBoundary that valid seeds never produce an error token, so a seed that fails identically for every fragmentation cannot pass the determinism comparison unnoticed. Refs #418 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c3e268ca-c6f4-4edb-826c-3a4018cf939c
Address Copilot review suggestions on the TDS fuzz harness: - frameReplyPackets now takes an explicit per-packet payload size instead of a fixed 1..8 fragment count, so callers can place a packet seam at any byte offset. The fuzz body derives the chunk size from the fuzzed byte, and TestProcessSingleResponsePacketBoundary now enumerates every byte boundary for each valid seed rather than four fixed splits. - Give the ERROR seed's terminating DONE the doneError status bit so it is a protocol-faithful error response per MS-TDS rather than an orphaned error. Refs #418 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c3e268ca-c6f4-4edb-826c-3a4018cf939c
Integrate upstream response-draining and connection-cleanup changes from #410 while preserving all TDS allocation fixes and regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
September 11, 2026 06:03
View session
Collaborator
|
No new findings at |
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
September 23, 2026 02:14
View session
Integrate the upstream release version, manifest, and changelog from #456 without changing the TDS allocation fixes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The fixed-width sql_variant payload size remains insufficiently validated, allowing malformed data to desynchronize parsing.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Reject invalid property counts, fixed and scale-dependent payload widths, decimal lengths, and undersized headers as StreamError before reading outside a variant. Preserve valid values across consecutive reads and packet boundaries, and add ROW, NBCROW, return-value and fuzz regressions. Fixes #420 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Address QF1003 in the new date/time variant width checks and their regression fixtures without changing behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved result-column, CEK allocation, and malformed UTF-16 handling issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
Resolved since last review (1)
Reject incomplete UTF-16 code units before reading variant properties or payloads so malformed results retire the connection. Cover both Unicode types, empty and even-length values, consecutive reads, fragmented responses, and permanent fuzz seeds. Fixes #420 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
Integrate the upstream yes/no boolean connection-string support from #468, including its tests and documentation, without changing the TDS allocation and variant fixes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
Integrate the upstream release version, manifest, and changelog from #470 without changing the TDS allocation and variant fixes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
Collaborator
|
Automated review deferred: No code review was performed. Please investigate the failure. This sweep will reconsider the PR only after a new commit, provided the |
Integrate the upstream Authentication keyword and ADO.NET value aliases from #368, including regression tests, without changing the TDS allocation and variant fixes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
Collaborator
Author
|
Refuted as a current behavior regression:
An isolated test through the metadata-selected reader confirmed known-/unknown-length empty values stay non-nil, NULL stays nil, and non-empty values decode correctly across two consecutive passes and single-packet/1-byte/3-byte fragmentation, without changing production code. |
Integrate upstream regression and compatibility review guidance from #471. Driver code, tests, and the TDS allocation fixes remain unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8081545c-165e-47b4-b155-797228e2ee67
Collaborator
|
No new code findings at Reviewed Checked and found sound: the FEDAUTHINFO bounds now hold on both ends ( The one area of the diff nobody had commented on is Verification gap, for the record: no Go toolchain was available on the sweep host, so none of this was executed — it is source reasoning plus the green CI matrix (all 12 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #420. Originally stacked on #419 (the Layer 1 fuzz harness); this PR now targets
main. Part of the fuzz-coverage work tracked in #418.Several TDS response parsers allocated buffers or slices from attacker-controlled length/count prefixes before reading or validating the corresponding data. Repeated metadata could multiply otherwise bounded per-column reservations into gigabytes. Malformed sql_variant sizes could also let a fixed-width read consume bytes belonging to the next value. The affected malformed lengths/counts now fail through
badStreamPanicasStreamError, allowing the driver to retire the connection instead of returning it to the pool.Allocation and boundary checks
parseFedAuthInfouint64arithmetic before slicing.parseColMetadata72readTypeInfo/readVarLenreadCekTablereadCekTableEntryreadLongLenTypereadPLPType/readPLPBytesreadVariantTypeWithEncodingThe CEK limit follows ALTER COLUMN ENCRYPTION KEY: two encrypted values support master-key rotation. PLP retains the existing treatment of total lengths as hints, consistent with the MS-TDS data-stream description, while limiting the actual accumulated payload. Variant property counts follow the MS-TDS sql_variant layout; decimal/numeric permits all four documented payload widths, including shorter representations of higher-precision values.
There are no exported API changes or changes to authentication, TLS, or certificate settings. Valid values, including empty binary/text variants and NULL variants, retain their existing behavior.
Scope of memory protection: This fixes allocations driven by unchecked prefixes and metadata-only reservations, not a general per-response memory quota. Fully received CEK entries and large legitimate values still require storage proportional to their data. A bounded review probe with the maximum advertised CEK table count allocated 56 bytes when no entries were supplied, about 1.46 MB after receiving one 395 KB maximum-field entry, and 2.90 MB after two. This confirms incremental consumption, not a cumulative hard limit; a genuinely huge supplied table can still use substantial memory. No new arbitrary CEK-table budget was added in response to that review suggestion.
Regression coverage
token_alloc_regression_test.go: FEDAUTHINFO underflow, oversized/count/offset-overflow cases, malformed/truncated LOB lengths, sql_variant allocation lengths, COLMETADATA allocation measurement, and complete malformed responses.token_cek_regression_test.go: maximum CEK counts, repeated allocation attempts, valid rotation, every truncated-entry prefix, UTF-16 boundaries, packet seams, ordinal lookup, and connection retirement.types_alloc_regression_test.go: metadata creates no value buffers; safely scaleda5/fffemetadata reproduces the allocation amplification; repeated fixed/byte/short reads preserve values and allocate only for actual lengths; decrypted buffers, invalid lengths, exact maximum PLP hints, many empty PLP columns, cumulative known/unknown-length limits, truncation and packet seams are exercised.types_variant_test.go: covers fixed-width overreads with adjacent sentinels, property counts, decimal widths, time scales, truncated/invalid headers and unsupported types. Covers malformed and valid ROW, NBCROW and RETURNVALUE paths, repeated valid values and packet seams. Unicode additions verify odd lengths 1/3/5 fail before properties or payload are read and retire the connection; empty and even-length values preserve it.FuzzProcessSingleResponsehas permanent malformed metadata/PLP/CEK/variant seeds, including both odd-width Unicode variant types, and runs every input with Always Encrypted disabled and enabled.io.CopyNdispatches tobytes.Buffer.ReadFrom, which allocates before observing zero-length input. No IMAGE-specific change was made.Validation on c1af349
go build ./...passes; changed files are gofmt-clean.readVariantTypeWithEncodingexcept its existing unreachable terminal panic, and 100% of the CEK parsers and lazy-buffer/PLP-byte helpers. The new Unicode rejection and healthy paths are both exercised by explicit assertions.go test -count=1 -timeout 30m ./...passes in every package except two pre-existing Windows certificate-provisioning failures inaecmk/localcert:TestLoadWindowsCertStoreCertificateandTestEncryptDecryptEncryptionKeyRoundTripfail while creating their certificate, before TDS parsing. No tests were skipped or weakened to hide these failures.go vet ./...repeatsintegratedauth/winsspi/winsspi.go:24:36: possible misuse of unsafe.Pointer. The identical finding was reproduced with Go 1.26.5 in an isolated, unmodifiedmainsnapshot during an earlier merge; that file remains unchanged. No unrelated authentication code or vet settings were changed.go test -run=^$ -fuzz=^FuzzProcessSingleResponse$ -fuzztime=180s .completes with no crash or worker OOM/EOF:Upstream base updates
The branch includes
main's v1.11.1 release update (#456) and #469, which intentionally reverts #410 and restores the known query-draining issue #407 for separate reassessment. This PR preserves that upstream decision rather than reinstating response-cleanup work. The base updates themselves did not change the allocation protections.