Skip to content

Require device authentication on the board refresh stream - #15

Open
IbraTech04 wants to merge 1 commit into
minbar-featuresfrom
sec/board-stream-device-auth
Open

IbraTech04 wants to merge 1 commit into
minbar-featuresfrom
sec/board-stream-device-auth

Conversation

@IbraTech04

Copy link
Copy Markdown
Collaborator

The flaw

/api/refresh-musallahboard performed no credential check. Its only gates were that
?deviceId=<uuid> named a known device and that the device was not revoked:

UUID deviceId = parseDeviceId(session);          // straight out of the query string
Optional<Device> device = deviceRepository.findById(deviceId);
if (device.isEmpty()) { close(session, CLOSE_UNKNOWN_DEVICE); return; }
if (device.get().getRevokedAt() != null) { close(session, CLOSE_REVOKED); return; }
session.getAttributes().put(DEVICE_ATTR, deviceId);
WebSocketSession previous = sessions.put(deviceId, session);   // ← you are now that board

The device id is a v4 UUID, but it is not a secret. The kiosk passes it as a query parameter,
so it sits in the board's own URL and in any screenshot of the display, and
GET /api/admin/board/devices hands it to every operator. Treating an identifier as a
credential is the bug.

WebSecurityConfig has .requestMatchers("/api/refresh-musallahboard").permitAll(), and the
setAllowedOrigins(frontendBaseUrl, musallahBoardBaseUrl) on the handler registration is not
a substitute. Origin restriction constrains browsers. A non-browser client sends no Origin
header at all and Spring accepts the upgrade — the PoC below is a plain java.net.http
WebSocket client with no Origin, and it connected fine.

What an attacker could do

Connect once with a real board's device id, and sessions.put(deviceId, session) closes the
legitimate kiosk's socket with 1000 superseded. The kiosk reconnects; do it in a loop and
that physical display is permanently starved of refresh pushes and falls back to its
ten-minute poll. Meanwhile the attacker receives that board's contentChanged,
configChanged and refreshAll stream.

What this builds

The channel now runs the same Ed25519 challenge–response AgentWebSocketHandler already
used on /api/agent/ws:

open      → server sends hello (seq 1) with a 32-byte SecureRandom challenge
recv auth → verify Ed25519 signature over the canonical string, check skew and revocation;
            on success bind deviceId and send auth_ok (seq 2)
(silence) → closed 4008 auth_timeout
recv ?    → 4001 auth_failed / 4002 seq_violation / 4003 bad_state / 4004 bad_frame
  • The signed string binds sessionId + challenge + deviceId + timestamp, with its own
    version prefix musallahboard-stream-auth-v1 — deliberately different from the agent
    channel's musallahboard-auth-v1, so a signature made for one channel cannot verify on the
    other. There is a test for that.
  • The verifying key is always loaded from the enrolled Device row. Nothing in the frame
    influences which key is used.
  • Five-minute timestamp skew cap, monotonic per-session seq, sessionId echo required.
  • Unknown, revoked, keyless and badly-signed devices all close 4001. The close code must not
    tell an unauthenticated caller which device ids are real.
  • ?deviceId= is now ignored entirely.

Supersede semantics are preserved, only moved behind authentication. Newest-wins on auth
success, so a reloading kiosk still evicts its own stale socket; and the two-arg
sessions.remove(deviceId, session) still stops a late-closing superseded session from
evicting its replacement. Both have tests.

Why this design over the alternative

The brief offered two options: mirror the challenge–response, or mint a short-lived stream
credential at enrollment. I mirrored the handshake, for three reasons.

  1. No new secret, and no new schema. Enrollment already registers an Ed25519 public key on
    the Device row. That key is the credential. A minted token would have meant a new column
    on devices, therefore parallel V10 migrations in both db/migration/sqlite/ and
    db/migration/postgresql/, therefore a ddl-auto=validate startup failure as a live
    failure mode if either half were wrong. This change ships with zero migrations.
  2. A token in a query string is the flaw again, one layer down. The natural way for a
    kiosk to carry a minted token is ?token=, which lands in access logs and proxy logs the
    same way the device id already did. The signature never appears in a URL.
  3. Replay. A bearer token is replayable by anyone who observes it. A signature over a
    server-issued, single-use challenge is not — and there is a test asserting a signature
    captured on one session fails on another.

