Files
remark42/backend/app/store/image/image.go
T
Dmitry VerkhoturovandGitHub 0e20861419 fix(security): reject non-image content-types in image proxy and /picture/ to prevent stored XSS (#2067)
* fix(security): reject non-image content-types in image proxy and /picture/ to prevent stored XSS

The /api/v1/img proxy and /api/v1/picture/{user}/{id} endpoints emitted
http.DetectContentType on the served bytes as the response Content-Type. A
controlled upstream serving Content-Type: image/png with an HTML body passed
the upstream check (only the response header was inspected, not the body),
and the body bytes then sniffed back to text/html — so the proxy served the
attacker's HTML from the remark42 origin. Browsers honoured the declared
text/html and executed the response as a document with access to cookies and
CSRF tokens. Affected from v1.6.0 (April 2020) through v1.15.0; verified live
via published docker images.

Layered defense applied to both handlers:

- rest.SafeImgContentType (in backend/app/rest/) validates sniffed content
  against a strict allowlist: image/png, image/jpeg, image/gif, image/webp,
  image/bmp, image/x-icon. Anything else (HTML, XML, SVG, plain text,
  octet-stream, or any future image type the stdlib sniffer may learn) is
  rejected with no body echo. SVG is implicitly excluded — it sniffs as
  text/xml or text/plain, never image/svg+xml, and SVG can execute scripts
  when navigated to top-level. The previous octet-stream → image/* fallback
  is gone.
- Per-endpoint Content-Security-Policy override sets
  "default-src 'none'; sandbox; frame-ancestors 'none'" on every response
  (success, 304, or error). Sandbox neuters scripts even if Content-Type
  ever regresses. The same policy is also applied to all /api/v1/* via
  apiCSPMiddleware as defense-in-depth.
- Content-Disposition: inline; filename="image" frames the response as a
  file rather than a renderable document.
- /picture/ rejection paths set Cache-Control: no-store so 4xx responses
  are never cached.

The defense headers and the strict ETag matcher are extracted as
rest.SetImageDefenseHeaders and rest.EtagMatches in the shared rest package
(consumed by both proxy/image and api/rest_public — no package cycle).

The /api/v1/img path additionally bumps the ETag to a versioned `"v2:..."`
so revalidating clients (top-level navigation, Ctrl+R, intermediaries) get
a fresh 200 instead of a 304 against poisoned pre-fix cached HTML.

DELIBERATE TRADEOFF: Cache-Control on /api/v1/img success responses remains
max-age=2592000 (30 days), unchanged from before. An aggressive "force
revalidate on every reuse" policy was prototyped during review but reverted
because the perf cost (a server round-trip on every image view, even with
304 saving the body bytes) outweighed the corner-case mitigation. The
realistic exposure of cache carryover is narrow: cache carryover only
affects users who navigated top-level to an attacker URL pre-fix and still
have it in their local cache — the normal <img> embed path cached text/html
but never executed it. Local browser caches that hold pre-fix bytes
continue to serve them until their 30-day TTL expires or are evicted under
memory pressure. The ETag bump reaches all clients that DO revalidate
during the cached lifetime (Ctrl+R, intermediaries, post-expiry use); for
the rest, exposure self-limits via cache expiry. Operators running a
CDN/edge cache in front of remark42 should purge /api/v1/img after deploy.

The /api/v1/img handler short-circuits on a matching current-version
If-None-Match before any store Load or upstream fetch, returning a bodyless
304 with the defense headers set. Safe because the 304 carries no body and
the client's cached bytes came from a prior validated 200; an attacker
fabricating an etag value can only short-circuit fetches for URLs they
themselves crafted. This avoids upstream DoS amplification when clients
revalidate on hot comment pages.

The /api/v1/img route was moved from the "open routes" group (which uses
middleware.NoCache, stripping If-None-Match from incoming requests) to the
"open routes, cached" group alongside /picture/ and /qr/telegram so the
304 revalidation path is no longer broken upstream of the handler.

The /picture/{user}/{id} endpoint does not need the v2 etag prefix. Upload
validates input format via readAndValidateImage and the serve path
re-validates the stored bytes via rest.SafeImgContentType. Bytes within
the resize dimension limits are preserved verbatim, so the browser defense
relies on the response headers (validated Content-Type + nosniff + strict
CSP + Content-Disposition: inline), not on byte normalization.

Global CSP: font-src data: → font-src 'none'. Audit confirmed no @font-face,
no base64 fonts, no icon-font library in the bundle. Drops an unnecessary
attack surface; no behavioural change.

Tests: TestImage_ContentTypeHandling table-tests a real PNG and attack
shapes (HTML claimed as image/png, image/jpeg, image/gif, image/svg+xml,
image/webp; svg with onload; html fragment; polyglot PNG+HTML), proving
the defense holds across arbitrary upstream Content-Type variation.
Polyglot case is intentionally served as image/png — the browser cannot
execute the trailing HTML when the response type is image/png with nosniff.
TestImage_ContentTypeHandling_CacheHit exercises the cache-hit branch with
attacker bytes preloaded into the store. TestImage_PerRequestRevalidation
alternates upstream PNG/HTML across four proxy calls to prove no trust
accumulates between requests. TestImage_RoutesUsingCachedImage asserts
cache-poisoning is caught at serve time. TestImage_EtagVersioned asserts
the v2 prefix invalidates pre-fix etags AND that the revalidation 304
triggers no store Load. TestImage_RevalidationSkipsIO proves the
short-circuit works even with no upstream reachable. TestSafeImgContentType
covers the allowlist directly. TestRest_LoadPictureDefenseHeaders and
TestRest_LoadPictureRejectsNonImage exercise the /picture/ endpoint.
TestRest_apiCSP covers the strict CSP middleware on JSON API + RSS routes;
TestRest_securityHeaders confirms /web/ HTML pages keep the global CSP.

Verified end-to-end against the dev docker image: the original demo URL
(arbitrary HTML claimed as image/png) now returns 415 application/json with
CSP/nosniff/Content-Disposition set, no XSS in the browser.

* fix(security): set Cache-Control: no-store on image-proxy error paths, sync stale route comment

Addresses two review comments on #2067:

1. Cache-Control: max-age=2592000 and Etag were set before the
   load/download/validation block, so 404/400/415 error responses inherited
   the 30-day cache TTL and the versioned etag — a transient failure (or an
   intentionally triggered 415) would be pinned in browser/intermediary
   caches for that TTL, keeping users locked out even after the underlying
   cause was resolved. Now: etag is computed but not set as a header until
   after validation succeeds; error paths route through sendImageProxyError
   which sets Cache-Control: no-store and never sets Etag. The 304
   short-circuit still sets both because that path serves the same validated
   content the client already has cached.

2. The comment at rest.go:282 still described the prototyped
   no-cache/must-revalidate Cache-Control policy that was reverted before
   the PR landed. Updated to match the actual 30-day max-age behavior.

Tests: TestImage_ContentTypeHandling now asserts reject paths carry
Cache-Control: no-store and have no Etag header, and accept paths carry
the max-age=2592000 + v2: etag.
2026-05-20 22:37:25 -05:00

428 lines
14 KiB
Go

// Package image handles storing, resizing and retrieval of images
// Provides Store with Save and Load implementations on top of local file system and bolt db.
// Service object encloses Store and add common methods, this is the one consumer should use.
package image
// NOTE: matryer/moq should be installed globally and works with `go generate ./...`
//go:generate moq --out image_mock.go . Store
import (
"bytes"
"context"
"crypto/sha1" //nolint:gosec // not used for cryptography
"encoding/base64"
"fmt"
"image"
_ "image/gif" // register gif decoder
_ "image/jpeg" // register jpeg decoder
"image/png"
"io"
"net/http"
"net/url"
"path"
"strings"
"sync"
"sync/atomic"
"time"
"github.com/PuerkitoBio/goquery"
log "github.com/go-pkgz/lgr"
"github.com/hashicorp/go-multierror"
"github.com/rs/xid"
"golang.org/x/image/draw"
_ "golang.org/x/image/webp" // register webp decoder so DecodeConfig accepts what readAndValidateImage allows
)
// Service wraps Store with common functions needed for any store implementation
// It also provides async Submit with func param retrieving all submitting IDs.
// Submitted IDs committed (i.e. moved from staging to final) on ServiceParams.EditDuration expiration.
type Service struct {
ServiceParams
store Store
wg sync.WaitGroup
submitCh chan submitReq
once sync.Once
term int32 // term value used atomically to detect emergency termination
submitCount int32 // atomic increment for counting submitted images
}
// ServiceParams contains externally adjustable parameters of Service
type ServiceParams struct {
EditDuration time.Duration // edit period for comments
ImageAPI string // image api matching path
ProxyAPI string // proxy api matching path
MaxSize int
MaxHeight int
MaxWidth int
}
// StoreInfo contains image store meta information
type StoreInfo struct {
FirstStagingImageTS time.Time
}
// Store defines interface for saving and loading pictures.
// Declares two-stage save with Commit. Save stores to staging area and Commit moves to the final location.
// Two-stage commit scheme is used for not storing images which are uploaded but later never used in the comments,
// e.g. when somebody uploaded a picture but did not sent the comment.
type Store interface {
Info() (StoreInfo, error) // get meta information about storage
Save(id string, img []byte) error // store image with passed id to staging
Load(id string) ([]byte, error) // load image by ID
Delete(id string) error // delete image by ID
ResetCleanupTimer(id string) error // resets cleanup timer for the image, called on comment preview
Commit(id string) error // move image from staging to permanent
Cleanup(ctx context.Context, ttl time.Duration) error // run removal loop for old images on staging
}
const submitQueueSize = 5000
type submitReq struct {
idsFn func() (ids []string)
TS time.Time
}
// NewService returns new Service instance
func NewService(s Store, p ServiceParams) *Service {
return &Service{ServiceParams: p, store: s}
}
// Commit multiple ids immediately
func (s *Service) Commit(idsFn func() []string) error {
errs := new(multierror.Error)
for _, id := range idsFn() {
err := s.store.Commit(id)
if err != nil {
errs = multierror.Append(errs, fmt.Errorf("failed to commit image %s: %w", id, err))
}
}
return errs.ErrorOrNil()
}
// Submit multiple ids via function for delayed commit
func (s *Service) Submit(idsFn func() []string) {
if idsFn == nil || s == nil {
return
}
s.once.Do(func() {
log.Printf("[DEBUG] image submitter activated")
s.submitCh = make(chan submitReq, submitQueueSize)
s.wg.Go(func() {
for req := range s.submitCh {
// wait for EditDuration expiration with emergency pass on term
for atomic.LoadInt32(&s.term) == 0 && time.Since(req.TS) <= s.EditDuration {
time.Sleep(time.Millisecond * 10) // small sleep to relive busy wait but keep reactive for term (close)
}
err := s.Commit(req.idsFn)
if err != nil {
log.Printf("[WARN] image commit error %v", err)
}
atomic.AddInt32(&s.submitCount, -1)
}
log.Printf("[INFO] image submitter terminated")
})
})
atomic.AddInt32(&s.submitCount, 1)
// reset cleanup timer before submitting the images
// to prevent them from being cleaned up while waiting for EditDuration to expire
for _, imgID := range idsFn() {
_ = s.store.ResetCleanupTimer(imgID)
}
s.submitCh <- submitReq{idsFn: idsFn, TS: time.Now()}
}
// ExtractPictures gets list of images from the doc html and convert from urls to ids, i.e. user/pic.png
func (s *Service) ExtractPictures(commentHTML string) (ids []string) {
return s.extractImageIDs(commentHTML, true)
}
// ExtractNonProxiedPictures gets list of non-proxied images from the doc html and convert from urls to ids, i.e. user/pic.png
// This method is used in image check on post preview and load, as proxied images have lazy loading
// and wouldn't be present on disk but still valid as they will be loaded the first time someone requests them.
func (s *Service) ExtractNonProxiedPictures(commentHTML string) (ids []string) {
return s.extractImageIDs(commentHTML, false)
}
// Cleanup runs periodic cleanup with 1.5*ServiceParams.EditDuration. Blocking loop, should be called inside of goroutine by consumer
func (s *Service) Cleanup(ctx context.Context) {
if s.EditDuration <= 0 {
log.Printf("[INFO] pictures cleanup disabled, edit duration is %v", s.EditDuration)
<-ctx.Done()
return
}
cleanupTTL := s.EditDuration * 15 / 10 // cleanup images older than 1.5 * EditDuration
log.Printf("[INFO] start pictures cleanup, staging ttl=%v", cleanupTTL)
for {
select {
case <-ctx.Done():
log.Printf("[INFO] cleanup terminated, %v", ctx.Err())
return
case <-time.After(cleanupTTL):
if err := s.store.Cleanup(ctx, cleanupTTL); err != nil {
log.Printf("[WARN] failed to cleanup, %v", err)
}
}
}
}
// ResetCleanupTimer resets cleanup timer for the image
func (s *Service) ResetCleanupTimer(id string) error {
return s.store.ResetCleanupTimer(id)
}
// Info returns meta information about storage
func (s *Service) Info() (StoreInfo, error) {
return s.store.Info()
}
// Close flushes all in-progress submits and enforces waiting commits
func (s *Service) Close(ctx context.Context) {
log.Printf("[INFO] close image service ")
waitForTerm := func(ctx context.Context) {
ticker := time.NewTicker(10 * time.Millisecond)
defer ticker.Stop()
for {
select {
case <-ctx.Done():
return
case <-ticker.C:
if atomic.LoadInt32(&s.submitCount) == 0 {
return
}
}
}
}
atomic.StoreInt32(&s.term, 1) // enforce non-delayed commits for all ids left in submitCh
waitForTerm(ctx)
if s.submitCh != nil {
close(s.submitCh)
}
s.wg.Wait()
}
// Load wraps storage Load function.
func (s *Service) Load(id string) ([]byte, error) {
return s.store.Load(id)
}
// Delete wraps storage Delete function.
func (s *Service) Delete(id string) error {
return s.store.Delete(id)
}
// Save wraps storage Save function, validating and resizing the image before calling it.
func (s *Service) Save(userID string, r io.Reader) (id string, err error) {
id = path.Join(userID, guid())
return id, s.SaveWithID(id, r)
}
// SaveWithID wraps storage Save function, validating and resizing the image before calling it.
func (s *Service) SaveWithID(id string, r io.Reader) error {
img, err := s.prepareImage(r)
if err != nil {
return err
}
return s.store.Save(id, img)
}
// returns list of image IDs from the comment html, including proxied images if includeProxied is true
func (s *Service) extractImageIDs(commentHTML string, includeProxied bool) (ids []string) {
doc, err := goquery.NewDocumentFromReader(strings.NewReader(commentHTML))
if err != nil {
log.Printf("[ERROR] can't parse commentHTML to parse images: %q, error: %v", commentHTML, err)
return nil
}
doc.Find("img").Each(func(_ int, sl *goquery.Selection) {
if im, ok := sl.Attr("src"); ok {
if strings.Contains(im, s.ImageAPI) {
elems := strings.Split(im, "/")
if len(elems) >= 2 {
id := elems[len(elems)-2] + "/" + elems[len(elems)-1]
ids = append(ids, id)
}
}
if includeProxied && strings.Contains(im, s.ProxyAPI) {
proxiedURL, err := url.Parse(im)
if err != nil {
return
}
imgURL, err := base64.URLEncoding.DecodeString(proxiedURL.Query().Get("src"))
if err != nil {
return
}
imgID, err := CachedImgID(string(imgURL))
if err != nil {
return
}
ids = append(ids, imgID)
}
}
})
return ids
}
// maxImagePixels caps the declared pixel count of an image before any raster decode
// is allowed. Without this, a tiny compressed "decompression bomb" image declaring
// e.g. 65535x65535 px would force image.Decode to allocate gigabytes of pixel memory
// and OOM the service on a single comment upload. 16 MP covers any realistic image
// (~4096x4096) while keeping peak allocation bounded.
const maxImagePixels = 16 * 1024 * 1024
// prepareImage calls readAndValidateImage and resize on provided image.
func (s *Service) prepareImage(r io.Reader) ([]byte, error) {
data, err := readAndValidateImage(r, s.MaxSize)
if err != nil {
return nil, fmt.Errorf("can't load image: %w", err)
}
resized := resize(data, s.MaxWidth, s.MaxHeight)
if resized == nil {
return nil, fmt.Errorf("image rejected: malformed or exceeds %d-pixel safe limit", maxImagePixels)
}
return resized, nil
}
// resize validates an image and, if needed, re-encodes it to fit within the given
// pixel limits preserving aspect ratio. Returns nil for malformed input or for
// declared dimensions exceeding maxImagePixels so attacker payloads (decompression
// bombs) never reach the store. With limit <= 0 or when the image already fits, the
// original bytes are returned verbatim so animated GIFs and other multi-frame formats
// round-trip without being flattened to one frame.
//
// Validation uses image.DecodeConfig (cheap — declares dimensions, allocates nothing)
// before any full image.Decode, so a 100 KB compressed image declaring 65535x65535 px
// is rejected without ever materializing the raster.
func resize(data []byte, limitW, limitH int) []byte {
if len(data) == 0 {
return nil
}
// validate format and dimensions without allocating pixel memory.
cfg, _, err := image.DecodeConfig(bytes.NewReader(data))
if err != nil {
log.Printf("[WARN] can't decode image config, %s", err)
return nil
}
// multiply in int64 — on 32-bit builds (GOARCH=386, 32-bit arm) the int
// product of two 16-bit-or-larger dimensions can overflow and wrap below
// maxImagePixels, bypassing the cap.
if cfg.Width <= 0 || cfg.Height <= 0 || int64(cfg.Width)*int64(cfg.Height) > int64(maxImagePixels) {
log.Printf("[WARN] image dimensions %dx%d exceed safe limit", cfg.Width, cfg.Height)
return nil
}
// dimensions are bounded — full decode is now safe to allocate. Decode also
// validates the raster body: a header that DecodeConfig accepts but with a
// corrupt or truncated payload would slip through if we returned early on the
// no-resize path without ever touching the pixels. Decode unconditionally,
// then either return the original bytes (no resize needed, multi-frame intact)
// or the re-encoded result.
src, _, err := image.Decode(bytes.NewReader(data))
if err != nil {
log.Printf("[WARN] can't decode image after dim-check, %s", err)
return nil
}
if limitW <= 0 || limitH <= 0 || (cfg.Width <= limitW && cfg.Height <= limitH) {
return data
}
w, h := src.Bounds().Dx(), src.Bounds().Dy()
newW, newH := getProportionalSizes(w, h, limitW, limitH)
m := image.NewRGBA(image.Rect(0, 0, newW, newH))
draw.CatmullRom.Scale(m, m.Bounds(), src, src.Bounds(), draw.Src, nil)
var out bytes.Buffer
if err = png.Encode(&out, m); err != nil {
log.Printf("[WARN] can't encode resized image to png, %s", err)
return data // fall back to the validated original
}
return out.Bytes()
}
// getProportionalSizes returns width and height resized by both dimensions proportionally
func getProportionalSizes(srcW, srcH, limitW, limitH int) (resW, resH int) {
if srcW <= limitW && srcH <= limitH {
return srcW, srcH
}
ratioW := float64(srcW) / float64(limitW)
propH := float64(srcH) / ratioW
ratioH := float64(srcH) / float64(limitH)
propW := float64(srcW) / ratioH
if int(propH) > limitH {
return int(propW), limitH
}
return limitW, int(propH)
}
// check if file f is a valid image format, i.e. gif, png, jpeg or webp and reads up to maxSize.
func readAndValidateImage(r io.Reader, maxSize int) ([]byte, error) {
isValidImage := func(b []byte) bool {
ct := http.DetectContentType(b)
return ct == "image/gif" || ct == "image/png" || ct == "image/jpeg" || ct == "image/webp"
}
lr := io.LimitReader(r, int64(maxSize)+1)
data, err := io.ReadAll(lr)
if err != nil {
return nil, err
}
if len(data) > maxSize {
return nil, fmt.Errorf("file is too large (limit=%d)", maxSize)
}
// read header first to check the format. http.DetectContentType inspects up
// to the first 512 bytes, but a smaller body is fine — pass the whole slice
// rather than panicking on a fixed-size sub-slice.
header := data
if len(header) > 512 {
header = header[:512]
}
if !isValidImage(header) {
return nil, fmt.Errorf("file format not allowed")
}
return data, nil
}
// guid makes a globally unique id
func guid() string {
return xid.New().String()
}
// Sha1Str converts provided string to sha1
func Sha1Str(s string) string {
return fmt.Sprintf("%x", sha1.Sum([]byte(s))) //nolint:gosec // not used for cryptography
}
// CachedImgID generates ID for a cached image.
// ID would look like: "cached_images/<sha1-of-image-url-hostname>-<sha1-of-image-entire-url>"
// <sha1-of-image-url-hostname> - would allow us to identify all images from particular site if ever needed
// <sha1-of-image-entire-url> - would allow us to avoid storing duplicates of the same image
// (as accurate as deduplication based on potentially mutable url can be)
func CachedImgID(imgURL string) (string, error) {
parsedURL, err := url.Parse(imgURL)
if err != nil {
return "", fmt.Errorf("can parse url %s: %w", imgURL, err)
}
return fmt.Sprintf("cached_images/%s-%s", Sha1Str(parsedURL.Hostname()), Sha1Str(imgURL)), nil
}