RFC for CONNECT requests not honored #1317

Closed
mkzero opened this Issue Mar 8, 2017 · 2 comments

Projects

None yet

2 participants

@mkzero
mkzero commented Mar 8, 2017

I got an HTTP proxy that was working until now, which will send the Transfer-Encoding and Content-Length headers for a CONNECT request before sending the blank line(start of tunneling):

< HTTP/1.1 200 CONNECT OK
< Transfer-Encoding: chunked

To either of which curl will respond with an error like this:

* Transfer-Encoding: in 200 response
* Connection #0 to host localhost left intact
curl: (56) Transfer-Encoding: in 200 response

In lib/http_proxy.c, the code responsible for that errror is even commented with the right RFC - quoting the first part of RFC 7231 section 4.3.6. Sadly, the part important for curl as a client is completely ignored:

A client MUST ignore any Content-Length or Transfer-Encoding
header fields received in a successful response to CONNECT.

The proxy still worked in 7.47.0, with the recent upgrades on my test systems to 7.52.1(Debian 9) and 7.53.1(Arch Linux) this didn't work anymore

Owner
jay commented Mar 8, 2017 edited

bisected to c50b878 which is:

CONNECT: reject TE or CL in 2xx responses

A server MUST NOT send any Transfer-Encoding or Content-Length header
fields in a 2xx (Successful) response to CONNECT. (RFC 7231 section
4.3.6)

Also fixes the three test cases that did this.

RFC 7231 4.3.6:

   A server MUST NOT send any Transfer-Encoding or Content-Length header
   fields in a 2xx (Successful) response to CONNECT.  A client MUST
   ignore any Content-Length or Transfer-Encoding header fields received
   in a successful response to CONNECT.

I think we should do what the RFC says here, clearly this is a buggy premise but still.

Here is a patch, if there is a +1 I'll write up a test case and land it.

diff --git a/lib/http_proxy.c b/lib/http_proxy.c
index 7fde11d..a673286 100644
--- a/lib/http_proxy.c
+++ b/lib/http_proxy.c
@@ -515,33 +515,34 @@ CURLcode Curl_proxyCONNECT(struct connectdata *conn,
         }
         else if(checkprefix("Content-Length:", line_start)) {
           if(k->httpcode/100 == 2) {
-            /* A server MUST NOT send any Transfer-Encoding or
-               Content-Length header fields in a 2xx (Successful)
-               response to CONNECT. (RFC 7231 section 4.3.6) */
-            failf(data, "Content-Length: in %03d response",
+            /* A client MUST ignore any Content-Length or Transfer-Encoding
+               header fields received in a successful response to CONNECT.
+               "Successful" described as: 2xx (Successful). RFC 7231 4.3.6 */
+            infof(data, "Ignoring Content-Length in CONNECT %03d response\n",
                   k->httpcode);
-            return CURLE_RECV_ERROR;
           }
-
-          cl = curlx_strtoofft(line_start +
-                               strlen("Content-Length:"), NULL, 10);
+          else {
+            cl = curlx_strtoofft(line_start +
+                                 strlen("Content-Length:"), NULL, 10);
+          }
         }
         else if(Curl_compareheader(line_start, "Connection:", "close"))
           closeConnection = TRUE;
-        else if(Curl_compareheader(line_start,
-                                   "Transfer-Encoding:",
-                                   "chunked")) {
+        else if(checkprefix("Transfer-Encoding:", line_start)) {
           if(k->httpcode/100 == 2) {
-            /* A server MUST NOT send any Transfer-Encoding or
-               Content-Length header fields in a 2xx (Successful)
-               response to CONNECT. (RFC 7231 section 4.3.6) */
-            failf(data, "Transfer-Encoding: in %03d response", k->httpcode);
-            return CURLE_RECV_ERROR;
+            /* A client MUST ignore any Content-Length or Transfer-Encoding
+               header fields received in a successful response to CONNECT.
+               "Successful" described as: 2xx (Successful). RFC 7231 4.3.6 */
+            infof(data, "Ignoring Transfer-Encoding in "
+                  "CONNECT %03d response\n", k->httpcode);
+          }
+          else if(Curl_compareheader(line_start,
+                                     "Transfer-Encoding:", "chunked")) {
+            infof(data, "CONNECT responded chunked\n");
+            chunked_encoding = TRUE;
+            /* init our chunky engine */
+            Curl_httpchunk_init(conn);
           }
-          infof(data, "CONNECT responded chunked\n");
-          chunked_encoding = TRUE;
-          /* init our chunky engine */
-          Curl_httpchunk_init(conn);
         }
         else if(Curl_compareheader(line_start, "Proxy-Connection:", "close"))
           closeConnection = TRUE;
@jay jay added a commit that referenced this issue Mar 11, 2017
@jay jay http_proxy: Ignore TE and CL in CONNECT 2xx responses
A client MUST ignore any Content-Length or Transfer-Encoding header
fields received in a successful response to CONNECT.
"Successful" described as: 2xx (Successful). RFC 7231 4.3.6

Prior to this change such a case would cause an error.

In some ways this bug appears to be a regression since c50b878. Prior to
that libcurl may have appeared to function correctly in such cases by
acting on those headers instead of causing an error. But that behavior
was also incorrect.

Bug: #1317
Reported-by: mkzero@users.noreply.github.com
ec1d0ed
Owner
jay commented Mar 11, 2017

Landed in ec1d0ed, thanks guys.

@jay jay closed this Mar 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment