Commit `184ce2c6dc`

184ce2c6dcdd047d6888fd243b9189c797f58d38

parent: 8e8c03891a

Unregistered key

cmc <hello@cleberg.net> · 2026-07-15 16:41 UTC
committer: <noreply@github.com>

fix: reject forged subdomains in the media proxy (#9)

* fix: make memcache concurrency-safe and stamp the version at link time

memcache was a crash waiting for traffic. Readers touched tempFS without
holding mx, while a per-entry goroutine deleted from it under the lock: a
concurrent map read and map write, which the runtime treats as a fatal error
that recover cannot catch. The option ships in config.example.json and was the
one cache key SETUP.md never documented, so it read like a free win to enable.

Put every map and field access behind the mutex, and age the whole map from one
janitor instead of a goroutine per cached file, each of which looped forever
holding its entry alive. mx is now a plain Mutex: every operation here mutates
something, and the old code took an RLock to write. Document the option, and
cover it with tests that run the readers, writers and janitor concurrently.

Split the disk/origin fetch out of DownloadAndSendMedia while there, so the
error path returns instead of falling through to write an empty body after the
error page.

Release.Version was hardcoded to 1.3.2, so images tagged v1.3.6 reported 1.3.2
from --help and /api/instance, and --help linked to the wrong release. Take it
from a main.version string the release workflow links in from the git tag.

* fix: reject forged subdomains in the media proxy

DownloadAndSendMedia built its upstream URL by concatenation, pasting the
subdomain segment of the request path straight into the host position. That
segment reaches the handler already percent-decoded, so it can carry "@", "#",
"?" and "/" — the characters that end a host. A request for

    /media/file/x@127.0.0.1:8080%2F/f/x.jpg

built a URL whose host parsed as 127.0.0.1:8080, with images-wixmp-x demoted to
userinfo, letting any caller aim the instance's fetcher at any address it could
reach, including services behind the firewall.

Validate the label against ^[a-zA-Z0-9-]+$ and refuse anything else with a 400.
Rejecting rather than escaping is what closes this: the label is the host, and
url.URL passes a host through verbatim, so building the URL structurally is not
sufficient on its own. DeviantArt's own media URLs use a hex-and-dash label, and
ParseMedia already splits on the first dot, so a legitimate label cannot contain
one.

Build the URL from url.URL fields as well, which escapes the path, and encode
the token argument, which reached the request unescaped.

Reported by CodeQL as go/request-forgery (CWE-918).

Layout: unified · split

.github/workflows/release.yml +4

		@@ -60,6 +60,10 @@ jobs:
60	60	push: true
61	61	tags: ${{ steps.meta.outputs.tags }}
62	62	labels: ${{ steps.meta.outputs.labels }}
	63	# Link the tag into the binary, so --help and /api/instance report the
	64	# same version as the image tag.
	65	build-args: \|
	66	VERSION=${{ steps.meta.outputs.version }}
63	67	cache-from: type=gha
64	68	cache-to: type=gha,mode=max
65	69

Dockerfile +3 −1

		@@ -3,10 +3,12 @@ ARG GO_VERSION=1.25
3	3	FROM --platform=$BUILDPLATFORM golang:${GO_VERSION} AS build
4	4	ARG TARGETOS
5	5	ARG TARGETARCH
	6	# Set by the release workflow from the git tag; --help and /api/instance report it.
	7	ARG VERSION=dev
6	8
7	9	WORKDIR /build
8	10	COPY . .
9		RUN CGO_ENABLED=0 GOARCH=${TARGETARCH} GOOS=${TARGETOS} go build -ldflags "-s -w -extldflags '-static'" && \
	11	RUN CGO_ENABLED=0 GOARCH=${TARGETARCH} GOOS=${TARGETOS} go build -ldflags "-s -w -extldflags '-static' -X main.version=${VERSION}" && \
10	12	echo "skunkyart:x:10000:10000:SkunkyArt user:/:/sbin/nologin" > /etc/minimal-passwd && \
11	13	echo "skunkyart:x:10000:" > /etc/minimal-group
12	14

README.md +3

		@@ -20,6 +20,9 @@ can also add the `-ldflags "-w -s"` argument (GCCGO has a different name for it
20	20
21	21	`go build -tags embed -ldflags "-w -s"`
22	22
	23	Such a build reports its version as `dev`. To stamp one in, as the release
	24	workflow does from the git tag, add `-X main.version=<version>` to the ldflags.
	25
23	26	## Docker
24	27	Prebuilt multi-arch images (`linux/amd64`, `linux/arm64`) are published to GHCR
25	28	on every release tag:

SETUP.md +4

		@@ -16,6 +16,10 @@ Time units:
16	16	runs as, and SkunkyArt refuses to start if it is not. The container image
17	17	runs as uid 10000, so a bind-mounted cache needs
18	18	`sudo chown -R 10000:10000 <dir>` on the host.
	19	* `memcache` — Also keep served media in RAM, on top of the on-disk cache.
	20	Entries are scored by how often they are requested and dropped once they go
	21	a round unused. The cache is bounded only by that scoring, so leave it off
	22	unless you have RAM to spare for your traffic.
19	23	* `lifetime` — Cached file life time, requires numeric value, followed by multiplicative suffix (see Time Units for details)
20	24	* `max-size` — Maximum file size in megabytes
21	25	* `update-interval` — Automatic rotation interval

app/cache.go +145 −66

		@@ -6,7 +6,9 @@ import (
6	6	"crypto/sha1" //nolint:gosec // G505: SHA-1 is a cache-key hash here, not a security primitive
7	7	"encoding/hex"
8	8	"io"
	9	"net/url"
9	10	"os"
	11	"regexp"
10	12	"strings"
11	13	"sync"
12	14	"syscall"
		@@ -18,90 +20,139 @@ type file struct {
18	20	Content []byte
19	21	}
20	22
	23	// tempFS is the in-memory media cache, guarded by mx. A plain Mutex rather than
	24	// an RWMutex on purpose: every operation here mutates something (a read bumps
	25	// Score), and the previous code took an RLock to write, which is not exclusive.
21	26	var tempFS = make(map[[20]byte]*file)
22		var mx = &sync.RWMutex{}
	27	var mx sync.Mutex
	28
	29	// memGet returns the cached body for key and raises its score so that popular
	30	// entries outlive the janitor, or nil when the entry is absent or still empty.
	31	func memGet(key [20]byte) []byte {
	32	mx.Lock()
	33	defer mx.Unlock()
	34
	35	f := tempFS[key]
	36	if f == nil \|\| f.Content == nil {
	37	return nil
	38	}
	39	f.Score += 2
	40	return f.Content
	41	}
	42
	43	// memPut caches body under key. An empty body is not cached, so a failed fetch
	44	// cannot poison the cache with a zero-length image.
	45	func memPut(key [20]byte, body []byte) {
	46	if len(body) == 0 {
	47	return
	48	}
	49
	50	mx.Lock()
	51	defer mx.Unlock()
	52	tempFS[key] = &file{Content: body}
	53	}
	54
	55	// InitMemCacheJanitor ages the in-memory cache forever, dropping entries whose
	56	// score has run out. Run it in its own goroutine, once, and only when memcache
	57	// is enabled.
	58	//
	59	// One loop ages the whole map. The previous design started a goroutine per
	60	// cached file, each looping until its own entry was evicted, and each touching
	61	// the map without holding mx — a concurrent map read and write, which the Go
	62	// runtime treats as a fatal error that recover cannot catch.
	63	func InitMemCacheJanitor() {
	64	for {
	65	time.Sleep(1 * time.Minute)
	66	ageMemCache()
	67	}
	68	}
	69
	70	// ageMemCache runs one round of aging: every entry loses a point, and entries
	71	// that are already out of points are dropped. An entry starts at zero, so a body
	72	// nothing asks for again is gone within a round.
	73	func ageMemCache() {
	74	mx.Lock()
	75	defer mx.Unlock()
	76
	77	for k, f := range tempFS {
	78	if f.Score <= 0 {
	79	delete(tempFS, k)
	80	continue
	81	}
	82	f.Score--
	83	}
	84	}
	85
	86	// mediaSubdomain matches the one hostname label wixmp media URLs vary: a hex
	87	// string, sometimes with dashes. Anything outside that set is rejected rather
	88	// than escaped, because this label is what selects the host to fetch from.
	89	var mediaSubdomain = regexp.MustCompile(`^[a-zA-Z0-9-]+$`)
	90
	91	// buildMediaURL returns the wixmp CDN URL for one media item, reporting false
	92	// when subdomain is not a bare hostname label.
	93	//
	94	// subdomain and path arrive already percent-decoded from the request path, so
	95	// they can carry the characters that end a host. Concatenated into a URL string,
	96	// a subdomain of "x@attacker.example#" reparses as host attacker.example, with
	97	// "images-wixmp-x" demoted to userinfo and the intended host to a fragment —
	98	// pointing the fetch at whatever the caller names, including addresses reachable
	99	// only from the instance itself.
	100	func buildMediaURL(subdomain, path, token string) (string, bool) {
	101	if !mediaSubdomain.MatchString(subdomain) {
	102	return "", false
	103	}
	104
	105	// Fields rather than concatenation: String escapes the path, so a decoded
	106	// "#" or "?" in it stays part of the path instead of ending it. The host is
	107	// checked above rather than escaped, because url.URL passes it through
	108	// verbatim.
	109	u := url.URL{
	110	Scheme: "https",
	111	Host: "images-wixmp-" + subdomain + ".wixmp.com",
	112	Path: "/" + path,
	113	}
	114	if token != "" {
	115	u.RawQuery = url.Values{"token": {token}}.Encode()
	116	}
	117	return u.String(), true
	118	}
23	119
24	120	// DownloadAndSendMedia proxies one image from DeviantArt's wixmp CDN to the
25	121	// client, serving it from the on-disk or in-memory cache when enabled. It
26	122	// responds 403 when proxying is turned off for this instance.
27	123	func (s skunkyart) DownloadAndSendMedia(subdomain, path string) {
28		var url strings.Builder
29		url.WriteString("https://images-wixmp-")
30		url.WriteString(subdomain)
31		url.WriteString(".wixmp.com/")
32		url.WriteString(path)
33		if t := s.Args.Get("token"); t != "" {
34		url.WriteString("?token=")
35		url.WriteString(t)
	124	mediaURL, ok := buildMediaURL(subdomain, path, s.Args.Get("token"))
	125	if !ok {
	126	s.ReturnHTTPError(400)
	127	return
36	128	}
37	129
38	130	var response []byte
39	131
40	132	switch {
41	133	case CFG.Cache.Enabled:
42		fileName := sha1.Sum([]byte(subdomain + path)) //nolint:gosec // G401: cache-key hash, not a security primitive
43		filePath := CFG.Cache.Path + "/" + hex.EncodeToString(fileName[:])
44
45		c := func() {
46		// filePath is built from a SHA-1 of the request, not from user input,
47		// so it cannot escape the cache directory.
48		file, err := os.Open(filePath) //nolint:gosec // G304: path is a hash, not user-controlled
49		if err != nil {
50		dwnld := Download(url.String())
51		if dwnld.Status == 200 && strings.HasPrefix(dwnld.Headers.Get("Content-Type"), "image") {
52		response = dwnld.Body
53		try(os.WriteFile(filePath, response, 0600))
54		} else {
55		s.ReturnHTTPError(dwnld.Status)
56		return
57		}
58		} else {
59		defer func() { try(file.Close()) }()
60		file, e := io.ReadAll(file)
61		try(e)
62		response = file
63		}
64		}
	134	key := sha1.Sum([]byte(subdomain + path)) //nolint:gosec // G401: cache-key hash, not a security primitive
	135	filePath := CFG.Cache.Path + "/" + hex.EncodeToString(key[:])
65	136
66	137	if CFG.Cache.MemCache {
67		mx.Lock()
68		if tempFS[fileName] == nil {
69		tempFS[fileName] = &file{}
70		}
71		mx.Unlock()
72
73		if tempFS[fileName].Content != nil {
74		response = tempFS[fileName].Content
75		tempFS[fileName].Score += 2
	138	if cached := memGet(key); cached != nil {
	139	response = cached
76	140	break
77		} else {
78		c()
79		go func() {
80		defer restore()
81
82		mx.RLock()
83		tempFS[fileName].Content = response
84		mx.RUnlock()
85
86		for {
87		time.Sleep(1 * time.Minute)
88
89		mx.Lock()
90		if tempFS[fileName].Score <= 0 {
91		delete(tempFS, fileName)
92		mx.Unlock()
93		return
94		}
95		tempFS[fileName].Score--
96		mx.Unlock()
97		}
98		}()
99	141	}
100		} else {
101		c()
	142	}
	143
	144	body, ok := s.loadOrFetchMedia(filePath, mediaURL)
	145	if !ok {
	146	// loadOrFetchMedia has already written the error response.
	147	return
	148	}
	149	response = body
	150
	151	if CFG.Cache.MemCache {
	152	memPut(key, response)
102	153	}
103	154	case CFG.Proxy:
104		dwnld := Download(url.String())
	155	dwnld := Download(mediaURL)
105	156	if dwnld.Status != 200 {
106	157	s.ReturnHTTPError(dwnld.Status)
107	158	return
		@@ -115,6 +166,34 @@ func (s skunkyart) DownloadAndSendMedia(subdomain, path string) {
115	166	_, _ = s.Writer.Write(response)
116	167	}
117	168
	169	// loadOrFetchMedia returns the media body for filePath, preferring the on-disk
	170	// cache and falling back to fetching mediaURL, which it then writes back to the
	171	// cache. It reports false when it has already written an error response, so the
	172	// caller must not write anything further.
	173	func (s skunkyart) loadOrFetchMedia(filePath, mediaURL string) ([]byte, bool) {
	174	// filePath is built from a SHA-1 of the request, not from user input, so it
	175	// cannot escape the cache directory.
	176	if f, err := os.Open(filePath); err == nil { //nolint:gosec // G304: path is a hash, not user-controlled
	177	defer func() { try(f.Close()) }()
	178
	179	if body, err := io.ReadAll(f); err == nil {
	180	return body, true
	181	} else {
	182	// An unreadable cache entry is not fatal; re-fetch it instead.
	183	try(err)
	184	}
	185	}
	186
	187	dwnld := Download(mediaURL)
	188	if dwnld.Status != 200 \|\| !strings.HasPrefix(dwnld.Headers.Get("Content-Type"), "image") {
	189	s.ReturnHTTPError(dwnld.Status)
	190	return nil, false
	191	}
	192
	193	try(os.WriteFile(filePath, dwnld.Body, 0600))
	194	return dwnld.Body, true
	195	}
	196
118	197	// InitCacheSystem runs the cache rotation loop forever, evicting files past
119	198	// their lifetime and emptying the cache when it outgrows max-size. Run it in its
120	199	// own goroutine.

app/cache_test.go added +207

		@@ -0,0 +1,207 @@
	1	package app
	2
	3	import (
	4	"bytes"
	5	"net/http/httptest"
	6	"net/url"
	7	"sync"
	8	"testing"
	9	)
	10
	11	// resetMemCache empties the in-memory cache so each test starts clean.
	12	func resetMemCache() {
	13	mx.Lock()
	14	defer mx.Unlock()
	15	tempFS = make(map[[20]byte]*file)
	16	}
	17
	18	func key(b byte) [20]byte {
	19	var k [20]byte
	20	k[0] = b
	21	return k
	22	}
	23
	24	// TestBuildMediaURLRejectsForgedSubdomain is the regression test for the SSRF in
	25	// the media proxy: subdomain reaches us percent-decoded from the request path,
	26	// so it can carry "@", "#", "?" and "/" — every character that ends a host. When
	27	// the URL was built by concatenation, each of these reparsed as a host the
	28	// caller chose. The label is the host, so it has to be rejected, not escaped.
	29	func TestBuildMediaURLRejectsForgedSubdomain(t *testing.T) {
	30	// The path a request for /media/file/<subdomain>/f.jpg would decode to.
	31	for _, subdomain := range []string{
	32	"x@attacker.example#", // userinfo + fragment: host is attacker.example
	33	"x@attacker.example/", // userinfo, host terminated by the slash
	34	"x@127.0.0.1:8080/", // the same, aimed inside the instance's network
	35	"x@[::1]:8080/", // IPv6 loopback
	36	"attacker.example#", // fragment alone truncates to images-wixmp-attacker.example
	37	"attacker.example?", // query does the same
	38	"a/../../secret", // slashes escape the label entirely
	39	"a\\attacker.example", // backslash, which some parsers fold to "/"
	40	"a.wixmp.com.attacker.eu", // dots: a label may not contain them
	41	"", // empty label
	42	} {
	43	if got, ok := buildMediaURL(subdomain, "f/x.jpg", ""); ok {
	44	t.Errorf("subdomain %q: accepted and built %q, want rejected", subdomain, got)
	45	}
	46	}
	47	}
	48
	49	// TestBuildMediaURLKeepsHostOnWixmp is the property that actually matters: for
	50	// anything accepted, the host the client ends up talking to is the CDN.
	51	func TestBuildMediaURLKeepsHostOnWixmp(t *testing.T) {
	52	got, ok := buildMediaURL("ed30a86b-8c4c-a887", "f/x.jpg", "abc")
	53	if !ok {
	54	t.Fatal("a plain hex-and-dash label was rejected, want accepted")
	55	}
	56
	57	u, err := url.Parse(got)
	58	if err != nil {
	59	t.Fatalf("built an unparseable URL %q: %v", got, err)
	60	}
	61	if u.Host != "images-wixmp-ed30a86b-8c4c-a887.wixmp.com" {
	62	t.Errorf("host is %q, want the wixmp CDN", u.Host)
	63	}
	64	if u.User != nil {
	65	t.Errorf("URL carries userinfo %v, want none", u.User)
	66	}
	67	if u.Query().Get("token") != "abc" {
	68	t.Errorf("token is %q, want abc", u.Query().Get("token"))
	69	}
	70	}
	71
	72	// TestBuildMediaURLEscapesPath checks that the path cannot end the URL early and
	73	// smuggle in a query or fragment of the caller's choosing.
	74	func TestBuildMediaURLEscapesPath(t *testing.T) {
	75	got, ok := buildMediaURL("ed30a86b", "f/x.jpg#frag?q=1", "")
	76	if !ok {
	77	t.Fatal("a plain label was rejected, want accepted")
	78	}
	79
	80	u, err := url.Parse(got)
	81	if err != nil {
	82	t.Fatalf("built an unparseable URL %q: %v", got, err)
	83	}
	84	if u.Fragment != "" {
	85	t.Errorf("path opened a fragment %q, want it escaped into the path", u.Fragment)
	86	}
	87	if u.RawQuery != "" {
	88	t.Errorf("path opened a query %q, want it escaped into the path", u.RawQuery)
	89	}
	90	if u.Path != "/f/x.jpg#frag?q=1" {
	91	t.Errorf("path is %q, want it preserved verbatim", u.Path)
	92	}
	93	}
	94
	95	// TestDownloadAndSendMediaRejectsForgedSubdomain drives the handler itself, to
	96	// pin down that a forged label is refused before any fetch is attempted rather
	97	// than merely being rejected by the helper. Proxying is enabled here, so the
	98	// pre-fix handler would have reached the network on this input.
	99	func TestDownloadAndSendMediaRejectsForgedSubdomain(t *testing.T) {
	100	proxy := CFG.Proxy
	101	CFG.Proxy = true
	102	defer func() { CFG.Proxy = proxy }()
	103
	104	w := httptest.NewRecorder()
	105	s := skunkyart{Writer: w, Host: "http://localhost", Args: url.Values{}}
	106	s.DownloadAndSendMedia("x@127.0.0.1:8080/", "f/x.jpg")
	107
	108	if w.Code != 400 {
	109	t.Errorf("status is %d, want 400 for a forged subdomain", w.Code)
	110	}
	111	}
	112
	113	// TestMemCacheConcurrentAccess hammers the in-memory cache from many goroutines
	114	// while the janitor ages it, which is what a media flood does on an instance
	115	// with memcache enabled.
	116	//
	117	// This is the regression test for the readers that touched tempFS without
	118	// holding mx: concurrently with the janitor's delete that is a concurrent map
	119	// read and map write, which the runtime reports as a fatal error that no
	120	// recover can catch. Run under -race to also catch the unsynchronised field
	121	// access that does not happen to trip the map check.
	122	func TestMemCacheConcurrentAccess(t *testing.T) {
	123	resetMemCache()
	124	defer resetMemCache()
	125
	126	const workers, rounds = 24, 200
	127	body := []byte("not-really-an-image")
	128
	129	var wg sync.WaitGroup
	130	for w := range workers {
	131	wg.Go(func() {
	132	for i := range rounds {
	133	// Overlapping keys, so goroutines contend for the same entries.
	134	k := key(byte((w + i) % 8)) //nolint:gosec // G115: (w+i)%8 is 0-7
	135	memPut(k, body)
	136	memGet(k)
	137	}
	138	})
	139	}
	140
	141	// Age the cache underneath the readers and writers: this is the delete that
	142	// the old per-entry goroutines raced against.
	143	wg.Go(func() {
	144	for range rounds {
	145	ageMemCache()
	146	}
	147	})
	148
	149	wg.Wait()
	150	}
	151
	152	// TestMemGetReturnsStoredBody covers the plain hit and miss paths.
	153	func TestMemGetReturnsStoredBody(t *testing.T) {
	154	resetMemCache()
	155	defer resetMemCache()
	156
	157	k := key(1)
	158	if got := memGet(k); got != nil {
	159	t.Fatalf("empty cache: got %q, want nil", got)
	160	}
	161
	162	want := []byte("body")
	163	memPut(k, want)
	164
	165	got := memGet(k)
	166	if !bytes.Equal(got, want) {
	167	t.Fatalf("after put: got %q, want %q", got, want)
	168	}
	169	}
	170
	171	// TestMemPutIgnoresEmptyBody stops a failed fetch from caching a zero-length
	172	// image that would then be served to everyone until it aged out.
	173	func TestMemPutIgnoresEmptyBody(t *testing.T) {
	174	resetMemCache()
	175	defer resetMemCache()
	176
	177	k := key(2)
	178	memPut(k, nil)
	179	memPut(k, []byte{})
	180
	181	if got := memGet(k); got != nil {
	182	t.Fatalf("empty body was cached: got %q, want nil", got)
	183	}
	184	}
	185
	186	// TestAgeMemCacheEvicts checks that a cold entry is dropped while a hot one
	187	// survives, since that scoring is the only bound on the cache's memory use.
	188	func TestAgeMemCacheEvicts(t *testing.T) {
	189	resetMemCache()
	190	defer resetMemCache()
	191
	192	cold, hot := key(3), key(4)
	193	memPut(cold, []byte("cold"))
	194	memPut(hot, []byte("hot"))
	195
	196	// A hit raises the hot entry's score above zero.
	197	memGet(hot)
	198
	199	ageMemCache()
	200
	201	if got := memGet(cold); got != nil {
	202	t.Errorf("cold entry survived aging: got %q, want nil", got)
	203	}
	204	if got := memGet(hot); got == nil {
	205	t.Error("hot entry was evicted after a hit, want it kept")
	206	}
	207	}

app/config.go +3

		@@ -127,6 +127,9 @@ func ExecuteConfig() {
127	127	// XOR (1026), not exponentiation — so the cap was ~1000x too small.
128	128	CFG.Cache.MaxSize = 1024 1024
129	129	go InitCacheSystem()
	130	if CFG.Cache.MemCache {
	131	go InitMemCacheJanitor()
	132	}
130	133	}
131	134
132	135	About = instanceAbout{

main.go +10 −1

		@@ -8,8 +8,17 @@ import (
8	8	"github.com/zerolabsco/devianter"
9	9	)
10	10
	11	// version is the release this binary was built from. The release workflow links
	12	// it in from the git tag so that --help and /api/instance cannot drift from the
	13	// tag the image was built at:
	14	//
	15	// go build -ldflags "-X main.version=1.3.7"
	16	//
	17	// A plain `go build` leaves it as "dev".
	18	var version = "dev"
	19
11	20	func main() {
12		app.Release.Version = "1.3.2"
	21	app.Release.Version = version
13	22	app.Release.Description = "Two API endpoints and template embedding into binary"
14	23
15	24	app.ExecuteCommandLineArguments()

Commit 184ce2c6dc

Commit `184ce2c6dc`