Bug 81361

Summary: [WebSocket]A client MUST close a connection if it detects a masked frame
Product: WebKit Reporter: Li Yin <li.yin>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, bashi, tkent, webkit.review.bot, yutak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Li Yin 2012-03-16 09:30:16 PDT
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.
Comment 1 Li Yin 2012-03-16 09:47:08 PDT
Created attachment 132303 [details]
Patch
Comment 2 Li Yin 2012-03-16 09:52:33 PDT
We can get the notes from the followed address.
http://tools.ietf.org/html/rfc6455#section-5.1
Comment 3 Yuta Kitamura 2012-03-16 10:19:40 PDT
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 ...").
Comment 4 Li Yin 2012-03-16 16:49:28 PDT
Created attachment 132417 [details]
Patch
Comment 5 Li Yin 2012-03-19 17:28:32 PDT
Hi tkent,yuta,
  Can you review this patch? 
  Thanks.
Comment 6 Yuta Kitamura 2012-03-20 10:31:43 PDT
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.
Comment 7 Li Yin 2012-03-21 00:19:07 PDT
Created attachment 132983 [details]
Patch
Comment 8 Li Yin 2012-03-21 00:28:05 PDT
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.
Comment 9 Yuta Kitamura 2012-03-21 11:25:51 PDT
Comment on attachment 132983 [details]
Patch

Looks fine to me. Please wait for comments from a reviewer.
Comment 10 Kent Tamura 2012-03-21 17:03:39 PDT
Comment on attachment 132983 [details]
Patch

ok
Comment 11 WebKit Review Bot 2012-03-22 23:08:57 PDT
Comment on attachment 132983 [details]
Patch

Clearing flags on attachment: 132983

Committed r111829: <http://trac.webkit.org/changeset/111829>
Comment 12 WebKit Review Bot 2012-03-22 23:09:02 PDT
All reviewed patches have been landed.  Closing bug.