From 1bba0ab39ea49c21291169b332f569039fbcee67 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 5 Jun 2026 01:07:55 +0000 Subject: [PATCH] fix(sqlite): bound .dump output cumulatively across all tables Pass the remaining output budget into dump() via dispatch(), and enforce it during construction with bounded_append(). Previously each per-table SELECT was bounded by max_result_bytes, but cumulative dump output could grow unboundedly before the caller applied push_stdout_bounded. An attacker controlling many tables/rows could exhaust memory. Changes: - DotError::OutputCap variant for clean Display-formatted cap errors - bounded_append() helper: checks cumulative budget before each push - dispatch() gains max_output_bytes param; run_statements passes remaining - New unit tests: dump_respects_output_cap, dump_empty_db_under_cap - New integration test: dump_output_cap_enforced_across_multiple_tables - THREAT[TM-DOS-091] annotation at mitigation point - Updated specs/threat-model.md (TM-DOS-091) and specs/sqlite-builtin.md (TM-SQL-013) Closes #1869 --- .../src/builtins/sqlite/dot_commands.rs | 146 ++++++++++++++++-- crates/bashkit/src/builtins/sqlite/mod.rs | 13 +- crates/bashkit/src/builtins/sqlite/tests.rs | 31 ++++ specs/sqlite-builtin.md | 1 + specs/threat-model.md | 1 + 5 files changed, 179 insertions(+), 13 deletions(-) diff --git a/crates/bashkit/src/builtins/sqlite/dot_commands.rs b/crates/bashkit/src/builtins/sqlite/dot_commands.rs index a0c087c1..49ea944e 100644 --- a/crates/bashkit/src/builtins/sqlite/dot_commands.rs +++ b/crates/bashkit/src/builtins/sqlite/dot_commands.rs @@ -52,6 +52,8 @@ pub(super) enum DotError { InvalidValue { cmd: &'static str, value: String }, #[error("sqlite engine error: {0}")] Engine(String), + #[error("sqlite output exceeds byte cap ({0} > {1})")] + OutputCap(usize, usize), } const HELP_TEXT: &str = concat!( @@ -69,12 +71,19 @@ const HELP_TEXT: &str = concat!( ".read PATH Execute SQL from a VFS file\n", ); +/// Dispatch a dot-command line. +/// +/// `max_output_bytes` is the *remaining* output budget for the current +/// invocation. `.dump` enforces this cumulatively across schema and all +/// table rows so memory growth is bounded during construction, not only +/// after the full string is returned to the caller. pub(super) fn dispatch( line: &str, engine: &SqliteEngine, opts: &mut OutputOpts, deadline: Deadline, limits: QueryLimits, + max_output_bytes: usize, ) -> Result { let (name, args) = tokenize_dot(line); match name.as_str() { @@ -89,7 +98,7 @@ pub(super) fn dispatch( "indexes" | "indices" => { indexes(args, engine, opts, deadline, limits).map(DotOutcome::Stdout) } - "dump" => dump(engine, deadline, limits).map(DotOutcome::Stdout), + "dump" => dump(engine, deadline, limits, max_output_bytes).map(DotOutcome::Stdout), "read" => { let path = args .into_iter() @@ -276,15 +285,39 @@ fn indexes( Ok(out) } +/// Append `chunk` to `buf`, returning `Err(OutputCap)` if doing so would +/// exceed `max`. This enforces the output budget *during* construction so +/// memory never grows beyond the cap even for large multi-table dumps. +/// THREAT[TM-DOS-091]: cumulative `.dump` output cap enforced here. +fn bounded_append(buf: &mut String, chunk: &str, max: usize) -> Result<(), DotError> { + let next = buf.len().saturating_add(chunk.len()); + if next > max { + return Err(DotError::OutputCap(next, max)); + } + buf.push_str(chunk); + Ok(()) +} + /// Emit `BEGIN; ...; ; COMMIT;`. /// This matches sqlite3's `.dump` for tables; views/triggers/indexes only get /// their CREATE statement, no rows. Blob literals are emitted as `X'..'`. +/// +/// `max_output_bytes` is the remaining output budget for the invocation. +/// The cap is applied cumulatively across schema lines and every row from +/// every table — construction stops as soon as the budget would be exceeded, +/// preventing unbounded memory growth from large multi-table databases. fn dump( engine: &SqliteEngine, deadline: Deadline, limits: QueryLimits, + max_output_bytes: usize, ) -> Result { - let mut out = String::from("PRAGMA foreign_keys=OFF;\nBEGIN TRANSACTION;\n"); + let mut out = String::new(); + bounded_append( + &mut out, + "PRAGMA foreign_keys=OFF;\nBEGIN TRANSACTION;\n", + max_output_bytes, + )?; // Schema first. let schema_outcome = engine @@ -296,8 +329,8 @@ fn dump( .map_err(DotError::Engine)?; for row in &schema_outcome.rows { if let Some(Value::Text(sql)) = row.get(2) { - out.push_str(sql.as_str()); - out.push_str(";\n"); + bounded_append(&mut out, sql.as_str(), max_output_bytes)?; + bounded_append(&mut out, ";\n", max_output_bytes)?; } } @@ -321,15 +354,12 @@ fn dump( .map_err(DotError::Engine)?; for data_row in &data.rows { let values: Vec = data_row.iter().map(format_sql_literal).collect(); - out.push_str(&format!( - "INSERT INTO \"{}\" VALUES({});\n", - quoted, - values.join(",") - )); + let line = format!("INSERT INTO \"{}\" VALUES({});\n", quoted, values.join(",")); + bounded_append(&mut out, &line, max_output_bytes)?; } } - out.push_str("COMMIT;\n"); + bounded_append(&mut out, "COMMIT;\n", max_output_bytes)?; Ok(out) } @@ -385,7 +415,14 @@ mod tests { engine: &SqliteEngine, opts: &mut OutputOpts, ) -> Result { - dispatch(line, engine, opts, no_deadline(), unlimited_limits()) + dispatch( + line, + engine, + opts, + no_deadline(), + unlimited_limits(), + usize::MAX, + ) } #[test] @@ -563,6 +600,93 @@ mod tests { assert!(s.contains("COMMIT;")); } + /// `.dump` must stop and return an error when the cumulative output would + /// exceed the configured output cap, preventing unbounded memory growth + /// across many tables (DeepSec issue #1869). + #[test] + fn dump_respects_output_cap() { + let engine = mk_engine(); + // Two tables with several rows of text data. + engine + .execute("CREATE TABLE a(v TEXT)", no_deadline(), unlimited_limits()) + .unwrap(); + engine + .execute("CREATE TABLE b(v TEXT)", no_deadline(), unlimited_limits()) + .unwrap(); + for i in 0..10 { + engine + .execute( + &format!( + "INSERT INTO a VALUES ('row-{i:04}-padding-aaaaaaaaaaaaaaaaaaaaaaaaa')" + ), + no_deadline(), + unlimited_limits(), + ) + .unwrap(); + engine + .execute( + &format!( + "INSERT INTO b VALUES ('row-{i:04}-padding-bbbbbbbbbbbbbbbbbbbbbbbbb')" + ), + no_deadline(), + unlimited_limits(), + ) + .unwrap(); + } + + // A tiny cap that the dump cannot fit into, ensuring the cap fires + // mid-construction rather than only after the full string is built. + let tiny_cap: usize = 100; + let mut o = opts(); + let err = dispatch( + ".dump", + &engine, + &mut o, + no_deadline(), + unlimited_limits(), + tiny_cap, + ) + .unwrap_err(); + + assert!( + matches!(err, DotError::OutputCap(_, _)), + "expected OutputCap, got: {err}" + ); + // Error message must not contain debug shapes — plain text only. + let msg = err.to_string(); + assert!( + !msg.contains("{:?}"), // debug-ok: testing that debug shapes are absent from error output + "debug shape leaked into error: {msg}" + ); + assert!( + msg.len() <= 1024, + "error message too long ({} bytes)", + msg.len() + ); + } + + /// `.dump` on an empty database still produces the PRAGMA/BEGIN/COMMIT + /// boilerplate even with a very tight cap that fits it. + #[test] + fn dump_empty_db_under_cap() { + let engine = mk_engine(); + let mut o = opts(); + // "PRAGMA foreign_keys=OFF;\nBEGIN TRANSACTION;\nCOMMIT;\n" is ~50 bytes. + let DotOutcome::Stdout(s) = dispatch( + ".dump", + &engine, + &mut o, + no_deadline(), + unlimited_limits(), + 4096, + ) + .unwrap() else { + panic!("expected stdout"); + }; + assert!(s.starts_with("PRAGMA foreign_keys=OFF;")); + assert!(s.ends_with("COMMIT;\n")); + } + #[test] fn read_returns_path() { let engine = mk_engine(); diff --git a/crates/bashkit/src/builtins/sqlite/mod.rs b/crates/bashkit/src/builtins/sqlite/mod.rs index 70fca53d..0bcba350 100644 --- a/crates/bashkit/src/builtins/sqlite/mod.rs +++ b/crates/bashkit/src/builtins/sqlite/mod.rs @@ -789,8 +789,17 @@ async fn run_statements( push_stdout_bounded(stdout, &rendered, limits.max_output_bytes)?; } Stmt::Dot(line) => { - let result = - dot_commands::dispatch(&line, engine, opts, deadline, query_limits(limits)); + // Pass the *remaining* output budget so .dump can enforce it + // cumulatively during construction (DeepSec #1869). + let remaining = limits.max_output_bytes.saturating_sub(stdout.len()); + let result = dot_commands::dispatch( + &line, + engine, + opts, + deadline, + query_limits(limits), + remaining, + ); match result { Ok(DotOutcome::Stdout(s)) => { push_stdout_bounded(stdout, &s, limits.max_output_bytes)?; diff --git a/crates/bashkit/src/builtins/sqlite/tests.rs b/crates/bashkit/src/builtins/sqlite/tests.rs index 0607c44d..c9d4acd0 100644 --- a/crates/bashkit/src/builtins/sqlite/tests.rs +++ b/crates/bashkit/src/builtins/sqlite/tests.rs @@ -518,6 +518,37 @@ async fn dump_data_respects_row_cap() { ); } +/// `.dump` output cap is enforced cumulatively across all tables, not just +/// per-query (DeepSec #1869). With a tiny max_output_bytes, a dump that +/// would fit under max_result_bytes per table still hits the invocation cap. +#[tokio::test] +async fn dump_output_cap_enforced_across_multiple_tables() { + let limits = SqliteLimits::default() + .max_rows_per_query(1000) + .max_result_bytes(64 * 1024) + .max_output_bytes(120) // too small for schema + any data rows + .max_duration(std::time::Duration::ZERO); + let r = run_with_limits( + &[ + ":memory:", + "CREATE TABLE a(v TEXT); CREATE TABLE b(v TEXT);\ + INSERT INTO a VALUES ('row-aaa'), ('row-bbb');\ + INSERT INTO b VALUES ('row-ccc'), ('row-ddd');\n.dump", + ], + None, + limits, + ) + .await; + assert_eq!(r.exit_code, 1, "should fail due to output cap"); + // Error must be plain text, not a debug-formatted struct. + let msg = &r.stderr; + assert!( + msg.contains("output exceeds byte cap") || msg.contains("sqlite:"), + "unexpected stderr: {msg}" + ); + assert!(!msg.contains("{:?}"), "debug shape leaked into stderr"); // debug-ok: testing that debug shapes are absent +} + #[tokio::test] async fn value_byte_cap_rejects_large_cell_before_rendering() { let limits = SqliteLimits::default() diff --git a/specs/sqlite-builtin.md b/specs/sqlite-builtin.md index 04a65ea4..6ecb7622 100644 --- a/specs/sqlite-builtin.md +++ b/specs/sqlite-builtin.md @@ -228,6 +228,7 @@ leading SQL keyword via the parser's lightweight tokeniser | TM-SQL-010 | DoS / fingerprinting via dangerous PRAGMAs | `SqliteLimits::pragma_deny` defaults block `cache_size`, `mmap_size`, `page_size`, `max_page_count`, `temp_store_directory`, `data_store_directory`, `compile_options`, `locking_mode`, `shared_cache`; parser handles comments plus quoted/schema-qualified names | | TM-SQL-011 | Information leakage via host-side error strings | `sanitize()` strips ` at /…:N:M` annotations from turso errors | | TM-SQL-012 | Sandbox escape via `VACUUM INTO` writing host files | Policy rejects `VACUUM` (with/without `INTO`) at the keyword sniffer; tested via `vacuum_into_blocked`/`vacuum_plain_blocked`/`vacuum_blocked_with_leading_comment` | +| TM-SQL-013 | DoS via `.dump` cumulative output bypass | `.dump` previously built the full string before `max_output_bytes` was applied; `bounded_append()` enforces the cap after each schema/row chunk with the remaining budget passed from `run_statements`; `THREAT[TM-DOS-091]`; tested via `dump_respects_output_cap` and `dump_output_cap_enforced_across_multiple_tables` | ## Test Plan diff --git a/specs/threat-model.md b/specs/threat-model.md index d4ca60df..fb18b4f3 100644 --- a/specs/threat-model.md +++ b/specs/threat-model.md @@ -1385,6 +1385,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track | TM-DOS-088 | Command substitution OOM via state cloning | OOM at depth N (memory ≈ N × state_size) | Dedicated `max_subst_depth` limit (default 32), separate from `max_function_depth` — **FIXED** via #1088 | | TM-DOS-089 | Command substitution stack overflow via inlined futures | SIGABRT at ~20-30 nested $() levels | Box::pin `expand_word` and `execute_cmd_subst` to cap per-level stack — **FIXED** via #1089 | | ~~TM-DOS-090~~ | ~~`shuf` unbounded range/repeat materialization~~ | ~~OOM/CPU exhaustion via huge `--input-range` or `--head-count` before stdout truncation~~ | ~~Sample numeric ranges without full collection and reject output that exceeds `ExecutionLimits` before allocation~~ — `shuf_resource_tests` cover huge range `-n 1` and repeat output caps (**FIXED**) | +| TM-DOS-091 | SQLite `.dump` cumulative output bypass | `.dump` built the full schema+rows string before checking `max_output_bytes`; an attacker controlling many tables/rows could cause memory to grow far beyond the configured output cap | `bounded_append()` helper enforces the cap after each schema line and each INSERT row; `dispatch()` receives the *remaining* budget (`max_output_bytes - stdout.len()`) from `run_statements` — **FIXED** via #1869 | ### Accepted (Low Priority)