diff --git a/extensions/thumbnails.test.ts b/extensions/thumbnails.test.ts index 3ec6fd998..0a9b5b62d 100644 --- a/extensions/thumbnails.test.ts +++ b/extensions/thumbnails.test.ts @@ -3,6 +3,7 @@ import { PutObjectCommand, type S3Client, } from '@aws-sdk/client-s3'; +import crypto from 'node:crypto'; import { afterAll, beforeAll, @@ -26,6 +27,9 @@ const TINY_PNG_BASE64 = const BUCKET = 'puter-local'; +// Keys the extension will accept back: its own `thumbnails/` namespace. +const mintedKey = () => `thumbnails/${crypto.randomUUID()}`; + const streamToBuffer = async ( body: { transformToByteArray: () => Promise } | undefined, ): Promise => { @@ -195,7 +199,7 @@ describe('thumbnails extension — handleThumbnailRead', () => { // Seed an object so the presigned URL points at something real // (the signer itself doesn't validate existence, but this keeps // the test honest). - const key = 'thumb-read-test'; + const key = mintedKey(); await s3.send( new PutObjectCommand({ Bucket: BUCKET, @@ -220,6 +224,57 @@ describe('thumbnails extension — handleThumbnailRead', () => { expect((entry.thumbnail as string).startsWith('http')).toBe(true); }); + // `fsentries.thumbnail` is writable through the FS API, so a stored + // pointer is attacker input. Signing one the extension didn't mint would + // hand out a presigned read of an arbitrary object — including another + // user's file, whose key is its fsentry uuid in this same bucket. + it.each([ + // Shaped exactly like an fs object key (and like a legacy thumbnail + // row) — indistinguishable from a planted pointer, so it fails closed. + [ + "another user's file object", + `s3://${BUCKET}/${crypto.randomUUID()}`, + ], + [ + 'a key outside the thumbnails namespace', + `s3://${BUCKET}/secrets/dump`, + ], + [ + 'a namespace-lookalike key', + `s3://${BUCKET}/thumbnails/../${crypto.randomUUID()}`, + ], + ['a non-uuid inside the namespace', `s3://${BUCKET}/thumbnails/etc`], + ])('refuses to presign a pointer naming %s', async (_label, thumbnail) => { + const entry: Record = { thumbnail }; + await handleThumbnailRead(entry, { + s3, + s3Presign, + bucketName: BUCKET, + bucketEndpoint: 'http://127.0.0.1:4566/puter-local/', + db: stubDb, + }); + expect(entry.thumbnail).toBeNull(); + }); + + it('signs against its own bucket, ignoring the one in the pointer', async () => { + const key = mintedKey(); + const entry: Record = { + thumbnail: `s3://attacker-named-bucket/${key}`, + }; + await handleThumbnailRead(entry, { + s3, + s3Presign, + bucketName: BUCKET, + bucketEndpoint: 'http://127.0.0.1:4566/puter-local/', + db: stubDb, + }); + // Signed for OUR bucket; `attacker-named-bucket` never reached S3. + const signed = entry.thumbnail as string; + expect(signed.startsWith('http')).toBe(true); + expect(signed).not.toContain('attacker-named-bucket'); + expect(signed).toContain(BUCKET); + }); + it('leaves the thumbnail untouched when not s3/https/data', async () => { const entry: Record = { thumbnail: 'about:blank' }; await handleThumbnailRead(entry, { @@ -285,7 +340,7 @@ describe('thumbnails extension — handleFsRemoveNodeThumbnail', () => { }); it('deletes the S3 object referenced by an s3:// thumbnail URL', async () => { - const key = 'thumb-remove-test'; + const key = mintedKey(); await s3.send( new PutObjectCommand({ Bucket: BUCKET, @@ -297,7 +352,7 @@ describe('thumbnails extension — handleFsRemoveNodeThumbnail', () => { await handleFsRemoveNodeThumbnail( { target: { thumbnail: `s3://${BUCKET}/${key}` } }, - { s3 }, + { s3, bucketName: BUCKET }, ); // GetObject should now error because the key was deleted. @@ -306,15 +361,43 @@ describe('thumbnails extension — handleFsRemoveNodeThumbnail', () => { ).rejects.toThrow(); }); + // The destructive half of the same confused deputy: the stored pointer + // decides which object is deleted, so a key the extension didn't mint + // must never reach DeleteObject. + it('does not delete an object the pointer names but we did not mint', async () => { + const victimKey = crypto.randomUUID(); // shaped like an fs object key + await s3.send( + new PutObjectCommand({ + Bucket: BUCKET, + Key: victimKey, + Body: Buffer.from(TINY_PNG_BASE64, 'base64'), + ContentType: 'image/png', + }), + ); + + await handleFsRemoveNodeThumbnail( + { target: { thumbnail: `s3://${BUCKET}/${victimKey}` } }, + { s3, bucketName: BUCKET }, + ); + + const survivor = await s3.send( + new GetObjectCommand({ Bucket: BUCKET, Key: victimKey }), + ); + expect(survivor.ContentType).toBe('image/png'); + }); + it('is a no-op when the target has no thumbnail', async () => { // Should not throw or attempt a delete. - await handleFsRemoveNodeThumbnail({ target: {} }, { s3 }); + await handleFsRemoveNodeThumbnail( + { target: {} }, + { s3, bucketName: BUCKET }, + ); }); it('is a no-op when the thumbnail URL is not an s3:// pointer', async () => { await handleFsRemoveNodeThumbnail( { target: { thumbnail: 'https://cdn.example.com/x.png' } }, - { s3 }, + { s3, bucketName: BUCKET }, ); }); }); diff --git a/extensions/thumbnails.ts b/extensions/thumbnails.ts index d948d2229..0bafffee8 100644 --- a/extensions/thumbnails.ts +++ b/extensions/thumbnails.ts @@ -13,6 +13,54 @@ const clients = extension.import('client'); const MAX_THUMBNAIL_BYTES = 2 * 1024 * 1024; const MAX_THUMBNAIL_PIXELS = 64e6; +// Namespace every object this extension writes. An fs object's key is its +// bare fsentry uuid and the default config points `thumbnailStore.name` at +// the same bucket as `s3_bucket`, so without a prefix of our own there is no +// way to tell a thumbnail we minted from any other object in the deployment. +const THUMBNAIL_KEY_PREFIX = 'thumbnails/'; +const UUID_PATTERN = + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +const mintThumbnailKey = (): string => + `${THUMBNAIL_KEY_PREFIX}${crypto.randomUUID()}`; + +/** + * Extract the object key from a stored thumbnail pointer, or null when the + * pointer isn't one this extension minted. + * + * `fsentries.thumbnail` is writable through the FS API, so neither half of the + * stored string is trusted: the bucket is discarded (callers always pass their + * own) and the key must sit under {@link THUMBNAIL_KEY_PREFIX} with a random + * uuid. Honouring an arbitrary key would lend this extension's storage + * credentials to whatever object the caller named — in the shared-bucket layout + * that is every user's file, since an fs object's key is its fsentry uuid. + * Legacy bare-uuid thumbnails fail the check and are treated as absent; they + * are indistinguishable from a planted pointer, so there is nothing safer to do + * with them than stop signing them. + */ +const resolveThumbnailKey = (pointer: string): string | null => { + let key: string; + if (pointer.startsWith('s3://')) { + const rest = pointer.slice('s3://'.length); + const slash = rest.indexOf('/'); + if (slash === -1) return null; + key = rest.slice(slash + 1); + } else { + let pathname: string; + try { + pathname = new URL(pointer).pathname; + } catch { + return null; + } + const segments = pathname.replace(/^\/+/, '').split('/'); + segments.shift(); // bucket + key = segments.join('/'); + } + if (!key.startsWith(THUMBNAIL_KEY_PREFIX)) return null; + const id = key.slice(THUMBNAIL_KEY_PREFIX.length); + return UUID_PATTERN.test(id) ? key : null; +}; + // S3 client + bucket config — lazily resolved after boot from config. let s3Client: S3Client | null = null; let s3PresignClient: S3Client | null = null; @@ -112,7 +160,7 @@ export async function handleThumbnailCreated( return; } - const key = crypto.randomUUID(); + const key = mintThumbnailKey(); event.url = `s3://${deps.bucketName}/${key}`; await deps.s3.send( @@ -151,7 +199,7 @@ export const handleThumbnailUploadPrepare = async ( continue; } - const key = crypto.randomUUID(); + const key = mintThumbnailKey(); const command = new PutObjectCommand({ Bucket: deps.bucketName, Key: key, @@ -178,27 +226,27 @@ export const handleThumbnailRead = async ( if (typeof thumb !== 'string' || !thumb) return; const presignClient = deps.s3Presign; - if (thumb.startsWith('s3://')) { - const [bucket, key] = thumb.slice(5).split('/'); - entry.thumbnail = await getSignedUrl( - presignClient, - new GetObjectCommand({ Bucket: bucket, Key: key }), - { expiresIn: 604800 }, - ); - } else if ( - thumb.startsWith('https') && - thumb.includes(new URL(deps.bucketEndpoint).hostname) - ) { + if ( + thumb.startsWith('s3://') || // Legacy format — remove after full migration - const [bucket, key] = new URL(thumb).pathname.slice(1).split('/'); + (thumb.startsWith('https') && + thumb.includes(new URL(deps.bucketEndpoint).hostname)) + ) { + const key = resolveThumbnailKey(thumb); + if (!key) { + // Not a pointer we minted — refuse to sign it rather than hand + // out a presigned read of whatever object it names. + entry.thumbnail = null; + return; + } entry.thumbnail = await getSignedUrl( presignClient, - new GetObjectCommand({ Bucket: bucket, Key: key }), + new GetObjectCommand({ Bucket: deps.bucketName, Key: key }), { expiresIn: 604800 }, ); } else if (thumb.startsWith('data')) { // Inline data-URL migration: upload to S3 and update the DB entry. - const key = crypto.randomUUID(); + const key = mintThumbnailKey(); const { mimeType, data } = base64ParseDataUrl(thumb); const newUrl = `s3://${deps.bucketName}/${key}`; @@ -233,14 +281,22 @@ export const handleThumbnailRead = async ( }; export const handleFsRemoveNodeThumbnail = async ( - payload: { target: Record }, - deps: { s3: S3Client }, + payload: { target: { thumbnail?: string | null } }, + deps: { s3: S3Client; bucketName: string }, ): Promise => { - const thumbnailUrl = payload.target.thumbnail as string | undefined; - if (!thumbnailUrl || !thumbnailUrl.startsWith('s3://')) return; + const thumbnailUrl = payload.target.thumbnail; + if (!thumbnailUrl) return; - const [bucket, key] = thumbnailUrl.slice(5).split('/'); - await deps.s3.send(new DeleteObjectCommand({ Bucket: bucket, Key: key })); + // Same trust rule as the read path, and load-bearing for the same reason: + // the pointer decides which object gets deleted, so a key we didn't mint + // would let the owner of one file destroy an object belonging to someone + // else just by naming it here. + const key = resolveThumbnailKey(thumbnailUrl); + if (!key) return; + + await deps.s3.send( + new DeleteObjectCommand({ Bucket: deps.bucketName, Key: key }), + ); }; extension.on( @@ -282,9 +338,9 @@ extension.on('thumbnail.read', async (_key, entry: Record) => { // -- fs.remove.node -------------------------------------------------- // Delete S3 thumbnail when the file is removed. -extension.on( - 'fs.remove.node', - async (_key, payload: { target: Record }) => { - await handleFsRemoveNodeThumbnail(payload, { s3: getClient() }); - }, -); +extension.on('fs.remove.node', async (_key, payload) => { + await handleFsRemoveNodeThumbnail(payload, { + s3: getClient(), + bucketName: thumbnailBucketName, + }); +}); diff --git a/src/backend/controllers/fs/LegacyFSController.test.ts b/src/backend/controllers/fs/LegacyFSController.test.ts index 019ddf22d..78fb88273 100644 --- a/src/backend/controllers/fs/LegacyFSController.test.ts +++ b/src/backend/controllers/fs/LegacyFSController.test.ts @@ -2743,6 +2743,48 @@ describe('LegacyFSController.updateFsentryThumbnail', () => { const body = captured.body as { thumbnail: string }; expect(typeof body.thumbnail).toBe('string'); }); + + // The write ACL here covers the entry being annotated, not whatever the + // thumbnail string points at. A storage pointer stored verbatim is later + // presigned (and deleted) by the thumbnails extension using the server's + // own credentials, so the owner of one file could name another user's + // object — an fs object's key is its fsentry uuid — and have the server + // read or destroy it. Only inline image data is accepted. + it.each([ + ['an s3:// pointer', 's3://puter-local/00000000-0000-4000-8000-000000000000'], + ['an https URL', 'https://cdn.example.com/x.png'], + ['a bare object key', 'thumbnails/whatever'], + ])('rejects %s instead of storing it verbatim', async (_label, thumbnail) => { + const { actor } = await makeUser(); + const username = actor.user!.username!; + const target = `/${username}/Documents/thumbme3-${uuidv4()}.txt`; + await withActor(actor, () => + controller.touch( + makeReq({ + body: { path: target, set_modified_to_now: true }, + actor, + }), + makeRes().res, + ), + ); + const entry = await server.stores.fsEntry.getEntryByPath(target); + + const { res } = makeRes(); + await expect( + withActor(actor, () => + controller.updateFsentryThumbnail( + makeReq({ + body: { uid: entry!.uuid, thumbnail }, + actor, + }), + res, + ), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + + const after = await server.stores.fsEntry.getEntryByUuid(entry!.uuid); + expect(after?.thumbnail ?? null).toBeNull(); + }); }); // ── GET /get-launch-apps ──────────────────────────────────────────── diff --git a/src/backend/controllers/fs/LegacyFSController.ts b/src/backend/controllers/fs/LegacyFSController.ts index 84fa7ee2a..bbaa4818f 100644 --- a/src/backend/controllers/fs/LegacyFSController.ts +++ b/src/backend/controllers/fs/LegacyFSController.ts @@ -863,6 +863,14 @@ export class LegacyFSController extends PuterController { throw new HttpError(400, 'Missing `thumbnail`', { legacyCode: 'bad_request', }); + // Only inline image data. Clients generate the thumbnail themselves + // and the thumbnails extension is what turns it into a storage + // pointer; accepting a pointer here would let a caller name an object + // the server would then sign reads of, and delete, on their behalf. + if (!thumbnail.startsWith('data:')) + throw new HttpError(400, '`thumbnail` must be a data: URL', { + legacyCode: 'bad_request', + }); const entry = await this.stores.fsEntry.getEntryByUuid(uid); if (!entry || !entry.path)