Subject: [hybi] TSV-Directorate review of draft-ietf-hybi-thewebsocketprotocol-07

From: Magnus Westerlund <[email-masked]>
Date: Fri, 29 Apr 2011 19:00:48 +0200

Hi,

On request I have performed a transport directorate review of this
document. As the document is in WG last call I have included any issue I
have found in the document. I will note that I haven't followed the WG.
I assume some issues are highly debated and have gone through consensus
process. If that is the case indicate so and preferably some summary for
my information.

Regarding TSV-Dir reviews you may read more about those here:
http://trac.tools.ietf.org/area/tsv/trac/wiki/TSV-Directorate-Reviews

My Summary: Document not ready for publication multiple issues that
needs to be resolved.

Note, the issues are not ordered in any particular order. Thus small and
big issues may follow each other.

1. I have a question regarding the copyright boiler plate and the
acknowledgment section. If a large number of people contributed text to
the first version provided to IETF, does the WG have ensured that all
contributors of text have provided the rights to IETF, or does WHATWG
have such requirements on contributors that IETF has in fact received
all the copyrights it needs not only to publish a document but also
transfer it other organizations if needed?

2. Section 1.3 and 9. I really don't see how the Sec-WebSocket-Origin
header provides any security without additional mechanisms. Yes, a
trusted browser implementation will set it correctly. However, as non
browser or hacked browser can fake this header the server has no direct
trust in this value. To me it appears that one would need to make
additional bindings to some other contexts to be able to trust the value
of this header. Or am I missing some important part here?

If not, then shouldn't the security considerations and description be
expanded to explain how you can use this header to actually accomplish
some real security?

3. Section 1.3, Page 7,
   For this header, the server has to take the value (as present in the
   header, e.g. the base64-encoded [RFC4648] version minus leading and
   trailing whitespace), and concatenate this with the GUID "258EAFA5-
   E914-47DA-95CA-C5AB0DC85B11" in string form, which is unlikely to be
   used by network endpoints that do not understand the WebSocket
   protocol.

Can you please expand the "GUID" acronym and if this GUID conforms to
any particular format for GUIDs then can you provide a reference to that
format?

4. Section 1.3, Page 7,
  "SHA-1 hash, base64-encoded, of this concatenation is
   then returned in the server's handshake [FIPS.180-2.2002]."

I would suggest that you indicate how many bits of the hash you uses.
SHA-1 hashes are commonly truncated to shorter forms.

5. Section 1.4:
  "The closing handshake is intended to replace the TCP closing
   handshake (FIN/ACK), on the basis that the TCP closing handshake is
   not always reliable end-to-end, especially in the presence of man-in-
   the-middle proxies and other intermediaries."

As the closing handshake doesn't replace the TCP closing, it complements
or augments it to work over intermediaries I would suggest to change the
wording.

6. Section 1.5:
"and re-implements the closing handshake in-band."

Similar to the previous comment. I think "re-implements" is the wrong
word here, complements or something else. You are not in fact changing
the TCP stack, but it does sounds like it.

7. Section 1.9, 10:

I definitely think this specification should create an registry for
protocol identifiers. As the header likely allows for very long strings
there is no risk of every filling the registry so a RFC 5226 policy of
"First come, first served" appears suitable. Then one only needs to
define the amount of information one needs when requesting a
registration. Likely the template can be this simple:

Protocol ID string:
Reference: <if something exist>
Contact Person:

I do think that one should move quite a lot of the discussion in Section
1.9 into a more relevant normative section where the actual header is
defined.

8. Lack of formal specification of the following HTTP headers:
Sec-WebSocket-Protocol, Sec-WebSocket-Origin, Sec-WebSocket-Accept. They
all lack any format definition in the format of ABNF. In general I think
it would be good to actually give each of the HTTP headers its own
section with definition.

9. Section 2.1:
I am missing a reference to UTF-8 and the U+0041 notation. I guess STD63
would do the trick.

10. Section 2.1:
The usage of *bold* _underline_ and /italic/ for different purpose in
the document should be explained. Especially the /italic/ for variable
names is important. Please do remember that a number of people may read
the specification in programs that does not embelish the text where we
get to see the star, the under score and the forward slash.

11. Section 3.

I am missing an introduction to the section where it explains that
websocket defines its own two schemes that are usable in various
environments where one would like to indicate to where to open a websocket.

12. Section 3.
Why isn't the URI scheme definition present in this section? That would
help a lot before going into the parsing rules.

13. Section 3.1:
"These steps return either a /host/, a /port/, a
   /resource name/, and a /secure/ flag, or they fail."

This is the first of many usage of "fail" to indicate some abort
procedure. I assume the one in section 7.1.4. However, that does not
seem to be consistent as parsing of websocket URI results inconclusive
results even prior to establishing any connection. I would recommend
that one check all the occurrences of "fail" and see if one can be more
specific on what is supposed to do. And in cases it is a common
procedure, please include a reference to that section that explains that
procedure.

14. Section 3.3 and 10.1 and 10.2:

   o  The /host/ MUST be ASCII-only (i.e. it MUST have been punycode-
      encoded [RFC3492] already if necessary, and MUST NOT contain any
      characters above U+007E).

   o  The /resource name/ string MUST be a non-empty string of
      characters in the range U+0021 to U+007E and MUST start with a
      U+002F SOLIDUS character (/).

For host is quite clear from the RFC 3986 ABNF that other characters
aren't allowed, is this a real-world problem that force on to make this
clear? Or are there any further limitation than what the RFC 3986 ABNF
has for host? In which case the ABNF in the URI scheme template should
be redefined.

The second one is a stricter than the ABNF in the RFC 3986 as
"path-abempty" allows for empty resource. Thus I think the ABNF in the
templace should be updated to reflect the actuall ABNF for the URI
scheme being defined.

15. section 4.1:
As such, in the absence of extensions negotiated during the opening
   handshake (Section 5), all reserved bits MUST be 0 and reserved
   opcode values MUST NOT be used.

I would recommend a clarification on what to do with reserved bits. My
preference is to say: MUST be set to 0 and ignored on reception. The
reason for this is that if one starts using them with some extensions
they will be separate from zero but still correctly ignored by a non
upgraded interpreter.

But in fact it may be that the above text should in fact be moved to the
definition of RSV1, RSV2, RSV3.

16. Section 4.2:
   Opcode:  4 bits

      Defines the interpretation of the payload data

I am missing a table for the value. As far as I understand the only
place they are present are in the ABNF.

17. Section 4.2: Payload length
The length of the payload: if 0-125, that is the payload length.

The text fails to tell in what units the payload length is provided in.
I guess it is bytes but it should be written.

18. Section 4.2: Payload Data, Extension Data and Application Data
all these headings says that they are "n" bytes. But in fact they are
n+m, n and m bytes respectively. Would it make sense to make this
clearer by using different integer variables?

19. The usage of ABNF to define binary protocols.
I don't think ABNF for binary protocols are a particular great idea as
it is difficult to express the field size of individual fields.

20. I have run the ABNF trhoug some parser and my own brain and have
found to following errors:

A.
frame-opcode            = %x0 ; continuation frame
                        / %x1 ; text frame
                        / %x2 ; binary frame
                        / %3-7 ; reserved for further non-control frames

Missing "x" before in / %3-7 that should be / %x3-7

B.
frame-masking-key       = <4>( %0x00-FF ) ; present only if frame-masked
is 1<CR><LF>

The intention here is to have 4 bytes, but by using <4> you have in fact
made the 4 into a comment rather that part of the rule. It should be:
frame-masking-key       = 4( %0x00-FF ) ; present only if frame-masked is 1

21. Section 4.3:
The masking key MUST be derived from a strong source of entropy, and
   the masking key for a given frame MUST NOT make it simple for a
   server to predict the masking key for a subsequent frame.

This appears to be a suitable place to reference RFC 4086
"Randomness Requirements for Security"

22. Section 4.3:
If I understand the masking correctly it for one particular frame it
uses the same mask over and over again over each 32-bits. As the frame
format supports message of size up to 2^63 I think I can perform a cache
poisining attack anyway. This as I will know the boundaries for where
each 32-bits will be used. Thus I create my message that I want to get
inserted into a cache. Then I ensure that this is 32-bits aligned. Then
I create a big message and hope it will not be fragmented is
particularly small frames. Then I guess on new mask values for each
repetition of the message. Depending on the frame sizes the underlying
implementation uses and the size of the message I need for poisining the
attack I can achieve a particular probablility of success. For a 256
byte message and frame sizes of 16 MiB would get a probability of
1/65535 to actually find the right masking key. And as the websocket
using web-app can have the websocket server to be in on the attack the
web-app can get feedback on the framing size structure the browser is
using to tailor the message sizes. Combining this with that the time it
takes to transfer a few gigabyte of data on the current Internet this is
an attack quite feasible.

>From my perspective one either needs a real crypto to handle arbitrarily
sized frames, or one needs to enforce limitations on the frame sizes in
relation to the masking fields sizes.

23. Section 4.4:
     "Its
      content is the concatenation of the application data (and any
      extension data that may be present) from each of those frames in
      order."

