Bug #7039
closedRequestParser can corrupt ws13 request content with continuation frames under limited circumstances
0%
Description
With github wt 4.0.5-31-ga05a7c5d, applications with websockets enabled (with or without the default per-message compression) can encounter corrupt request contents for clients that send multi-fragment ws13 messages. In particular, this issue has been observed with Chromium 72 and a test application that generates a large client request (170K+).
The issue appears to affect non-final message fragments (frames) when additional data is available at the time parseWebSocketMessage is called. In that case, the last buffer of payload in the non-final frame is not processed, resulting in corruption of the queryString as seen by CgiParser. The corruption is more obvious when compression is enabled.
To drive the issue for testing:
1. Use the test program and configuration instructions on issue #7034.
- Apply the patch attached to #7034 (otherwise that issue will mask this one).
- Invoke gdb, similar to following:
gdb ---args ./test_program.wt ---max-memory-request-size=512000 ---http-address=0.0.0.0 ---http-port=8080 -c wt_config.xml ---docroot=. ---deploy-path=/test ---gdb
start
b RequestParser.C:717 if state != Request::Complete
command 2
p end-begin
p dataEnd-dataBegin
end
run
Visit http://localhost:8080 with Chromium 72 browser
gdb should stop at breakpoint 2, the first value (end-begin) should be 0. Enter "c" to continue and the page should display as expected with all tableviews on row 400.
To drive the failure, add a breakpoint so that more input will be available at the call to parseWebSocketMessage:
ctrl-c to enter gdb
b Connection.C:455
run
The browser should refresh. Type "c" to continue each time gdb pauses at Connection.C:455 (less than 10 times, typically)
When gdb stops at RequestParser.C:717 (breakpoint 2), it should display two numbers. The first value (end-begin) should be non-zero. This indicates that the problem case has been hit. The second number (dataEnd-dataBegin) is the number of bytes of compressed data that will not get processed. Type "c" to continue and the browser should fail to display the expected output. Enabling debug output, the corruption should be visible in queryString (can also compare against frames captured with network tab in chromium developer tools).
Two patch files are attached:
0002-Process-last-payload-fragment-in-ws13-continuation.patch attempts to address this issue and is meant to be applied on top of 0001-Log-better-info-on-websocket-connect-fail-and-kill.patch from issue #7034. It contains block comments with additional information. The patch has only been lightly tested --- with this test application --- so it would benefit from additional review and testing.
0003-Optional-debugging-code.patch contains some optional debugging code that I found helpful while tracking this issue. It is probably too verbose for other purposes.
If I can provide further information, or if you have trouble reproducing the issue, please let me know.
Files
Updated by Roel Standaert over 5 years ago
- Status changed from New to Resolved
This should be resolved with my latest push. I also made another test case that only tests this issue and #7034, and doesn't also involve WTableView
. (max-request-size
needs to be fairly large, though, and Firefox's frames are rejected, because they're too big. Not sure if this means we should reconsider our hard coded frame length limit).
Updated by Roel Standaert over 5 years ago
- File issue_7034_2.cpp issue_7034_2.cpp added
Updated by Bruce Toll over 5 years ago
Thanks for taking the time to work through this issue (and the related #7034) to provide an elegant fix. Your test program was very clear and helpful in confirming correct behavior. Regarding firefox, it seems that adding ---max-memory-request-size=5000000 to the command line along with a large max-request-size may be sufficient?
Updated by Roel Standaert over 5 years ago
It seems that the max. WebSocket message length is indeed determined by --max-memory-request-size
. It appears to be done in a rather ugly way, though (changing the value of a static var in ALL_CAPS
, so it pretends to be a constant but isn't), I'll need to fix that.
Updated by Roel Standaert over 5 years ago
I guess since it is called MAX_WEBSOCKET_MESSAGE_LENGTH
it should also limit the message size, but it is actually only limiting the frame size, hence why Chrome's messages get through (they split it up with continuation frames) and Firefox's don't.
Updated by Roel Standaert over 5 years ago
Ok, I made it so that:
A) we actually limit the message length with --max-memory-request-size
, not just the frame length
B) MAX_WEBSOCKET_MESSAGE_LENGTH
, the fake constant, was removed and instead we're just checking maxMemoryRequestSize()
Updated by Roel Standaert about 5 years ago
- Status changed from Resolved to Closed