Segfault when attempting to re-use a curl handle and turning on Proxy NTLM authentication #765

Closed
rcanavan opened this Issue Apr 14, 2016 · 11 comments

Projects

None yet

4 participants

@rcanavan
rcanavan commented Apr 14, 2016 edited

In our unittests, there are two tests that abuse a normal HTTP server as a proxy, just to verify that the correct headers are sent. Both use the same "proxy"; the first teset does not use any proxy authentication, the second attempts to use NTLM. As a result, the second test always segfaults at the following location:

 │3395            /* Same for Proxy NTLM authentication */   
 │3396            if(wantProxyNTLMhttp) {     
>│3397              if(!strequal(needle->proxyuser, check->proxyuser) || 
 │3398                 !strequal(needle->proxypasswd, check->proxypasswd))   
 │3399                continue; 
 │3400            }

because in *check, both proxyuser = 0x0 and proxypasswd = 0x0. The following patch would prevent the segfault:

diff --git a/lib/url.c b/lib/url.c
index d165d9c..ea79292 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -3394,7 +3394,8 @@ ConnectionExists(struct SessionHandle *data,

         /* Same for Proxy NTLM authentication */
         if(wantProxyNTLMhttp) {
-          if(!strequal(needle->proxyuser, check->proxyuser) ||
+          if((check->proxyuser == NULL) || (check->proxypasswd == NULL) ||
+             !strequal(needle->proxyuser, check->proxyuser) ||
              !strequal(needle->proxypasswd, check->proxypasswd))
             continue;
         }

curl/libcurl version

curl-7.48.0 and earlier.

operating system

Ubuntu 15.10, OpenSuSE 13.1

@jay jay added crash HTTP labels Apr 14, 2016
@frenche
Contributor
frenche commented Apr 14, 2016

Ouch, I'm the one who introduced this code :(

However, I think I've seen this segfault and addressed it by adding a check to make sure proxy_user_passwd is set, see:
https://github.com/curl/curl/blob/ccf7a826050fe33aace329b86d77895bdb7dd4c3/lib/url.c#L3126

But perhaps is it isn't enough, maybe password missing could still be a problem, or I suspect that connection reuse might cause such inconsistency, see this block:
https://github.com/curl/curl/blob/ccf7a826050fe33aace329b86d77895bdb7dd4c3/lib/url.c#L5340

Could you suggest sample code or a command that would reproduce?

Thanks!

@frenche
Contributor
frenche commented Apr 14, 2016

Hmm, actually, the verification of proxy_user_passwd is only for the 'needle' conn, not for the 'check' conn.

@frenche
Contributor
frenche commented Apr 14, 2016

Ok, I have it reproduced with:
curl -v -x http://localhost:8181 http://localhost/ --next -x http://localhost:8181 http://localhost/ -U user:pwd --proxy-ntlm

Looking into it - sorry for this bug!

@frenche
Contributor
frenche commented Apr 15, 2016

The crash that I saw back then was with inversed order (first auth, then no auth), however the first fix I wanted was to make it similar to server authentication where the user/pass are never NULL, instead they are empty strings - and this fix works both ways.
Later I opted for the other fix, which doesn't work the other way, so I think I'll go ahead and open a PR for the first fix so the logic of server and proxy auth will be similar.

Quote from back then correspondance:

Now it works fine for my libcurl tests but I encounter a segfault at
when running:
curl -v -x http://proxy:8181 http://host/ -U user:pwd --proxy-ntlm
--next -x http://proxy:8181 http://host/

The crash occurs at 'strequal' because 'needle->proxyuser' and
'needle->proxypasswd' are NULL when passed here: [old link]

The crash doesn't happen when I try the host authentication equivalent
of the above command so I tried to understand the difference.
It turns out that in the host authentication case 'needle->user' and
'needle->passwd' are empty strings - not NULL (the flow differs).

So the additional patch below fixes this issue (and now proxyuser and
proxypasswd are also empty strings):
diff --git a/lib/url.c b/lib/url.c
index cb43138..30f6caf 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -5576,7 +5576,7 @@ static CURLcode create_conn(struct SessionHandle *data,
   /*************************************************************
    * Extract the user and password from the authentication string
    *************************************************************/
-  if(conn->bits.proxy_user_passwd) {
+  if(conn->bits.proxy) {
     result = parse_proxy_auth(data, conn);
     if(result)
       goto out;

A simpler fix with less changes could be to turn on 'wantPoxyNTLM'
only if we have credentials (so we won't reach 'strequal'), like:
diff --git a/lib/url.c b/lib/url.c
index cb43138..7a2ad5a 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -3134,7 +3134,7 @@ ConnectionExists(struct SessionHandle *data,
   bool wantNTLMhttp = ((data->state.authhost.want &
                       (CURLAUTH_NTLM | CURLAUTH_NTLM_WB)) &&
                       (needle->handler->protocol & PROTO_FAMILY_HTTP));
-  bool wantProxyNTLMhttp = (needle->bits.proxy &&
+  bool wantProxyNTLMhttp = (needle->bits.proxy_user_passwd &&
                            ((data->state.authproxy.want &
                            (CURLAUTH_NTLM | CURLAUTH_NTLM_WB)) &&
                            (needle->handler->protocol & PROTO_FAMILY_HTTP)));
@frenche frenche added a commit to frenche/curl that referenced this issue Apr 15, 2016
@frenche frenche Proxy: always populate proxy user and pwd in conn struct
Even if empty, so we won't risk dereferencing NULL pointer.
This is similar to what we do for server user and pwd.

See #765
b53993d
@frenche
Contributor
frenche commented Apr 15, 2016

Hi @bagder do you think (frenche@b53993d) is ok, or you'd prefer a local fix.

Thanks @rcanavan for reporting this.

@jay
Member
jay commented Apr 15, 2016

The previous fix you are referring to is d41dcba? bits.proxy_user_passwd is whether we're using the user and pass for the proxy, similar to bits.user_passwd for the server. (IIRC it may be a little more complicated than that, for example user_passwd is when we're not using a default password; it doesn't necessarily mean no password will be sent if the protocol requires one. think ftp).

It seems correct to me that we don't parse the proxy auth (which basically just unescapes and copies proxy username and pass) if !bits.proxy_user_passwd, unless I misunderstand.

@frenche
Contributor
frenche commented Apr 15, 2016

On Sat, Apr 16, 2016 at 2:08 AM, Jay Satiro notifications@github.com wrote:

The previous fix you are referring to is d41dcba?

Yes, this is the commit in which I introduced this bug.

It seems correct to me that we don't parse the proxy auth (which basically just unescapes and copies proxy username and pass) if !bits.proxy_user_passwd, unless I misunderstand.

Initially, I just added a check for 'strequal(needle->proxyuser,
check->proxyuser)' similar to the check just above for server
authentication (conn->user).
Then I found out that conn->user and conn->proxyuser are not the same
when no user/pwd are provided, while conn->user is a valid pointer to
an empty string conn->proxyuser is NULL so calling strequal cause a
segfault.
Back then, my first hunch was to make sure conn->proxyuser is also a
valid pointer even if empty - b53993d.
However, later I settled with a local fix instead, in ConnectionExists
to only enter the strequal if needle->bits.proxy_user_passwd.
But that only solved the case where the 'needle' conn doesn't have
user/pwd - not the case where the 'check' conn doesn't have user/pwd.

@frenche frenche added a commit to frenche/curl that referenced this issue Apr 16, 2016
@frenche frenche NTLM: check for NULL pointer before deferencing
At ConnectionExists, both check->proxyuser and check->proxypasswd
could be NULL, so make sure to check first.

See #765
3d8736d
@frenche
Contributor
frenche commented Apr 16, 2016

Alternatively, a local fix similar to the one proposed by OP: frenche@3d8736d

@frenche
Contributor
frenche commented Apr 20, 2016

@bagder @jay any thoughts on this?

@bagder
Member
bagder commented Apr 20, 2016

Thanks, merged!

@bagder bagder added a commit that closed this issue Apr 20, 2016
@frenche @bagder frenche + bagder NTLM: check for NULL pointer before deferencing
At ConnectionExists, both check->proxyuser and check->proxypasswd
could be NULL, so make sure to check first.

Fixes #765
fa5fa65
@bagder bagder closed this in fa5fa65 Apr 20, 2016
@frenche
Contributor
frenche commented Apr 20, 2016

Thank you!

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