Repository navigation
reject permessage-deflate response with a larger window than offered - #3113
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3113 +/- ##
===========================================
- Coverage 94.00% 93.97% -0.03%
===========================================
Files 177 177
Lines 13857 13898 +41
===========================================
+ Hits 13026 13061 +35
- Misses 831 837 +6
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Merged develop to pick up the CI workflow fixes: the gcc 5-8 and clang 3.7-9 jobs were dying at checkout (the forced node 24 runtime can't start inside the ubuntu 16.04/18.04 containers, per the job logs) and the clang-win jobs failed while bootstrapping b2 itself, so none of those were related to the patch. Also covered the new error::bad_sec_extensions mapping in test/beast/websocket/error.cpp, which was the one uncovered line codecov flagged. b2 libs/beast/test/beast/websocket passes locally against current develop. |
There was a problem hiding this comment.
Thank you for the PR.
The changes are not compliant with a few sections of RFC 7692, which I've noted in comments on the relevant lines. Note that most of the conformance issues mentioned are pre-existing, but they become important if the goal of this PR is to address them.
We may also want to decide whether to be lenient with certain RFC requirements based on how well existing servers conform to them in the wild. Therefore, some research is needed for each rejection condition. Since this only affects client mode, comparing the behavior of major browsers (Chrome and Firefox) and well-known WebSocket client implementations in Python, Rust, and Go should be sufficient to inform each decision.
|
The following table is generated using Claude Fable 5.1 to survey how well-known WebSocket client implementations handle the server's Legend: websockets = python-websockets; lws = libwebsockets; Chromium policy = its
The proposed implementation: detail::pmd_offer const offer = pmd_config_; // as sent, see do_pmd_config(req)
detail::pmd_offer agreed;
detail::pmd_read(agreed, res);
if(agreed.accept)
{
if( ! offer.accept || // rfc6455 4.1 step 5, rfc7692 5: not offered
(offer.server_max_window_bits > 0 && // rfc7692 7.1.2.1: must be echoed, <= offer
(agreed.server_max_window_bits == 0 ||
agreed.server_max_window_bits > offer.server_max_window_bits)) ||
agreed.client_max_window_bits == -1 || // rfc7692 7.1.2.2: response form requires a value
(offer.client_max_window_bits == 0 && // rfc7692 7.1.2.2: MUST NOT appear if not offered
agreed.client_max_window_bits != 0) ||
(offer.client_max_window_bits > 0 && // ws, python-websockets, .NET: above offered value
agreed.client_max_window_bits > offer.client_max_window_bits))
{
BOOST_BEAST_ASSIGN_EC(ec, error::bad_sec_extensions);
return;
}
if(agreed.client_max_window_bits == 0 && // omitted: honor our own hint (ws, websockets)
offer.client_max_window_bits > 0)
agreed.client_max_window_bits = offer.client_max_window_bits;
}
pmd_config_ = agreed; // 8 stays; open_pmd compresses at 9 |
|
That's quite the code block :) |
|
That table settles the wild-behaviour question. I've taken the proposed implementation: on_response_pmd now validates the response against the offer as sent (pmd_config_ is filled from the request in build_request_pmd, per your do_pmd_config note), rejects a permessage-deflate element we never offered (rfc7692 5), rejects a valueless client_max_window_bits in the response since that form is only valid in an offer (7.1.2.2), and treats client_max_window_bits as a hint the server may drop or reduce rather than one it must echo. It stays error::bad_sec_extensions throughout. One adjustment to the server-window branch. The literal condition rejects whenever the server omits server_max_window_bits and we offered a value, but build_request_pmd always offers server_max_window_bits from pmd_opts_, which is 15 by default, so a compliant server accepting with a bare permessage-deflate would be refused, and that includes Beast's own server (its negotiate omits the parameter once the agreed value is 15). So I read an omitted response value as the default 15 and reject only when that effective value exceeds the offer, which keeps every row of the table and leaves the default handshake intact. I walked each case through the new condition and testExtResponseWindow now covers them: omitting below a reduced offer rejects, omitting under the default-15 offer accepts, the hint is kept when the server stays silent, the valueless response form and an unoffered acceptance both reject, and a window of 8 is accepted. |
|
@sahvx655-wq, I restructured the code to make each rejection condition easier to read. I also clamped |
c73a65a to
22f612a
Compare
The client used to overwrite its own offer with whatever the server returned in Sec-WebSocket-Extensions. on_response_pmd now checks the response and fails the handshake with the new error::bad_sec_extensions (condition::handshake_failed) when: * permessage-deflate is accepted although the client never offered it * server_max_window_bits, or its 15-bit default when omitted, exceeds the offered limit * client_max_window_bits is present without a value * client_max_window_bits is returned although it was not offered When the response omits client_max_window_bits or returns a larger value than offered, the offered value is kept so compression uses the window the client proposed. Co-authored-by: Mohammad Nejati <ashtumashtum@gmail.com>
22f612a to
0ac76b0
Compare
A permessage-deflate client never checks the server's negotiation response against its own offer.
on_response_pmdreadsSec-WebSocket-Extensionsfrom the 101 into apmd_offerand stores it verbatim (the existingVFALCOnote flags exactly this), so a server can hand backserver_max_window_bits/client_max_window_bitslarger than the client asked for and the client willzi.reset/zo.resetwith that larger window. I reproduced it by offeringserver_max_window_bits=10and replying withpermessage-deflate; server_max_window_bits=15: the handshake completes andget_statusreports a 15-bit (32 KiB) inflate window, ignoring the cap the application configured throughpermessage_deflate. rfc7692 7.1.2.1/7.1.2.2 require the response window to be no larger than the offer, and 7.1.1 requires the client to fail the connection otherwise, so a peer can force this endpoint past a memory bound it deliberately set.The check belongs beside the response parsing, so
on_response_pmdnow compares the selectedserver_max_window_bits/client_max_window_bitsagainst the values already held inpmd_opts_and fails the handshake with a newerror::bad_sec_extensionswhen either exceeds the offer. It only rejects a window the peer explicitly sends that is larger than the client asked for, so a compliant server (which sends an equal or smaller value, or omits the parameter) is untouched and the existing deflate handshake tests still pass. Added a regression test that drives the client handshake against a crafted response covering both the server and client window cases.