mirror of
https://github.com/rustfs/rustfs.git
synced 2026-09-08 13:06:00 +00:00
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.
This commit is contained in:
@@ -1,2 +1,2 @@
|
||||
sha256-darwin=f9bd098352b824ec81f913e85030fa791f58cf4ee822e9f67bb04d7f48174bb4
|
||||
sha256-darwin=dd14f49a7b0e2c156b4457fdd836499d837890d3e439689a4eff9e8875ee2f5b
|
||||
sha256-linux=7c892afa4b9d1591b46bd79c976b647109a277284fddb3b98edced4b0297eda2
|
||||
|
||||
@@ -1 +1 @@
|
||||
sha256=9a09b878dc29d578df615e7e78c86f08ef864de64121fddebe60a648058c2b32
|
||||
sha256=6d18f9cce820c51d5589de944e8cc185f73eeca0ea9a9916651943e3759169d0
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<dyn std::error::Error + Send + Sync>> {
|
||||
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<dyn std::error::Error + Send + Sync>> {
|
||||
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<dyn std::error::Error + Send + Sync>>
|
||||
{
|
||||
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(())
|
||||
}
|
||||
|
||||
@@ -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<Body>) -> 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<AdminOperation> = 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.
|
||||
|
||||
+38
-5
@@ -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<String> = 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<String> = 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]
|
||||
|
||||
@@ -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());
|
||||
|
||||
Reference in New Issue
Block a user