From: Amos Jeffries <yadij@users.noreply.github.com>
Date: Tue, 20 Jul 2021 19:01:40 +0000 (+0000)
Subject: Reject different HTTP requests with unusual framing (#753)
X-Git-Tag: SQUID_6_0_1~309
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4bd88eb46c74d3c58ae6310f7ec494784e325642;p=thirdparty%2Fsquid.git

Reject different HTTP requests with unusual framing (#753)

... and remove support for request_entities.

Squid now follows the following (approximate) rules when checking HTTP
request framing. The first matching rule wins.

* HTTP requests with a Transfer-Encoding:chunked header, including GET
  and HEAD requests with that header, are accepted. No changes here.

* HTTP requests with unsupported Transfer-Encoding values are rejected
  (Squid replies with HTTP 501 "Not Implemented"). No changes here.

* HTTP requests having conflicting Content-Length values are rejected
  (Squid replies with HTTP 400 "Bad Request"). No changes here.

* HTTP/1.0 and HTTP/0.9 POST and PUT requests without a valid
  Content-Length header are now rejected (Squid replies with HTTP 411
  "Length Required"). All of these were allowed before.

* HTTP/1.0 GET and HEAD requests with a Content-Length:0 header are now
  rejected (Squid replies with HTTP 400 "Bad Request"). All of these
  were allowed before.

* HTTP/1.0 GET and HEAD requests with a positive Content-Length header
  are now rejected (Squid replies with HTTP 400 "Bad Request"). All of
  these were allowed before if and only if the request_entities
  directive was explicitly set to "on".

There are no other framing-related HTTP request restrictions. Prior to
these changes, HTTP/1.1 GET and HEAD requests with a positive
Content-Length header were rejected unless the request_entities
directive was explicitly set to "on". The following configuration sketch
keeps rejecting those requests:

    acl getOrHead method GET HEAD
    acl withContentLength req_header Content-Length .
    http_access deny getOrHead withContentLength

The new restrictions were added due to possibility of cache corruption
attacks and other security issues related to HTTP request framing.

The request_entities directive was removed to simplify decision logic.

Some developers believe that these changes should be accompanied by
configuration options that allow admins to bypass (most of) the
previously absent restrictions. However, these developers do not know of
any important use cases that these changes break, and such cases may not
even exist. The authors insist on these security-driven changes.
---

diff --git a/doc/release-notes/release-6.sgml b/doc/release-notes/release-6.sgml
index d31ee8b4e7..3efeba84a7 100644
--- a/doc/release-notes/release-6.sgml
+++ b/doc/release-notes/release-6.sgml
@@ -69,8 +69,23 @@ This section gives an account of those changes in three categories:
 <sect1>Removed directives<label id="removeddirectives">
 <p>
 <descrip>
-	<p>There have been no directives changed.
-
+	<tag>request_entities</tag>
+	<p>Obsolete. Squid accepts an entity (aka payload, body) on
+	   HTTP/1.1 GET or HEAD requests when a Content-Length or
+	   Transfer-Encoding header is presented to clearly determine size.
+	<p>To retain the old behaviour of rejecting GET/HEAD payloads
+	   for HTTP/1.1 use <em>http_access</em> rules:
+<verb>
+  acl fetch method GET HEAD
+  acl entity req_header Content-Length .
+  http_access deny fetch entity
+</verb>
+	<p>Squid will reject use of Content-Length header on HTTP/1.0
+	   messages with GET, HEAD, DELETE, LINK, UNLINK methods. Since
+	   the HTTP/1.0 specification defines those as not having entities.
+	   To deliver entities on these methods the chunked encoding
+	   feature defined by HTTP/1.1 must be used, or the request
+	   upgraded to an HTTP/1.1 message.
 </descrip>
 
 
diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 6dcce96f7c..4e0c2e8e51 100644
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -651,6 +651,70 @@ HttpRequest::canHandle1xx() const
     return true;
 }
 
+Http::StatusCode
+HttpRequest::checkEntityFraming() const
+{
+    // RFC 7230 section 3.3.1:
+    // "
+    //  A server that receives a request message with a transfer coding it
+    //  does not understand SHOULD respond with 501 (Not Implemented).
+    // "
+    if (header.unsupportedTe())
+        return Http::scNotImplemented;
+
+    // RFC 7230 section 3.3.3 #3 paragraph 3:
+    // Transfer-Encoding overrides Content-Length
+    if (header.chunked())
+        return Http::scNone;
+
+    // RFC 7230 Section 3.3.3 #4:
+    // conflicting Content-Length(s) mean a message framing error
+    if (header.conflictingContentLength())
+        return Http::scBadRequest;
+
+    // HTTP/1.0 requirements differ from HTTP/1.1
+    if (http_ver <= Http::ProtocolVersion(1,0)) {
+        const auto m = method.id();
+
+        // RFC 1945 section 8.3:
+        // "
+        //   A valid Content-Length is required on all HTTP/1.0 POST requests.
+        // "
+        // RFC 1945 Appendix D.1.1:
+        // "
+        //   The fundamental difference between the POST and PUT requests is
+        //   reflected in the different meaning of the Request-URI.
+        // "
+        if (m == Http::METHOD_POST || m == Http::METHOD_PUT)
+            return (content_length >= 0 ? Http::scNone : Http::scLengthRequired);
+
+        // RFC 1945 section 7.2:
+        // "
+        //   An entity body is included with a request message only when the
+        //   request method calls for one.
+        // "
+        // section 8.1-2: GET and HEAD do not define ('call for') an entity
+        if (m == Http::METHOD_GET || m == Http::METHOD_HEAD)
+            return (content_length < 0 ? Http::scNone : Http::scBadRequest);
+        // appendix D1.1.2-4: DELETE, LINK, UNLINK do not define ('call for') an entity
+        if (m == Http::METHOD_DELETE || m == Http::METHOD_LINK || m == Http::METHOD_UNLINK)
+            return (content_length < 0 ? Http::scNone : Http::scBadRequest);
+
+        // other methods are not defined in RFC 1945
+        // assume they support an (optional) entity
+        return Http::scNone;
+    }
+
+    // RFC 7230 section 3.3
+    // "
+    //   The presence of a message body in a request is signaled by a
+    //   Content-Length or Transfer-Encoding header field.  Request message
+    //   framing is independent of method semantics, even if the method does
+    //   not define any use for a message body.
+    // "
+    return Http::scNone;
+}
+
 bool
 HttpRequest::parseHeader(Http1::Parser &hp)
 {
diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index 7a319f226d..820f4f01de 100644
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -248,6 +248,10 @@ public:
 
     virtual void configureContentLengthInterpreter(Http::ContentLengthInterpreter &) {}
 
+    /// Check whether the message framing headers are valid.
+    /// \returns Http::scNone or an HTTP error status
+    Http::StatusCode checkEntityFraming() const;
+
     /// Parses request header using Parser.
     /// Use it in contexts where the Parser object is available.
     bool parseHeader(Http1::Parser &hp);
diff --git a/src/SquidConfig.h b/src/SquidConfig.h
index 0117ea7808..f9c2ff39f9 100644
--- a/src/SquidConfig.h
+++ b/src/SquidConfig.h
@@ -318,7 +318,6 @@ public:
 
         int vary_ignore_expire;
         int surrogate_is_remote;
-        int request_entities;
         int detect_broken_server_pconns;
         int relaxed_header_parser;
         int check_hostnames;
diff --git a/src/cf.data.pre b/src/cf.data.pre
index a5d0624b7e..be6741ec2e 100644
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -181,6 +181,14 @@ DOC_START
 	This option is not yet supported by Squid-3.
 DOC_END
 
+# Options removed in 6.x
+NAME: request_entities
+TYPE: obsolete
+DOC_START
+	Remove this line. Squid now accepts HTTP/1.1 requests with bodies.
+	To simplify UI and code, Squid rejects certain HTTP/1.0 requests with bodies.
+DOC_END
+
 # Options removed in 5.x
 NAME: dns_v4_first
 TYPE: obsolete
@@ -6758,22 +6766,6 @@ DOC_START
 	varying objects not intended for caching to get cached.
 DOC_END
 
-NAME: request_entities
-TYPE: onoff
-LOC: Config.onoff.request_entities
-DEFAULT: off
-DOC_START
-	Squid defaults to deny GET and HEAD requests with request entities,
-	as the meaning of such requests are undefined in the HTTP standard
-	even if not explicitly forbidden.
-
-	Set this directive to on if you have clients which insists
-	on sending request entities in GET or HEAD requests. But be warned
-	that there is server software (both proxies and web servers) which
-	can fail to properly process this kind of request which may make you
-	vulnerable to cache pollution attacks if enabled.
-DOC_END
-
 NAME: request_header_access
 IFDEF: USE_HTTP_VIOLATIONS
 TYPE: http_header_access
diff --git a/src/client_side.cc b/src/client_side.cc
index 004f9c907d..8e1659a028 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -180,7 +180,6 @@ static IOACB httpAccept;
 #if USE_IDENT
 static IDCB clientIdentDone;
 #endif
-static int clientIsContentLengthValid(HttpRequest * r);
 static int clientIsRequestBodyTooLargeForPolicy(int64_t bodyLength);
 
 static void clientUpdateStatHistCounters(const LogTags &logType, int svc_time);
@@ -698,31 +697,6 @@ clientSetKeepaliveFlag(ClientHttpRequest * http)
     request->flags.proxyKeepalive = request->persistent();
 }
 
-/// checks body length of non-chunked requests
-static int
-clientIsContentLengthValid(HttpRequest * r)
-{
-    // No Content-Length means this request just has no body, but conflicting
-    // Content-Lengths mean a message framing error (RFC 7230 Section 3.3.3 #4).
-    if (r->header.conflictingContentLength())
-        return 0;
-
-    switch (r->method.id()) {
-
-    case Http::METHOD_GET:
-
-    case Http::METHOD_HEAD:
-        /* We do not want to see a request entity on GET/HEAD requests */
-        return (r->content_length <= 0 || Config.onoff.request_entities);
-
-    default:
-        /* For other types of requests we don't care */
-        return 1;
-    }
-
-    /* NOT REACHED */
-}
-
 int
 clientIsRequestBodyTooLargeForPolicy(int64_t bodyLength)
 {
@@ -1697,11 +1671,9 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
         request->http_ver.minor = http_ver.minor;
     }
 
-    const auto unsupportedTe = request->header.unsupportedTe();
-
     mustReplyToOptions = (request->method == Http::METHOD_OPTIONS) &&
                          (request->header.getInt64(Http::HdrType::MAX_FORWARDS) == 0);
-    if (!urlCheckRequest(request.getRaw()) || mustReplyToOptions || unsupportedTe) {
+    if (!urlCheckRequest(request.getRaw()) || mustReplyToOptions) {
         clientStreamNode *node = context->getClientReplyContext();
         conn->quitAfterError(request.getRaw());
         clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
@@ -1714,15 +1686,13 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
         return;
     }
 
-    const auto chunked = request->header.chunked();
-    if (!chunked && !clientIsContentLengthValid(request.getRaw())) {
+    const auto frameStatus = request->checkEntityFraming();
+    if (frameStatus != Http::scNone) {
         clientStreamNode *node = context->getClientReplyContext();
         clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
         assert (repContext);
         conn->quitAfterError(request.getRaw());
-        repContext->setReplyToError(ERR_INVALID_REQ,
-                                    Http::scLengthRequired, request->method, NULL,
-                                    conn, request.getRaw(), nullptr, nullptr);
+        repContext->setReplyToError(ERR_INVALID_REQ, frameStatus, request->method, nullptr, conn, request.getRaw(), nullptr, nullptr);
         assert(context->http->out.offset == 0);
         context->pullData();
         clientProcessRequestFinished(conn, request);
@@ -1744,6 +1714,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
 #endif
 
     /* Do we expect a request-body? */
+    const auto chunked = request->header.chunked();
     expectBody = chunked || request->content_length > 0;
     if (!context->mayUseConnection() && expectBody) {
         request->body_pipe = conn->expectRequestBody(
diff --git a/src/http/MethodType.h b/src/http/MethodType.h
index 41fa7c25e2..7842ce156c 100644
--- a/src/http/MethodType.h
+++ b/src/http/MethodType.h
@@ -31,11 +31,9 @@ typedef enum _method_t {
     METHOD_OPTIONS,
     METHOD_DELETE,
 
-#if NO_SPECIAL_HANDLING
     // RFC 2068
     METHOD_LINK,
     METHOD_UNLINK,
-#endif
 
     // RFC 3253
     METHOD_CHECKOUT,