Bug 82714 - WebSockets: The handshake should check for US-ASCII data instead of UTF-8.
Summary: WebSockets: The handshake should check for US-ASCII data instead of UTF-8.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks: 155602
  Show dependency treegraph
 
Reported: 2012-03-30 03:43 PDT by Jocelyn Turcotte
Modified: 2016-11-14 10:49 PST (History)
14 users (show)

See Also:


Attachments
Patch (11.86 KB, patch)
2013-02-28 12:14 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (11.85 KB, patch)
2013-02-28 13:44 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (14.87 KB, patch)
2013-02-28 14:06 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (16.07 KB, patch)
2013-03-08 09:43 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (16.62 KB, patch)
2013-03-27 19:31 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (36.93 KB, patch)
2016-03-16 13:28 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (38.81 KB, patch)
2016-03-16 16:02 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (804.26 KB, application/zip)
2016-03-16 16:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (862.93 KB, application/zip)
2016-03-16 16:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (631.41 KB, application/zip)
2016-03-16 17:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (876.46 KB, application/zip)
2016-03-16 17:07 PDT, Build Bot
no flags Details
Patch (47.04 KB, patch)
2016-03-17 15:59 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (47.50 KB, patch)
2016-03-22 10:03 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2012-03-30 03:43:47 PDT
The parsing code now allows UTF-8 data in the handshake HTTP headers. The HTTP RFC specifies that they should only contain US-ASCII and this should be reflected in the constraints.
Comment 1 Lamarque V. Souza 2013-02-28 12:14:41 PST
Created attachment 190777 [details]
Patch

Proposed patch
Comment 2 Early Warning System Bot 2013-02-28 12:19:56 PST
Comment on attachment 190777 [details]
Patch

Attachment 190777 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16768839
Comment 3 Early Warning System Bot 2013-02-28 12:20:46 PST
Comment on attachment 190777 [details]
Patch

Attachment 190777 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16792125
Comment 4 EFL EWS Bot 2013-02-28 12:38:55 PST
Comment on attachment 190777 [details]
Patch

Attachment 190777 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16801153
Comment 5 Lamarque V. Souza 2013-02-28 13:44:55 PST
Created attachment 190800 [details]
Patch

Fix warning reported by qt buildbot that caused build to fail.
Comment 6 Lamarque V. Souza 2013-02-28 14:06:27 PST
Created attachment 190804 [details]
Patch

Add missing text expectations.
Comment 7 WebKit Review Bot 2013-02-28 15:17:28 PST
Comment on attachment 190804 [details]
Patch

Attachment 190804 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16849176

New failing tests:
http/tests/websocket/tests/hybi/handshake-fail-by-prepended-null.html
Comment 8 Build Bot 2013-02-28 16:09:27 PST
Comment on attachment 190804 [details]
Patch

Attachment 190804 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16772670

New failing tests:
http/tests/websocket/tests/hybi/handshake-fail-by-prepended-null.html
Comment 9 Build Bot 2013-02-28 16:43:07 PST
Comment on attachment 190804 [details]
Patch

Attachment 190804 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16777640

New failing tests:
http/tests/websocket/tests/hybi/handshake-fail-by-prepended-null.html
Comment 10 Lamarque V. Souza 2013-03-01 06:13:16 PST
http/tests/websocket/tests/hybi/handshake-fail-by-prepended-null.html sends utf-8 string during handshake phase of its test, that is why ch-linux and mac-wk2 buildbots are red now. I do not know exactly how to deal with this issue since handshake-fail-by-prepended-null.html should not be using utf-8 according to the HTML RFC.
Comment 11 Jocelyn Turcotte 2013-03-08 06:56:45 PST
(In reply to comment #10)
> http/tests/websocket/tests/hybi/handshake-fail-by-prepended-null.html sends utf-8 string during handshake phase of its test, that is why ch-linux and mac-wk2 buildbots are red now. I do not know exactly how to deal with this issue since handshake-fail-by-prepended-null.html should not be using utf-8 according to the HTML RFC.

I can't see any non-ASCII UTF-8 char in handshake-fail-by-prepended-null_wsh.py, maybe the expectation doesn't match because of the '\0'?

This tests an handshake failure and it should still fail with you patch, so you might just have to adjust the test or the expectation to fit the new behavior.
Comment 12 Lamarque V. Souza 2013-03-08 07:22:14 PST
(In reply to comment #11)
> (In reply to comment #10)
> > http/tests/websocket/tests/hybi/handshake-fail-by-prepended-null.html sends utf-8 string during handshake phase of its test, that is why ch-linux and mac-wk2 buildbots are red now. I do not know exactly how to deal with this issue since handshake-fail-by-prepended-null.html should not be using utf-8 according to the HTML RFC.
> 
> I can't see any non-ASCII UTF-8 char in handshake-fail-by-prepended-null_wsh.py, maybe the expectation doesn't match because of the '\0'?

The line below in handshake-fail-by-prepended-null_wsh.py creates UTF-8 string, whose first byte is 0xff (or -127). Because of that the unit test I added to my patch triggers an error and the error string printed in handshake-fail-by-prepended-null-expected.txt changes from "failed: Status line contains embedded null" to "Status line contains invalid ASCII sequence".

frame = stream.create_text_frame('\0Frame-contains-thirty-two-bytes')
 
> This tests an handshake failure and it should still fail with you patch, so you might just have to adjust the test or the expectation to fit the new behavior.

Yes, it does fail with my patch. The problem is that my unit test is "shadowing" handshake-fail-by-prepended-null. My unit test tests character values below zero and handshake-fail-by-prepended-null tests characters equal to zero. However, "\0" in UTF-8 and little endian starts with 0xff (-127), not zero. Just updating handshake-fail-by-prepended-null-expected.txt is not correct. What I need is changing handshake-fail-by-prepended-null_wsh.py to create US-ASCII string instead of UTF-8.
Comment 13 Lamarque V. Souza 2013-03-08 09:43:57 PST
Created attachment 192240 [details]
Patch

Fix regression in handshake-fail-by-prepended-null.html.
Comment 14 Lamarque V. Souza 2013-03-08 09:50:24 PST
CC'ing yutak@chromium.org to comment the change in http/tests/websocket/tests/hybi/handshake-fail-by-prepended-null_wsh.py. I moved the test with stream.create_text_frame() from handshake-fail-by-prepended-null_wsh.py to handshake-fail-by-non-ascii-status-line_wsh.py (included in this patch). The handshake-fail-by-non-ascii-status-line.html unit test catches the non-US-ASCII bytes created by stream.create_text_frame(), so the error is till detected. This change as suggested by Jocelyn Turcotte. Please comment if you agree with this change.
Comment 15 Alexey Proskuryakov 2013-03-21 17:16:18 PDT
Comment on attachment 192240 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        The HTTP RFC specifies that HTTP headers should only contain US-ASCII
> +        characters. Make sure handshake headers are conformant to this
> +        constraint.

What does the WebSocket spec say about this?

Practically, clients do not enforce this for HTTP, so adding enforcement for WebSocket appears inconsistent.
Comment 16 Lamarque V. Souza 2013-03-21 19:43:02 PDT
(In reply to comment #15)
> (From update of attachment 192240 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192240&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        The HTTP RFC specifies that HTTP headers should only contain US-ASCII
> > +        characters. Make sure handshake headers are conformant to this
> > +        constraint.
> 
> What does the WebSocket spec say about this?

I have found this:

Section 1.2 of WebSocket specification [1] says status line and header fields follow what is in HTTP specification [2]. Section 4.2 of [2] says header names are "token", which are US-ASCII strings according to section 2.2 of [2]. The values can be either UTF-8 or US-ASCII by what I understood from section 4.2. The only part of status line that would be non-US-ASCII is the URI, which section 2.1 of [3] says must be US-ASCII.

> Practically, clients do not enforce this for HTTP, so adding enforcement for WebSocket appears inconsistent.

Any reason why clients do not enforce this? When you say clients you mean other implementations other than WebKit, right?

[1] http://tools.ietf.org/html/rfc6455
[2] http://tools.ietf.org/html/rfc2616
[3] http://tools.ietf.org/html/rfc2396
Comment 17 Alexey Proskuryakov 2013-03-21 20:47:21 PDT
> Any reason why clients do not enforce this? When you say clients you mean other implementations other than WebKit, right?

I haven't tested specifically, but I'm pretty sure that no browser would refuse an HTTP response with headers that have non-ASCII characters. Many will handle these "reasonably", too.
Comment 18 Takeshi Yoshino 2013-03-22 01:06:55 PDT
I prefer that we reject WebSocket object creation with bad URL data or a bad field value given by the script rather than replacing them to '?' using .ascii() method. Such transformation is not specified by the RFCs (only mention about IDN regarding the host part). It brings unexpected/confusing behavior, troubles to developers.

I don't have strong opinion on rejecting handshake with non-ASCII, as far as it's rejected, not accepted after replacing bad characters to '?' or something.
Comment 19 Lamarque V. Souza 2013-03-27 19:31:48 PDT
Created attachment 195463 [details]
Patch

Simplify checks. The changes in the source code are small, please comment.
Comment 20 Takeshi Yoshino 2013-04-02 01:40:32 PDT
(In reply to comment #19)
> Created an attachment (id=195463) [details]
> Patch
> 
> Simplify checks. The changes in the source code are small, please comment.

Good that now it fails on receiving bad handshake. (I'm not a WebKit reviewer)

You made a fix on handshake-fail-by-prepended-null_wsh.py. Does this need to make it pass with this patch? handshake-fail-by-prepended-null_wsh.py is a stale test living since HyBi 00 era. We need to delete or change it to match the latest protocol, later.
Comment 21 Lamarque V. Souza 2013-04-02 16:11:26 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > Created an attachment (id=195463) [details] [details]
> > Patch
> > 
> > Simplify checks. The changes in the source code are small, please comment.
> 
> Good that now it fails on receiving bad handshake. (I'm not a WebKit reviewer)

I investigated mozilla/firefox's source and they do more checks than I do. However, some of their checks are for ancient webservers from the time of HTTP/0.9 and for a specific webserver that does answer properly to HTTP/1.1 requests (it returns HTTP/1.0 in its status line, which my test now checks for). Anyway, probably there must be very few webservers out there that does not understand/work properly with HTTP/1.1 nowadays.

> You made a fix on handshake-fail-by-prepended-null_wsh.py. Does this need to make it pass with this patch? handshake-fail-by-prepended-null_wsh.py is a stale test living since HyBi 00 era. We need to delete or change it to match the latest protocol, later.

Yes, the change is necessary because the extra status line checks I added triggers an error when running handshake-fail-by-prepended-null.html, which causes changes to handshake-fail-by-prepended-null-expected.txt. Jocelyn Turcotte suggested that I changed handshake-fail-by-prepended-null_wsh.py the way I did to make both my tests and handshake-fail-by-prepended-null.html to live together.
Comment 22 Brent Fulgham 2016-03-14 12:09:22 PDT
Comment on attachment 195463 [details]
Patch

This change seems like a good idea, but the patch is too old to apply cleanly. I'm marking the patch r- in the hopes that this will spur someone to rebase the patch against current sources and confirm it works.
Comment 23 John Wilander 2016-03-15 10:43:00 PDT
From relevant specs …

Under https://tools.ietf.org/html/rfc6455#section-4.1

   2.  If the response lacks an |Upgrade| header field or the |Upgrade|
       header field contains a value that is not an ASCII case-
       insensitive match for the value "websocket", the client MUST
       _Fail the WebSocket Connection_.

   3.  If the response lacks a |Connection| header field or the
       |Connection| header field doesn't contain a token that is an
       ASCII case-insensitive match for the value "Upgrade", the client
       MUST _Fail the WebSocket Connection_.



Under https://tools.ietf.org/html/rfc7230#section-3.2.4

   Historically, HTTP has allowed field content with text in the
   ISO-8859-1 charset [ISO-8859-1], supporting other charsets only
   through use of [RFC2047] encoding.  In practice, most HTTP header
   field values use only a subset of the US-ASCII charset [USASCII].
   Newly defined header fields SHOULD limit their field values to
   US-ASCII octets.  A recipient SHOULD treat other octets in field
   content (obs-text) as opaque data.
Comment 24 John Wilander 2016-03-15 12:40:48 PDT
The two paragraphs from the WebSockets RFC are already addressed by the following:

bool WebSocketHandshake::checkResponseHeaders()
{
    …
    if (serverUpgrade.isNull()) {
        m_failureReason = "Error during WebSocket handshake: 'Upgrade' header is missing";
        return false;
    }
    if (serverConnection.isNull()) {
        m_failureReason = "Error during WebSocket handshake: 'Connection' header is missing";
        return false;
    }

    …

    if (!equalLettersIgnoringASCIICase(serverUpgrade, "websocket")) {
        m_failureReason = "Error during WebSocket handshake: 'Upgrade' header value is not 'WebSocket'";
        return false;
    }
    if (!equalLettersIgnoringASCIICase(serverConnection, "upgrade")) {
        m_failureReason = "Error during WebSocket handshake: 'Connection' header value is not 'Upgrade'";
        return false;
    }

    …
}
Comment 25 John Wilander 2016-03-16 13:28:58 PDT
Created attachment 274214 [details]
Patch
Comment 26 John Wilander 2016-03-16 13:31:40 PDT
The patch is based on Lamarque V. Souza's original one. Thanks!

I removed the general ASCII restriction for HTTP headers since I could not find support for such behavior in the specs.

Please review.
Comment 27 Brent Fulgham 2016-03-16 13:54:45 PDT
Comment on attachment 274214 [details]
Patch

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

I think this looks very good. I'm marking it r- for now, because the restriction to HTTP/1.1 is wrong for newer HTTP/2.0 uses. I also think that it's very likely we want to continue to support HTTP/1.0.

> Source/WebCore/ChangeLog:19
> +            - Only allow HTTP version 1.1 in status line.

Is this a requirement of the specification? I don't see anything that says we must only accept HTTP/1.1.

I do not think we should add this restriction unless the specification explicitly states that.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:451
> +    if (!httpVersionString.contains("HTTP/1.1", false)) {

Is this saying that WebSockets are not permissible with older HTTP versions?

This will break for HTTP/2.0 responses, which we support.

I think you should strip the "HTTP/" from the httpVersionString, convert to a float, and ensure it is >= 1.1.

Actually, we probably should allow HTTP/1.0 if it's not explicitly excluded in the spec (is it)?

If it's not, you can probably just check for "HTTP/0.9" like we do elsewhere to block untrusted content.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:497
> +            m_failureReason = name + " header value SHOULD only contain ASCII characters";

I don't think the "SHOULD" should be in all caps.

> LayoutTests/ChangeLog:14
> +            - This test case was renamed "error-event-ready-state-non-existent-url-with-server-responding-404" to make it clear it relies on a server responding with HTTP 1.1 404.

See my note above -- I don't think we want to block HTTP/1.0 unless the specification says we should.
Comment 28 Brent Fulgham 2016-03-16 13:58:23 PDT
Comment on attachment 274214 [details]
Patch

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

>> Source/WebCore/ChangeLog:19
>> +            - Only allow HTTP version 1.1 in status line.
> 
> Is this a requirement of the specification? I don't see anything that says we must only accept HTTP/1.1.
> 
> I do not think we should add this restriction unless the specification explicitly states that.

I'm clearly wrong! WebSockets are built on top of HTTP/1.0, so I agree we want this restriction. However, I don't think we want to limit it to *only* 1.1, we want it to be "1.1 or newer".
Comment 29 Brent Fulgham 2016-03-16 13:58:47 PDT
Comment on attachment 274214 [details]
Patch

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

>>> Source/WebCore/ChangeLog:19
>>> +            - Only allow HTTP version 1.1 in status line.
>> 
>> Is this a requirement of the specification? I don't see anything that says we must only accept HTTP/1.1.
>> 
>> I do not think we should add this restriction unless the specification explicitly states that.
> 
> I'm clearly wrong! WebSockets are built on top of HTTP/1.0, so I agree we want this restriction. However, I don't think we want to limit it to *only* 1.1, we want it to be "1.1 or newer".

I meant it's built on top of "HTTP/1.1".
Comment 30 John Wilander 2016-03-16 14:16:02 PDT
RFC 6455 says:
The method of the request MUST be GET, and the HTTP version MUST be at least 1.1.

Then there's the expired IETF draft for WebSocket over HTTP/2:
draft https://tools.ietf.org/html/draft-hirano-httpbis-websocket-over-http2-01

> This will break for HTTP/2.0 responses, which we support.

Potentially. HTTP/2 uses the same kind of HTTP 1.1 upgrade pattern as WebSocket for http:// connections. For https:// connections it uses TLS Application-Layer Protocol Negotiation Extension (https://tools.ietf.org/html/rfc7301). I'm not sure how that comes into play here. We need to set up tests.

We should not support HTTP less than 1.1 but until we know how to support HTTP/2 we should probably wait.

> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:497
> > +            m_failureReason = name + " header value SHOULD only contain ASCII characters";
> 
> I don't think the "SHOULD" should be in all caps.

OK, will fix.
Comment 31 John Wilander 2016-03-16 16:02:41 PDT
Created attachment 274231 [details]
Patch
Comment 32 Build Bot 2016-03-16 16:46:16 PDT
Comment on attachment 274231 [details]
Patch

Attachment 274231 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/991041

New failing tests:
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-protocol.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-protocol-header.html
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-extensions.html
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-accept.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-extensions-header.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-accept-header.html
Comment 33 Build Bot 2016-03-16 16:46:20 PDT
Created attachment 274234 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 34 Build Bot 2016-03-16 16:49:55 PDT
Comment on attachment 274231 [details]
Patch

Attachment 274231 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/991048

New failing tests:
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-protocol.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-protocol-header.html
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-extensions.html
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-accept.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-extensions-header.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-accept-header.html
Comment 35 Build Bot 2016-03-16 16:49:59 PDT
Created attachment 274236 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 36 Darin Adler 2016-03-16 16:59:19 PDT
Comment on attachment 274231 [details]
Patch

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

Looks good except that the tests are failing. Please do not land until the EWS indicates they are passing. Otherwise, I do have a few small comments about areas for improvement.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:424
> +            m_failureReason = "Status line contains invalid ASCII sequence";

Not sure that a single character counts as a sequence. I would say:

    "Status line contains non-ASCII character"

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:452
> +    String httpVersionString(header, space1 - header);

Using StringView instead of String will cut down on memory allocation. Sadly, it seems that the rest of the surrounding code is using String for this purpose.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:453
> +    size_t posOfHttpVersionNumberString = httpVersionString.find("HTTP/") + 5;

Not great style to do math on the result of find before checking for notFound. Probably not great to rely on the fact that notFound + 5 will be 4.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:454
> +    if ((posOfHttpVersionNumberString + 3) < headerLength) {

Seems a bit peculiar that we consider it OK to not have HTTP/ in the string at all! Also unclear where the magic number 3 comes from.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:460
> +        String httpVersionNumberString = httpVersionString.substring(posOfHttpVersionNumberString, 3);
> +        float httpVersionNumber = httpVersionNumberString.toFloat();
> +        if (httpVersionNumber < 1.1f) {
> +            m_failureReason = "Invalid HTTP version string: " + httpVersionString;
> +            return lineLength;
> +        }

Not clear on why we are looking at exactly 3 characters. Is trailing junk really OK? What about more digits?

Typically it’s not great to convert to a floating point number to do version number comparison. In this specific case I think we could check that:

1) second character must be '.'
2) if first character is '1', third character must be '1'-'9'
3) otherwise first character must be '2'-'9'

That would probably be better than running the floating point conversion algorithm.

The current function will say that "HTTP/2xyz" is OK. Is that what we want?

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:502
> +        if ((equalLettersIgnoringASCIICase(name, "sec-websocket-extensions")
> +            || equalLettersIgnoringASCIICase(name, "sec-websocket-accept")
> +            || equalLettersIgnoringASCIICase(name, "sec-websocket-protocol"))

Quite unfortunate to have a list of field names here, not sharing that list with any other code.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:504
> +            m_failureReason = name + " header value should only contain ASCII characters";

If this is a "SHOULD" in the specification, then why are we enforcing it? I don’t think that’s what "SHOULD" means.
Comment 37 Build Bot 2016-03-16 17:03:18 PDT
Comment on attachment 274231 [details]
Patch

Attachment 274231 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/991056

New failing tests:
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-protocol.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-protocol-header.html
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-extensions.html
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-accept.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-extensions-header.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-accept-header.html
Comment 38 Build Bot 2016-03-16 17:03:22 PDT
Created attachment 274240 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5
Comment 39 Build Bot 2016-03-16 17:07:19 PDT
Comment on attachment 274231 [details]
Patch

Attachment 274231 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/991052

New failing tests:
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-protocol.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-protocol-header.html
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-extensions.html
http/tests/websocket/tests/hybi/handshake-fail-by-non-ascii-header-value-sec-websocket-accept.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-extensions-header.html
http/tests/websocket/tests/hybi/handshake-fail-by-more-accept-header.html
Comment 40 Build Bot 2016-03-16 17:07:23 PDT
Created attachment 274241 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 41 John Wilander 2016-03-17 15:32:06 PDT
(In reply to comment #36)
> Comment on attachment 274231 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274231&action=review
> 
> Looks good except that the tests are failing. Please do not land until the
> EWS indicates they are passing. Otherwise, I do have a few small comments
> about areas for improvement.
> 
> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:424
> > +            m_failureReason = "Status line contains invalid ASCII sequence";
> 
> Not sure that a single character counts as a sequence. I would say:
> 
>     "Status line contains non-ASCII character"

Fixed.
 
> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:452
> > +    String httpVersionString(header, space1 - header);
> 
> Using StringView instead of String will cut down on memory allocation.
> Sadly, it seems that the rest of the surrounding code is using String for
> this purpose.

That's a good suggestion. I filed https://bugs.webkit.org/show_bug.cgi?id=155602 to track that.

> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:453
> > +    size_t posOfHttpVersionNumberString = httpVersionString.find("HTTP/") + 5;
> 
> Not great style to do math on the result of find before checking for
> notFound. Probably not great to rely on the fact that notFound + 5 will be 4.

Agreed. Fixed.
 
> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:454
> > +    if ((posOfHttpVersionNumberString + 3) < headerLength) {
> 
> Seems a bit peculiar that we consider it OK to not have HTTP/ in the string
> at all! Also unclear where the magic number 3 comes from.

Agreed. Fixed.

> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:460
> > +        String httpVersionNumberString = httpVersionString.substring(posOfHttpVersionNumberString, 3);
> > +        float httpVersionNumber = httpVersionNumberString.toFloat();
> > +        if (httpVersionNumber < 1.1f) {
> > +            m_failureReason = "Invalid HTTP version string: " + httpVersionString;
> > +            return lineLength;
> > +        }
> 
> Not clear on why we are looking at exactly 3 characters. Is trailing junk
> really OK? What about more digits?

I'm not sure trailing junk is OK but there are more characters in there in a legitimate response – 'HTTP/1.1 101 Switching Protocols'.

> Typically it’s not great to convert to a floating point number to do version
> number comparison. In this specific case I think we could check that:
> 
> 1) second character must be '.'
> 2) if first character is '1', third character must be '1'-'9'
> 3) otherwise first character must be '2'-'9'

Agreed. Fixed.
 
> That would probably be better than running the floating point conversion
> algorithm.
> 
> The current function will say that "HTTP/2xyz" is OK. Is that what we want?

I'm not sure the previous patch accepted 2xyz since it parsed three characters as a float but I've restructured the code according to your three-step suggestion above which should rule out 2xyz.

> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:502
> > +        if ((equalLettersIgnoringASCIICase(name, "sec-websocket-extensions")
> > +            || equalLettersIgnoringASCIICase(name, "sec-websocket-accept")
> > +            || equalLettersIgnoringASCIICase(name, "sec-websocket-protocol"))
> 
> Quite unfortunate to have a list of field names here, not sharing that list
> with any other code.
> 
> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:504
> > +            m_failureReason = name + " header value should only contain ASCII characters";
> 
> If this is a "SHOULD" in the specification, then why are we enforcing it? I
> don’t think that’s what "SHOULD" means.

I think it's reasonable to restrict new HTTP headers this way. But yes, the spec says should.

New patch with the above fixes and an additional test case for HTTP versions above 1.1 coming up.

Thanks for the review!
Comment 42 John Wilander 2016-03-17 15:59:17 PDT
Created attachment 274335 [details]
Patch
Comment 43 Brent Fulgham 2016-03-18 13:22:46 PDT
Comment on attachment 274335 [details]
Patch

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

Marking r- due to the little typo in the Changelog. Also, please make a "headerHasValidHTTPVersion" method to clean the code up a little bit.

> Source/WebCore/ChangeLog:3
> +        Restrict WebSockets header parsing accodring to RFC6455 and RFC7230. Based on Lamarque V. Souza's original patch.

according!  Whoops! :-)

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:453
> +    String httpVersionString(header, space1 - header);

Please encapsulate the below code into a method "headerHasValidHTTPVersion" that accepts the header and a reference to 'lineLength' and returns a boolean if successful.

Then all of this becomes:

if (!headerHasValidHTTPVersion(httpVersionString, lineLength)) {
    m_failureReason = "Invalid HTTP version string: " + httpVersionString;
    return lineLength;
}
Comment 44 John Wilander 2016-03-22 10:03:23 PDT
Created attachment 274657 [details]
Patch
Comment 45 John Wilander 2016-03-22 10:04:58 PDT
Fixed the changeling typo and broke out the HTTP version code into its own function. Thanks for the review!
Comment 46 Brent Fulgham 2016-03-22 14:51:46 PDT
Comment on attachment 274657 [details]
Patch

r=me. Thanks!
Comment 47 WebKit Commit Bot 2016-03-22 15:27:28 PDT
Comment on attachment 274657 [details]
Patch

Clearing flags on attachment: 274657

Committed r198561: <http://trac.webkit.org/changeset/198561>
Comment 48 WebKit Commit Bot 2016-03-22 15:27:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 49 Radar WebKit Bug Importer 2016-03-22 15:35:23 PDT
<rdar://problem/25301972>
Comment 50 Joseph Pecoraro 2016-03-22 15:43:27 PDT
Comment on attachment 274657 [details]
Patch

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

Some nits after seeing the patch go by. These do not need to be addressed, just things to keep in mind for future patches.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:403
> +    // Check that there is a version number which should be three characters after "HTTP/"

Style: WebKit comments are sentences and should end in a period.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:409
> +    // Check that the version number is 1.1 or above
> +    bool versionNumberIsOneDotOneOrAbove = false;

Nit: The comment seems unnecessary since it is just repeating what is already codified in the variable name.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:450
> +            m_failureReason = "Status line contains non-ASCII character";

Nit: You could use ASCIILiteral("Status line contains non-ASCII character"); in all of these direct char* assignments to the WTF::String. This is slightly more efficient. However for error strings like this, it is probably negligible. For more info, see:
http://trac.webkit.org/wiki/EfficientStrings#Construction