The cost is that the client must hold a private key, which the browser-hosted kiosk page
cannot. That is covered in CLIENT_MIGRATION.md §5.

Verification

Tests: 596 pass, 0 failures, 0 errors (./mvnw test). Baseline on minbar-features was
575; BoardStreamHandlerTest goes from 12 tests to 33, and DeviceAdminControllerTest picks
up an assertion.

BoardStreamHandlerTest covers both halves the brief asked for explicitly:

  • namingARealDeviceIdWithoutSigningDeliversNothing — the exact pre-fix exploit. Connects
    with a real enrolled device id, never signs, and receives no refresh from a broadcast, a
    targeted push, or a manual refresh.
  • unauthenticatedConnectionCannotSupersedeALiveBoard — the denial-of-service half. An
    unauthenticated connection naming a live board's id does not close it.
  • authenticatedBoardReceivesAuthOkAndThenPushes — the positive path still works.

Plus rejection of: a signature from a key the device does not own, an unknown device, a
revoked device, a device with no key on file, a stale timestamp, a signature replayed from
another session, an agent-channel signature (domain separation), a mismatched sessionId, a
non-monotonic seq, a replayed seq, an unparseable frame, an unknown frame type, an
oversized frame, and re-authentication after auth_ok.

Live server, port 8108, verified end to end. Seeded one enrolled device into a private
copy of the dev DB (the shared /c/Code/LensBridgeBackend/ tree was not modified) and drove
it with a throwaway java.net.http WebSocket client — wscat/websocat are not installed
here. Not committed.

Attack, sending no Origin header and naming the real device id:

[open] upgrade accepted
[recv] {"type":"hello","seq":1,"sessionId":"d9fc9ab6-...","challenge":"gtcjmE33...","serverTime":1787710567435}
[anon] staying silent, as the old exploit did
[close] code=4008 reason=auth_timeout

Legitimate board:

[send] {"type":"auth","seq":1,"sessionId":"93a71f7d-...","deviceId":"e4abcef9-...","timestamp":...,"signature":"ZoYE/shr..."}
[recv] {"type":"auth_ok","seq":2,"sessionId":"93a71f7d-...","deviceId":"e4abcef9-..."}

Supersede, live — only an authenticated second connection evicts the first:

board stream device e4abcef9-... superseded session 94255fd2-... with ceccb074-...
[close] code=1000 reason=superseded          ← on the first session

And an unknown device id closes 4001 auth_failed, identical to a bad signature.

Review findings, all fixed

/code-review high returned six. All are in this branch:

  • MEDIUM — BoardStreamSession.send caught only JsonProcessingException and
    IOException. Tomcat throws an unchecked IllegalStateException when a socket goes bad
    between the isOpen() check and the write, which would have aborted the broadcast loop so
    every board later in iteration order silently missed the refresh, and then escaped
    PosterService.create to fail the admin's HTTP request after the poster was already saved.
    Now catches Exception. Test: aBoardThatThrowsOnWriteDoesNotStopTheBroadcast.
  • MEDIUM — outbound seq was allocated outside the transport monitor, so two request
    threads pushing to the same board could write seq 5 before seq 4 — a regression a client
    validating the sequence would reconnect over. Allocation moved inside the monitor. Test:
    outboundSeqIncreasesMonotonicallyAcrossPushes.
  • LOW — markAuthenticated threw IllegalStateException if the transport dropped
    mid-handshake. Now returns false and the handler deregisters quietly.
  • LOW — the frame-size guard used getPayloadLength(), which UTF-8-encodes the whole
    payload to answer, allocating a second copy of exactly the oversized frame it was refusing.
    Now measures characters.
  • LOW — reaper interval and auth timeout both defaulted to 15 s, giving an effective
    ceiling near 30 s rather than the documented 15. Reaper interval defaulted to 5 s and the
    real ceiling documented.
  • LOW — CLIENT_MIGRATION.md omitted the socials reason that
    PromotableSocialMediaService actually pushes. Added.

While fixing those I also wired BoardStreamHandler.closeIfPresent into
DeviceAdminController.revoke. Revocation is checked at handshake time, so without it a board
revoked mid-session kept receiving pushes until it happened to reconnect — the agent channel
already closed on revoke via AgentSessionRegistry.closeIfPresent, and now that this channel
binds a device identity it owes the same guarantee.

Two items the review raised and I deliberately left: revocation still races the handshake in a
very narrow window (identical shape on the agent channel), and CloseStatus.SESSION_NOT_RELIABLE
is a reserved code that Tomcat remaps (copied verbatim from the existing AgentSession).

Breaking change

CLIENT_MIGRATION.md is the spec for the Raspberry Pi kiosk agent, whose source is not in this
repository. It gives the full wire protocol: every frame shape and field rule, the exact bytes
to sign with worked Go and Python examples, ordering, all close codes and what to do about
each, the Origin header requirement, frame size limits, reconnect rules, and migration order.

Close codes changed meaning — 4001 was device_id_required, 4002 unknown_device, 4003
device_revoked. Anything matching on the old numbers must be updated.

There is no dual-accept grace period. Boards fall back to their ten-minute poll in the gap, so
screens go stale rather than blank.

Untouched, as instructed: AgentWebSocketHandler, /api/agent/ws,
config/stomp/StompJwtChannelInterceptor.java, and the frontend repo.

🤖 Generated with Claude Code

https://claude.ai/code/session_011xQusSt39vP3c82gAUXRCm

/api/refresh-musallahboard accepted any connection that could name a
known, unrevoked ?deviceId=<uuid>. A device id is an identifier, not a
credential: the kiosk passes it in its own URL, it is readable off the
physical display, and GET /api/admin/board/devices lists it. Anyone
holding one could open a socket as that board, which superseded the real
kiosk's session (closing it 1000 "superseded") and made a reconnect loop
starve the display of refresh pushes, while the attacker received that
board's contentChanged / configChanged / refreshAll stream.

Origin restriction was not a defence here. setAllowedOrigins constrains
browsers only; a non-browser client sends no Origin header at all and
Spring accepts the upgrade, which the PoC confirms.

The channel now runs the same Ed25519 challenge-response as
AgentWebSocketHandler: a server-generated 32-byte SecureRandom challenge
per session, a canonical signed string binding sessionId + challenge +
deviceId + timestamp under its own domain prefix
(musallahboard-stream-auth-v1), a five-minute skew cap, a monotonic seq,
and a public key always read from the enrolled Device row rather than
from the frame. Unknown, revoked, keyless and badly-signed devices all
close 4001 so the code is not a device-id oracle. Unauthenticated
sockets are reaped at 4008.

Supersede semantics are unchanged, only moved behind authentication:
newest-wins on auth success, and the two-arg sessions.remove still stops
a late-closing superseded session from evicting its replacement.

No schema change, so no migration: the credential already exists on the
device row from enrollment.

Also from review:
- BoardStreamSession.send catches unchecked errors, so one dying board
  cannot abort a fleet-wide broadcast or fail the admin request that
  triggered it, and allocates its seq inside the write monitor so
  concurrent pushes cannot deliver seq 5 before seq 4.
- markAuthenticated returns false instead of throwing when the transport
  drops mid-handshake.
- Incoming frames are capped at 4096 characters before parsing.
- Revoking a device now closes its refresh-stream session too, not just
  its agent session.

CLIENT_MIGRATION.md documents the new wire protocol for the Raspberry Pi
kiosk agent, whose source lives outside this repository.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011xQusSt39vP3c82gAUXRCm
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@IbraTech04

Copy link
Copy Markdown
Collaborator Author

This has uncovered a more serious flaw that needs further investigation and thought before fully merging/committing to a solution :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant