From d14cdedaf9c7180e306e1dfd6576a0ffefdefab8 Mon Sep 17 00:00:00 2001 From: overtrue Date: Tue, 8 Sep 2026 00:15:42 +0800 Subject: [PATCH] fix(s3): apply presigned signed-header rule to custom routes and harden parsing Move the GHSA-g8w9-qw9q-fghr check to the first statement of S3Access::check, apply it in S3Router::check_access so admin, console, STS and extension routes that never reach the access hook enforce the same rule, read X-Amz-SignedHeaders with the exact key the verifier uses and treat a duplicate as signing nothing, and log the rejection as a warn event with the repository field shape. Add presigned GET, unsigned x-amz-copy-source and unsigned Content-Type e2e cases plus a router unit test; raise the security smoke floor to 26 and refresh the selection digests. --- .config/e2e-full-selection.txt | 2 +- .config/e2e-smoke-selection.txt | 2 +- .config/security-smoke-floor.txt | 2 +- .../e2e_test/src/presigned_negative_test.rs | 119 ++++++++++++++++++ rustfs/src/admin/router.rs | 39 +++++- rustfs/src/auth.rs | 43 ++++++- rustfs/src/storage/access.rs | 14 +-- 7 files changed, 205 insertions(+), 16 deletions(-) diff --git a/.config/e2e-full-selection.txt b/.config/e2e-full-selection.txt index 937edfc68..177c91103 100644 --- a/.config/e2e-full-selection.txt +++ b/.config/e2e-full-selection.txt @@ -1,2 +1,2 @@ -sha256-darwin=f9bd098352b824ec81f913e85030fa791f58cf4ee822e9f67bb04d7f48174bb4 +sha256-darwin=dd14f49a7b0e2c156b4457fdd836499d837890d3e439689a4eff9e8875ee2f5b sha256-linux=7c892afa4b9d1591b46bd79c976b647109a277284fddb3b98edced4b0297eda2 diff --git a/.config/e2e-smoke-selection.txt b/.config/e2e-smoke-selection.txt index 1476e9f6c..aa3804f21 100644 --- a/.config/e2e-smoke-selection.txt +++ b/.config/e2e-smoke-selection.txt @@ -1 +1 @@ -sha256=9a09b878dc29d578df615e7e78c86f08ef864de64121fddebe60a648058c2b32 +sha256=6d18f9cce820c51d5589de944e8cc185f73eeca0ea9a9916651943e3759169d0 diff --git a/.config/security-smoke-floor.txt b/.config/security-smoke-floor.txt index 9de7de9dc..2ee0d4190 100644 --- a/.config/security-smoke-floor.txt +++ b/.config/security-smoke-floor.txt @@ -9,4 +9,4 @@ # if the selected count drops below this number, so a rename or removal that # thins the security smoke gate must update this file in the same PR. # Adding tests does not require a bump, but bumping keeps the guard tight. -20 +26 diff --git a/crates/e2e_test/src/presigned_negative_test.rs b/crates/e2e_test/src/presigned_negative_test.rs index 0fac39e13..ec8928f72 100644 --- a/crates/e2e_test/src/presigned_negative_test.rs +++ b/crates/e2e_test/src/presigned_negative_test.rs @@ -472,3 +472,122 @@ async fn ghsa_g8w9_presigned_put_accepts_signed_x_amz_headers() -> Result<(), Bo info!("signed presigned tagging control passed"); Ok(()) } + +/// GHSA-g8w9-qw9q-fghr on the read side: a presigned GET signed with +/// `SignedHeaders=host` must not accept an unsigned SSE-C header. The header +/// would otherwise select a decryption path the presigner never authorised. +#[tokio::test] +async fn ghsa_g8w9_presigned_get_rejects_unsigned_x_amz_headers() -> Result<(), Box> { + init_logging(); + let mut env = RustFSTestEnvironment::new().await?; + setup(&mut env).await?; + + let pr = env + .create_s3_client() + .get_object() + .bucket(BUCKET) + .key(CANONICAL_KEY) + .presigned(valid_config()) + .await?; + + let unsigned: Vec<(&str, &str)> = vec![("x-amz-server-side-encryption-customer-algorithm", "AES256")]; + let headers = pr.headers().chain(unsigned.iter().copied()); + let resp = send_raw(pr.method(), pr.uri(), headers, None).await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status.as_u16(), + 403, + "presigned GET with an unsigned x-amz-* header must be 403, body:\n{body}" + ); + assert_error_code(&body, "AccessDenied"); + assert!( + !body.contains(std::str::from_utf8(CANONICAL_BODY)?), + "rejected GET must not leak the object body" + ); + Ok(()) +} + +/// GHSA-g8w9-qw9q-fghr: an unsigned `x-amz-copy-source` would turn a presigned +/// PutObject into a CopyObject of an arbitrary readable key, since operation +/// routing happens before authorization. The presigned upload must fail and +/// leave nothing behind. +#[tokio::test] +async fn ghsa_g8w9_presigned_put_rejects_unsigned_copy_source() -> Result<(), Box> { + init_logging(); + let mut env = RustFSTestEnvironment::new().await?; + setup(&mut env).await?; + + let key = "presigned-put-unsigned-copy-source.txt"; + let pr = env + .create_s3_client() + .put_object() + .bucket(BUCKET) + .key(key) + .presigned(valid_config()) + .await?; + + let copy_source = format!("/{BUCKET}/{CANONICAL_KEY}"); + let unsigned: Vec<(&str, &str)> = vec![("x-amz-copy-source", copy_source.as_str())]; + let headers = pr.headers().chain(unsigned.iter().copied()); + let resp = send_raw(pr.method(), pr.uri(), headers, None).await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status.as_u16(), + 403, + "presigned PUT with an unsigned copy source must be 403, body:\n{body}" + ); + assert_error_code(&body, "AccessDenied"); + + let error = env + .create_s3_client() + .head_object() + .bucket(BUCKET) + .key(key) + .send() + .await + .expect_err("rejected copy must not create the destination object"); + assert_eq!(error.raw_response().map(|response| response.status().as_u16()), Some(404)); + Ok(()) +} + +/// GHSA-g8w9-qw9q-fghr boundary control: the rule covers `x-amz-*` only. A +/// plain `Content-Type` on a `SignedHeaders=host` presigned PUT is outside +/// SigV4's signed-header requirement (AWS S3 accepts it too) and must keep +/// working, so the negative tests above cannot pass by rejecting every +/// unsigned header. +#[tokio::test] +async fn ghsa_g8w9_presigned_put_still_accepts_unsigned_non_amz_headers() -> Result<(), Box> +{ + init_logging(); + let mut env = RustFSTestEnvironment::new().await?; + setup(&mut env).await?; + + let key = "presigned-put-unsigned-content-type.txt"; + let pr = env + .create_s3_client() + .put_object() + .bucket(BUCKET) + .key(key) + .presigned(valid_config()) + .await?; + + let unsigned: Vec<(&str, &str)> = vec![("content-type", "text/x-rustfs-test")]; + let headers = pr.headers().chain(unsigned.iter().copied()); + let resp = send_raw(pr.method(), pr.uri(), headers, Some(b"plain-header-upload".to_vec())).await?; + let status = resp.status(); + let body = resp.text().await?; + assert!( + status.is_success(), + "presigned PUT with an unsigned Content-Type must succeed, got {status}, body:\n{body}" + ); + + let head = env.create_s3_client().head_object().bucket(BUCKET).key(key).send().await?; + assert_eq!( + head.content_type(), + Some("text/x-rustfs-test"), + "unsigned Content-Type must still be applied" + ); + Ok(()) +} diff --git a/rustfs/src/admin/router.rs b/rustfs/src/admin/router.rs index 1945acc1f..0c34ff810 100644 --- a/rustfs/src/admin/router.rs +++ b/rustfs/src/admin/router.rs @@ -34,7 +34,7 @@ use crate::admin::runtime_sources::{ }; use crate::admin::storage_api::access::{ReqInfo, authorize_request, spawn_traced}; use crate::admin::storage_api::contract::bucket::{BucketOperations, BucketOptions}; -use crate::auth::{check_key_valid, constant_time_eq, get_session_token}; +use crate::auth::{check_key_valid, constant_time_eq, get_session_token, reject_unsigned_amz_headers_on_presigned_request}; use crate::error::ApiError; use crate::license::license_check; use crate::server::{ @@ -3269,6 +3269,11 @@ where // check_access before call async fn check_access(&self, req: &mut S3Request) -> S3Result<()> { + // GHSA-g8w9-qw9q-fghr: custom routes bypass `S3Access::check`, so the + // presigned signed-header rule is enforced here as well. A request + // without a presigned signature passes through untouched. + reject_unsigned_amz_headers_on_presigned_request(&req.headers, req.uri.query())?; + if let Some(server_ctx) = &self.server_ctx { req.extensions.insert(server_ctx.clone()); if !is_public_health_path(req.uri.path()) && server_ctx.installed_app_context().is_none() { @@ -5611,6 +5616,38 @@ mod tests { assert_eq!(err.code(), &S3ErrorCode::AccessDenied); } + /// GHSA-g8w9-qw9q-fghr: custom routes must apply the presigned + /// signed-header rule too, since they never reach `S3Access::check`. + #[tokio::test] + async fn ghsa_g8w9_check_access_rejects_unsigned_amz_header_on_presigned_custom_route() { + let router: S3Router = S3Router::new(false); + let mut headers = HeaderMap::new(); + headers.insert("x-amz-tagging", HeaderValue::from_static("owner=attacker")); + let mut req = S3Request { + input: Body::from(String::new()), + method: Method::GET, + uri: "/demo-bucket?replication-metrics&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20260827T000000Z&X-Amz-Expires=900&X-Amz-SignedHeaders=host&X-Amz-Credential=test%2F20260827%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Signature=signature" + .parse() + .expect("uri should parse"), + headers, + extensions: http::Extensions::new(), + credentials: Some(s3s::auth::Credentials { + access_key: "test".into(), + secret_key: s3s::auth::SecretKey::from("secret".to_string()), + }), + region: None, + service: None, + trailing_headers: None, + }; + + let err = router + .check_access(&mut req) + .await + .expect_err("presigned custom-route request with an unsigned x-amz header must be denied"); + assert_eq!(err.code(), &S3ErrorCode::AccessDenied); + assert_eq!(err.message(), Some(crate::auth::UNSIGNED_HEADERS_MESSAGE)); + } + // backlog#1052 S2: the router hands its server's context slot to every // dispatched request via extensions, so the static admin operations can // resolve their server's store instead of the process default. diff --git a/rustfs/src/auth.rs b/rustfs/src/auth.rs index 80427f137..dbc9689aa 100644 --- a/rustfs/src/auth.rs +++ b/rustfs/src/auth.rs @@ -50,6 +50,7 @@ const EVENT_KEYSTONE_CREDENTIALS_DETECTED: &str = "keystone_credentials_detected const EVENT_KEYSTONE_CREDENTIALS_VALIDATED: &str = "keystone_credentials_validated"; const EVENT_KEYSTONE_CONTEXT_MISSING: &str = "keystone_context_missing"; const EVENT_SESSION_TOKEN_EXTRACTION: &str = "session_token_extraction"; +const EVENT_PRESIGNED_UNSIGNED_AMZ_HEADER: &str = "presigned_unsigned_amz_header"; /// RustFS-specific query capability for a single presigned PutObject request. pub(crate) const RUSTFS_MAX_CONTENT_LENGTH_QUERY: &str = "x-rustfs-max-content-length"; @@ -1064,26 +1065,39 @@ pub(crate) const UNSIGNED_HEADERS_MESSAGE: &str = "There were headers present in /// Detection keys on the query, not on the derived [`AuthType`], because the /// upstream verifier dispatches to the presigned path whenever the query /// carries `X-Amz-Signature`, even if an `Authorization` header is present too. +/// The rule relies on the verifier signing every query parameter except the +/// signature itself, so neither `X-Amz-SignedHeaders` nor a property-carrying +/// query parameter can be added after presigning. pub(crate) fn reject_unsigned_amz_headers_on_presigned_request(header: &HeaderMap, query: Option<&str>) -> S3Result<()> { let Some(query) = query else { return Ok(()); }; + // Presence detection is case-insensitive so a query the upstream verifier + // would not treat as presigned still fails closed here; the signed list is + // read with the exact key the verifier uses (`X-Amz-SignedHeaders`, unique), + // so both sides always see the same list. A duplicate or missing key + // yields an empty list, which signs nothing. let mut is_presigned_v4 = false; let mut signed_headers: Option = None; + let mut duplicate_signed_headers = false; for (name, value) in form_urlencoded::parse(query.as_bytes()) { if name.eq_ignore_ascii_case("x-amz-signature") { is_presigned_v4 = true; - } else if name.eq_ignore_ascii_case("x-amz-signedheaders") && signed_headers.is_none() { + } else if name == "X-Amz-SignedHeaders" { + if signed_headers.is_some() { + duplicate_signed_headers = true; + } signed_headers = Some(value.into_owned()); } } if !is_presigned_v4 { return Ok(()); } + if duplicate_signed_headers { + signed_headers = None; + } - // A missing or empty list signs nothing, so every `x-amz-*` header is - // unsigned; the upstream verifier rejects the malformed query anyway. let signed: Vec = signed_headers .as_deref() .unwrap_or_default() @@ -1099,11 +1113,12 @@ pub(crate) fn reject_unsigned_amz_headers_on_presigned_request(header: &HeaderMa continue; } if !signed.iter().any(|signed_name| signed_name == name) { - debug!( + warn!( + event = EVENT_PRESIGNED_UNSIGNED_AMZ_HEADER, component = LOG_COMPONENT_AUTH, subsystem = LOG_SUBSYSTEM_REQUEST, - header = name, reason = "unsigned_amz_header", + header = name, "Presigned request rejected" ); return Err(S3Error::with_message(S3ErrorCode::AccessDenied, UNSIGNED_HEADERS_MESSAGE.to_string())); @@ -2045,6 +2060,24 @@ mod tests { .code(), &S3ErrorCode::AccessDenied ); + + // Only the exact key the upstream verifier reads counts; a second + // (or differently cased) list must not widen the signed set, and a + // duplicate exact key signs nothing at all. + let widened_by_case = format!("{presigned_host_only}&x-amz-signedheaders=host%3Bx-amz-tagging"); + assert_eq!( + reject_unsigned_amz_headers_on_presigned_request(&headers, Some(&widened_by_case)) + .unwrap_err() + .code(), + &S3ErrorCode::AccessDenied + ); + let duplicated = format!("{presigned_host_only}&X-Amz-SignedHeaders=host%3Bx-amz-tagging"); + assert_eq!( + reject_unsigned_amz_headers_on_presigned_request(&headers, Some(&duplicated)) + .unwrap_err() + .code(), + &S3ErrorCode::AccessDenied + ); } #[test] diff --git a/rustfs/src/storage/access.rs b/rustfs/src/storage/access.rs index 022a6d90d..a2a95b017 100644 --- a/rustfs/src/storage/access.rs +++ b/rustfs/src/storage/access.rs @@ -1793,6 +1793,11 @@ fn validate_post_object_success_controls(input: &PostObjectInput) -> S3Result<() #[async_trait::async_trait] impl S3Access for FS { async fn check(&self, cx: &mut S3AccessContext<'_>) -> S3Result<()> { + // GHSA-g8w9-qw9q-fghr: a presigned URL only authorises the headers it + // signed. Reject unsigned `x-amz-*` headers first, before the session + // token lookup below or any handler reads a request header. + reject_unsigned_amz_headers_on_presigned_request(cx.headers(), cx.uri().query())?; + // Upper layer has verified ak/sk // info!( // "s3 check uri: {:?}, method: {:?} path: {:?}, s3_op: {:?}, cred: {:?}, headers:{:?}", @@ -1837,16 +1842,11 @@ impl S3Access for FS { ..Default::default() }; - // Publish this server's context slot so downstream data-plane handlers - // resolve the same store (backlog#1052 S6). - // GHSA-g8w9-qw9q-fghr: a presigned URL only authorises the headers it - // signed. Reject unsigned `x-amz-*` headers before any operation-level - // authorization or handler can read them. - reject_unsigned_amz_headers_on_presigned_request(cx.headers(), cx.uri().query())?; - let auth_type = get_request_auth_type_with_query(cx.headers(), cx.uri().query()); let verified_presigned = matches!(auth_type, AuthType::Presigned); let verified_sigv4 = matches!(auth_type, AuthType::Presigned | AuthType::Signed); + // Publish this server's context slot so downstream data-plane handlers + // resolve the same store (backlog#1052 S6). { let ext = cx.extensions_mut(); ext.insert(self.server_ctx().clone());