diff --git a/backend/app/rest/api/rest_public.go b/backend/app/rest/api/rest_public.go index 914b82b3..b9491942 100644 --- a/backend/app/rest/api/rest_public.go +++ b/backend/app/rest/api/rest_public.go @@ -11,6 +11,7 @@ import ( "strconv" "strings" "time" + "unicode" "github.com/go-chi/chi/v5" cache "github.com/go-pkgz/lcw/v2" @@ -365,19 +366,27 @@ func (s *public) listCtrl(w http.ResponseWriter, r *http.Request) { } // safePictureSegment reports whether seg is acceptable as a path segment in -// the picture URL (no traversal markers, no path separators, no NULs). Picture -// IDs are server-generated hashes plus a known extension, so any value carrying -// these characters is hostile and must be rejected before reaching the store. +// the picture URL (no traversal markers, no path separators, no control +// characters). Picture IDs are server-generated hashes plus a known +// extension, so any value carrying these characters is hostile and must be +// rejected before reaching the store. Rejecting controls (CR, LF, TAB, NUL, +// etc.) also closes a log-injection vector since the rejected segment is +// echoed into the access log. func safePictureSegment(seg string) bool { if seg == "" || seg == "." || seg == ".." { return false } - if strings.ContainsAny(seg, "/\\\x00") { + if strings.ContainsAny(seg, "/\\") { return false } if strings.Contains(seg, "..") { return false } + for _, r := range seg { + if unicode.IsControl(r) { + return false + } + } return true } diff --git a/backend/app/rest/api/rest_public_test.go b/backend/app/rest/api/rest_public_test.go index 646c0298..54bca27a 100644 --- a/backend/app/rest/api/rest_public_test.go +++ b/backend/app/rest/api/rest_public_test.go @@ -1066,3 +1066,41 @@ func TestRest_LoadPictureRejectsPathTraversal(t *testing.T) { }) } } + +// TestRest_LoadPictureRejectsControlCharsInSegment makes sure a CRLF / tab / NUL +// in the URL segment is rejected by safePictureSegment. Without the rejection +// the [WARN] log line constructed from %q-formatted segments would still be +// safe (Go's %q escapes control chars), but a future log change to %s would +// turn this into log forgery — and no legitimate picture id ever needs control +// characters, so the right place to slam the door is in the validator. +func TestRest_LoadPictureRejectsControlCharsInSegment(t *testing.T) { + ts, _, teardown := startupT(t) + defer teardown() + + cases := []struct { + name string + path string + }{ + {name: "lf in user segment", path: "/api/v1/picture/dev%0Auser/abc.png"}, + {name: "cr in user segment", path: "/api/v1/picture/dev%0Duser/abc.png"}, + {name: "tab in user segment", path: "/api/v1/picture/dev%09user/abc.png"}, + {name: "lf in id segment", path: "/api/v1/picture/dev_user/abc%0A.png"}, + {name: "nul in id segment", path: "/api/v1/picture/dev_user/abc%00.png"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + req, err := http.NewRequest(http.MethodGet, ts.URL+c.path, http.NoBody) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + s := string(body) + assert.Contains(t, s, "invalid picture id", "must reject as invalid input, not fall through to storage") + assert.NotContains(t, s, "no such file", "must not reach the filesystem") + }) + } +}