fix(anti-hijack): validate cookie_id against DB on every authed read
Closes the post-recovery re-attack window. Previously cookies were authenticated purely cryptographically — once a hijacker received a signed cookie for student_id=X, that cookie remained valid forever (until QUIZ_SECRET_KEY rotated), even after admin clear-student + legit re-claim issued a fresh cookie_id for X. Now /me, /event, and /ws/student all check that the cookie's cookie_id matches participants.cookie_id for the (sid, student_id). Mismatch -> 401 + Set-Cookie clearing for HTTP, ws.close(4001) for WS. The legit re-claim wins because admin clear_student deletes the row and the next join inserts the new student's cookie_id; the hijacker's cookie now fails the DB check on every subsequent request. Test: tests/test_anti_cheat.py::test_post_recovery_old_cookie_is_dead covers the full hijack -> clear -> re-claim -> hijacker-locked-out sequence end to end.
This commit is contained in:
16
app/room.py
16
app/room.py
@@ -189,6 +189,22 @@ class RoomManager:
|
|||||||
+ sum(len(clients) for clients in self.projector_clients.values())
|
+ sum(len(clients) for clients in self.projector_clients.values())
|
||||||
)
|
)
|
||||||
|
|
||||||
|
async def cookie_id_matches(self, sid: str, student_id: str, cookie_id: str) -> bool:
|
||||||
|
"""Check the student's signed cookie_id against the DB participant
|
||||||
|
row. Used to defend against the post-recovery re-attack: after
|
||||||
|
admin clears a hijacked id and the legitimate student re-joins
|
||||||
|
with a fresh cookie_id, the original hijacker's cookie is still
|
||||||
|
cryptographically valid (the secret key is unchanged), but the
|
||||||
|
DB cookie_id now belongs to the legit student. We reject any
|
||||||
|
request whose cookie_id doesn't match the current row."""
|
||||||
|
async with connect(self.settings.db_path) as db:
|
||||||
|
cur = await db.execute(
|
||||||
|
"SELECT cookie_id FROM participants WHERE sid = ? AND student_id = ?",
|
||||||
|
(sid, student_id),
|
||||||
|
)
|
||||||
|
row = await cur.fetchone()
|
||||||
|
return row is not None and row["cookie_id"] == cookie_id
|
||||||
|
|
||||||
async def add_participant(self, sid: str, student_id: str, name: str, cookie_id: str) -> None:
|
async def add_participant(self, sid: str, student_id: str, name: str, cookie_id: str) -> None:
|
||||||
"""First-claim-wins. Raises DuplicateStudentId if this student_id
|
"""First-claim-wins. Raises DuplicateStudentId if this student_id
|
||||||
is already in the participants table for this sid (an attempt to
|
is already in the participants table for this sid (an attempt to
|
||||||
|
|||||||
@@ -101,6 +101,10 @@ def router(settings: Settings, rooms: RoomManager) -> APIRouter:
|
|||||||
identity = auth.get_student_identity(settings, request, sid)
|
identity = auth.get_student_identity(settings, request, sid)
|
||||||
if not identity:
|
if not identity:
|
||||||
raise HTTPException(status_code=401, detail="Student cookie required")
|
raise HTTPException(status_code=401, detail="Student cookie required")
|
||||||
|
if not await rooms.cookie_id_matches(sid, identity["student_id"], identity["cookie_id"]):
|
||||||
|
# Same defence as /me: a stale post-recovery cookie should
|
||||||
|
# not be able to pollute the audit log.
|
||||||
|
raise HTTPException(status_code=401, detail="Re-join required")
|
||||||
await rooms.log_event(
|
await rooms.log_event(
|
||||||
sid,
|
sid,
|
||||||
student_id=identity["student_id"],
|
student_id=identity["student_id"],
|
||||||
@@ -120,17 +124,18 @@ def router(settings: Settings, rooms: RoomManager) -> APIRouter:
|
|||||||
identity = auth.get_student_identity(settings, request, sid)
|
identity = auth.get_student_identity(settings, request, sid)
|
||||||
if not identity:
|
if not identity:
|
||||||
raise HTTPException(status_code=401, detail="Student cookie required")
|
raise HTTPException(status_code=401, detail="Student cookie required")
|
||||||
try:
|
# Validate cookie_id against DB. Two cases this catches:
|
||||||
return await rooms.me(sid, identity["student_id"])
|
# (a) participant row is gone (session reset, admin clear, DB
|
||||||
except KeyError:
|
# rebuild) → cookie_id_matches returns False → 401 + cleared.
|
||||||
# Cookie's student_id is no longer in the DB (e.g. session reset
|
# (b) participant row exists but with a different cookie_id (a
|
||||||
# or DB rebuilt while the cookie persisted). Send 401 with the
|
# prior hijacker's cookie still cryptographically valid
|
||||||
# cookie cleared so the client renders the join form. We build
|
# after the legit student re-claimed via admin recovery)
|
||||||
# the JSONResponse directly because raising HTTPException would
|
# → 401 + cleared. The hijacker's stale cookie is now dead.
|
||||||
# bypass the cookie mutation.
|
if not await rooms.cookie_id_matches(sid, identity["student_id"], identity["cookie_id"]):
|
||||||
resp = JSONResponse({"detail": "Re-join required"}, status_code=401)
|
resp = JSONResponse({"detail": "Re-join required"}, status_code=401)
|
||||||
resp.delete_cookie(auth.STUDENT_COOKIE, path="/")
|
resp.delete_cookie(auth.STUDENT_COOKIE, path="/")
|
||||||
return resp
|
return resp
|
||||||
|
return await rooms.me(sid, identity["student_id"])
|
||||||
|
|
||||||
@api.get("/api/session/{sid}/stats")
|
@api.get("/api/session/{sid}/stats")
|
||||||
async def stats(sid: str, request: Request, question_idx: int | None = None):
|
async def stats(sid: str, request: Request, question_idx: int | None = None):
|
||||||
@@ -145,6 +150,12 @@ def router(settings: Settings, rooms: RoomManager) -> APIRouter:
|
|||||||
if not identity or not await rooms.session_exists(sid):
|
if not identity or not await rooms.session_exists(sid):
|
||||||
await websocket.close(code=4001)
|
await websocket.close(code=4001)
|
||||||
return
|
return
|
||||||
|
# cookie_id-vs-DB check closes the post-recovery re-attack window:
|
||||||
|
# a hijacker's WS won't authenticate after the legit student has
|
||||||
|
# re-claimed their id via admin clear-student.
|
||||||
|
if not await rooms.cookie_id_matches(sid, identity["student_id"], identity["cookie_id"]):
|
||||||
|
await websocket.close(code=4001)
|
||||||
|
return
|
||||||
await rooms.student_ws(websocket, sid, identity)
|
await rooms.student_ws(websocket, sid, identity)
|
||||||
|
|
||||||
# ---- Projector view (public, read-only) -------------------------------
|
# ---- Projector view (public, read-only) -------------------------------
|
||||||
|
|||||||
@@ -74,6 +74,42 @@ def test_admin_clear_student_404s_when_no_match(client, sid):
|
|||||||
assert client.delete("/admin/api/students/nobody").status_code == 404
|
assert client.delete("/admin/api/students/nobody").status_code == 404
|
||||||
|
|
||||||
|
|
||||||
|
def test_post_recovery_old_cookie_is_dead(client, sid):
|
||||||
|
"""Hijack -> recovery flow: after admin clears a hijacked id and the
|
||||||
|
legitimate student re-claims with a fresh cookie_id, the original
|
||||||
|
hijacker's still-cryptographically-valid cookie must NOT continue to
|
||||||
|
authenticate. The DB cookie_id check is what closes that gap."""
|
||||||
|
Hijacker = client.__class__
|
||||||
|
hijacker = Hijacker(client.app)
|
||||||
|
response = hijacker.post(f"/api/session/{sid}/join", json={"student_id": "alice", "name": "Hijacker"})
|
||||||
|
assert response.status_code == 200
|
||||||
|
# Hijacker's cookie/me works while they hold the slot.
|
||||||
|
assert hijacker.get(f"/api/session/{sid}/me").status_code == 200
|
||||||
|
|
||||||
|
# Admin clears the hijacked id; legit student re-claims with a fresh
|
||||||
|
# browser (= fresh cookie jar = fresh signed cookie_id).
|
||||||
|
admin_login(client)
|
||||||
|
assert client.delete(f"/admin/api/students/alice").status_code == 200
|
||||||
|
legit = Hijacker(client.app)
|
||||||
|
response = legit.post(f"/api/session/{sid}/join", json={"student_id": "alice", "name": "Real Alice"})
|
||||||
|
assert response.status_code == 200
|
||||||
|
assert legit.get(f"/api/session/{sid}/me").json()["name"] == "Real Alice"
|
||||||
|
|
||||||
|
# The hijacker's old (still-cryptographically-valid) cookie now fails
|
||||||
|
# auth because its cookie_id doesn't match the DB row anymore.
|
||||||
|
response = hijacker.get(f"/api/session/{sid}/me")
|
||||||
|
assert response.status_code == 401
|
||||||
|
# And the cookie should be cleared so their browser bounces back to
|
||||||
|
# the join form rather than retrying with the dead cookie.
|
||||||
|
assert any(
|
||||||
|
h.lower() == "set-cookie" and "qz_student" in v and ("max-age=0" in v.lower() or "expires=" in v.lower())
|
||||||
|
for h, v in response.headers.items()
|
||||||
|
), response.headers
|
||||||
|
# Same goes for the audit-event endpoint.
|
||||||
|
response = hijacker.post(f"/api/session/{sid}/event", json={"kind": "blur"})
|
||||||
|
assert response.status_code == 401
|
||||||
|
|
||||||
|
|
||||||
def test_submit_lockout_is_server_enforced(client, sid):
|
def test_submit_lockout_is_server_enforced(client, sid):
|
||||||
"""Server-side: a second submit for the same (sid, student_id, qidx)
|
"""Server-side: a second submit for the same (sid, student_id, qidx)
|
||||||
returns the *original* ack rather than overwriting the answer. The
|
returns the *original* ack rather than overwriting the answer. The
|
||||||
|
|||||||
Reference in New Issue
Block a user