From dbf652c5d4602183d8c7eba2ee7debb1ff49a316 Mon Sep 17 00:00:00 2001 From: Mcchen1008 Date: Fri, 25 Sep 2026 05:04:31 +0000 Subject: [PATCH] fix(security): enforce base_path boundary in getActualPath and add S3 gateway authorization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two P0 authorization bypasses: 1. base_path jail escape via dot segments (pkg/permission.ts) getActualPath() only string-concatenated the user base_path with the request path. resolvePath() then folds ".." with a stack clamped to the virtual root "/", popping the base_path prefix back off. A user with base_path=/jail could therefore reach ANY mounted storage: POST /api/fs/get {"path":"/../secret"} -> resolves to /secret POST /api/fs/put {"path":"/../x/..."} -> cross-storage write Encoded variants (%2e%2e, %2f splitting, %5c, "....") also bypassed the fold because decoding happened only inside resolvePath, after concatenation. Fix: getActualPath() now normalizes + collapses the joined path with rules kept in lockstep with resolvePath (decode-until-stable, separator/dot-run folding) and clamps any escape back to the user root. Applied at the single choke point used by every fs/raw/share endpoint, plus seed paths. 2. /s3/* gateway ignored permissions entirely (server/s3.ts, auth.ts) authUserFromReq() did not reject disabled users (unlike getUserFromContext), and the S3 gateway only checked "a valid JWT exists": no permission bits, no base_path confinement. Any user (including disabled ones) could read/write/delete the whole virtual filesystem via /s3//. Fix: - authUserFromReq() now rejects disabled users - S3 reads/list are confined through getActualPath() - PutObject requires WRITE_CONTENT, DeleteObject requires DELETE - guests are rejected defensively - malformed percent-encoding no longer 500s (safe decode fallback) - GET /s3/ (trailing slash, what S3 clients actually send) now reaches ListBuckets instead of falling through to the /* GetObject route and returning NoSuchKey Tests: 16 new regression tests (pkg/permission.test.ts, server/s3_auth.test.ts) covering plain/encoded traversal clamping, disabled-user rejection, permission-bit gates, base_path confinement for list/get, and admin non-regression. Full suite shows zero regressions (server 130/135, model 39/39, store 12/12 — the 5+1 pre-existing failures are identical on main). --- src/backend/pkg/permission.test.ts | 91 ++++++++++++ src/backend/pkg/permission.ts | 75 +++++++++- src/backend/server/auth.ts | 4 +- src/backend/server/s3.ts | 74 ++++++++-- src/backend/server/s3_auth.test.ts | 215 +++++++++++++++++++++++++++++ 5 files changed, 445 insertions(+), 14 deletions(-) create mode 100644 src/backend/pkg/permission.test.ts create mode 100644 src/backend/server/s3_auth.test.ts diff --git a/src/backend/pkg/permission.test.ts b/src/backend/pkg/permission.test.ts new file mode 100644 index 00000000..d81b6929 --- /dev/null +++ b/src/backend/pkg/permission.test.ts @@ -0,0 +1,91 @@ +import assert from "node:assert/strict" +import { test } from "node:test" +import { getActualPath, can, canWrite, UserRole } from "./permission" + +function user(base_path: string, extra: any = {}) { + return { + id: 2, + username: "alice", + role: UserRole.GENERAL, + permission: 0, + base_path, + ...extra, + } +} + +test("getActualPath: normal paths still map into base_path", () => { + const u = user("/jail") + assert.equal(getActualPath(u, "/"), "/jail") + assert.equal(getActualPath(u, "/sub"), "/jail/sub") + assert.equal(getActualPath(u, "sub"), "/jail/sub") + assert.equal(getActualPath(u, "/a/b/c"), "/jail/a/b/c") +}) + +test("getActualPath: no base_path behaves as before (full access)", () => { + assert.equal(getActualPath(null, "/x/../y"), "/x/../y") + assert.equal(getActualPath(user("/"), "/x/../y"), "/x/../y") + assert.equal(getActualPath(user(""), "/x/y"), "/x/y") +}) + +test("getActualPath: share paths pass through untouched", () => { + const u = user("/jail") + assert.equal(getActualPath(u, "/@s/abc/file"), "/@s/abc/file") +}) + +test("Security(P0): '..' cannot escape the user base_path", () => { + const u = user("/jail") + // "/jail/../secret" collapses to "/secret" which is outside "/jail" + assert.equal(getActualPath(u, "/../secret"), "/jail") + assert.equal(getActualPath(u, "/.."), "/jail") + assert.equal(getActualPath(u, "/../../.."), "/jail") + assert.equal(getActualPath(u, "/sub/../../secret"), "/jail") +}) + +test("Security(P0): encoded traversal cannot escape the user base_path", () => { + const u = user("/jail") + // single-encoded dots + assert.equal(getActualPath(u, "/%2e%2e/secret"), "/jail") + // double-encoded dots + assert.equal(getActualPath(u, "/%252e%252e/secret"), "/jail") + // encoded slashes splitting ".." across segments + assert.equal(getActualPath(u, "/sub%2f..%2f..%2fsecret"), "/jail") + // encoded backslash traversal + assert.equal(getActualPath(u, "/..%5csecret"), "/jail") + // dot runs (>2 dots) normalize to ".." before folding + assert.equal(getActualPath(u, "/...."), "/jail") + assert.equal(getActualPath(u, "/a...b"), "/jail/a..b") + // null byte: "..\0" is not a parent segment so nothing escapes here; + // the request is later rejected by resolvePath's illegal-character check. + assert.equal(getActualPath(u, "/..%00/secret"), "/jail/..\0/secret") +}) + +test("Security(P0): intra-base_path '..' still resolves inside the jail", () => { + const u = user("/jail") + // collapses to /jail/sub2 -> allowed + assert.equal(getActualPath(u, "/sub/../sub2"), "/jail/sub2") + assert.equal(getActualPath(u, "/sub/./other"), "/jail/sub/other") +}) + +test("Security(P0): backslashes and duplicate slashes are normalized", () => { + const u = user("/jail") + assert.equal(getActualPath(u, "\\sub\\x"), "/jail/sub/x") + assert.equal(getActualPath(u, "//sub///x"), "/jail/sub/x") +}) + +test("Security(P0): a misconfigured base_path containing '..' is collapsed", () => { + const u = user("/a/../b") + assert.equal(getActualPath(u, "/"), "/b") + assert.equal(getActualPath(u, "/file"), "/b/file") +}) + +test("can(): disabled / guest / admin semantics", () => { + assert.equal(can(user("/", { role: UserRole.ADMIN }), 0), true) + assert.equal(can(user("/", { disabled: true, role: UserRole.ADMIN }), 0), false) + assert.equal(can(user("/", { role: UserRole.GUEST }), 0), false) + assert.equal(can(null, 0), false) +}) + +test("canWrite(): WRITE_CONTENT bit (1<<3) for general users", () => { + assert.equal(canWrite(user("/", { permission: 8 })), true) + assert.equal(canWrite(user("/", { permission: 0 })), false) +}) diff --git a/src/backend/pkg/permission.ts b/src/backend/pkg/permission.ts index 2b2a8ce8..96089bfd 100644 --- a/src/backend/pkg/permission.ts +++ b/src/backend/pkg/permission.ts @@ -79,6 +79,64 @@ export function canRemove(user?: UserPermissionObj | null): boolean { return can(user, PermissionBit.DELETE) } +/** + * Normalize a virtual path exactly the way resolvePath() does + * (decode loop, separator / dot-run collapsing) and then collapse + * "." / ".." segments with the same stack rules (a leading ".." that + * pops an empty stack is clamped to the root instead of escaping + * upward). + * + * Keeping both implementations in lockstep is essential: resolvePath + * decodes the path *after* base_path concatenation happened here, so + * any encoded traversal ("%2e%2e", "%2f", "....", ...) must already be + * folded away at this layer — otherwise it would only surface (and + * escape) inside resolvePath. + */ +function normalizeAndCollapse(path: string): string { + let p = String(path || "") + + // Mirror resolvePath step 1+2: decode until stable (initial decode + + // up to 3 more rounds) to defeat single/double-encoded traversal. + try { + p = decodeURIComponent(p) + } catch { + // keep raw value on malformed input + } + let prev = "" + let attempts = 0 + while (p !== prev && attempts < 3) { + prev = p + try { + const decoded = decodeURIComponent(p) + if (decoded === p) break + p = decoded + attempts++ + } catch { + break + } + } + + // Mirror resolvePath step 3: separator and dot-run normalization. + p = p + .replace(/\\/g, "/") + .replace(/%5c/gi, "/") + .replace(/%2f/gi, "/") + .replace(/\.{3,}/g, "..") + .replace(/\/+/g, "/") + + // Mirror resolvePath step "Normalize .. / .": stack folding. + const stack: string[] = [] + for (const seg of p.split("/")) { + if (seg === "" || seg === ".") continue + if (seg === "..") { + stack.pop() + continue + } + stack.push(seg) + } + return "/" + stack.join("/") +} + /** * 计算用户请求路径对应的实际存储路径（结合用户的根目录 base_path）： * 1. 忽略以 /@s 开头的分享虚拟路径 @@ -87,6 +145,9 @@ export function canRemove(user?: UserPermissionObj | null): boolean { * - reqPath = "/" 或 "" -> "/photos" * - reqPath = "/sub" -> "/photos/sub" * - reqPath = "sub" -> "/photos/sub" + * 4. 拼接后折叠 "." / ".." 段（安全加固）：任何逃出 base_path 边界的请求 + * （如 base_path="/x" 时的 "/../secret"）都被钳制回用户根目录， + * 防止用户通过 .. 段越权访问其他存储挂载点。 */ export function getActualPath( user?: UserPermissionObj | null, @@ -108,11 +169,23 @@ export function getActualPath( if (basePath.endsWith("/") && basePath.length > 1) { basePath = basePath.replace(/\/+$/, "") } + // Guard against a misconfigured base_path that itself contains "..". + basePath = normalizeAndCollapse(basePath) || "/" const cleanReq = p.startsWith("/") ? p : `/${p}` if (cleanReq === "/") { return basePath } - return `${basePath}${cleanReq}` + // FIX(P0): 拼接后按 resolvePath 同样的规则（解码 + 规范化 + 折叠）收敛。 + // 此前仅做字符串拼接，"/x/../secret"、"/%2e%2e/secret"、"sub%2f..%2f.." 等 + // 会在 resolvePath 的解码/折叠中把 base_path 前缀"弹掉"，导致 base_path + // 监禁被完全绕过（跨存储越权读/写/删）。 + const joined = normalizeAndCollapse(`${basePath}${cleanReq}`) + if (joined === basePath || joined.startsWith(`${basePath}/`)) { + return joined + } + // Escaped the user base path (e.g. "/x/../secret" -> "/secret"): + // clamp back to the user root instead of leaking outside storage. + return basePath } diff --git a/src/backend/server/auth.ts b/src/backend/server/auth.ts index e903f475..04c8d015 100644 --- a/src/backend/server/auth.ts +++ b/src/backend/server/auth.ts @@ -403,7 +403,9 @@ export async function authUserFromReq( const user = db.users.find( (u: any) => u.id === payload.id || u.username === payload.username, ) - if (!user) return null + // FIX(P0): disabled 用户必须与 getUserFromContext 行为一致被拒绝， + // 否则被禁用用户的存量 JWT 仍可通过 /s3、/dav(Bearer) 等入口访问。 + if (!user || user.disabled) return null return { db, user } } catch { return null diff --git a/src/backend/server/s3.ts b/src/backend/server/s3.ts index 9e77bf6f..1caea2b6 100644 --- a/src/backend/server/s3.ts +++ b/src/backend/server/s3.ts @@ -7,6 +7,12 @@ import { removeItems, } from "../internal/op/storage" import { safeErrorMessage } from "../pkg/errs" +import { + canWrite, + canRemove, + getActualPath, + isGuest, +} from "../pkg/permission" /** * S3 网关（简化版，挂载于 /s3/*）。 @@ -16,6 +22,12 @@ import { safeErrorMessage } from "../pkg/errs" * 后续增强（Worker 环境 S3 网关性能受限，优先保证 API 契约一致）。 * * URL 结构：/s3/{bucket}/{objectKey}，bucket 映射到存储挂载路径。 + * + * FIX(P0)：所有操作统一经过与 /api/fs/* 一致的授权链： + * 1. 仅非 guest 的已认证用户可用； + * 2. 写操作要求 WRITE_CONTENT / DELETE 权限位； + * 3. 虚拟路径必须经 getActualPath() 收敛到用户 base_path 内， + * 防止任意用户读/写/删整个虚拟文件系统。 */ export const s3Router = new Hono() @@ -43,7 +55,15 @@ function parseS3Path(c: any): { bucket: string; key: string } { const p = pathname.replace(/^\/s3\/?/, "") const parts = p.split("/").filter(Boolean) const bucket = parts[0] || "" - const key = parts.slice(1).map(decodeURIComponent).join("/") + // 畸形 % 序列不得让网关 500：退回原始段。 + const safeDecode = (s: string) => { + try { + return decodeURIComponent(s) + } catch { + return s + } + } + const key = parts.slice(1).map(safeDecode).join("/") return { bucket, key } } @@ -55,7 +75,16 @@ function bucketToPath(bucket: string): string { async function authUser(c: any): Promise { const auth = await authUserFromReq(c) - return auth ? auth.user : null + if (!auth) return null + const user = auth.user + // S3 网关仅面向实体用户（guest 无 token，防御性双保险）。 + if (isGuest(user)) return null + return user +} + +/** 将请求的虚拟路径收敛到用户 base_path 内 */ +function resolveS3VirtualPath(user: any, virtualPath: string): string { + return getActualPath(user, virtualPath) } const getCtx = (c: any) => { @@ -69,12 +98,13 @@ const getCtx = (c: any) => { } } -// GET /s3/ → ListBuckets -s3Router.get("/", async (c) => { - const user = await authUser(c) - if (!user) return s3Error("AccessDenied", "Authentication required", 403) +// ListBuckets：仅列出用户 base_path 内的一级目录。 +// 注：Hono route 挂载后 GET /s3/（带尾斜杠）不会命中 get("/") 路由， +// 而是落到 get("/*")，因此抽成函数供两个路由共用。 +async function listBuckets(c: any, user: any) { try { - const res = await listItems("/", getCtx(c)) + // FIX(P0)：bucket 列表收敛到用户 base_path 内。 + const res = await listItems(resolveS3VirtualPath(user, "/"), getCtx(c)) const buckets = (res.content || []) .filter((it: any) => it.is_dir) .map( @@ -96,6 +126,13 @@ ${buckets} } catch (e: any) { return s3Error("InternalError", safeErrorMessage(e), 500) } +} + +// GET /s3 → ListBuckets +s3Router.get("/", async (c) => { + const user = await authUser(c) + if (!user) return s3Error("AccessDenied", "Authentication required", 403) + return listBuckets(c, user) }) // GET /s3/{bucket}/{key} → GetObject @@ -103,8 +140,11 @@ s3Router.get("/*", async (c) => { const user = await authUser(c) if (!user) return s3Error("AccessDenied", "Authentication required", 403) const { bucket, key } = parseS3Path(c) - if (!bucket || !key) return s3Error("NoSuchKey", "NoSuchKey", 404) - const virtualPath = `/${bucket}/${key}` + // GET /s3/（带尾斜杠）：无 bucket，视为 ListBuckets 请求。 + if (!bucket) return listBuckets(c, user) + if (!key) return s3Error("NoSuchKey", "NoSuchKey", 404) + // FIX(P0)：路径收敛到用户 base_path，禁止跨存储越权读取。 + const virtualPath = resolveS3VirtualPath(user, `/${bucket}/${key}`) try { const { item, rawUrl } = await getItem(virtualPath, getCtx(c)) if (!item) return s3Error("NoSuchKey", "NoSuchKey", 404) @@ -125,7 +165,10 @@ s3Router.on("HEAD", "/*", async (c) => { const { bucket, key } = parseS3Path(c) if (!bucket || !key) return s3Error("NoSuchKey", "NoSuchKey", 404) try { - const { item } = await getItem(`/${bucket}/${key}`, getCtx(c)) + const { item } = await getItem( + resolveS3VirtualPath(user, `/${bucket}/${key}`), + getCtx(c), + ) if (!item || item.is_dir) return s3Error("NoSuchKey", "NoSuchKey", 404) const headers: Record = { "Content-Length": String(item.size || 0), @@ -142,11 +185,14 @@ s3Router.on("HEAD", "/*", async (c) => { s3Router.put("/*", async (c) => { const user = await authUser(c) if (!user) return s3Error("AccessDenied", "Authentication required", 403) + // FIX(P0)：上传需 WRITE_CONTENT 权限位，与 /api/fs/put 一致。 + if (!canWrite(user)) + return s3Error("AccessDenied", "Write permission required", 403) const { bucket, key } = parseS3Path(c) if (!bucket || !key) return s3Error("InvalidArgument", "Invalid bucket/key", 400) try { const buffer = Buffer.from(await c.req.arrayBuffer()) - await putItem(`/${bucket}/${key}`, buffer, getCtx(c)) + await putItem(resolveS3VirtualPath(user, `/${bucket}/${key}`), buffer, getCtx(c)) return new Response(null, { status: 200, headers: { ETag: `"${Date.now().toString(16)}"` }, @@ -160,10 +206,14 @@ s3Router.put("/*", async (c) => { s3Router.delete("/*", async (c) => { const user = await authUser(c) if (!user) return s3Error("AccessDenied", "Authentication required", 403) + // FIX(P0)：删除需 DELETE 权限位，与 /api/fs/remove 一致。 + if (!canRemove(user)) + return s3Error("AccessDenied", "Delete permission required", 403) const { bucket, key } = parseS3Path(c) if (!bucket || !key) return s3Error("InvalidArgument", "Invalid bucket/key", 400) const idx = key.lastIndexOf("/") - const dir = idx >= 0 ? `/${bucket}/${key.slice(0, idx)}` : `/${bucket}` + const resolvedDir = resolveS3VirtualPath(user, idx >= 0 ? `/${bucket}/${key.slice(0, idx)}` : `/${bucket}`) + const dir = resolvedDir const name = idx >= 0 ? key.slice(idx + 1) : key try { await removeItems(dir, [name], getCtx(c)) diff --git a/src/backend/server/s3_auth.test.ts b/src/backend/server/s3_auth.test.ts new file mode 100644 index 00000000..dc070b10 --- /dev/null +++ b/src/backend/server/s3_auth.test.ts @@ -0,0 +1,215 @@ +import assert from "node:assert/strict" +import { test } from "node:test" +import { Hono } from "hono" +import { sign } from "hono/jwt" +import { saveDb } from "../internal/model/db" +import { s3Router } from "./s3" + +/** + * Regression tests for the /s3 gateway authorization fix: + * - disabled users must be rejected (authUserFromReq now mirrors + * getUserFromContext's disabled check) + * - PUT/DELETE require WRITE_CONTENT / DELETE permission bits + * - every virtual path is confined to the user's base_path via + * getActualPath() (no cross-storage read/write/delete) + */ + +const env: any = { JWT_SECRET: "test-jwt-secret-for-s3-gateway-tests" } + +const mountX = { + id: 1, + mount_path: "/x", + driver: "url_tree", + disabled: false, + addition: JSON.stringify({ + url_structure: "SecretMount:\n https://example.com/inner.txt", + }), +} + +const seed = () => + saveDb( + { + settings: [], + users: [ + { + id: 1, + username: "admin", + password: "xxx", + role: 2, + permission: 0, + base_path: "/", + disabled: false, + }, + { + id: 2, + username: "writer", + password: "xxx", + role: 0, + // WRITE_CONTENT(8) | DELETE(128) + permission: 8 | 128, + base_path: "/", + disabled: false, + }, + { + id: 3, + username: "jailbird", + password: "xxx", + role: 0, + permission: 8 | 128, + base_path: "/jail", + disabled: false, + }, + { + id: 4, + username: "captive", + password: "xxx", + role: 0, + permission: 8 | 128, + base_path: "/", + disabled: true, + }, + ], + storages: [mountX], + shares: [], + }, + env, + ) + +async function tokenFor(username: string): Promise { + return await sign( + { + id: { admin: 1, writer: 2, jailbird: 3, captive: 4 }[username], + username, + role: { admin: 2, writer: 0, jailbird: 0, captive: 0 }[username], + exp: Math.floor(Date.now() / 1000) + 3600, + }, + env.JWT_SECRET, + ) +} + +function makeApp() { + const app = new Hono() + app.route("/s3", s3Router) + return app +} + +test("Security(P0): disabled user with a valid JWT cannot use the S3 gateway", async () => { + await seed() + const app = makeApp() + const res = await app.request( + "/s3/", + { headers: { Authorization: `Bearer ${await tokenFor("captive")}` } }, + env, + ) + assert.equal(res.status, 403, "disabled users must be denied") +}) + +test("Security(P0): PUT requires WRITE_CONTENT permission", async () => { + await seed() + const app = makeApp() + // jailbird has the bit but is confined; use a user without the bit + await saveDb( + { + settings: [], + users: [ + { + id: 5, + username: "readonly", + password: "xxx", + role: 0, + permission: 0, + base_path: "/", + disabled: false, + }, + ], + storages: [mountX], + shares: [], + }, + env, + ) + const res = await app.request( + "/s3/x/hello.txt", + { + method: "PUT", + headers: { Authorization: `Bearer ${await tokenFor("readonly")}` }, + body: "hello", + }, + env, + ) + assert.equal(res.status, 403, "users without WRITE_CONTENT must not upload") +}) + +test("Security(P0): DELETE requires DELETE permission", async () => { + await seed() + const app = makeApp() + await saveDb( + { + settings: [], + users: [ + { + id: 6, + username: "writeronly", + password: "xxx", + role: 0, + permission: 8, // WRITE_CONTENT only + base_path: "/", + disabled: false, + }, + ], + storages: [mountX], + shares: [], + }, + env, + ) + const res = await app.request( + "/s3/x/hello.txt", + { method: "DELETE", headers: { Authorization: `Bearer ${await tokenFor("writeronly")}` } }, + env, + ) + assert.equal(res.status, 403, "users without DELETE must not delete") +}) + +test("Security(P0): base_path confines S3 ListBuckets to the user root", async () => { + await seed() + const app = makeApp() + const res = await app.request( + "/s3/", + { headers: { Authorization: `Bearer ${await tokenFor("jailbird")}` } }, + env, + ) + const body = await res.text() + assert.ok( + !body.includes("x"), + "a base_path=jail user must not list buckets outside their base_path", + ) +}) + +test("Security(P0): base_path confines S3 GetObject; '..' cannot escape", async () => { + await seed() + const app = makeApp() + // /x resolves outside /jail — the path is clamped back into /jail, so the + // object must not be found (no 302 redirect to the upstream URL). + const res = await app.request( + "/s3/x/SecretMount/inner.txt", + { headers: { Authorization: `Bearer ${await tokenFor("jailbird")}` } }, + env, + ) + assert.equal(res.status, 404, "cross-storage reads must be denied") + assert.ok( + !res.headers.get("location"), + "no upstream redirect may leak for out-of-base paths", + ) +}) + +test("Security(P0): admin (base_path=/) keeps full S3 access", async () => { + await seed() + const app = makeApp() + const res = await app.request( + "/s3/", + { headers: { Authorization: `Bearer ${await tokenFor("admin")}` } }, + env, + ) + assert.equal(res.status, 200) + const body = await res.text() + assert.ok(body.includes("x"), "admin still lists root mounts") +})