Memory Leak Observed in libcurl With Negotiate (Kerberos) Failure case #1115

Closed
ashman-p opened this Issue Nov 8, 2016 · 3 comments

Projects

None yet

3 participants

@ashman-p
ashman-p commented Nov 8, 2016

I did this

Access a web -site protected with Kerberos but, the client krb ticket is not available.

I expected the following

Access was rejected as expected but, valgrind reported a memory leak.
The leak seems to occur because Curl_http_done() is not called if the context init fails when there is no ticket (or it has expired???).

curl/libcurl version

curl-7.50.3
(and earlier versions as well)

[curl -V output perhaps?]
curl 7.50.3 (x86_64-pc-linux-gnu) libcurl/7.50.3 OpenSSL/1.0.2h zlib/1.2.7
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP UnixSockets

operating system

Centos

Test App Src that can recreate the issue

int main(int argc, char* argv[])
{
CURLcode result;
int x;
char url[1024];

if (argc != 5) {
    printf("Enter usrID, IP, K or N, Passwd\n");
    return 1;
}
for (x =0; x<1023; x++)
    url[x] = 0;

sprintf(url, "http://%s/THE-MS-URL”, argv[2]);
CURL* curl = curl_easy_init();
if(curl){
    curl_easy_setopt(curl,CURLOPT_USERNAME, argv[1]);  
    curl_easy_setopt(curl, CURLOPT_URL, url);

    if(strcmp("K", argv[3]) == 0)
        result = curl_easy_setopt(curl,CURLOPT_HTTPAUTH, CURLAUTH_GSSNEGOTIATE);    
    else {
        result = curl_easy_setopt(curl,CURLOPT_HTTPAUTH, CURLAUTH_NTLM);    
        curl_easy_setopt(curl,CURLOPT_PASSWORD, argv[4]);
    }


    curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, "false");
    curl_easy_setopt(curl, CURLOPT_USERAGENT, “My Agent”);
    result = curl_easy_setopt(curl, CURLOPT_KRBLEVEL, "private");

    curl_easy_perform(curl);
    curl_easy_cleanup(curl);
}
return 0;

}

Patch that is a possible fix

Attached....
http.c.curl-7.50.patch.txt

@frenche
Contributor
frenche commented Nov 8, 2016 edited

Hi @ashman-p, I could reproduce the leak with the below curl command:

