1351948 - Firefox/Websockets accepts non-minimal-encoded payload length

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Hideki Takeoka]

Attachment: non-minimal-payload.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.110 Safari/537.36

Steps to reproduce:

* Environment
- Firefox/52.0-20100101
  - User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
  - User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:52.0) Gecko/20100101 Firefox/52.0
  - User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
- Firefox/55.0-20100101
  - User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

* Step to reproduce
Connect the WS server which sends the message without encoding its payload length as minimum as possible

I write test case as case 1.1.9 of my Autobahn|Testsuite (https://github.com/camparijet/autobahn-testsuite), this will help to check results quickly by running "wstest -m fuzzingserver".




Actual results:

Firefox processes the request as the same as the one with minimal encoded payload length.

Here is the result under my environment (https://camparijet.github.io/ws-server-minimal-payload-test-v1.4/reports/clients/firefox_52_0_20100101_64bit_case_1_1_9.html https://camparijet.github.io/ws-server-minimal-payload-test-v1.4/reports/clients/firefox_55_0_20100101_nightly_case_1_1_9.html)

The essence of this problem is the same as  https://bugzilla.mozilla.org/show_bug.cgi?id=687243 but the direction this case is server -> client.


Expected results:

* Expected results
RFC6455 says in 5.2. Base Framing Protocol, 

```
Note that in all cases, the minimal number of bytes MUST be used to encode the length, for example, the length of a 124-byte-long string can't be encoded as the sequence 126, 0, 124.
```

If Firefox accept the non-minimal encoded message same as the minimal-encoded one, it's hard for developers of websocket application to use Firefox as a reference of websocket client.

* my proposal and sample patch

I guess the right behavior is:

1. closing the WebSocket Session initiated by client-side.
2. its status code is 1002 (Protocol Error)

On the other hand, I don't find anything about what client should do for the above case in RFC6455.

So I test major browsers and the results are here: https://camparijet.github.io/ws-server-minimal-payload-test-v1.4/reports/clients/

I assume 1) can be minimum solution to notify developers for violation of spec.

I attach the patch to archieve 1). 

and the result is here: https://camparijet.github.io/ws-server-minimal-payload-test-v1.4/reports/clients/firefox_55_0_20100101_patched_case_1_1_9.html

Comment 1

[Daniel Stenberg [:bagder]]

Patrick, you wrote the patch for bug 687243. What do you say about this?

Comment 2

[Patrick McManus [:mcmanus]]

hideki, thanks for this work.

as you say, 6455 puts the normative requirement on the sender not to do this and firefox is the receiver and there is not a normative requirement for what the receiver should do when encountering a sender violation. So I really don't think you ought to put this case in your client validation test suite as there isn't a correct defined behavior.

That being said, rejecting invalid sending when we can helps create a better interop environment. The usual blocker to that is backwards compat. however in this case you've already done the legwork to show that chrome rejects this case (which is also a valid behavior) so I'm happy to follow suit.

thanks for the patch.

Comment 3

[Hideki Takeoka]

Hi Patrick and all, thank you for reactions.

> So I really don't think you ought to put this case in your client validation test suite as there isn't a correct defined behavior.

You are completely right. I use it was to see the behaviors among different browser easily. But if its purpose is to validate client, it's too much.

Comment 4

[Hideki Takeoka]

Comment on attachment 8852761 [details] [diff] [review]
non-minimal-payload.patch

># User Hideki Takeoka <[e-mail]>
># Bug 1351948 - websockets accept only minimal-payload-encoded message.
>diff --git a/netwerk/protocol/websocket/WebSocketChannel.cpp b/netwerk/protocol/websocket/WebSocketChannel.cpp
>--- a/netwerk/protocol/websocket/WebSocketChannel.cpp
>+++ b/netwerk/protocol/websocket/WebSocketChannel.cpp
>@@ -1551,6 +1551,14 @@ WebSocketChannel::ProcessInput(uint8_t *
>         break;
> 
>       payloadLength64 = mFramePtr[2] << 8 | mFramePtr[3];
>+
>+      if(payloadLength64 < 126){
>+        // Section 5.2 says that the minimal number of bytes MUST
>+        // be used to encode the length in all cases
>+        LOG(("WebSocketChannel:: non-minimal-encoded payload length"));
>+        return NS_ERROR_ILLEGAL_VALUE;
>+      }
>+
>     } else {
>       // 64 bit length
>       framingLength += 8;
>@@ -1566,6 +1574,14 @@ WebSocketChannel::ProcessInput(uint8_t *
> 
>       // copy this in case it is unaligned
>       payloadLength64 = NetworkEndian::readInt64(mFramePtr + 2);
>+
>+      if(payloadLength64 <= 0xffff){
>+        // Section 5.2 says that the minimal number of bytes MUST
>+        // be used to encode the length in all cases
>+        LOG(("WebSocketChannel:: non-minimal-encoded payload length"));
>+        return NS_ERROR_ILLEGAL_VALUE;
>+      }
>+
>     }
> 
>     payload = mFramePtr + framingLength;

Comment 5

[Hideki Takeoka]

Attachment: non-minimal-payload.patch

Sorry for mistakenly sending notification.

What I wanted to do there is add header (i.e. user/commit message) in patch, following advice in IRC.

I submit the patch ( this time with the header ) again.

It's my first time to submit bug report, thus I don't have idea to do "try server" process. If I should do something further, please let me know.

Comment 6

[Patrick McManus [:mcmanus]]

I will submit this to try and prep it for check it in for you if it passes.

Comment 7

[Hideki Takeoka]

I see, thanks a lot!

Comment 8

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=faf9e5a77f2c

Comment 9

[Patrick McManus [:mcmanus]]

Attachment: refuse ws messages that don't use minimal encoding

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6553dcb0df0c
Refuse ws messages that don't use minimal encoding. r=michal

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6553dcb0df0c

Comment 12

[Hideki Takeoka]

Hi Patrick, I appreciate your help.