From 5d3221c7ff9195d3af04a57565ff6bbe07fe3f13 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Thu, 6 Aug 2026 23:59:03 -0500 Subject: [PATCH] fix(yq): reject YAML aliases before parsing --- crates/bashkit/docs/yq.md | 13 +-- crates/bashkit/src/builtins/yq.rs | 86 +++++++++++++++++++ .../tests/integration/yq_integration_tests.rs | 38 +++++--- knowledge/operations/limitations.md | 2 +- knowledge/security/threat-model.md | 6 +- 5 files changed, 124 insertions(+), 21 deletions(-) diff --git a/crates/bashkit/docs/yq.md b/crates/bashkit/docs/yq.md index e1871c6a..b4ecf8ff 100644 --- a/crates/bashkit/docs/yq.md +++ b/crates/bashkit/docs/yq.md @@ -53,12 +53,13 @@ selection, iteration, `select`, `map`, construction, reduction, and assignment filters work. mikefarah/yq-only operators for comments, styles, anchors, tags, file metadata, and cross-file evaluation are not implemented. -YAML custom tags, non-string mapping keys, and non-finite numbers are rejected -rather than silently losing information at the JSON-value boundary. Mapping -keys are sorted deterministically. Comments, scalar style, and anchors are not -retained after conversion. The parser follows YAML 1.1. TOML, CSV, and XML -conversion are not part of this builtin; Bashkit's separate `tomlq` and `csv` -helpers remain available for their existing narrow command surfaces. +YAML aliases, custom tags, non-string mapping keys, and non-finite numbers are +rejected rather than expanding attacker-controlled graphs or silently losing +information at the JSON-value boundary. Mapping keys are sorted deterministically. +Comments, scalar style, and anchors are not retained after conversion. The parser +follows YAML 1.1. TOML, CSV, and XML conversion are not part of this builtin; +Bashkit's separate `tomlq` and `csv` helpers remain available for their existing +narrow command surfaces. ## See also diff --git a/crates/bashkit/src/builtins/yq.rs b/crates/bashkit/src/builtins/yq.rs index f4a8b860..75c730ba 100644 --- a/crates/bashkit/src/builtins/yq.rs +++ b/crates/bashkit/src/builtins/yq.rs @@ -453,6 +453,7 @@ fn parse_json_documents(input: &str) -> DataResult> { // THREAT[TM-DOS-101]: serde_yaml_ng rejects nesting beyond 128 while this // conversion enforces Bashkit's lower shared structured-data depth of 100. fn parse_yaml_documents(input: &str) -> DataResult> { + reject_yaml_aliases(input)?; let mut values = Vec::new(); for document in serde_yaml_ng::Deserializer::from_str(input) { if values.len() >= MAX_DOCUMENTS { @@ -466,6 +467,91 @@ fn parse_yaml_documents(input: &str) -> DataResult> { Ok(values) } +// THREAT[TM-DOS-101]: serde_yaml_ng resolves aliases while deserializing, before +// Bashkit can meter the expanded tree. Reject alias tokens with a bounded lexical +// pass, while ignoring quoted and block scalar content, before invoking libyaml. +fn reject_yaml_aliases(input: &str) -> DataResult<()> { + let mut block_scalar_indent = None; + let mut single_quoted = false; + let mut double_quoted = false; + for line in input.lines() { + let indent = line.bytes().take_while(|byte| *byte == b' ').count(); + if !single_quoted + && !double_quoted + && let Some(parent_indent) = block_scalar_indent + { + if line.trim().is_empty() || indent > parent_indent { + continue; + } + block_scalar_indent = None; + } + + let mut escaped = false; + let bytes = line.as_bytes(); + let mut index = 0; + while index < bytes.len() { + let byte = bytes[index]; + if double_quoted { + if escaped { + escaped = false; + } else if byte == b'\\' { + escaped = true; + } else if byte == b'"' { + double_quoted = false; + } + index += 1; + continue; + } + if single_quoted { + if byte == b'\'' { + if bytes.get(index + 1) == Some(&b'\'') { + index += 2; + continue; + } + single_quoted = false; + } + index += 1; + continue; + } + match byte { + b'"' => double_quoted = true, + b'\'' => single_quoted = true, + b'#' if index == 0 || bytes[index - 1].is_ascii_whitespace() => break, + b'|' | b'>' if is_block_scalar_header(bytes, index) => { + block_scalar_indent = Some(indent); + break; + } + b'*' if token_starts_at(bytes, index) => { + anyhow::bail!("yq: YAML aliases are not supported"); + } + _ => {} + } + index += 1; + } + } + Ok(()) +} + +fn token_starts_at(line: &[u8], index: usize) -> bool { + index == 0 + || line[index - 1].is_ascii_whitespace() + || matches!(line[index - 1], b'[' | b'{' | b',' | b':' | b'?' | b'-') +} + +fn is_block_scalar_header(line: &[u8], index: usize) -> bool { + if !token_starts_at(line, index) { + return false; + } + let mut suffix = &line[index + 1..]; + while suffix + .first() + .is_some_and(|byte| matches!(byte, b'0'..=b'9' | b'+' | b'-')) + { + suffix = &suffix[1..]; + } + suffix.is_empty() || suffix[0].is_ascii_whitespace() || suffix[0] == b'#' +} + fn yaml_to_json(value: serde_yaml_ng::Value, depth: usize) -> DataResult { if depth > MAX_DEPTH { anyhow::bail!("yq: nesting too deep ({depth} levels, max {MAX_DEPTH})"); diff --git a/crates/bashkit/tests/integration/yq_integration_tests.rs b/crates/bashkit/tests/integration/yq_integration_tests.rs index 48a295c9..e009c69f 100644 --- a/crates/bashkit/tests/integration/yq_integration_tests.rs +++ b/crates/bashkit/tests/integration/yq_integration_tests.rs @@ -90,6 +90,30 @@ async fn yaml_document_count_is_bounded() { assert!(result.stderr.contains("document limit exceeded (4096)")); } +#[tokio::test] +async fn yaml_aliases_are_rejected_before_expansion() { + let mut bash = Bash::new(); + let result = bash + .exec("printf 'seed: &seed [x, x, x]\nexpanded: [*seed, *seed, *seed]\n' | yq '.'") + .await + .unwrap(); + + assert_eq!(result.exit_code, 1); + assert!(result.stderr.contains("YAML aliases are not supported")); + + let scalars = bash + .exec("printf '%s\n' 'plain: a*b' 'quoted: \"*seed\"' \"single: 'it''s *seed'\" 'multiline: \"first' ' *seed\"' 'literal: |' ' *seed' | yq -r '.plain, .quoted, .single, .multiline, .literal'") + .await + .unwrap(); + assert_eq!(scalars.exit_code, 0, "{}", scalars.stderr); + // Every literal `*` survives (none is an alias); multiple filter results are + // rendered as separate YAML documents (`---`), which is existing yq behavior. + assert_eq!( + scalars.stdout, + "a*b\n---\n*seed\n---\nit's *seed\n---\nfirst *seed\n---\n*seed\n\n" + ); +} + #[tokio::test] async fn yaml_tags_and_non_string_keys_fail_closed() { let mut bash = Bash::new(); @@ -106,18 +130,10 @@ async fn yaml_tags_and_non_string_keys_fail_closed() { } #[tokio::test] -async fn yaml_aliases_are_resolved_and_lossy_numbers_fail_closed() { +async fn yaml_duplicate_keys_and_lossy_numbers_fail_closed() { + // Alias resolution was replaced by fail-closed rejection (TM-DOS-101); see + // `yaml_aliases_are_rejected_before_expansion` for the alias-bomb coverage. let mut bash = Bash::new(); - let alias = bash - .exec("printf 'base: &base\n one: 1\ncopy: *base\n' | yq -o=json -I=0 '.'") - .await - .unwrap(); - assert_eq!(alias.exit_code, 0, "{}", alias.stderr); - assert_eq!( - alias.stdout, - "{\"base\":{\"one\":1},\"copy\":{\"one\":1}}\n" - ); - let duplicate = bash .exec("printf 'key: 1\nkey: 2\n' | yq '.'") .await diff --git a/knowledge/operations/limitations.md b/knowledge/operations/limitations.md index 01119241..e824de96 100644 --- a/knowledge/operations/limitations.md +++ b/knowledge/operations/limitations.md @@ -124,7 +124,7 @@ pass in CI); only divergences and boundaries are recorded here. | L-JQ-001 | jq | Alternative `//`: jaq errors on `.foo` applied to null instead of returning null (upstream jaq divergence) | 1 skipped spec test | | L-JQ-002 | jq | Regex natives compile the pattern per filter invocation; mapping `test`/`match`/`split` over many inputs can repeat compilation because jaq's native callback has no per-run cache state | `regex_compat.rs::re_native` | | L-YQ-001 | yq | Expressions are Bashkit jq expressions; mikefarah/yq-only node, comment, style, anchor, tag, filename, and eval-all operators are not implemented | stance | -| L-YQ-002 | yq | YAML conversion follows YAML 1.1, deterministically sorts mapping keys at the JSON-value boundary, drops comments/style/anchors, and rejects custom tags, non-string mapping keys, and non-finite numbers rather than silently corrupting them | `yaml_tags_and_non_string_keys_fail_closed`, `yaml_aliases_are_resolved_and_lossy_numbers_fail_closed`, `inplace_update_is_atomic_and_suppresses_stdout` | +| L-YQ-002 | yq | YAML conversion follows YAML 1.1, deterministically sorts mapping keys at the JSON-value boundary, drops comments/style/anchors, and rejects aliases, custom tags, non-string mapping keys, and non-finite numbers rather than expanding graphs or silently corrupting data | `yaml_aliases_are_rejected_before_expansion`, `yaml_tags_and_non_string_keys_fail_closed`, `yaml_duplicate_keys_and_lossy_numbers_fail_closed`, `inplace_update_is_atomic_and_suppresses_stdout` | | L-YQ-003 | yq | Input/output conversion supports YAML and JSON only; mikefarah/yq's XML, CSV, TOML, properties, HCL, Lua, and INI formats are not exposed through yq | stance | | L-GREP-001 | grep | `--color`/`--colour`, `--line-buffered` accepted as no-ops | `l_grep_001_noop_flags` | | L-CURL-001 | curl | Spec-test coverage for methods/headers/auth/redirects not ported (needs `http_client` + allowlist in harness); payload behavior has integration and real-curl differential coverage | stance | diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index a210272b..4ea7d24a 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -275,7 +275,7 @@ runaway scripts without permanently breaking the session. | TM-DOS-061 | Snapshot function restore bypasses parser/function limits | Crafted snapshot restores functions exceeding the tenant's parser depth or function memory budget | Restored function source re-parsed with current `ExecutionLimits`; function memory budget re-applied before insertion | **MITIGATED** | | TM-DOS-062 | jq file binding amplification | Repeated `--rawfile` / `--slurpfile` bindings to one max-sized VFS file multiply retained jq globals and `$ARGS.named` values without consuming more VFS quota | `MAX_FILE_VAR_REQUESTS` caps binding count; `MAX_FILE_VAR_BYTES` counts cumulative file bytes per binding before retaining globals | **MITIGATED** | | TM-DOS-063 | Persistent fd exhaustion | `exec N>/tmp/f` across many `N` values grows `exec_fd_table`/coproc fd buffers for the session | `ExecutionLimits::max_file_descriptors` (default 1024) caps persistent custom descriptors; reused fds and standard 0/1/2 don't count | **MITIGATED** | -| TM-DOS-101 | yq structured-data amplification | Deep YAML/JSON, document floods, jaq generators, and YAML re-serialization can consume stack, CPU, or memory beyond the source size | VFS/stdin and aggregate input budgets; serde_yaml_ng recursion cap 128 plus Bashkit depth 100; 4096-document cap; shared jaq work/deadline/output limits; post-serialization stdout cap; YAML+JSON depth/document/input/output regressions plus `yq_fuzz` and arbitrary-input proptest | **MITIGATED** | +| TM-DOS-101 | yq structured-data amplification | YAML aliases can expand exponentially during deserialization; deep YAML/JSON, document floods, jaq generators, and YAML re-serialization can consume stack, CPU, or memory beyond the source size | Reject YAML alias tokens with a bounded lexical pass before deserialization; VFS/stdin and aggregate input budgets; serde_yaml_ng recursion cap 128 plus Bashkit depth 100; 4096-document cap; shared jaq work/deadline/output limits; post-serialization stdout cap; YAML+JSON depth/document/input/output regressions plus `yq_fuzz` and arbitrary-input proptest | **MITIGATED** | **TM-DOS-051** is historical. The custom parser was deleted with the `yaml` helper; the replacement `yq` parser/evaluator boundary is tracked by TM-DOS-101. @@ -1214,7 +1214,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track | TM-DOS-098 | Suspended host-call retention or request accumulation | An untrusted script repeatedly invokes an event-backed builtin, or the host never resumes a yielded request, retaining interpreter and request data indefinitely | The sequential interpreter can reach only one call at a time; a capacity-one channel adds backpressure; the existing command, aggregate-budget, output, and wall-clock limits remain in force; `ExecutionHandle` exclusively owns the session so dropping it releases all retained state rather than exposing a partially unwound interpreter — **MITIGATED** | | TM-DOS-099 | `time -f/-o` report amplification bypasses output limits | A large attacker-controlled format repeats expanding fields and writes the result to the VFS instead of stderr | Report rendering is capped by `ExecutionLimits::max_stderr_bytes` before either stderr emission or VFS write; invalid/over-limit reports do not replace an existing `-o` target — **MITIGATED** | | TM-DOS-100 | jq control-character normalization amplification | A jq JSON string consisting of literal controls expands sixfold when each byte becomes `\u00XX`; an unmetered compatibility copy can exhaust memory or CPU before strict parsing | The jq-only normalizer charges input-length work before its single pass, borrows unchanged input, and acquires/grows a shared live-intermediate lease before every allocation growth (`builtins/jq/input.rs`) — **MITIGATED** | -| TM-DOS-101 | yq structured-data amplification | YAML/JSON nesting, multi-document floods, filters, and output format expansion are all attacker-controlled | Real parsers with recursion/depth caps, aggregate budgets, shared jaq work/deadline/output controls, and a final rendered-output cap; integration tests cover YAML+JSON depth/document/input/output bounds, proptest composes arbitrary YAML+filters, and `yq_fuzz` covers stdin/in-place format paths — **MITIGATED** | +| TM-DOS-101 | yq structured-data amplification | YAML aliases, YAML/JSON nesting, multi-document floods, filters, and output format expansion are all attacker-controlled | A pre-deserialization lexical pass rejects aliases without misclassifying quoted or block scalar content; real parsers with recursion/depth caps, aggregate budgets, shared jaq work/deadline/output controls, and a final rendered-output cap; integration tests cover alias bombs plus YAML+JSON depth/document/input/output bounds, proptest composes arbitrary YAML+filters, and `yq_fuzz` covers stdin/in-place format paths — **MITIGATED** | | TM-DOS-102 | Archive decoder allocation-before-check | A small gzip/bzip2 stream makes the decoder output buffer allocate beyond live-memory or filesystem quotas before the post-growth size check runs | Decoder chunks validate absolute/ratio bounds and acquire a shared execution-budget lease before `try_reserve_exact` and copy; corrupt, truncated, CRC-invalid, ratio-bomb, and live-budget tests fail closed — **MITIGATED** | | TM-DOS-103 | Post-allocation resource charging | Archive/compression and other growing buffers could call `reserve`/`extend` before taking a live-byte lease; concurrent `fetch_add` accounting could also wrap or transiently overcommit the ceiling | `BudgetedVec`/`BudgetedBytes` and `BudgetedString` own their `ExecutionBudgetLease`, charge planned capacity before `try_reserve_exact`, roll back on allocation/error, and release on drop. CAS admission rejects overflow and concurrent overcommit. Archive creation, extraction/list output, gzip/bzip2 encoders, and decompression use the builders — **MITIGATED** | @@ -1325,7 +1325,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track | VFS copy/rename semantic bugs | TM-DOS-047, TM-DOS-048 | Fix limit check in copy(), type check in rename() | **MITIGATED** | | Date time info leak | TM-INF-018 | Closed UTC timezone default + sandbox-only IANA `TZ`; `fixed_epoch` / `epoch_offset` for clock virtualization | **MITIGATED** | | Python BashTool.reset() drops limits | TM-PY-028 | `BashTool::reset` rebuilds via `replace_live_bash_with_builder` matching `PyBash::reset` | **MITIGATED** | -| yq parser/evaluator/output limits | TM-DOS-101 | serde_yaml_ng depth 128, Bashkit depth 100, 4096 documents, aggregate budget, shared jaq controls, final rendered-output cap | **MITIGATED** | +| yq parser/evaluator/output limits | TM-DOS-101 | Pre-deserialization alias rejection, serde_yaml_ng depth 128, Bashkit depth 100, 4096 documents, aggregate budget, shared jaq controls, final rendered-output cap | **MITIGATED** | | Template engine depth limit | TM-DOS-052 | `depth` parameter on `render_template_inner` with `MAX_TEMPLATE_DEPTH = 100` | **MITIGATED** | | Unzip path traversal validation | TM-INJ-017 | `validate_extract_entry_path` rejects non-`Normal` components in `zip_cmd.rs` | **MITIGATED** | | Dotenv internal variable guard | TM-INJ-018 | `is_internal_variable()` check in `Dotenv::execute` (`builtins/dotenv.rs:138`) | **MITIGATED** |