This text is unclear and likely related to the issue in 25 below. But it
is also indicating the parts in the concatenation in the wrong order.

24. Section 4.4:
   o  Control frames MAY be injected in the middle of a fragmented
      message.  Control frames themselves MUST NOT be fragmented.

has the above rule, but no rule forcing the transmission of the data
frame fragments in order, nor that other data frames may interrupt the
current one. Fairly obvious but not stated.

25. Section 4.4
I fail to find any explanation if the entity being fragmented is the
"payload data" or the "application data". The reason I am asking is if
one can have extension data in each fragment. The later makes sense if
one uses extensions that are related to each fragment frame, rather than
the whole payload. This can become a real issue if one want one set of
extension over the whole payload and another set of extension in each
fragments. Then the delimiting between the extension data and the
application data becomes a problem.

26. Section 4.5.1:

It SHOULD do so as soon as is practical.

When writing SHOULD it is good to indicate when there is reason for not
doing it. I guess in cases where one likes to complete the transfer of
the current frame or fragmented frame prior to closing. Any other
reasons for not doing it as soon as the current frame is completed, even
if a fragment?

27. Section 4.5.1:
   After both sending and receiving a close message, an endpoint
   considers the websocket connection closed, and SHOULD close the
   underlying TCP connection.

What is the alternative to actually closing the TCP connection? Can one
actually start a new websocket over the existing TCP connection? If
there is no alternative, why not change SHOULD to MUST or at least will.

Secondly, as who closes the TCP connection does matter for who is going
to hold the time_wait state I think one should recommend the client to
do close the TCP connection.

Thirdly, should this really be specified here. There seems to be overlap
with section 7.

28. Section 4.5.2:

The message
   bodies (i.e. both the Extension data (if any) and the Application
   data) of the Ping and Pong MUST be the same.

I find the fact that the extension body data needs to be the same to be
an issue. If one develops an extension that actually has to do with the
frames and their transport then not being able to change the data will
be an issue. As an example could be a Websocket RTT measurement
extension where you like to include in the PONG both the PING sending
time from the PING message and the amount of time between receving PING
and sending PONG. Thus requiring one to add additional data.

29. Section 4.5.2:
   An endpoint MAY send a Ping message any time after the connection is
   established and before the connection is closed.  NOTE: A ping
   message may serve either as a keepalive, or to verify that the remote
   endpoint is still responsive.

So one allows to have multiple outstanding PING messages? That sounds a
bit strange over a reliable in-order delivery channel such as TCP. And
if one does allow it should there be any rules for answering pings in
order one receives them?

30. I think one should consider adding a section for underlying
transport requirements for websockets. It currently assumes TCP, but to
my understanding it could be used over any transport that are reliable,
byte clean, support messages sizes of relatively large size. Transport
of websocket over SCTP is the first one that comes to mind. Not that I
want it specified in this document. But that one lists the requirements
from websocket on underlying transport.

31. Section 5.1:
       If the user agent cannot determine the IP address of the remote
       host (for example because all communication is being done through
       a proxy server that performs DNS queries itself), then the user
       agent MUST assume for the purposes of this step that each host
       name refers to a distinct remote host, but should instead limit
       the total number of simultaneous connections that are not
       established to a reasonably low number (e.g., in a Web browser,
       to the number of tabs the user has open).

I don't find this paragraph particular clear. But if one only uses name
then what is the limiation, only one connecting per host name, and in
addition not more than one per tab context or similar?

32. Section 5.1, page 29:
  Once a connection to the server has been established (including a
   connection via a proxy or over a TLS-encrypted tunnel), the client
   MUST send a handshake to the server.  The handshake consists of an
   HTTP upgrade request, along with a list of required and optional
   headers.  The requirements for this handshake are as follows.

Considering the cache poising and the security issue, why isn't
websocket specified to always run in a TLS tunnel? Why is the non-secure
mode existing at all?

33. Section 5.1:
   3.   The request MUST contain a "Request-URI" as part of the GET
        method.  This MUST match the /resource name/ Section 3.
Is this inteded as limitation on the GET request or it is simply
forgetting the possibility to have absolute URIs in the GET header?

34. Section 5.1 and 5.2.1
Page 30 says:
The request MAY include a header with the name "Sec-WebSocket-
        Protocol".  If present, this value indicates the subprotocol(s)
        the client wishes to speak.

Page 32 says:
   6.  Optionally, a "Sec-WebSocket-Protocol" header, with a list of
       values indicating which protocols the client would like to speak,
       ordered by preference.

There is discrepancies here. And I think it should be fixed by having
one section define the header properly and the reference it from the
procedural text.

35. The delimiation inside Extension Data.
Currently the extension data part of the protocol is not explicitly
provided. It is assumed to to be known from the negotiation of the
extensions. I think that works fine for single extensions that are
present in all frames. What happens to extensions that are present only
in some frames and where this can't be specified based on the other
fields in the frame header?

Would it not be easier to use one of the RSV bits to indicate the
presence of a extension data part. Then each element in the extension
part is a TLV structure thus ensuring internal delimiting. That way
multiple independent extensions can be used and they don't need to
include data always.

36. Section 7.1.1.
This section should recommend that the client is the one that performs
the TCP close so that it holds the TIME_WAIT state and not the server.

37. Section 7.1.2:
 Once an endpoint
   has both sent and received a Close control frame, that endpoint
   SHOULD _Close the WebSocket Connection_ as defined in Section 7.1.1.

This text should be considered to be changed in light of issue 36.

38. Section 8.1:
The usage of "x-" in extension tokes seems not thought over enough. The
experience from SIP and a few other protocol with x- is that once you
have created an name x-something then the only effect if that is a
popular extension and later specifed in a still compatible way is that
you then need to support both x-something and something. Then it is
better to allow for easy registration of names so that anyone that goes
beyond simple testing can register that name to keep it unique.

39. Section 8.1:
   Note that the order of extensions is significant.  Any interactions
   between multiple extensions MAY be defined in the documents defining
   the extensions.  In the absence of such definition, the
   interpretation is that the headers listed by the client in its
   request represent a preference of the headers it wishes to use, with
   the first options listed being most preferable.  The extensions
   listed by the server in response represent the extensions actually in
   use for the connection.  Should the extensions modify the data and/or
   framing, the order of operations on the data should be assumed to be
   the same as the order in which the extensions are listed in the
   server's response in the opening handshake.

So do I understand this correctly, any extensions that would be mutually
exclusive, needs to indicate that this is the fact in the definition of
an extension. That I think is an argument for why a simple registration
of extensions makes sense and ensure that already in the template for
registration one ensures to provide known extensions that have defined
interactions.

40. Section 8.2.1:
   Senders using this extension MUST apply RFC 1951 encodings to all
   bytes of the data stream following the handshake including both data
   and control messages.  The data stream MAY include multiple blocks of
   both compressed and uncompressed types as defined by [RFC1951].

I can't understand what is to be done. This section contains the words
"data stream" that doesn't exist anywhere else in the spec. I think the
extension needs to properly define over which elements it functions and
from what phase in the websocket establishment.

41. Section 9.

The security consideration section does not discuss client
authentication. Is that because this is supposed to happen in the HTTP
GET with the upgrade headers. there is some reference to cookies etc.
But shouldn't there be more on this? So that a server can ensure the
client identity?

42. Section 10.2:

   A |wss:| URI identifies a WebSocket server and resource name, and
   indicates that traffic over that connection is to be encrypted.

Is really "encrypted" the right word here. Isn't what one expect to have
with a "s" scheme at least confidentiality and integrity. Also certain
aspects of authentication exist with TLS.

43. Section 10.2:
Are there any minimal security levels implied in this scheme? Am I
allowed to do TLS with NULL crypto and only integrity protection? Am I
allowed to use only 40 bits keys?

44. Section 10.3:

This registration request should be clear on which registry the
"websocket" keyword should be added in.

45. Section 10.

I am missing registry definitions for the following entities defined in
the protocol specification that appears to need registries:

- Protocol identifiers for the Sec-WebSocket-Protocols header
- Extension identifier for the Sec-WebSocket-Extension header
- Version numbers for the Websocket protocol itself
- Status codes in the Close frame.
- Frame opcode registry

And if you include my suggestion for Extension Data TLVs they type field
in the TLV will need a registry also.

Have a nice weekend

Magnus Westerlund

----------------------------------------------------------------------
Multimedia Technologies, Ericsson Research EAB/TVM
----------------------------------------------------------------------
Ericsson AB                | Phone  +46 10 7148287
Färögatan 6                | Mobile +46 73 0949079
SE-164 80 Stockholm, Sweden| mailto: [email-masked]
----------------------------------------------------------------------

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 29 Apr 2011 20:34:47 +0200

Hi Magnus,

thank you for this deep review. I can't spend enough time to comment on
many of your points (in part because many are the results of very long
discussions), but there are a few short ones that can already be easily
addressed.

