From 5672b882e3869f566fb48ea96f2dbf717b802d64 Mon Sep 17 00:00:00 2001 From: "Graciliano M. P." Date: Mon, 29 Jun 2026 21:12:37 -0300 Subject: [PATCH] feat(parser): model fd-duplication & combined redirects; stop misclassifying innocuous redirections (1.4.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Innocuous shell redirections were misclassified. The bash tokenizer only knew `2>`/`2>>`, so `cmd 2>&1` produced a target-less `2>` redirection, a spurious "Redirection without a target" diagnostic, and a phantom command named `1` — whose `errorOutput` node falsely added a writeFilesystem capability. `1>&2` and `&>/dev/null` broke the same way, and even well-formed `>/dev/null` reported a file write. - AST: add RedirectionType.mergeStreams, combinedOutput, combinedAppendOutput. - Parsers (bash/posix, windows cmd, powershell): recognize fd-duplication (N>&M, >&-) and combined redirects (&>, &>>). Merges are self-contained: no consumed word, no phantom command, no diagnostic. - Capability detector: fd-merges add nothing; null-sink targets (/dev/null, NUL, $null) no longer add writeFilesystem/readFilesystem. Genuine targets (> out.txt, &> out.log) still do. - New BenignRedirectionDetector (default suite): records merges and null-sink discards as safe-level audit findings; decision-neutral. - DangerousOperatorDetector: recognize >& / &> as redirection sequences. - Bump to 1.4.0 + CHANGELOG; +24 tests across all parsers. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 31 +++++ lib/command_shield.dart | 1 + lib/src/ast/command_node.dart | 18 +++ lib/src/capabilities/capability_detector.dart | 23 +++- lib/src/parser/powershell_parser.dart | 95 ++++++++++++++- lib/src/parser/shell_parser.dart | 115 ++++++++++++++++-- lib/src/parser/windows_cmd_parser.dart | 99 ++++++++++++++- .../benign_redirection_detector.dart | 74 +++++++++++ .../dangerous_operator_detector.dart | 34 ++++++ lib/src/security/security_analyzer.dart | 2 + pubspec.yaml | 2 +- test/integration/pipeline_test.dart | 42 +++++++ test/unit/capabilities/capability_test.dart | 28 +++++ test/unit/parser/powershell_parser_test.dart | 16 +++ test/unit/parser/shell_parser_test.dart | 66 ++++++++++ test/unit/parser/windows_cmd_parser_test.dart | 17 +++ test/unit/security/security_test.dart | 20 +++ 17 files changed, 666 insertions(+), 17 deletions(-) create mode 100644 lib/src/security/detectors/benign_redirection_detector.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c51f2e..630c483 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,34 @@ +## 1.4.0 + +### Added + +- **File-descriptor redirections are modelled.** New `RedirectionType` cases + `mergeStreams` (`2>&1`, `1>&2`, `>&2`, `>&-`), `combinedOutput` (`&>file`, + `>&file`) and `combinedAppendOutput` (`&>>file`). All three shell parsers + (bash/POSIX, Windows `cmd`, PowerShell) now recognise these forms. +- **Benign-redirection marker.** A new `BenignRedirectionDetector` (code + `benign-redirection`, in the default suite) records stream merges and + null-sink discards as explicit `SecurityLevel.safe` findings for audit + visibility. Being below every actionable level, it never changes a decision. + +### Changed + +- Innocuous redirections no longer over-report a filesystem write. Stream + merges (`2>&1`) touch no file, and discards to a null sink (`/dev/null`, + `NUL`, `$null`) write/read no real file, so none of them contribute + `writeFilesystem`/`readFilesystem` (and thus no `modifyFiles` effect). A + genuine target such as `> out.txt` or `&> out.log` still does. + +### Fixed + +- **`2>&1` is no longer mis-parsed.** Previously the bash tokenizer only knew + `2>`/`2>>`, so `cmd 2>&1` produced a target-less `2>` redirection, a spurious + "Redirection without a target" diagnostic, and a phantom `CommandInvocation` + with executable `1`. `1>&2` and `&>/dev/null` broke the same way. These now + parse as a single clean command with the correct redirection and no + diagnostic. The discard-everything idiom `cmd >/dev/null 2>&1` parses as one + command with two redirections. + ## 1.3.0 ### Added diff --git a/lib/command_shield.dart b/lib/command_shield.dart index c87dad8..ea099f5 100644 --- a/lib/command_shield.dart +++ b/lib/command_shield.dart @@ -73,6 +73,7 @@ export 'src/policies/length_limit_policy.dart'; export 'src/policies/path_traversal_policy.dart'; export 'src/policies/risk_threshold_policy.dart'; export 'src/policies/shell_execution_policy.dart'; +export 'src/security/detectors/benign_redirection_detector.dart'; export 'src/security/detectors/command_substitution_detector.dart'; export 'src/security/detectors/dangerous_operator_detector.dart'; export 'src/security/detectors/destructive_command_detector.dart'; diff --git a/lib/src/ast/command_node.dart b/lib/src/ast/command_node.dart index ba51e6e..73398a2 100644 --- a/lib/src/ast/command_node.dart +++ b/lib/src/ast/command_node.dart @@ -31,6 +31,24 @@ enum RedirectionType { /// `2>>` — append standard error to a file. appendErrorOutput, + + /// File-descriptor duplication that merges one stream into another, e.g. + /// `2>&1` (stderr → stdout), `1>&2` (stdout → stderr), `>&2`, or `>&-` + /// (close fd). No file is written, so this is an innocuous no-op for the + /// filesystem. [RedirectionNode.target] holds the redirection as written + /// (e.g. `2>&1`). + /// + /// Only single-digit file descriptors are modelled; multi-digit fds such as + /// `12>&3` are not recognised as merges. + mergeStreams, + + /// `&>file` / `>&file` — truncate-and-write both stdout and stderr to a + /// file. Writes the file just like [output]. + combinedOutput, + + /// `&>>file` — append both stdout and stderr to a file. Writes the file + /// just like [appendOutput]. + combinedAppendOutput, } /// The root of the typed command abstract-syntax tree (AST). diff --git a/lib/src/capabilities/capability_detector.dart b/lib/src/capabilities/capability_detector.dart index f30bb2a..4e6de93 100644 --- a/lib/src/capabilities/capability_detector.dart +++ b/lib/src/capabilities/capability_detector.dart @@ -57,10 +57,29 @@ final class CapabilityDetector { case RedirectionType.appendOutput: case RedirectionType.errorOutput: case RedirectionType.appendErrorOutput: - out.add(CommandCapability.writeFilesystem); + case RedirectionType.combinedOutput: + case RedirectionType.combinedAppendOutput: + // Discarding a stream to a null sink (`>/dev/null`, `2>NUL`) writes no + // real file, so it grants no filesystem-write capability. + if (!_isNullSink(redir.target)) { + out.add(CommandCapability.writeFilesystem); + } case RedirectionType.input: case RedirectionType.hereDocument: - out.add(CommandCapability.readFilesystem); + if (!_isNullSink(redir.target)) { + out.add(CommandCapability.readFilesystem); + } + case RedirectionType.mergeStreams: + // Fd duplication (`2>&1`, `1>&2`) merges streams without touching the + // filesystem — an innocuous no-op for capability purposes. + break; } } + + /// Whether [target] is a discard sink that reads/writes no real file: + /// `/dev/null` (POSIX), `NUL`/`NUL:` (Windows), or `$null` (PowerShell). + static bool _isNullSink(String target) { + final t = target.toLowerCase(); + return t == '/dev/null' || t == 'nul' || t == 'nul:' || t == r'$null'; + } } diff --git a/lib/src/parser/powershell_parser.dart b/lib/src/parser/powershell_parser.dart index d55dc29..44aae90 100644 --- a/lib/src/parser/powershell_parser.dart +++ b/lib/src/parser/powershell_parser.dart @@ -47,6 +47,7 @@ enum _PsTokenType { newline, redirOut, redirAppend, + redirMerge, // fd duplication: 2>&1, 1>&2, >&1 } class _PsToken { @@ -67,9 +68,11 @@ class _PsToken { bool get isRedirection => type == _PsTokenType.redirOut || type == _PsTokenType.redirAppend; - RedirectionType get redirectionType => type == _PsTokenType.redirAppend - ? RedirectionType.appendOutput - : RedirectionType.output; + RedirectionType get redirectionType => switch (type) { + _PsTokenType.redirAppend => RedirectionType.appendOutput, + _PsTokenType.redirMerge => RedirectionType.mergeStreams, + _ => RedirectionType.output, + }; } class _PsTokenizer { @@ -121,7 +124,17 @@ class _PsTokenizer { _pos++; continue; case '>': - if (_peek(1) == '>') { + if (_peek(1) == '&') { + final gtStart = _pos; + _pos++; // move onto the `&` + final dup = _tryReadFdDup(gtStart, '>'); + if (dup != null) { + tokens.add(dup); + } else { + _pos++; // consume `&`; the filename follows as a word target. + tokens.add(_PsToken(_PsTokenType.redirOut, '>&', gtStart)); + } + } else if (_peek(1) == '>') { tokens.add(_PsToken(_PsTokenType.redirAppend, '>>', _pos)); _pos += 2; } else { @@ -129,6 +142,17 @@ class _PsTokenizer { _pos++; } continue; + case '0': + case '1': + case '2': + // A leading single-digit fd is a redirection only when immediately + // followed by `>` (e.g. `2>&1`); otherwise it is a normal word. + if (_peek(1) == '>') { + tokens.add(_readFdRedirect()); + continue; + } + tokens.add(_scanWord()); + continue; default: tokens.add(_scanWord()); } @@ -149,6 +173,58 @@ class _PsTokenizer { static final RegExp _nameStart = RegExp(r'[A-Za-z_]'); static final RegExp _nameChar = RegExp(r'[A-Za-z0-9_]'); + static bool _isDigit(String? ch) => + ch != null && ch.codeUnitAt(0) >= 0x30 && ch.codeUnitAt(0) <= 0x39; + + static bool _isFdBoundary(String? ch) => ch == null || _isTerminator(ch); + + /// Parses a redirection that begins with an explicit fd digit, with `_pos` + /// on the digit and `_peek(1) == '>'`. Handles fd duplication (`2>&1`), + /// append (`2>>`) and plain (`2>`) forms. The non-merge forms collapse to + /// output redirection. + _PsToken _readFdRedirect() { + final start = _pos; + final fd = input[_pos]; + if (_peek(2) == '&') { + _pos += 2; // move onto the `&` + final dup = _tryReadFdDup(start, '$fd>'); + if (dup != null) return dup; + _pos++; // consume `&`; the filename follows as a word target. + return _PsToken(_PsTokenType.redirOut, '$fd>&', start); + } + if (_peek(2) == '>') { + _pos += 3; + return _PsToken(_PsTokenType.redirAppend, '$fd>>', start); + } + _pos += 2; + return _PsToken(_PsTokenType.redirOut, '$fd>', start); + } + + /// With `_pos` on the `&` of a `…>&…` redirection, returns a + /// [_PsTokenType.redirMerge] token when a valid fd reference (digits or `-`) + /// terminated by a word boundary follows, consuming it. Returns `null` + /// otherwise, leaving `_pos` on the `&`. + _PsToken? _tryReadFdDup(int start, String prefix) { + final after = _peek(1); + if (after == '-') { + if (_isFdBoundary(_peek(2))) { + _pos += 2; // consume `&-` + return _PsToken(_PsTokenType.redirMerge, '$prefix&-', start); + } + return null; + } + if (!_isDigit(after)) return null; + var ahead = 1; + final digits = StringBuffer(); + while (_isDigit(_peek(ahead))) { + digits.write(_peek(ahead)); + ahead++; + } + if (!_isFdBoundary(_peek(ahead))) return null; + _pos += ahead; // consume `&` plus the fd digits + return _PsToken(_PsTokenType.redirMerge, '$prefix&$digits', start); + } + _PsToken _scanWord() { final start = _pos; final buffer = StringBuffer(); @@ -405,6 +481,17 @@ class _PsTokenParser { _i++; continue; } + if (tok.type == _PsTokenType.redirMerge) { + // Fd duplication (`2>&1`) carries its own target and consumes no word. + redirections.add( + RedirectionNode( + type: RedirectionType.mergeStreams, + target: tok.value, + ), + ); + _i++; + continue; + } if (tok.isRedirection) { _i++; if (!_atEnd && tokens[_i].type == _PsTokenType.word) { diff --git a/lib/src/parser/shell_parser.dart b/lib/src/parser/shell_parser.dart index af5e0e5..9485ff2 100644 --- a/lib/src/parser/shell_parser.dart +++ b/lib/src/parser/shell_parser.dart @@ -75,6 +75,9 @@ enum _TokenType { redirHereDoc, // << redirErr, // 2> redirErrAppend, // 2>> + redirMerge, // fd duplication: 2>&1, 1>&2, >&2, >&- + redirCombined, // &>file, >&file + redirCombinedAppend, // &>>file } class _Token { @@ -92,6 +95,9 @@ class _Token { final List substitutions; final List environmentReferences; + /// Redirection operators that take a following filename word as their target. + /// [_TokenType.redirMerge] is excluded: it is self-contained (the fd target + /// is embedded in its value) and is handled separately by the parser. bool get isRedirection => const { _TokenType.redirOut, _TokenType.redirAppend, @@ -99,6 +105,8 @@ class _Token { _TokenType.redirHereDoc, _TokenType.redirErr, _TokenType.redirErrAppend, + _TokenType.redirCombined, + _TokenType.redirCombinedAppend, }.contains(type); RedirectionType get redirectionType => switch (type) { @@ -108,6 +116,9 @@ class _Token { _TokenType.redirHereDoc => RedirectionType.hereDocument, _TokenType.redirErr => RedirectionType.errorOutput, _TokenType.redirErrAppend => RedirectionType.appendErrorOutput, + _TokenType.redirMerge => RedirectionType.mergeStreams, + _TokenType.redirCombined => RedirectionType.combinedOutput, + _TokenType.redirCombinedAppend => RedirectionType.combinedAppendOutput, _ => RedirectionType.output, }; } @@ -168,10 +179,28 @@ class _ShellTokenizer { _pos += 2; return _Token(_TokenType.and, '&&', start); } + // `&>`/`&>>` (bash): redirect both stdout and stderr to a file. + if (_peek(1) == '>') { + if (_peek(2) == '>') { + _pos += 3; + return _Token(_TokenType.redirCombinedAppend, '&>>', start); + } + _pos += 2; + return _Token(_TokenType.redirCombined, '&>', start); + } // A lone `&` (background) is treated as a sequential separator. _pos++; return _Token(_TokenType.semicolon, '&', start); case '>': + // `>&` is either fd duplication (`>&1`, `>&-`) or, when followed by a + // filename, the bash combined redirect `>&file`. + if (_peek(1) == '&') { + _pos++; // move onto the `&` + final dup = _tryReadFdDup(start, prefix: '>'); + if (dup != null) return dup; + _pos++; // consume the `&`; the filename follows as a word target. + return _Token(_TokenType.redirCombined, '>&', start); + } if (_peek(1) == '>') { _pos += 2; return _Token(_TokenType.redirAppend, '>>', start); @@ -185,21 +214,79 @@ class _ShellTokenizer { } _pos++; return _Token(_TokenType.redirIn, '<', start); + case '0': + case '1': case '2': - if (_peek(1) == '>') { - if (_peek(2) == '>') { - _pos += 3; - return _Token(_TokenType.redirErrAppend, '2>>', start); - } - _pos += 2; - return _Token(_TokenType.redirErr, '2>', start); - } + // A leading single-digit fd is an operator only when immediately + // followed by `>` (e.g. `2>`, `1>&2`); otherwise it is an ordinary + // word character handled by `_scanWord` (e.g. `2foo`, `0 > x`). + if (_peek(1) == '>') return _readFdRedirect(start, fd: ch); return null; default: return null; } } + /// Parses a file-descriptor redirection that begins with an explicit fd + /// digit, with `_pos` on the digit and `_peek(1) == '>'`. Handles fd + /// duplication (`2>&1`), append (`2>>`), and plain (`2>`) forms. + _Token _readFdRedirect(int start, {required String fd}) { + // `fd>&…` — duplication or `fd>&file` combined redirect. + if (_peek(2) == '&') { + _pos += 2; // move onto the `&` + final dup = _tryReadFdDup(start, prefix: '$fd>'); + if (dup != null) return dup; + _pos++; // consume the `&`; the filename follows as a word target. + return _Token(_TokenType.redirCombined, '$fd>&', start); + } + final isErr = fd == '2'; + if (_peek(2) == '>') { + _pos += 3; + return _Token( + isErr ? _TokenType.redirErrAppend : _TokenType.redirAppend, + '$fd>>', + start, + ); + } + _pos += 2; + return _Token( + isErr ? _TokenType.redirErr : _TokenType.redirOut, + '$fd>', + start, + ); + } + + /// With `_pos` on the `&` of a `…>&…` redirection, returns a [_TokenType + /// .redirMerge] token when a valid fd reference (digits or `-`) terminated by + /// a word boundary follows, consuming it. Returns `null` when what follows is + /// not a clean fd reference (e.g. `>&file`), leaving `_pos` unchanged. + _Token? _tryReadFdDup(int start, {required String prefix}) { + final after = _peek(1); + if (after == null) return null; + if (after == '-') { + if (_isFdBoundary(_peek(2))) { + _pos += 2; // consume `&-` + return _Token(_TokenType.redirMerge, '$prefix&-', start); + } + return null; + } + if (!_isDigit(after)) return null; + var ahead = 1; + final digits = StringBuffer(); + while (_isDigit(_peek(ahead))) { + digits.write(_peek(ahead)); + ahead++; + } + if (!_isFdBoundary(_peek(ahead))) return null; + _pos += ahead; // consume `&` plus the fd digits + return _Token(_TokenType.redirMerge, '$prefix&$digits', start); + } + + static bool _isDigit(String? ch) => + ch != null && ch.codeUnitAt(0) >= 0x30 && ch.codeUnitAt(0) <= 0x39; + + static bool _isFdBoundary(String? ch) => ch == null || _isWordTerminator(ch); + static bool _isWordTerminator(String ch) => ch == ' ' || ch == '\t' || @@ -566,6 +653,18 @@ class _TokenParser { _i++; continue; } + if (tok.type == _TokenType.redirMerge) { + // Fd duplication (`2>&1`, `>&-`, …) carries its own target; it does not + // consume a following word and is never target-less. + redirections.add( + RedirectionNode( + type: RedirectionType.mergeStreams, + target: tok.value, + ), + ); + _i++; + continue; + } if (tok.isRedirection) { _i++; if (!_atEnd && _current.type == _TokenType.word) { diff --git a/lib/src/parser/windows_cmd_parser.dart b/lib/src/parser/windows_cmd_parser.dart index 63104a5..e4f00d9 100644 --- a/lib/src/parser/windows_cmd_parser.dart +++ b/lib/src/parser/windows_cmd_parser.dart @@ -37,7 +37,17 @@ final class WindowsCmdParser extends CommandParser { } } -enum _CmdTokenType { word, pipe, and, or, amp, redirOut, redirAppend, redirIn } +enum _CmdTokenType { + word, + pipe, + and, + or, + amp, + redirOut, + redirAppend, + redirIn, + redirMerge, // fd duplication: 2>&1, 1>&2, >&1 +} class _CmdToken { _CmdToken( @@ -60,6 +70,7 @@ class _CmdToken { RedirectionType get redirectionType => switch (type) { _CmdTokenType.redirAppend => RedirectionType.appendOutput, _CmdTokenType.redirIn => RedirectionType.input, + _CmdTokenType.redirMerge => RedirectionType.mergeStreams, _ => RedirectionType.output, }; } @@ -103,7 +114,17 @@ class _CmdTokenizer { } continue; case '>': - if (_peek(1) == '>') { + if (_peek(1) == '&') { + final gtStart = _pos; + _pos++; // move onto the `&` + final dup = _tryReadFdDup(gtStart, '>'); + if (dup != null) { + tokens.add(dup); + } else { + _pos++; // consume `&`; the filename follows as a word target. + tokens.add(_CmdToken(_CmdTokenType.redirOut, '>&', gtStart)); + } + } else if (_peek(1) == '>') { tokens.add(_CmdToken(_CmdTokenType.redirAppend, '>>', _pos)); _pos += 2; } else { @@ -115,6 +136,17 @@ class _CmdTokenizer { tokens.add(_CmdToken(_CmdTokenType.redirIn, '<', _pos)); _pos++; continue; + case '0': + case '1': + case '2': + // A leading single-digit fd is a redirection only when immediately + // followed by `>` (e.g. `2>&1`); otherwise it is a normal word. + if (_peek(1) == '>') { + tokens.add(_readFdRedirect()); + continue; + } + tokens.add(_scanWord()); + continue; default: tokens.add(_scanWord()); } @@ -132,6 +164,58 @@ class _CmdTokenizer { ch == '>' || ch == '<'; + static bool _isDigit(String? ch) => + ch != null && ch.codeUnitAt(0) >= 0x30 && ch.codeUnitAt(0) <= 0x39; + + static bool _isFdBoundary(String? ch) => ch == null || _isTerminator(ch); + + /// Parses a redirection that begins with an explicit fd digit, with `_pos` + /// on the digit and `_peek(1) == '>'`. Handles fd duplication (`2>&1`), + /// append (`2>>`) and plain (`2>`) forms. `cmd` does not model a separate + /// stderr stream, so non-merge forms collapse to output redirection. + _CmdToken _readFdRedirect() { + final start = _pos; + final fd = input[_pos]; + if (_peek(2) == '&') { + _pos += 2; // move onto the `&` + final dup = _tryReadFdDup(start, '$fd>'); + if (dup != null) return dup; + _pos++; // consume `&`; the filename follows as a word target. + return _CmdToken(_CmdTokenType.redirOut, '$fd>&', start); + } + if (_peek(2) == '>') { + _pos += 3; + return _CmdToken(_CmdTokenType.redirAppend, '$fd>>', start); + } + _pos += 2; + return _CmdToken(_CmdTokenType.redirOut, '$fd>', start); + } + + /// With `_pos` on the `&` of a `…>&…` redirection, returns a + /// [_CmdTokenType.redirMerge] token when a valid fd reference (digits or `-`) + /// terminated by a word boundary follows, consuming it. Returns `null` + /// otherwise, leaving `_pos` on the `&`. + _CmdToken? _tryReadFdDup(int start, String prefix) { + final after = _peek(1); + if (after == '-') { + if (_isFdBoundary(_peek(2))) { + _pos += 2; // consume `&-` + return _CmdToken(_CmdTokenType.redirMerge, '$prefix&-', start); + } + return null; + } + if (!_isDigit(after)) return null; + var ahead = 1; + final digits = StringBuffer(); + while (_isDigit(_peek(ahead))) { + digits.write(_peek(ahead)); + ahead++; + } + if (!_isFdBoundary(_peek(ahead))) return null; + _pos += ahead; // consume `&` plus the fd digits + return _CmdToken(_CmdTokenType.redirMerge, '$prefix&$digits', start); + } + _CmdToken _scanWord() { final start = _pos; final buffer = StringBuffer(); @@ -247,6 +331,17 @@ class _CmdTokenParser { _i++; continue; } + if (tok.type == _CmdTokenType.redirMerge) { + // Fd duplication (`2>&1`) carries its own target and consumes no word. + redirections.add( + RedirectionNode( + type: RedirectionType.mergeStreams, + target: tok.value, + ), + ); + _i++; + continue; + } if (tok.isRedirection) { _i++; if (!_atEnd && tokens[_i].type == _CmdTokenType.word) { diff --git a/lib/src/security/detectors/benign_redirection_detector.dart b/lib/src/security/detectors/benign_redirection_detector.dart new file mode 100644 index 0000000..ce7bf35 --- /dev/null +++ b/lib/src/security/detectors/benign_redirection_detector.dart @@ -0,0 +1,74 @@ +import '../../ast/command_node.dart'; +import '../security_detector.dart'; +import '../security_finding.dart'; +import '../security_level.dart'; + +/// Records innocuous I/O redirections as explicit, [SecurityLevel.safe] +/// findings so they are visible for auditing without affecting the decision. +/// +/// Two redirection shapes are benign no-ops for the filesystem: +/// +/// * **Stream merges** — file-descriptor duplication such as `2>&1`, `1>&2` or +/// `>&2`, which wire one stream into another and write no file. +/// * **Null-sink discards** — redirecting to `/dev/null`, `NUL` or `$null`, +/// which throws the stream away. +/// +/// These commonly trail otherwise-ordinary commands (`cmd 2>&1`, +/// `cmd >/dev/null 2>&1`); surfacing them as `safe` keeps the audit trail +/// honest while guaranteeing they never push a command toward REVIEW/DENY +/// (`safe` is below every actionable level). +final class BenignRedirectionDetector extends SecurityDetector { + /// Creates the detector. + const BenignRedirectionDetector(); + + @override + String get code => 'benign-redirection'; + + @override + List detect(SecurityContext context) { + final findings = []; + for (final inv in context.invocations) { + for (final redir in inv.redirections) { + final description = _benignDescription(redir); + if (description == null) continue; + findings.add( + SecurityFinding( + level: SecurityLevel.safe, + message: description, + code: code, + ), + ); + } + } + return findings; + } + + /// A human-readable note when [redir] is a benign no-op, otherwise `null`. + static String? _benignDescription(RedirectionNode redir) { + switch (redir.type) { + case RedirectionType.mergeStreams: + return 'Benign redirection "${redir.target}" ' + '(stream merge); writes no file.'; + case RedirectionType.output: + case RedirectionType.appendOutput: + case RedirectionType.errorOutput: + case RedirectionType.appendErrorOutput: + case RedirectionType.combinedOutput: + case RedirectionType.combinedAppendOutput: + case RedirectionType.input: + case RedirectionType.hereDocument: + if (_isNullSink(redir.target)) { + return 'Benign redirection to null sink "${redir.target}"; ' + 'discards the stream.'; + } + return null; + } + } + + /// Whether [target] is a discard sink that reads/writes no real file: + /// `/dev/null` (POSIX), `NUL`/`NUL:` (Windows), or `$null` (PowerShell). + static bool _isNullSink(String target) { + final t = target.toLowerCase(); + return t == '/dev/null' || t == 'nul' || t == 'nul:' || t == r'$null'; + } +} diff --git a/lib/src/security/detectors/dangerous_operator_detector.dart b/lib/src/security/detectors/dangerous_operator_detector.dart index 408de8f..e2813a7 100644 --- a/lib/src/security/detectors/dangerous_operator_detector.dart +++ b/lib/src/security/detectors/dangerous_operator_detector.dart @@ -46,6 +46,24 @@ final class DangerousOperatorDetector extends SecurityDetector { i += 2; continue; } + // `&>`/`&>>` (bash): redirect both streams to a file. Treat as a + // redirection, not chaining, and consume the `>` so it is not + // re-scanned as a separate output redirection. + if (next == '>') { + if (i + 2 < n && s[i + 2] == '>') { + add( + '&>>', + SecurityLevel.lowRisk, + 'Combined-append redirection', + i, + ); + i += 3; + } else { + add('&>', SecurityLevel.lowRisk, 'Combined redirection', i); + i += 2; + } + continue; + } case '|': if (next == '|') { add('||', SecurityLevel.mediumRisk, 'Conditional-OR chaining', i); @@ -56,6 +74,19 @@ final class DangerousOperatorDetector extends SecurityDetector { case ';': add(';', SecurityLevel.mediumRisk, 'Sequential chaining', i); case '>': + // `>&` is fd duplication (`2>&1`, `>&-`) or a combined redirect + // (`>&file`). Fd duplication writes no file and is innocuous — the + // benign-redirection detector records it — so emit nothing here. + if (next == '&') { + final afterAmp = i + 2 < n ? s[i + 2] : ''; + if (afterAmp == '-' || _isDigit(afterAmp)) { + i += 2; // skip `>&`; following fd digits are harmless chars + continue; + } + add('>&', SecurityLevel.lowRisk, 'Combined redirection', i); + i += 2; + continue; + } if (next == '>') { add('>>', SecurityLevel.lowRisk, 'Append redirection', i); i += 2; @@ -74,4 +105,7 @@ final class DangerousOperatorDetector extends SecurityDetector { } return findings; } + + static bool _isDigit(String ch) => + ch.length == 1 && ch.codeUnitAt(0) >= 0x30 && ch.codeUnitAt(0) <= 0x39; } diff --git a/lib/src/security/security_analyzer.dart b/lib/src/security/security_analyzer.dart index 8b0c161..9b4412c 100644 --- a/lib/src/security/security_analyzer.dart +++ b/lib/src/security/security_analyzer.dart @@ -1,5 +1,6 @@ import 'package:meta/meta.dart' show immutable; +import 'detectors/benign_redirection_detector.dart'; import 'detectors/command_substitution_detector.dart'; import 'detectors/dangerous_operator_detector.dart'; import 'detectors/destructive_command_detector.dart'; @@ -51,6 +52,7 @@ final class SecurityAnalyzer { RemoteExecDetector(), PathTraversalDetector(), EnvExpansionDetector(), + BenignRedirectionDetector(), ]; /// Runs all [detectors] over [context] and aggregates the result. diff --git a/pubspec.yaml b/pubspec.yaml index 699fa01..22fd3ca 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -3,7 +3,7 @@ description: >- Security-first command-line analysis: parse, normalize, classify, analyze and policy-validate shell commands into ALLOW / REVIEW / DENY decisions without ever executing them. Built for AI agents and sandboxed executors. -version: 1.3.0 +version: 1.4.0 homepage: https://github.com/OmnyGrid/command_shield repository: https://github.com/OmnyGrid/command_shield issue_tracker: https://github.com/OmnyGrid/command_shield/issues diff --git a/test/integration/pipeline_test.dart b/test/integration/pipeline_test.dart index 0fc24e9..45a16b8 100644 --- a/test/integration/pipeline_test.dart +++ b/test/integration/pipeline_test.dart @@ -150,6 +150,48 @@ void main() { }); }); + group('innocuous redirections', () { + test('ls -la 2>&1 stays read-only and allowed', () { + final analysis = shield.analyze('ls -la 2>&1'); + final result = shield.validate('ls -la 2>&1'); + expect(analysis.effects, isNot(contains(CommandEffect.modifyFiles))); + expect(analysis.isReadOnly, isTrue); + expect(result.decision, CommandDecision.allow); + expect( + analysis.findings.any((f) => f.code == 'benign-redirection'), + isTrue, + ); + }); + + test('grep x f > /dev/null is allowed with a benign marker', () { + final analysis = shield.analyze('grep x f > /dev/null'); + expect(analysis.effects, isNot(contains(CommandEffect.modifyFiles))); + expect( + shield.validate('grep x f > /dev/null').decision, + CommandDecision.allow, + ); + expect( + analysis.findings.any((f) => f.code == 'benign-redirection'), + isTrue, + ); + }); + + test('discard-everything idiom > /dev/null 2>&1 is allowed', () { + final analysis = shield.analyze('echo hi > /dev/null 2>&1'); + expect(analysis.effects, isNot(contains(CommandEffect.modifyFiles))); + expect( + shield.validate('echo hi > /dev/null 2>&1').decision, + CommandDecision.allow, + ); + }); + + test('redirection suppression does not mask the command risk', () { + // `rm -rf` is still dangerous even when its output is discarded. + final result = shield.validate('rm -rf /tmp/x > /dev/null 2>&1'); + expect(result.decision, isNot(CommandDecision.allow)); + }); + }); + group('custom policy wiring', () { test('custom allow-list policy overrides default', () { final shield = CommandShield( diff --git a/test/unit/capabilities/capability_test.dart b/test/unit/capabilities/capability_test.dart index afa9a4d..b0880c9 100644 --- a/test/unit/capabilities/capability_test.dart +++ b/test/unit/capabilities/capability_test.dart @@ -108,6 +108,34 @@ void main() { ); }); + test('stream merge 2>&1 implies no filesystem write', () { + // `ls` reads the filesystem; the merge itself must add nothing. + final c = caps('ls 2>&1'); + expect(c, isNot(contains(CommandCapability.writeFilesystem))); + }); + + test('discard to /dev/null implies no filesystem write', () { + expect( + caps('echo hi > /dev/null'), + isNot(contains(CommandCapability.writeFilesystem)), + ); + expect( + caps('cmd 2> /dev/null'), + isNot(contains(CommandCapability.writeFilesystem)), + ); + expect( + caps('cmd &> /dev/null'), + isNot(contains(CommandCapability.writeFilesystem)), + ); + }); + + test('combined redirect to a real file still implies write', () { + expect( + caps('cmd &> out.log'), + contains(CommandCapability.writeFilesystem), + ); + }); + test('command substitution implies execute', () { expect( caps(r'echo $(whoami)'), diff --git a/test/unit/parser/powershell_parser_test.dart b/test/unit/parser/powershell_parser_test.dart index e9a8f6f..06d4af2 100644 --- a/test/unit/parser/powershell_parser_test.dart +++ b/test/unit/parser/powershell_parser_test.dart @@ -68,6 +68,22 @@ void main() { expect(inv.redirections.first.type, RedirectionType.output); }); + test('error-to-output merge 2>&1', () { + final result = parser.parse('Get-Item x 2>&1'); + final inv = result.ast! as CommandInvocation; + expect(inv.executable, 'Get-Item'); + expect(inv.arguments, ['x']); // no phantom `1` command + expect(inv.redirections.first.type, RedirectionType.mergeStreams); + expect(inv.redirections.first.target, '2>&1'); + }); + + test(r'discard to $null implies no filesystem write', () { + expect( + CapabilityDetector().detect(ast(r'Get-Item x > $null')), + isNot(contains(CommandCapability.writeFilesystem)), + ); + }); + test('empty input yields no AST', () { expect(parser.parse('').ast, isNull); }); diff --git a/test/unit/parser/shell_parser_test.dart b/test/unit/parser/shell_parser_test.dart index defaf29..418fc2d 100644 --- a/test/unit/parser/shell_parser_test.dart +++ b/test/unit/parser/shell_parser_test.dart @@ -134,6 +134,72 @@ void main() { isTrue, ); }); + + test('stderr-to-stdout merge 2>&1 is a single clean redirection', () { + final result = bash.parse('cmd 2>&1'); + final inv = result.ast! as CommandInvocation; + // No phantom `1` command, no spurious diagnostic. + expect(inv.executable, 'cmd'); + expect(inv.arguments, isEmpty); + expect(result.diagnostics, isEmpty); + expect(inv.redirections, hasLength(1)); + expect(inv.redirections.first.type, RedirectionType.mergeStreams); + expect(inv.redirections.first.target, '2>&1'); + }); + + test('stdout-to-stderr merge 1>&2', () { + final inv = ast('cmd 1>&2') as CommandInvocation; + expect(inv.executable, 'cmd'); + expect(inv.redirections.first.type, RedirectionType.mergeStreams); + expect(inv.redirections.first.target, '1>&2'); + }); + + test('bare >&2 merge', () { + final inv = ast('cmd >&2') as CommandInvocation; + expect(inv.redirections.first.type, RedirectionType.mergeStreams); + expect(inv.redirections.first.target, '>&2'); + }); + + test('close-fd >&- merge', () { + final inv = ast('cmd >&-') as CommandInvocation; + expect(inv.redirections.first.type, RedirectionType.mergeStreams); + expect(inv.redirections.first.target, '>&-'); + }); + + test('combined redirect &>file', () { + final inv = ast('cmd &> out.log') as CommandInvocation; + expect(inv.redirections.first.type, RedirectionType.combinedOutput); + expect(inv.redirections.first.target, 'out.log'); + }); + + test('combined append &>>file', () { + final inv = ast('cmd &>> out.log') as CommandInvocation; + expect(inv.redirections.first.type, RedirectionType.combinedAppendOutput); + expect(inv.redirections.first.target, 'out.log'); + }); + + test('discard to /dev/null parses with the real target', () { + final inv = ast('cmd 2> /dev/null') as CommandInvocation; + expect(inv.redirections.first.type, RedirectionType.errorOutput); + expect(inv.redirections.first.target, '/dev/null'); + }); + + test('canonical discard-everything idiom >/dev/null 2>&1', () { + final result = bash.parse('cmd > /dev/null 2>&1'); + final inv = result.ast! as CommandInvocation; + expect(inv.executable, 'cmd'); + expect(result.diagnostics, isEmpty); + expect(inv.redirections, hasLength(2)); + expect(inv.redirections[0].type, RedirectionType.output); + expect(inv.redirections[0].target, '/dev/null'); + expect(inv.redirections[1].type, RedirectionType.mergeStreams); + }); + + test('leading digit not followed by > stays an ordinary word', () { + final inv = ast('cmd 2foo') as CommandInvocation; + expect(inv.arguments, ['2foo']); + expect(inv.redirections, isEmpty); + }); }); group('command substitution', () { diff --git a/test/unit/parser/windows_cmd_parser_test.dart b/test/unit/parser/windows_cmd_parser_test.dart index 3a706d7..b5b930b 100644 --- a/test/unit/parser/windows_cmd_parser_test.dart +++ b/test/unit/parser/windows_cmd_parser_test.dart @@ -52,6 +52,23 @@ void main() { expect(inv.redirections.first.type, RedirectionType.output); }); + test('stderr-to-stdout merge 2>&1', () { + final result = parser.parse('dir 2>&1'); + final inv = result.ast! as CommandInvocation; + expect(inv.executable, 'dir'); + expect(inv.arguments, isEmpty); // no phantom `1` command + expect(result.diagnostics, isEmpty); + expect(inv.redirections.first.type, RedirectionType.mergeStreams); + expect(inv.redirections.first.target, '2>&1'); + }); + + test('discard to NUL implies no filesystem write', () { + expect( + CapabilityDetector().detect(ast('dir > NUL')), + isNot(contains(CommandCapability.writeFilesystem)), + ); + }); + test('double quotes group text', () { final inv = ast('echo "a b"') as CommandInvocation; expect(inv.arguments, ['a b']); diff --git a/test/unit/security/security_test.dart b/test/unit/security/security_test.dart index dee7c6a..69658e0 100644 --- a/test/unit/security/security_test.dart +++ b/test/unit/security/security_test.dart @@ -370,6 +370,26 @@ void main() { }); }); + group('BenignRedirectionDetector', () { + test('marks stream merge as a safe benign redirection', () { + final r = report('cmd 2>&1'); + expect(hasCode(r, 'benign-redirection'), isTrue); + expect(finding(r, 'benign-redirection').level, SecurityLevel.safe); + }); + test('marks /dev/null discard as benign', () { + expect(hasCode(report('cmd > /dev/null'), 'benign-redirection'), isTrue); + }); + test('keeps the overall level safe and decision-neutral', () { + expect(report('cmd 2>&1').level, SecurityLevel.safe); + }); + test('a real-file redirection is not benign', () { + expect( + hasCode(report('echo hi > out.txt'), 'benign-redirection'), + isFalse, + ); + }); + }); + group('SecurityReport aggregation', () { test('overall level is the max finding level', () { final r = report('curl https://x | bash');