Repository navigation
Reject transfer-encoding in HTTP/1.0 messages and non-chunked request bodies. - #66
Open
samuel-williams-shopify wants to merge 6 commits into
Open
samuel-williams-shopify wants to merge 6 commits into
samuel-williams-shopify wants to merge 6 commits into
Conversation
…uplicate `chunked` codings.
…HTTP/1.0 `transfer-encoding` trade-off.
…aming checks without a version.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Connection#read_bodynow rejects message framings that RFC 9112 requires recipients to treat as faulty: any HTTP/1.0 message carryingtransfer-encoding, any request whose transfer coding is anything other than exactlychunked, and any message applyingchunkedmore than once. All raiseBadRequestand mark the connection non-persistent so it is not reused. Request and status line parsing now only acceptsHTTP/1.xversions, and theupgradeheader is ignored in HTTP/1.0 requests, closing the paths by which the framing checks could be sidestepped.Problem
HTTP/1.1 message framing was more lenient than the specification allows, which matters when
protocol-http1sits behind a stricter front end (proxy or load balancer) that frames the same bytes differently — the classic building block for request smuggling.Transfer-Encoding: chunkedwas accepted and fully decoded (including trailers). RFC 9112 §6.1: a server or client receiving an HTTP/1.0 message containingTransfer-EncodingMUST treat the message as if the framing is faulty, even ifContent-Lengthis present, and close the connection.Transfer-Encodingwherechunkedis not the final coding (e.g.gziporchunked, gzip) was read as a close-delimitedBody::Remainder. RFC 9112 §6.3: the server MUST respond 400 and close the connection, because the body length cannot be determined reliably. Close-delimited reading is only correct for responses.Transfer-Encoding: gzip, chunkedwas accepted asBody::Chunked, and sinceread_bodyconsumes thetransfer-encodingheader, the application received a still-gzip-coded body with no indication of the coding. RFC 9112 §6.1: a server SHOULD reject a transfer coding it does not understand.Transfer-Encoding: chunked, chunked(or two separateTransfer-Encoding: chunkedheaders, which the header policy merges) was accepted on both requests and responses. RFC 9112 §7.1:chunkedMUST NOT be applied more than once.REQUEST_LINEmatchedHTTP\/\d.\dwith an unescaped.and any major version, soHTTP/0.9,HTTP/2.0andHTTP/1x1were parsed and treated as HTTP/1.1 — sidestepping the literalHTTP/1.0check above. The client did not validate the status line version at all, sohttp/1.1 200 OKwas accepted despite HTTP-version being case-sensitive (RFC 9112 §2.3).Upgradeon an HTTP/1.0 request switched to close-delimited reading, bypassing thetransfer-encodingcheck. RFC 9110 §7.8: a server MUST ignoreUpgradein an HTTP/1.0 request.Solution
read_requestandread_responsenow pass the parsedversionthroughread_request_body/read_response_bodytoread_body. The new parameter is a trailing optional positional (version = nil) so existing callers (includingasync-http, which only usesread_request/read_response) are unaffected; when the version is unknown the HTTP/1.0 check is skipped but all other framing checks still apply.read_body, whentransfer-encodingis present:version == HTTP10, raiseBadRequestand set@persistent = false.transfer-encoding+content-lengthrejection now also sets@persistent = false, since the RFC requires closing the connection in that case too.chunkedappears more than once, raiseBadRequestand set@persistent = false(requests and responses).remainder == true):chunkedlast →Body::Chunked(e.g.gzip, chunkedstill decodes the chunked layer, leaving the remaining coding to the application, as before); otherwise read until close as before.[chunked]is accepted; anything else raisesBadRequestand sets@persistent = false.REQUEST_LINEnow usesHTTP\/1\.\d;read_request_lineraisesInvalidRequestfor anything else.read_response_linevalidates the version against the same pattern and raisesBadRequest. Unknown minor versions such asHTTP/1.2remain accepted and are treated as HTTP/1.1 (RFC 9110 §2.5).read_request_bodyonly takes the upgrade path whenversion != HTTP10, so HTTP/1.0 requests fall through toread_body. The existing upgrade test, which happened to use HTTP/1.0, now uses HTTP/1.1.read_request,read_response,read_request_bodyandread_response_bodynow document theBadRequestthey propagate. Theread_bodydocumentation records the client-side trade-off: browsers ignoretransfer-encodingon HTTP/1.0 responses and read to close; if interoperability ever requires relaxing this, the correct relaxation is close-delimited reading, never chunked decoding.In
async-http,Protocol::HTTP1::Server#next_requestalready rescuesProtocol::HTTP::BadRequest, writes a400 Bad Requestwithconnection: close, and re-raises so the connection is torn down — verified end-to-end against a localAsync::HTTP::Serverfor the rejection cases. On the client side,read_responseraisesBadRequest(consistent with the existing handling oftransfer-encoding+content-lengthin responses).Tests
Added to
test/protocol/http1/connection.rbandtest/protocol/http1/upgrade.rb:transfer-encoding: chunked(with and withoutcontent-length, both withConnection: keep-aliveso the persistence assertion is meaningful) raisesBadRequest;persistentis false.transfer-encoding: gzip,chunked, gzip,gzip, chunked,chunked, chunked, or twotransfer-encoding: chunkedheaders raisesBadRequest;persistentis false.transfer-encodingandcontent-lengthraisesBadRequest;persistentis false.gzip, chunkedresponse still decodes asBody::Chunked;transfer-encoding: gzipresponse is still read close-delimited asBody::Remainder;chunked, chunkedand duplicatechunkedheaders raiseBadRequest.transfer-encoding: chunkedraisesBadRequeston the client;persistentis false.HTTP/0.9,HTTP/2.0,HTTP/1x1,http/1.1raiseInvalidRequest;HTTP/1.2is accepted. Status lines with the same invalid versions raiseBadRequest;HTTP/1.2is accepted.Upgradeis read with anilbody rather thanBody::Remainder, andUpgrade+transfer-encoding: chunkedon HTTP/1.0 is still rejected.read_body(headers, false)called directly without a version (the original signature) still acceptschunkedand rejectsgzipfor requests, and still readsgzipresponses until close.