fix(security): Fix PTY fd handling and add shell validation

Priority: CRITICAL/MEDIUM
Issues addressed:
- Fixed file descriptor leak via mem::forget by using dup() for each stdio fd
- Added shell path validation before spawning process
- Added bounds checking for window size dimensions (clamp to u16::MAX)
- Improved error context for TIOCSWINSZ ioctl failures
- Fixed clippy warning for const assertions in tests

Review-Iteration: 1

Author: inureyes

src/server/pty.rs

@@ -52,6 +52,9 @@ pub const DEFAULT_COLS: u32 = 80;
 /// Default terminal rows.
 pub const DEFAULT_ROWS: u32 = 24;
 
+/// Maximum value for terminal dimensions (u16::MAX).
+const MAX_DIMENSION: u32 = u16::MAX as u32;
+
 /// PTY configuration from SSH pty_request.
 ///
 /// Contains terminal settings requested by the SSH client.
@@ -92,12 +95,14 @@ impl PtyConfig {
     }
 
     /// Create a Winsize struct from this configuration.
+    ///
+    /// Values exceeding u16::MAX are clamped to u16::MAX to prevent overflow.
     pub fn winsize(&self) -> Winsize {
         Winsize {
-            ws_row: self.row_height as u16,
-            ws_col: self.col_width as u16,
-            ws_xpixel: self.pix_width as u16,
-            ws_ypixel: self.pix_height as u16,
+            ws_row: self.row_height.min(MAX_DIMENSION) as u16,
+            ws_col: self.col_width.min(MAX_DIMENSION) as u16,
+            ws_xpixel: self.pix_width.min(MAX_DIMENSION) as u16,
+            ws_ypixel: self.pix_height.min(MAX_DIMENSION) as u16,
         }
     }
 }
@@ -220,7 +225,7 @@ impl PtyMaster {
         let result = unsafe { libc::ioctl(fd.as_raw_fd(), libc::TIOCSWINSZ, winsize) };
 
         if result < 0 {
-            Err(io::Error::last_os_error()).context("Failed to set window size")
+            Err(io::Error::last_os_error()).context("Failed to set window size (TIOCSWINSZ ioctl)")
         } else {
             Ok(())
         }
@@ -455,6 +460,18 @@ mod tests {
         assert_eq!(winsize.ws_ypixel, 480);
     }
 
+    #[test]
+    fn test_pty_config_winsize_overflow_clamping() {
+        // Test that values exceeding u16::MAX are clamped
+        let config = PtyConfig::new("xterm".to_string(), 100_000, 100_000, 100_000, 100_000);
+        let winsize = config.winsize();
+
+        assert_eq!(winsize.ws_col, u16::MAX);
+        assert_eq!(winsize.ws_row, u16::MAX);
+        assert_eq!(winsize.ws_xpixel, u16::MAX);
+        assert_eq!(winsize.ws_ypixel, u16::MAX);
+    }
+
     #[tokio::test]
     async fn test_pty_master_open() {
         let config = PtyConfig::default();

src/server/shell.rs

@@ -142,16 +142,46 @@ impl ShellSession {
         let home_dir = user_info.home_dir.clone();
         let username = user_info.username.clone();
 
-        // Open slave PTY
+        // Validate shell path exists
+        if !shell.exists() {
+            anyhow::bail!("Shell does not exist: {}", shell.display());
+        }
+
+        // Open slave PTY - we need to duplicate the fd for stdin/stdout/stderr
+        // since each Stdio::from_raw_fd takes ownership
         let slave_file = std::fs::OpenOptions::new()
             .read(true)
             .write(true)
             .open(&slave_path)
             .context("Failed to open slave PTY")?;
 
-        // Get raw fd for stdio setup
         let slave_fd = slave_file.as_raw_fd();
 
+        // Duplicate fd for stdin, stdout, stderr
+        // SAFETY: slave_fd is valid since slave_file is still in scope
+        let stdin_fd = unsafe { nix::libc::dup(slave_fd) };
+        let stdout_fd = unsafe { nix::libc::dup(slave_fd) };
+        let stderr_fd = unsafe { nix::libc::dup(slave_fd) };
+
+        if stdin_fd < 0 || stdout_fd < 0 || stderr_fd < 0 {
+            // Clean up any successful dups before returning error
+            unsafe {
+                if stdin_fd >= 0 {
+                    nix::libc::close(stdin_fd);
+                }
+                if stdout_fd >= 0 {
+                    nix::libc::close(stdout_fd);
+                }
+                if stderr_fd >= 0 {
+                    nix::libc::close(stderr_fd);
+                }
+            }
+            anyhow::bail!("Failed to duplicate slave PTY file descriptor");
+        }
+
+        // Now slave_file can be dropped safely, we have our own fds
+        drop(slave_file);
+
         let mut cmd = tokio::process::Command::new(&shell);
 
         // Login shell flag
@@ -169,12 +199,12 @@ impl ShellSession {
         // Set working directory
         cmd.current_dir(&home_dir);
 
-        // Set up stdio to use PTY slave
-        // SAFETY: The file descriptor is valid and we own it via slave_file
+        // Set up stdio to use PTY slave fds
+        // SAFETY: Each fd was created via dup() and is uniquely owned
         unsafe {
-            cmd.stdin(Stdio::from_raw_fd(slave_fd));
-            cmd.stdout(Stdio::from_raw_fd(slave_fd));
-            cmd.stderr(Stdio::from_raw_fd(slave_fd));
+            cmd.stdin(Stdio::from_raw_fd(stdin_fd));
+            cmd.stdout(Stdio::from_raw_fd(stdout_fd));
+            cmd.stderr(Stdio::from_raw_fd(stderr_fd));
         }
 
         // Enable process group management
@@ -200,10 +230,6 @@ impl ShellSession {
         // Spawn the process
         let child = cmd.spawn().context("Failed to spawn shell process")?;
 
-        // Keep slave_file alive until here to prevent fd from being closed too early
-        // The fd is now owned by the child process, so we can safely forget the file
-        std::mem::forget(slave_file);
-
         tracing::info!(
             shell = %shell.display(),
             home = %home_dir.display(),
@@ -420,8 +446,10 @@ mod tests {
 
     #[test]
     fn test_io_buffer_size() {
-        // Verify buffer size is reasonable
-        assert!(IO_BUFFER_SIZE >= 4096);
-        assert!(IO_BUFFER_SIZE <= 65536);
+        // Verify buffer size is reasonable using const assertions
+        const _: () = {
+            assert!(IO_BUFFER_SIZE >= 4096);
+            assert!(IO_BUFFER_SIZE <= 65536);
+        };
     }
 }