Skip to content

fix: deny expect_used and guard both CSV emitters - #14

Merged
h4x0r merged 6 commits into
mainfrom
fix/panic-lints-and-csv-guard
Aug 6, 2026
Merged

fix: deny expect_used and guard both CSV emitters#14
h4x0r merged 6 commits into
mainfrom
fix/panic-lints-and-csv-guard

Conversation

@h4x0r

@h4x0r h4x0r commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Two independent defects, kept in separate RED/GREEN commit pairs so each half reads on its own.

1. CSV emitted with no escaping (commits 1–4)

supertimeline.rs::emit_csv interpolated every field straight into the row. Only description got quote-doubling, and even that was wrapped in unconditional quotes rather than escaped.

  • No formula guard. A description beginning with =, +, - or @ reached the file as a live formula. Descriptions are built from evidence, so those values are attacker-chosen.
  • No escaping at all on event_type, source or tags. A comma in a tag added a column, silently shifting every later field of that row. EventType::Other(String) carries a parser-supplied string and breaks the same way.

Both emitters now build rows with one csv_record helper that runs every field through jsonguard::csv_field — adopting the fleet guard rather than hand-rolling one. No field is exempt, so a column added later cannot reach the file unguarded.

A second instance, beyond the reported line. timeline_format.rs::write_csv (the issen timeline --format csv path, more used than supertimeline) wrote through csv::Writer: quoting was correct, but it applies no formula guard, on the same evidence-derived path and description. Fixing only supertimeline would have left the busier command exposed, so it is fixed here too, with its own RED. Replacing the writer costs none of its escaping — csv_separators_in_values_round_trip reads the output back with the csv crate and asserts commas, embedded quotes and embedded newlines survive.

Tests split rows with the csv crate (BurntSushi), already an issen-cli dependency — an independent RFC 4180 reader rather than a splitter written for the test.

What this does NOT close — two jsonguard gaps, both upstream

Adopting csv_field is the right move, but it does not make the output fully guarded or fully faithful. Measured against the vendored crate, not inferred:

Input Emitted Verdict
=cmd|'/C calc'!A0 '=cmd|'/C calc'!A0 guarded
\t=cmd|'/C calc'!A0 '=cmd|'/C calc'!A0 guarded (tab stripped first, so = becomes char 0)
\r=cmd|'/C calc'!A0 "\r=cmd|'/C calc'!A0" quoted, not guarded
\n=cmd|'/C calc'!A0 "\n=cmd|'/C calc'!A0" quoted, not guarded
=cmd|'/C calc'!A0 =cmd|'/C calc'!A0 bare — neither quoted nor guarded
\u{00A0}=cmd \u{00A0}=cmd bare — neither quoted nor guarded

1. The formula guard keys off character 0 only. Any lead-in that survives the control-character filter defeats it. CR/LF at least get quoted; a leading space or non-breaking space gets nothing at all and reaches the file completely bare. A filename beginning with a space is not exotic in real evidence. jsonguard PR #5 addresses the CR/LF half and is unmerged; the whitespace half is not covered by it.

2. csv_field silently strips control characters without setting lossy. "a\tb.txt" is emitted as "ab.txt" with lossy == false — a field reported differently from the source, with nothing signalling the change. For a supertimeline that is a faithfulness problem, not just a cosmetic one.

Neither is fixed here, and neither is worked around locally: forking the guard at the call site would leave issen and the rest of the fleet disagreeing about what "guarded" means. They belong upstream in jsonguard.

Version footing: 0.2.4 is the newest published; this workspace's lock resolves jsonguard to 0.2.3, whose text.rs is byte-identical to 0.2.4 (diffed both vendored copies). So no lock bump changes any of the above today.

cap_display is deliberately not used on these paths: CSV is a machine view, so values are emitted whole. issen's human-view truncation is already char-safe (truncate / truncate_desc use char_indices); no &s[..n] byte-slice remains in the CLI.

2. expect_used was not denied (commits 5–6)

issen denied unwrap_used but not expect_used, so .expect(..) stayed a sanctioned way to panic. correctness and suspicious were also absent. All 66 members inherit via lints.workspace = true and none override with their own [lints] table, so the root table is the effective config everywhere.

Adding the lints took clippy from clean to 510 findings across 60 files (--keep-going; without it cargo stops at the first failing crate and reports 1). Of those, 25 were production sites — the rest were test code.

No #[allow] was added to silence a production site. Every one was fixed where it was:

Count Cause Fix
8 Mutex::lock().expect("mutex poisoned") in DataSource::read_at (qcow2, aff4, vhd, dmg, dd, vmdk, iso, vhdx) return RtError::Io naming the source
7 literal IOC regexes recompiled per call, issen-signatures STIX one IOC_PATTERNS table compiled once, plus a test
6 env::var(..) in three aggregator build.rs main returns Result; message names the variable
2 ProgressStyle::with_template(..), issen-mft-tree fall back to default styling (the bar is cosmetic)
2 serde_json::to_string_pretty(..) in frequency / processes --json propagate; both runs already return Result
1 FieldRegistry::resolve("logon-type") in presets::logons named LOGON_TYPE const that FIELDS contains
1 sugar() in timeline_query returns Result

