Bound COLMETADATA pre-allocation to prevent allocation-amplification DoS - #290
Bound COLMETADATA pre-allocation to prevent allocation-amplification DoS#290Saurabh Singh (saurabh500) wants to merge 3 commits into
Conversation
The COLMETADATA parser reserved Vec::with_capacity(col_count) before reading any column bytes. With size_of::<ColumnMetadata>() = 640, an attacker-controlled col_count up to 0xFFFE forces a ~42 MB eager allocation from as few as 3 bytes. Under the ASan fuzz build this allocation/poisoning takes seconds, which libFuzzer reports as a timeout (fuzz_token_stream, build 164079). Reject col_count above SQL Server's 4096 columns-per-SELECT maximum with a ProtocolError before reserving storage, and add a regression test. Reproducers now run in ~22-26 ms under ASan. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Bounds COLMETADATA column counts to prevent allocation-amplification attacks.
Changes:
- Rejects counts above SQL Server’s 4,096-column limit.
- Preserves the no-metadata sentinel.
- Adds a regression test.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The 4096 column count assumption doesn't seem right. Sparse tables can have 30000 columns. |
Reserving the full untrusted col_count up front was the amplification vector. Bounding the initial capacity kills the DoS regardless of count, so any legitimate wide-table column count (up to 30,000) parses without an artificial reject. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed in the latest commit: I dropped the column-count cap entirely rather than raising it to 30,000/64,000. Since |
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
🔗 Quick Links |
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
Summary
Small, correct fix. Capping the initial capacity at COLUMN_PREALLOC_CAP (256) removes the amplification without imposing an artificial column limit, and behavior is unchanged for legitimate result sets — the 0xFFFF sentinel still short-circuits and the loop still runs col_count times, so the column_count == columns.len() invariant that row_parser / nbcrow_parser rely on still holds.
I verified the amplification math locally: size_of::<ColumnMetadata>() == 640, so 0xFFFE × 640 ≈ 41.9 MB. The description is accurate.
One blocking issue on the tests (inline), plus two findings that fall outside the diff:
Suggestion — parse_cek_table has the identical bug, ten lines below the fix
colmetadata_parser.rs:275 still does Vec::<CekTableEntry>::with_capacity(table_size as usize) from an attacker-controlled u16, before reading any entry bytes. I measured size_of::<CekTableEntry>() == 48, so that's ~3.1 MB reserved from 2 bytes on the wire (~1.5M× amplification). It's reachable only when Always Encrypted is negotiated, so it's lower severity and may not be on the fuzz target's current path — but it's the same pattern in the same file, and bounding it here closes the class rather than one instance. (parse_cek_table_entry's value_count is a u8, max ~24 KB — fine as-is.)
Suggestion — seed the fuzz corpus with the reproducer
mssql-tds/fuzz/corpus-fuzz_token_stream.tar.gz is checked in and run-fuzz.sh extracts it as the starting corpus. Adding the minimized inputs (02 81 33 aa, 81 84 b8) to that archive makes the fuzz job itself re-exercise the case, which covers the ASan-timing dimension unit tests can't reach.
Nit — follow-up
ColumnMetadata at 640 bytes is the multiplier that made this exploitable at all. Boxing the large members (type_info / crypto_metadata) would shrink the factor across every Vec<ColumnMetadata> in the crate. Separate work item, not this PR.
Checked, no issues
Linked ADO work item present (AB#47316) and the Linked Issue check passes; coverage-report and PR validation are green; no public API change, so the unchecked docs box is correct. cargo test -p mssql-tds --lib colmetadata → 23 passed locally, matching the description.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Also addressed the two review suggestions outside the inline threads in 02e4359: bounded the Always Encrypted CEK-table pre-allocation and seeded the checked-in token-stream fuzz corpus with both minimized reproducers. |
Description
The COLMETADATA (
0x81) token parser read a 16-bitcol_countand immediately didVec::<ColumnMetadata>::with_capacity(col_count)before reading any column bytes. Withsize_of::<ColumnMetadata>() = 640, an attacker-controlledcol_countup to0xFFFE(65534) forces an eager allocation of up to ~42 MB from as few as 3 input bytes (~14,000,000× amplification).Under the ASan fuzz build, allocating and shadow-poisoning that region takes several seconds, which libFuzzer reports as a timeout. The
GH-Rust Fuzz Testpipeline (definitionId 2207) runs with-timeout=10 -fork=1 -ignore_crashes=1, where timeout reproducers are written with thecrash-prefix — so this surfaced as a "crash" in build 164079 (Found 1 crash(es) during fuzzing, job Fuzz Test Token Stream Linux, crash artifactfuzz-crashes-fuzz_token_stream-__default/crash-80c55599ebe9adee81668324e4c6151894a33fbe).Fix
Bound attacker-controlled vector pre-allocation instead of capping protocol counts.
initial_column_capacitylimits the initialVec<ColumnMetadata>capacity to 256 (currently 160 KiB), and the parser grows the vector only as column bytes are consumed. The same strategy now bounds the Always Encrypted CEK table's initial capacity to 256 entries (currently 12 KiB), closing the identical allocation-amplification path inparse_cek_table.No artificial column or CEK-table count limit is imposed. Legitimate wide/sparse-column result sets continue to parse, and the existing
0xFFFFno-metadata sentinel is preserved. The original EOF-path tests remain, direct capacity tests assert both bounds, and the minimized reproducers are included in the checked-in fuzz corpus.Verification
02 81 33 aa,81 84 b8) ran in 1.5–5.6 s under ASan before the fix; 22–26 ms after.cargo nextest run --frozen -p mssql-tds -E 'test(colmetadata)'→ 25 passed.cargo bfmt/cargo bclippyclean.Related Issues
ADO work item: https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/47316
AB#47316
Checklist
cargo bfmtpassescargo bclippypassescargo btestpasses on the latest commit (PR validation is running)