New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[V3] Fix parsing WebSocket close frame length #203

Merged
merged 2 commits into from Jan 28, 2018

Conversation

Projects
None yet
3 participants
@tiwoc
Copy link
Contributor

tiwoc commented Jan 19, 2018

Without this patch, I get the following error when sending a Close frame without a payload to an Engine WebSocket server:

⚠️ [WebSocket.WebSocketError.invalidMask: Masks must be 4 bytes, no more, no less] [/Users/redacted/.build/checkouts/engine.git-3311994267206676365/Sources/WebSocket/FrameParser.swift:213:39] [Possible causes: The mask provided was not 4 bytes long as per standard] [Suggested fixes: Generate 4 random bytes using any Random Number Generator for clients,Leave the mask empty (nil) for servers]

See commit messages for details on my changes.

tiwoc added some commits Jan 19, 2018

FrameParser: Fix parsing Close frames with empty payload
The current formula has an off-by-one error: A Close frame without a
payload sent by a client has length 6:

* 2 bytes for flags, opcode, mask and payload length
* 4 bytes for mask

When we reach the guard, the first 2 bytes have been read and the 4 mask
bytes are left, so `consumed + 4 == 6`, which is equal to length.
FrameParser: Move stricter length check above less strict check
If `length &- consumed >= payloadLength &+ 4` is true, then
`consumed &+ 4 <= length` is true, so the `.invalidMask` error would
never be thrown:

length &- consumed >= payloadLength &+ 4
<=>
payloadLength &+ 4 <= length &- consumed
<=>
consumed &+ payloadLength &+ 4 <= length

@tanner0101 tanner0101 added this to the 3.0.0 milestone Jan 23, 2018

@tanner0101 tanner0101 added the bug label Jan 23, 2018

@tanner0101 tanner0101 requested a review from Joannis Jan 23, 2018

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Jan 23, 2018

@Joannis please review this one :)

@Joannis
Copy link
Member

Joannis left a comment

I completely missed this one, it looks good to me. Thank you! 👍

@Joannis Joannis merged commit 0be1a87 into vapor:beta Jan 28, 2018

2 checks passed

ci/circleci: linux Your tests passed on CircleCI!
Details
ci/circleci: macos Your tests passed on CircleCI!
Details

@tanner0101 tanner0101 added this to Done in Vapor 3 Feb 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment