diff options
Diffstat (limited to 'app')
| -rwxr-xr-x | app/cache.go | 211 | ||||
| -rw-r--r-- | app/cache_test.go | 207 | ||||
| -rwxr-xr-x | app/config.go | 3 |
3 files changed, 355 insertions, 66 deletions
diff --git a/app/cache.go b/app/cache.go index 91c9847..e4822db 100755 --- a/app/cache.go +++ b/app/cache.go @@ -6,7 +6,9 @@ import ( "crypto/sha1" //nolint:gosec // G505: SHA-1 is a cache-key hash here, not a security primitive "encoding/hex" "io" + "net/url" "os" + "regexp" "strings" "sync" "syscall" @@ -18,90 +20,139 @@ type file struct { Content []byte } +// tempFS is the in-memory media cache, guarded by mx. A plain Mutex rather than +// an RWMutex on purpose: every operation here mutates something (a read bumps +// Score), and the previous code took an RLock to write, which is not exclusive. var tempFS = make(map[[20]byte]*file) -var mx = &sync.RWMutex{} +var mx sync.Mutex + +// memGet returns the cached body for key and raises its score so that popular +// entries outlive the janitor, or nil when the entry is absent or still empty. +func memGet(key [20]byte) []byte { + mx.Lock() + defer mx.Unlock() + + f := tempFS[key] + if f == nil || f.Content == nil { + return nil + } + f.Score += 2 + return f.Content +} + +// memPut caches body under key. An empty body is not cached, so a failed fetch +// cannot poison the cache with a zero-length image. +func memPut(key [20]byte, body []byte) { + if len(body) == 0 { + return + } + + mx.Lock() + defer mx.Unlock() + tempFS[key] = &file{Content: body} +} + +// InitMemCacheJanitor ages the in-memory cache forever, dropping entries whose +// score has run out. Run it in its own goroutine, once, and only when memcache +// is enabled. +// +// One loop ages the whole map. The previous design started a goroutine per +// cached file, each looping until its own entry was evicted, and each touching +// the map without holding mx — a concurrent map read and write, which the Go +// runtime treats as a fatal error that recover cannot catch. +func InitMemCacheJanitor() { + for { + time.Sleep(1 * time.Minute) + ageMemCache() + } +} + +// ageMemCache runs one round of aging: every entry loses a point, and entries +// that are already out of points are dropped. An entry starts at zero, so a body +// nothing asks for again is gone within a round. +func ageMemCache() { + mx.Lock() + defer mx.Unlock() + + for k, f := range tempFS { + if f.Score <= 0 { + delete(tempFS, k) + continue + } + f.Score-- + } +} + +// mediaSubdomain matches the one hostname label wixmp media URLs vary: a hex +// string, sometimes with dashes. Anything outside that set is rejected rather +// than escaped, because this label is what selects the host to fetch from. +var mediaSubdomain = regexp.MustCompile(`^[a-zA-Z0-9-]+$`) + +// buildMediaURL returns the wixmp CDN URL for one media item, reporting false +// when subdomain is not a bare hostname label. +// +// subdomain and path arrive already percent-decoded from the request path, so +// they can carry the characters that end a host. Concatenated into a URL string, +// a subdomain of "[email protected]#" reparses as host attacker.example, with +// "images-wixmp-x" demoted to userinfo and the intended host to a fragment — +// pointing the fetch at whatever the caller names, including addresses reachable +// only from the instance itself. +func buildMediaURL(subdomain, path, token string) (string, bool) { + if !mediaSubdomain.MatchString(subdomain) { + return "", false + } + + // Fields rather than concatenation: String escapes the path, so a decoded + // "#" or "?" in it stays part of the path instead of ending it. The host is + // checked above rather than escaped, because url.URL passes it through + // verbatim. + u := url.URL{ + Scheme: "https", + Host: "images-wixmp-" + subdomain + ".wixmp.com", + Path: "/" + path, + } + if token != "" { + u.RawQuery = url.Values{"token": {token}}.Encode() + } + return u.String(), true +} // DownloadAndSendMedia proxies one image from DeviantArt's wixmp CDN to the // client, serving it from the on-disk or in-memory cache when enabled. It // responds 403 when proxying is turned off for this instance. func (s skunkyart) DownloadAndSendMedia(subdomain, path string) { - var url strings.Builder - url.WriteString("https://images-wixmp-") - url.WriteString(subdomain) - url.WriteString(".wixmp.com/") - url.WriteString(path) - if t := s.Args.Get("token"); t != "" { - url.WriteString("?token=") - url.WriteString(t) + mediaURL, ok := buildMediaURL(subdomain, path, s.Args.Get("token")) + if !ok { + s.ReturnHTTPError(400) + return } var response []byte switch { case CFG.Cache.Enabled: - fileName := sha1.Sum([]byte(subdomain + path)) //nolint:gosec // G401: cache-key hash, not a security primitive - filePath := CFG.Cache.Path + "/" + hex.EncodeToString(fileName[:]) - - c := func() { - // filePath is built from a SHA-1 of the request, not from user input, - // so it cannot escape the cache directory. - file, err := os.Open(filePath) //nolint:gosec // G304: path is a hash, not user-controlled - if err != nil { - dwnld := Download(url.String()) - if dwnld.Status == 200 && strings.HasPrefix(dwnld.Headers.Get("Content-Type"), "image") { - response = dwnld.Body - try(os.WriteFile(filePath, response, 0600)) - } else { - s.ReturnHTTPError(dwnld.Status) - return - } - } else { - defer func() { try(file.Close()) }() - file, e := io.ReadAll(file) - try(e) - response = file - } - } + key := sha1.Sum([]byte(subdomain + path)) //nolint:gosec // G401: cache-key hash, not a security primitive + filePath := CFG.Cache.Path + "/" + hex.EncodeToString(key[:]) if CFG.Cache.MemCache { - mx.Lock() - if tempFS[fileName] == nil { - tempFS[fileName] = &file{} - } - mx.Unlock() - - if tempFS[fileName].Content != nil { - response = tempFS[fileName].Content - tempFS[fileName].Score += 2 + if cached := memGet(key); cached != nil { + response = cached break - } else { - c() - go func() { - defer restore() - - mx.RLock() - tempFS[fileName].Content = response - mx.RUnlock() - - for { - time.Sleep(1 * time.Minute) - - mx.Lock() - if tempFS[fileName].Score <= 0 { - delete(tempFS, fileName) - mx.Unlock() - return - } - tempFS[fileName].Score-- - mx.Unlock() - } - }() } - } else { - c() + } + + body, ok := s.loadOrFetchMedia(filePath, mediaURL) + if !ok { + // loadOrFetchMedia has already written the error response. + return + } + response = body + + if CFG.Cache.MemCache { + memPut(key, response) } case CFG.Proxy: - dwnld := Download(url.String()) + dwnld := Download(mediaURL) if dwnld.Status != 200 { s.ReturnHTTPError(dwnld.Status) return @@ -115,6 +166,34 @@ func (s skunkyart) DownloadAndSendMedia(subdomain, path string) { _, _ = s.Writer.Write(response) } +// loadOrFetchMedia returns the media body for filePath, preferring the on-disk +// cache and falling back to fetching mediaURL, which it then writes back to the +// cache. It reports false when it has already written an error response, so the +// caller must not write anything further. +func (s skunkyart) loadOrFetchMedia(filePath, mediaURL string) ([]byte, bool) { + // filePath is built from a SHA-1 of the request, not from user input, so it + // cannot escape the cache directory. + if f, err := os.Open(filePath); err == nil { //nolint:gosec // G304: path is a hash, not user-controlled + defer func() { try(f.Close()) }() + + if body, err := io.ReadAll(f); err == nil { + return body, true + } else { + // An unreadable cache entry is not fatal; re-fetch it instead. + try(err) + } + } + + dwnld := Download(mediaURL) + if dwnld.Status != 200 || !strings.HasPrefix(dwnld.Headers.Get("Content-Type"), "image") { + s.ReturnHTTPError(dwnld.Status) + return nil, false + } + + try(os.WriteFile(filePath, dwnld.Body, 0600)) + return dwnld.Body, true +} + // InitCacheSystem runs the cache rotation loop forever, evicting files past // their lifetime and emptying the cache when it outgrows max-size. Run it in its // own goroutine. diff --git a/app/cache_test.go b/app/cache_test.go new file mode 100644 index 0000000..eafd8b1 --- /dev/null +++ b/app/cache_test.go @@ -0,0 +1,207 @@ +package app + +import ( + "bytes" + "net/http/httptest" + "net/url" + "sync" + "testing" +) + +// resetMemCache empties the in-memory cache so each test starts clean. +func resetMemCache() { + mx.Lock() + defer mx.Unlock() + tempFS = make(map[[20]byte]*file) +} + +func key(b byte) [20]byte { + var k [20]byte + k[0] = b + return k +} + +// TestBuildMediaURLRejectsForgedSubdomain is the regression test for the SSRF in +// the media proxy: subdomain reaches us percent-decoded from the request path, +// so it can carry "@", "#", "?" and "/" — every character that ends a host. When +// the URL was built by concatenation, each of these reparsed as a host the +// caller chose. The label is the host, so it has to be rejected, not escaped. +func TestBuildMediaURLRejectsForgedSubdomain(t *testing.T) { + // The path a request for /media/file/<subdomain>/f.jpg would decode to. + for _, subdomain := range []string{ + "[email protected]#", // userinfo + fragment: host is attacker.example + "[email protected]/", // userinfo, host terminated by the slash + "[email protected]:8080/", // the same, aimed inside the instance's network + "x@[::1]:8080/", // IPv6 loopback + "attacker.example#", // fragment alone truncates to images-wixmp-attacker.example + "attacker.example?", // query does the same + "a/../../secret", // slashes escape the label entirely + "a\\attacker.example", // backslash, which some parsers fold to "/" + "a.wixmp.com.attacker.eu", // dots: a label may not contain them + "", // empty label + } { + if got, ok := buildMediaURL(subdomain, "f/x.jpg", ""); ok { + t.Errorf("subdomain %q: accepted and built %q, want rejected", subdomain, got) + } + } +} + +// TestBuildMediaURLKeepsHostOnWixmp is the property that actually matters: for +// anything accepted, the host the client ends up talking to is the CDN. +func TestBuildMediaURLKeepsHostOnWixmp(t *testing.T) { + got, ok := buildMediaURL("ed30a86b-8c4c-a887", "f/x.jpg", "abc") + if !ok { + t.Fatal("a plain hex-and-dash label was rejected, want accepted") + } + + u, err := url.Parse(got) + if err != nil { + t.Fatalf("built an unparseable URL %q: %v", got, err) + } + if u.Host != "images-wixmp-ed30a86b-8c4c-a887.wixmp.com" { + t.Errorf("host is %q, want the wixmp CDN", u.Host) + } + if u.User != nil { + t.Errorf("URL carries userinfo %v, want none", u.User) + } + if u.Query().Get("token") != "abc" { + t.Errorf("token is %q, want abc", u.Query().Get("token")) + } +} + +// TestBuildMediaURLEscapesPath checks that the path cannot end the URL early and +// smuggle in a query or fragment of the caller's choosing. +func TestBuildMediaURLEscapesPath(t *testing.T) { + got, ok := buildMediaURL("ed30a86b", "f/x.jpg#frag?q=1", "") + if !ok { + t.Fatal("a plain label was rejected, want accepted") + } + + u, err := url.Parse(got) + if err != nil { + t.Fatalf("built an unparseable URL %q: %v", got, err) + } + if u.Fragment != "" { + t.Errorf("path opened a fragment %q, want it escaped into the path", u.Fragment) + } + if u.RawQuery != "" { + t.Errorf("path opened a query %q, want it escaped into the path", u.RawQuery) + } + if u.Path != "/f/x.jpg#frag?q=1" { + t.Errorf("path is %q, want it preserved verbatim", u.Path) + } +} + +// TestDownloadAndSendMediaRejectsForgedSubdomain drives the handler itself, to +// pin down that a forged label is refused before any fetch is attempted rather +// than merely being rejected by the helper. Proxying is enabled here, so the +// pre-fix handler would have reached the network on this input. +func TestDownloadAndSendMediaRejectsForgedSubdomain(t *testing.T) { + proxy := CFG.Proxy + CFG.Proxy = true + defer func() { CFG.Proxy = proxy }() + + w := httptest.NewRecorder() + s := skunkyart{Writer: w, Host: "http://localhost", Args: url.Values{}} + s.DownloadAndSendMedia("[email protected]:8080/", "f/x.jpg") + + if w.Code != 400 { + t.Errorf("status is %d, want 400 for a forged subdomain", w.Code) + } +} + +// TestMemCacheConcurrentAccess hammers the in-memory cache from many goroutines +// while the janitor ages it, which is what a media flood does on an instance +// with memcache enabled. +// +// This is the regression test for the readers that touched tempFS without +// holding mx: concurrently with the janitor's delete that is a concurrent map +// read and map write, which the runtime reports as a fatal error that no +// recover can catch. Run under -race to also catch the unsynchronised field +// access that does not happen to trip the map check. +func TestMemCacheConcurrentAccess(t *testing.T) { + resetMemCache() + defer resetMemCache() + + const workers, rounds = 24, 200 + body := []byte("not-really-an-image") + + var wg sync.WaitGroup + for w := range workers { + wg.Go(func() { + for i := range rounds { + // Overlapping keys, so goroutines contend for the same entries. + k := key(byte((w + i) % 8)) //nolint:gosec // G115: (w+i)%8 is 0-7 + memPut(k, body) + memGet(k) + } + }) + } + + // Age the cache underneath the readers and writers: this is the delete that + // the old per-entry goroutines raced against. + wg.Go(func() { + for range rounds { + ageMemCache() + } + }) + + wg.Wait() +} + +// TestMemGetReturnsStoredBody covers the plain hit and miss paths. +func TestMemGetReturnsStoredBody(t *testing.T) { + resetMemCache() + defer resetMemCache() + + k := key(1) + if got := memGet(k); got != nil { + t.Fatalf("empty cache: got %q, want nil", got) + } + + want := []byte("body") + memPut(k, want) + + got := memGet(k) + if !bytes.Equal(got, want) { + t.Fatalf("after put: got %q, want %q", got, want) + } +} + +// TestMemPutIgnoresEmptyBody stops a failed fetch from caching a zero-length +// image that would then be served to everyone until it aged out. +func TestMemPutIgnoresEmptyBody(t *testing.T) { + resetMemCache() + defer resetMemCache() + + k := key(2) + memPut(k, nil) + memPut(k, []byte{}) + + if got := memGet(k); got != nil { + t.Fatalf("empty body was cached: got %q, want nil", got) + } +} + +// TestAgeMemCacheEvicts checks that a cold entry is dropped while a hot one +// survives, since that scoring is the only bound on the cache's memory use. +func TestAgeMemCacheEvicts(t *testing.T) { + resetMemCache() + defer resetMemCache() + + cold, hot := key(3), key(4) + memPut(cold, []byte("cold")) + memPut(hot, []byte("hot")) + + // A hit raises the hot entry's score above zero. + memGet(hot) + + ageMemCache() + + if got := memGet(cold); got != nil { + t.Errorf("cold entry survived aging: got %q, want nil", got) + } + if got := memGet(hot); got == nil { + t.Error("hot entry was evicted after a hit, want it kept") + } +} diff --git a/app/config.go b/app/config.go index ad07aa3..74c01f7 100755 --- a/app/config.go +++ b/app/config.go @@ -127,6 +127,9 @@ func ExecuteConfig() { // XOR (1026), not exponentiation — so the cap was ~1000x too small. CFG.Cache.MaxSize *= 1024 * 1024 go InitCacheSystem() + if CFG.Cache.MemCache { + go InitMemCacheJanitor() + } } About = instanceAbout{ |