Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC for CONNECT requests not honored #1317

Closed
izzy opened this issue Mar 8, 2017 · 2 comments
Closed

RFC for CONNECT requests not honored #1317

izzy opened this issue Mar 8, 2017 · 2 comments

Comments

@izzy
Copy link

izzy 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

@jay
Copy link
Member

jay commented Mar 8, 2017

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 added a commit that referenced this issue Mar 11, 2017
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
@jay
Copy link
Member

jay commented Mar 11, 2017

Landed in ec1d0ed, thanks guys.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

2 participants