diff --git a/.agents/skills/security-advisory-lessons/SKILL.md b/.agents/skills/security-advisory-lessons/SKILL.md index 3ba00aaea..c5617a2f3 100644 --- a/.agents/skills/security-advisory-lessons/SKILL.md +++ b/.agents/skills/security-advisory-lessons/SKILL.md @@ -25,8 +25,8 @@ For the full pattern map, read [advisory-patterns.md](references/advisory-patter ## Workflow ### 1. Scope the change -- Identify touched routes, 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/rio/`, and console preview/auth code. +- 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. ### 2. Map to advisory classes - Read [advisory-patterns.md](references/advisory-patterns.md) for matching GHSA lessons. @@ -54,6 +54,7 @@ For the full pattern map, read [advisory-patterns.md](references/advisory-patter - Match the admin action to the operation exactly. Copy-paste action constants are a known RustFS vulnerability class. - Avoid authentication-only helpers for state-changing admin APIs; use `validate_admin_request` or the established equivalent with the right `AdminAction`. - Read-only admin APIs such as metrics, server info, and diagnostics still require admin authorization; checking only that credentials exist is not enough. +- Replication admin reads can expose remote target credentials; list/get target endpoints require replication/admin authorization and must not return secrets to low-privilege callers. - Do not assume admin-action `Resource` scoping constrains blast radius unless the policy engine actually enforces resources for that action. ### IAM and service accounts @@ -67,6 +68,12 @@ For the full pattern map, read [advisory-patterns.md](references/advisory-patter - Do not let `CreateMultipartUpload`, `UploadPartCopy`, `CompleteMultipartUpload`, or `AbortMultipartUpload` return success without authorization. - Presigned POST policies are server-side contracts. Enforce `content-length-range`, key prefix, exact metadata/content-type, and all signed policy conditions. +### Protocol frontends and IAM parity +- FTP/FTPS, SFTP, gateway, and other protocol drivers must enforce IAM per operation before calling storage backends; authentication to a protocol listener is not authorization. +- Match protocol commands to the same S3 actions as HTTP, such as `RETR` to `GetObject`, `SIZE`/`MDTM` to `HeadObject`, `MKD` to `CreateBucket`, and bucket probes to `ListBucket` or `HeadBucket`. +- Review every handler in a protocol driver, not only the changed handler, because RustFS advisories show mixed guarded and unguarded siblings in the same driver. +- Regression tests for protocol frontends should deny the shared authorization hook and prove the backend is not reached for the denied command. + ### Paths, object keys, and filesystem access - Never join untrusted bucket/object/RPC path strings onto filesystem roots without normalization and boundary checks. - Reject or safely handle `..`, absolute paths, URL-encoded traversal, platform separators, empty components, and paths that canonicalize outside the intended root. @@ -116,8 +123,10 @@ For the full pattern map, read [advisory-patterns.md](references/advisory-patter Use these prompts while reviewing a diff: - Could a low-privileged authenticated user reach this path with the wrong action, parent, bucket, or source object? +- Does a non-HTTP protocol path call the same authorization boundary as the S3 API before touching storage? - 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? - 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 the test prove the exploit form is denied, or only that the intended form still works? diff --git a/.agents/skills/security-advisory-lessons/references/advisory-patterns.md b/.agents/skills/security-advisory-lessons/references/advisory-patterns.md index 8e7f2550c..337bf9b4b 100644 --- a/.agents/skills/security-advisory-lessons/references/advisory-patterns.md +++ b/.agents/skills/security-advisory-lessons/references/advisory-patterns.md @@ -21,6 +21,7 @@ Update this file only when an advisory adds or changes a reusable lesson, affect - `GHSA-vcwh-pff9-64cc`: `ImportIam` checked `ExportIAMAction` for an import/write operation. Lesson: every admin handler must authorize the action it actually performs. - `GHSA-jqmc-mg33-v45g` and `GHSA-8784-9m7f-c6p6`: `/profile/cpu` and `/profile/memory` were whitelisted from auth and allowed expensive diagnostics plus path disclosure. Lesson: profiling/debug endpoints need admin auth, opt-in, rate limits, and non-sensitive responses. - `GHSA-f5cv-v44x-2xgf`: `/rustfs/admin/v3/metrics` accepted any authenticated IAM user and skipped admin authorization. Lesson: read-only metrics and diagnostic admin endpoints still require an operation-specific admin action check. +- `GHSA-796f-j7xp-hwf4`: `/rustfs/admin/v3/list-remote-targets` checked only that credentials existed and leaked replication target credentials. Lesson: replication target reads are privileged admin operations, and stored remote credentials require strict authz plus response redaction review. - `GHSA-xp32-gxq2-3v52`: console license metadata endpoint was public and exposed subject and expiration fields. Lesson: management metadata endpoints should require admin auth or return only coarse public status. ### IAM import, service accounts, and privilege boundaries @@ -35,6 +36,11 @@ Update this file only when an advisory adds or changes a reusable lesson, affect - `GHSA-wfxj-ph3v-7mjf`: `UploadPartCopy` checked source and destination independently but missed destination copy-source policy constraints. Lesson: source read and destination write checks are not sufficient when policy constrains allowed copy sources. - `GHSA-w5fh-f8xh-5x3p`: presigned POST accepted uploads without enforcing signed policy conditions. Lesson: parse and enforce all POST policy constraints server-side, including size, key prefix, and content type. +### Protocol frontends and IAM parity + +- `GHSA-3g29-xff2-92vp`: FTP `RETR` and `SIZE`/`MDTM` read paths authenticated the user but skipped IAM before calling storage. Lesson: non-HTTP protocol frontends must enforce the same per-operation authorization as the S3 API before backend access. +- `GHSA-g3vq-vv42-f647`: FTPS `MKD` called `create_bucket` without checking `s3:CreateBucket`. Lesson: protocol command handlers need action-specific checks even when sibling handlers already authorize correctly. + ### Filesystem paths and object key traversal - `GHSA-pq29-69jg-9mxc`: RPC `read_file_stream` joined untrusted paths under a volume directory without canonical boundary checks. Lesson: `PathBuf::join` plus length checks are not path security. @@ -85,6 +91,7 @@ Use these targeted searches when a diff touches security-sensitive code: ```bash rg -n "validate_admin_request|check_permissions|AdminAction::|deny_only|is_allowed" rustfs crates +rg -n "authorize_operation|FtpsDriver|SftpDriver|RETR|MKD|SIZE|MDTM|CreateBucket|GetObject|HeadObject" crates/protocols rustfs rg -n "UploadPartCopy|upload_part_copy|CompleteMultipart|PostObject|content-length-range|starts-with" rustfs crates rg -n "normalize_extract_entry_key|Snowball|auto-extract|PathBuf::join|canonicalize|\\.\\.|x-forwarded-for|x-real-ip|SourceIp" rustfs crates rg -n "DEFAULT_SECRET|DEFAULT_ACCESS|TEST_PRIVATE_KEY|rustfs rpc|RUSTFS_RPC_SECRET" rustfs crates @@ -97,6 +104,7 @@ rg -n "deny_unknown_fields|serde.default|as u32|as usize|as i32" rustfs crates ## Minimum Regression Test Expectations - Authz fixes: include unauthenticated, valid low-privilege, wrong-action, correct-action, owner, non-owner, and root/admin cases as applicable. +- Protocol frontend authz fixes: include denied `RETR`, `SIZE`/`MDTM`, `MKD`, bucket probe, and sibling allowed-operation cases, and assert denied paths do not reach the storage backend. - IAM fixes: include import/update/list service-account cases with attacker-controlled parent, claims, access key, secret key, and policy. - Copy/upload fixes: include cross-bucket, cross-user, source-denied, destination-denied, copy-source-condition, and multipart completion cases. - Path fixes: include encoded traversal, absolute path, nested traversal, archive entries with `..`, valid object keys that resemble traversal text but should be rejected, and canonical bucket/prefix boundary checks.