novnc / websockify

Websockify is a WebSocket to TCP proxy/bridge. This allows a browser to connect to any application/server/service.
GNU Lesser General Public License v3.0
3.94k stars 779 forks source link

websockify accepts unmasked frames #164

Closed DirectXMan12 closed 9 years ago

DirectXMan12 commented 9 years ago

Cross-filed from https://bugs.launchpad.net/nova/+bug/1278342

Currently, websockify will happily accept unmasked frames, simply logging a debug message when it gets one (see websockify/websockify.py:240).

However, according to RFC 6455 (section 5.1),

The server MUST close the connection upon receiving a frame that is not masked.

For compatibility reasons, we should continue to have an option to have the current behavior. However, we should also probably introduce a "strict mode" (on by default in new releases) which follows the RFC and rejects unmasked frames.

@sdague, @kanaka , @samhed , @astrand : thoughts on the proposed solution?

kanaka commented 9 years ago

Yes, making it refuse unmasked frames from the client by default is fine.

Some historical context: the HyBi working group went back and forth for a long time on whether client to server frames should be masked and whether it should be mandated. The masking is only relevant in a browser to server context and it was acknowledged that websocket clients may include more than just browsers in which case client to server masking is worse than useless because it provides no security benefit and is just CPU overhead to mask/unmask. Even in a browser to server context, the threat that this is addressing was really just theoretical (the main concern was that broken caching intermediaries could have their caches poisoned with malicious code). But there were important members of the working group that were unwilling to accept websockets at all without this potential threat mitigated. And because browser to server was really the scope of the working group, it was decided to include mandatory wording with a parenthetical "(These rules might be relaxed in a future specification)" to cover the future use cases that will likely arise outside of websocket to server context (or when there is consensus that the threat is no longer applicable).

samhed commented 9 years ago

Looks good to me!