The one worth a second look is the mutex family. read_at already returned Result<usize, RtError>, so the failure had an error channel and was panicking anyway. A poisoned lock means an earlier read panicked while holding it — and with the disk legs running under rayon, that turned one failed read of one image into a panic on every subsequent read of that image. Reachable, not theoretical.

Two fixes deserve their rationale stated, because both replace a panic with a quiet path:

  • STIX IOC extraction. A pattern that fails to compile is now dropped rather than panicking, which on its own would cost an IOC class silently — a bundle's SHA-256 indicators would simply stop being extracted. every_ioc_pattern_compiles is the new test that makes the drop safe: it fails the build if the table ever shrinks. The guarantee moved from a runtime expect to a build-time check.
  • uac PAM credential-staging regex. Now returns Option; a None pushes a RootkitFinding recording that the scan did not run. Returning an empty Vec would have read as "no credential staging found" — the opposite of what happened.

The remaining 482 test-side sites took the fleet-sanctioned opt-out: #![cfg_attr(test, allow(clippy::unwrap_used, clippy::expect_used))] in 11 crate roots, and a plain #![allow(..)] in 19 tests//examples/ files, which are separate crates the crate-root attribute cannot reach.

Gate

  • cargo build --workspace — clean
  • cargo clippy --workspace --all-targets --keep-going0 findings
  • cargo fmt --all --check — clean
  • cargo test --workspace --no-fail-fast — one failure, issen-mem dispatch::tests::dispatch_windows_netstat_returns_ok. Pre-existing: it fails identically on origin/main, and this branch changes no file under crates/issen-mem/.

Cargo.lock is deliberately not in this PR. Building here rewrites it because the root Cargo.toml carries a local [patch.crates-io] into a sibling repo whose forensicnomicon-core has moved to 1.5.0 — churn from another worktree's in-flight state, not from this change.

🤖 Generated with Claude Code

@h4x0r
h4x0r force-pushed the fix/panic-lints-and-csv-guard branch from 9cab373 to be31975 Compare August 5, 2026 19:33
h4x0r and others added 6 commits August 6, 2026 10:13
`emit_csv` interpolated every field straight into the row. Only `description`
got quote-doubling, and even that was wrapped in unconditional quotes rather
than escaped, so:

  - a description beginning with `=`, `+`, `-` or `@` reaches the file as a
    live formula (these values come from evidence, so they are attacker-chosen);
  - a comma in a tag or in `EventType::Other(..)` adds a column, silently
    shifting every later field of that row;
  - a double quote in a tag is emitted raw.

Extract `csv_row` / `CSV_HEADER` from `emit_csv` unchanged (Humble Object, so
the row is assertable without capturing stdout), then assert the properties the
emitter should hold. Rows are split with the `csv` crate — an independent
RFC 4180 reader already in issen-cli's dependency set, not a splitter written
for this test — so the column-structure claim is checked by the same class of
parser a spreadsheet uses.

4 of the 6 new tests fail:

  csv_formula_prefixed_description_is_guarded
    description beginning with a formula character must be guarded with a
    leading apostrophe; got "=cmd|'/C calc'!A0"
  csv_comma_in_tag_does_not_break_columns          left: 6  right: 5
  csv_comma_in_event_type_does_not_break_columns   left: 6  right: 5
  csv_quote_in_tag_is_escaped                      left: 6  right: 5

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…csv_field

Route all five fields through `jsonguard::csv_field`, which applies the RFC 4180
quoting and the `= + - @` spreadsheet formula guard in one place. No field is
exempt, so a column added later cannot reach the file unguarded — the reason the
guard is applied per-field rather than to the two that happen to be riskiest
today.

issen already depended on jsonguard, so this adopts the fleet guard rather than
hand-rolling one.

CSV is a machine view, so values are emitted whole — no `cap_display`
truncation on this path.

Caveat, unchanged by this commit: `csv_field` keys the formula guard off the
first character, so a payload whose first character is CR or LF (`\r=cmd|...`)
is quoted but not prefixed, and a spreadsheet that strips the leading newline
still sees a formula. jsonguard PR #5 fixes this and is unmerged; 0.2.4 is the
newest published version and this workspace's lock resolves jsonguard to 0.2.3,
whose `text.rs` is byte-identical to 0.2.4. The bypass closes when 0.2.5
publishes and the lock moves — not here.

6 passed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…uard

Second instance of the defect fixed in supertimeline, in the same crate and on
the more commonly used command. `write_csv` writes through `csv::Writer`, so
RFC 4180 quoting is correct and columns do not break — but the writer applies no
formula guard, and `path` / `description` are built from evidence bytes. A
filename or description beginning with `=`, `+`, `-` or `@` reaches the file
bare and is a live formula when the examiner opens it in Excel or LibreOffice.

  csv_formula_prefixed_field_is_guarded
    path beginning with a formula character must be guarded;
    got "=cmd|'/C calc'!A0"

