Closed Bug 1351948 Opened 5 years ago Closed 5 years ago

Firefox/Websockets accepts non-minimal-encoded payload length

Categories

(Core :: Networking: WebSockets, defect)

55 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: iichikolamp, Assigned: mcmanus)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 2 obsolete files)

Attached patch non-minimal-payload.patch (obsolete) — Splinter Review
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
Component: Untriaged → Networking: WebSockets
Product: Firefox → Core
Patrick, you wrote the patch for bug 687243. What do you say about this?
Flags: needinfo?(mcmanus)
Flags: needinfo?(mcmanus)
Attachment #8852761 - Flags: review?(michal.novotny)
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.
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.
Assignee: nobody → iichikolamp
Whiteboard: [necko-active]
Attachment #8852761 - Flags: review?(michal.novotny) → review+
Summary: Firefox/Websockets accpets non-minimal-encoded payload length → Firefox/Websockets accepts non-minimal-encoded payload length
Comment on attachment 8852761 [details] [diff] [review]
non-minimal-payload.patch

># User Hideki Takeoka <iichikolamp@gmail.com>
># 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;
Flags: needinfo?(michal.novotny)
Attached patch non-minimal-payload.patch (obsolete) — Splinter Review
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.
Attachment #8852761 - Attachment is obsolete: true
Flags: needinfo?(michal.novotny)
I will submit this to try and prep it for check it in for you if it passes.
I see, thanks a lot!
Assignee: iichikolamp → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8853992 - Attachment is obsolete: true
Attachment #8854155 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6553dcb0df0c
Refuse ws messages that don't use minimal encoding. r=michal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6553dcb0df0c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Hi Patrick, I appreciate your help.
You need to log in before you can comment on or make changes to this bug.