fix(api): reject path traversal and sanitise error in /picture/{user}/{id}
The unauthenticated GET /api/v1/picture/{user}/{id} handler concatenated the
two URL params verbatim into a filesystem path via path.Join, so a request
like /api/v1/picture/../remark.db resolved to <base>/../remark.db, escaping
the image directory. With Partitions=0 (a documented option) this is a
direct arbitrary-file read; with the default Partitions=100 the constructed
path lands in a CRC-derived subdirectory but the server still leaks the
internal filesystem path back to the unauthenticated caller via the JSON
error body — confirmed against demo.remark42.com (master-80c12a3) which
returned `stat /var/folders/.../staging/.../remark.db` for `..` requests.
Validate both URL segments via safePictureSegment (no traversal markers,
no path separators, no NULs) at the handler entry, and replace the raw
storage error with a generic "image not found" response. The original
error is logged for operators.
Reproduction test asserts that ../remark.db, foo/..%2Fremark.db and
%2E%2E/remark.db all return 400 with no internal path leaked.
This commit is contained in:
committed by
Umputun
parent
ddcb2c7b5f
commit
59c92f8c4d
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 `<base>/../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")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user