Slice-bounds-out-of-range panic in share cleanup loop (mutating slice while ranging) denies share-listing endpoints

Status

Repository / Component

Plain-English Description

Listing shares can crash the request whenever two or more shares in the queried set have expired, because a cleanup loop deletes entries while looping over the list incorrectly. A low-privilege user can trigger this on demand and also break the admin view that lists everyone's shares.

Description of the Underlying Issue

The share store cleanup loops iterate over a slice while reassigning it to a shorter slice inside the loop. At share/storage.go:39-46 (All), and identically at :59-66 (FindByUserID) and :101-108 (Gets), the code does `for i, link := range links { if expired { s.Delete(...); links = append(links[:i], links[i+1:]...) } }`. The `range` binds its length to the original N, but each expired iteration shrinks `links`. Once the loop index i exceeds the current len(links), the expression `links[i+1:]` evaluates to `links[i+1:len(links)]` with low>high, causing a runtime 'slice bounds out of range' panic. GetsByPath (:114-115) calls All() and inherits the bug. These functions are reached synchronously by GET /api/shares (shareListHandler, http/share.go:31-40) and GET /api/share (shareGetsHandler, :59-68). FileBrowser installs only a CSP middleware and no panic-recovery middleware (http/http.go:29-35, http/data.go:50-85), so the panic aborts the request.

Potential Attack

Any user holding Perm.Share + Perm.Download (gated by withPermShare, http/share.go:21-29) POSTs two shares for a path with a 1-second expiry (Expires/Unit are attacker-controlled, http/share.go:121-140), waits just over a second, then calls GET /api/shares. FindByUserID's cleanup loop hits the slice-bounds panic and drops the request/connection (500). Because the admin listing path uses All()/GetsByPath and processes every user's shares, the same two-expired-shares condition from any single user causes the admin's global share listing to panic as well.

Outcomes of Potential Attack

Repeatable denial of service against the share-listing endpoints (GET /api/shares, GET /api/share) for the affected user, and — via All()/GetsByPath — poisoning of the admin global share listing so administrators cannot enumerate shares while any user holds >=2 expired shares. The panic is caught by net/http's per-connection recover(), so the process does not crash and the loop deletes expired links before panicking (partially self-healing), but the condition is re-triggerable at will.

Affected Scope

share/storage.go All() (:39-46), FindByUserID() (:59-66), Gets() (:101-108), and GetsByPath()->All() (:114-115); reached synchronously by GET /api/shares (shareListHandler, http/share.go:31-40) and GET /api/share (shareGetsHandler, http/share.go:59-68).

Suggested Fix (plain english)

Stop deleting items from the list while looping over that same list; build a fresh list of the shares that are still valid instead.

Suggested Fix (detailed)

Do not mutate a slice while ranging over it. Rewrite the cleanup in share/storage.go All(), FindByUserID(), and Gets() to either (a) build a new result slice by appending only non-expired links (deleting expired ones via s.Delete along the way), (b) collect the hashes to delete and rebuild the slice afterward, or (c) iterate the index downward (for i := len(links)-1; i >= 0; i--). Verify with a case of >=2 (and 3) expired links that no panic occurs and all expired links are deleted. As defense-in-depth, add panic-recovery middleware in the http router (http/http.go) so an unexpected panic returns 500 instead of dropping the connection. Do not mutate a slice while ranging over it. Build a fresh result slice by appending only non-expired links, or collect hashes to delete and rebuild afterward, or iterate an index downward. Add panic-recovery middleware as defense-in-depth.

Validation

Deterministically reachable slice-bounds panic proven by full source trace across three storage functions; on-demand triggerable by low-privilege share users and able to poison the admin listing. Recoverable per-connection (no full crash), so severity held at medium.

first_opinion

Full Evidence

share/storage.go:39-46 All(): for i,link := range links { ... links = append(links[:i], links[i+1:]...) }

share/storage.go:59-66 FindByUserID() identical loop; :101-108 Gets() identical loop; :114-115 GetsByPath() calls All()

http/share.go:37/39/65/67 admin->All()/GetsByPath, non-admin->FindByUserID/Gets

http/share.go:101-170 sharePostHandler: expire=time.Now().Add(add).Unix() (:139), one Save per POST (:165); Expires/Unit attacker-controlled (:121-140)

http/data.go:50-85 handle() and http/http.go:29-35 router install only a CSP middleware -> no panic-recovery middleware

Proven fact: The loop ranges over links (length fixed at original N) while reassigning links to a shorter slice each expired iteration; once loop index i exceeds current len(links), the slice expression links[i+1:] == links[i+1:len(links)] has low>high and panics with 'slice bounds out of range'.

Proven fact: Deterministic trace with all links expired: N=2 panics at i=1 (links[2:] on len=1); N=3 panics at i=2 (links[3:] on len<=2); reachable whenever >=2 links in the queried set are expired.

Proven fact: Reachable on demand: any user with Perm.Share+Perm.Download (withPermShare, http/share.go:21-29) can POST two shares with expires=1 second then list them; admin All()/GetsByPath processes ALL users' shares, so any user's >=2 expired shares also panic the admin global listing.

Proven fact: No handler-level panic recovery in filebrowser; panic is raised inside the request goroutine.

Unvalidated fact: net/http installs a per-connection recover(), so the panic aborts the single request/connection rather than crashing the process; the loop deletes expired links before panicking, so it is partially self-healing, but it is re-triggerable at will and can repeatedly deny the share-listing endpoints and poison the admin global listing.

share/storage.go:39-46 All(): `for i, link := range links { if link.Expire != 0 && link.Expire <= time.Now().Unix() { s.Delete(link.Hash); links = append(links[:i], links[i+1:]...) } }` — verified in source; range length fixed to original N while links is reassigned shorter.

share/storage.go:59-66 FindByUserID() identical loop; :101-108 Gets() identical loop; :114-115 GetsByPath() calls All().

http/share.go:37/39/65/67 admin->All()/GetsByPath, non-admin->FindByUserID/Gets.

http/share.go:101-170 sharePostHandler: expire=time.Now().Add(add).Unix() (:139), one Save per POST (:165); Expires/Unit attacker-controlled (:121-140).

http/data.go:50-85 handle() and http/http.go:29-35 router install only a CSP middleware -> no panic-recovery middleware.

Proven fact: The loop ranges over links (length fixed at original N) while reassigning links to a shorter slice each expired iteration; once i exceeds current len(links), links[i+1:] has low>high and panics with 'slice bounds out of range'.

Proven fact: Reachable on demand: any Perm.Share+Perm.Download user can POST two shares with expires=1s then list them; admin All()/GetsByPath processes all users' shares, so any user's >=2 expired shares also panic the admin global listing.

Proven fact: No handler-level panic recovery exists; the panic is raised inside the request goroutine.

Unvalidated fact: net/http installs a per-connection recover(), so the panic aborts the single request/connection rather than crashing the process; the loop deletes expired links before panicking (partially self-healing), but it is re-triggerable at will and can repeatedly deny the share-listing endpoints and poison the admin global listing.

share/storage.go:39-46 All(): `for i, link := range links { ... links = append(links[:i], links[i+1:]...) }` (panic at :44)

share/storage.go:59-66 FindByUserID(): identical loop (panic at :64)

share/storage.go:101-108 Gets(): identical loop (panic at :106)

share/storage.go:114-115 GetsByPath() calls s.All() and thus inherits the panic (admin path)

http/share.go:36-39 shareListHandler: admin->All(), non-admin->FindByUserID(d.user.ID)

http/share.go:64-67 shareGetsHandler: admin->GetsByPath(path), non-admin->Gets(path,d.user.ID)

http/share.go:21-29 withPermShare gate (Perm.Share && Perm.Download only)

http/share.go:101-170 sharePostHandler allows multiple expiring shares (incl. multiple for the same Path)

http/http.go:74-75 routes GET /api/shares and GET /api/share; http/http.go:29-35 installs only a CSP middleware (no panic-recovery middleware), so panics reach net/http's per-conn recover

Go slice semantics: `s[k:]` == `s[k:len(s)]`; with k>len(s) the bound check low<=high fails -> panic regardless of cap; the last backing slot is never overwritten because append writes at most index len(v)-2