http: fix CONNECT_ONLY with Negotiate authentication

[tvbuehler]

CONNECT-only connections were closed immediately before the
user had a change to extract the socket when the proxy required
Negotiate authentication.

This is probably a regression from 79b9d5f (CVE-2015-3148).

[bagder]

Ok, that explains the added check for data->set.connect_only, but why the check that makes a HTTP response 200 not close it? The security problem you link to is about bad re-use of Negotiate connections so we added that forced close (as a work-around I should say, but nobody has fixed the problem any better since then). By undoing the close for the 200 case, don't you effectively bring the problem back?

[tvbuehler]

I might be wrong, but I think connect_only can only be set if we are actually in a "CONNECT" request; when CONNECT results in a 200 this means the HTTP part is done, and we now basically got a raw TCP connection - there are no follow-up HTTP requests. Any other response code means the CONNECT itself failed, and we might send more HTTP requests on the same connection.

[iboukris]

I think 'data->set.connect_only' actually refers to CURLOPT_CONNECT_ONLY and not Proxy CONNECT request.
Sounds similar to issue #369 (and not an issue with CURLOPT_CONNECT_ONLY).

[bagder]

Yes it does, I modified the title to make it more obvious.

[iboukris]

I meant to say that it looks like @tvbuehler intention is to fix the proxy CONNECT issue (similar to #369)
So I am not sure we have a problem indeed with CURLOPT_CONNECT_ONLY.

[tvbuehler]

My colleague just pointed out that we use CURLOPT_CONNECT_ONLY (connect_only) + CURLOPT_HTTPPROXYTUNNEL (tunnel_thru_httpproxy) to get a HTTP proxy (authenticated) TCP connection.

So yes, this basically is #369. Although for the connect-only case the fix should be easier: it is up to the curl user to decide whether and how to (re)use the connection.

For the generic case tunnel_thru_httpproxy should probably close the connection on top of the tunnel instead of the tunnel itself - you still want to execute the request on top of it.

Didn't curl use to compare credentials before reusing connections? I thought it did (the "normal" credentials, not the proxy credentials - it was buggy iirc), but cannot find the code anymore.

[iboukris]

Strange though, I have made some tests and I get no error using negotiate and HTTPS target server via proxy (tried several combinations - proxy-negotiate, server-negotiate, both, ntlm, krb...).
Perhaps the issue only arises when setting CURLOPT_CONNECT_ONLY as well.

[tvbuehler]

Yes; I'm guessing in case of an actual HTTP request the connection is only closed at the end of a request; for CONNECTs (this issue) and tunneling of other transports (#369) though the successful connection establishing already counts as "request end".

[MarcelRaad]

Could this be considered for 7.48.0? We currently have to patch all new libcurl versions as CONNECT_ONLY through proxies requiring Negotiate authentication isn't working at all anymore since 7.42.0.

[iboukris]

Why do you need the '|| data->req.httpcode != 200' part?
It defeats the purpose of this whole section.
Does it work for you if you omit it?

In my opinion, we could indeed escape the check in 'connect_only' mode as libcurl has no longer control over the connection anyway so its the responsibility of the application to use the authenticated connection correctly.

[MarcelRaad]

I'm a colleague of @tvbuehler 's and he's currently inactive on github, so I'll answer for him: yes, omitting the check for HTTP status code 200 works too. If I recall correctly, we added it as we thought only of our use case of extracting the socket after a successful HTTP CONNECT and this was the least invasive change for that use case.

[iboukris]

On Sun, Feb 14, 2016 at 1:11 AM, Marcel Raad notifications@github.com
wrote:

In lib/http.c #520 (comment)
:

@@ -1456,8 +1456,10 @@ CURLcode Curl_http_done(struct connectdata conn,
data->state.negotiate.state == GSS_AUTHSENT) {
/ add forbid re-use if http-code != 401/407 as a WA only needed for
* 401/407 that signal auth failure (empty) otherwise state will be RECV

• \* with current code */
  

• if((data->req.httpcode != 401) && (data->req.httpcode != 407))

• \* with current code.
  

• \* Do NOT close successful CONNECT-only connections. */
  

• if((data->req.httpcode != 401) && (data->req.httpcode != 407) &&

•   (!data->set.connect_only || data->req.httpcode != 200))
  

I'm a colleague of @tvbuehler https://github.com/tvbuehler 's and he's
currently inactive on github, so I'll answer for him: yes, omitting the
check for HTTP status code 200 works too. If I recall correctly, we added
it as we thought only of our use case of extracting the socket after a
successful HTTP CONNECT and this was the least invasive change for that use
case.

Then in my opinion a corrected patch which omits the '|| data->req.httpcode
!= 200' part, will be good for time being (till we have a full fix for
negotiate).

Regards.

[MarcelRaad]

Then in my opinion a corrected patch which omits the '|| data->req.httpcode
!= 200' part, will be good for time being (till we have a full fix for
negotiate).

Thanks, new patch here: #655

[MarcelRaad]

This has already been merged including a fix for @Frenche 's review comment in #655 / c2b3f26.