675983 - Websockets - fail connection on 1st fragment with a continuation op code

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Patrick McManus [:mcmanus]]

https://github.com/oberstet/Autobahn test cases 3.8 3.9. and 3.14 send a frame with a continuation op-code (and the fin bit set) that isn't part of a continuation sequence. (i.e. there is no previous first-frag which would have no fin bit and a non-continuation non-control op code).

This isn't part of a valid fragment sequence, and it isn't a valid unfragmented message either. We ignore it.

Draft -10 doesn't appear to have a specific handling for this, but we close the connection on other invalid protocol sequences so I think that's the right thing to do here too as suggested in 664344.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 1

Comment 2

[Tobias Oberstein]

+1 for closing on those invalid sequences, even when draft-10 doesn't specify that. I think it should.

One remark: with case 3.14, FF does ignore only the payload of the first "continuation that doesn't continue anything":

Message send: ["sendframes",[{"opcode":0,"fin":true,"payload":"fragment1"},{"opcode":1,"fin":false,"payload":"fragment2"},{"opcode":0,"fin":true,"payload":"fragment3"},{"opcode":0,"fin":true,"payload":"fragment1"},{"opcode":1,"fin":false,"payload":"fragment2"},{"opcode":0,"fin":true,"payload":"fragment3"}]]
Message received: fragment2fragment3
Message received: fragment1
Message received: fragment2fragment3

The payload "fragment1" (of the 2nd invalid frame) _is_ popped up in JS.

Btw: depressing to watch you fix those bugs much faster than it takes me to find them;)

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to comment #2)

> One remark: with case 3.14, FF does ignore only the payload of the first
> "continuation that doesn't continue anything":
> 

I think this patch will fix that.. we weren't really totally ignoring the bogus frame - it just seemed that way ;) It still impacted the state.

> Btw: depressing to watch you fix those bugs much faster than it takes me to
> find them;)

your tool is a great help to firefox - I really appreciate it!

When I get to the end of your list of things I will start a build for you with all the queued (but unreviewed and uncommitted) patches in it so you can retest. What platform do you use?

Comment 4

[Tobias Oberstein]

that'd be great! most of the time I'm on Win64 ..

Comment 5

[Patrick McManus [:mcmanus]]

(In reply to comment #4)
> that'd be great! most of the time I'm on Win64 ..

The build directory is here - the windows build will be done in 3 or 4 hours from now:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-95503e0b10d9/

Comment 6

[Jason Duell]

Comment on attachment 550113 [details] [diff] [review]
patch 1

Review of attachment 550113 [details] [diff] [review]:
-----------------------------------------------------------------

Logic OK, but I'm submitting a cleaned up version.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +578,5 @@
>    mMaxMessageSize(16000000),
>    mStopOnClose(NS_OK),
>    mServerCloseCode(CLOSE_ABNORMAL),
>    mScriptCloseCode(0),
> +  mFragmentOpcode(kContinuation),

Is there any reason we store the fragment code in a PRUint8, when the spec dictates it's only 4 bits, so would fit in an unsigned char?  Or conversely, if we're going to keep it as an 8 bit value, why not initialize mFragmentOpcode to some "illegal" value, just for clarity? (I'm not doing either change in my patch: I'd lean toward using the 4-bit type: thoughts?).

Also, since we only set mFragmentOpCode once (for the first fragment), it's really the "overall message" opcode (and doesn't correspond to the current fragment's opcode).  I've renamed it mMessageType.

@@ +879,5 @@
>        if (opcode == kContinuation) {
> +
> +        // Make sure this continuation fragment isn't the first fragment
> +        if (mFragmentOpcode == kContinuation) {
> +          LOG(("WebSocketHeandler:: continuation code in first fragment\n"));

The wrong class name AND it's misspelled: for shame! :)

@@ +903,5 @@
>          payloadLength += mFragmentAccumulator;
>          avail += mFragmentAccumulator;
>          mFragmentAccumulator = 0;
>          opcode = mFragmentOpcode;
> +        mFragmentOpcode = kContinuation;

add

  // reset opcode to detect if next message illegally starts with continuation

Comment 7

[Jason Duell]

Attachment: v2: a few cleanups, same logic

Let me know if you're ok with my fixes (and maybe change mMessageType to 4 bit if you think that's ok).

Comment 8

[Patrick McManus [:mcmanus]]

Thanks for the updates jason. But actually, I prefer my patch in all respects except for the typo in the log message and your additional comment.

mMessageType is not correctly named. It is the opcode of an in-progress fragmented message - it does not apply to all messages.  mFragmentMessageOpcode would be acceptable to me if you insist on a change.

You also changed my log about nested fragments making it less informative imo (and shamefully left my mangled class name in it after you changed it!). That's not germane to this bug in any event.
 
I have mixed feelings on the 4 bit data type (you incur a lot of under the covers masking for no particular benefit.. there can only be 200 of these objects after all) - no reason to make it part of this bug fix in any event.

Comment 9

[Jason Duell]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e47729ef7552

Scrapped my changes other than comment fixups.

Started to write test for this and other fragment msg cases, but ran into some stupid issues (python--meh), so moved to bug 677461.

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/e47729ef7552