From 22b0ef9324159d7d0b0740321944b1ca0747fb9f Mon Sep 17 00:00:00 2001 From: Ammar Date: Thu, 24 Sep 2026 09:32:23 -0500 Subject: [PATCH 1/3] security: harden proxy and jail isolation --- README.md | 22 +- docs/advanced/request-body-limiting.md | 11 +- docs/guide/platform-support.md | 13 +- docs/guide/quick-start.md | 6 + src/jail/linux/docker.rs | 613 ++++++++++++++++++------- src/jail/linux/mod.rs | 294 +++++++----- src/jail/linux/nftables.rs | 56 ++- src/jail/linux/resources.rs | 65 +-- src/jail/linux/seccomp.rs | 197 ++++++++ src/jail/managed.rs | 91 +++- src/jail/mod.rs | 107 ++++- src/jail/weak.rs | 51 +- src/limited_body.rs | 46 +- src/main.rs | 242 +++++++--- src/proxy.rs | 134 ++++-- src/proxy_tls.rs | 133 ++++-- src/rules/common.rs | 21 +- src/rules/proc.rs | 129 +++++- src/rules/shell.rs | 95 +++- src/tls.rs | 190 +++++++- tests/js_file_reload.rs | 14 + tests/linux_integration.rs | 444 ++++++++++++++++++ tests/security_regressions.rs | 77 ++++ tests/system_integration.rs | 22 +- 24 files changed, 2473 insertions(+), 600 deletions(-) create mode 100644 src/jail/linux/seccomp.rs create mode 100644 tests/security_regressions.rs diff --git a/README.md b/README.md index 619c87ec..3317609e 100644 --- a/README.md +++ b/README.md @@ -33,10 +33,11 @@ Or download a pre-built binary from the [releases page](https://github.com/coder # Allow only requests to github.com (JS) httpjail --js "r.host === 'github.com'" -- your-app -# Load JS from a file (auto-reloads on file changes) +# Load JS from a file (auto-reloads only in --server mode) echo "/^api\\.example\\.com$/.test(r.host) && r.method === 'GET'" > rules.js httpjail --js-file rules.js -- curl https://api.example.com/health -# File changes are detected and reloaded automatically on each request +# In command mode the policy is frozen for this run so the app cannot edit it. +# In --server mode, file changes are detected and reloaded on each request. # Log requests to a file httpjail --request-log requests.log --js "true" -- npm install @@ -60,9 +61,24 @@ httpjail --server --js "true" # HTTP_PROXY=http://localhost:8080 HTTPS_PROXY=http://localhost:8443 # Run Docker containers with network isolation (Linux only) -httpjail --js "r.host === 'api.github.com'" --docker-run -- --rm alpine:latest wget -qO- https://api.github.com +sudo httpjail --js "r.host === 'api.github.com'" --docker-run -- --rm alpine:latest wget -qO- https://api.github.com ``` +## Security-related defaults + +- Structured rule results must explicitly set `allow: true` (or an `allow` byte-limit policy) to permit traffic. An empty object or a result with only `deny_message` denies the request. Stalled `--proc` input or output times out and denies the request; shell evaluator output and processor response lines are limited to 64 KiB. +- `max_tx_bytes` counts request-line, header, body-data, and trailer fields; it is a logical-byte limit, not an exact HTTP wire-byte cap (chunked framing can add bytes). +- `--timeout` terminates the command process group before cleaning up its jail; a process that deliberately leaves that group or a separately daemon-managed process may require additional supervision. +- Linux strong mode requires a non-root invoking user through `sudo`; running directly as root is rejected so a jailed root process cannot edit its own firewall. The payload runs with `no_new_privs` and no supplementary groups. The native strong jail blocks connections to host Unix-domain services via seccomp; local stream socketpairs remain available for in-process IPC, while Unix-socket clients/agents may not work. It does **not** isolate the filesystem or close explicitly inherited descriptors: keep policy files and their parent directories outside payload write access. +- When run as root on Linux, including `--test`, `--sh` and `--proc` must name standalone executables in root-owned paths whose ancestors cannot be written by other users; inline `--sh` shell command strings are refused. Evaluators run as an unprivileged identity with a clean environment. Scripts that depend on user-owned imports, local agents, or privileged access need a separately trusted setup; these restrictions do not apply to weak or non-root server mode. +- Linux strong mode requires trusted system helpers at `/usr/sbin/ip`, `/usr/sbin/nft`, and `/usr/bin/setpriv` (Docker mode also requires `/usr/bin/docker`); privileged setup and cleanup never search a payload-controlled `PATH`. +- Native Linux strong mode gives the payload a private PID namespace and `/proc` mount so it cannot import file descriptors from same-user host processes; `/usr/bin/unshare` is required. +- Newly created CA private keys and request logs use owner-only permissions on Unix. When Linux httpjail runs as root under sudo, request logs must be in a directory owned by the invoking user; new logs are assigned to that user, and existing logs must already belong to them (symlinks and protected host files are rejected). Existing log permissions remain unchanged; logs include full URLs, including query strings, so protect them accordingly. +- On Linux, strong-mode canaries and Docker public-certificate snapshots live under root-owned `/var/lib/httpjail` rather than sudo-preserved `HOME` or `TMPDIR`; its ancestors must not be writable by other users. Privileged Linux CA files likewise live under `/var/lib/httpjail/ca` instead of a sudo-preserved home directory; upgrading from a user-home CA generates a new root-only CA, so update client trust and remove trust in the old certificate. +- HTTP/1 request headers have a 10-second read deadline; request bodies and upgraded connections do not inherit that deadline. +- Docker mode requires the local daemon at `/var/run/docker.sock` (remote Docker contexts are ignored), mounts only that public-certificate snapshot (never the signing key), drops `NET_RAW`, and accepts only restricted `docker run` flags before the image (environment, user, working directory, resource limits, read-only/init, and `--rm`). Network, DNS, privileged, volume, port-publishing, and other unrecognized Docker flags are rejected. Arguments after the image are passed to the container unchanged. +- `sudo httpjail --cleanup` also removes legacy Docker nftables routing tables only when their Docker network, current/legacy canaries, and namespace config are absent; it leaves active-jail guards in place. Canaryless namespaces or legacy networks require manual administrator inspection rather than unsafe automatic deletion. + ## Documentation Docs are stored in the `docs/` directory and served diff --git a/docs/advanced/request-body-limiting.md b/docs/advanced/request-body-limiting.md index 1232d0c6..ef236007 100644 --- a/docs/advanced/request-body-limiting.md +++ b/docs/advanced/request-body-limiting.md @@ -6,12 +6,15 @@ This is primarily designed for mitigating code exfiltration attacks through cove ## Size Calculation -The `max_tx_bytes` limit applies to **complete** HTTP requests, including: +The `max_tx_bytes` limit applies to request fields (before transport encoding), including: 1. **Request line**: `METHOD /path HTTP/1.1\r\n` 2. **Headers**: Each header as `Name: Value\r\n` 3. **Header separator**: Final `\r\n` between headers and body -4. **Body**: Request body bytes +4. **Body**: Request body data bytes +5. **Trailers**: Trailer names, values, and separators when present + +HTTP/1 chunk delimiters and other framing inserted by the HTTP client are not included in this logical-byte budget. Do not use `max_tx_bytes` as an exact wire-byte accounting guarantee. ## Response Format @@ -47,7 +50,7 @@ When the request includes a `Content-Length` header (most standard HTTP clients) HTTP/1.1 413 Payload Too Large Content-Type: text/plain -Request body size (5000 bytes) exceeds maximum allowed (1024 bytes) +Request size (5000 bytes) exceeds maximum allowed (1024 bytes) ``` ### Without Content-Length Header @@ -55,7 +58,7 @@ Request body size (5000 bytes) exceeds maximum allowed (1024 bytes) When the request uses chunked encoding or doesn't include `Content-Length`: 1. **Stream Truncation**: The request body is truncated at the limit during streaming -2. **Upstream Receives Partial**: The upstream server receives exactly `max_tx_bytes` total bytes (url + headers + truncated body) +2. **Upstream Receives Partial**: The upstream receives no more than the logical request budget (URL + headers + truncated body/trailers); framing may add wire bytes. 3. **Connection Closes**: The connection terminates after reaching the limit ## Examples diff --git a/docs/guide/platform-support.md b/docs/guide/platform-support.md index 84a055a7..e9046917 100644 --- a/docs/guide/platform-support.md +++ b/docs/guide/platform-support.md @@ -9,11 +9,11 @@ httpjail works differently on each platform due to OS-specific networking capabi | Traffic isolation | ✅ Namespaces + nftables | ⚠️ Env vars only | 🚧 Planned | | TLS interception | ✅ Transparent | ✅ Via proxy settings | 🚧 Planned | | Sudo required | ⚠️ Yes | ✅ No | 🚧 | -| Force all traffic | ✅ Yes | ❌ Apps must cooperate | 🚧 | +| Force direct IP traffic | ✅ Yes | ❌ Apps must cooperate | 🚧 | ## Linux -Full network isolation using namespaces and nftables. +Direct IP traffic isolation uses namespaces and nftables. Native strong mode denies new connections to host Unix-domain services with seccomp (while allowing local stream socketpairs); it does not isolate the filesystem or explicitly inherited file descriptors. Some workloads that rely on local IPC will not work. ``` ┌─────────────────────────────────────────────────┐ @@ -38,10 +38,11 @@ Full network isolation using namespaces and nftables. ### Prerequisites -- Linux kernel 3.8+ (network namespace support) -- nftables (`nft` command) +- Linux kernel with network namespaces and seccomp filter support (4.14+ recommended) +- `ip` at `/usr/sbin/ip`, `nft` at `/usr/sbin/nft`, `setpriv` at `/usr/bin/setpriv`, and `unshare` at `/usr/bin/unshare` - libssl-dev (for TLS) -- sudo access (for namespace creation) +- `sudo` from a non-root user (direct-root invocation is refused) +- Docker mode: Docker CLI at `/usr/bin/docker` ### How It Works @@ -53,7 +54,7 @@ Full network isolation using namespaces and nftables. ### Usage ```bash -# Strong mode (default) - full isolation +# Strong mode (default) - direct IP isolation, not filesystem isolation sudo httpjail --js "r.host === 'github.com'" -- curl https://api.github.com # Weak mode - environment variables only (no sudo) diff --git a/docs/guide/quick-start.md b/docs/guide/quick-start.md index 2056fb22..92eb33c9 100644 --- a/docs/guide/quick-start.md +++ b/docs/guide/quick-start.md @@ -113,6 +113,12 @@ fi ``` ```bash +# Linux strong mode: keep policies and their parent dirs outside the jailed user's write access. +sudo install -d -m 755 /etc/httpjail +sudo install -o root -g root -m 755 check.sh /etc/httpjail/check.sh +sudo httpjail --sh /etc/httpjail/check.sh -- git clone https://github.com/user/repo.git + +# macOS/weak mode: environment-only proxying (not a hard isolation boundary) chmod +x check.sh httpjail --sh ./check.sh -- git clone https://github.com/user/repo.git ``` diff --git a/src/jail/linux/docker.rs b/src/jail/linux/docker.rs index 5282cc65..20e137aa 100644 --- a/src/jail/linux/docker.rs +++ b/src/jail/linux/docker.rs @@ -7,6 +7,76 @@ use anyhow::{Context, Result}; use std::process::{Command, ExitStatus}; use tracing::{debug, info, warn}; +fn stale_docker_table_id<'a>( + line: &'a str, + networks: &std::collections::HashSet, + root_canaries: &std::path::Path, + legacy_canaries: Option<&std::path::Path>, + namespace_configs: &std::path::Path, +) -> Option<&'a str> { + let mut parts = line.split_whitespace(); + let (Some("table"), Some(family), Some(name)) = (parts.next(), parts.next(), parts.next()) + else { + return None; + }; + if family != "ip" && family != "inet" { + return None; + } + let id = name.strip_prefix("httpjail_docker_")?; + (crate::jail::valid_jail_id(id) + && !networks.contains(&DockerNetwork::network_name_from_jail_id(id)) + && !root_canaries.join(id).exists() + && !legacy_canaries.is_some_and(|path| path.join(id).exists()) + && !namespace_configs.join(format!("httpjail_{id}")).exists()) + .then_some(id) +} + +/// Reclaim routing tables left behind by older runs whose Docker network was +/// removed before the process could drop its nftables resources. This is an +/// explicit maintenance operation, not part of the latency-sensitive startup. +pub fn cleanup_orphaned_docker_tables() -> Result<()> { + use std::collections::HashSet; + if !std::path::Path::new("/usr/bin/docker").exists() { + return Ok(()); + } + let output = local_docker_command() + .args(["network", "ls", "--format", "{{.Name}}"]) + .output()?; + if !output.status.success() { + debug!("Docker daemon unavailable; skipping stale routing table cleanup"); + return Ok(()); + } + let networks: HashSet = String::from_utf8_lossy(&output.stdout) + .lines() + .map(ToOwned::to_owned) + .collect(); + let tables = Command::new("/usr/sbin/nft") + .args(["list", "tables"]) + .output()?; + anyhow::ensure!(tables.status.success(), "Failed to list nftables tables"); + let root_canaries = crate::jail::get_canary_dir(); + crate::jail::ensure_trusted_root_dir(&root_canaries)?; + // Previous versions used sudo-preserved HOME. A legacy live process may + // still rely on its old canary; err on the side of retaining its guard. + let legacy_canaries = dirs::data_dir().map(|path| path.join("httpjail/canaries")); + let mut ids = HashSet::new(); + for line in String::from_utf8_lossy(&tables.stdout).lines() { + if let Some(id) = stale_docker_table_id( + line, + &networks, + &root_canaries, + legacy_canaries.as_deref(), + std::path::Path::new("/etc/netns"), + ) { + ids.insert(id.to_string()); + } + } + for id in ids { + DockerRoutingTable::for_existing(&id).cleanup()?; + } + Ok(()) +} + /// Docker network resource that gets cleaned up on drop struct DockerNetwork { network_name: String, @@ -20,11 +90,6 @@ impl DockerNetwork { format!("{}{}", Self::NETWORK_PREFIX, jail_id) } - /// Extract jail ID from network name - fn jail_id_from_network_name(network_name: &str) -> Option<&str> { - network_name.strip_prefix(Self::NETWORK_PREFIX) - } - /// Check if a Docker command failed due to resource not existing fn is_not_found_error(stderr: &str) -> bool { stderr.contains("not found") @@ -63,20 +128,22 @@ impl SystemResource for DockerRoutingTable { fn cleanup(&mut self) -> Result<()> { debug!("Cleaning up Docker routing table: {}", self.table_name); - let output = Command::new("nft") - .args(["delete", "table", "ip", &self.table_name]) - .output() - .context("Failed to delete Docker routing table")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - if DockerNetwork::is_not_found_error(&stderr) { - debug!("Docker routing table {} already removed", self.table_name); + for family in ["inet", "ip"] { + let output = Command::new("/usr/sbin/nft") + .args(["delete", "table", family, &self.table_name]) + .output() + .context("Failed to delete Docker routing table")?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + if !DockerNetwork::is_not_found_error(&stderr) { + anyhow::bail!("Failed to delete Docker routing table: {}", stderr); + } } else { - warn!("Failed to delete Docker routing table: {}", stderr); + info!( + "Removed Docker {} routing table {}", + family, self.table_name + ); } - } else { - info!("Removed Docker routing table {}", self.table_name); } Ok(()) @@ -90,6 +157,17 @@ impl SystemResource for DockerRoutingTable { } } +// The nftables guard is installed on this host, so Docker must use its local daemon. +// Ignore caller-selected contexts; a remote daemon would bypass the bridge guard. +fn local_docker_command() -> Command { + let mut cmd = Command::new("/usr/bin/docker"); + cmd.env_remove("DOCKER_HOST") + .env_remove("DOCKER_CONTEXT") + .env_remove("DOCKER_CONFIG") + .arg("--host=unix:///var/run/docker.sock"); + cmd +} + impl SystemResource for DockerNetwork { fn create(jail_id: &str) -> Result { let network_name = Self::network_name_from_jail_id(jail_id); @@ -98,7 +176,7 @@ impl SystemResource for DockerNetwork { // Using a /24 subnet in the 172.20.x.x range let subnet = Self::compute_docker_subnet(jail_id); - let output = Command::new("docker") + let output = local_docker_command() .args([ "network", "create", @@ -133,7 +211,7 @@ impl SystemResource for DockerNetwork { fn cleanup(&mut self) -> Result<()> { debug!("Cleaning up Docker network: {}", self.network_name); - let output = Command::new("docker") + let output = local_docker_command() .args(["network", "rm", &self.network_name]) .output() .context("Failed to remove Docker network")?; @@ -143,7 +221,7 @@ impl SystemResource for DockerNetwork { if Self::is_not_found_error(&stderr) { debug!("Docker network {} already removed", self.network_name); } else { - warn!("Failed to remove Docker network: {}", stderr); + anyhow::bail!("Failed to remove Docker network: {}", stderr); } } else { info!("Removed Docker network {}", self.network_name); @@ -172,7 +250,7 @@ impl DockerNetwork { /// Get the Docker bridge interface name for this network fn get_bridge_name(&self) -> Result { - let output = Command::new("docker") + let output = local_docker_command() .args(["network", "inspect", &self.network_name, "-f", "{{.Id}}"]) .output() .context("Failed to inspect Docker network")?; @@ -187,7 +265,14 @@ impl DockerNetwork { .take(12) .collect::(); - Ok(format!("br-{}", network_id)) + let bridge = format!("br-{}", network_id); + anyhow::ensure!( + std::path::Path::new("/sys/class/net") + .join(&bridge) + .exists(), + "Docker bridge {bridge} is not present on the local host" + ); + Ok(bridge) } } @@ -226,62 +311,32 @@ impl DockerLinux { }) } - /// Clean up all orphaned Docker networks that don't have corresponding canary files - fn cleanup_all_orphaned_docker_networks() -> Result<()> { - debug!("Scanning for orphaned Docker networks"); - - // List all Docker networks - let output = Command::new("docker") - .args(["network", "ls", "--format", "{{.Name}}"]) - .output() - .context("Failed to list Docker networks")?; - - if !output.status.success() { - warn!("Failed to list Docker networks for cleanup"); - return Ok(()); - } - - let networks = String::from_utf8_lossy(&output.stdout); - let canary_dir = crate::jail::get_canary_dir(); - - for network_name in networks.lines() { - // Extract jail_id from network name (skip non-httpjail networks) - let Some(jail_id) = DockerNetwork::jail_id_from_network_name(network_name) else { - continue; - }; - - // Check if canary file exists for this jail - let canary_path = canary_dir.join(jail_id); - if !canary_path.exists() { - info!( - "Found orphaned Docker network {} without canary, removing", - network_name - ); - - // Remove the orphaned network - let rm_output = Command::new("docker") - .args(["network", "rm", network_name]) - .output() - .context("Failed to remove orphaned Docker network")?; - - if !rm_output.status.success() { - let stderr = String::from_utf8_lossy(&rm_output.stderr); - if !DockerNetwork::is_not_found_error(&stderr) { - warn!( - "Failed to remove orphaned Docker network {}: {}", - network_name, stderr - ); - } - } - } - } - - Ok(()) + fn container_name(&self) -> String { + format!("httpjail_{}_run", self.config.jail_id) } - /// Docker flags that take a value as the next argument - const FLAGS_WITH_VALUES: &'static [&'static str] = - &["-e", "-v", "-p", "--name", "--entrypoint", "-w", "--user"]; + /// Only options that cannot change the container's network, privileges, or host access. + const SAFE_VALUE_FLAGS: &'static [&'static str] = &[ + "-e", + "--env", + "--entrypoint", + "-w", + "--workdir", + "-u", + "--user", + "--pull", + "--memory", + "--cpus", + "--label", + ]; + const SAFE_BOOL_FLAGS: &'static [&'static str] = &[ + "--rm", + "--read-only", + "--init", + "--no-healthcheck", + "-i", + "-t", + ]; /// Build the docker command with isolated network #[allow(clippy::collapsible_if)] @@ -289,55 +344,84 @@ impl DockerLinux { &self, docker_args: &[String], extra_env: &[(String, String)], - ) -> Result { + ) -> Result<(Command, Option)> { let network_name = DockerNetwork::network_name_from_jail_id(&self.config.jail_id); - // Parse docker arguments to filter out conflicting options and find the image - let modified_args = Self::filter_network_args(docker_args); - - // Find where the image name is in the args - let image_idx = Self::find_image_index(&modified_args) - .context("Could not find Docker image in arguments")?; - - // Split args into: docker options, image, and command - let docker_opts = &modified_args[..image_idx]; - let image = &modified_args[image_idx]; - let user_command = if modified_args.len() > image_idx + 1 { - &modified_args[image_idx + 1..] - } else { - &[] - }; + let image_idx = Self::find_image_index(docker_args)?; + let docker_opts = &docker_args[..image_idx]; + let image = &docker_args[image_idx]; + let user_command = &docker_args[image_idx + 1..]; // Build the docker run command - let mut cmd = Command::new("docker"); + let mut cmd = local_docker_command(); cmd.arg("run"); - // Use our isolated Docker network - cmd.args(["--network", &network_name]); - - // Add CA certificate environment variables and bind mount the CA certificate + // Route DNS to the dummy resolver, never Docker's host-configured resolver. + let host_ip = + super::format_ip(LinuxJail::compute_host_ip_for_jail_id(&self.config.jail_id)); + let container_name = self.container_name(); + cmd.args([ + "--network", + &network_name, + "--dns", + &host_ip, + "--name", + &container_name, + "--cap-drop=NET_RAW", + "--rm", + ]); + + // Mount only a public certificate snapshot, never the CA key directory. let mut ca_cert_path = None; for (key, value) in extra_env { + if key == "SSL_CERT_DIR" { + continue; // That directory also holds the CA private key. + } cmd.arg("-e").arg(format!("{}={}", key, value)); - - // Track the CA certificate path for bind mounting if key == "SSL_CERT_FILE" && ca_cert_path.is_none() { ca_cert_path = Some(value.clone()); } } - // Bind mount the CA certificate if we have one + // Docker resolves bind sources later. A separate local actor could change + // the original path in between, so mount a root-private regular-file copy. + let mut cert_dir = None; if let Some(cert_path) = ca_cert_path { - // Mount the CA certificate to the same path in the container (read-only) - cmd.arg("-v").arg(format!("{}:{}:ro", cert_path, cert_path)); - - // Also mount the parent directory if it exists (for SSL_CERT_DIR) - if let Some(parent) = std::path::Path::new(&cert_path).parent() - && parent.exists() - { - let parent_str = parent.to_string_lossy(); - cmd.arg("-v") - .arg(format!("{}:{}:ro", parent_str, parent_str)); + use std::io::Read; + use std::os::unix::fs::OpenOptionsExt; + let cert = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(&cert_path) + .context("Failed to open public CA certificate")?; + let metadata = cert.metadata()?; + anyhow::ensure!( + metadata.is_file() && metadata.len() < 1024 * 1024, + "Invalid public CA certificate" + ); + let mut pem = String::new(); + cert.take(1024 * 1024).read_to_string(&mut pem)?; + anyhow::ensure!( + pem.trim_start().starts_with("-----BEGIN CERTIFICATE-----") + && pem.contains("-----END CERTIFICATE-----") + && !pem.contains("PRIVATE KEY"), + "Invalid public CA certificate" + ); + let dir = if unsafe { libc::geteuid() == 0 } { + let root_dir = std::path::Path::new("/var/lib/httpjail"); + crate::jail::ensure_trusted_root_dir(root_dir)?; + tempfile::Builder::new() + .prefix("cert-") + .tempdir_in(root_dir) + } else { + tempfile::tempdir() } + .context("Failed to create protected CA mount directory")?; + let snapshot = dir.path().join("ca-cert.pem"); + std::fs::write(&snapshot, pem).context("Failed to write public CA snapshot")?; + cmd.arg("-v") + .arg(format!("{}:{}:ro", snapshot.display(), cert_path)); + cert_dir = Some(dir); + cmd.env_remove("SSL_CERT_DIR"); } // Add user's docker options @@ -353,55 +437,48 @@ impl DockerLinux { cmd.arg(arg); } - Ok(cmd) + Ok((cmd, cert_dir)) } - /// Find the index of the Docker image in the arguments - fn find_image_index(args: &[String]) -> Option { - let mut skip_next = false; - - for (i, arg) in args.iter().enumerate() { - if skip_next { - skip_next = false; - continue; + /// Find the image while rejecting Docker options that could bypass isolation. + /// Everything after the image belongs to the container command, not Docker. + fn find_image_index(args: &[String]) -> Result { + let mut i = 0; + while let Some(arg) = args.get(i) { + if !arg.starts_with('-') { + anyhow::ensure!(!arg.is_empty(), "Docker image must not be empty"); + return Ok(i); } - - // Skip known flags that take values - if Self::FLAGS_WITH_VALUES.contains(&arg.as_str()) { - skip_next = true; + if Self::SAFE_BOOL_FLAGS.contains(&arg.as_str()) { + i += 1; continue; } - - // If it doesn't start with -, it's likely the image - if !arg.starts_with('-') { - return Some(i); - } - } - - None - } - - /// Filter out any existing --network arguments from docker args - fn filter_network_args(docker_args: &[String]) -> Vec { - let mut modified_args = Vec::new(); - let mut i = 0; - - while i < docker_args.len() { - if docker_args[i] == "--network" || docker_args[i].starts_with("--network=") { - info!("Overriding Docker --network flag with httpjail namespace"); - - if docker_args[i] == "--network" { - // Skip the next argument too - i += 2; - continue; - } + let (flag, inline_value) = arg + .split_once('=') + .map_or((arg.as_str(), None), |(name, value)| (name, Some(value))); + anyhow::ensure!( + Self::SAFE_VALUE_FLAGS.contains(&flag), + "Docker option not permitted in an isolated jail: {}", + arg + ); + if let Some(value) = inline_value { + anyhow::ensure!( + !value.is_empty(), + "Missing value for Docker option {}", + flag + ); + i += 1; } else { - modified_args.push(docker_args[i].clone()); + let value = args.get(i + 1).context("Missing Docker option value")?; + anyhow::ensure!( + !value.starts_with('-'), + "Invalid value for Docker option {}", + flag + ); + i += 2; } - i += 1; } - - modified_args + anyhow::bail!("Could not find Docker image in arguments") } /// Setup nftables rules to route Docker network traffic to jail @@ -456,8 +533,32 @@ impl DockerLinux { bridge_name ); + // Docker bridge traffic to its host gateway takes INPUT, not FORWARD. + // This inet guard blocks both IPv4 and IPv6 outside proxy and dummy DNS. + let guard = format!( + r#"table inet {table_name} {{ + chain input {{ + type filter hook input priority -5; policy accept; + iifname "{bridge_name}" ip saddr {docker_subnet} ip daddr {host_ip_str} tcp dport {{ {http_port}, {https_port} }} accept + iifname "{bridge_name}" ip saddr {docker_subnet} ip daddr {host_ip_str} udp dport 53 accept + iifname "{bridge_name}" drop + }} + chain forward {{ + type filter hook forward priority -5; policy accept; + iifname "{bridge_name}" drop + }} + }}"#, + table_name = table_name, + bridge_name = bridge_name, + docker_subnet = DockerNetwork::compute_docker_subnet(&self.config.jail_id), + host_ip_str = host_ip_str, + http_port = self.config.http_proxy_port, + https_port = self.config.https_proxy_port, + ); + let nft_rules = format!("{nft_rules}\n{guard}"); + // Apply the rules - let mut nft_cmd = Command::new("nft"); + let mut nft_cmd = Command::new("/usr/sbin/nft"); nft_cmd.arg("-f").arg("-"); nft_cmd.stdin(std::process::Stdio::piped()); @@ -491,9 +592,8 @@ impl DockerLinux { impl Jail for DockerLinux { fn setup(&mut self, proxy_port: u16) -> Result<()> { - // Clean up any orphaned Docker networks first - // This handles cases where Docker networks exist without corresponding canary files - Self::cleanup_all_orphaned_docker_networks()?; + // A missing canary cannot prove an older jail is dead: it may live + // beneath another sudo-preserved HOME. Never delete its network here. // First setup the inner Linux jail self.inner_jail.setup(proxy_port)?; @@ -514,22 +614,48 @@ impl Jail for DockerLinux { info!("Executing Docker container in isolated network"); // Build and execute the docker command - let mut cmd = self.build_docker_command(command, extra_env)?; + let (cmd, _cert_dir) = self.build_docker_command(command, extra_env)?; debug!("Docker command: {:?}", cmd); // Execute docker run and wait for it to complete - let status = cmd - .status() - .context("Failed to execute docker run command")?; + crate::jail::run_command(cmd, None).context("Failed to execute docker run command") + } + fn execute_with_timeout( + &self, + command: &[String], + extra_env: &[(String, String)], + timeout: std::time::Duration, + ) -> Result { + let (cmd, _cert_dir) = self.build_docker_command(command, extra_env)?; + let status = crate::jail::run_command(cmd, Some(timeout)) + .context("Failed to execute docker run command")?; + if status.code() == Some(124) { + // Killing the Docker CLI does not necessarily stop its daemon-owned container. + let output = local_docker_command() + .args(["rm", "-f", &self.container_name()]) + .output() + .context("Failed to stop timed-out Docker container")?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::ensure!( + DockerNetwork::is_not_found_error(&stderr), + "Failed to stop timed-out Docker container: {}", + stderr + ); + } + } Ok(status) } fn cleanup(&self) -> Result<()> { - // Docker network and routing will be cleaned up automatically via ManagedResource drop - - // Delegate to inner jail for cleanup + // The command may exit via process::exit and a signal handler holds a + // clone, so do not rely on Drop to clean up daemon-owned resources. + // Remove the network before its guard; on failure retain both and the + // canary so a later orphan cleanup can retry without exposing traffic. + DockerNetwork::for_existing(&self.config.jail_id).cleanup()?; + DockerRoutingTable::for_existing(&self.config.jail_id).cleanup()?; self.inner_jail.cleanup() } @@ -541,16 +667,33 @@ impl Jail for DockerLinux { where Self: Sized, { - // Clean up Docker-specific resources first - // These will be automatically cleaned up when they go out of scope - let _docker_network = ManagedResource::::for_existing(jail_id); - let _docker_routing = ManagedResource::::for_existing(jail_id); - - // Then delegate to LinuxJail for standard orphan cleanup + // Keep the bridge firewall until Docker confirms the network is gone. + DockerNetwork::for_existing(jail_id).cleanup()?; + DockerRoutingTable::for_existing(jail_id).cleanup()?; LinuxJail::cleanup_orphaned(jail_id) } } +impl Drop for DockerLinux { + fn drop(&mut self) { + // Never remove the firewall guard while Docker still reports an active + // network: a surviving container would regain direct host/Internet access. + if let Some(mut network) = self.docker_network.take() { + let result = network.inner_mut().map_or(Ok(()), SystemResource::cleanup); + if let Err(error) = result { + warn!( + "Retaining Docker bridge firewall after network cleanup failure: {}", + error + ); + if let Some(guard) = self.docker_routing.take() { + std::mem::forget(guard); + } + } + } + // Otherwise the routing guard is removed after the network. + } +} + impl Clone for DockerLinux { fn clone(&self) -> Self { Self { @@ -561,3 +704,137 @@ impl Clone for DockerLinux { } } } + +#[cfg(test)] +mod tests { + use super::DockerLinux; + + #[test] + fn stale_tables_skip_live_networks_and_canaries() { + use std::collections::HashSet; + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("root"); + let legacy = dir.path().join("legacy"); + let namespace_configs = dir.path().join("netns"); + std::fs::create_dir_all(&namespace_configs).unwrap(); + std::fs::create_dir_all(&root).unwrap(); + std::fs::create_dir_all(&legacy).unwrap(); + let line = "table ip httpjail_docker_abcd1234"; + let mut networks = HashSet::new(); + assert_eq!( + super::stale_docker_table_id(line, &networks, &root, Some(&legacy), &namespace_configs), + Some("abcd1234") + ); + networks.insert("httpjail_abcd1234".to_string()); + assert_eq!( + super::stale_docker_table_id(line, &networks, &root, Some(&legacy), &namespace_configs), + None + ); + networks.clear(); + std::fs::write(legacy.join("abcd1234"), "1").unwrap(); + assert_eq!( + super::stale_docker_table_id(line, &networks, &root, Some(&legacy), &namespace_configs), + None + ); + std::fs::remove_file(legacy.join("abcd1234")).unwrap(); + std::fs::create_dir(namespace_configs.join("httpjail_abcd1234")).unwrap(); + assert_eq!( + super::stale_docker_table_id(line, &networks, &root, Some(&legacy), &namespace_configs), + None + ); + assert_eq!( + super::stale_docker_table_id( + "table ip httpjail_docker_bad!", + &networks, + &root, + None, + &namespace_configs + ), + None + ); + } + + #[test] + fn docker_mounts_only_public_ca_file() { + let dir = tempfile::tempdir().unwrap(); + let cert = dir.path().join("ca-cert.pem"); + std::fs::write( + &cert, + b"-----BEGIN CERTIFICATE-----\npublic\n-----END CERTIFICATE-----\n", + ) + .unwrap(); + std::fs::write(dir.path().join("ca-key.pem"), b"dummy private key").unwrap(); + let cert_path = cert.to_string_lossy().to_string(); + let parent = dir.path().to_string_lossy().to_string(); + let jail = DockerLinux::new(crate::jail::JailConfig::new()).unwrap(); + let (cmd, _cert_dir) = jail + .build_docker_command( + &["alpine:latest".to_string()], + &[ + ("SSL_CERT_FILE".to_string(), cert_path.clone()), + ("SSL_CERT_DIR".to_string(), parent.clone()), + ], + ) + .unwrap(); + let args = cmd + .get_args() + .map(|arg| arg.to_string_lossy().into_owned()) + .collect::>(); + let mount = args + .iter() + .find(|arg| arg.ends_with(&format!(":{cert_path}:ro"))) + .unwrap(); + let source = mount.strip_suffix(&format!(":{cert_path}:ro")).unwrap(); + assert_ne!(source, cert_path); + assert!( + std::fs::read_to_string(source) + .unwrap() + .contains("BEGIN CERTIFICATE") + ); + assert!(!args.contains(&format!("{parent}:{parent}:ro"))); + assert!(!args.iter().any(|arg| arg.starts_with("SSL_CERT_DIR="))); + + std::fs::remove_file(&cert).unwrap(); + std::os::unix::fs::symlink("ca-key.pem", &cert).unwrap(); + assert!( + std::fs::read_to_string(source) + .unwrap() + .contains("BEGIN CERTIFICATE") + ); + assert!( + jail.build_docker_command( + &["alpine:latest".to_string()], + &[("SSL_CERT_FILE".to_string(), cert_path)] + ) + .is_err() + ); + } + + #[test] + fn docker_options_cannot_override_isolation() { + let args = |parts: &[&str]| parts.iter().map(|s| (*s).to_string()).collect::>(); + assert_eq!( + DockerLinux::find_image_index(&args(&[ + "--rm", + "-e", + "FOO=bar", + "alpine", + "--net=other" + ])) + .unwrap(), + 3 + ); + for option in [ + "--net=other", + "--network=host", + "--privileged", + "--dns=8.8.8.8", + "-v", + ] { + assert!( + DockerLinux::find_image_index(&args(&[option, "alpine"])).is_err(), + "unsafe Docker option {option} was accepted" + ); + } + } +} diff --git a/src/jail/linux/mod.rs b/src/jail/linux/mod.rs index 7d6165f8..1cc57b5e 100644 --- a/src/jail/linux/mod.rs +++ b/src/jail/linux/mod.rs @@ -1,15 +1,17 @@ pub mod dns; mod nftables; mod resources; +mod seccomp; #[cfg(target_os = "linux")] pub mod docker; use super::Jail; use super::JailConfig; -use crate::sys_resource::ManagedResource; +use crate::sys_resource::{ManagedResource, SystemResource}; use anyhow::{Context, Result}; use resources::{NFTable, NetnsResolv, NetworkNamespace, VethPair}; +use std::os::unix::process::CommandExt; use std::process::{Command, ExitStatus}; use tracing::{debug, info, warn}; @@ -82,6 +84,132 @@ pub struct LinuxJail { } impl LinuxJail { + fn validate_invoking_user() -> Result<()> { + let uid = std::env::var("SUDO_UID") + .ok() + .and_then(|value| value.parse::().ok()); + let gid = std::env::var("SUDO_GID") + .ok() + .and_then(|value| value.parse::().ok()); + anyhow::ensure!( + uid.is_some_and(|uid| uid != 0) && gid.is_some(), + "Linux strong mode requires sudo from a non-root user" + ); + Ok(()) + } + + fn build_command(&self, command: &[String], extra_env: &[(String, String)]) -> Result { + if command.is_empty() { + anyhow::bail!("No command specified"); + } + + debug!( + "Executing command in namespace {}: {:?}", + self.namespace_name(), + command + ); + + // Check if we're running as root and should drop privileges + let current_uid = unsafe { libc::getuid() }; + let drop_privs = if current_uid == 0 { + // Running as root - check for SUDO_UID/SUDO_GID to drop privileges to original user + match (std::env::var("SUDO_UID"), std::env::var("SUDO_GID")) { + (Ok(uid), Ok(gid)) + if uid.parse::().is_ok_and(|uid| uid != 0) + && gid.parse::().is_ok() => + { + debug!( + "Will drop privileges to uid={} gid={} after entering namespace", + uid, gid + ); + Some((uid, gid)) + } + _ => { + anyhow::bail!( + "Linux strong mode requires sudo from a non-root user so the command cannot alter its own network policy" + ); + } + } + } else { + // Not root - no privilege dropping needed + None + }; + + // DNS CONFIGURATION: Use standard ip netns exec approach + // + // ip netns exec automatically creates a mount namespace and bind-mounts + // /etc/netns//resolv.conf over /etc/resolv.conf when present. + // + // KNOWN LIMITATION: This may fail on systems where /etc/resolv.conf is a symlink + // to a target that doesn't exist in the mount namespace (e.g., systemd-resolved). + // In such cases, DNS queries will still reach our dummy DNS server via the nftables + // rules, but applications that directly check /etc/resolv.conf may see stale content. + // + // We cannot safely "fix" this because: + // - Mount namespaces only isolate mount tables, not filesystems + // - Any file operations (rm, cp, touch) affect the host + // - Bind-mounts over symlinks require the symlink target to exist + // + // Reference: https://man7.org/linux/man-pages/man8/ip-netns.8.html + + // A private PID namespace and fresh /proc prevent same-UID payloads + // from tracing or importing sockets from host-network processes. + let mut cmd = Command::new("/usr/sbin/ip"); + cmd.args([ + "netns", + "exec", + &self.namespace_name(), + "/usr/bin/unshare", + "--pid", + "--fork", + "--kill-child=SIGKILL", + "--mount-proc", + ]); + + // Add setpriv for privilege dropping if needed + if let Some((uid, gid)) = drop_privs { + cmd.arg("/usr/bin/setpriv"); + cmd.arg("--no-new-privs"); + cmd.arg(format!("--reuid={}", uid)); + cmd.arg(format!("--regid={}", gid)); + cmd.arg("--clear-groups"); + cmd.arg("--"); + } + + // Add user command + for arg in command { + cmd.arg(arg); + } + + // Set environment variables + for (key, value) in extra_env { + cmd.env(key, value); + } + + // Preserve SUDO environment variables for consistency with macOS + if let Ok(sudo_user) = std::env::var("SUDO_USER") { + cmd.env("SUDO_USER", sudo_user); + } + if let Ok(sudo_uid) = std::env::var("SUDO_UID") { + cmd.env("SUDO_UID", sudo_uid); + } + if let Ok(sudo_gid) = std::env::var("SUDO_GID") { + cmd.env("SUDO_GID", sudo_gid); + } + + debug!("Executing command: {:?}", cmd); + + // Note: We do NOT set HTTP_PROXY/HTTPS_PROXY environment variables here. + // The jail uses nftables rules to transparently redirect traffic to the proxy, + // making it work with applications that don't respect proxy environment variables. + + // Prevent access to host pathname Unix sockets (including local agents + // capable of making requests outside this network namespace). + // SAFETY: the pre-exec hook uses only raw syscalls and static BPF data. + unsafe { cmd.pre_exec(seccomp::block_unix_sockets) }; + Ok(cmd) + } + pub fn new(config: JailConfig) -> Result { let (host_ip, host_cidr, guest_cidr, subnet_cidr) = Self::compute_subnet_for_jail(&config.jail_id); @@ -190,7 +318,7 @@ impl LinuxJail { ); // Move veth_ns end into the namespace - let output = Command::new("ip") + let output = Command::new("/usr/sbin/ip") .args([ "link", "set", @@ -222,16 +350,23 @@ impl LinuxJail { // Commands to run inside the namespace let commands = vec![ // Bring up loopback - vec!["ip", "link", "set", "lo", "up"], + vec!["/usr/sbin/ip", "link", "set", "lo", "up"], // Configure veth interface with IP - vec!["ip", "addr", "add", &self.guest_cidr, "dev", &veth_ns], - vec!["ip", "link", "set", &veth_ns, "up"], + vec![ + "/usr/sbin/ip", + "addr", + "add", + &self.guest_cidr, + "dev", + &veth_ns, + ], + vec!["/usr/sbin/ip", "link", "set", &veth_ns, "up"], // Add default route pointing to host - vec!["ip", "route", "add", "default", "via", &host_ip], + vec!["/usr/sbin/ip", "route", "add", "default", "via", &host_ip], ]; for cmd_args in commands { - let mut cmd = Command::new("ip"); + let mut cmd = Command::new("/usr/sbin/ip"); cmd.args(["netns", "exec", &namespace_name]); cmd.args(&cmd_args); @@ -252,8 +387,15 @@ impl LinuxJail { } // Verify routes were added - let mut verify_cmd = Command::new("ip"); - verify_cmd.args(["netns", "exec", &namespace_name, "ip", "route", "show"]); + let mut verify_cmd = Command::new("/usr/sbin/ip"); + verify_cmd.args([ + "netns", + "exec", + &namespace_name, + "/usr/sbin/ip", + "route", + "show", + ]); if let Ok(output) = verify_cmd.output() { let routes = String::from_utf8_lossy(&output.stdout); info!( @@ -284,7 +426,7 @@ impl LinuxJail { ]; for cmd_args in commands { - let output = Command::new("ip") + let output = Command::new("/usr/sbin/ip") .args(&cmd_args) .output() .context(format!("Failed to execute: ip {:?}", cmd_args))?; @@ -302,18 +444,8 @@ impl LinuxJail { } } - // Enable IP forwarding for this interface - let output = Command::new("sysctl") - .args(["-w", "net.ipv4.ip_forward=1"]) - .output() - .context("Failed to enable IP forwarding")?; - - if !output.status.success() { - warn!( - "Failed to enable IP forwarding: {}", - String::from_utf8_lossy(&output.stderr) - ); - } + // Proxy and dummy DNS traffic is delivered to the host locally; host-wide + // IP forwarding is unnecessary and must not be changed by a jail. debug!("Configured host side networking for {}", veth_host); Ok(()) @@ -438,8 +570,9 @@ impl Drop for LinuxJail { impl Jail for LinuxJail { fn setup(&mut self, _proxy_port: u16) -> Result<()> { - // Check for root access + // Reject unsafe privilege retention before allocating jail resources. Self::check_root()?; + Self::validate_invoking_user()?; // Create network namespace self.create_namespace()?; @@ -472,100 +605,16 @@ impl Jail for LinuxJail { } fn execute(&self, command: &[String], extra_env: &[(String, String)]) -> Result { - if command.is_empty() { - anyhow::bail!("No command specified"); - } - - debug!( - "Executing command in namespace {}: {:?}", - self.namespace_name(), - command - ); - - // Check if we're running as root and should drop privileges - let current_uid = unsafe { libc::getuid() }; - let drop_privs = if current_uid == 0 { - // Running as root - check for SUDO_UID/SUDO_GID to drop privileges to original user - match (std::env::var("SUDO_UID"), std::env::var("SUDO_GID")) { - (Ok(uid), Ok(gid)) => { - debug!( - "Will drop privileges to uid={} gid={} after entering namespace", - uid, gid - ); - Some((uid, gid)) - } - _ => { - debug!("Running as root but no SUDO_UID/SUDO_GID found, continuing as root"); - None - } - } - } else { - // Not root - no privilege dropping needed - None - }; - - // DNS CONFIGURATION: Use standard ip netns exec approach - // - // ip netns exec automatically creates a mount namespace and bind-mounts - // /etc/netns//resolv.conf over /etc/resolv.conf when present. - // - // KNOWN LIMITATION: This may fail on systems where /etc/resolv.conf is a symlink - // to a target that doesn't exist in the mount namespace (e.g., systemd-resolved). - // In such cases, DNS queries will still reach our dummy DNS server via the nftables - // rules, but applications that directly check /etc/resolv.conf may see stale content. - // - // We cannot safely "fix" this because: - // - Mount namespaces only isolate mount tables, not filesystems - // - Any file operations (rm, cp, touch) affect the host - // - Bind-mounts over symlinks require the symlink target to exist - // - // Reference: https://man7.org/linux/man-pages/man8/ip-netns.8.html - - // Build command: ip netns exec [setpriv ...] - let mut cmd = Command::new("ip"); - cmd.args(["netns", "exec", &self.namespace_name()]); - - // Add setpriv for privilege dropping if needed - if let Some((uid, gid)) = drop_privs { - cmd.arg("setpriv"); - cmd.arg(format!("--reuid={}", uid)); - cmd.arg(format!("--regid={}", gid)); - cmd.arg("--init-groups"); - cmd.arg("--"); - } - - // Add user command - for arg in command { - cmd.arg(arg); - } - - // Set environment variables - for (key, value) in extra_env { - cmd.env(key, value); - } - - // Preserve SUDO environment variables for consistency with macOS - if let Ok(sudo_user) = std::env::var("SUDO_USER") { - cmd.env("SUDO_USER", sudo_user); - } - if let Ok(sudo_uid) = std::env::var("SUDO_UID") { - cmd.env("SUDO_UID", sudo_uid); - } - if let Ok(sudo_gid) = std::env::var("SUDO_GID") { - cmd.env("SUDO_GID", sudo_gid); - } - - debug!("Executing command: {:?}", cmd); - - // Note: We do NOT set HTTP_PROXY/HTTPS_PROXY environment variables here. - // The jail uses nftables rules to transparently redirect traffic to the proxy, - // making it work with applications that don't respect proxy environment variables. - - let status = cmd - .status() - .context("Failed to execute command in namespace")?; + super::run_command(self.build_command(command, extra_env)?, None) + } - Ok(status) + fn execute_with_timeout( + &self, + command: &[String], + extra_env: &[(String, String)], + timeout: std::time::Duration, + ) -> Result { + super::run_command(self.build_command(command, extra_env)?, Some(timeout)) } fn cleanup(&self) -> Result<()> { @@ -592,13 +641,12 @@ impl Jail for LinuxJail { { debug!("Cleaning up orphaned Linux jail: {}", jail_id); - // Create managed resources for existing system resources - // When these go out of scope, they will clean themselves up - let _namespace = ManagedResource::::for_existing(jail_id); - let _veth = ManagedResource::::for_existing(jail_id); - let _nftables = ManagedResource::::for_existing(jail_id); - let _netns_resolv = ManagedResource::::for_existing(jail_id); - + // Propagate failures instead of relying on Drop (which can only log them). + // Only remove the namespace name after deleting host-side resources. + NetnsResolv::for_existing(jail_id).cleanup()?; + NFTable::for_existing(jail_id).cleanup()?; + VethPair::for_existing(jail_id).cleanup()?; + NetworkNamespace::for_existing(jail_id).cleanup()?; Ok(()) } } diff --git a/src/jail/linux/nftables.rs b/src/jail/linux/nftables.rs index 2af290a7..51089b83 100644 --- a/src/jail/linux/nftables.rs +++ b/src/jail/linux/nftables.rs @@ -73,7 +73,7 @@ table ip {table_name} {{ // Apply the ruleset atomically use std::io::Write; - let mut child = Command::new("nft") + let mut child = Command::new("/usr/sbin/nft") .arg("-f") .arg("-") .stdin(std::process::Stdio::piped()) @@ -159,6 +159,14 @@ table ip {table_name} {{ ip protocol tcp drop }} }} + +# IPv6 link-local addresses on veth pairs must not reach host services. +table ip6 httpjail6 {{ + chain outfilter {{ + type filter hook output priority 0; policy drop; + oifname "lo" accept + }} +}} "#, table_name = table_name, host_ip = host_ip, @@ -172,8 +180,8 @@ table ip {table_name} {{ ); // Execute nft within the namespace - let mut child = Command::new("ip") - .args(["netns", "exec", namespace, "nft", "-f", "-"]) + let mut child = Command::new("/usr/sbin/ip") + .args(["netns", "exec", namespace, "/usr/sbin/nft", "-f", "-"]) .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) @@ -209,22 +217,53 @@ table ip {table_name} {{ } /// Remove the nftables table - fn remove(&mut self) -> Result<()> { + pub(super) fn remove(&mut self) -> Result<()> { if !self.created { return Ok(()); } + if let Some(ref namespace) = self.namespace { + let output = Command::new("/usr/sbin/ip") + .args([ + "netns", + "exec", + namespace, + "/usr/sbin/nft", + "delete", + "table", + "ip6", + "httpjail6", + ]) + .output() + .context("Failed to delete IPv6 nftables table in namespace")?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + if !stderr.contains("No such file or directory") + && !stderr.contains("does not exist") + { + anyhow::bail!("Failed to remove IPv6 table: {}", stderr); + } + } + } + let output = if let Some(ref namespace) = self.namespace { // Delete table in namespace - Command::new("ip") + Command::new("/usr/sbin/ip") .args([ - "netns", "exec", namespace, "nft", "delete", "table", "ip", &self.name, + "netns", + "exec", + namespace, + "/usr/sbin/nft", + "delete", + "table", + "ip", + &self.name, ]) .output() .context("Failed to execute nft delete in namespace")? } else { // Delete table on host - Command::new("nft") + Command::new("/usr/sbin/nft") .args(["delete", "table", "ip", &self.name]) .output() .context("Failed to execute nft delete")? @@ -234,8 +273,7 @@ table ip {table_name} {{ let stderr = String::from_utf8_lossy(&output.stderr); // Ignore if table doesn't exist (already removed) if !stderr.contains("No such file or directory") && !stderr.contains("does not exist") { - // Log but don't fail - best effort cleanup - debug!("Failed to remove nftables table {}: {}", self.name, stderr); + anyhow::bail!("Failed to remove nftables table {}: {}", self.name, stderr); } } else { debug!("Removed nftables table: {}", self.name); diff --git a/src/jail/linux/resources.rs b/src/jail/linux/resources.rs index 57fd4109..fc5a0e3d 100644 --- a/src/jail/linux/resources.rs +++ b/src/jail/linux/resources.rs @@ -3,7 +3,7 @@ use anyhow::{Context, Result}; use std::fs; use std::path::PathBuf; use std::process::Command; -use tracing::{debug, info, warn}; +use tracing::{debug, info}; /// Network namespace resource pub struct NetworkNamespace { @@ -22,7 +22,7 @@ impl SystemResource for NetworkNamespace { fn create(jail_id: &str) -> Result { let name = format!("httpjail_{}", jail_id); - let output = Command::new("ip") + let output = Command::new("/usr/sbin/ip") .args(["netns", "add", &name]) .output() .context("Failed to execute ip netns add")?; @@ -47,7 +47,7 @@ impl SystemResource for NetworkNamespace { return Ok(()); } - let output = Command::new("ip") + let output = Command::new("/usr/sbin/ip") .args(["netns", "del", &self.name]) .output() .context("Failed to execute ip netns del")?; @@ -102,7 +102,7 @@ impl SystemResource for VethPair { let host_name = format!("vh_{}", jail_id); let ns_name = format!("vn_{}", jail_id); - let output = Command::new("ip") + let output = Command::new("/usr/sbin/ip") .args([ "link", "add", &host_name, "type", "veth", "peer", "name", &ns_name, ]) @@ -129,11 +129,17 @@ impl SystemResource for VethPair { return Ok(()); } - // Deleting the host side will automatically delete both ends - let _ = Command::new("ip") + // Deleting the host side will automatically delete both ends. + let output = Command::new("/usr/sbin/ip") .args(["link", "del", &self.host_name]) - .output(); - + .output() + .context("Failed to execute ip link del")?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + if !stderr.contains("Cannot find device") && !stderr.contains("does not exist") { + anyhow::bail!("Failed to delete veth pair: {}", stderr); + } + } self.created = false; Ok(()) } @@ -171,8 +177,9 @@ impl SystemResource for NFTable { } fn cleanup(&mut self) -> Result<()> { - // Table cleans itself up via Drop trait - self.table = None; + if let Some(mut table) = self.table.take() { + table.remove()?; + } Ok(()) } @@ -202,36 +209,20 @@ impl SystemResource for NFTable { /// /// 1. **Creation**: Ensure the symlink target exists on the host (we create an empty /// placeholder in /run/systemd/resolve/ if needed - safe since /run is tmpfs) -/// 2. **Cleanup**: Explicitly unmount the bind-mount at the symlink target during -/// cleanup to prevent stale mounts from accumulating +/// 2. **Cleanup**: Remove the namespace configuration directory. Bind mounts +/// belong to the mount namespace created by `ip netns exec`, not the host. /// /// # Safety /// /// - Host's /etc/resolv.conf is never modified directly /// - Placeholder creation is best-effort and won't affect systemd-resolved -/// - Cleanup unmounts are idempotent and won't fail if already unmounted +/// - Never unmount the host's resolver symlink target during cleanup pub struct NetnsResolv { netns_dir: PathBuf, created: bool, } impl NetnsResolv { - /// Resolve /etc/resolv.conf to its canonical path, handling symlinks - /// - /// Returns None if /etc/resolv.conf is not a symlink or cannot be resolved - fn resolve_resolv_conf_target() -> Option { - let symlink_target = fs::read_link("/etc/resolv.conf").ok()?; - - // Convert relative path to absolute (e.g., "../run/systemd/resolve/stub-resolv.conf") - let absolute_path = if symlink_target.is_absolute() { - symlink_target - } else { - PathBuf::from("/etc").join(symlink_target) - }; - - fs::canonicalize(absolute_path).ok() - } - /// Create /etc/netns/httpjail_/resolv.conf with specified nameserver pub fn create_with_nameserver(jail_id: &str, nameserver_ip: &str) -> Result { let netns_dir = PathBuf::from(format!("/etc/netns/httpjail_{}", jail_id)); @@ -282,21 +273,15 @@ impl SystemResource for NetnsResolv { return Ok(()); } - // Unmount bind-mount at symlink target (see struct documentation for why) - // Best-effort: ignore failures since mount might already be cleaned up - if let Some(target_path) = Self::resolve_resolv_conf_target() { - let _ = Command::new("umount").arg(&target_path).output(); - debug!( - "Attempted to unmount bind-mount at {}", - target_path.display() - ); - } - + // `ip netns exec` owns a separate mount namespace. Unmounting the + // symlink target here would operate on the host mount namespace instead. // Remove /etc/netns// directory match fs::remove_dir_all(&self.netns_dir) { Ok(()) => debug!("Removed {}", self.netns_dir.display()), Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => warn!("Failed to remove {}: {}", self.netns_dir.display(), e), + Err(e) => { + return Err(e).context(format!("Failed to remove {}", self.netns_dir.display())); + } } self.created = false; diff --git a/src/jail/linux/seccomp.rs b/src/jail/linux/seccomp.rs new file mode 100644 index 00000000..4e12b1ba --- /dev/null +++ b/src/jail/linux/seccomp.rs @@ -0,0 +1,197 @@ +//! Restrict native strong-jail payloads to IPv4/IPv6 and netlink sockets, +//! permitting only connected AF_UNIX stream socketpairs for local thread IPC. +//! This prevents host pathname Unix socket IPC and VM VSOCK egress, neither +//! of which can be filtered by the namespace's IP nftables chains. + +use std::io; + +#[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] +const fn ins(code: u16, jt: u8, jf: u8, k: u32) -> libc::sock_filter { + libc::sock_filter { code, jt, jf, k } +} + +#[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] +const LD: u16 = (libc::BPF_LD | libc::BPF_W | libc::BPF_ABS) as u16; +#[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] +const JEQ: u16 = (libc::BPF_JMP | libc::BPF_JEQ | libc::BPF_K) as u16; +#[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] +const RET: u16 = (libc::BPF_RET | libc::BPF_K) as u16; + +// seccomp_data: syscall number @0, architecture @4, first argument @16. +// Unknown ABIs are killed instead of allowing 32-bit socketcall bypasses. +#[cfg(target_arch = "x86_64")] +static FILTER: [libc::sock_filter; 25] = [ + ins(LD, 0, 0, 4), + ins(JEQ, 1, 0, 0xc000_003e), // AUDIT_ARCH_X86_64 + ins(RET, 0, 0, libc::SECCOMP_RET_KILL_PROCESS), + ins(LD, 0, 0, 0), + ins( + (libc::BPF_JMP | libc::BPF_JGE | libc::BPF_K) as u16, + 0, + 1, + 0x4000_0000, + ), // x32 + ins(RET, 0, 0, libc::SECCOMP_RET_KILL_PROCESS), + ins(JEQ, 5, 0, libc::SYS_socket as u32), + ins(JEQ, 10, 0, libc::SYS_socketpair as u32), + ins(JEQ, 2, 0, libc::SYS_io_uring_setup as u32), + ins(JEQ, 1, 0, libc::SYS_io_uring_enter as u32), + ins(JEQ, 0, 6, libc::SYS_io_uring_register as u32), + ins(RET, 0, 0, libc::SECCOMP_RET_ERRNO | libc::EPERM as u32), + ins(LD, 0, 0, 16), + ins(JEQ, 3, 0, libc::AF_INET as u32), + ins(JEQ, 2, 0, libc::AF_INET6 as u32), + ins(JEQ, 1, 0, libc::AF_NETLINK as u32), + ins(RET, 0, 0, libc::SECCOMP_RET_ERRNO | libc::EPERM as u32), + ins(RET, 0, 0, libc::SECCOMP_RET_ALLOW), + ins(LD, 0, 0, 16), // socketpair domain + ins(JEQ, 0, 3, libc::AF_UNIX as u32), + ins(LD, 0, 0, 24), // socketpair type (without CLOEXEC/NONBLOCK) + ins( + (libc::BPF_ALU | libc::BPF_AND | libc::BPF_K) as u16, + 0, + 0, + 0xf, + ), + ins(JEQ, 1, 0, libc::SOCK_STREAM as u32), + ins(RET, 0, 0, libc::SECCOMP_RET_ERRNO | libc::EPERM as u32), + ins(RET, 0, 0, libc::SECCOMP_RET_ALLOW), +]; + +#[cfg(target_arch = "aarch64")] +static FILTER: [libc::sock_filter; 23] = [ + ins(LD, 0, 0, 4), + ins(JEQ, 1, 0, 0xc000_00b7), // AUDIT_ARCH_AARCH64 + ins(RET, 0, 0, libc::SECCOMP_RET_KILL_PROCESS), + ins(LD, 0, 0, 0), + ins(JEQ, 5, 0, libc::SYS_socket as u32), + ins(JEQ, 10, 0, libc::SYS_socketpair as u32), + ins(JEQ, 2, 0, libc::SYS_io_uring_setup as u32), + ins(JEQ, 1, 0, libc::SYS_io_uring_enter as u32), + ins(JEQ, 0, 6, libc::SYS_io_uring_register as u32), + ins(RET, 0, 0, libc::SECCOMP_RET_ERRNO | libc::EPERM as u32), + ins(LD, 0, 0, 16), + ins(JEQ, 3, 0, libc::AF_INET as u32), + ins(JEQ, 2, 0, libc::AF_INET6 as u32), + ins(JEQ, 1, 0, libc::AF_NETLINK as u32), + ins(RET, 0, 0, libc::SECCOMP_RET_ERRNO | libc::EPERM as u32), + ins(RET, 0, 0, libc::SECCOMP_RET_ALLOW), + ins(LD, 0, 0, 16), // socketpair domain + ins(JEQ, 0, 3, libc::AF_UNIX as u32), + ins(LD, 0, 0, 24), // socketpair type (without CLOEXEC/NONBLOCK) + ins( + (libc::BPF_ALU | libc::BPF_AND | libc::BPF_K) as u16, + 0, + 0, + 0xf, + ), + ins(JEQ, 1, 0, libc::SOCK_STREAM as u32), + ins(RET, 0, 0, libc::SECCOMP_RET_ERRNO | libc::EPERM as u32), + ins(RET, 0, 0, libc::SECCOMP_RET_ALLOW), +]; + +#[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] +pub(super) fn block_unix_sockets() -> io::Result<()> { + let program = libc::sock_fprog { + len: FILTER.len() as u16, + filter: FILTER.as_ptr() as *mut libc::sock_filter, + }; + // SAFETY: Called in a child pre_exec hook. Only raw syscalls and static + // BPF instructions are used here; no locks or allocation after fork. + if unsafe { libc::prctl(libc::PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) } != 0 { + return Err(io::Error::last_os_error()); + } + if unsafe { libc::prctl(libc::PR_SET_SECCOMP, libc::SECCOMP_MODE_FILTER, &program) } != 0 { + return Err(io::Error::last_os_error()); + } + Ok(()) +} + +#[cfg(all(test, any(target_arch = "x86_64", target_arch = "aarch64")))] +mod tests { + use super::*; + + fn verdict(syscall: u32, domain: u32, socket_type: u32) -> u32 { + let mut accumulator = 0; + let mut pc = 0; + for _ in 0..FILTER.len() { + let instruction = FILTER[pc]; + if instruction.code == LD { + accumulator = match instruction.k { + 0 => syscall, + 4 => FILTER[1].k, // native architecture + 16 => domain, + 24 => socket_type, + _ => panic!("invalid BPF load"), + }; + pc += 1; + } else if instruction.code == JEQ { + pc += 1 + if accumulator == instruction.k { + instruction.jt + } else { + instruction.jf + } as usize; + } else if instruction.code == (libc::BPF_JMP | libc::BPF_JGE | libc::BPF_K) as u16 { + pc += 1 + if accumulator >= instruction.k { + instruction.jt + } else { + instruction.jf + } as usize; + } else if instruction.code == (libc::BPF_ALU | libc::BPF_AND | libc::BPF_K) as u16 { + accumulator &= instruction.k; + pc += 1; + } else if instruction.code == RET { + return instruction.k; + } else { + panic!("invalid BPF instruction"); + } + } + panic!("BPF program failed to return"); + } + + #[test] + fn filter_allows_ordinary_exec_and_ip_but_denies_ipc() { + let denied = libc::SECCOMP_RET_ERRNO | libc::EPERM as u32; + assert_eq!( + verdict(libc::SYS_execve as u32, 0, 0), + libc::SECCOMP_RET_ALLOW + ); + assert_eq!( + verdict(libc::SYS_socket as u32, libc::AF_INET as u32, 0), + libc::SECCOMP_RET_ALLOW + ); + assert_eq!( + verdict(libc::SYS_socket as u32, libc::AF_UNIX as u32, 0), + denied + ); + assert_eq!( + verdict(libc::SYS_socket as u32, libc::AF_VSOCK as u32, 0), + denied + ); + assert_eq!( + verdict( + libc::SYS_socketpair as u32, + libc::AF_UNIX as u32, + libc::SOCK_DGRAM as u32 + ), + denied + ); + assert_eq!( + verdict( + libc::SYS_socketpair as u32, + libc::AF_UNIX as u32, + libc::SOCK_STREAM as u32 | libc::SOCK_CLOEXEC as u32 + ), + libc::SECCOMP_RET_ALLOW + ); + assert_eq!(verdict(libc::SYS_io_uring_setup as u32, 0, 0), denied); + } +} + +#[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] +pub(super) fn block_unix_sockets() -> io::Result<()> { + Err(io::Error::new( + io::ErrorKind::Unsupported, + "unsupported seccomp architecture", + )) +} diff --git a/src/jail/managed.rs b/src/jail/managed.rs index a5f21f84..e1180ba3 100644 --- a/src/jail/managed.rs +++ b/src/jail/managed.rs @@ -1,6 +1,7 @@ use super::{Jail, JailConfig}; use anyhow::{Context, Result}; use std::fs; +use std::io::Write; use std::path::PathBuf; use std::process::ExitStatus; use std::sync::Arc; @@ -9,7 +10,7 @@ use std::thread::{self, JoinHandle}; use std::time::{Duration, SystemTime}; use tracing::{debug, error, info, warn}; -use crate::jail::get_canary_dir; +use crate::jail::{canary_owner_alive, get_canary_dir, valid_jail_id}; /// Manages jail lifecycle and cleanup with automatic cleanup on drop pub struct ManagedJail { @@ -44,6 +45,16 @@ impl ManagedJail { }) } + fn ensure_canary_dir(&self) -> Result<()> { + #[cfg(target_os = "linux")] + if unsafe { libc::geteuid() == 0 } { + crate::jail::ensure_trusted_root_dir(&self.canary_dir)?; + return Ok(()); + } + fs::create_dir_all(&self.canary_dir)?; + Ok(()) + } + /// Public method to trigger orphan cleanup for debugging pub fn debug_cleanup_orphans(&self) -> Result<()> { self.cleanup_orphans() @@ -53,10 +64,10 @@ impl ManagedJail { fn cleanup_orphans(&self) -> Result<()> { debug!("Starting orphan cleanup scan in {:?}", self.canary_dir); - // Create directory if it doesn't exist - if !self.canary_dir.exists() { - debug!("Canary directory does not exist, creating it"); - fs::create_dir_all(&self.canary_dir).context("Failed to create canary directory")?; + let existed = self.canary_dir.exists(); + self.ensure_canary_dir() + .context("Failed to create trusted canary directory")?; + if !existed { return Ok(()); } @@ -85,6 +96,10 @@ impl ManagedJail { .file_name() .and_then(|n| n.to_str()) .unwrap_or("unknown"); + if !valid_jail_id(jail_id) || canary_owner_alive(&path) { + debug!("Skipping canary for live or invalid jail {}", jail_id); + continue; + } info!( "Found orphaned jail '{}' (age: {:?}), cleaning up", @@ -113,9 +128,7 @@ impl ManagedJail { return Ok(()); } - // Create canary file first - self.create_canary()?; - + // The canary is created once before jail setup. Never reopen it for writing. // Setup heartbeat thread let canary_path = self.canary_path.clone(); let interval = self.heartbeat_interval; @@ -176,13 +189,17 @@ impl ManagedJail { /// Create the canary file fn create_canary(&self) -> Result<()> { - // Ensure directory exists - if !self.canary_dir.exists() { - fs::create_dir_all(&self.canary_dir).context("Failed to create canary directory")?; - } - - // Create empty canary file - fs::write(&self.canary_path, b"").context("Failed to create canary file")?; + self.ensure_canary_dir() + .context("Failed to create trusted canary directory")?; + + // Record the owner so a paused heartbeat cannot cause live-jail cleanup. + let mut file = fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&self.canary_path) + .context("Failed to create exclusive canary file")?; + file.write_all(std::process::id().to_string().as_bytes()) + .context("Failed to write canary owner")?; debug!("Created canary file for jail '{}'", self.jail.jail_id()); Ok(()) @@ -213,10 +230,8 @@ fn touch_file_mtime(path: &PathBuf) -> Result<()> { filetime::FileTime::from_system_time(atime), filetime::FileTime::from_system_time(mtime), )?; - } else { - // Create empty file if it doesn't exist - fs::write(path, b"")?; } + // A missing canary is not recreated by the heartbeat thread. Ok(()) } @@ -248,23 +263,28 @@ impl Jail for ManagedJail { } fn execute(&self, command: &[String], extra_env: &[(String, String)]) -> Result { - // Simply delegate to the inner jail self.jail.execute(command, extra_env) } + fn execute_with_timeout( + &self, + command: &[String], + extra_env: &[(String, String)], + timeout: Duration, + ) -> Result { + self.jail.execute_with_timeout(command, extra_env, timeout) + } + fn cleanup(&self) -> Result<()> { // Signal the heartbeat to stop so it doesn't recreate the canary self.signal_stop_heartbeat(); - // Cleanup the inner jail first - let result = self.jail.cleanup(); - - // Delete canary last + // Keep the canary if cleanup fails so a later orphan pass can retry. + self.jail.cleanup()?; if self.enable_heartbeat { self.delete_canary()?; } - - result + Ok(()) } fn jail_id(&self) -> &str { @@ -300,3 +320,24 @@ impl Drop for ManagedJail { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn canary_creation_never_follows_an_existing_symlink() { + let dir = tempfile::tempdir().unwrap(); + let sentinel = dir.path().join("sentinel"); + fs::write(&sentinel, "unchanged").unwrap(); + let config = JailConfig::new(); + let weak = crate::jail::weak::WeakJail::new(config.clone()).unwrap(); + let mut jail = ManagedJail::new(weak, &config).unwrap(); + jail.enable_heartbeat = false; + jail.canary_dir = dir.path().to_path_buf(); + jail.canary_path = dir.path().join("canary"); + std::os::unix::fs::symlink(&sentinel, &jail.canary_path).unwrap(); + assert!(jail.create_canary().is_err()); + assert_eq!(fs::read_to_string(&sentinel).unwrap(), "unchanged"); + } +} diff --git a/src/jail/mod.rs b/src/jail/mod.rs index c389c293..bb7dc507 100644 --- a/src/jail/mod.rs +++ b/src/jail/mod.rs @@ -1,5 +1,8 @@ -use anyhow::Result; +use anyhow::{Context, Result}; use rand::Rng; +use std::os::unix::process::{CommandExt, ExitStatusExt}; +use std::process::{Command, ExitStatus}; +use std::time::{Duration, Instant}; pub mod weak; @@ -9,6 +12,39 @@ pub mod linux; #[cfg(any(target_os = "macos", target_os = "linux"))] pub mod managed; +/// Wait for an executed command, killing its entire process group before returning on timeout. +/// The caller must not clean up network isolation while the command is still running. +pub(crate) fn run_command(mut command: Command, limit: Option) -> Result { + let Some(limit) = limit else { + return command.status().context("Failed to execute command"); + }; + + // Put descendants in a distinct process group, rather than killing only the + // `ip netns exec` / docker / shell wrapper on timeout. + command.process_group(0); + let mut child = command.spawn().context("Failed to execute command")?; + let started = Instant::now(); + loop { + if let Some(status) = child.try_wait()? { + return Ok(status); + } + if started.elapsed() >= limit { + let result = unsafe { libc::kill(-(child.id() as i32), libc::SIGKILL) }; + let kill_error = (result != 0).then(std::io::Error::last_os_error); + // Reap the direct child even if it exited concurrently with the signal. + let _ = child.kill(); + child.wait()?; + if let Some(error) = kill_error + && error.raw_os_error() != Some(libc::ESRCH) + { + return Err(error).context("Failed to kill timed-out process group"); + } + return Ok(ExitStatus::from_raw(124 << 8)); + } + std::thread::sleep(Duration::from_millis(10).min(limit.saturating_sub(started.elapsed()))); + } +} + /// Trait for platform-specific jail implementations #[allow(dead_code)] pub trait Jail: Send + Sync { @@ -36,6 +72,14 @@ pub trait Jail: Send + Sync { extra_env: &[(String, String)], ) -> Result; + /// Execute with a deadline; terminate and reap the command before returning on timeout. + fn execute_with_timeout( + &self, + command: &[String], + extra_env: &[(String, String)], + timeout: Duration, + ) -> Result; + /// Cleanup jail resources fn cleanup(&self) -> Result<()>; @@ -49,8 +93,57 @@ pub trait Jail: Send + Sync { Self: Sized; } +/// Only automatically clean up resource names generated by this tool. +pub fn valid_jail_id(id: &str) -> bool { + id.len() == 8 + && id + .bytes() + .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit()) +} + +/// A stale heartbeat is not evidence of process death (setup may be slow or paused). +/// Unknown/legacy canaries are treated as live rather than risking deletion of a live jail. +pub fn canary_owner_alive(path: &std::path::Path) -> bool { + let Ok(contents) = std::fs::read_to_string(path) else { + return true; + }; + let Ok(pid) = contents.trim().parse::() else { + return true; + }; + if pid <= 0 { + return true; + } + unsafe { + libc::kill(pid, 0) == 0 + || std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) + } +} + +#[cfg(target_os = "linux")] +pub(crate) fn ensure_trusted_root_dir(path: &std::path::Path) -> anyhow::Result<()> { + use std::os::unix::fs::{DirBuilderExt, MetadataExt}; + let mut builder = std::fs::DirBuilder::new(); + builder.recursive(true).mode(0o700).create(path)?; + for ancestor in path.ancestors() { + let metadata = std::fs::symlink_metadata(ancestor)?; + anyhow::ensure!( + metadata.file_type().is_dir() && metadata.uid() == 0 && metadata.mode() & 0o022 == 0, + "Directory and ancestors must be root-owned and not writable by other users: {}", + ancestor.display() + ); + } + Ok(()) +} + /// Get the canary directory for tracking jail lifetimes pub fn get_canary_dir() -> std::path::PathBuf { + #[cfg(target_os = "linux")] + if unsafe { libc::geteuid() == 0 } { + // sudo -E may preserve an invoking-user-owned HOME. Root canaries must + // not live beneath a directory that user can rename or replace. + return std::path::PathBuf::from("/var/lib/httpjail/canaries"); + } + // Use user data directory instead of /tmp to avoid issues with tmp cleaners // This ensures canaries persist across reboots and are only removed when we want them to be if let Some(data_dir) = dirs::data_dir() { @@ -196,6 +289,18 @@ mod tests { } } + #[test] + fn stale_heartbeat_does_not_make_a_live_jail_an_orphan() { + let temp = tempfile::tempdir().unwrap(); + let canary = temp.path().join("abc12345"); + std::fs::write(&canary, std::process::id().to_string()).unwrap(); + assert!(valid_jail_id("abc12345")); + assert!(!valid_jail_id("../etc/passwd")); + assert!(canary_owner_alive(&canary)); + std::fs::write(&canary, i32::MAX.to_string()).unwrap(); + assert!(!canary_owner_alive(&canary)); + } + #[test] fn test_jail_id_uniqueness() { // Generate many IDs and check for collisions diff --git a/src/jail/weak.rs b/src/jail/weak.rs index 9835cd55..8ac27d8a 100644 --- a/src/jail/weak.rs +++ b/src/jail/weak.rs @@ -13,24 +13,8 @@ impl WeakJail { pub fn new(config: JailConfig) -> Result { Ok(Self { config }) } -} -impl Jail for WeakJail { - fn setup(&mut self, _proxy_port: u16) -> Result<()> { - info!("Setting up weak jail (environment variables only)"); - info!( - "HTTP proxy will be set to: http://127.0.0.1:{}", - self.config.http_proxy_port - ); - info!( - "HTTPS proxy will be set to: http://127.0.0.1:{}", - self.config.https_proxy_port - ); - - Ok(()) - } - - fn execute(&self, command: &[String], extra_env: &[(String, String)]) -> Result { + fn build_command(&self, command: &[String], extra_env: &[(String, String)]) -> Result { if command.is_empty() { anyhow::bail!("No command specified"); } @@ -78,11 +62,36 @@ impl Jail for WeakJail { http_proxy, https_proxy ); - let status = cmd - .status() - .map_err(|e| anyhow::anyhow!("Failed to execute command: {}", e))?; + Ok(cmd) + } +} + +impl Jail for WeakJail { + fn setup(&mut self, _proxy_port: u16) -> Result<()> { + info!("Setting up weak jail (environment variables only)"); + info!( + "HTTP proxy will be set to: http://127.0.0.1:{}", + self.config.http_proxy_port + ); + info!( + "HTTPS proxy will be set to: http://127.0.0.1:{}", + self.config.https_proxy_port + ); + + Ok(()) + } + + fn execute(&self, command: &[String], extra_env: &[(String, String)]) -> Result { + super::run_command(self.build_command(command, extra_env)?, None) + } - Ok(status) + fn execute_with_timeout( + &self, + command: &[String], + extra_env: &[(String, String)], + timeout: std::time::Duration, + ) -> Result { + super::run_command(self.build_command(command, extra_env)?, Some(timeout)) } fn cleanup(&self) -> Result<()> { diff --git a/src/limited_body.rs b/src/limited_body.rs index 250dd0ef..cc832be8 100644 --- a/src/limited_body.rs +++ b/src/limited_body.rs @@ -50,7 +50,7 @@ use tracing::debug; /// - **Within Limit**: Frames are passed through unchanged /// - **At Limit**: Stream terminates immediately (returns `None`) /// - **Exceeding Limit**: Frame is truncated to fit remaining bytes -/// - **Non-Data Frames**: Trailers and other non-data frames pass through unchanged +/// - **Trailers**: Charged against the remaining budget or dropped if they do not fit /// /// # Example /// @@ -143,9 +143,20 @@ impl Body for LimitedBody { ); Poll::Ready(Some(Ok(frame))) } + } else if let Some(trailers) = frame.trailers_ref() { + let trailer_size = trailers.iter().fold(2_u64, |size, (name, value)| { + size.saturating_add(name.as_str().len() as u64 + value.len() as u64 + 4) + }); + if trailer_size > self.max_bytes - self.bytes_transmitted { + debug!(trailer_size, "Trailer exceeds remaining byte limit"); + Poll::Ready(None) + } else { + self.bytes_transmitted += trailer_size; + Poll::Ready(Some(Ok(frame))) + } } else { - // Non-data frame (like trailers), pass through unchanged - Poll::Ready(Some(Ok(frame))) + // Unknown frame types cannot be charged safely. + Poll::Ready(None) } } Poll::Ready(Some(Err(e))) => Poll::Ready(Some(Err(e))), @@ -197,6 +208,35 @@ mod tests { .boxed() } + struct TrailerOnly(Option); + + impl Body for TrailerOnly { + type Data = Bytes; + type Error = HyperError; + + fn poll_frame( + mut self: Pin<&mut Self>, + _cx: &mut Context<'_>, + ) -> Poll, HyperError>>> { + Poll::Ready(self.0.take().map(|headers| Ok(Frame::trailers(headers)))) + } + } + + #[tokio::test] + async fn test_trailers_obey_byte_limit() { + let mut trailers = hyper::HeaderMap::new(); + trailers.insert("x-secret", "canary".parse().unwrap()); + let mut limited = LimitedBody::new(TrailerOnly(Some(trailers.clone())).boxed(), 5); + assert!( + limited.frame().await.is_none(), + "oversized trailer was forwarded" + ); + + let mut limited = LimitedBody::new(TrailerOnly(Some(trailers)).boxed(), 100); + let frame = limited.frame().await.unwrap().unwrap(); + assert_eq!(frame.into_trailers().unwrap()["x-secret"], "canary"); + } + #[tokio::test] async fn test_limited_body_within_limit() { // Test: Body smaller than limit passes through unchanged diff --git a/src/main.rs b/src/main.rs index 3fc1fb92..c31e0115 100644 --- a/src/main.rs +++ b/src/main.rs @@ -8,7 +8,6 @@ use httpjail::rules::v8_js::V8JsRuleEngine; use httpjail::rules::{Action, RuleEngine}; use hyper::Method; use std::fs::OpenOptions; -use std::os::unix::process::ExitStatusExt; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; use tracing::{debug, info, warn}; @@ -139,6 +138,103 @@ struct RunArgs { exec_command: Vec, } +#[cfg(target_os = "linux")] +fn trusted_policy_path(path: &str, shell_rule: bool) -> Result { + use std::os::unix::fs::MetadataExt; + + anyhow::ensure!( + !shell_rule || !path.chars().any(char::is_whitespace), + "Linux strong mode requires --sh to name one trusted executable, not a shell command" + ); + let resolved = std::fs::canonicalize(path) + .with_context(|| format!("Failed to resolve rule executable: {path}"))?; + for component in resolved.ancestors() { + let metadata = std::fs::metadata(component)?; + anyhow::ensure!( + metadata.uid() == 0 && metadata.mode() & 0o022 == 0, + "Linux strong mode requires root-owned, non-writable rule executable and ancestors: {}", + component.display() + ); + } + Ok(resolved + .to_str() + .context("Rule executable path must be UTF-8")? + .to_string()) +} + +#[cfg(target_os = "linux")] +fn open_request_log_as_invoker(path: &str) -> Result { + use std::ffi::CString; + use std::os::fd::{AsRawFd, FromRawFd}; + use std::os::unix::ffi::OsStrExt; + use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; + use std::path::Path; + + let uid = std::env::var("SUDO_UID") + .context("Root request logs require an invoking sudo user")? + .parse::()?; + let gid = std::env::var("SUDO_GID") + .context("Root request logs require an invoking sudo group")? + .parse::()?; + anyhow::ensure!( + uid != 0, + "Root request logs require a non-root invoking user" + ); + + let path = Path::new(path); + let name = path.file_name().context("Request log needs a filename")?; + anyhow::ensure!(name != "." && name != "..", "Invalid request log name"); + let parent = path + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or(Path::new(".")); + // Pin a directory the invoking user owns. Even if a parent pathname is + // swapped afterward, openat stays anchored to this checked directory. + let dir = OpenOptions::new() + .read(true) + .custom_flags(libc::O_DIRECTORY | libc::O_NOFOLLOW) + .open(parent) + .context("Failed to open request-log directory")?; + anyhow::ensure!( + dir.metadata()?.uid() == uid, + "Request-log directory must be owned by invoking user" + ); + let name = CString::new(name.as_bytes())?; + let flags = + libc::O_WRONLY | libc::O_APPEND | libc::O_CLOEXEC | libc::O_NOFOLLOW | libc::O_NONBLOCK; + let (fd, created) = unsafe { + let fd = libc::openat( + dir.as_raw_fd(), + name.as_ptr(), + flags | libc::O_CREAT | libc::O_EXCL, + 0o600, + ); + if fd >= 0 { + (fd, true) + } else if std::io::Error::last_os_error().kind() == std::io::ErrorKind::AlreadyExists { + (libc::openat(dir.as_raw_fd(), name.as_ptr(), flags), false) + } else { + return Err(std::io::Error::last_os_error().into()); + } + }; + if fd < 0 { + return Err(std::io::Error::last_os_error().into()); + } + let file = unsafe { std::fs::File::from_raw_fd(fd) }; + let metadata = file.metadata()?; + anyhow::ensure!(metadata.is_file(), "Request log must be a regular file"); + if created { + let result = unsafe { libc::fchown(fd, uid, gid) }; + anyhow::ensure!(result == 0, "Failed to assign request log to invoking user"); + } else { + anyhow::ensure!( + metadata.uid() == uid, + "Existing request log must be owned by invoking user" + ); + } + Ok(file) +} + fn setup_logging(verbosity: u8) { use tracing_subscriber::fmt::time::FormatTime; @@ -184,8 +280,6 @@ fn setup_logging(verbosity: u8) { fn cleanup_orphans() -> Result<()> { use anyhow::Context; use std::fs; - #[cfg(target_os = "linux")] - use std::path::PathBuf; use std::time::{Duration, SystemTime}; use tracing::{debug, info}; @@ -228,6 +322,12 @@ fn cleanup_orphans() -> Result<()> { .file_name() .and_then(|n| n.to_str()) .unwrap_or("unknown"); + if !httpjail::jail::valid_jail_id(jail_id) + || httpjail::jail::canary_owner_alive(&path) + { + debug!("Skipping live or invalid jail canary: {:?}", path); + continue; + } info!( "Found orphaned jail '{}' via canary file (age: {:?}), cleaning up", @@ -267,32 +367,14 @@ fn cleanup_orphans() -> Result<()> { debug!("Canary directory does not exist"); } - // On Linux, also scan for orphaned namespace configs directly - // This handles cases where canary files were deleted (e.g., /tmp cleanup) - #[cfg(target_os = "linux")] - { - let netns_dir = PathBuf::from("/etc/netns"); - if netns_dir.exists() { - debug!("Scanning for orphaned namespace configs in {:?}", netns_dir); - for entry in fs::read_dir(&netns_dir)? { - let entry = entry?; - let path = entry.path(); - let name = path.file_name().and_then(|n| n.to_str()).unwrap_or(""); - - // Only process httpjail namespace configs - if name.starts_with("httpjail_") && !cleaned_jails.contains(name) { - info!( - "Found orphaned namespace config '{}' without canary file, cleaning up", - name - ); + // A namespace without a trusted canary may still belong to a live older + // jail (possibly under another user's legacy HOME). Never infer it is dead + // just from a missing canary. Orphaned canaryless namespaces need manual + // administrator inspection instead of automatic destructive cleanup. - ::cleanup_orphaned( - name, - )?; - cleaned_jails.insert(name.to_string()); - } - } - } + #[cfg(target_os = "linux")] + if unsafe { libc::geteuid() == 0 } { + httpjail::jail::linux::docker::cleanup_orphaned_docker_tables()?; } if cleaned_jails.is_empty() { @@ -396,25 +478,68 @@ async fn main() -> Result<()> { // Build rule engine based on script or JS let request_log = if let Some(path) = &args.run_args.request_log { - Some(Arc::new(Mutex::new( - OpenOptions::new() - .create(true) - .append(true) - .open(path) - .with_context(|| format!("Failed to open request log file: {}", path))?, - ))) + let mut options = OpenOptions::new(); + options.create(true).append(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + #[cfg(target_os = "linux")] + let file = if unsafe { libc::geteuid() == 0 } { + open_request_log_as_invoker(path) + } else { + options.open(path).map_err(Into::into) + }; + #[cfg(not(target_os = "linux"))] + let file: Result = options.open(path).map_err(Into::into); + Some(Arc::new(Mutex::new(file.with_context(|| { + format!("Failed to open request log file: {}", path) + })?))) } else { None }; + #[cfg(target_os = "linux")] + // Dry-run evaluates rules too; it must not bypass root rule-engine restrictions. + let strong_rule = unsafe { libc::geteuid() == 0 }; + let rule_engine = if let Some(script) = &args.run_args.sh { + #[cfg(target_os = "linux")] + let script = if strong_rule { + trusted_policy_path(script, true)? + } else { + script.clone() + }; + #[cfg(not(target_os = "linux"))] + let script = script.clone(); info!("Using shell script rule evaluation: {}", script); - let shell_engine = Box::new(ShellRuleEngine::new(script.clone())); - RuleEngine::from_trait(shell_engine, request_log) + let shell_engine = ShellRuleEngine::new(script); + #[cfg(target_os = "linux")] + let shell_engine = if strong_rule { + shell_engine.restricted() + } else { + shell_engine + }; + RuleEngine::from_trait(Box::new(shell_engine), request_log) } else if let Some(proc) = &args.run_args.proc { + #[cfg(target_os = "linux")] + let proc = if strong_rule { + trusted_policy_path(proc, false)? + } else { + proc.clone() + }; + #[cfg(not(target_os = "linux"))] + let proc = proc.clone(); info!("Using line processor rule evaluation: {}", proc); - let proc_engine = Box::new(ProcRuleEngine::new(proc.clone())); - RuleEngine::from_trait(proc_engine, request_log) + let proc_engine = ProcRuleEngine::new(proc); + #[cfg(target_os = "linux")] + let proc_engine = if strong_rule { + proc_engine.restricted() + } else { + proc_engine + }; + RuleEngine::from_trait(Box::new(proc_engine), request_log) } else if let Some(js_code) = &args.run_args.js { info!("Using V8 JavaScript rule evaluation"); let js_engine = match V8JsRuleEngine::new(js_code.clone()) { @@ -429,8 +554,14 @@ async fn main() -> Result<()> { info!("Using V8 JavaScript rule evaluation from file: {}", js_file); let code = std::fs::read_to_string(js_file) .with_context(|| format!("Failed to read JS file: {}", js_file))?; - let js_file_path = std::path::PathBuf::from(js_file); - let js_engine = match V8JsRuleEngine::new_with_file(code, Some(js_file_path)) { + // A jailed command may share write access to this file. Freeze the + // policy while it executes; hot reload is safe only in server mode, + // where no untrusted command is launched by this process. + let reload_path = args + .run_args + .server + .then(|| std::path::PathBuf::from(js_file)); + let js_engine = match V8JsRuleEngine::new_with_file(code, reload_path) { Ok(engine) => Box::new(engine), Err(e) => { eprintln!("Failed to create V8 JavaScript engine: {}", e); @@ -689,27 +820,20 @@ async fn main() -> Result<()> { let status = if let Some(timeout_secs) = args.run_args.timeout { info!("Executing command with {}s timeout", timeout_secs); - // Use tokio to handle timeout let command = args.run_args.exec_command.clone(); - let extra_env_clone = extra_env.clone(); let jail_clone = jail.clone(); - - // We need to use spawn_blocking since jail.execute is blocking - let handle = - tokio::task::spawn_blocking(move || jail_clone.execute(&command, &extra_env_clone)); - - // Apply timeout to the blocking task - match tokio::time::timeout(std::time::Duration::from_secs(timeout_secs), handle).await { - Ok(Ok(result)) => result?, - Ok(Err(e)) => anyhow::bail!("Task execution failed: {}", e), - Err(_) => { - warn!("Command timed out after {}s", timeout_secs); - // Note: We can't actually kill the process from here since it's in a separate - // process/namespace. The process will continue running but we return timeout. - // This matches the behavior of GNU timeout when it can't kill the process. - std::process::ExitStatus::from_raw(124 << 8) - } + let duration = std::time::Duration::from_secs(timeout_secs); + // Keep the blocking task alive until it has killed and reaped the child. + // Detaching a timed-out spawn_blocking task would let it run after jail cleanup. + let status = tokio::task::spawn_blocking(move || { + jail_clone.execute_with_timeout(&command, &extra_env, duration) + }) + .await + .context("Command execution task failed")??; + if status.code() == Some(124) { + warn!("Command timed out after {}s", timeout_secs); } + status } else { jail.execute(&args.run_args.exec_command, &extra_env)? }; diff --git a/src/proxy.rs b/src/proxy.rs index 373251eb..a993ce0a 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -12,7 +12,7 @@ use hyper::service::service_fn; use hyper::{Error as HyperError, Request, Response, StatusCode, Uri}; use hyper_rustls::HttpsConnectorBuilder; use hyper_util::client::legacy::Client; -use hyper_util::rt::{TokioExecutor, TokioIo}; +use hyper_util::rt::{TokioExecutor, TokioIo, TokioTimer}; use rand::Rng; #[cfg(target_os = "linux")] @@ -68,8 +68,8 @@ use crate::limited_body::LimitedBody; pub enum ByteLimitResult { /// Request is within limit (or no Content-Length), proceed with wrapped body WithinLimit(Box>>), - /// Request exceeds limit based on Content-Length header - ExceedsLimit { content_length: u64, max_bytes: u64 }, + /// Request exceeds limit based on headers or a Content-Length header + ExceedsLimit { request_size: u64, max_bytes: u64 }, } /// Applies a byte limit to an outgoing request by wrapping its body. @@ -117,6 +117,14 @@ pub fn apply_request_byte_limit( // Final "\r\n" separator between headers and body let total_header_size = request_line_size + headers_size + 2; + // Header bytes are transmitted even when there is no Content-Length/body. + if total_header_size > max_bytes { + return ByteLimitResult::ExceedsLimit { + request_size: total_header_size, + max_bytes, + }; + } + // Check Content-Length as a heuristic to reject oversized requests early // This both provides convenience (immediate error) and prevents hangs if let Some(content_length) = parts @@ -125,7 +133,7 @@ pub fn apply_request_byte_limit( .and_then(|v| v.to_str().ok()) .and_then(|s| s.parse::().ok()) { - let total_size = total_header_size + content_length; + let total_size = total_header_size.saturating_add(content_length); if total_size > max_bytes { debug!( content_length = content_length, @@ -135,7 +143,7 @@ pub fn apply_request_byte_limit( "Request exceeds byte limit based on Content-Length" ); return ByteLimitResult::ExceedsLimit { - content_length, + request_size: total_size, max_bytes, }; } @@ -476,6 +484,17 @@ impl ProxyServer { } } +/// Limit only request-header parsing, not long-lived response bodies or upgrades. +pub(crate) fn http1_builder() -> http1::Builder { + let mut builder = http1::Builder::new(); + builder + .preserve_header_case(true) + .title_case_headers(false) + .timer(TokioTimer::new()) + .header_read_timeout(Duration::from_secs(10)); + builder +} + /// Generic listener task spawner to avoid code duplication between HTTP and HTTPS fn spawn_listener_task( listener: TcpListener, @@ -517,11 +536,7 @@ async fn handle_http_connection( let io = TokioIo::new(stream); let service = service_fn(move |req| handle_http_request(req, context.clone(), remote_addr)); - http1::Builder::new() - .preserve_header_case(true) - .title_case_headers(false) - .serve_connection(io, service) - .await?; + http1_builder().serve_connection(io, service).await?; Ok(()) } @@ -535,6 +550,18 @@ async fn handle_https_connection( crate::proxy_tls::handle_https_connection(stream, context, remote_addr).await } +/// Detect our nonce among all loop-prevention headers (including chained proxies). +pub(crate) fn has_loop_nonce(headers: &hyper::HeaderMap, nonce: &str) -> bool { + headers + .get_all(HTTPJAIL_LOOP_DETECTION_HEADER) + .iter() + .any(|value| { + value + .to_str() + .is_ok_and(|value| value.split(',').any(|part| part.trim() == nonce)) + }) +} + pub async fn handle_http_request( req: Request, context: ProxyContext, @@ -544,23 +571,11 @@ pub async fn handle_http_request( let uri = req.uri().clone(); let headers = req.headers().clone(); - // SECURITY: Check for loop detection header (Issue #84) - // HTTP supports multiple values for the same header name. - // Each httpjail instance adds its nonce; if we see our own, it's a loop. - let our_nonce = context.loop_nonce.as_str(); - for value in headers.get_all(HTTPJAIL_LOOP_DETECTION_HEADER).iter() { - if let Ok(nonce) = value.to_str() { - if nonce == our_nonce { - debug!( - "Loop detected: our nonce '{}' found in request to {}", - nonce, uri - ); - return create_forbidden_response(Some( - "Loop detected: request already processed by this httpjail instance" - .to_string(), - )); - } - } + if has_loop_nonce(&headers, &context.loop_nonce) { + debug!("Loop detected in HTTP request to {}", uri); + return create_forbidden_response(Some( + "Loop detected: request already processed by this httpjail instance".to_string(), + )); } // Check if the URI already contains the full URL (proxy request) @@ -628,13 +643,12 @@ async fn proxy_request( match apply_request_byte_limit(prepared_req, max_bytes) { ByteLimitResult::WithinLimit(req) => *req, ByteLimitResult::ExceedsLimit { - content_length, + request_size, max_bytes, } => { - // Request exceeds limit based on Content-Length - reject immediately let message = format!( - "Request body size ({} bytes) exceeds maximum allowed ({} bytes)", - content_length, max_bytes + "Request size ({} bytes) exceeds maximum allowed ({} bytes)", + request_size, max_bytes ); return Ok(create_error_response( StatusCode::PAYLOAD_TOO_LARGE, @@ -726,6 +740,64 @@ mod tests { use super::*; use crate::rules::v8_js::V8JsRuleEngine; + #[test] + fn loop_nonce_matches_any_header_value() { + let mut headers = hyper::HeaderMap::new(); + headers.append(HTTPJAIL_LOOP_DETECTION_HEADER, "other".parse().unwrap()); + headers.append( + HTTPJAIL_LOOP_DETECTION_HEADER, + "foreign, ours".parse().unwrap(), + ); + assert!(has_loop_nonce(&headers, "ours")); + assert!(!has_loop_nonce(&headers, "unknown")); + } + + #[tokio::test] + async fn incomplete_http_header_expires() { + use tokio::io::AsyncWriteExt; + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let mut client = TcpStream::connect(listener.local_addr().unwrap()) + .await + .unwrap(); + let server = tokio::spawn(async move { + let (stream, _) = listener.accept().await.unwrap(); + let service = service_fn(|_: Request| async { + Ok::<_, std::convert::Infallible>(Response::new(Full::new(Bytes::new()))) + }); + let mut builder = http1_builder(); + builder.header_read_timeout(Duration::from_millis(50)); + builder + .serve_connection(TokioIo::new(stream), service) + .await + }); + client + .write_all(b"GET / HTTP/1.1\r\nHost: localhost\r\n") + .await + .unwrap(); + let result = tokio::time::timeout(Duration::from_secs(1), server) + .await + .unwrap() + .unwrap(); + assert!(result.is_err(), "incomplete header was not timed out"); + } + + #[test] + fn zero_byte_limit_rejects_headers_without_content_length() { + let request = Request::builder() + .uri("http://example.invalid/?token=canary") + .header("X-Canary", "secret") + .body( + http_body_util::Empty::::new() + .map_err(|never| match never {}) + .boxed(), + ) + .unwrap(); + assert!(matches!( + apply_request_byte_limit(request, 0), + ByteLimitResult::ExceedsLimit { .. } + )); + } + #[tokio::test] async fn test_proxy_server_creation() { let js = r"/^github\.com$/.test(r.host)"; diff --git a/src/proxy_tls.rs b/src/proxy_tls.rs index f7c1da56..75da98c2 100644 --- a/src/proxy_tls.rs +++ b/src/proxy_tls.rs @@ -1,6 +1,6 @@ use crate::proxy::{ HTTPJAIL_HEADER, HTTPJAIL_HEADER_VALUE, ProxyContext, apply_request_byte_limit, - create_connect_403_response_with_context, create_forbidden_response, + create_connect_403_response_with_context, create_forbidden_response, has_loop_nonce, }; use crate::rules::Action; #[cfg(target_os = "macos")] @@ -9,14 +9,13 @@ use anyhow::Result; use bytes::Bytes; use http_body_util::{BodyExt, combinators::BoxBody}; use hyper::body::Incoming; -use hyper::server::conn::http1; use hyper::service::service_fn; use hyper::{Error as HyperError, Method, Request, Response, StatusCode, Uri}; use hyper_util::rt::TokioIo; use rustls::ServerConfig; use std::sync::Arc; use tls_parser::{TlsMessage, parse_tls_plaintext}; -use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; +use tokio::io::{AsyncBufReadExt, AsyncReadExt, AsyncWriteExt, BufReader}; use tokio::net::TcpStream; use tokio::time::{Duration, Instant, timeout}; use tokio_rustls::TlsAcceptor; @@ -26,6 +25,8 @@ use tracing::{debug, error, info, warn}; const PROTOCOL_DETECT_TIMEOUT: Duration = Duration::from_secs(5); // Timeout for reading CONNECT headers const CONNECT_READ_TIMEOUT: Duration = Duration::from_secs(10); +const MAX_CONNECT_LINE_BYTES: usize = 8 * 1024; +const MAX_CONNECT_HEADERS_BYTES: usize = 64 * 1024; // Timeout for TLS handshake const TLS_HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(10); // Timeout for writing responses @@ -218,15 +219,32 @@ async fn handle_transparent_tls( }); debug!("Starting HTTP/1.1 server for decrypted requests"); - http1::Builder::new() - .preserve_header_case(true) - .title_case_headers(false) + crate::proxy::http1_builder() .serve_connection(io, service) .await?; Ok(()) } +// Use a bounded reader: `read_line` by itself buffers an arbitrarily large header. +async fn read_connect_line( + reader: &mut BufReader, + limit: usize, +) -> std::io::Result> { + let mut line = String::new(); + let count = reader.take((limit + 1) as u64).read_line(&mut line).await?; + if count == 0 { + return Ok(None); + } + if count > limit || !line.ends_with('\n') { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "CONNECT header line exceeds limit or is incomplete", + )); + } + Ok(Some(line)) +} + /// Handle a CONNECT tunnel request with TLS interception async fn handle_connect_tunnel( stream: TcpStream, @@ -237,16 +255,20 @@ async fn handle_connect_tunnel( // Buffer the stream for reading lines let mut reader = BufReader::new(stream); - let mut first_line = String::new(); - // Read the first line to get the CONNECT request - let read_result = timeout(CONNECT_READ_TIMEOUT, reader.read_line(&mut first_line)).await; - match read_result { - Ok(Ok(0)) => { + // Read the first line to get the CONNECT request. A timeout alone does not + // prevent a client from making us buffer an arbitrarily large line. + let first_line = match timeout( + CONNECT_READ_TIMEOUT, + read_connect_line(&mut reader, MAX_CONNECT_LINE_BYTES), + ) + .await + { + Ok(Ok(Some(line))) => line, + Ok(Ok(None)) => { debug!("Connection closed before CONNECT request"); return Ok(()); } - Ok(Ok(_)) => {} Ok(Err(e)) => { debug!("Failed to read CONNECT request: {}", e); return Ok(()); @@ -255,7 +277,7 @@ async fn handle_connect_tunnel( warn!("Timeout reading CONNECT request"); return Ok(()); } - } + }; debug!("CONNECT line: {}", first_line.trim()); @@ -274,33 +296,32 @@ async fn handle_connect_tunnel( info!("CONNECT request for: {}", target); - // Read the rest of the headers until we find the empty line - let mut headers = vec![first_line.clone()]; + // Require the terminating blank line and cap both individual lines and + // aggregate header bytes before evaluating any policy or opening a tunnel. + let mut total_bytes = first_line.len(); let start_time = tokio::time::Instant::now(); loop { - // Check if we've exceeded the total timeout - if start_time.elapsed() > CONNECT_READ_TIMEOUT { - warn!("Timeout reading CONNECT headers"); + let remaining_time = CONNECT_READ_TIMEOUT.saturating_sub(start_time.elapsed()); + if remaining_time.is_zero() || total_bytes >= MAX_CONNECT_HEADERS_BYTES { + debug!("CONNECT headers timed out or exceeded limit"); return Ok(()); } - - let mut line = String::new(); - let remaining_time = CONNECT_READ_TIMEOUT.saturating_sub(start_time.elapsed()); - match timeout(remaining_time, reader.read_line(&mut line)).await { - Ok(Ok(0)) => break, - Ok(Ok(_)) => { + let line_limit = MAX_CONNECT_LINE_BYTES.min(MAX_CONNECT_HEADERS_BYTES - total_bytes); + match timeout(remaining_time, read_connect_line(&mut reader, line_limit)).await { + Ok(Ok(Some(line))) => { + total_bytes += line.len(); if line == "\r\n" || line == "\n" { break; } - headers.push(line); } + Ok(Ok(None)) => return Ok(()), Ok(Err(e)) => { - debug!("Error reading header: {}", e); - break; + debug!("Invalid CONNECT header: {}", e); + return Ok(()); } Err(_) => { - warn!("Timeout reading headers"); - break; + debug!("Timeout reading CONNECT headers"); + return Ok(()); } } } @@ -428,9 +449,7 @@ async fn perform_tls_interception( }); debug!("Starting HTTP/1.1 server for decrypted requests"); - http1::Builder::new() - .preserve_header_case(true) - .title_case_headers(false) + crate::proxy::http1_builder() .serve_connection(io, service) .await?; @@ -449,9 +468,7 @@ async fn handle_plain_http( let service = service_fn(move |req| crate::proxy::handle_http_request(req, context.clone(), remote_addr)); - http1::Builder::new() - .preserve_header_case(true) - .title_case_headers(false) + crate::proxy::http1_builder() .serve_connection(io, service) .await?; @@ -468,6 +485,14 @@ async fn handle_decrypted_https_request( let method = req.method().clone(); let uri = req.uri().clone(); + // Mirror the HTTP handler's loop guard before any rule evaluation or + // upstream connection (including when HTTPS is bound on port 443). + if has_loop_nonce(req.headers(), &context.loop_nonce) { + return create_forbidden_response(Some( + "Loop detected: request already processed by this httpjail instance".to_string(), + )); + } + // Build the full URL for rule evaluation let path = uri.path_and_query().map(|pq| pq.as_str()).unwrap_or("/"); let full_url = format!("https://{}{}", host, path); @@ -529,13 +554,12 @@ async fn proxy_https_request( match apply_request_byte_limit(prepared_req, max_bytes) { crate::proxy::ByteLimitResult::WithinLimit(req) => *req, crate::proxy::ByteLimitResult::ExceedsLimit { - content_length, + request_size, max_bytes, } => { - // Request exceeds limit based on Content-Length - reject immediately let message = format!( - "Request body size ({} bytes) exceeds maximum allowed ({} bytes)", - content_length, max_bytes + "Request size ({} bytes) exceeds maximum allowed ({} bytes)", + request_size, max_bytes ); return Ok(crate::proxy::create_error_response( StatusCode::PAYLOAD_TOO_LARGE, @@ -752,6 +776,37 @@ mod tests { ); } + #[tokio::test] + async fn test_connect_oversized_header_is_closed() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let context = ProxyContext { + rule_engine: create_test_rule_engine(true), + cert_manager: create_test_cert_manager().await, + loop_nonce: Arc::new("test-nonce".to_string()), + }; + tokio::spawn(async move { + let (stream, peer) = listener.accept().await.unwrap(); + handle_connect_tunnel(stream, context, peer).await.unwrap(); + }); + + let mut client = TcpStream::connect(addr).await.unwrap(); + client + .write_all(b"CONNECT example.com:443 HTTP/1.1\r\nX-Pad: ") + .await + .unwrap(); + client + .write_all(&vec![b'a'; MAX_CONNECT_LINE_BYTES + 1]) + .await + .unwrap(); + let mut response = [0; 64]; + let count = timeout(Duration::from_secs(2), client.read(&mut response)) + .await + .unwrap() + .unwrap(); + assert_eq!(count, 0, "oversized CONNECT header must not open a tunnel"); + } + #[tokio::test] async fn test_connect_tunnel_denied() { // Start a test proxy server diff --git a/src/rules/common.rs b/src/rules/common.rs index 0bcffe49..d029b4ab 100644 --- a/src/rules/common.rs +++ b/src/rules/common.rs @@ -117,7 +117,7 @@ impl RuleResponse { /// Convert to evaluation result tuple (allowed, context, max_tx_bytes) /// Following the rules: - /// - If deny_message exists but allow is not set, default to deny + /// - Only an explicit allow policy permits traffic; a missing policy denies /// - Only include context message when denying /// - max_tx_bytes is returned when allow policy has a byte limit pub fn to_evaluation_result(&self) -> (bool, Option, Option) { @@ -125,15 +125,7 @@ impl RuleResponse { Some(AllowPolicy::Bool(true)) => (true, None, None), Some(AllowPolicy::Bool(false)) => (false, self.deny_message.clone(), None), Some(AllowPolicy::Limited { max_tx_bytes }) => (true, None, Some(*max_tx_bytes)), - None => { - // If allow is not specified but deny_message exists, default to deny - let allowed = self.deny_message.is_none(); - if allowed { - (true, None, None) - } else { - (false, self.deny_message.clone(), None) - } - } + None => (false, self.deny_message.clone(), None), } } } @@ -224,12 +216,9 @@ mod tests { (false, Some("blocked".to_string()), None) ); - // Neither field set (defaults to allow) - let resp = RuleResponse { - allow: None, - deny_message: None, - }; - assert_eq!(resp.to_evaluation_result(), (true, None, None)); + // An empty object must not silently bypass the default-deny policy. + let resp = RuleResponse::from_string("{}"); + assert_eq!(resp.to_evaluation_result(), (false, None, None)); } #[test] diff --git a/src/rules/proc.rs b/src/rules/proc.rs index cb3b5ea0..f777757b 100644 --- a/src/rules/proc.rs +++ b/src/rules/proc.rs @@ -11,9 +11,41 @@ use tokio::process::{Child, ChildStdin, ChildStdout, Command}; use tokio::sync::Mutex; use tracing::{debug, error, warn}; +const MAX_RESPONSE_BYTES: usize = 64 * 1024; + +async fn read_bounded_line( + reader: &mut R, + bytes: &mut Vec, +) -> std::io::Result { + loop { + let chunk = reader.fill_buf().await?; + if chunk.is_empty() { + return Ok(bytes.len()); + } + let end = chunk + .iter() + .position(|byte| *byte == b'\n') + .map_or(chunk.len(), |pos| pos + 1); + let copy = end.min(MAX_RESPONSE_BYTES + 1 - bytes.len()); + let complete = chunk[copy - 1] == b'\n'; + bytes.extend_from_slice(&chunk[..copy]); + reader.consume(copy); + if bytes.len() > MAX_RESPONSE_BYTES { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "Program response exceeds 64 KiB", + )); + } + if complete { + return Ok(bytes.len()); + } + } +} + pub struct ProcRuleEngine { program: String, process: Arc>>, + restricted: bool, } /// State for a running process @@ -29,9 +61,15 @@ impl ProcRuleEngine { ProcRuleEngine { program, process: Arc::new(Mutex::new(None)), + restricted: false, } } + pub fn restricted(mut self) -> Self { + self.restricted = true; + self + } + /// Explicitly kill a process by taking ownership and dropping it /// This is more self-documenting than relying on implicit drops #[inline] @@ -72,7 +110,23 @@ impl ProcRuleEngine { if process_guard.is_none() { debug!("Starting program process: {}", self.program); - let mut cmd = Command::new(&self.program); + let mut cmd = if self.restricted { + let mut cmd = Command::new("/usr/bin/setpriv"); + cmd.args([ + "--no-new-privs", + "--reuid=65534", + "--regid=65534", + "--clear-groups", + "--", + &self.program, + ]); + cmd + } else { + Command::new(&self.program) + }; + if self.restricted { + cmd.env_clear().env("PATH", "/usr/sbin:/usr/bin:/sbin:/bin"); + } cmd.stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::inherit()) @@ -119,29 +173,40 @@ impl ProcRuleEngine { let mut request_line = json_request.to_string(); request_line.push('\n'); - if let Err(e) = process_state.stdin.write_all(request_line.as_bytes()).await { - error!("Failed to write to program stdin: {}", e); - if let Some(state) = process_guard.take() { - Self::kill_process(state); + let timeout = Duration::from_secs(5); + // A stopped processor can fill its stdin pipe before we start the + // response timeout. Bound writes and flushes by the same deadline. + match tokio::time::timeout(timeout, async { + process_state + .stdin + .write_all(request_line.as_bytes()) + .await?; + process_state.stdin.flush().await + }) + .await + { + Ok(Ok(())) => {} + Ok(Err(e)) => { + error!("Failed to write to program stdin: {}", e); + if let Some(state) = process_guard.take() { + Self::kill_process(state); + } + return Err("Failed to write to program".to_string()); } - return Err("Failed to write to program".to_string()); - } - - if let Err(e) = process_state.stdin.flush().await { - error!("Failed to flush stdin: {}", e); - if let Some(state) = process_guard.take() { - Self::kill_process(state); + Err(_) => { + warn!("Program stdin timeout after {:?}", timeout); + if let Some(state) = process_guard.take() { + Self::kill_process(state); + } + return Err("Program stdin timeout".to_string()); } - return Err("Failed to flush stdin".to_string()); } - // Read response with timeout - let timeout = Duration::from_secs(5); - let mut response_line = String::new(); + // Bound both wait time and memory even when the processor never sends a newline. + let mut response_bytes = Vec::new(); + let read = read_bounded_line(&mut process_state.stdout, &mut response_bytes); - match tokio::time::timeout(timeout, process_state.stdout.read_line(&mut response_line)) - .await - { + match tokio::time::timeout(timeout, read).await { Ok(Ok(0)) => { // EOF - process exited warn!("Program closed stdout unexpectedly"); @@ -151,6 +216,7 @@ impl ProcRuleEngine { Err("Program closed unexpectedly".to_string()) } Ok(Ok(_)) => { + let response_line = String::from_utf8_lossy(&response_bytes); let response = response_line.trim(); debug!("Program response: {}", response); @@ -247,6 +313,12 @@ impl ProcRuleEngine { } Err(e) => { debug!("Request failed: {}", e); + if e == "Program stdin timeout" || e == "Program response timeout" { + // An unresponsive processor must not hold the shared mutex + // through another full timeout for the same request. + return EvaluationResult::deny() + .with_context("Program evaluation timed out".to_string()); + } if attempt == 0 { debug!("Retrying with fresh process"); // Process will be restarted on next iteration by send_request_to_process @@ -290,6 +362,25 @@ mod tests { use std::io::Write; use tempfile::{NamedTempFile, TempPath}; + #[tokio::test] + async fn unterminated_processor_response_is_bounded() { + let mut reader = BufReader::new(tokio::io::repeat(b'x')); + let mut bytes = Vec::new(); + let error = read_bounded_line(&mut reader, &mut bytes) + .await + .unwrap_err(); + assert_eq!(error.kind(), std::io::ErrorKind::InvalidData); + assert_eq!(bytes.len(), MAX_RESPONSE_BYTES + 1); + + let mut reader = BufReader::new(&b"true\nfalse\n"[..]); + bytes.clear(); + read_bounded_line(&mut reader, &mut bytes).await.unwrap(); + assert_eq!(bytes, b"true\n"); + bytes.clear(); + read_bounded_line(&mut reader, &mut bytes).await.unwrap(); + assert_eq!(bytes, b"false\n"); + } + /// Helper function to create an executable program file with the given content fn create_program_file(content: &str) -> TempPath { let mut program_file = NamedTempFile::new().unwrap(); diff --git a/src/rules/shell.rs b/src/rules/shell.rs index 49dda05a..8fdf7dfc 100644 --- a/src/rules/shell.rs +++ b/src/rules/shell.rs @@ -5,14 +5,43 @@ use std::time::Duration; use tracing::debug; use url::Url; +const MAX_RULE_OUTPUT: usize = 64 * 1024; + +async fn read_bounded_output( + reader: R, +) -> std::io::Result> { + use tokio::io::AsyncReadExt; + let mut bytes = Vec::new(); + reader + .take((MAX_RULE_OUTPUT + 1) as u64) + .read_to_end(&mut bytes) + .await?; + if bytes.len() > MAX_RULE_OUTPUT { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "Rule output exceeds 64 KiB", + )); + } + Ok(bytes) +} + #[derive(Clone)] pub struct ShellRuleEngine { script: String, + restricted: bool, } impl ShellRuleEngine { pub fn new(script: String) -> Self { - ShellRuleEngine { script } + ShellRuleEngine { + script, + restricted: false, + } + } + + pub fn restricted(mut self) -> Self { + self.restricted = true; + self } async fn execute_script( @@ -39,7 +68,18 @@ impl ShellRuleEngine { ); // Build the command - let mut cmd = if self.script.contains(' ') { + let mut cmd = if self.restricted { + let mut cmd = tokio::process::Command::new("/usr/bin/setpriv"); + cmd.args([ + "--no-new-privs", + "--reuid=65534", + "--regid=65534", + "--clear-groups", + "--", + &self.script, + ]); + cmd + } else if self.script.contains(' ') { let mut cmd = tokio::process::Command::new("sh"); cmd.arg("-c").arg(&self.script); cmd @@ -47,6 +87,10 @@ impl ShellRuleEngine { tokio::process::Command::new(&self.script) }; + if self.restricted { + cmd.env_clear().env("PATH", "/usr/sbin:/usr/bin:/sbin:/bin"); + } + cmd.env("HTTPJAIL_URL", url) .env("HTTPJAIL_METHOD", method.as_str()) .env("HTTPJAIL_SCHEME", scheme) @@ -58,7 +102,7 @@ impl ShellRuleEngine { .kill_on_drop(true); // Ensure child is killed if dropped // Spawn the child process - let child = match cmd.spawn() { + let mut child = match cmd.spawn() { Ok(child) => child, Err(e) => { debug!("Failed to spawn script: {}", e); @@ -66,25 +110,34 @@ impl ShellRuleEngine { } }; - // Wait for completion with timeout + // Drain both pipes concurrently, but fail as soon as either exceeds the + // limit. wait_with_output would buffer an unbounded stream until timeout. + let stdout = child.stdout.take().expect("piped stdout"); + let stderr = child.stderr.take().expect("piped stderr"); + let output = async { + let (stdout, stderr) = + tokio::try_join!(read_bounded_output(stdout), read_bounded_output(stderr))?; + let status = child.wait().await?; + Ok::<_, std::io::Error>((status, stdout, stderr)) + }; let timeout = Duration::from_secs(30); - match tokio::time::timeout(timeout, child.wait_with_output()).await { - Ok(Ok(output)) => { - let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + match tokio::time::timeout(timeout, output).await { + Ok(Ok((status, stdout, stderr))) => { + let stdout = String::from_utf8_lossy(&stdout).trim().to_string(); + let stderr = String::from_utf8_lossy(&stderr).trim().to_string(); if !stderr.is_empty() { debug!("Script stderr: {}", stderr); } - let allowed = output.status.success(); + let allowed = status.success(); debug!( "Script returned {} for {} {} (exit code: {:?})", if allowed { "ALLOW" } else { "DENY" }, method, url, - output.status.code() + status.code() ); (allowed, stdout) @@ -136,6 +189,28 @@ mod tests { use std::fs; use tempfile::NamedTempFile; + #[cfg(target_os = "linux")] + #[tokio::test] + async fn test_restricted_rule_runs_without_root() { + if unsafe { libc::geteuid() } != 0 { + return; + } + let engine = ShellRuleEngine::new("/usr/bin/id".to_string()).restricted(); + let (allowed, stdout) = engine + .execute_script(Method::GET, "https://example.invalid/", "127.0.0.1") + .await; + assert!(allowed, "{stdout}"); + assert!(stdout.contains("uid=65534"), "{stdout}"); + } + + #[tokio::test] + async fn unbounded_rule_output_is_rejected() { + let error = read_bounded_output(tokio::io::repeat(b'x')) + .await + .unwrap_err(); + assert_eq!(error.kind(), std::io::ErrorKind::InvalidData); + } + #[tokio::test] async fn test_script_allow() { let mut script_file = NamedTempFile::new().unwrap(); diff --git a/src/tls.rs b/src/tls.rs index c5ed8f1b..d20b267c 100644 --- a/src/tls.rs +++ b/src/tls.rs @@ -3,7 +3,8 @@ use camino::Utf8PathBuf; use lru::LruCache; use rcgen::{Certificate, CertificateParams, DistinguishedName, DnType, KeyPair, SanType}; use rustls::pki_types::{CertificateDer, PrivateKeyDer}; -use std::fs; +use std::fs::{self, OpenOptions}; +use std::io::{Read, Write}; use std::num::NonZeroUsize; use std::path::PathBuf; use std::sync::{Arc, RwLock}; @@ -14,6 +15,29 @@ use tracing::{debug, info}; #[cfg(target_os = "macos")] use crate::macos_keychain::KeychainManager; +fn default_ca_dir() -> Result { + #[cfg(target_os = "linux")] + if unsafe { libc::geteuid() == 0 } { + let dir = std::path::Path::new("/var/lib/httpjail/ca"); + crate::jail::ensure_trusted_root_dir(dir)?; + // The signing key remains 0600, but jailed clients must traverse these + // trusted directories to read the public certificate. + use std::os::unix::fs::PermissionsExt; + for public_dir in [dir.parent().expect("CA parent"), dir] { + std::fs::set_permissions(public_dir, std::fs::Permissions::from_mode(0o755))?; + } + return dir + .to_path_buf() + .try_into() + .map_err(|_| anyhow::anyhow!("Invalid CA directory path")); + } + dirs::config_dir() + .context("Could not find user config directory")? + .join("httpjail") + .try_into() + .context("Config directory path is not valid UTF-8") +} + const CERT_CACHE_SIZE: usize = 1024; /// Manages TLS certificates for HTTPS interception @@ -41,16 +65,39 @@ impl CertificateManager { let config_dir = if let Some(dir) = config_dir { dir.clone() } else { - dirs::config_dir() - .context("Could not find user config directory")? - .join("httpjail") - .try_into() - .context("Config directory path is not valid UTF-8")? + default_ca_dir()? }; // Create directory if it doesn't exist fs::create_dir_all(&config_dir).context("Failed to create config directory")?; + #[cfg(target_os = "linux")] + let _ca_lock = if unsafe { libc::geteuid() == 0 } { + use std::os::fd::AsRawFd; + use std::os::unix::fs::OpenOptionsExt; + // Concurrent strong jails must not see a half-written cert/key pair. + let lock = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .mode(0o600) + .custom_flags(libc::O_NOFOLLOW) + .open(config_dir.join(".ca.lock"))?; + use std::os::unix::fs::MetadataExt; + let metadata = lock.metadata()?; + anyhow::ensure!( + metadata.is_file() && metadata.uid() == 0, + "CA lock must be root-owned and regular" + ); + anyhow::ensure!( + unsafe { libc::flock(lock.as_raw_fd(), libc::LOCK_EX) } == 0, + "Failed to lock CA directory" + ); + Some(lock) + } else { + None + }; + let ca_cert_path = config_dir.join("ca-cert.pem"); let ca_key_path = config_dir.join("ca-key.pem"); @@ -58,8 +105,54 @@ impl CertificateManager { if ca_cert_path.exists() && ca_key_path.exists() { debug!("Loading cached CA certificate from {:?}", ca_cert_path); + #[cfg(target_os = "linux")] + let _cert_pem = if unsafe { libc::geteuid() == 0 } { + use std::os::unix::fs::{MetadataExt, OpenOptionsExt, PermissionsExt}; + let mut cert = OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(&ca_cert_path) + .context("Failed to open public CA certificate")?; + let metadata = cert.metadata()?; + anyhow::ensure!( + metadata.is_file() && metadata.uid() == 0, + "CA certificate must be root-owned and regular" + ); + cert.set_permissions(fs::Permissions::from_mode(0o644))?; + let mut pem = String::new(); + cert.read_to_string(&mut pem)?; + pem + } else { + fs::read_to_string(&ca_cert_path).context("Failed to read CA certificate")? + }; + #[cfg(not(target_os = "linux"))] let _cert_pem = fs::read_to_string(&ca_cert_path).context("Failed to read CA certificate")?; + #[cfg(unix)] + let key_pem = { + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + let mut key = OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(&ca_key_path) + .context("Failed to open CA key")?; + let metadata = key.metadata()?; + anyhow::ensure!(metadata.is_file(), "CA key must be a regular file"); + #[cfg(target_os = "linux")] + if unsafe { libc::geteuid() == 0 } { + use std::os::unix::fs::MetadataExt; + anyhow::ensure!(metadata.uid() == 0, "Privileged CA key must be root-owned"); + } + if metadata.permissions().mode() & 0o077 != 0 { + key.set_permissions(fs::Permissions::from_mode(0o600)) + .context("Failed to restrict existing CA key permissions")?; + } + let mut pem = String::new(); + key.read_to_string(&mut pem) + .context("Failed to read CA key")?; + pem + }; + #[cfg(not(unix))] let key_pem = fs::read_to_string(&ca_key_path).context("Failed to read CA key")?; // Parse the PEM files @@ -117,18 +210,32 @@ impl CertificateManager { .self_signed(&ca_key_pair) .context("Failed to generate CA certificate")?; - // Save to disk - fs::write(&ca_cert_path, ca_cert.pem()).context("Failed to write CA certificate")?; - fs::write(&ca_key_path, ca_key_pair.serialize_pem()).context("Failed to write CA key")?; - - // Set permissions to 600 (read/write for owner only) + // Never follow a preexisting cert symlink into an unrelated root-owned file. + let mut cert_file = OpenOptions::new() + .write(true) + .create_new(true) + .open(&ca_cert_path) + .context("Failed to create exclusive CA certificate")?; + cert_file + .write_all(ca_cert.pem().as_bytes()) + .context("Failed to write CA certificate")?; + #[cfg(target_os = "linux")] + if unsafe { libc::geteuid() == 0 } { + use std::os::unix::fs::PermissionsExt; + cert_file.set_permissions(fs::Permissions::from_mode(0o644))?; + } + // Restrict permissions at creation, before any private-key bytes are written. + let mut key_file = OpenOptions::new(); + key_file.write(true).create_new(true); #[cfg(unix)] { - use std::os::unix::fs::PermissionsExt; - let mut perms = fs::metadata(&ca_key_path)?.permissions(); - perms.set_mode(0o600); - fs::set_permissions(&ca_key_path, perms)?; + use std::os::unix::fs::OpenOptionsExt; + key_file.mode(0o600); } + key_file + .open(&ca_key_path) + .and_then(|mut file| file.write_all(ca_key_pair.serialize_pem().as_bytes())) + .context("Failed to write CA key")?; info!("Saved new CA certificate to {}", ca_cert_path); @@ -298,13 +405,7 @@ impl CertificateManager { /// Get the path to the CA certificate file pub fn get_ca_cert_path() -> Result { - let config_dir = dirs::config_dir() - .context("Could not find user config directory")? - .join("httpjail"); - let config_dir: Utf8PathBuf = config_dir - .try_into() - .context("Config directory path is not valid UTF-8")?; - Ok(config_dir.join("ca-cert.pem")) + Ok(default_ca_dir()?.join("ca-cert.pem")) } /// Generate environment variables for common tools to use the CA certificate @@ -313,6 +414,14 @@ impl CertificateManager { // This handles cases where the effective user changes (e.g., sudo in CI) let mut ca_path = Self::get_ca_cert_path()?; + #[cfg(target_os = "linux")] + if unsafe { libc::geteuid() == 0 } { + anyhow::ensure!( + ca_path.exists(), + "Privileged CA certificate missing from trusted root directory" + ); + } + if !ca_path.exists() { // If not found in current user's config, check common locations let possible_paths = [ @@ -373,3 +482,40 @@ impl CertificateManager { Ok(env_vars) } } + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use std::os::unix::fs::PermissionsExt; + + #[test] + fn ca_creation_refuses_preexisting_cert_symlink() { + let dir = tempfile::tempdir().unwrap(); + let config = Utf8PathBuf::from_path_buf(dir.path().join("ca")).unwrap(); + fs::create_dir(&config).unwrap(); + let sentinel = dir.path().join("sentinel"); + fs::write(&sentinel, "unchanged").unwrap(); + std::os::unix::fs::symlink(&sentinel, config.join("ca-cert.pem")).unwrap(); + assert!(CertificateManager::load_or_generate_ca_with_dir(Some(&config)).is_err()); + assert_eq!(fs::read_to_string(&sentinel).unwrap(), "unchanged"); + } + + #[test] + fn ca_key_is_private_on_creation_and_cached_load() { + let dir = tempfile::tempdir().unwrap(); + let config = Utf8PathBuf::from_path_buf(dir.path().join("ca")).unwrap(); + CertificateManager::load_or_generate_ca_with_dir(Some(&config)).unwrap(); + let key = config.join("ca-key.pem"); + assert_eq!( + fs::metadata(&key).unwrap().permissions().mode() & 0o777, + 0o600 + ); + + fs::set_permissions(&key, fs::Permissions::from_mode(0o644)).unwrap(); + CertificateManager::load_or_generate_ca_with_dir(Some(&config)).unwrap(); + assert_eq!( + fs::metadata(&key).unwrap().permissions().mode() & 0o777, + 0o600 + ); + } +} diff --git a/tests/js_file_reload.rs b/tests/js_file_reload.rs index 8ce3aa1b..083e16d3 100644 --- a/tests/js_file_reload.rs +++ b/tests/js_file_reload.rs @@ -59,6 +59,20 @@ async fn test_js_file_reload() { let _ = fs::remove_file(&file_path); } +#[tokio::test] +async fn test_frozen_rule_ignores_child_file_edits() { + let file = NamedTempFile::new().unwrap(); + fs::write(file.path(), "false").unwrap(); + // Command mode reads the initial file but does not pass a reload path. + let engine = + V8JsRuleEngine::new_with_file(fs::read_to_string(file.path()).unwrap(), None).unwrap(); + fs::write(file.path(), "true").unwrap(); + let result = engine + .evaluate(Method::GET, "https://example.invalid/", "127.0.0.1") + .await; + assert!(matches!(result.action, httpjail::rules::Action::Deny)); +} + #[tokio::test(flavor = "multi_thread")] async fn test_js_file_reload_syntax_error() { // Create a temporary JS file and persist it diff --git a/tests/linux_integration.rs b/tests/linux_integration.rs index d6bc4402..7fa96d33 100644 --- a/tests/linux_integration.rs +++ b/tests/linux_integration.rs @@ -39,6 +39,231 @@ mod tests { // Linux-specific tests below use serial_test::serial; + #[test] + fn test_root_dry_run_enforces_restricted_policy() { + LinuxPlatform::require_privileges(); + assert_eq!( + httpjail::jail::get_canary_dir(), + std::path::PathBuf::from("/var/lib/httpjail/canaries") + ); + let mut inline = httpjail_cmd(); + let rejected = inline + .args([ + "--sh", + "/usr/bin/id -u", + "--test", + "https://example.invalid/", + ]) + .output() + .unwrap(); + assert!(!rejected.status.success()); + assert!(String::from_utf8_lossy(&rejected.stderr).contains("one trusted executable")); + + let mut trusted = httpjail_cmd(); + let output = trusted + .args(["--sh", "/usr/bin/id", "--test", "https://example.invalid/"]) + .output() + .unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(String::from_utf8_lossy(&output.stdout).contains("uid=65534")); + } + + #[test] + fn test_public_ca_is_readable_without_exposing_key() { + LinuxPlatform::require_privileges(); + let mut cmd = httpjail_cmd(); + let run = cmd + .args(["--js", "false", "--", "/usr/bin/true"]) + .output() + .unwrap(); + assert!( + run.status.success(), + "{}", + String::from_utf8_lossy(&run.stderr) + ); + + let cert = httpjail::tls::CertificateManager::get_ca_cert_path().unwrap(); + let uid = std::env::var("SUDO_UID").unwrap(); + let gid = std::env::var("SUDO_GID").unwrap(); + let read = |path: &std::path::Path| { + std::process::Command::new("/usr/bin/setpriv") + .args([ + "--no-new-privs", + &format!("--reuid={uid}"), + &format!("--regid={gid}"), + "--clear-groups", + "--", + "/usr/bin/cat", + ]) + .arg(path) + .output() + .unwrap() + }; + let cert_read = read(cert.as_std_path()); + assert!(cert_read.status.success()); + assert!(String::from_utf8_lossy(&cert_read.stdout).contains("BEGIN CERTIFICATE")); + let key = cert.with_file_name("ca-key.pem"); + assert!(!read(key.as_std_path()).status.success()); + } + + #[test] + fn test_root_request_log_cannot_append_protected_file() { + LinuxPlatform::require_privileges(); + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("protected"); + std::fs::write(&target, "unchanged").unwrap(); + let mut cmd = httpjail_cmd(); + let output = cmd + .args(["--js", "false", "--request-log"]) + .arg(&target) + .args(["--test", "https://example.invalid/\nINJECTED"]) + .output() + .unwrap(); + assert!(!output.status.success()); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "unchanged"); + } + + #[test] + #[ignore] + fn pid_namespace_probe_helper() { + assert_eq!(unsafe { libc::getpid() }, 1); + assert!( + std::fs::read_to_string("/proc/1/comm") + .unwrap() + .starts_with("linux_integrat") + ); + } + + #[test] + fn test_native_jail_has_private_pid_namespace() { + LinuxPlatform::require_privileges(); + let mut cmd = httpjail_cmd(); + cmd.args(["--js", "false", "--"]) + .arg(std::env::current_exe().unwrap()) + .args(["--ignored", "--exact", "tests::pid_namespace_probe_helper"]); + let output = cmd.output().unwrap(); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + output.status.success() && stdout.contains("1 passed"), + "PID namespace probe failed: {stdout} {}", + String::from_utf8_lossy(&output.stderr) + ); + } + + #[test] + #[ignore] + fn unix_socket_probe_helper() { + let path = std::env::var("HTTPJAIL_TEST_UNIX_SOCKET").unwrap(); + let error = std::os::unix::net::UnixStream::connect(path).unwrap_err(); + assert_eq!(error.raw_os_error(), Some(libc::EPERM)); + // Local stream socketpairs are needed by threaded DNS resolvers. + let (a, _b) = std::os::unix::net::UnixStream::pair().unwrap(); + drop(a); + let mut pair = [-1_i32; 2]; + let result = + unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_DGRAM, 0, pair.as_mut_ptr()) }; + assert_eq!(result, -1); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EPERM) + ); + // io_uring can create sockets without calling socket(2), so its + // submission syscalls must also be denied in this namespace. + let result = unsafe { libc::syscall(libc::SYS_io_uring_setup, 1, std::ptr::null::()) }; + assert_eq!(result, -1); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EPERM) + ); + let vsock = unsafe { libc::socket(libc::AF_VSOCK, libc::SOCK_STREAM, 0) }; + assert_eq!(vsock, -1); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EPERM) + ); + } + + #[test] + fn test_native_jail_blocks_host_unix_sockets() { + LinuxPlatform::require_privileges(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("host.sock"); + let _listener = std::os::unix::net::UnixListener::bind(&path).unwrap(); + let mut cmd = httpjail_cmd(); + cmd.args(["--js", "false", "--"]) + .arg(std::env::current_exe().unwrap()) + .args(["--ignored", "--exact", "tests::unix_socket_probe_helper"]) + .env("HTTPJAIL_TEST_UNIX_SOCKET", &path); + let output = cmd.output().unwrap(); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + output.status.success() && stdout.contains("1 passed"), + "Unix socket probe failed: {stdout} {}", + String::from_utf8_lossy(&output.stderr) + ); + } + + #[test] + fn test_strong_jail_rejects_user_writable_policy_program() { + LinuxPlatform::require_privileges(); + let dir = tempfile::tempdir().unwrap(); + let script = dir.path().join("policy.sh"); + std::fs::write(&script, "#!/bin/sh\nexit 0\n").unwrap(); + let mut cmd = httpjail_cmd(); + cmd.arg("--sh").arg(&script).args(["--", "true"]); + let output = cmd.output().unwrap(); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("root-owned, non-writable"), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + } + + #[test] + fn test_direct_root_strong_jail_refused() { + LinuxPlatform::require_privileges(); + let output = std::process::Command::new(env!("CARGO_BIN_EXE_httpjail")) + .env_remove("SUDO_UID") + .env_remove("SUDO_GID") + .args(["--js", "false", "--", "true"]) + .output() + .unwrap(); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("requires sudo from a non-root user"), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + } + + #[test] + fn test_strong_jail_clears_groups_and_blocks_setuid() { + LinuxPlatform::require_privileges(); + let mut groups = httpjail_cmd(); + groups.args(["--js", "true", "--", "id", "-G"]); + let output = groups.output().unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let expected = std::env::var("SUDO_GID").expect("run as sudo from non-root user"); + assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), expected); + + let mut sudo = httpjail_cmd(); + sudo.args(["--js", "true", "--", "sudo", "-n", "id", "-u"]); + let output = sudo.output().unwrap(); + assert!( + !output.status.success(), + "jailed process regained root via sudo" + ); + } + /// Linux-specific test: verify namespace cleanup #[test] #[serial] @@ -316,6 +541,225 @@ mod tests { ); } + #[test] + #[ignore] + fn ipv6_host_probe_helper() { + let Some(host) = std::env::var("HTTPJAIL_TEST_HOST_IPV6").ok() else { + return; + }; + let iface = + std::ffi::CString::new(std::env::var("HTTPJAIL_TEST_GUEST_IFACE").unwrap()).unwrap(); + let index = unsafe { libc::if_nametoindex(iface.as_ptr()) }; + assert_ne!(index, 0); + let ip = host.parse::().unwrap(); + let port = std::env::var("HTTPJAIL_TEST_HOST_PORT") + .unwrap() + .parse::() + .unwrap(); + let target = std::net::SocketAddr::V6(std::net::SocketAddrV6::new(ip, port, 0, index)); + let connection = + std::net::TcpStream::connect_timeout(&target, std::time::Duration::from_millis(300)); + let expected = std::env::var_os("HTTPJAIL_TEST_ALLOW_IPV6").is_some(); + assert_eq!( + connection.is_ok(), + expected, + "Unexpected IPv6 host-veth reachability: {connection:?}" + ); + } + + #[test] + fn test_namespace_blocks_host_ipv6_link_local() { + LinuxPlatform::require_privileges(); + let listener = std::net::TcpListener::bind("[::]:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + let mut jail = std::process::Command::new(env!("CARGO_BIN_EXE_httpjail")); + jail.env("HTTPJAIL_SKIP_KEYCHAIN_INSTALL", "1").args([ + "--timeout", + "15", + "--js", + "false", + "--", + "sleep", + "5", + ]); + let mut child = jail.spawn().unwrap(); + let canary_dir = httpjail::jail::get_canary_dir(); + let id = (0..60) + .find_map(|_| { + let match_id = std::fs::read_dir(&canary_dir).ok().and_then(|entries| { + entries.flatten().find_map(|entry| { + (std::fs::read_to_string(entry.path()).ok()?.trim() + == child.id().to_string()) + .then(|| entry.file_name().to_string_lossy().into_owned()) + }) + }); + if match_id.is_none() { + std::thread::sleep(std::time::Duration::from_millis(50)); + } + match_id + }) + .expect("jail canary did not appear"); + let host_iface = format!("vh_{id}"); + let namespace = format!("httpjail_{id}"); + let address = (0..60) + .find_map(|_| { + let output = std::process::Command::new("ip") + .args([ + "-6", + "-o", + "addr", + "show", + "dev", + &host_iface, + "scope", + "link", + ]) + .output() + .ok()?; + let ip = String::from_utf8_lossy(&output.stdout) + .split_whitespace() + .find_map(|part| { + part.strip_suffix("/64") + .filter(|ip| ip.starts_with("fe80::")) + }) + .map(str::to_owned); + let ready = std::process::Command::new("ip") + .args([ + "netns", + "exec", + &namespace, + "nft", + "list", + "table", + "ip6", + "httpjail6", + ]) + .output() + .is_ok_and(|output| output.status.success()); + if ip.is_none() || !ready { + std::thread::sleep(std::time::Duration::from_millis(50)); + } + ip.filter(|_| ready) + }) + .expect("IPv6 veth and output filter did not become ready"); + // Some hosts disable automatic guest veth IPv6 addresses. Give this + // disposable namespace its own link-local source for the control probe. + let guest_iface = format!("vn_{id}"); + let enable = std::process::Command::new("/usr/sbin/ip") + .args(["netns", "exec", &namespace, "/usr/sbin/sysctl", "-w"]) + .arg(format!("net.ipv6.conf.{guest_iface}.disable_ipv6=0")) + .output() + .unwrap(); + assert!( + enable.status.success(), + "{}", + String::from_utf8_lossy(&enable.stderr) + ); + let add = std::process::Command::new("/usr/sbin/ip") + .args([ + "netns", + "exec", + &namespace, + "/usr/sbin/ip", + "-6", + "addr", + "add", + "fe80::2/64", + "dev", + &guest_iface, + "nodad", + ]) + .output() + .unwrap(); + assert!( + add.status.success(), + "{}", + String::from_utf8_lossy(&add.stderr) + ); + let probe = std::process::Command::new("ip") + .args(["netns", "exec", &format!("httpjail_{id}")]) + .arg(std::env::current_exe().unwrap()) + .args(["--ignored", "--exact", "tests::ipv6_host_probe_helper"]) + .env("HTTPJAIL_TEST_HOST_IPV6", &address) + .env("HTTPJAIL_TEST_GUEST_IFACE", format!("vn_{id}")) + .env("HTTPJAIL_TEST_HOST_PORT", port.to_string()) + .output() + .unwrap(); + // Control: only inside this disposable jail namespace, remove the IPv6 + // output table and confirm the same host service becomes reachable. + let removal = std::process::Command::new("ip") + .args([ + "netns", + "exec", + &namespace, + "nft", + "delete", + "table", + "ip6", + "httpjail6", + ]) + .output() + .unwrap(); + assert!( + removal.status.success(), + "{}", + String::from_utf8_lossy(&removal.stderr) + ); + let control = std::process::Command::new("ip") + .args(["netns", "exec", &namespace]) + .arg(std::env::current_exe().unwrap()) + .args(["--ignored", "--exact", "tests::ipv6_host_probe_helper"]) + .env("HTTPJAIL_TEST_HOST_IPV6", &address) + .env("HTTPJAIL_TEST_GUEST_IFACE", format!("vn_{id}")) + .env("HTTPJAIL_TEST_HOST_PORT", port.to_string()) + .env("HTTPJAIL_TEST_ALLOW_IPV6", "1") + .output() + .unwrap(); + let _ = child.wait(); + let stdout = String::from_utf8_lossy(&probe.stdout); + assert!( + probe.status.success() && stdout.contains("1 passed"), + "IPv6 probe did not run or failed: {stdout} {}", + String::from_utf8_lossy(&probe.stderr) + ); + if !control.status.success() { + eprintln!( + "Host IPv6 networking also blocks the control probe; only the jail table and blocked probe were verified: {}", + String::from_utf8_lossy(&control.stderr) + ); + return; + } + assert!(String::from_utf8_lossy(&control.stdout).contains("1 passed")); + } + + /// Docker bridge traffic to the host gateway must not bypass HTTP rules. + #[test] + fn test_docker_cannot_reach_host_gateway_service() { + LinuxPlatform::require_privileges(); + let listener = std::net::TcpListener::bind(("0.0.0.0", 0)).unwrap(); + listener.set_nonblocking(true).unwrap(); + let port = listener.local_addr().unwrap().port(); + let mut cmd = httpjail_cmd(); + cmd.args(["--js", "false", "--docker-run", "--", "--rm", "alpine:latest", "sh", "-c"]) + .arg(format!( + r#"gw=$(ip route | awk '$1=="default" {{print $3; exit}}'); wget -q -T 2 -O - http://$gw:{port}/ >/dev/null 2>&1 && echo REACHED || echo BLOCKED"# + )); + let output = cmd.output().unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!( + String::from_utf8_lossy(&output.stdout).contains("BLOCKED"), + "Docker container reached a host gateway service" + ); + assert!( + listener.accept().is_err(), + "Host listener received a Docker request" + ); + } + /// Test Docker container execution with --docker-run #[test] fn test_docker_run_basic() { diff --git a/tests/security_regressions.rs b/tests/security_regressions.rs new file mode 100644 index 00000000..fd2d7ffb --- /dev/null +++ b/tests/security_regressions.rs @@ -0,0 +1,77 @@ +//! Regressions for security-sensitive CLI behavior shared by macOS and Linux. + +use std::process::Command; +use std::time::Duration; + +#[test] +#[ignore] +fn timeout_marker_child() { + let Some(marker) = std::env::var_os("HTTPJAIL_TIMEOUT_MARKER") else { + return; + }; + std::thread::sleep(Duration::from_secs(2)); + std::fs::write(marker, b"survived").unwrap(); +} + +#[test] +fn timeout_terminates_child_before_returning() { + let temp = tempfile::tempdir().unwrap(); + let marker = temp.path().join("survived"); + let child = std::env::current_exe().unwrap(); + let status = Command::new(env!("CARGO_BIN_EXE_httpjail")) + .args(["--weak", "--js", "true", "--timeout", "1", "--"]) + .arg(child) + .args(["--ignored", "--exact", "timeout_marker_child"]) + .env("HTTPJAIL_TIMEOUT_MARKER", &marker) + .status() + .unwrap(); + assert_eq!(status.code(), Some(124)); + std::thread::sleep(Duration::from_millis(1200)); + assert!(!marker.exists(), "timed-out child ran after jail cleanup"); +} + +#[cfg(target_os = "macos")] +#[test] +fn stalled_processor_stdin_times_out() { + let url = format!("https://example.invalid/{}", "a".repeat(120_000)); + let start = std::time::Instant::now(); + let output = Command::new(env!("CARGO_BIN_EXE_httpjail")) + .args(["--proc", "/usr/bin/caffeinate", "--test"]) + .arg(url) + .output() + .unwrap(); + assert_eq!(output.status.code(), Some(1)); + assert!(start.elapsed() < Duration::from_secs(8)); + assert!( + String::from_utf8_lossy(&output.stderr).contains("Program stdin timeout"), + "{}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[cfg(unix)] +#[test] +fn new_request_log_is_private() { + use std::os::unix::fs::PermissionsExt; + use std::os::unix::process::CommandExt; + + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("requests.log"); + let mut command = Command::new(env!("CARGO_BIN_EXE_httpjail")); + command + .args(["--js", "false", "--request-log"]) + .arg(&path) + .args(["--test", "https://example.invalid/?token=dummy"]); + // Set the child umask, not the process-wide test runner umask. + unsafe { + command.pre_exec(|| { + libc::umask(0o022); + Ok(()) + }); + } + assert_eq!(command.status().unwrap().code(), Some(1)); + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600 + ); +} diff --git a/tests/system_integration.rs b/tests/system_integration.rs index a8b921af..6caf9c51 100644 --- a/tests/system_integration.rs +++ b/tests/system_integration.rs @@ -209,7 +209,27 @@ pub fn test_jail_method_specific_rules() { pub fn test_jail_request_log() { P::require_privileges(); - let log_file = tempfile::NamedTempFile::new().expect("Failed to create temp file"); + let log_dir = tempfile::tempdir().expect("Failed to create log directory"); + #[cfg(target_os = "linux")] + if unsafe { libc::geteuid() == 0 } { + use std::os::unix::ffi::OsStrExt; + let uid: u32 = std::env::var("SUDO_UID").unwrap().parse().unwrap(); + let gid: u32 = std::env::var("SUDO_GID").unwrap().parse().unwrap(); + let path = std::ffi::CString::new(log_dir.path().as_os_str().as_bytes()).unwrap(); + assert_eq!(unsafe { libc::chown(path.as_ptr(), uid, gid) }, 0); + } + let log_file = + tempfile::NamedTempFile::new_in(log_dir.path()).expect("Failed to create temp file"); + #[cfg(target_os = "linux")] + if unsafe { libc::geteuid() == 0 } { + use std::os::unix::io::AsRawFd; + let uid: u32 = std::env::var("SUDO_UID").unwrap().parse().unwrap(); + let gid: u32 = std::env::var("SUDO_GID").unwrap().parse().unwrap(); + assert_eq!( + unsafe { libc::fchown(log_file.as_file().as_raw_fd(), uid, gid) }, + 0 + ); + } let log_path = log_file.path().to_str().unwrap().to_string(); let mut cmd = httpjail_cmd(); From b8b4f493c33794dc0237c3af7e271080a10a1568 Mon Sep 17 00:00:00 2001 From: Ammar Date: Thu, 24 Sep 2026 10:49:02 -0500 Subject: [PATCH 2/3] fix: recover CA credentials and simplify audit hardening --- README.md | 4 +- src/jail/linux/docker.rs | 149 +--------------- src/main.rs | 22 +-- src/rules/proc.rs | 10 +- src/tls.rs | 320 ++++++++++++++++++---------------- tests/security_regressions.rs | 8 +- 6 files changed, 199 insertions(+), 314 deletions(-) diff --git a/README.md b/README.md index 3317609e..291463a2 100644 --- a/README.md +++ b/README.md @@ -74,10 +74,10 @@ sudo httpjail --js "r.host === 'api.github.com'" --docker-run -- --rm alpine:lat - Linux strong mode requires trusted system helpers at `/usr/sbin/ip`, `/usr/sbin/nft`, and `/usr/bin/setpriv` (Docker mode also requires `/usr/bin/docker`); privileged setup and cleanup never search a payload-controlled `PATH`. - Native Linux strong mode gives the payload a private PID namespace and `/proc` mount so it cannot import file descriptors from same-user host processes; `/usr/bin/unshare` is required. - Newly created CA private keys and request logs use owner-only permissions on Unix. When Linux httpjail runs as root under sudo, request logs must be in a directory owned by the invoking user; new logs are assigned to that user, and existing logs must already belong to them (symlinks and protected host files are rejected). Existing log permissions remain unchanged; logs include full URLs, including query strings, so protect them accordingly. -- On Linux, strong-mode canaries and Docker public-certificate snapshots live under root-owned `/var/lib/httpjail` rather than sudo-preserved `HOME` or `TMPDIR`; its ancestors must not be writable by other users. Privileged Linux CA files likewise live under `/var/lib/httpjail/ca` instead of a sudo-preserved home directory; upgrading from a user-home CA generates a new root-only CA, so update client trust and remove trust in the old certificate. +- On Linux, strong-mode canaries and Docker public-certificate snapshots live under root-owned `/var/lib/httpjail` rather than sudo-preserved `HOME` or `TMPDIR`; its ancestors must not be writable by other users. Privileged Linux CA files likewise live under `/var/lib/httpjail/ca` instead of a sudo-preserved home directory; upgrading from a user-home CA generates a new root-only CA, so update client trust and remove trust in the old certificate. Interrupted CA writes are repaired on startup: a valid signing key is preserved, but a missing or corrupt key generates a new CA and requires renewing client trust. - HTTP/1 request headers have a 10-second read deadline; request bodies and upgraded connections do not inherit that deadline. - Docker mode requires the local daemon at `/var/run/docker.sock` (remote Docker contexts are ignored), mounts only that public-certificate snapshot (never the signing key), drops `NET_RAW`, and accepts only restricted `docker run` flags before the image (environment, user, working directory, resource limits, read-only/init, and `--rm`). Network, DNS, privileged, volume, port-publishing, and other unrecognized Docker flags are rejected. Arguments after the image are passed to the container unchanged. -- `sudo httpjail --cleanup` also removes legacy Docker nftables routing tables only when their Docker network, current/legacy canaries, and namespace config are absent; it leaves active-jail guards in place. Canaryless namespaces or legacy networks require manual administrator inspection rather than unsafe automatic deletion. +- Legacy canaryless namespaces, networks, and leftover firewall tables require manual administrator inspection rather than unsafe automatic deletion. ## Documentation diff --git a/src/jail/linux/docker.rs b/src/jail/linux/docker.rs index 20e137aa..49ab9f05 100644 --- a/src/jail/linux/docker.rs +++ b/src/jail/linux/docker.rs @@ -7,76 +7,6 @@ use anyhow::{Context, Result}; use std::process::{Command, ExitStatus}; use tracing::{debug, info, warn}; -fn stale_docker_table_id<'a>( - line: &'a str, - networks: &std::collections::HashSet, - root_canaries: &std::path::Path, - legacy_canaries: Option<&std::path::Path>, - namespace_configs: &std::path::Path, -) -> Option<&'a str> { - let mut parts = line.split_whitespace(); - let (Some("table"), Some(family), Some(name)) = (parts.next(), parts.next(), parts.next()) - else { - return None; - }; - if family != "ip" && family != "inet" { - return None; - } - let id = name.strip_prefix("httpjail_docker_")?; - (crate::jail::valid_jail_id(id) - && !networks.contains(&DockerNetwork::network_name_from_jail_id(id)) - && !root_canaries.join(id).exists() - && !legacy_canaries.is_some_and(|path| path.join(id).exists()) - && !namespace_configs.join(format!("httpjail_{id}")).exists()) - .then_some(id) -} - -/// Reclaim routing tables left behind by older runs whose Docker network was -/// removed before the process could drop its nftables resources. This is an -/// explicit maintenance operation, not part of the latency-sensitive startup. -pub fn cleanup_orphaned_docker_tables() -> Result<()> { - use std::collections::HashSet; - if !std::path::Path::new("/usr/bin/docker").exists() { - return Ok(()); - } - let output = local_docker_command() - .args(["network", "ls", "--format", "{{.Name}}"]) - .output()?; - if !output.status.success() { - debug!("Docker daemon unavailable; skipping stale routing table cleanup"); - return Ok(()); - } - let networks: HashSet = String::from_utf8_lossy(&output.stdout) - .lines() - .map(ToOwned::to_owned) - .collect(); - let tables = Command::new("/usr/sbin/nft") - .args(["list", "tables"]) - .output()?; - anyhow::ensure!(tables.status.success(), "Failed to list nftables tables"); - let root_canaries = crate::jail::get_canary_dir(); - crate::jail::ensure_trusted_root_dir(&root_canaries)?; - // Previous versions used sudo-preserved HOME. A legacy live process may - // still rely on its old canary; err on the side of retaining its guard. - let legacy_canaries = dirs::data_dir().map(|path| path.join("httpjail/canaries")); - let mut ids = HashSet::new(); - for line in String::from_utf8_lossy(&tables.stdout).lines() { - if let Some(id) = stale_docker_table_id( - line, - &networks, - &root_canaries, - legacy_canaries.as_deref(), - std::path::Path::new("/etc/netns"), - ) { - ids.insert(id.to_string()); - } - } - for id in ids { - DockerRoutingTable::for_existing(&id).cleanup()?; - } - Ok(()) -} - /// Docker network resource that gets cleaned up on drop struct DockerNetwork { network_name: String, @@ -505,32 +435,18 @@ impl DockerLinux { // 2. DNAT HTTP/HTTPS traffic to the proxy let table_name = DockerRoutingTable::table_name_from_jail_id(&self.config.jail_id); - // Create nftables rules + // HTTP(S) is DNATed to a local host listener (INPUT). Forwarded + // bridge traffic is denied by the single inet guard below. let nft_rules = format!( - "table ip {} {{ + r#"table ip {table_name} {{ chain prerouting {{ type nat hook prerouting priority -100; - iifname \"{}\" tcp dport 80 dnat to {}:{}; - iifname \"{}\" tcp dport 443 dnat to {}:{}; - }} - - chain forward {{ - type filter hook forward priority 0; - iifname \"{}\" oifname \"vh_{}\" accept; - iifname \"vh_{}\" oifname \"{}\" ct state established,related accept; + iifname "{bridge_name}" tcp dport 80 dnat to {host_ip_str}:{http_port}; + iifname "{bridge_name}" tcp dport 443 dnat to {host_ip_str}:{https_port}; }} - }}", - table_name, - bridge_name, - host_ip_str, - self.config.http_proxy_port, - bridge_name, - host_ip_str, - self.config.https_proxy_port, - bridge_name, - self.config.jail_id, - self.config.jail_id, - bridge_name + }}"#, + http_port = self.config.http_proxy_port, + https_port = self.config.https_proxy_port, ); // Docker bridge traffic to its host gateway takes INPUT, not FORWARD. @@ -654,9 +570,7 @@ impl Jail for DockerLinux { // clone, so do not rely on Drop to clean up daemon-owned resources. // Remove the network before its guard; on failure retain both and the // canary so a later orphan cleanup can retry without exposing traffic. - DockerNetwork::for_existing(&self.config.jail_id).cleanup()?; - DockerRoutingTable::for_existing(&self.config.jail_id).cleanup()?; - self.inner_jail.cleanup() + Self::cleanup_orphaned(&self.config.jail_id) } fn jail_id(&self) -> &str { @@ -709,51 +623,6 @@ impl Clone for DockerLinux { mod tests { use super::DockerLinux; - #[test] - fn stale_tables_skip_live_networks_and_canaries() { - use std::collections::HashSet; - let dir = tempfile::tempdir().unwrap(); - let root = dir.path().join("root"); - let legacy = dir.path().join("legacy"); - let namespace_configs = dir.path().join("netns"); - std::fs::create_dir_all(&namespace_configs).unwrap(); - std::fs::create_dir_all(&root).unwrap(); - std::fs::create_dir_all(&legacy).unwrap(); - let line = "table ip httpjail_docker_abcd1234"; - let mut networks = HashSet::new(); - assert_eq!( - super::stale_docker_table_id(line, &networks, &root, Some(&legacy), &namespace_configs), - Some("abcd1234") - ); - networks.insert("httpjail_abcd1234".to_string()); - assert_eq!( - super::stale_docker_table_id(line, &networks, &root, Some(&legacy), &namespace_configs), - None - ); - networks.clear(); - std::fs::write(legacy.join("abcd1234"), "1").unwrap(); - assert_eq!( - super::stale_docker_table_id(line, &networks, &root, Some(&legacy), &namespace_configs), - None - ); - std::fs::remove_file(legacy.join("abcd1234")).unwrap(); - std::fs::create_dir(namespace_configs.join("httpjail_abcd1234")).unwrap(); - assert_eq!( - super::stale_docker_table_id(line, &networks, &root, Some(&legacy), &namespace_configs), - None - ); - assert_eq!( - super::stale_docker_table_id( - "table ip httpjail_docker_bad!", - &networks, - &root, - None, - &namespace_configs - ), - None - ); - } - #[test] fn docker_mounts_only_public_ca_file() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/main.rs b/src/main.rs index c31e0115..eb784ff8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -372,11 +372,6 @@ fn cleanup_orphans() -> Result<()> { // just from a missing canary. Orphaned canaryless namespaces need manual // administrator inspection instead of automatic destructive cleanup. - #[cfg(target_os = "linux")] - if unsafe { libc::geteuid() == 0 } { - httpjail::jail::linux::docker::cleanup_orphaned_docker_tables()?; - } - if cleaned_jails.is_empty() { debug!("No orphaned jails found"); } else { @@ -422,19 +417,10 @@ async fn main() -> Result<()> { } if *install { - // First ensure CA exists - let config_dir = dirs::config_dir() - .context("Could not find user config directory")? - .join("httpjail"); - let ca_cert_path = config_dir.join("ca-cert.pem"); - - if !ca_cert_path.exists() { - // Generate CA first - info!("Generating CA certificate..."); - let _ = httpjail::tls::CertificateManager::new()?; - } - - keychain_manager.install_ca(&ca_cert_path)?; + // Repair incomplete credentials before trusting the public certificate. + let _ = httpjail::tls::CertificateManager::new()?; + let ca_cert_path = httpjail::tls::CertificateManager::get_ca_cert_path()?; + keychain_manager.install_ca(ca_cert_path.as_std_path())?; println!("✓ httpjail CA certificate installed successfully"); return Ok(()); } diff --git a/src/rules/proc.rs b/src/rules/proc.rs index f777757b..b327c067 100644 --- a/src/rules/proc.rs +++ b/src/rules/proc.rs @@ -9,7 +9,7 @@ use std::time::Duration; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::process::{Child, ChildStdin, ChildStdout, Command}; use tokio::sync::Mutex; -use tracing::{debug, error, warn}; +use tracing::{debug, error}; const MAX_RESPONSE_BYTES: usize = 64 * 1024; @@ -88,7 +88,7 @@ impl ProcRuleEngine { if let Some(mut process_state) = process_guard.take() { match process_state.child.try_wait() { Ok(Some(status)) => { - warn!( + debug!( "Program process exited with status: {:?}, restarting", status ); @@ -194,7 +194,7 @@ impl ProcRuleEngine { return Err("Failed to write to program".to_string()); } Err(_) => { - warn!("Program stdin timeout after {:?}", timeout); + debug!("Program stdin timeout after {:?}", timeout); if let Some(state) = process_guard.take() { Self::kill_process(state); } @@ -209,7 +209,7 @@ impl ProcRuleEngine { match tokio::time::timeout(timeout, read).await { Ok(Ok(0)) => { // EOF - process exited - warn!("Program closed stdout unexpectedly"); + debug!("Program closed stdout unexpectedly"); if let Some(state) = process_guard.take() { Self::kill_process(state); } @@ -243,7 +243,7 @@ impl ProcRuleEngine { Err(format!("Error reading from program: {}", e)) } Err(_) => { - warn!("Program response timeout after {:?}", timeout); + debug!("Program response timeout after {:?}", timeout); if let Some(state) = process_guard.take() { Self::kill_process(state); } diff --git a/src/tls.rs b/src/tls.rs index d20b267c..5433696e 100644 --- a/src/tls.rs +++ b/src/tls.rs @@ -1,8 +1,8 @@ use anyhow::{Context, Result}; -use camino::Utf8PathBuf; +use camino::{Utf8Path, Utf8PathBuf}; use lru::LruCache; use rcgen::{Certificate, CertificateParams, DistinguishedName, DnType, KeyPair, SanType}; -use rustls::pki_types::{CertificateDer, PrivateKeyDer}; +use rustls::pki_types::{CertificateDer, PrivateKeyDer, pem::PemObject}; use std::fs::{self, OpenOptions}; use std::io::{Read, Write}; use std::num::NonZeroUsize; @@ -38,6 +38,89 @@ fn default_ca_dir() -> Result { .context("Config directory path is not valid UTF-8") } +// Missing/corrupt content is recoverable; unsafe paths and I/O failures are not. +fn check_ca_file(file: &fs::File) -> Result<()> { + let metadata = file.metadata()?; + anyhow::ensure!(metadata.is_file(), "CA file must be regular"); + #[cfg(target_os = "linux")] + if unsafe { libc::geteuid() == 0 } { + use std::os::unix::fs::MetadataExt; + anyhow::ensure!(metadata.uid() == 0, "Privileged CA file must be root-owned"); + } + Ok(()) +} + +#[cfg(unix)] +fn lock_ca_dir(dir: &Utf8Path) -> Result { + use std::os::fd::AsRawFd; + use std::os::unix::fs::OpenOptionsExt; + let lock = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .mode(0o600) + .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) + .open(dir.join(".ca.lock"))?; + check_ca_file(&lock)?; + anyhow::ensure!( + unsafe { libc::flock(lock.as_raw_fd(), libc::LOCK_EX) } == 0, + "Failed to lock CA directory" + ); + Ok(lock) +} + +fn read_ca_file(path: &Utf8Path, mode: u32) -> Result>> { + let mut options = OpenOptions::new(); + options.read(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK); + } + let mut file = match options.open(path) { + Ok(file) => file, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(e) => return Err(e).with_context(|| format!("Failed to open CA file: {path}")), + }; + check_ca_file(&file)?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + file.set_permissions(fs::Permissions::from_mode(mode))?; + } + let mut bytes = Vec::new(); + file.read_to_end(&mut bytes)?; + Ok(Some(bytes)) +} + +fn write_ca_file(path: &Utf8Path, bytes: &[u8], mode: u32) -> Result<()> { + // Same-directory rename never follows the destination symlink and cannot + // expose partially written bytes. NamedTempFile starts private (0600). + let mut file = tempfile::NamedTempFile::new_in(path.parent().context("Missing CA directory")?)?; + file.write_all(bytes)?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + file.as_file() + .set_permissions(fs::Permissions::from_mode(mode))?; + } + file.as_file().sync_all()?; + file.persist(path) + .with_context(|| format!("Failed to publish CA file: {path}"))?; + Ok(()) +} + +// Detect incomplete/mismatched publications, not arbitrary modifications to +// the identity or constraints of a certificate signed with the same key. +fn ca_cert_matches_key(pem: &[u8], key: &KeyPair) -> bool { + let Ok(cert) = CertificateDer::from_pem_slice(pem) else { + return false; + }; + rustls::server::ParsedCertificate::try_from(&cert) + .is_ok_and(|cert| cert.subject_public_key_info().as_ref() == key.public_key_der()) +} + const CERT_CACHE_SIZE: usize = 1024; /// Manages TLS certificates for HTTPS interception @@ -71,173 +154,53 @@ impl CertificateManager { // Create directory if it doesn't exist fs::create_dir_all(&config_dir).context("Failed to create config directory")?; - #[cfg(target_os = "linux")] - let _ca_lock = if unsafe { libc::geteuid() == 0 } { - use std::os::fd::AsRawFd; - use std::os::unix::fs::OpenOptionsExt; - // Concurrent strong jails must not see a half-written cert/key pair. - let lock = OpenOptions::new() - .read(true) - .write(true) - .create(true) - .mode(0o600) - .custom_flags(libc::O_NOFOLLOW) - .open(config_dir.join(".ca.lock"))?; - use std::os::unix::fs::MetadataExt; - let metadata = lock.metadata()?; - anyhow::ensure!( - metadata.is_file() && metadata.uid() == 0, - "CA lock must be root-owned and regular" - ); - anyhow::ensure!( - unsafe { libc::flock(lock.as_raw_fd(), libc::LOCK_EX) } == 0, - "Failed to lock CA directory" - ); - Some(lock) - } else { - None - }; - + // Serialize initialization/recovery on both platforms. Each file is + // published atomically; a crash between publications is repaired below. + #[cfg(unix)] + let _ca_lock = lock_ca_dir(&config_dir)?; let ca_cert_path = config_dir.join("ca-cert.pem"); let ca_key_path = config_dir.join("ca-key.pem"); + let cert_pem = read_ca_file(&ca_cert_path, 0o644)?; + let key_pem = read_ca_file(&ca_key_path, 0o600)?; + let cached_key = key_pem + .as_deref() + .and_then(|pem| std::str::from_utf8(pem).ok()) + .and_then(|pem| KeyPair::from_pem(pem).ok()); + let new_key = cached_key.is_none(); + let ca_key_pair = match cached_key { + Some(key) => key, + None => KeyPair::generate().context("Failed to generate CA key")?, + }; - // Try to load existing CA - if ca_cert_path.exists() && ca_key_path.exists() { - debug!("Loading cached CA certificate from {:?}", ca_cert_path); - - #[cfg(target_os = "linux")] - let _cert_pem = if unsafe { libc::geteuid() == 0 } { - use std::os::unix::fs::{MetadataExt, OpenOptionsExt, PermissionsExt}; - let mut cert = OpenOptions::new() - .read(true) - .custom_flags(libc::O_NOFOLLOW) - .open(&ca_cert_path) - .context("Failed to open public CA certificate")?; - let metadata = cert.metadata()?; - anyhow::ensure!( - metadata.is_file() && metadata.uid() == 0, - "CA certificate must be root-owned and regular" - ); - cert.set_permissions(fs::Permissions::from_mode(0o644))?; - let mut pem = String::new(); - cert.read_to_string(&mut pem)?; - pem - } else { - fs::read_to_string(&ca_cert_path).context("Failed to read CA certificate")? - }; - #[cfg(not(target_os = "linux"))] - let _cert_pem = - fs::read_to_string(&ca_cert_path).context("Failed to read CA certificate")?; - #[cfg(unix)] - let key_pem = { - use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; - let mut key = OpenOptions::new() - .read(true) - .custom_flags(libc::O_NOFOLLOW) - .open(&ca_key_path) - .context("Failed to open CA key")?; - let metadata = key.metadata()?; - anyhow::ensure!(metadata.is_file(), "CA key must be a regular file"); - #[cfg(target_os = "linux")] - if unsafe { libc::geteuid() == 0 } { - use std::os::unix::fs::MetadataExt; - anyhow::ensure!(metadata.uid() == 0, "Privileged CA key must be root-owned"); - } - if metadata.permissions().mode() & 0o077 != 0 { - key.set_permissions(fs::Permissions::from_mode(0o600)) - .context("Failed to restrict existing CA key permissions")?; - } - let mut pem = String::new(); - key.read_to_string(&mut pem) - .context("Failed to read CA key")?; - pem - }; - #[cfg(not(unix))] - let key_pem = fs::read_to_string(&ca_key_path).context("Failed to read CA key")?; - - // Parse the PEM files - let key_pair = KeyPair::from_pem(&key_pem).context("Failed to parse CA key")?; - - // Recreate the CA certificate from the stored files - // Since rcgen doesn't support loading existing certificates, - // we'll need to regenerate if this fails - // For now, just return the key pair and cert as stored - // This is a limitation of rcgen - in production you'd use a different approach - - // Generate new params but use existing key - let mut ca_params = CertificateParams::default(); - ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); - - let mut dn = DistinguishedName::new(); - dn.push(DnType::CountryName, "US"); - dn.push(DnType::OrganizationName, "httpjail"); - dn.push(DnType::CommonName, "httpjail CA"); - ca_params.distinguished_name = dn; - - ca_params.key_usages = vec![ - rcgen::KeyUsagePurpose::DigitalSignature, - rcgen::KeyUsagePurpose::KeyCertSign, - rcgen::KeyUsagePurpose::CrlSign, - ]; - - let ca_cert = ca_params - .self_signed(&key_pair) - .context("Failed to recreate CA certificate")?; - - info!("Loaded cached CA certificate from {}", ca_cert_path); - return Ok((ca_cert, key_pair)); - } - - // Generate new CA certificate - info!("Generating new CA certificate"); let mut ca_params = CertificateParams::default(); ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); - let mut dn = DistinguishedName::new(); dn.push(DnType::CountryName, "US"); dn.push(DnType::OrganizationName, "httpjail"); dn.push(DnType::CommonName, "httpjail CA"); ca_params.distinguished_name = dn; - ca_params.key_usages = vec![ rcgen::KeyUsagePurpose::DigitalSignature, rcgen::KeyUsagePurpose::KeyCertSign, rcgen::KeyUsagePurpose::CrlSign, ]; - - let ca_key_pair = KeyPair::generate()?; let ca_cert = ca_params .self_signed(&ca_key_pair) .context("Failed to generate CA certificate")?; - // Never follow a preexisting cert symlink into an unrelated root-owned file. - let mut cert_file = OpenOptions::new() - .write(true) - .create_new(true) - .open(&ca_cert_path) - .context("Failed to create exclusive CA certificate")?; - cert_file - .write_all(ca_cert.pem().as_bytes()) - .context("Failed to write CA certificate")?; - #[cfg(target_os = "linux")] - if unsafe { libc::geteuid() == 0 } { - use std::os::unix::fs::PermissionsExt; - cert_file.set_permissions(fs::Permissions::from_mode(0o644))?; + let cert_matches = cert_pem + .as_deref() + .is_some_and(|pem| ca_cert_matches_key(pem, &ca_key_pair)); + if new_key { + write_ca_file(&ca_key_path, ca_key_pair.serialize_pem().as_bytes(), 0o600)?; } - // Restrict permissions at creation, before any private-key bytes are written. - let mut key_file = OpenOptions::new(); - key_file.write(true).create_new(true); - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - key_file.mode(0o600); + if cert_matches { + // Keep a healthy certificate byte-for-byte stable (including trust + // installed in the macOS keychain) rather than republishing it. + return Ok((ca_cert, ca_key_pair)); } - key_file - .open(&ca_key_path) - .and_then(|mut file| file.write_all(ca_key_pair.serialize_pem().as_bytes())) - .context("Failed to write CA key")?; - - info!("Saved new CA certificate to {}", ca_cert_path); + write_ca_file(&ca_cert_path, ca_cert.pem().as_bytes(), 0o644)?; + debug!("Created or repaired CA certificate at {}", ca_cert_path); // On macOS, install the CA to the keychain (unless disabled for testing) #[cfg(target_os = "macos")] @@ -500,6 +463,67 @@ mod tests { assert_eq!(fs::read_to_string(&sentinel).unwrap(), "unchanged"); } + #[test] + fn ca_recovers_missing_or_invalid_key() { + let dir = tempfile::tempdir().unwrap(); + let config = Utf8PathBuf::from_path_buf(dir.path().to_path_buf()).unwrap(); + let cert = config.join("ca-cert.pem"); + let key = config.join("ca-key.pem"); + CertificateManager::load_or_generate_ca_with_dir(Some(&config)).unwrap(); + // Interrupted old certificate-first writes, including an empty key. + for broken_key in [ + None, + Some(b"".as_slice()), + Some(b"truncated key".as_slice()), + ] { + fs::remove_file(&key).unwrap(); + if let Some(bytes) = broken_key { + fs::write(&key, bytes).unwrap(); + } + let (_, recovered) = + CertificateManager::load_or_generate_ca_with_dir(Some(&config)).unwrap(); + assert!(ca_cert_matches_key(&fs::read(&cert).unwrap(), &recovered)); + assert_eq!( + fs::metadata(&key).unwrap().permissions().mode() & 0o777, + 0o600 + ); + let healthy = (fs::read(&cert).unwrap(), fs::read(&key).unwrap()); + CertificateManager::load_or_generate_ca_with_dir(Some(&config)).unwrap(); + assert_eq!(healthy, (fs::read(&cert).unwrap(), fs::read(&key).unwrap())); + } + } + + #[test] + fn ca_repairs_certificate_without_rotating_valid_key() { + let dir = tempfile::tempdir().unwrap(); + let config = Utf8PathBuf::from_path_buf(dir.path().to_path_buf()).unwrap(); + let cert = config.join("ca-cert.pem"); + let key = config.join("ca-key.pem"); + let (_, original) = + CertificateManager::load_or_generate_ca_with_dir(Some(&config)).unwrap(); + let key_bytes = fs::read(&key).unwrap(); + // Model a crash after key publication, a torn old cert, and a mismatched pair. + for replacement in [ + None, + Some(b"".to_vec()), + Some( + rcgen::generate_simple_self_signed(vec!["other.invalid".into()]) + .unwrap() + .cert + .pem() + .into_bytes(), + ), + ] { + fs::remove_file(&cert).unwrap(); + if let Some(bytes) = replacement { + fs::write(&cert, bytes).unwrap(); + } + CertificateManager::load_or_generate_ca_with_dir(Some(&config)).unwrap(); + assert_eq!(key_bytes, fs::read(&key).unwrap()); + assert!(ca_cert_matches_key(&fs::read(&cert).unwrap(), &original)); + } + } + #[test] fn ca_key_is_private_on_creation_and_cached_load() { let dir = tempfile::tempdir().unwrap(); diff --git a/tests/security_regressions.rs b/tests/security_regressions.rs index fd2d7ffb..bd609ede 100644 --- a/tests/security_regressions.rs +++ b/tests/security_regressions.rs @@ -37,13 +37,19 @@ fn stalled_processor_stdin_times_out() { let start = std::time::Instant::now(); let output = Command::new(env!("CARGO_BIN_EXE_httpjail")) .args(["--proc", "/usr/bin/caffeinate", "--test"]) + .env_remove("RUST_LOG") .arg(url) .output() .unwrap(); assert_eq!(output.status.code(), Some(1)); assert!(start.elapsed() < Duration::from_secs(8)); assert!( - String::from_utf8_lossy(&output.stderr).contains("Program stdin timeout"), + String::from_utf8_lossy(&output.stdout).contains("Program evaluation timed out"), + "{}", + String::from_utf8_lossy(&output.stdout) + ); + assert!( + output.stderr.is_empty(), "{}", String::from_utf8_lossy(&output.stderr) ); From 6b9e7363dfd2ccb6f84ad28415b2693ba57b85c4 Mon Sep 17 00:00:00 2001 From: Ammar Date: Thu, 24 Sep 2026 10:49:02 -0500 Subject: [PATCH 3/3] test: stabilize connection-close and shared-port regressions --- src/proxy_tls.rs | 9 ++++++--- tests/weak_integration_max_tx_bytes.rs | 4 ++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/proxy_tls.rs b/src/proxy_tls.rs index 75da98c2..12d9213a 100644 --- a/src/proxy_tls.rs +++ b/src/proxy_tls.rs @@ -800,11 +800,14 @@ mod tests { .await .unwrap(); let mut response = [0; 64]; - let count = timeout(Duration::from_secs(2), client.read(&mut response)) + let result = timeout(Duration::from_secs(2), client.read(&mut response)) .await - .unwrap() .unwrap(); - assert_eq!(count, 0, "oversized CONNECT header must not open a tunnel"); + // Closing with unread request bytes may send RST rather than FIN. + match result { + Ok(count) => assert_eq!(count, 0, "oversized CONNECT header must not open a tunnel"), + Err(error) => assert_eq!(error.kind(), std::io::ErrorKind::ConnectionReset), + } } #[tokio::test] diff --git a/tests/weak_integration_max_tx_bytes.rs b/tests/weak_integration_max_tx_bytes.rs index 724a274c..9ed410cd 100644 --- a/tests/weak_integration_max_tx_bytes.rs +++ b/tests/weak_integration_max_tx_bytes.rs @@ -141,6 +141,8 @@ async fn start_httpjail(js_config: &str, proxy_port: u16) -> std::process::Child httpjail } +// Both cases bind the same HTTP/HTTPS proxy ports. +#[serial_test::serial] #[tokio::test] async fn test_max_tx_bytes_truncates_without_content_length() { let (backend_listener, backend_port, bytes_counter) = setup_backend().await; @@ -216,6 +218,8 @@ async fn test_max_tx_bytes_truncates_without_content_length() { assert!(bytes_received > 0, "Backend should have received some data"); } +// Both cases bind the same HTTP/HTTPS proxy ports. +#[serial_test::serial] #[tokio::test] async fn test_max_tx_bytes_rejects_with_content_length() { let (backend_listener, backend_port, bytes_counter) = setup_backend().await;