Skip to content
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

Fixes protocol handling. Smallest frame size is 6 for ping frames. #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nshmyrev
Copy link
Contributor

@nshmyrev nshmyrev commented May 6, 2020

If server sends ping frames which are just 6 bytes current implementation aborts and closes connection since it expects the frame to be at least 9 bytes.

@mjerris
Copy link
Contributor

mjerris commented May 10, 2020

can you add a unit test that confirms your assertion that we will fail on a 6 byte packet and that your patch corrects that

@nshmyrev
Copy link
Contributor Author

Ok, let me work on it.

@crienzo crienzo requested a review from Astaelan February 23, 2021 14:50
@crienzo
Copy link
Member

crienzo commented Feb 23, 2021

@Astaelan is this correct?

@Astaelan
Copy link
Contributor

Okay, this is a more convoluted issue than simply reducing the size. This might be acceptable given a certain situation, but it goes much deeper.
First, we need to reference the RFC, which is RFC6455 here https://tools.ietf.org/html/rfc6455
Section 5.2 talks about base framing after the websocket upgrade.

The thing to note here is that technically a message could be as small as 2 bytes, with no additional data. The minimum is 1 byte for some flags, the opcode, and 1 byte for a mask bit plus 7 bits of length data for the smallest frames. The masking key is potentially optional, and it's possible that there would be zero bytes of data.

The specification on PING and PONG do not indicate that any data must be included. Now, according to the spec all frames sent from client to server must include the mask bit, which means they must also include the mask key. That's another 4 bytes. But this is only those sent client to server, anything going server->client can, according to spec, choose to not include the mask key.

Therefore, by spec, server->client minimum size is 2 bytes, and client->server minimum size is 6 bytes. Since this is reading a frame, as a client, this is server->client and thus the minimum is potentially 2 bytes.

To that end, I am not convinced this patch is correct according to RFC, and if we're going to change the behavior we best be to the actual specification and not just arbitrarily change things.

@David-dp-
Copy link

I think this is an appropriate place to report this, instead of creating an issue.

Starting in Chrome98 Beta, our FS 1.10.7 on Debian 10 with openssl 1.1.1d began regularly prompting verto to relogin multiple times. The FS log shows "BAD READ -1" for these. Chrome DevTools shows many non-control frames of length 1028 with content #SPB. But once verto sends a non-control length 4 message with content #SPE, this is what seems to result in the BAD READ. The length of 4 fits with the analysis above about allowing smaller frames.

We're using ws.c from your release branch for the iOS 15 problem, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants