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

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

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

Comments

@ashman-p
Copy link

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

@iboukris
Copy link
Contributor

iboukris commented Nov 8, 2016

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
Copy link
Author

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//issues/1115#issuecomment-259247890, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AWQm5W0R8H-_9sYHl_U5wQyDJlddDgt5ks5q8NlXgaJpZM4Ksoz-.

@iboukris
Copy link
Contributor

iboukris 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.

iboukris added a commit to iboukris/curl that referenced this issue Nov 9, 2016
If SPNEGO fails, cleanup the negotiate handle right away.

Fixes curl#1115

Signed-off-by: Isaac Boukris <iboukris@gmail.com>
Reported-by: ashman-p
@bagder bagder closed this as completed in 46f906a Nov 9, 2016
@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants