From a24b62451c95ef8bf7d344ab4d59174bc476a099 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Mon, 16 Mar 2026 16:08:27 +0530 Subject: [PATCH 1/3] Sanitize synthetic ID value before Set-Cookie interpolation Validate synthetic_id against RFC 6265 cookie-octet rules in set_synthetic_cookie() before header interpolation. Values containing semicolons, whitespace, control characters or non-ASCII bytes are rejected with a warning log and no cookie is set, preventing header-injection attacks. --- crates/common/src/cookies.rs | 137 +++++++++++++++++---- crates/common/src/integrations/registry.rs | 3 + crates/common/src/publisher.rs | 3 + 3 files changed, 119 insertions(+), 24 deletions(-) diff --git a/crates/common/src/cookies.rs b/crates/common/src/cookies.rs index 1b33d99b..294d97a5 100644 --- a/crates/common/src/cookies.rs +++ b/crates/common/src/cookies.rs @@ -59,12 +59,29 @@ pub fn handle_request_cookies( } } -/// Creates a synthetic ID cookie string. +/// Returns `true` if every byte in `value` is a valid RFC 6265 `cookie-octet`. /// -/// Generates a properly formatted cookie with security attributes -/// for storing the synthetic ID. -#[must_use] -pub fn create_synthetic_cookie(settings: &Settings, synthetic_id: &str) -> String { +/// RFC 6265 restricts cookie values to printable US-ASCII excluding whitespace, +/// double-quote, comma, semicolon, and backslash. Rejecting these characters +/// prevents header-injection attacks where a crafted value could append +/// spurious cookie attributes (e.g. `evil; Domain=.attacker.com`). +/// +/// Non-ASCII characters (multi-byte UTF-8) are always rejected because their +/// byte values exceed `0x7E`. +fn is_safe_cookie_value(value: &str) -> bool { + // RFC 6265 §4.1.1 cookie-octet: + // 0x21 — '!' + // 0x23–0x2B — '#' through '+' (excludes 0x22 DQUOTE) + // 0x2D–0x3A — '-' through ':' (excludes 0x2C comma) + // 0x3C–0x5B — '<' through '[' (excludes 0x3B semicolon) + // 0x5D–0x7E — ']' through '~' (excludes 0x5C backslash, 0x7F DEL) + // All control characters (0x00–0x20) and non-ASCII (0x80+) are also excluded. + value + .bytes() + .all(|b| matches!(b, 0x21 | 0x23..=0x2B | 0x2D..=0x3A | 0x3C..=0x5B | 0x5D..=0x7E)) +} + +fn create_synthetic_cookie(settings: &Settings, synthetic_id: &str) -> String { format!( "{}={}; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}", COOKIE_SYNTHETIC_ID, synthetic_id, settings.publisher.cookie_domain, COOKIE_MAX_AGE, @@ -73,13 +90,23 @@ pub fn create_synthetic_cookie(settings: &Settings, synthetic_id: &str) -> Strin /// Sets the synthetic ID cookie on the given response. /// -/// This helper abstracts the logic of creating the cookie string and appending -/// the Set-Cookie header to the response. +/// Validates `synthetic_id` against RFC 6265 `cookie-octet` rules before +/// interpolation. If the value contains unsafe characters (e.g. semicolons), +/// the cookie is not set and a warning is logged. This prevents an attacker +/// from injecting spurious cookie attributes via a controlled ID value. +/// +/// `cookie_domain` comes from operator configuration and is considered trusted. pub fn set_synthetic_cookie( settings: &Settings, response: &mut fastly::Response, synthetic_id: &str, ) { + if !is_safe_cookie_value(synthetic_id) { + log::warn!( + "Rejecting synthetic_id for Set-Cookie: contains characters illegal in a cookie value" + ); + return; + } response.append_header( header::SET_COOKIE, create_synthetic_cookie(settings, synthetic_id), @@ -168,35 +195,97 @@ mod tests { } #[test] - fn test_create_synthetic_cookie() { + fn test_set_synthetic_cookie() { let settings = create_test_settings(); - let result = create_synthetic_cookie(&settings, "12345"); + let mut response = fastly::Response::new(); + set_synthetic_cookie(&settings, &mut response, "abc123.XyZ789"); + + let cookie_str = response + .get_header(header::SET_COOKIE) + .expect("Set-Cookie header should be present") + .to_str() + .expect("header should be valid UTF-8"); + assert_eq!( - result, + cookie_str, format!( - "{}=12345; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}", + "{}=abc123.XyZ789; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}", COOKIE_SYNTHETIC_ID, settings.publisher.cookie_domain, COOKIE_MAX_AGE, - ) + ), + "Set-Cookie header should match expected format" ); } #[test] - fn test_set_synthetic_cookie() { + fn test_set_synthetic_cookie_rejects_semicolon() { let settings = create_test_settings(); let mut response = fastly::Response::new(); - set_synthetic_cookie(&settings, &mut response, "test-id-123"); + set_synthetic_cookie(&settings, &mut response, "evil; Domain=.attacker.com"); - let cookie_header = response - .get_header(header::SET_COOKIE) - .expect("Set-Cookie header should be present"); - let cookie_str = cookie_header - .to_str() - .expect("header should be valid UTF-8"); + assert!( + response.get_header(header::SET_COOKIE).is_none(), + "Set-Cookie should not be set when value contains a semicolon" + ); + } - let expected = create_synthetic_cookie(&settings, "test-id-123"); - assert_eq!( - cookie_str, expected, - "Set-Cookie header should match create_synthetic_cookie output" + #[test] + fn test_set_synthetic_cookie_rejects_crlf() { + let settings = create_test_settings(); + let mut response = fastly::Response::new(); + set_synthetic_cookie(&settings, &mut response, "evil\r\nX-Injected: header"); + + assert!( + response.get_header(header::SET_COOKIE).is_none(), + "Set-Cookie should not be set when value contains CRLF" + ); + } + + #[test] + fn test_set_synthetic_cookie_rejects_space() { + let settings = create_test_settings(); + let mut response = fastly::Response::new(); + set_synthetic_cookie(&settings, &mut response, "bad value"); + + assert!( + response.get_header(header::SET_COOKIE).is_none(), + "Set-Cookie should not be set when value contains whitespace" + ); + } + + #[test] + fn test_is_safe_cookie_value_accepts_valid_synthetic_id_characters() { + // Hex digits, dot separator, alphanumeric suffix — the full synthetic ID character set + assert!( + is_safe_cookie_value("abcdef0123456789.ABCDEFabcdef"), + "should accept hex digits, dots, and alphanumeric characters" + ); + } + + #[test] + fn test_is_safe_cookie_value_rejects_non_ascii() { + assert!( + !is_safe_cookie_value("valüe"), + "should reject non-ASCII UTF-8 characters" + ); + } + + #[test] + fn test_is_safe_cookie_value_rejects_illegal_characters() { + assert!(!is_safe_cookie_value("val;ue"), "should reject semicolon"); + assert!(!is_safe_cookie_value("val,ue"), "should reject comma"); + assert!( + !is_safe_cookie_value("val\"ue"), + "should reject double-quote" + ); + assert!(!is_safe_cookie_value("val\\ue"), "should reject backslash"); + assert!(!is_safe_cookie_value("val ue"), "should reject space"); + assert!( + !is_safe_cookie_value("val\x00ue"), + "should reject null byte" + ); + assert!( + !is_safe_cookie_value("val\x7fue"), + "should reject DEL character" ); } } diff --git a/crates/common/src/integrations/registry.rs b/crates/common/src/integrations/registry.rs index 158a1e29..bb6d1c06 100644 --- a/crates/common/src/integrations/registry.rs +++ b/crates/common/src/integrations/registry.rs @@ -670,6 +670,9 @@ impl IntegrationRegistry { match synthetic_id_result { Ok(ref synthetic_id) => { response.set_header(HEADER_X_SYNTHETIC_ID, synthetic_id.as_str()); + // Cookie is intentionally not set when synthetic_id contains RFC 6265-illegal + // characters (e.g. a crafted x-synthetic-id header value). The response header + // is still emitted; only cookie persistence is skipped. set_synthetic_cookie(settings, response, synthetic_id.as_str()); } Err(ref err) => { diff --git a/crates/common/src/publisher.rs b/crates/common/src/publisher.rs index 7c86d06d..2d3da079 100644 --- a/crates/common/src/publisher.rs +++ b/crates/common/src/publisher.rs @@ -335,6 +335,9 @@ pub fn handle_publisher_request( } response.set_header(HEADER_X_SYNTHETIC_ID, synthetic_id.as_str()); + // Cookie is intentionally not set when synthetic_id contains RFC 6265-illegal characters + // (e.g. a crafted x-synthetic-id header value). The response header is still emitted so + // upstream components see the ID; only cookie persistence is skipped. set_synthetic_cookie(settings, &mut response, synthetic_id.as_str()); Ok(response) From afbef6c0d63dad6680ce44149cef771fe0f26038 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Tue, 17 Mar 2026 13:38:46 +0530 Subject: [PATCH 2/3] Harden is_safe_cookie_value based on PR review feedback Reject empty strings explicitly (vacuous Iterator::all was a silent pass-through), add #[must_use] to guard against future callers discarding the result, include byte length in the rejection log for operator diagnostics, and document the empty-string contract in the doc comment. --- crates/common/src/cookies.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/crates/common/src/cookies.rs b/crates/common/src/cookies.rs index 294d97a5..564a1259 100644 --- a/crates/common/src/cookies.rs +++ b/crates/common/src/cookies.rs @@ -60,6 +60,7 @@ pub fn handle_request_cookies( } /// Returns `true` if every byte in `value` is a valid RFC 6265 `cookie-octet`. +/// An empty string is always rejected. /// /// RFC 6265 restricts cookie values to printable US-ASCII excluding whitespace, /// double-quote, comma, semicolon, and backslash. Rejecting these characters @@ -68,6 +69,7 @@ pub fn handle_request_cookies( /// /// Non-ASCII characters (multi-byte UTF-8) are always rejected because their /// byte values exceed `0x7E`. +#[must_use] fn is_safe_cookie_value(value: &str) -> bool { // RFC 6265 §4.1.1 cookie-octet: // 0x21 — '!' @@ -76,11 +78,13 @@ fn is_safe_cookie_value(value: &str) -> bool { // 0x3C–0x5B — '<' through '[' (excludes 0x3B semicolon) // 0x5D–0x7E — ']' through '~' (excludes 0x5C backslash, 0x7F DEL) // All control characters (0x00–0x20) and non-ASCII (0x80+) are also excluded. - value - .bytes() - .all(|b| matches!(b, 0x21 | 0x23..=0x2B | 0x2D..=0x3A | 0x3C..=0x5B | 0x5D..=0x7E)) + !value.is_empty() + && value + .bytes() + .all(|b| matches!(b, 0x21 | 0x23..=0x2B | 0x2D..=0x3A | 0x3C..=0x5B | 0x5D..=0x7E)) } +/// Formats the `Set-Cookie` header value for the synthetic ID cookie. fn create_synthetic_cookie(settings: &Settings, synthetic_id: &str) -> String { format!( "{}={}; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}", @@ -103,7 +107,8 @@ pub fn set_synthetic_cookie( ) { if !is_safe_cookie_value(synthetic_id) { log::warn!( - "Rejecting synthetic_id for Set-Cookie: contains characters illegal in a cookie value" + "Rejecting synthetic_id for Set-Cookie: value of {} bytes contains characters illegal in a cookie value", + synthetic_id.len() ); return; } @@ -252,6 +257,11 @@ mod tests { ); } + #[test] + fn test_is_safe_cookie_value_rejects_empty_string() { + assert!(!is_safe_cookie_value(""), "should reject empty string"); + } + #[test] fn test_is_safe_cookie_value_accepts_valid_synthetic_id_characters() { // Hex digits, dot separator, alphanumeric suffix — the full synthetic ID character set From d4c87b6d296fbad8647dddb6ad57796513ae628d Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Wed, 18 Mar 2026 13:35:06 +0530 Subject: [PATCH 3/3] Address PR review feedback on sanitize-set-cookie-value --- crates/common/src/cookies.rs | 3 ++- crates/common/src/integrations/registry.rs | 7 ++++++- crates/common/src/publisher.rs | 2 ++ crates/common/src/synthetic.rs | 7 +++++++ 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/crates/common/src/cookies.rs b/crates/common/src/cookies.rs index 564a1259..55cf49ba 100644 --- a/crates/common/src/cookies.rs +++ b/crates/common/src/cookies.rs @@ -85,6 +85,7 @@ fn is_safe_cookie_value(value: &str) -> bool { } /// Formats the `Set-Cookie` header value for the synthetic ID cookie. +#[must_use] fn create_synthetic_cookie(settings: &Settings, synthetic_id: &str) -> String { format!( "{}={}; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}", @@ -144,7 +145,7 @@ mod tests { } #[test] - fn test_parse_cookies_to_jar_emtpy() { + fn test_parse_cookies_to_jar_empty() { let cookie_str = ""; let jar = parse_cookies_to_jar(cookie_str); diff --git a/crates/common/src/integrations/registry.rs b/crates/common/src/integrations/registry.rs index bb6d1c06..7d87d4f8 100644 --- a/crates/common/src/integrations/registry.rs +++ b/crates/common/src/integrations/registry.rs @@ -658,7 +658,9 @@ impl IntegrationRegistry { // Generate synthetic ID before consuming request let synthetic_id_result = get_or_generate_synthetic_id(settings, &req); - // Set synthetic ID header on the request so integrations can read it + // Set synthetic ID header on the request so integrations can read it. + // Header injection: Fastly's HeaderValue API rejects values containing \r, \n, or \0, + // so a crafted synthetic_id cannot inject additional request headers. if let Ok(ref synthetic_id) = synthetic_id_result { req.set_header(HEADER_X_SYNTHETIC_ID, synthetic_id.as_str()); } @@ -669,6 +671,9 @@ impl IntegrationRegistry { if let Ok(ref mut response) = result { match synthetic_id_result { Ok(ref synthetic_id) => { + // Response-header injection: Fastly's HeaderValue API rejects values + // containing \r, \n, or \0, so a crafted synthetic_id cannot inject + // additional response headers. response.set_header(HEADER_X_SYNTHETIC_ID, synthetic_id.as_str()); // Cookie is intentionally not set when synthetic_id contains RFC 6265-illegal // characters (e.g. a crafted x-synthetic-id header value). The response header diff --git a/crates/common/src/publisher.rs b/crates/common/src/publisher.rs index 76a52c07..841211bd 100644 --- a/crates/common/src/publisher.rs +++ b/crates/common/src/publisher.rs @@ -334,6 +334,8 @@ pub fn handle_publisher_request( ); } + // Response-header injection: Fastly's HeaderValue API rejects values containing \r, \n, + // or \0, so a crafted synthetic_id cannot inject additional response headers. response.set_header(HEADER_X_SYNTHETIC_ID, synthetic_id.as_str()); // Cookie is intentionally not set when synthetic_id contains RFC 6265-illegal characters // (e.g. a crafted x-synthetic-id header value). The response header is still emitted so diff --git a/crates/common/src/synthetic.rs b/crates/common/src/synthetic.rs index 853c7141..7b59b436 100644 --- a/crates/common/src/synthetic.rs +++ b/crates/common/src/synthetic.rs @@ -132,6 +132,10 @@ pub fn get_synthetic_id(req: &Request) -> Result, Report Result, Report { if let Some(cookie) = jar.get(COOKIE_SYNTHETIC_ID) { let id = cookie.value().to_string(); + // The ID is user-supplied from the synthetic_id cookie. The same log-injection + // note as the header path applies: Fastly's log-fastly backend does not treat + // embedded newlines as record separators. log::info!("Using existing Trusted Server ID from cookie: {}", id); return Ok(Some(id)); }