`csv_separators_in_values_round_trip` passes as written — it is a regression
guard for the writer swap the fix requires, not evidence of a defect.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…itters

`write_csv` now builds rows with `csv_record`, which runs every field through
`jsonguard::csv_field` — the RFC 4180 quoting `csv::Writer` already did, plus
the `= + - @` formula guard it did not. `supertimeline::csv_row` calls the same
helper, so issen has one CSV emission mechanism rather than two that guard
differently.

Replacing `csv::Writer` costs none of its escaping: commas, embedded quotes and
embedded newlines still round-trip (`csv_separators_in_values_round_trip`,
which reads the output back with the `csv` crate).

The CR/LF caveat recorded on the supertimeline fix applies here identically —
it is a jsonguard-side gap (PR #5, unmerged), not a call-site one.

13 passed across both modules.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pace-wide

issen denied `unwrap_used` but not `expect_used`, so `.expect(..)` stayed a
sanctioned way to panic in crates contracted never to. ADR-0012's recipe
requires both, plus the `correctness` and `suspicious` group denials, which were
also absent. All 66 members inherit via `lints.workspace = true` and none
override with their own `[lints]` table, so the root table is the effective
config for every crate.

The pragmatic allows move to `priority = 1`, matching the fleet recipe. That is
load-bearing rather than cosmetic: cargo ignores table order, so a lint group
and an individual lint at the same priority is a hard error — clippy refuses to
run at all rather than silently picking a precedence.

The failing signal here is clippy, not a test. `cargo clippy --workspace
--all-targets --keep-going` goes from clean to 510 findings across 60 files:
18 in production code, 435 inside `#[cfg(test)]` modules and 47 in
`tests/`/`examples/` files. (Without `--keep-going` cargo stops at the first
failing crate and reports 1.)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…test ones

`cargo clippy --workspace --all-targets --keep-going` is clean (0 findings).
No `#[allow]` was added to silence a production site; each was fixed where it
was.

25 production sites, by cause:

  8  `Mutex::lock().expect("mutex poisoned")` in `DataSource::read_at`
     (qcow2, aff4, vhd, dmg, dd, vmdk, iso, vhdx). Reachable, and the most
     consequential of the set: `read_at` already returns
     `Result<usize, RtError>`, so the failure had an error channel and was
     panicking anyway. A poisoned lock means an earlier read panicked while
     holding it — with the disk legs running under rayon, that turned one
     failed read of one image into a panic on every subsequent read of it.
     Now an `RtError::Io` naming the source that poisoned.

  7  literal IOC regexes recompiled per call in `issen-signatures` STIX
     extraction. Replaced by one `IOC_PATTERNS` table compiled once behind a
     `OnceLock`. A pattern that fails to compile is dropped rather than
     panicking, and `every_ioc_pattern_compiles` is the new test that makes
     that safe — it fails the build if the table ever shrinks, so a malformed
     pattern can no longer cost an IOC class silently. Also stops rebuilding
     seven regexes for every pattern in a bundle.

  6  `env::var(..).expect(..)` in the three aggregator `build.rs` scripts.
     `main` now returns `Result`, and the message names the missing variable
     (`VarError` alone does not).

  2  `ProgressStyle::with_template(..).expect(..)` in issen-mft-tree. The bar
     is cosmetic, so it falls back to indicatif's default styling rather than
     taking an MFT parse down.

  1  `FieldRegistry::resolve("logon-type").expect(..)` in `presets::logons`.
     The field is now a named `LOGON_TYPE` const that `FIELDS` itself contains,
     so the preset references the definition instead of looking its name up at
     runtime — registry and presets cannot disagree, and there is nothing left
     to fail.

  1  `sugar()` in `timeline_query` returns `Result`. Dropping an unresolvable
     sugar filter would have silently widened the analyst's result set, so it
     reports instead.

  2  `serde_json::to_string_pretty(..).expect(..)` in the `frequency` and
     `processes` `--json` paths. Both enclosing `run`s already return
     `anyhow::Result<()>`.

  The uac PAM credential-staging regex now returns `Option`, and a `None`
  pushes a `RootkitFinding` saying the scan did not run. Returning an empty
  `Vec` there would have read as "no credential staging found" — the opposite
  of what happened.

482 test-side sites took the fleet-sanctioned opt-out instead:
`#![cfg_attr(test, allow(clippy::unwrap_used, clippy::expect_used))]` in 11
crate roots, and a plain `#![allow(..)]` in 19 `tests/`/`examples/` files,
which are separate crates the crate-root attribute cannot reach.

Gate: build, clippy `--all-targets --keep-going` (0), `fmt --check`. Tests pass
except the pre-existing `issen-mem dispatch::tests::dispatch_windows_netstat_returns_ok`,
which fails identically on origin/main and touches no crate changed here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@h4x0r
h4x0r force-pushed the fix/panic-lints-and-csv-guard branch from be31975 to 4315d2d Compare August 6, 2026 02:13
@h4x0r
h4x0r marked this pull request as ready for review August 6, 2026 02:33
@h4x0r
h4x0r merged commit c345618 into main Aug 6, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant