Bug 82714 – WebSockets: The handshake should check for US-ASCII data instead of UTF-8.

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

[Jocelyn Turcotte]

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.

[Lamarque V. Souza]

Created attachment 190777 [details]
Patch

Proposed patch

[Early Warning System Bot]

Comment on attachment 190777 [details]
Patch

Attachment 190777 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16768839

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 190777 [details]
Patch

Attachment 190777 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16801153

[Lamarque V. Souza]

Created attachment 190800 [details]
Patch

Fix warning reported by qt buildbot that caused build to fail.

[Lamarque V. Souza]

Created attachment 190804 [details]
Patch

Add missing text expectations.

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

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

[Lamarque V. Souza]

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.

[Jocelyn Turcotte]

(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.

[Lamarque V. Souza]

(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.

[Lamarque V. Souza]

Created attachment 192240 [details]
Patch

Fix regression in handshake-fail-by-prepended-null.html.

[Lamarque V. Souza]

CC'ing [e-mail] 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.

[Alexey Proskuryakov]

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.

[Lamarque V. Souza]

(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

[Alexey Proskuryakov]

> 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.

[Takeshi Yoshino]

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.

[Lamarque V. Souza]

Created attachment 195463 [details]
Patch

Simplify checks. The changes in the source code are small, please comment.

[Takeshi Yoshino]

(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.

[Lamarque V. Souza]

(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.

[Brent Fulgham]

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.

[John Wilander]

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.

[John Wilander]

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;
    }

    …
}

[John Wilander]

Created attachment 274214 [details]
Patch

[John Wilander]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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".

[Brent Fulgham]

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".

[John Wilander]

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.

[John Wilander]

Created attachment 274231 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[John Wilander]

(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!

[John Wilander]

Created attachment 274335 [details]
Patch

[Brent Fulgham]

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;
}

[John Wilander]

Created attachment 274657 [details]
Patch

[John Wilander]

Fixed the changeling typo and broke out the HTTP version code into its own function. Thanks for the review!

[Brent Fulgham]

Comment on attachment 274657 [details]
Patch

r=me. Thanks!

[WebKit Commit Bot]

Comment on attachment 274657 [details]
Patch

Clearing flags on attachment: 274657

Committed r198561: <http://trac.webkit.org/changeset/198561>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/25301972>

[Joseph Pecoraro]

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