diff --git a/backend/app/rest/api/rest_public.go b/backend/app/rest/api/rest_public.go index 48981e3b..914b82b3 100644 --- a/backend/app/rest/api/rest_public.go +++ b/backend/app/rest/api/rest_public.go @@ -364,12 +364,36 @@ 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. +func safePictureSegment(seg string) bool { + if seg == "" || seg == "." || seg == ".." { + return false + } + if strings.ContainsAny(seg, "/\\\x00") { + return false + } + if strings.Contains(seg, "..") { + return false + } + return true +} + // GET /picture/{user}/{id} - get picture func (s *public) loadPictureCtrl(w http.ResponseWriter, r *http.Request) { - id := chi.URLParam(r, "user") + "/" + chi.URLParam(r, "id") + user, imgID := chi.URLParam(r, "user"), chi.URLParam(r, "id") + if user == "" || imgID == "" || !safePictureSegment(user) || !safePictureSegment(imgID) { + log.Printf("[WARN] rejected picture request with unsafe id segments user=%q id=%q", user, imgID) + rest.SendErrorJSON(w, r, http.StatusBadRequest, fmt.Errorf("invalid picture id"), "invalid picture id", rest.ErrAssetNotFound) + return + } + id := user + "/" + imgID img, err := s.imageService.Load(id) if err != nil { - rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't get image "+id, rest.ErrAssetNotFound) + log.Printf("[WARN] can't load image %s: %v", id, err) + rest.SendErrorJSON(w, r, http.StatusBadRequest, fmt.Errorf("image not found"), "can't get image", rest.ErrAssetNotFound) return } // enforce client-side caching @@ -431,7 +455,7 @@ func (s *public) telegramQrCtrl(w http.ResponseWriter, r *http.Request) { } w.Header().Set("Content-Type", "image/png") - if _, err = w.Write(png); err != nil { + if _, err = w.Write(png); err != nil { //nolint:gosec // png bytes from go-qrcode, not HTML log.Printf("[WARN] can't render qr, %v", err) } } diff --git a/backend/app/rest/api/rest_public_test.go b/backend/app/rest/api/rest_public_test.go index 0f97e28b..646c0298 100644 --- a/backend/app/rest/api/rest_public_test.go +++ b/backend/app/rest/api/rest_public_test.go @@ -1028,3 +1028,41 @@ func TestRest_Robots(t *testing.T) { "Allow: /api/v1/list\nAllow: /api/v1/config\nAllow: /api/v1/user\nAllow: /api/v1/img\n"+ "Allow: /api/v1/avatar\nAllow: /api/v1/picture\n", body) } + +// TestRest_LoadPictureRejectsPathTraversal reproduces the unauthenticated path-traversal +// vulnerability in GET /api/v1/picture/{user}/{id}. Before the fix, the handler concatenated +// the URL params verbatim into a filesystem path via path.Join, so a request like +// `/api/v1/picture/../remark.db` would resolve to `/../remark.db`, escaping the image +// directory. Even when the file did not exist (default Partitions=100 mitigates direct hits), +// the FS error message leaked the constructed internal path back to the unauthenticated caller. +func TestRest_LoadPictureRejectsPathTraversal(t *testing.T) { + ts, _, teardown := startupT(t) + defer teardown() + + cases := []struct { + name string + path string + }{ + {name: "dotdot in user segment", path: "/api/v1/picture/../remark.db"}, + {name: "dotdot in id segment", path: "/api/v1/picture/dev_user/..%2Fremark.db"}, + {name: "encoded dotdot in user segment", path: "/api/v1/picture/%2E%2E/remark.db"}, + } + 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.NotContains(t, s, "..", "error body must not echo traversal marker") + assert.NotContains(t, s, "remark.db", "error body must not echo attacker-supplied filename") + assert.NotContains(t, s, "no such file", "error body must not leak filesystem state") + assert.NotContains(t, s, "/var/", "error body must not leak internal filesystem path") + }) + } +}