Subject: [Gen-art] Gen-ART last call review of draft-ietf-hybi-thewebsocketprotocol-10

From: Richard L. Barnes <[email-masked]>
Date: Tue, 19 Jul 2011 23:01:53 -0400

I am the assigned Gen-ART reviewer for this draft. For background on 
Gen-ART, please see the FAQ at 
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments 
you may receive.

Document: draft-ietf-hybi-thewebsocketprotocol-10.txt
Reviewer: Richard Barnes
Review Date: 19 July 2011
IETF LC End Date: 25 July 2011
IESG Telechat date: (if known) -

Summary:
Not ready.  

Major issues:

[Is GET/Upgrade appropriate?]
It seems like the use of GET here goes beyond the normal "safe" semantics for that method.  To quote RFC 2616:
"
   In particular, the convention has been established that the GET and
   HEAD methods SHOULD NOT have the significance of taking an action
   other than retrieval.
"
In addition, alternative protocols specified in Upgrade headers are generally optional, whereas in this case, the transaction fails overall if the server does not support the websocket protocol.  It doesn't look to me like any of the current methods provide much better semantics (except possiblyCONNECT or the use of OPTIONS as in RFC 2817), and it seems like defining a new method could help get around the Upgrade issue as well.

[Huge buffers]
The frame length can be 7, 16, or 64 bits long.  Since the client is expected to buffer data until the end of a frame, this is asking clients to buffer 128 B, 64 KB, or 16 EB.  If it were 32 bits, the max would be 4 MB.  Why not just make this a 32-bit fixed length field?

[Why is masking necessary?]
I seriously question the necessity of the masking of data frames.  As I understand it, the goal is to prevent proxies that don't understand Upgrade from confusing WebSocket data with HTTP data.  This risk seems a little dubious to me; has such a poisoning attack been demonstrated?  It seems like there are much simpler ways of doing this, like using a method other than GET (either CONNECT or something new).   

[Why only client-to-server masking?]
Why isn't masking required on server-to-client frames?

[Unlimited buffering with fragmentation]
Much like with the frame length issue above, the fragmentation mechanism here seems like it imposes a heavy burden on the receive side.  Since the receiving client is supposed to buffer data until the end of a frame, it seems like fragmentation could be used to cause a receiving client to buffer a frame of indefinite size. 

Minor issues:

[Setting protocol]
In the NOTE in Section 1.3, the document observes that Sec- headers can't be set in XHRs.  But clearly the Sec-WebSocket-Protocol header can be set through some API that applications use to get to this protocol.  So it seems a little misleading to imply that scripts can't set these headers (besides -Version and -Origin).

[Use existing Origin header]
Why is this document creating a separate Sec-WebSocket-Origin header, rather than using the Origin header from draft-ietf-websec-origin?   

[Need a reference to CORS]
It would be helpful to have a reference to Cross-Origin Resource Sharing in the NOTE in this section, since the pre-request check that is described is exactly a CORS priming request.

[Key is over-engineered]
What is the need for the hash and GUID in the handshake processing of the key?  It seems like it would be sufficient for the server to do anything deterministic with the key, even something as simple as incrementing it or flipping the bits.

Nits / Editorial comments:

[Why not plain sockets?]
The introduction makes clear why this protocol is needed instead of HTTP, but not why this protocol improves over providing a plain socket interface.  Presumably this is because the HTTP header provides a space where the browser can inject trusted information?  

[Put the handshake section first]
It would really increase readability to put all the negotiation/handshake parts up front, before you get to the data format (Section 4).  As it is, when I got to the extension data part, I didn't realize that there was an extension mechanism, since it hadn't been mentioned yet.

[Stray sections]
It seems like Section 8 (Error Handling) should be moved up to Section 4.6, and Section 9 (Extensions) should be part of section 4.

---

From: Alexey Melnikov <[email-masked]>
Date: Wed, 20 Jul 2011 17:03:11 +0100

Hi Richard,
Thanks for the review. I will answer to some of your comments and I or 
my co-editor will followup on remaining issues later on.

>In addition, alternative protocols specified in Upgrade headers are generally optional, whereas in this case, the transaction fails overall if the server does not support the websocket protocol.  It…
This issue was discussed extensively in the WG and was reraised during 
IETF LC by others. I will followup with a more detailed answer (or maybe 
the WG agrees with you and this would be changed), but a quick comment 
on one of the points.

The WG discussed defining a new HTTP method, but the idea didn't have 
the support of majority because new HTTP methods are hard to deploy (due 
to old intermediaries which don't handle them as well as they should 
have if they were compliant with RFC 2616).

>The frame length can be 7, 16, or 64 bits long.  Since the client is expected to buffer data until the end of a frame, this is asking clients to buffer 128 B, 64 KB, or 16 EB.  If it were 32 bits, t…
This was discussed in the WG. On one hand the frame length encoding is 
optimized for packet size (the field overhead is small for smaller 
packets) and on another hand there was a desire to support big buffer 
sizes, thus the current proposal. I believe a single 32bit field was 
proposed but was explicitly decided against.

>I seriously question the necessity of the masking of data frames.  As I understand it, the goal is to prevent proxies that don't understand Upgrade from confusing WebSocket data with HTTP data.  Thi…
I am not a good person to answer this, but believe me the WG debated 
this very issue for weeks.

>Why isn't masking required on server-to-client frames?
As above.

>Much like with the frame length issue above, the fragmentation mechanism here seems like it imposes a heavy burden on the receive side.  Since the receiving client is supposed to buffer data until t…
This is not really different from what other protocols like HTTP and 
SMTP already allow. I think it would be fair to note this in the 
Security Considerations section, but I don't think this feature should 
be removed. Fragmentation does allow senders to start sending data 
before the total size of the object is known (e.g. if data is generated 
on the fly), which was considered as an important feature by the WG.

>In the NOTE in Section 1.3, the document observes that Sec- headers can't be set in XHRs.  But clearly the Sec-WebSocket-Protocol header can be set through some API that applications use to get to t…
But a script can't set the Sec-WebSocket-Protocol header field value 
directly.

 [snip]

I will also look at your editorial proposals.

Best Regards,
Alexey

---

From: "Richard L. Barnes" <[email-masked]>
Date: Sun, 24 Jul 2011 15:11:10 -0400

Hey Alexey,  

Thanks for following up.  A couple of responses inline.

--Richard

> This issue was discussed extensively in the WG and was reraised during IETF LC by others. I will followup with a more detailed answer (or maybe the WG agrees with you and this would be changed), but a quick comment on one of the points.
> 
> The WG discussed defining a new HTTP method, but the idea didn't have the support of majority because new HTTP methods are hard to deploy (due to old intermediaries which don't handle them as well as they should have if they were compliant with RFC 2616).

What's the argument against CONNECT?  It seems like that's exactly what is desired here -- for proxies to act as tunnels.  RFC 2817 uses an upgrade to TLS in all its examples, but the is specified more generally:
"
   Any successful (2xx) response to a CONNECT request indicates that the
   proxy has established a connection to the requested host and port,
   and has switched to tunneling the current connection to that server
   connection.
"

It seems like this gets you around the threat that masking is supposed to address -- the proxy won't see anything mid-stream that looks like HTTP, since it's just acting as a tunnel at that point.  It's a little weird to use CONNECT end-to-end, but that doesn't seem bad enough to be a blocking issue. 

> This was discussed in the WG. On one hand the frame length encoding is optimized for packet size (the field overhead is small for smaller packets) and on another hand there was a desire to support big buffer sizes, thus the current proposal. I believe a single 32bit field was proposed but was explicitly decided against.

Actually, now that I think about it, aren't you solving the same problem twice, once with frame length and once with fragmentation?  If the goal is to have the browser buffer for me until my whole message has arrived, isn't that met by either a giant frame or fragmentation?  Given that, it seems like having a 16-bit field would even be sufficient.  It seems like 64k will probably cover a lot of applications (64k should be enough for anyone!), and for large things that use fragmentation, the header overhead would only be 8 octets per 64k, way down in the noise.  (3 octets if you get rid of masking!) 

> This is not really different from what other protocols like HTTP and SMTP already allow. I think it would be fair to note this in the Security Considerations section, but I don't think this feature should be removed. Fragmentation does allow senders to start sending data before the total size of the object is known (e.g. if data is generated on the fly), which was considered as an important feature by the WG.

A mention in the Security Considerations sounds like a sensible solution to me.

> But a script can't set the Sec-WebSocket-Protocol header field value directly.

That's not really true: You can't set it *directly*, in the sense that the following is not allowed:
   client.setRequestHeader("Sec-WebSocket-Protocol", "kick-puppies");
But you can set it by setting the protocols in the WebSocket API:
   var ws = new WebSocket("ws://evil.tld/kick-puppies", ["kick-puppies"]);

---

From: Greg Wilkins <[email-masked]>
Date: Mon, 25 Jul 2011 10:40:51 +1000

> It seems like this gets you around the threat that masking is supposed to address -- the proxy won't see anything mid-stream that looks like HTTP, since it's just acting as a tunnel at that point.…

One concern expressed was that existing infrastructure may have
special handling that triggers on the CONNECT method.   Eg. code that
will look at only the first HTTP request on a connection before
becoming a bit pipe.  Such proxies will work with websockets when GET
is used, but will fail with CONNECT because it will trigger special
handling to look for the destination server, which will not be present
in the handshake.

If the concern with using GET is that we are changing established
semantics, then surely that concern must also apply to changing the
semantics of CONNECT.  If changed semantics is the concern, then using
an entirely new method would be applicable.

However, my reading of the discussion resulting to Roy's objection to
the use of GET, is that he was mostly concerned that the 101 response
not be consider the final response to the GET, and that semantically
there is still a request to be responded to that is related to the
specific URL passed in the GET.    If semantically the WS stream is
considered the response to the GET for the URI, then I think's Roy's
issues are resolved.   (NB. I've misread Roy's concerns a few times...
so confirmation that I've got it right this time would be good).

cheers