From eb0268894d9573c8eaeb80bd4fcf47aa84864613 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 4 Feb 2022 17:02:15 +0000 Subject: [PATCH] client_side_request.cc:2028 "request->method.id()" assertion (#971) ConnStateData::tunnelOnError() ignored its method parameter and always called initiateTunneledRequest() with METHOD_NONE. buildFakeRequest() then set HttpRequest::method to METHOD_NONE. Squid does not support such HttpRequest objects well because quite a bit of code assumes that HttpRequest::method must be known. For example, depending on configuration and other factors, Squid may assert. Moreover, many Squids using the on_unsupported_protocol directive also have special rules for handling tunnels and those rules may not work as intended for these METHOD_NONE transactions. Squid now uses CONNECT method when it creates a CONNECT-like request that facilitates on_unsupported_protocol tunneling. This helps meet code expectations about HttpRequest::method being defined and natural admin expectations about tunneled requests having a CONNECT method. Admins that want to distinguish on_unsupported_protocol tunnels from other tunnels can use ACL annotations (for now). If needed, one can add a better/dedicated way of identifying on_unsupported_protocol tunnels. Also removed the method parameter from clientTunnelOnError() and related methods. That method was extracted from a low-level parser field and - for cases where the higher-level code deemed input to be non-HTTP, it was wrong to use essentially garbage/non-HTTP chars as a method name; - for other cases, the method is available via HttpRequest::method. TODO: We may be able to remove more duplicated parameters or unnecessary checks on this code path: Perhaps clientReplyContext::setReplyToError() method parameter can be retrieved from errstate->request? Perhaps errstate->request itself is always the same as http->request? This is a Measurement Factory project. --- src/client_side.cc | 35 ++++++++++++++++------------------- src/client_side.h | 6 +++--- src/client_side_reply.cc | 4 ++-- src/client_side_reply.h | 2 +- src/client_side_request.cc | 2 +- src/redirect.cc | 2 +- src/servers/Http1Server.cc | 22 +++++++++++----------- src/servers/Http1Server.h | 2 +- 8 files changed, 36 insertions(+), 39 deletions(-) diff --git a/src/client_side.cc b/src/client_side.cc index f52b9a5aa0..1d770b5e7f 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1553,7 +1553,7 @@ bool ConnStateData::serveDelayedError(Http::Stream *context) /// initiate tunneling if possible or return false otherwise bool -ConnStateData::tunnelOnError(const HttpRequestMethod &, const err_type requestError) +ConnStateData::tunnelOnError(const err_type requestError) { if (!Config.accessList.on_unsupported_protocol) { debugs(33, 5, "disabled; send error: " << requestError); @@ -1577,7 +1577,7 @@ ConnStateData::tunnelOnError(const HttpRequestMethod &, const err_type requestEr if (context) context->finished(); // Will remove from pipeline queue Comm::SetSelect(clientConnection->fd, COMM_SELECT_READ, NULL, NULL, 0); - return initiateTunneledRequest(request, Http::METHOD_NONE, "unknown-protocol", preservedClientData); + return initiateTunneledRequest(request, "unknown-protocol", preservedClientData); } debugs(33, 3, "denied; send error: " << requestError); return false; @@ -1667,7 +1667,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp, conn->quitAfterError(request.getRaw()); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); - repContext->setReplyToError(ERR_UNSUP_REQ, Http::scNotImplemented, request->method, NULL, + repContext->setReplyToError(ERR_UNSUP_REQ, Http::scNotImplemented, nullptr, conn, request.getRaw(), nullptr, nullptr); assert(context->http->out.offset == 0); context->pullData(); @@ -1681,7 +1681,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp, clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); conn->quitAfterError(request.getRaw()); - repContext->setReplyToError(ERR_INVALID_REQ, frameStatus, request->method, nullptr, conn, request.getRaw(), nullptr, nullptr); + repContext->setReplyToError(ERR_INVALID_REQ, frameStatus, nullptr, conn, request.getRaw(), nullptr, nullptr); assert(context->http->out.offset == 0); context->pullData(); clientProcessRequestFinished(conn, request); @@ -1717,7 +1717,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp, assert (repContext); conn->quitAfterError(request.getRaw()); repContext->setReplyToError(ERR_TOO_BIG, - Http::scPayloadTooLarge, Http::METHOD_NONE, NULL, + Http::scPayloadTooLarge, nullptr, conn, http->request, nullptr, nullptr); assert(context->http->out.offset == 0); context->pullData(); @@ -2104,7 +2104,6 @@ ConnStateData::abortChunkedRequestBody(const err_type error) const Http::StatusCode scode = (error == ERR_TOO_BIG) ? Http::scPayloadTooLarge : HTTP_BAD_REQUEST; repContext->setReplyToError(error, scode, - repContext->http->request->method, repContext->http->uri, CachePeer, repContext->http->request, @@ -2140,7 +2139,7 @@ ConnStateData::requestTimeout(const CommTimeoutCbParams &io) const err_type error = receivedFirstByte_ ? ERR_REQUEST_PARSE_TIMEOUT : ERR_REQUEST_START_TIMEOUT; updateError(error); - if (tunnelOnError(HttpRequestMethod(), error)) + if (tunnelOnError(error)) return; /* @@ -2936,7 +2935,7 @@ ConnStateData::parseTlsHandshake() HttpRequest::Pointer request = context->http->request; debugs(83, 5, "Got something other than TLS Client Hello. Cannot SslBump."); updateError(ERR_PROTOCOL_UNKNOWN, parseErrorDetails); - if (!tunnelOnError(HttpRequestMethod(), ERR_PROTOCOL_UNKNOWN)) + if (!tunnelOnError(ERR_PROTOCOL_UNKNOWN)) clientConnection->close(); return; } @@ -3018,7 +3017,7 @@ ConnStateData::splice() // respond with "Connection Established" to the client. // This fake CONNECT request required to allow use of SNI in // doCallout() checks and adaptations. - return initiateTunneledRequest(request, Http::METHOD_CONNECT, "splice", preservedClientData); + return initiateTunneledRequest(request, "splice", preservedClientData); } } @@ -3110,7 +3109,7 @@ ConnStateData::handleSslBumpHandshakeError(const Security::IoResult &handshakeRe } - if (!tunnelOnError(HttpRequestMethod(), errCategory)) + if (!tunnelOnError(errCategory)) clientConnection->close(); } @@ -3148,7 +3147,7 @@ ConnStateData::httpsPeeked(PinnedIdleContext pic) #endif /* USE_OPENSSL */ bool -ConnStateData::initiateTunneledRequest(HttpRequest::Pointer const &cause, Http::MethodType const method, const char *reason, const SBuf &payload) +ConnStateData::initiateTunneledRequest(HttpRequest::Pointer const &cause, const char *reason, const SBuf &payload) { // fake a CONNECT request to force connState to tunnel SBuf connectHost; @@ -3182,7 +3181,7 @@ ConnStateData::initiateTunneledRequest(HttpRequest::Pointer const &cause, Http:: } debugs(33, 2, "Request tunneling for " << reason); - ClientHttpRequest *http = buildFakeRequest(method, connectHost, connectPort, payload); + ClientHttpRequest *http = buildFakeRequest(connectHost, connectPort, payload); HttpRequest::Pointer request = http->request; request->flags.forceTunnel = true; http->calloutContext = new ClientRequestContext(http); @@ -3211,7 +3210,7 @@ ConnStateData::fakeAConnectRequest(const char *reason, const SBuf &payload) connectHost.assign(ip); } - ClientHttpRequest *http = buildFakeRequest(Http::METHOD_CONNECT, connectHost, connectPort, payload); + ClientHttpRequest *http = buildFakeRequest(connectHost, connectPort, payload); http->calloutContext = new ClientRequestContext(http); HttpRequest::Pointer request = http->request; @@ -3221,7 +3220,7 @@ ConnStateData::fakeAConnectRequest(const char *reason, const SBuf &payload) } ClientHttpRequest * -ConnStateData::buildFakeRequest(Http::MethodType const method, SBuf &useHost, unsigned short usePort, const SBuf &payload) +ConnStateData::buildFakeRequest(SBuf &useHost, unsigned short usePort, const SBuf &payload) { ClientHttpRequest *http = new ClientHttpRequest(this); Http::Stream *stream = new Http::Stream(clientConnection, http); @@ -3246,9 +3245,8 @@ ConnStateData::buildFakeRequest(Http::MethodType const method, SBuf &useHost, un // Setup Http::Request object. Maybe should be replaced by a call to (modified) // clientProcessRequest HttpRequest::Pointer request = new HttpRequest(mx); - AnyP::ProtocolType proto = (method == Http::METHOD_NONE) ? AnyP::PROTO_AUTHORITY_FORM : AnyP::PROTO_HTTP; - request->url.setScheme(proto, nullptr); - request->method = method; + request->url.setScheme(AnyP::PROTO_AUTHORITY_FORM, nullptr); + request->method = Http::METHOD_CONNECT; request->url.host(useHost.c_str()); request->url.port(usePort); @@ -3257,8 +3255,7 @@ ConnStateData::buildFakeRequest(Http::MethodType const method, SBuf &useHost, un request->manager(this, http->al); - if (proto == AnyP::PROTO_HTTP) - request->header.putStr(Http::HOST, useHost.c_str()); + request->header.putStr(Http::HOST, useHost.c_str()); request->sources |= ((switchedToHttps() || port->transport.protocol == AnyP::PROTO_HTTPS) ? Http::Message::srcHttps : Http::Message::srcHttp); #if USE_AUTH diff --git a/src/client_side.h b/src/client_side.h index 891fbd2069..1c4d040c09 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -335,14 +335,14 @@ public: bool fakeAConnectRequest(const char *reason, const SBuf &payload); /// generates and sends to tunnel.cc a fake request with a given payload - bool initiateTunneledRequest(HttpRequest::Pointer const &cause, Http::MethodType const method, const char *reason, const SBuf &payload); + bool initiateTunneledRequest(HttpRequest::Pointer const &cause, const char *reason, const SBuf &payload); /// whether we should start saving inBuf client bytes in anticipation of /// tunneling them to the server later (on_unsupported_protocol) bool shouldPreserveClientData() const; /// build a fake http request - ClientHttpRequest *buildFakeRequest(Http::MethodType const method, SBuf &useHost, unsigned short usePort, const SBuf &payload); + ClientHttpRequest *buildFakeRequest(SBuf &useHost, unsigned short usePort, const SBuf &payload); /// From-client handshake bytes (including bytes at the beginning of a /// CONNECT tunnel) which we may need to forward as-is if their syntax does @@ -434,7 +434,7 @@ protected: /// whether preservedClientData is valid and should be kept up to date bool preservingClientData_ = false; - bool tunnelOnError(const HttpRequestMethod &, const err_type); + bool tunnelOnError(const err_type); private: /* ::Server API */ diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 94edd1604f..f35c1cc117 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -98,7 +98,7 @@ clientReplyContext::clientReplyContext(ClientHttpRequest *clientContext) : */ void clientReplyContext::setReplyToError( - err_type err, Http::StatusCode status, const HttpRequestMethod& method, char const *uri, + err_type err, Http::StatusCode status, char const *uri, const ConnStateData *conn, HttpRequest *failedrequest, const char *unparsedrequest, #if USE_AUTH Auth::UserRequest::Pointer auth_user_request @@ -115,7 +115,7 @@ clientReplyContext::setReplyToError( #if USE_AUTH errstate->auth_user_request = auth_user_request; #endif - setReplyToError(method, errstate); + setReplyToError(failedrequest ? failedrequest->method : HttpRequestMethod(Http::METHOD_NONE), errstate); } void clientReplyContext::setReplyToError(const HttpRequestMethod& method, ErrorState *errstate) diff --git a/src/client_side_reply.h b/src/client_side_reply.h index 8548b4b702..9963d5c573 100644 --- a/src/client_side_reply.h +++ b/src/client_side_reply.h @@ -43,7 +43,7 @@ public: /// replaces current response store entry with the given one void setReplyToStoreEntry(StoreEntry *e, const char *reason); /// builds error using clientBuildError() and calls setReplyToError() below - void setReplyToError(err_type, Http::StatusCode, const HttpRequestMethod&, char const *, const ConnStateData *, HttpRequest *, const char *, + void setReplyToError(err_type, Http::StatusCode, char const *, const ConnStateData *, HttpRequest *, const char *, #if USE_AUTH Auth::UserRequest::Pointer); #else diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 09ae8ba42f..bbdd45ac64 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -563,7 +563,7 @@ ClientRequestContext::hostHeaderVerifyFailed(const char *A, const char *B) clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); repContext->setReplyToError(ERR_CONFLICT_HOST, Http::scConflict, - http->request->method, NULL, + nullptr, http->getConn(), http->request, NULL, diff --git a/src/redirect.cc b/src/redirect.cc index fc6bfd2404..d72384d9a3 100644 --- a/src/redirect.cc +++ b/src/redirect.cc @@ -259,7 +259,7 @@ constructHelperQuery(const char *name, helper *hlp, HLPCB *replyHandler, ClientH clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); repContext->setReplyToError(ERR_GATEWAY_FAILURE, status, - http->request->method, NULL, + nullptr, http->getConn(), http->request, NULL, diff --git a/src/servers/Http1Server.cc b/src/servers/Http1Server.cc index c672d226e6..9b6e56f83c 100644 --- a/src/servers/Http1Server.cc +++ b/src/servers/Http1Server.cc @@ -123,8 +123,8 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context) assert(http->log_uri); const char * requestErrorBytes = inBuf.c_str(); - if (!tunnelOnError(parser_->method(), errPage)) { - setReplyError(context, request, parser_->method(), errPage, parser_->parseStatusCode, requestErrorBytes); + if (!tunnelOnError(errPage)) { + setReplyError(context, request, errPage, parser_->parseStatusCode, requestErrorBytes); // HttpRequest object not build yet, there is no reason to call // clientProcessRequestFinished method } @@ -142,8 +142,8 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context) http->setLogUriToRawUri(http->uri, parser_->method()); const char * requestErrorBytes = inBuf.c_str(); - if (!tunnelOnError(parser_->method(), ERR_INVALID_URL)) { - setReplyError(context, request, parser_->method(), ERR_INVALID_URL, Http::scBadRequest, requestErrorBytes); + if (!tunnelOnError(ERR_INVALID_URL)) { + setReplyError(context, request, ERR_INVALID_URL, Http::scBadRequest, requestErrorBytes); // HttpRequest object not build yet, there is no reason to call // clientProcessRequestFinished method } @@ -161,8 +161,8 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context) http->setLogUriToRawUri(http->uri, parser_->method()); const char * requestErrorBytes = NULL; //HttpParserHdrBuf(parser_); - if (!tunnelOnError(parser_->method(), ERR_UNSUP_HTTPVERSION)) { - setReplyError(context, request, parser_->method(), ERR_UNSUP_HTTPVERSION, Http::scHttpVersionNotSupported, requestErrorBytes); + if (!tunnelOnError(ERR_UNSUP_HTTPVERSION)) { + setReplyError(context, request, ERR_UNSUP_HTTPVERSION, Http::scHttpVersionNotSupported, requestErrorBytes); clientProcessRequestFinished(this, request); } return false; @@ -174,8 +174,8 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context) // setReplyToError() requires log_uri http->setLogUriToRawUri(http->uri, parser_->method()); const char * requestErrorBytes = NULL; //HttpParserHdrBuf(parser_); - if (!tunnelOnError(parser_->method(), ERR_INVALID_REQ)) { - setReplyError(context, request, parser_->method(), ERR_INVALID_REQ, Http::scBadRequest, requestErrorBytes); + if (!tunnelOnError(ERR_INVALID_REQ)) { + setReplyError(context, request, ERR_INVALID_REQ, Http::scBadRequest, requestErrorBytes); clientProcessRequestFinished(this, request); } return false; @@ -204,7 +204,7 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context) } void -Http::One::Server::setReplyError(Http::StreamPointer &context, HttpRequest::Pointer &request, const HttpRequestMethod& method, err_type requestError, Http::StatusCode errStatusCode, const char *requestErrorBytes) +Http::One::Server::setReplyError(Http::StreamPointer &context, HttpRequest::Pointer &request, err_type requestError, Http::StatusCode errStatusCode, const char *requestErrorBytes) { quitAfterError(request.getRaw()); if (!context->connRegistered()) { @@ -216,7 +216,7 @@ Http::One::Server::setReplyError(Http::StreamPointer &context, HttpRequest::Poin clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); - repContext->setReplyToError(requestError, errStatusCode, method, context->http->uri, this, nullptr, requestErrorBytes, nullptr); + repContext->setReplyToError(requestError, errStatusCode, context->http->uri, this, nullptr, requestErrorBytes, nullptr); assert(context->http->out.offset == 0); context->pullData(); @@ -259,7 +259,7 @@ Http::One::Server::processParsedRequest(Http::StreamPointer &context) assert(http->log_uri); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); - repContext->setReplyToError(ERR_INVALID_REQ, Http::scExpectationFailed, request->method, http->uri, + repContext->setReplyToError(ERR_INVALID_REQ, Http::scExpectationFailed, http->uri, this, request.getRaw(), nullptr, nullptr); assert(context->http->out.offset == 0); context->pullData(); diff --git a/src/servers/Http1Server.h b/src/servers/Http1Server.h index a69751c2d8..303070794d 100644 --- a/src/servers/Http1Server.h +++ b/src/servers/Http1Server.h @@ -56,7 +56,7 @@ private: /// Return false if parsing is failed, true otherwise. bool buildHttpRequest(Http::StreamPointer &context); - void setReplyError(Http::StreamPointer &context, HttpRequest::Pointer &request, const HttpRequestMethod& method, err_type requestError, Http::StatusCode errStatusCode, const char *requestErrorBytes); + void setReplyError(Http::StreamPointer &context, HttpRequest::Pointer &request, err_type requestError, Http::StatusCode errStatusCode, const char *requestErrorBytes); Http1::RequestParserPointer parser_; HttpRequestMethod method_; ///< parsed HTTP method -- 2.47.2