Bug 81361 – [WebSocket]A client MUST close a connection if it detects a masked frame

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Li Yin]

From RFC6455, A server MUST NOT mask any frames that it sends to the client. 
A client MUST close a connection if it detects a masked frame.

[Li Yin]

Created attachment 132303 [details]
Patch

[Li Yin]

We can get the notes from the followed address.
http://tools.ietf.org/html/rfc6455#section-5.1

[Yuta Kitamura]

Comment on attachment 132303 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132303&action=review

This is a good thing to do. Thanks for doing this.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:647
> +    if (frame.masked) {
> +        fail("A server MUST NOT mask any frames that it sends to the client.");
> +        return false;
> +    }
> +

This check should be done inside parseFrame(), and you can safely remove unmasking code there (line 595-600).

You can also remove the stale comment in WebSocketChannel.h ("May modify part of ...").

[Li Yin]

Created attachment 132417 [details]
Patch

[Li Yin]

Hi tkent,yuta,
  Can you review this patch? 
  Thanks.

[Yuta Kitamura]

Comment on attachment 132417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132417&action=review

Except for some style issues, the patch looks fine to me. (Please wait for comments from a reviewer, as I'm not a WebKit reviewer yet.)

> Source/WebCore/ChangeLog:7
> +        A client MUST close a connection if it detects a masked frame
> +        https://bugs.webkit.org/show_bug.cgi?id=81361
> +        A server MUST NOT mask any frames that it sends to the client.
> +
> +        Reviewed by NOBODY (OOPS!).

Usual ChangeLog format is:

  <bug title>
  <bug URL>

  Reviewed by ...

  <more description>

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:562
> +        fail("A server MUST NOT mask any frames that it sends to the client.");

Nitpick: Capital "MUST NOT" sounds too exaggerated; just small letters would work fine.

[Li Yin]

Created attachment 132983 [details]
Patch

[Li Yin]

Add a new test case to track whether the browser failed the connection or not, when it received the masked frame from the server.

In addition, rename the test case masked-frame* into unmasked-frame*, and using unmasked frames to replace masked frames.

[Yuta Kitamura]

Comment on attachment 132983 [details]
Patch

Looks fine to me. Please wait for comments from a reviewer.

[Kent Tamura]

Comment on attachment 132983 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 132983 [details]
Patch

Clearing flags on attachment: 132983

Committed r111829: <http://trac.webkit.org/changeset/111829>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.