$ valgrind --leak-check=full curl -v -u: --negotiate http://ms.frenche.cp/mika/
==16894== Memcheck, a memory error detector
==16894== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==16894== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==16894== Command: curl -v -u: --negotiate http://ms.frenche.cp/mika/
==16894==
*   Trying 10.0.0.200...
* TCP_NODELAY set
* Connected to ms.frenche.cp (10.0.0.200) port 80 (#0)
> GET /mika/ HTTP/1.1
> Host: ms.frenche.cp
> User-Agent: curl/7.51.1-DEV
> Accept: */*
>
< HTTP/1.1 401 Unauthorized
< Content-Length: 1656
< Content-Type: text/html
< Server: Microsoft-IIS/6.0
* gss_init_sec_context() failed: SPNEGO cannot find mechanisms to negotiate.
< WWW-Authenticate: Negotiate
< WWW-Authenticate: NTLM
< WWW-Authenticate: Basic realm="frenche.cp"
< X-Powered-By: ASP.NET
< Date: Tue, 08 Nov 2016 19:11:29 GMT
<
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
...
* Curl_http_done: called premature == 0
* Connection #0 to host ms.frenche.cp left intact
==16894==
==16894== HEAP SUMMARY:
==16894==     in use at exit: 457 bytes in 19 blocks
==16894==   total heap usage: 5,045 allocs, 5,026 frees, 211,832 bytes allocated
==16894==
==16894== 64 (20 direct, 44 indirect) bytes in 1 blocks are definitely lost in loss record 17 of 19
==16894==    at 0x402B289: malloc (vg_replace_malloc.c:299)
==16894==    by 0x4371C80: gss_import_name (g_imp_name.c:110)
==16894==    by 0x408813D: Curl_auth_decode_spnego_message (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
==16894==    by 0x406D5EA: Curl_input_negotiate (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
==16894==    by 0x4048012: Curl_http_input_auth (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
==16894==    by 0x404BE03: Curl_http_readwrite_headers (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
==16894==    by 0x4060588: Curl_readwrite (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
==16894==    by 0x406B43A: multi_runsingle (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
==16894==    by 0x406BF73: curl_multi_perform (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
==16894==    by 0x40623C4: curl_easy_perform (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
==16894==    by 0x8053922: operate_do (in /home/admin/git/curl/src/.libs/curl)
==16894==    by 0x805403A: operate (in /home/admin/git/curl/src/.libs/curl)
==16894==
==16894== LEAK SUMMARY:
==16894==    definitely lost: 20 bytes in 1 blocks
==16894==    indirectly lost: 44 bytes in 4 blocks
==16894==      possibly lost: 0 bytes in 0 blocks
==16894==    still reachable: 393 bytes in 14 blocks
==16894==         suppressed: 0 bytes in 0 blocks
==16894== Reachable blocks (those to which a pointer was found) are not shown.
==16894== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==16894==
==16894== For counts of detected and suppressed errors, rerun with: -v
==16894== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

I can confirm your fix does solve the leak problem, but perhaps we could instead simply cleanup the negotiate data immediately right when it fails, something like:

$ git diff
diff --git a/lib/vauth/spnego_gssapi.c b/lib/vauth/spnego_gssapi.c
index 8840db8..213f4eb 100644
--- a/lib/vauth/spnego_gssapi.c
+++ b/lib/vauth/spnego_gssapi.c
@@ -169,6 +169,7 @@ CURLcode Curl_auth_decode_spnego_message(struct Curl_easy *data,

     Curl_gss_log_error(data, "gss_init_sec_context() failed: ",
                        major_status, minor_status);
+    Curl_auth_spnego_cleanup(nego);

     return CURLE_OUT_OF_MEMORY;
   }

What do you think?

@ashman-p
ashman-p commented Nov 8, 2016

Thanks very much. I like your solution much better. Very nice.
Thanks.

On Nov 8, 2016, at 3:19 PM, Isaac Boukris <notifications@github.commailto:notifications@github.com> wrote:

Hi @ashman-phttps://github.com/ashman-p, I could reproduce the leak with the below curl command:

$ valgrind --leak-check=full curl -v -u: --negotiate http://ms.frenche.cp/mika/
==16894== Memcheck, a memory error detector
==16894== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==16894== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==16894== Command: curl -v -u: --negotiate http://ms.frenche.cp/mika/
==16894==

  • Trying 10.0.0.200...
  • TCP_NODELAY set
  • Connected to ms.frenche.cp (10.0.0.200) port 80 (#0)

    GET /mika/ HTTP/1.1
    Host: ms.frenche.cp
    User-Agent: curl/7.51.1-DEV
    Accept: /

    < HTTP/1.1 401 Unauthorized
    < Content-Length: 1656
    < Content-Type: text/html
    < Server: Microsoft-IIS/6.0
  • gss_init_sec_context() failed: SPNEGO cannot find mechanisms to negotiate.
    < WWW-Authenticate: Negotiate
    < WWW-Authenticate: NTLM
    < WWW-Authenticate: Basic realm="frenche.cp"
    < X-Powered-By: ASP.NEThttp://asp.net
    < Date: Tue, 08 Nov 2016 19:11:29 GMT
    < ...
  • Curl_http_done: called premature == 0
  • Connection #0 to host ms.frenche.cp left intact
    ==16894==
    ==16894== HEAP SUMMARY:
    ==16894== in use at exit: 457 bytes in 19 blocks
    ==16894== total heap usage: 5,045 allocs, 5,026 frees, 211,832 bytes allocated
    ==16894==
    ==16894== 64 (20 direct, 44 indirect) bytes in 1 blocks are definitely lost in loss record 17 of 19
    ==16894== at 0x402B289: malloc (vg_replace_malloc.c:299)
    ==16894== by 0x4371C80: gss_import_name (g_imp_name.c:110)
    ==16894== by 0x408813D: Curl_auth_decode_spnego_message (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
    ==16894== by 0x406D5EA: Curl_input_negotiate (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
    ==16894== by 0x4048012: Curl_http_input_auth (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
    ==16894== by 0x404BE03: Curl_http_readwrite_headers (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
    ==16894== by 0x4060588: Curl_readwrite (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
    ==16894== by 0x406B43A: multi_runsingle (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
    ==16894== by 0x406BF73: curl_multi_perform (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
    ==16894== by 0x40623C4: curl_easy_perform (in /home/admin/git/curl/lib/.libs/libcurl.so.4.4.0)
    ==16894== by 0x8053922: operate_do (in /home/admin/git/curl/src/.libs/curl)
    ==16894== by 0x805403A: operate (in /home/admin/git/curl/src/.libs/curl)
    ==16894==
    ==16894== LEAK SUMMARY:
    ==16894== definitely lost: 20 bytes in 1 blocks
    ==16894== indirectly lost: 44 bytes in 4 blocks
    ==16894== possibly lost: 0 bytes in 0 blocks
    ==16894== still reachable: 393 bytes in 14 blocks
    ==16894== suppressed: 0 bytes in 0 blocks
    ==16894== Reachable blocks (those to which a pointer was found) are not shown.
    ==16894== To see them, rerun with: --leak-check=full --show-leak-kinds=all
    ==16894==
    ==16894== For counts of detected and suppressed errors, rerun with: -v
    ==16894== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

I can confirm your fix does solve the leak problem, but perhaps we could simply cleanup the negotiate data immediately right when it fails, something like:

$ git diff
diff --git a/lib/vauth/spnego_gssapi.c b/lib/vauth/spnego_gssapi.c
index 8840db8..213f4eb 100644
--- a/lib/vauth/spnego_gssapi.c
+++ b/lib/vauth/spnego_gssapi.c
@@ -169,6 +169,7 @@ CURLcode Curl_auth_decode_spnego_message(struct Curl_easy *data,

 Curl_gss_log_error(data, "gss_init_sec_context() failed: ",
                    major_status, minor_status);
  • Curl_auth_spnego_cleanup(nego);

return CURLE_OUT_OF_MEMORY;
}

What do you think?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/curl/curl/issues/1115#issuecomment-259247890, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AWQm5W0R8H-_9sYHl_U5wQyDJlddDgt5ks5q8NlXgaJpZM4Ksoz-.

@frenche
Contributor
frenche commented Nov 8, 2016

Oh, I think this might be needed for windows SSPI as well, so maybe we should move the cleanup logic to the caller, see below:

diff --git a/lib/http_negotiate.c b/lib/http_negotiate.c
index eb17ed4..5ea9f34 100644
--- a/lib/http_negotiate.c
+++ b/lib/http_negotiate.c
@@ -37,6 +37,7 @@
 CURLcode Curl_input_negotiate(struct connectdata *conn, bool proxy,
                               const char *header)
 {
+  CURLcode result;
   struct Curl_easy *data = conn->data;
   size_t len;

@@ -89,8 +90,13 @@ CURLcode Curl_input_negotiate(struct connectdata *conn, bool proxy,
   }

   /* Initilise the security context and decode our challenge */
-  return Curl_auth_decode_spnego_message(data, userp, passwdp, service, host,
-                                         header, neg_ctx);
+  result = Curl_auth_decode_spnego_message(data, userp, passwdp, service,
+                                           host, header, neg_ctx);
+
+  if(result)
+    Curl_auth_spnego_cleanup(neg_ctx);
+
+  return result;
 }

 CURLcode Curl_output_negotiate(struct connectdata *conn, bool proxy)

Alternatively, we can simply do the same cleanup in spnego_sspi.c file as well.

@bagder bagder added the memory-leak label Nov 8, 2016
@frenche frenche added a commit to frenche/curl that referenced this issue Nov 9, 2016
@frenche frenche Fix memory leak when SPNEGO authentication fails
If SPNEGO fails, cleanup the negotiate handle right away.

Fixes #1115

Signed-off-by: Isaac Boukris <iboukris@gmail.com>
Reported-by: ashman-p
4e5d5dc
@bagder bagder added a commit that closed this issue Nov 9, 2016
@frenche @bagder frenche + bagder SPNEGO: Fix memory leak when authentication fails
If SPNEGO fails, cleanup the negotiate handle right away.

Fixes #1115

Signed-off-by: Isaac Boukris <iboukris@gmail.com>
Reported-by: ashman-p
46f906a
@bagder bagder closed this in 46f906a Nov 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment