Security: Add authentication to MediaServer and re-enable DoS protection#512
Security: Add authentication to MediaServer and re-enable DoS protection#512r3352 wants to merge 1 commit intogoogle:masterfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
e93fa64 to
d200b4a
Compare
d200b4a to
f70f6d5
Compare
| commBufRead.flip(); | ||
| } | ||
| currByte = (commBufRead.get() & 0xFF); | ||
| // if (readTillPanic-- < 0) |
There was a problem hiding this comment.
There's a reason I disabled this, but it was probably 20+ years ago and I don't remember why...so I don't want to just blindly re-enable it.
There was a problem hiding this comment.
Fair point — I don't want to blindly re-enable something you intentionally disabled either. The concern is that without it, a client can send an arbitrarily long line and the readLineBytes() loop will keep appending to the buffer until memory is exhausted. If you remember the original reason being something like legitimate clients needing to send large payloads, we could increase DOS_BYTE_LIMIT to something more generous (say 10MB instead of 1MB) while still capping it. Would that be a reasonable middle ground?
There was a problem hiding this comment.
No, picking a specific value for it that's higher isn't going to assuage my concerns. I don't see any reason this would get to be huge...I really don't see the benefit of accidentally breaking something because I forgot why I disabled this vs. blocking an attack vector that isn't secure in the first place that's been open for 20+ years.
There was a problem hiding this comment.
Makes sense, I'll drop the readTillPanic change. Not worth risking breakage for marginal benefit on something that's been open this long.
| } | ||
| commBufRead = java.nio.ByteBuffer.allocate(4096); | ||
| commBufWrite = java.nio.ByteBuffer.allocate(4096); | ||
| // Shared-secret authentication check |
There was a problem hiding this comment.
Where is this code on any client side?
There was a problem hiding this comment.
Good question — there's no corresponding client-side implementation for this. The media_server_auth_key check is new server-side code that would need a matching client update to send the key on connect. As written, it's opt-in: when the property is empty (default), auth is skipped entirely, so existing clients continue to work unchanged. But you're right that for the auth gate to actually be useful, the client side (MediaServerConnection.java handles client connections to MediaServer) would need to read the same property and send the key as the first line. I can add that if you'd like, or if you'd prefer a different auth approach entirely, I'm open to suggestions.
There was a problem hiding this comment.
Where do you see MediaServerConnection.java? Most of the client side code for the media server is in other native clients, such as the source filter in DirectShow, the C code for the mini client on the extenders, the STV file handler in MPlayer, etc. And getting the actual auth key into any of that code is going to be complex.
Also, this isn't really a good auth technique as it's just sending it in plain text and not encrypting the channel before doing something like that. So overall, I don't think this change is really buying us anything.
There was a problem hiding this comment.
You're right on both fronts. I was wrong about MediaServerConnection.java and hadn't considered that most clients are native code (DirectShow, MPlayer, extender firmware). Getting auth into all of those would be a way bigger lift than a server-side change, and sending a key in plaintext without TLS doesn't really add meaningful security anyway. Closing this one out.
Summary
media_server_auth_keyis configured, clients must send the key as the first line before commands are acceptedreadTillPaniccounter), preventing memory exhaustion via oversized command linesVulnerability Details
CWE-306 (Missing Authentication for Critical Function)
MediaServer.java:161-177binds to0.0.0.0:7818and accepts connections inConnection.run()(line 981) with no authentication handshake. The server immediately processes commands includingOPEN(file read),WRITEOPEN(file write),LISTW/LISTRECURSIVEW(directory listing), andXCODE_SETUP(transcoder initialization). While file access is gated bycheckFileAccess(), the connection itself requires no credentials.Additionally, the DoS protection at lines 357-360 was commented out, allowing unbounded input lines to consume server memory.
Changes
media_server_auth_key. If the key matches,OKis sent and commands proceed. If it doesn't match, the connection is closed.media_server_auth_keyis empty (default), auth is skipped entirely — existing deployments continue to work without configuration changes.readTillPaniccounter that limits individual command lines toDOS_BYTE_LIMIT(1MB).