Fix horizontal authorization bypass on /download (IDOR) - #11
Open
thistehneisen wants to merge 1 commit into
Open
Fix horizontal authorization bypass on /download (IDOR)#11thistehneisen wants to merge 1 commit into
thistehneisen wants to merge 1 commit into
Conversation
`downloadHandler` previously only checked that the requester held a valid
session and that the requested filename did not contain ".." or "/".
It then `http.ServeFile`'d any matching file under upload/processed/.
That let any signed-in user read any other user's processed report by
asking for the (predictable, timestamp-prefixed) filename — a horizontal
authorization bypass affecting the confidentiality of every user's
indicator lists and query results.
This patch:
* Tightens the filename filter (rejects "", "\\", and NUL in addition
to ".." and "/").
* Looks up the requester's account and verifies the requested filename
appears in their own Uploads.Out list before serving.
* Returns 404 (with a server-side log) for both rejected filenames and
cross-user attempts, instead of the previous silent empty response.
Verified:
* Attacker (User B) requesting User A's processed file → HTTP 404,
server log "Unauthorized download attempt".
* Owner (User A) requesting their own file → HTTP 200, 834-byte report,
unchanged behaviour.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix horizontal authorization bypass on /download (IDOR)
Reported by: OffSeq — Nils Putniņš
<npu@offseq.com>Affected version: v2.6.1 (
a2a794ef6916d16da3fcc2f9f8d86225482e7263)What this PR fixes
downloadHandlerpreviously verified only:..or/.It then called
http.ServeFile(w, r, config.Upload.Path+"/processed/"+file)for any matching file. There was no check that the requester actually
owned that file.
Filenames in
upload/processed/follow the deterministic pattern<timestamp>-<original-name>, where the timestamp comes from a1-second-resolution clock (
upload.go:141). Any signed-in user who canguess (or simply enumerate) the timestamped name of another user's
processed report can fetch the full JSON / table output:
The reports contain the user's full uploaded indicator list and the
results of querying every backend data source — sensitive material for
a threat-intel platform.
The fix
\\, and\x00ontop of the existing
..//check.appears in their own
Uploads.Outlist before serving.404 Not Found(with a server-sideERRlog) for both rejectedfilenames and cross-user attempts. The previous code returned an empty
response with no status, which silently obscured the bypass.
Single file changed, +32 / -1 lines.
Repro
Pre-patch
Post-patch
Same script (
pocs/03_idor_download.sh):Server log entry:
Positive control (owner downloading their own file) still works:
Notes
db.getAccount(username)is called once per/downloadrequest. Thesame cost is paid by
indexHandlerand other authenticated handlers,and the Mongo collection is small / indexed, so the overhead is
negligible. If a future hot path matters, the
online[]map alreadycaches the account for users with an active WebSocket and could be
consulted first.
account.Uploads.Outis currently scanned linearly. Lists are boundedby
upload.deleteInterval/upload.deleteExpirationso this is fine inpractice; a
map[string]struct{}could replace it later if profilesgrow.
Nils Putniņš / npu[at]offseq[dot]com / OffSeq
https://offseq.com / https://radar.offseq.com