diff --git a/docs/database/attachments.md b/docs/database/attachments.md index 7c452942..82f006b6 100644 --- a/docs/database/attachments.md +++ b/docs/database/attachments.md @@ -95,7 +95,7 @@ Originals in JPEG, PNG, GIF and WebP can be thumbnailed. The thumbnailer cannot The client file name never reaches a blob key. The disk name is 22 random hexadecimal characters plus an extension, and the client's lower-cased extension must be one to ten letters or digits from the allowed list: `Limits.Extensions`, or, when that is empty, `attach.DefaultImageExtensions` in image mode (jpg, jpeg, png, gif and webp) and `attach.DefaultFileExtensions` otherwise. The file list is WinterCMS's default list without the types that can carry script: svg, js, map, css, less, scss, swf and xml. A refused extension returns `attach.ErrFileType`. -The disk name's extension follows the content, not the client's name, because a web server or CDN in front of the bucket picks the Content-Type from the extension. `attach.DiskExtension` derives it from the sniffed type and the client's extension. For a specific type such as PNG, JPEG, PDF, MP3 or WebM, the client's extension is kept when it names that type, else the type's usual extension is used: a PNG uploaded as `x.html` (with `html` allowed) is stored as `.png`, and its URL ends in `.png`. For a generic sniff (plain text, HTML, XML, a zip container or unknown bytes) the client's extension is kept, so `report.docx` stays `.docx` and an HTML body uploaded as `note.txt` stays `.txt`, unless the extension is one a browser renders or runs (html, svg, xml, js, css, php and the like), which becomes `.txt` for text and `.bin` otherwise. `file_name` always keeps the client's name, so downloads still carry it. +The disk name's extension follows the content, not the client's name, because a static web server in front of the uploads directory picks the Content-Type from the extension. A public bucket, or a CDN that pulls from it, sends the type stored with the object instead; see the [deploy note](../services/storage.md#deploy-note) for both cases. `attach.DiskExtension` derives the extension from the sniffed type and the client's extension. For a specific type such as PNG, JPEG, PDF, MP3 or WebM, the client's extension is kept when it names that type, else the type's usual extension is used: a PNG uploaded as `x.html` (with `html` allowed) is stored as `.png`, and its URL ends in `.png`. For a generic sniff (plain text, HTML, XML, a zip container or unknown bytes) the client's extension is kept, so `report.docx` stays `.docx` and an HTML body uploaded as `note.txt` stays `.txt`, unless the extension is one a browser renders or runs (html, svg, xml, js, css, php and the like), which becomes `.txt` for text and `.bin` otherwise. `file_name` always keeps the client's name, so downloads still carry it. The content type is sniffed from the first bytes of the body, never taken from the client. `Limits.MIMETypes` narrows it further: an entry with a slash is a MIME pattern such as `image/png` or `image/*`, an entry without one is an extension, and a file that matches no entry returns `attach.ErrMIMEType`. `Limits.MaxBytes` is enforced while the body streams into the bucket; a body one byte over the limit returns `attach.ErrTooLarge` and leaves no blob and no row behind. diff --git a/docs/services/storage.md b/docs/services/storage.md index 4aa2a137..6c604a2d 100644 --- a/docs/services/storage.md +++ b/docs/services/storage.md @@ -121,7 +121,43 @@ The route looks up the bucket and the database on each request: `route:list` lis Without the key, the application decides where files are served: mount `attach.StaticHandlerPublic` itself, or put a web server or CDN in front of the bucket directory. Either way, serve uploads from a separate origin when you can; the [Attachments](../database/attachments.md) page explains why. The built-in route is meant for development and for deployments that cannot split the origin yet. -> **Deploy note.** When a static server such as nginx, or a CDN, serves the uploads directory or bucket directly, it picks the Content-Type from the file extension and sends none of the route's headers. `attach.Store` names each file after its sniffed content (`attach.DiskExtension`), so a PNG uploaded as `x.html` is stored as `.png`, but configure the server as the route behaves anyway: send `X-Content-Type-Options: nosniff` on every upload, and serve only the extensions the application accepts (for example the image and document types in `attach.DefaultFileExtensions`), answering 404 for anything else. +### Deploy note + +`attach.Store` names each file after its sniffed content (`attach.DiskExtension`), so a PNG uploaded as `x.html` is stored as `.png`. What reaches the browser still depends on who serves the uploads when the built-in route does not, and there are two cases. + +**A static server serving the uploads directory.** nginx and similar servers pick the Content-Type from the file extension and send none of the route's headers. Send `X-Content-Type-Options: nosniff` on every response, serve an allow-list of extensions with a types map of its own, and answer 404 for everything else: + +```nginx +server { + # nosniff for every response. An add_header inside a location replaces + # every add_header inherited from the server level, so keep the security + # headers here. + add_header X-Content-Type-Options nosniff always; + + # Raster images only. The types come from this block, not from the + # server's mime.types. + location ~* ^/storage/app/uploads/public/.+\.(?:jpe?g|png|gif|webp)$ { + root /srv/acme; + types { + image/jpeg jpg jpeg; + image/png png; + image/gif gif; + image/webp webp; + } + default_type application/octet-stream; + } + + # Everything else under /storage/ is refused. This must stay a plain + # prefix location, not ^~, so that nginx tries the regex above first. + location /storage/ { + return 404; + } +} +``` + +Extend the extension list only with types the application accepts (see `attach.DefaultFileExtensions`), and serve anything that is not a raster image as a download (`Content-Disposition: attachment`) or from a separate origin, the way the built-in route does. + +**A public S3 bucket, or a CDN that pulls from it, serving the files under `storage.uploads.public_url`.** It does not look at the extension: it sends the Content-Type stored with the object, and `attach.Store` stores the sniffed type. An HTML body uploaded as `note.txt` (txt is in `attach.DefaultFileExtensions`) is stored as `.txt` but served as `text/html`. Serve such a bucket from its own origin, never the application's, and set `X-Content-Type-Options: nosniff` and, for everything but raster images, `Content-Disposition: attachment` through the bucket's or the CDN's response-header settings. ## Upload size diff --git a/modules/lagoon/attach/store_test.go b/modules/lagoon/attach/store_test.go index 8b61295b..726b7d15 100644 --- a/modules/lagoon/attach/store_test.go +++ b/modules/lagoon/attach/store_test.go @@ -4,11 +4,14 @@ import ( "archive/zip" "bytes" "context" + "encoding/binary" "errors" + "hash/crc32" "image" "image/jpeg" "image/png" "io" + "net/http" "regexp" "strings" "testing" @@ -29,6 +32,32 @@ func smokePNG(t *testing.T) []byte { return buf.Bytes() } +// scriptText is the payload of the stored-XSS todo: a page that reads the +// application's cookies if a browser renders the upload as HTML. +const scriptText = "" + +// pngWithScriptText returns a real, decodable PNG that carries scriptText in +// a tEXt chunk right after IHDR. It passes attach.IsAllowedImage, which is +// why the disk name, not the image guard, has to keep it from being served +// as HTML. +func pngWithScriptText(t *testing.T) []byte { + t.Helper() + base := smokePNG(t) + // 8-byte signature, then IHDR: 4 length, 4 type, 13 data, 4 CRC. + const split = 8 + 4 + 4 + 13 + 4 + if len(base) < split || string(base[12:16]) != "IHDR" { + t.Fatalf("unexpected PNG layout: % x", base[:min(len(base), 16)]) + } + data := append([]byte("Comment\x00"), scriptText...) + chunk := binary.BigEndian.AppendUint32(nil, uint32(len(data))) + typed := append([]byte("tEXt"), data...) + chunk = append(chunk, typed...) + chunk = binary.BigEndian.AppendUint32(chunk, crc32.ChecksumIEEE(typed)) + out := append([]byte{}, base[:split]...) + out = append(out, chunk...) + return append(out, base[split:]...) +} + func bucketKeys(t *testing.T, bucket *blob.Bucket) []string { t.Helper() var keys []string @@ -345,6 +374,69 @@ func TestStoreDiskExtensionSmoke(t *testing.T) { } } +// TestPNGScriptChunkNaming pins the naming half of the stored-XSS todo +// without Docker: a write-scoped client uploads a valid image under an .html +// name. The PNG with a script tEXt chunk decodes, passes attach.IsAllowedImage +// and sniffs as image/png, so attach.DiskExtension names it .png, not .html. +func TestPNGScriptChunkNaming(t *testing.T) { + fixture := pngWithScriptText(t) + if _, err := png.Decode(bytes.NewReader(fixture)); err != nil { + t.Fatalf("fixture does not decode: %v", err) + } + if !attach.IsAllowedImage(fixture) { + t.Fatal("fixture is refused by IsAllowedImage") + } + if !bytes.Contains(fixture, []byte(scriptText)) { + t.Fatal("fixture lost the script payload") + } + ct := http.DetectContentType(fixture) + if ct != "image/png" { + t.Fatalf("sniffed %q, want image/png", ct) + } + if got := attach.DiskExtension(ct, "html"); got != "png" { + t.Fatalf("DiskExtension(%q, html) = %q, want png", ct, got) + } +} + +// TestStoreScriptChunkPNG runs the stored-XSS todo scenario through Store on +// Postgres: a write-scoped client uploads a valid image under an .html name +// in image mode, with html allowed so the extension check is not what stops +// it. The file is stored and served as .png with content_type image/png, +// file_name keeps x.html, and the bytes, script chunk included, are stored +// as uploaded: only the name makes them harmless. +func TestStoreScriptChunkPNG(t *testing.T) { + if testing.Short() { + t.Skip("requires testcontainers postgres") + } + ctx := t.Context() + gdb := attachGorm(t) + if err := lagoon.Migrate(gdb, nil); err != nil { + t.Fatal(err) + } + bucket := memblob.OpenBucket(nil) + t.Cleanup(func() { _ = bucket.Close() }) + + fixture := pngWithScriptText(t) + f, err := attach.Store(ctx, gdb, bucket, attach.Upload{FileName: "x.html", Body: bytes.NewReader(fixture), Public: true}, + attach.Limits{Image: true, Extensions: []string{"png", "html"}}) + if err != nil { + t.Fatal(err) + } + if !strings.HasSuffix(f.DiskName, ".png") || !strings.HasSuffix(f.URL(), ".png") { + t.Fatalf("disk name %q, URL %q, want .png", f.DiskName, f.URL()) + } + if f.FileName != "x.html" || f.ContentType != "image/png" { + t.Fatalf("file_name %q, content_type %q, want x.html image/png", f.FileName, f.ContentType) + } + got, err := bucket.ReadAll(ctx, attach.BlobKey(f.DiskName)) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(got, fixture) { + t.Fatalf("stored %d bytes differ from the %d uploaded", len(got), len(fixture)) + } +} + // TestStoreDiskExtension (plan 05 of phase 19.3, D-15) runs the stored-XSS // regression table through Store on Postgres: the disk name and the URL end // in the extension DiskExtension picks for the sniffed bytes, file_name keeps