>From memory, it's because XHR can't set a Sec- header so we're certain
that evil code running in browsers will not fake WS handshakes. We don't
want to prevent any non-browser from establishing a connection (that is
neither possible nor desired), but to ensure that owned browsers won't
do that.

> or augments it to work over intermediaries I would suggest to change the

I agree, especially considering the amount of developers who forget to
close their sockets, leaving them in CLOSE_WAIT because they think there
is nothing left to do.

> get to see the star, the under score and the forward slash.

Indeed, in old drafts (-76) I had trouble reading all those variable names
at one point.

> poisining attack anyway.

There was a very lengthy debate on this several months ago and it was hard
to reach a rough consensus. I invite you to consult the archives. In short,
we're certain products exist with a risk for the first bytes, we're not
aware of any product which present this risk past whatever bytes break
HTTP parsing, and the way to write such a vulnerable product appears very
difficult to several developers, though it can't be demonstrated that none
exists. The resulting proposal was accepted as a trade-off between a very
low risk and some serious performance hits in some specific usages, and
still provides good protection against the known issues.

> there is no alternative, why not change SHOULD to MUST or at least will.

Agreed (same as point 5 above BTW).

> if one does allow it should there be any rules for answering pings in

Indeed, we should possibly remind implementors that it's useless to send
a new ping as long as one is still pending.

> websocket specified to always run in a TLS tunnel? Why is the non-secure

TLS + NPN was evocated as a possible later upgrade. Still there is a real
need for cleartext traffic so that communications can be analysed and
filtered in schools or other places where it is mandatory to filter
contents and/or to protect people.

> the TCP close so that it holds the TIME_WAIT state and not the server.

If we recommend something, we should recommend that the server does it, as
only the client is impacted by holding a time_wait, as it prevents it from
re-opening the connection for 2 MSL, while on the server it has no impact
as reopining a time_wait connection is immediate upon a new SYN with a
higher seq number. Do not forget that there will be quite a number of
intermediaries in the whole chain and we don't want to cause basic networking
issues between server-side proxies and the servers themselves.

> But shouldn't there be more on this? So that a server can ensure the

I agree with the needs here. I remember we initiated some discussions
around this a long time ago when the handshake was not HTTP-compliant,
but it seems that we did not do that work again when the HTTP issues
were fixed. All I remember is that handling of non-101 responses was
left a bit undefined (just not a failed upgrade attempt) :-/

> Have a nice weekend

Thanks for your comments, and have a nice week-end too. I'm sure many
others will comment on your points and you'll get a fat mailbox over
the week-end :-)

Regards,
Willy

---

From: Patrick McManus <[email-masked]>
Date: Fri, 29 Apr 2011 16:31:20 -0400

Hi Magnus - thanks for all the effort you put forward into reviewing the
draft!

> of this header. Or am I missing some important part here?

For the purposes of the Sec-WebSocket-Origin header, the untrusted party
is the author of the javascript that is executing inside the client. The
client itself of course has privs to contact the server and fake
whatever it wants, it is trying to figure out if it should extend the
communication privilege to the author of the javascript. This header
lets the client and server collaborate on a CORS-like model to constrain
that 3rd party.

-Patrick

---

From: Julian Reschke <[email-masked]>
Date: Sat, 30 Apr 2011 15:44:12 +0200

On 29.04.2011 19:00, Magnus Westerlund wrote:
> ...
> ...

I think the important question is whether there are parts left are 
actually identical to what was once brought into this WG. I'm not sure 
about that.

That being said... <http://kaazing.com/about/index.html> states:

> "Kaazing basically invented the protocol as it stands now." - Ian Hickson, Google & HTML5 Author

So it might be a good idea to check with these guys what they think 
about the current status.

Best regards, Julian

---

From: Iñaki Baz Castillo <[email-masked]>
Date: Sat, 30 Apr 2011 17:18:43 +0200

> But shouldn't there be more on this? So that a server can ensure the

Cookies are retrieved from a web page, so if we assume that the
websocket server will be able to validate the client provided cookie
then we are wrong (the websocket server could be a separate server).

Anyhow, this revision of the draft finally states that:

   Any status code other than 101 indicates that the WebSocket handshake
   has not completed, and that the semantics of HTTP still apply.  The
   headers follow the status code.

So the websocket server is free to reply a 401 to ask for HTTP
Basic/Digest authentication.

-- 
Iñaki Baz Castillo
<[email-masked]>

---

From: Willy Tarreau <[email-masked]>
Date: Sat, 30 Apr 2011 17:21:33 +0200

> then we are wrong (the websocket server could be a separate server).

Not only that, but cookies are used by infrastructure components too
in order to maintain persistence (association between a client and a
server).

Regards,
Willy