From 53c5cbed6e88ab21c3d659f7bbc96e6f98309f07 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sun, 12 Jul 2026 22:19:16 +0800 Subject: [PATCH] docs: update security advisory lessons (#4780) --- .agents/skills/security-advisory-lessons/SKILL.md | 13 +++++++++++-- .../references/advisory-patterns.md | 10 +++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/.agents/skills/security-advisory-lessons/SKILL.md b/.agents/skills/security-advisory-lessons/SKILL.md index d6fcba217..639b67bce 100644 --- a/.agents/skills/security-advisory-lessons/SKILL.md +++ b/.agents/skills/security-advisory-lessons/SKILL.md @@ -26,7 +26,7 @@ For the full pattern map, read [advisory-patterns.md](references/advisory-patter ### 1. Scope the change - Identify touched routes, protocol frontends, handlers, storage paths, credentials, logs, browser surfaces, CI/release code, and policy checks. -- Treat these paths as security-sensitive by default: `rustfs/src/admin/`, `rustfs/src/storage/`, `rustfs/src/auth.rs`, `rustfs/src/server/layer.rs`, `crates/iam/`, `crates/policy/`, `crates/credentials/`, `crates/ecstore/src/rpc/`, `crates/protocols/`, `crates/rio/`, and console preview/auth code. +- Treat these paths as security-sensitive by default: `rustfs/src/admin/`, `rustfs/src/storage/`, `rustfs/src/auth.rs`, `rustfs/src/server/layer.rs`, `crates/iam/`, `crates/policy/`, `crates/credentials/`, `crates/ecstore/src/rpc/`, `crates/protocols/`, `crates/rio/`, OIDC/STS federation code, and console preview/auth code. ### 2. Map to advisory classes - Read [advisory-patterns.md](references/advisory-patterns.md) for matching GHSA lessons. @@ -59,10 +59,16 @@ For the full pattern map, read [advisory-patterns.md](references/advisory-patter ### IAM and service accounts - Treat imported IAM payload fields as attacker-controlled: `parent`, `claims`, `accessKey`, `secretKey`, status, policy names, and groups. -- For service account create/update/import, prove parent ownership or root/admin authority before writing credentials or claims. +- For service account create/update/import, prove parent ownership or root/admin authority before writing credentials or claims; an action permission alone must not allow choosing root or another user as `target_user`. - Do not let `deny_only` or "no explicit deny" become an allow decision that skips required allow checks. - Test cross-user list/update/import flows with wrong, correct, self, parent, and root identities. +### STS, OIDC, and federation flows +- Every STS endpoint must have an explicit authentication story: SigV4 where required, OIDC token verification for web identity, and role/session policy validation before issuing credentials. +- JWT session tokens must be signed and verified by a trusted issuer/key path, not by service-account-controlled material or a reused root secret. +- Public OIDC bootstrap and callback routes must treat `Host`, `X-Forwarded-Proto`, redirect targets, `state`, and callback parameters as untrusted; credential-bearing redirects require a configured, allowlisted origin. +- OIDC discovery and validation URLs are SSRF sinks. Resolve and classify hostnames at connection time, reject rebinding to loopback/private/link-local ranges, and do not rely on literal string checks. + ### S3 copy, multipart, and presigned POST - Multipart copy must enforce source `GetObject` and destination `PutObject` semantics equivalent to `CopyObject`, including copy-source and policy conditions. - Do not let `CreateMultipartUpload`, `UploadPartCopy`, `CompleteMultipartUpload`, or `AbortMultipartUpload` return success without authorization. @@ -113,6 +119,7 @@ For the full pattern map, read [advisory-patterns.md](references/advisory-patter ### Trusted proxy and network identity - Only honor `X-Forwarded-For` or `X-Real-IP` when the request came from a configured trusted proxy. +- Apply the same trusted-proxy rule to scheme and host derivation; direct clients must not control security-sensitive redirects through `Host`, `X-Forwarded-Host`, or `X-Forwarded-Proto`. - Direct clients must use the socket peer address for `aws:SourceIp` and policy condition evaluation. - Add tests for direct spoofed headers and trusted-proxy headers. @@ -130,6 +137,8 @@ Use these prompts while reviewing a diff: - Does a public/default/empty config change security behavior from fail-closed to fail-open? - Is any attacker-controlled value later used as a path, policy condition, credential identity, log field, URL, Origin, or response body? - Does this response contain stored replication, remote target, or service credentials that need redaction or stricter authorization? +- Can this STS/OIDC path issue credentials without SigV4, trusted issuer validation, allowlisted redirects, or trusted-proxy host/scheme handling? +- Does this outbound validation path resolve attacker-supplied hostnames and reject private, loopback, link-local, and rebound addresses at the actual connection boundary? - Is an archive entry, object key, or policy resource normalized differently between authorization and storage? - Is the same operation implemented in multiple paths, such as `CopyObject` vs `UploadPartCopy`, and do all paths enforce the same security contract? - Does a preview or browser-surface fix preserve the original security invariant when adding alternate viewers or file-type detection? diff --git a/.agents/skills/security-advisory-lessons/references/advisory-patterns.md b/.agents/skills/security-advisory-lessons/references/advisory-patterns.md index 93efcddf8..f99f6d701 100644 --- a/.agents/skills/security-advisory-lessons/references/advisory-patterns.md +++ b/.agents/skills/security-advisory-lessons/references/advisory-patterns.md @@ -27,9 +27,17 @@ Update this file only when an advisory adds or changes a reusable lesson, affect ### IAM import, service accounts, and privilege boundaries - `GHSA-566f-q62r-wcr8`: `ImportIam` accepted attacker-controlled service account `parent`, `claims`, `accessKey`, and `secretKey`, enabling persistent backdoor accounts under root. Lesson: imported IAM payloads are untrusted data and must be validated against privilege boundaries. +- `GHSA-5354-r3w2-34m8`: `AddServiceAccount` checked `CreateServiceAccountAdminAction` but trusted caller-supplied `target_user`, allowing service accounts under the root parent. Lesson: service-account create paths must validate parent ownership or root/admin authority, not only the create action. - `GHSA-xgr5-qc6w-vcg9`: `deny_only=true` skipped allow checks and let restricted service accounts mint unrestricted children. Lesson: deny-only logic must never become implicit allow for privilege creation. - `GHSA-mm2q-qcmx-gw4w`: leaked service account access keys plus update-without-ownership formed an escalation chain. Lesson: service-account identifiers are security-sensitive because update APIs consume them. +### STS, OIDC, and federation flows + +- `GHSA-5qfg-mf7r-jp3w`: `AssumeRoleWithWebIdentity` was reachable without the required request authentication and could issue temporary credentials from crafted web identity input. Lesson: every STS route needs explicit SigV4 or trusted identity-provider validation before role assumption. +- `GHSA-ccrv-v8v9-ch9q`: service-account-controlled material could self-sign JWT session tokens with forged policy claims. Lesson: session tokens must be signed by a trusted issuer/key path and validation must reject self-signed or principal-controlled tokens. +- `GHSA-9pjf-w3c2-m32r`, `GHSA-4x2q-cpx9-9h26`, and `GHSA-xvpm-p3f7-34c3`: public OIDC authorize/callback flows trusted request `Host` or forwarded scheme when building credential-bearing redirects. Lesson: OIDC redirects must use configured allowlisted origins and trusted-proxy handling; never derive the post-login credential destination from direct client headers. +- `GHSA-m479-9x88-94w6`, `GHSA-frwq-mfqx-83p8`, `GHSA-q9q8-rf9r-fg9f`, and `GHSA-j5c2-hhf7-6gf5`: OIDC validation accepted attacker-controlled discovery URLs because hostname checks rejected only literal forbidden IPs, allowing DNS rebinding SSRF. Lesson: outbound federation URL validation must resolve and classify hostnames at the connection boundary and reject loopback, private, link-local, and rebound addresses. + ### S3 copy, multipart, and upload policy validation - `GHSA-mx42-j6wv-px98`: `UploadPartCopy` missed source authorization and allowed cross-bucket object exfiltration. Lesson: multipart copy must enforce the same source and destination contract as `CopyObject`. @@ -50,7 +58,7 @@ Update this file only when an advisory adds or changes a reusable lesson, affect ### Secrets, defaults, and cryptographic misuse -- `GHSA-j59h-h7q5-q348`: RustFS shipped known default root credentials that could authenticate to S3, admin APIs, IAM, KMS, and console surfaces. Lesson: root credentials must be operator-provided or generated per install; known defaults and warnings are not acceptable for network-reachable deployments. +- `GHSA-j59h-h7q5-q348`, `GHSA-3wm5-wpm5-hmfm`, `GHSA-6wc8-xm48-qhmx`, and `GHSA-9gf3-jx4p-4xxf`: RustFS shipped known default root credentials that could authenticate to S3, admin APIs, IAM, KMS, console, and token-signing surfaces. Lesson: root credentials must be operator-provided or generated per install; known defaults and warnings are not acceptable for network-reachable deployments. - `GHSA-h956-rh7x-ppgj`: gRPC used the hard-coded token `rustfs rpc` on both client and server. Lesson: source-visible shared tokens are authentication bypasses. - `GHSA-r5qv-rc46-hv8q`: internode RPC HMAC secret fell back to the public default `rustfsadmin`. Lesson: RPC/internode auth must fail closed instead of silently using public defaults. - `GHSA-75fx-qg6f-8rm7` and `GHSA-68cw-96m3-h2cf`: internode RPC secrets were derivable from known root credentials, making raw storage RPC signatures forgeable when explicit RPC secrets were unset. Lesson: RPC auth keys must be independent random secrets, never derived from S3 root credentials, and raw storage RPC should not share the public S3 listener without an internode-only boundary.