Skip to content

Commit 834f8aa

Browse files
authored
fix: security hardening batch 1 (SEC-002 through SEC-010) (#548)
* fix(l7): reject ambiguous HTTP framing in REST proxy (SEC-009) Harden the L7 REST proxy HTTP parser against request smuggling: - Reject requests containing both Content-Length and Transfer-Encoding headers per RFC 7230 Section 3.3.3 (CL/TE ambiguity) - Replace String::from_utf8_lossy with strict UTF-8 validation to prevent interpretation gaps with upstream servers - Reject bare LF line endings (require CRLF per HTTP spec) - Validate HTTP version string (HTTP/1.0 or HTTP/1.1 only) * fix(server): harden shell_escape and command construction (SEC-002) Harden the gRPC exec handler against command injection via the structured-to-shell-string conversion: - Reject null bytes and newlines/carriage returns in shell_escape() - Add input validation in exec_sandbox: reject control characters in command args, env values, and workdir - Enforce size limits: max 1024 args, 32 KiB per arg/value, 4 KiB workdir, 256 KiB total assembled command string - Change shell_escape and build_remote_exec_command to return Result so callers must handle validation failures * fix(server): add command validation at SSH transport boundary (SEC-003) Add defense-in-depth validation in run_exec_with_russh before sending the command to the sandbox SSH server: - Reject null bytes in command string at transport boundary - Enforce max command length (256 KiB) at transport boundary - Enhance stream_exec_over_ssh logging with command length, stdin length, and truncated command preview for audit trail * fix(sandbox): validate port range and extend loopback check in SSH (SEC-007) Harden the SSH direct-tcpip channel handler: - Validate port_to_connect <= 65535 before u32-to-u16 cast to prevent port truncation (e.g., 65537 becoming port 1) - Replace string-literal loopback check with is_loopback_host() that covers the full 127.0.0.0/8 range, IPv4-mapped IPv6 (::ffff:127.x), bracketed IPv6, and case-insensitive localhost - Remove #[allow(clippy::cast_possible_truncation)] since the cast is now proven safe by the preceding range check * fix(sandbox): sanitize inference error messages returned to sandbox (SEC-008) Replace verbatim internal error strings in router_error_to_http with generic messages to prevent information leakage to sandboxed code. Upstream URLs, internal hostnames, TLS details, and file paths are no longer exposed. Full error context is still logged server-side at warn level by the caller for debugging. * fix(server): block internal IPs in SSH proxy target validation (SEC-006) Add IP validation in start_single_use_ssh_proxy to prevent SSRF if a sandbox status record were poisoned: - Resolve DNS before connecting and validate the resolved IP - Block loopback (127.0.0.0/8) and link-local (169.254.0.0/16, covers cloud metadata endpoint) addresses - Block IPv4-mapped IPv6 variants of the same ranges - Connect to the validated SocketAddr directly to prevent TOCTOU - Add debug logging of resolved target IP for audit * fix(sandbox): add resource limits to chunked body parser (SEC-010) Harden parse_chunked_body in the inference interception path: - Replace all unchecked +2 additions with checked_add for consistent overflow safety across all target architectures - Add MAX_CHUNKED_BODY (10 MiB) to cap decoded body size - Add MAX_CHUNK_COUNT (4096) to prevent CPU exhaustion via tiny chunks - Early-reject chunk sizes larger than remaining buffer space * fix(cli): double-escape command for SSH path, validate host and name (SEC-004) Harden doctor_exec against command injection in the SSH remote path: - Apply shell_escape to inner_cmd in the SSH path so it survives the double shell interpretation (SSH remote shell + sh -lc). This also fixes a correctness bug where multi-word commands were silently broken in the SSH path. - Add validate_gateway_name to reject shell metacharacters in gateway names before use in container_name - Add validate_ssh_host to reject metacharacters in remote_host loaded from metadata.json * fix(sandbox): add CIDR breadth warning and control-plane port blocklist (SEC-005) Defense-in-depth for the allowed_ips feature: - Log a warning when a CIDR entry has a prefix length < /16, as overly broad ranges may unintentionally expose control-plane services - Block K8s API (6443), etcd (2379/2380), and kubelet (10250/10255) ports unconditionally in resolve_and_check_allowed_ips, even when the resolved IP matches an allowed_ips entry * test(e2e): update assertion for sanitized inference error message (SEC-008) The SEC-008 fix changed the error message from 'no compatible route for source protocol ...' to 'no compatible inference route available'. Update the E2E assertion substring to match.
1 parent b4e20c1 commit 834f8aa

File tree

7 files changed

+860
-68
lines changed

7 files changed

+860
-68
lines changed

crates/openshell-cli/src/run.rs

Lines changed: 102 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use hyper::{Request, StatusCode};
1414
use hyper_rustls::HttpsConnectorBuilder;
1515
use hyper_util::{client::legacy::Client, rt::TokioExecutor};
1616
use indicatif::{MultiProgress, ProgressBar, ProgressStyle};
17-
use miette::{IntoDiagnostic, Result, WrapErr};
17+
use miette::{IntoDiagnostic, Result, WrapErr, miette};
1818
use openshell_bootstrap::{
1919
DeployOptions, GatewayMetadata, RemoteOptions, clear_active_gateway, container_name,
2020
extract_host_from_ssh_destination, get_gateway_metadata, list_gateways, load_active_gateway,
@@ -1653,6 +1653,7 @@ pub fn doctor_exec(
16531653
ssh_key: Option<&str>,
16541654
command: &[String],
16551655
) -> Result<()> {
1656+
validate_gateway_name(name)?;
16561657
let container = container_name(name);
16571658
let is_tty = std::io::stdin().is_terminal();
16581659

@@ -1676,7 +1677,15 @@ pub fn doctor_exec(
16761677
};
16771678

16781679
let mut cmd = if let Some(ref host) = remote_host {
1680+
validate_ssh_host(host)?;
1681+
16791682
// Remote: ssh <host> docker exec [-it] <container> sh -lc '<inner_cmd>'
1683+
//
1684+
// SSH concatenates all arguments after the hostname into a single
1685+
// string for the remote shell, so inner_cmd must be escaped twice:
1686+
// once for `sh -lc` (already done above) and once for the SSH
1687+
// remote shell (done here).
1688+
let ssh_escaped_cmd = shell_escape(&inner_cmd);
16801689
let mut c = Command::new("ssh");
16811690
if let Some(key) = ssh_key {
16821691
c.args(["-i", key]);
@@ -1693,7 +1702,7 @@ pub fn doctor_exec(
16931702
} else {
16941703
c.arg("-i");
16951704
}
1696-
c.args([&container, "sh", "-lc", &inner_cmd]);
1705+
c.args([&container, "sh", "-lc", &ssh_escaped_cmd]);
16971706
c
16981707
} else {
16991708
// Local: docker exec [-it] <container> sh -lc '<inner_cmd>'
@@ -1790,6 +1799,42 @@ fn shell_escape(s: &str) -> String {
17901799
format!("'{}'", s.replace('\'', "'\\''"))
17911800
}
17921801

1802+
/// Validate that a gateway name is safe for use in container/volume/network
1803+
/// names and shell commands. Rejects names with characters outside the set
1804+
/// `[a-zA-Z0-9._-]`.
1805+
fn validate_gateway_name(name: &str) -> Result<()> {
1806+
if name.is_empty() {
1807+
return Err(miette!("gateway name is empty"));
1808+
}
1809+
if !name
1810+
.bytes()
1811+
.all(|b| b.is_ascii_alphanumeric() || matches!(b, b'.' | b'-' | b'_'))
1812+
{
1813+
return Err(miette!(
1814+
"gateway name contains invalid characters (allowed: alphanumeric, '.', '-', '_')"
1815+
));
1816+
}
1817+
Ok(())
1818+
}
1819+
1820+
/// Validate that an SSH host string is a reasonable hostname or IP address.
1821+
/// Rejects values with shell metacharacters, spaces, or control characters
1822+
/// that could be used for injection via a poisoned metadata.json.
1823+
fn validate_ssh_host(host: &str) -> Result<()> {
1824+
if host.is_empty() {
1825+
return Err(miette!("SSH host is empty"));
1826+
}
1827+
// Allow: alphanumeric, dots, hyphens, colons (IPv6), square brackets ([::1]),
1828+
// and @ (user@host).
1829+
if !host
1830+
.bytes()
1831+
.all(|b| b.is_ascii_alphanumeric() || matches!(b, b'.' | b'-' | b':' | b'[' | b']' | b'@'))
1832+
{
1833+
return Err(miette!("SSH host contains invalid characters: {host}"));
1834+
}
1835+
Ok(())
1836+
}
1837+
17931838
/// Create a sandbox when no gateway is configured.
17941839
///
17951840
/// Bootstraps a new gateway first, then delegates to [`sandbox_create`].
@@ -4942,7 +4987,7 @@ mod tests {
49424987
git_sync_files, http_health_check, image_requests_gpu, inferred_provider_type,
49434988
parse_cli_setting_value, parse_credential_pairs, provisioning_timeout_message,
49444989
ready_false_condition_message, resolve_gateway_control_target_from, sandbox_should_persist,
4945-
source_requests_gpu,
4990+
shell_escape, source_requests_gpu, validate_gateway_name, validate_ssh_host,
49464991
};
49474992
use crate::TEST_ENV_LOCK;
49484993
use hyper::StatusCode;
@@ -5457,4 +5502,58 @@ mod tests {
54575502
server.join().expect("server thread");
54585503
assert_eq!(status, Some(StatusCode::OK));
54595504
}
5505+
5506+
// ---- SEC-004: validate_gateway_name, validate_ssh_host, shell_escape ----
5507+
5508+
#[test]
5509+
fn validate_gateway_name_accepts_valid_names() {
5510+
assert!(validate_gateway_name("openshell").is_ok());
5511+
assert!(validate_gateway_name("my-gateway").is_ok());
5512+
assert!(validate_gateway_name("gateway_v2").is_ok());
5513+
assert!(validate_gateway_name("gw.prod").is_ok());
5514+
}
5515+
5516+
#[test]
5517+
fn validate_gateway_name_rejects_invalid_names() {
5518+
assert!(validate_gateway_name("").is_err());
5519+
assert!(validate_gateway_name("gw;rm -rf /").is_err());
5520+
assert!(validate_gateway_name("gw name").is_err());
5521+
assert!(validate_gateway_name("gw$(id)").is_err());
5522+
assert!(validate_gateway_name("gw\nmalicious").is_err());
5523+
}
5524+
5525+
#[test]
5526+
fn validate_ssh_host_accepts_valid_hosts() {
5527+
assert!(validate_ssh_host("192.168.1.1").is_ok());
5528+
assert!(validate_ssh_host("example.com").is_ok());
5529+
assert!(validate_ssh_host("user@host.com").is_ok());
5530+
assert!(validate_ssh_host("[::1]").is_ok());
5531+
assert!(validate_ssh_host("2001:db8::1").is_ok());
5532+
}
5533+
5534+
#[test]
5535+
fn validate_ssh_host_rejects_invalid_hosts() {
5536+
assert!(validate_ssh_host("").is_err());
5537+
assert!(validate_ssh_host("host;rm -rf /").is_err());
5538+
assert!(validate_ssh_host("host$(id)").is_err());
5539+
assert!(validate_ssh_host("host name").is_err());
5540+
assert!(validate_ssh_host("host\nmalicious").is_err());
5541+
}
5542+
5543+
#[test]
5544+
fn shell_escape_double_escape_for_ssh() {
5545+
// Simulate the double-escape path for SSH:
5546+
// First escape for sh -lc, then escape again for SSH remote shell.
5547+
let inner_cmd = "KUBECONFIG=/etc/rancher/k3s/k3s.yaml echo 'hello world'";
5548+
let ssh_escaped = shell_escape(inner_cmd);
5549+
// The result should be single-quoted (wrapping the entire inner_cmd)
5550+
assert!(
5551+
ssh_escaped.starts_with('\''),
5552+
"should be single-quoted: {ssh_escaped}"
5553+
);
5554+
assert!(
5555+
ssh_escaped.ends_with('\''),
5556+
"should end with single-quote: {ssh_escaped}"
5557+
);
5558+
}
54605559
}

crates/openshell-sandbox/src/l7/inference.rs

Lines changed: 74 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -171,43 +171,69 @@ pub fn try_parse_http_request(buf: &[u8]) -> ParseResult {
171171
)
172172
}
173173

174+
/// Maximum decoded body size from chunked transfer encoding (10 MiB).
175+
/// Matches the caller's `MAX_INFERENCE_BUF` limit.
176+
const MAX_CHUNKED_BODY: usize = 10 * 1024 * 1024;
177+
178+
/// Maximum number of chunks to process. Normal HTTP clients send the body
179+
/// in a handful of large chunks; thousands of tiny chunks indicate abuse.
180+
const MAX_CHUNK_COUNT: usize = 4096;
181+
174182
/// Parse an HTTP chunked body from `buf[start..]`.
175183
///
176184
/// Returns `(decoded_body, total_consumed_bytes_from_buf_start)` when complete,
177-
/// or `None` if more bytes are needed.
185+
/// or `None` if more bytes are needed or resource limits are exceeded.
178186
fn parse_chunked_body(buf: &[u8], start: usize) -> Option<(Vec<u8>, usize)> {
179187
let mut pos = start;
180188
let mut body = Vec::new();
189+
let mut chunk_count: usize = 0;
181190

182191
loop {
192+
chunk_count += 1;
193+
if chunk_count > MAX_CHUNK_COUNT {
194+
return None;
195+
}
196+
183197
let size_line_end = find_crlf(buf, pos)?;
184198
let size_line = std::str::from_utf8(&buf[pos..size_line_end]).ok()?;
185199
let size_token = size_line.split(';').next()?.trim();
186200
let chunk_size = usize::from_str_radix(size_token, 16).ok()?;
187-
pos = size_line_end + 2;
201+
pos = size_line_end.checked_add(2)?;
188202

189203
if chunk_size == 0 {
190204
// Parse trailers (if any). Terminates on empty trailer line.
191205
loop {
192206
let trailer_end = find_crlf(buf, pos)?;
193207
let trailer_line = &buf[pos..trailer_end];
194-
pos = trailer_end + 2;
208+
pos = trailer_end.checked_add(2)?;
195209
if trailer_line.is_empty() {
196210
return Some((body, pos));
197211
}
198212
}
199213
}
200214

215+
// Early reject: chunk cannot possibly fit in remaining buffer.
216+
let remaining = buf.len().saturating_sub(pos);
217+
if chunk_size > remaining {
218+
return None;
219+
}
220+
221+
// Reject if decoded body would exceed size limit.
222+
if body.len().saturating_add(chunk_size) > MAX_CHUNKED_BODY {
223+
return None;
224+
}
225+
201226
let chunk_end = pos.checked_add(chunk_size)?;
202-
if buf.len() < chunk_end + 2 {
227+
let chunk_crlf_end = chunk_end.checked_add(2)?;
228+
if buf.len() < chunk_crlf_end {
203229
return None;
204230
}
205-
if &buf[chunk_end..chunk_end + 2] != b"\r\n" {
231+
if &buf[chunk_end..chunk_crlf_end] != b"\r\n" {
206232
return None;
207233
}
208234

209235
body.extend_from_slice(&buf[pos..chunk_end]);
210-
pos = chunk_end + 2;
236+
pos = chunk_crlf_end;
211237
}
212238
}
213239

@@ -484,4 +510,46 @@ mod tests {
484510
assert!(chunk.ends_with(b"\r\n"));
485511
assert_eq!(chunk.len(), 3 + 2 + 256 + 2); // "100" + \r\n + data + \r\n
486512
}
513+
514+
// ---- SEC-010: parse_chunked_body resource limits ----
515+
516+
#[test]
517+
fn parse_chunked_multi_chunk_body() {
518+
// Two chunks: 5 bytes + 6 bytes
519+
let request = b"POST /v1/chat HTTP/1.1\r\nTransfer-Encoding: chunked\r\n\r\n5\r\nhello\r\n6\r\n world\r\n0\r\n\r\n";
520+
let ParseResult::Complete(parsed, _) = try_parse_http_request(request) else {
521+
panic!("expected Complete");
522+
};
523+
assert_eq!(parsed.body, b"hello world");
524+
}
525+
526+
#[test]
527+
fn parse_chunked_rejects_too_many_chunks() {
528+
// Build a request with MAX_CHUNK_COUNT + 1 tiny chunks
529+
let mut buf = Vec::new();
530+
buf.extend_from_slice(b"POST /v1/chat HTTP/1.1\r\nTransfer-Encoding: chunked\r\n\r\n");
531+
for _ in 0..=MAX_CHUNK_COUNT {
532+
buf.extend_from_slice(b"1\r\nX\r\n");
533+
}
534+
buf.extend_from_slice(b"0\r\n\r\n");
535+
assert!(matches!(
536+
try_parse_http_request(&buf),
537+
ParseResult::Incomplete
538+
));
539+
}
540+
541+
#[test]
542+
fn parse_chunked_within_chunk_count_limit() {
543+
// MAX_CHUNK_COUNT chunks should succeed
544+
let mut buf = Vec::new();
545+
buf.extend_from_slice(b"POST /v1/chat HTTP/1.1\r\nTransfer-Encoding: chunked\r\n\r\n");
546+
for _ in 0..100 {
547+
buf.extend_from_slice(b"1\r\nX\r\n");
548+
}
549+
buf.extend_from_slice(b"0\r\n\r\n");
550+
let ParseResult::Complete(parsed, _) = try_parse_http_request(&buf) else {
551+
panic!("expected Complete for 100 chunks");
552+
};
553+
assert_eq!(parsed.body.len(), 100);
554+
}
487555
}

0 commit comments

Comments
 (0)