Skip to content

Commit c6d2d42

Browse files
chrislearnchrislearn
authored
[codex] Harden proxy security review findings (#36)
* Harden proxy security review findings * Support dynamic upstream snapshots
1 parent 2ede2c4 commit c6d2d42

17 files changed

Lines changed: 1249 additions & 384 deletions

File tree

‎Cargo.lock‎

Lines changed: 10 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ hyper-rustls = { version = "0.27", default-features = false, features = [
4242
"http1",
4343
"http2",
4444
"logging",
45+
"webpki-roots",
4546
] }
4647
certon = { version = "0.1.3" }
4748
kdl = "6"

‎_todos.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# Security and Code Review Todos
2+
3+
## This pass
4+
5+
- [x] Use real trust roots for HTTPS upstream certificate verification unless `tls-skip-verify` is explicitly enabled.
6+
- [x] Avoid buffering reverse-proxy request bodies when retries are disabled.
7+
- [x] Block CGI script resolution outside the configured CGI root and add focused tests.
8+
- [x] Harden template `include` path resolution so configured roots cannot be escaped, and preserve UTF-8 while scanning templates.
9+
- [x] Bound admin API JSON request bodies to prevent unbounded memory growth.
10+
- [x] Add focused documentation for HTTPS upstream verification and admin API token exposure.
11+
12+
## Follow-up review findings
13+
14+
- [x] Dynamic DNS/SRV upstreams are parsed and documented, but the current `UpstreamPool` selection path is static. This needs a separate design that reconciles dynamic backend snapshots with health checks, connection counters, weighted policies, and retries.

‎crates/core/src/admin/mod.rs‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use std::net::SocketAddr;
1010
use std::sync::Arc;
1111

1212
use http::{Request, Response, StatusCode};
13-
use http_body_util::BodyExt;
13+
use http_body_util::{BodyExt, LengthLimitError, Limited};
1414
use hyper::body::Incoming;
1515
use hyper::service::service_fn;
1616
use hyper_util::rt::{TokioIo, TokioTimer};
@@ -27,6 +27,8 @@ use crate::runtime_services::{
2727
use crate::server::{AppState, RuntimeStateError};
2828
use crate::{Body, ProxyError, full_body};
2929

30+
const MAX_ADMIN_JSON_BODY_SIZE: usize = 1024 * 1024;
31+
3032
#[derive(serde::Serialize)]
3133
struct RuntimeTargetView {
3234
service_id: String,
@@ -999,9 +1001,18 @@ async fn json_body<T>(req: Request<Incoming>) -> Result<T, Response<Body>>
9991001
where
10001002
T: serde::de::DeserializeOwned,
10011003
{
1002-
let body_bytes = match req.into_body().collect().await {
1004+
let body_bytes = match Limited::new(req.into_body(), MAX_ADMIN_JSON_BODY_SIZE)
1005+
.collect()
1006+
.await
1007+
{
10031008
Ok(collected) => collected.to_bytes(),
10041009
Err(error) => {
1010+
if error.downcast_ref::<LengthLimitError>().is_some() {
1011+
return Err(json_response(
1012+
StatusCode::PAYLOAD_TOO_LARGE,
1013+
r#"{"error":"request body too large"}"#,
1014+
));
1015+
}
10051016
return Err(json_response(
10061017
StatusCode::BAD_REQUEST,
10071018
&format!(r#"{{"error":"failed to read body: {error}"}}"#),

‎crates/core/src/hoops/templates.rs‎

Lines changed: 84 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::path::{Path, PathBuf};
1+
use std::path::{Component, Path, PathBuf};
22

33
use bytes::Bytes;
44
use http::header::{CONTENT_LENGTH, CONTENT_TYPE, HOST};
@@ -159,8 +159,12 @@ fn process_template(input: &str, vars: &TemplateVars, root: &Option<PathBuf>) ->
159159
}
160160
}
161161

162-
result.push(bytes[i] as char);
163-
i += 1;
162+
let ch = input[i..]
163+
.chars()
164+
.next()
165+
.expect("index is always on a UTF-8 character boundary");
166+
result.push(ch);
167+
i += ch.len_utf8();
164168
}
165169

166170
result
@@ -214,27 +218,11 @@ fn resolve_tag(tag: &str, vars: &TemplateVars, root: &Option<PathBuf>) -> String
214218

215219
/// Read and return the contents of an included file.
216220
fn resolve_include(path_str: &str, root: &Option<PathBuf>) -> String {
217-
let path = Path::new(path_str);
218-
219-
// If the path is relative and root is set, resolve relative to root.
220-
let full_path = if path.is_relative() {
221-
match root {
222-
Some(root_dir) => root_dir.join(path),
223-
None => PathBuf::from(path),
224-
}
225-
} else {
226-
PathBuf::from(path)
221+
let Some(full_path) = resolve_include_path(path_str, root) else {
222+
debug!(path = path_str, "blocked template include outside root");
223+
return String::new();
227224
};
228225

229-
// Prevent path traversal in includes.
230-
let path_str_normalized = full_path.to_string_lossy().replace('\\', "/");
231-
for component in path_str_normalized.split('/') {
232-
if component == ".." {
233-
debug!(path = %full_path.display(), "blocked include with path traversal");
234-
return String::new();
235-
}
236-
}
237-
238226
match std::fs::read_to_string(&full_path) {
239227
Ok(contents) => {
240228
debug!(path = %full_path.display(), "included template file");
@@ -246,3 +234,77 @@ fn resolve_include(path_str: &str, root: &Option<PathBuf>) -> String {
246234
}
247235
}
248236
}
237+
238+
fn resolve_include_path(path_str: &str, root: &Option<PathBuf>) -> Option<PathBuf> {
239+
let path = Path::new(path_str);
240+
if has_forbidden_path_components(path) {
241+
return None;
242+
}
243+
244+
match root {
245+
Some(root_dir) => {
246+
if path.is_absolute() {
247+
return None;
248+
}
249+
let root = root_dir.canonicalize().ok()?;
250+
let candidate = root.join(path).canonicalize().ok()?;
251+
if candidate.starts_with(&root) {
252+
Some(candidate)
253+
} else {
254+
None
255+
}
256+
}
257+
None => Some(path.to_path_buf()),
258+
}
259+
}
260+
261+
fn has_forbidden_path_components(path: &Path) -> bool {
262+
path.components().any(|component| {
263+
matches!(
264+
component,
265+
Component::ParentDir | Component::RootDir | Component::Prefix(_)
266+
)
267+
})
268+
}
269+
270+
#[cfg(test)]
271+
mod tests {
272+
use super::*;
273+
274+
fn vars() -> TemplateVars {
275+
TemplateVars {
276+
host: "example.com".to_string(),
277+
path: "/hello".to_string(),
278+
method: "GET".to_string(),
279+
scheme: "https".to_string(),
280+
client_ip: "127.0.0.1".to_string(),
281+
query: "a=1".to_string(),
282+
uri: "/hello?a=1".to_string(),
283+
remote_addr: "127.0.0.1:1234".to_string(),
284+
server_name: "example.com".to_string(),
285+
}
286+
}
287+
288+
#[test]
289+
fn process_template_preserves_utf8_text() {
290+
let rendered = process_template("你好 {{path}}", &vars(), &None);
291+
292+
assert_eq!(rendered, "你好 /hello");
293+
}
294+
295+
#[test]
296+
fn include_path_must_stay_under_configured_root() {
297+
let dir = tempfile::tempdir().unwrap();
298+
let include = dir.path().join("fragment.html");
299+
std::fs::write(&include, "ok").unwrap();
300+
301+
let root = Some(dir.path().to_path_buf());
302+
303+
assert_eq!(
304+
resolve_include_path("fragment.html", &root),
305+
Some(include.canonicalize().unwrap())
306+
);
307+
assert!(resolve_include_path("../secret.html", &root).is_none());
308+
assert!(resolve_include_path(include.to_str().unwrap(), &root).is_none());
309+
}
310+
}

‎crates/core/src/proxy/cgi.rs‎

Lines changed: 112 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
//! as a CGI-style response (headers followed by body).
66
77
use std::collections::HashMap;
8-
use std::path::PathBuf;
8+
use std::path::{Component, Path, PathBuf};
99
use std::process::Stdio;
1010

1111
use http::{Response, StatusCode};
@@ -66,13 +66,19 @@ impl CgiHandler {
6666
client_addr: std::net::SocketAddr,
6767
) -> Result<Response<crate::Body>, ProxyError> {
6868
let path = request.uri().path().to_string();
69-
let script_path = self.root.join(path.trim_start_matches('/'));
70-
71-
if !script_path.exists() {
72-
return Ok(Response::builder()
73-
.status(StatusCode::NOT_FOUND)
74-
.body(full_body("Not Found"))?);
75-
}
69+
let script_path = match resolve_script_path(&self.root, &path) {
70+
Ok(script_path) => script_path,
71+
Err(CgiPathError::NotFound) => {
72+
return Ok(Response::builder()
73+
.status(StatusCode::NOT_FOUND)
74+
.body(full_body("Not Found"))?);
75+
}
76+
Err(CgiPathError::Forbidden) => {
77+
return Ok(Response::builder()
78+
.status(StatusCode::FORBIDDEN)
79+
.body(full_body("Forbidden"))?);
80+
}
81+
};
7682

7783
// Collect the request body before decomposing the request.
7884
let (parts, body) = request.into_parts();
@@ -163,6 +169,79 @@ impl CgiHandler {
163169
}
164170
}
165171

172+
// ---------------------------------------------------------------------------
173+
// CGI path resolution
174+
// ---------------------------------------------------------------------------
175+
176+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
177+
enum CgiPathError {
178+
NotFound,
179+
Forbidden,
180+
}
181+
182+
fn resolve_script_path(root: &Path, uri_path: &str) -> Result<PathBuf, CgiPathError> {
183+
let decoded_path = percent_decode_path(uri_path).ok_or(CgiPathError::Forbidden)?;
184+
if has_forbidden_path_components(Path::new(decoded_path.trim_start_matches('/'))) {
185+
return Err(CgiPathError::Forbidden);
186+
}
187+
188+
let candidate = root.join(uri_path.trim_start_matches('/'));
189+
let root = root.canonicalize().map_err(|_| CgiPathError::NotFound)?;
190+
let script = candidate
191+
.canonicalize()
192+
.map_err(|_| CgiPathError::NotFound)?;
193+
194+
if !script.starts_with(&root) {
195+
return Err(CgiPathError::Forbidden);
196+
}
197+
if !script.is_file() {
198+
return Err(CgiPathError::NotFound);
199+
}
200+
201+
Ok(script)
202+
}
203+
204+
fn has_forbidden_path_components(path: &Path) -> bool {
205+
path.components().any(|component| {
206+
matches!(
207+
component,
208+
Component::ParentDir | Component::RootDir | Component::Prefix(_)
209+
)
210+
})
211+
}
212+
213+
fn percent_decode_path(path: &str) -> Option<String> {
214+
let bytes = path.as_bytes();
215+
let mut output = Vec::with_capacity(bytes.len());
216+
let mut i = 0;
217+
218+
while i < bytes.len() {
219+
if bytes[i] == b'%' {
220+
if i + 2 >= bytes.len() {
221+
return None;
222+
}
223+
let hi = hex_value(bytes[i + 1])?;
224+
let lo = hex_value(bytes[i + 2])?;
225+
output.push((hi << 4) | lo);
226+
i += 3;
227+
} else {
228+
output.push(bytes[i]);
229+
i += 1;
230+
}
231+
}
232+
233+
String::from_utf8(output).ok()
234+
}
235+
236+
fn hex_value(byte: u8) -> Option<u8> {
237+
match byte {
238+
b'0'..=b'9' => Some(byte - b'0'),
239+
b'a'..=b'f' => Some(byte - b'a' + 10),
240+
b'A'..=b'F' => Some(byte - b'A' + 10),
241+
_ => None,
242+
}
243+
}
244+
166245
// ---------------------------------------------------------------------------
167246
// CGI response parsing (shared with SCGI)
168247
// ---------------------------------------------------------------------------
@@ -268,4 +347,29 @@ mod tests {
268347
let resp = parse_cgi_response(data).unwrap();
269348
assert_eq!(resp.status(), 200);
270349
}
350+
351+
#[test]
352+
fn resolve_script_path_allows_files_under_root() {
353+
let dir = tempfile::tempdir().unwrap();
354+
let script = dir.path().join("run.cgi");
355+
std::fs::write(&script, "echo ok").unwrap();
356+
357+
let resolved = resolve_script_path(dir.path(), "/run.cgi").unwrap();
358+
359+
assert_eq!(resolved, script.canonicalize().unwrap());
360+
}
361+
362+
#[test]
363+
fn resolve_script_path_rejects_parent_escape() {
364+
let dir = tempfile::tempdir().unwrap();
365+
let outside = dir.path().parent().unwrap().join("outside-gatel-cgi-test");
366+
std::fs::write(&outside, "echo outside").unwrap();
367+
368+
let result = resolve_script_path(dir.path(), "/../outside-gatel-cgi-test");
369+
let encoded = resolve_script_path(dir.path(), "/%2e%2e/outside-gatel-cgi-test");
370+
371+
std::fs::remove_file(&outside).unwrap();
372+
assert_eq!(result, Err(CgiPathError::Forbidden));
373+
assert_eq!(encoded, Err(CgiPathError::Forbidden));
374+
}
271375
}

‎crates/core/src/proxy/dns_upstream.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
//! DNS-based dynamic upstream resolution.
22
//!
33
//! Periodically resolves a DNS name and updates an `UpstreamPool` with the
4-
//! resulting addresses. Currently supports A/AAAA records via
5-
//! `tokio::net::lookup_host`; SRV record support is planned as a future
6-
//! extension.
4+
//! resulting addresses. Currently supports A/AAAA records via
5+
//! `tokio::net::lookup_host`; DNS SRV records are handled by
6+
//! `crate::proxy::srv_upstream`.
77
88
use std::sync::Arc;
99
use std::sync::atomic::{AtomicBool, AtomicUsize};
@@ -23,7 +23,7 @@ use super::upstream::Backend;
2323
pub enum DnsRecordType {
2424
/// A and AAAA records. The port is taken from configuration.
2525
A,
26-
/// SRV records (future extension). Port comes from the SRV record.
26+
/// SRV records. Port comes from the SRV record.
2727
SRV,
2828
}
2929

0 commit comments

Comments
 (0)