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

Jump to bottom

Memory leak when using schannel ssl_backend #5855

Closed
fullincome opened this issue Aug 25, 2020 · 3 comments
Closed

Memory leak when using schannel ssl_backend #5855

fullincome opened this issue Aug 25, 2020 · 3 comments
Labels
memory-leak TLS

Comments

@fullincome
Copy link
Contributor

I did this

Build curl with schannel interface as ssl_backend interface on linux.
Test it with valgrind and get something like:

{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   match-leak-kinds: definite
   ...
   fun:_tcsdup
   fun:get_cert_location
   fun:schannel_connect_step1
   fun:schannel_connect_common
   fun:Curl_schannel_connect_nonblocking
   fun:Curl_ssl_connect_nonblocking
   fun:https_connecting
   fun:Curl_http_connect
   fun:Curl_protocol_connect
   fun:multi_runsingle
   fun:curl_multi_perform
   fun:easy_transfer
   fun:easy_perform
   fun:curl_easy_perform
   fun:operate_do
   fun:operate
   fun:main
}

I expected the following

No memory leak

curl/libcurl version

master

operating system

uname -a:
Linux test-x64-ub16 4.4.0-97-generic #120-Ubuntu SMP Tue Sep 19 17:28:18 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

explanation

get_cert_location() can return CURLE_SSL_CERTPROBLEM and allocate memory for store_path. In this case, we lost memory in cert_store_path here:

curl/lib/vtls/schannel.c

Line 618 in 259a815

if(result && !fInCert) {

Perhaps get_cert_location() is slightly incorrect?
Perhaps change the code something like this (or better)?:

diff --git a/lib/vtls/schannel.c b/lib/vtls/schannel.c
index 1c1432d75..a656c8a55 100644
--- a/lib/vtls/schannel.c
+++ b/lib/vtls/schannel.c
@@ -346,6 +346,8 @@ set_ssl_ciphers(SCHANNEL_CRED *schannel_cred, char *ciphers)
 }
 
 #ifdef HAS_CLIENT_CERT_PATH
+
+// Function allocate memory for store_path if CURLE_OK is returned!
 static CURLcode
 get_cert_location(TCHAR *path, DWORD *store_name, TCHAR **store_path,
                   TCHAR **thumbprint)
@@ -388,16 +390,16 @@ get_cert_location(TCHAR *path, DWORD *store_name, TCHAR **store_path,
   if(sep == NULL)
     return CURLE_SSL_CERTPROBLEM;
 
+  *thumbprint = sep + 1;
+  if(_tcslen(*thumbprint) != CERT_THUMBPRINT_STR_LEN)
+    return CURLE_SSL_CERTPROBLEM;
+
   *sep = TEXT('\0');
   *store_path = _tcsdup(store_path_start);
   *sep = TEXT('\\');
   if(*store_path == NULL)
     return CURLE_OUT_OF_MEMORY;
 
-  *thumbprint = sep + 1;
-  if(_tcslen(*thumbprint) != CERT_THUMBPRINT_STR_LEN)
-    return CURLE_SSL_CERTPROBLEM;
-
   return CURLE_OK;
 }
 #endif
@fullincome fullincome changed the title Memory leak if use schannel ssl_backend Memory leak when using schannel ssl_backend Aug 25, 2020
@bagder
Copy link
Member

bagder commented Aug 25, 2020

Build curl with schannel interface as ssl_backend interface on linux.

How do you do get schannel for Linux?

@bagder
Copy link
Member

bagder commented Aug 25, 2020

@fullincome can you convert your suggested patch into a full Pull Request perhaps? (and if you do, just remember we don't do // comments)

@fullincome
Copy link
Contributor Author

Build curl with schannel interface as ssl_backend interface on linux.

How do you do get schannel for Linux?

We have a Linux SSPI implementation in CryptoPro CSP.

@fullincome can you convert your suggested patch into a full Pull Request perhaps? (and if you do, just remember we don't do // comments)

Ok.

@fullincome fullincome mentioned this issue Aug 26, 2020
Closed
@bagder bagder closed this as completed in 01e2679 Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
memory-leak TLS
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

schannel: fix memory leak when using get_cert_location
2 participants
@bagder @fullincome