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

ntlm: avoid malloc(0) for zero length passwords #2054

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@bagder
Member

bagder commented Nov 4, 2017

It triggers an assert() when built with memdebug since malloc(0) may
return NULL or a valid pointer.

Detected by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=4054

Assisted-by: Max Dymond

ntlm: avoid malloc(0) for zero length passwords
It triggers an assert() when built with memdebug since malloc(0) may
return NULL *or* a valid pointer.

Detected by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=4054

Assisted-by: Max Dymond
@@ -557,7 +557,7 @@ CURLcode Curl_ntlm_core_mk_nt_hash(struct Curl_easy *data,
unsigned char *ntbuffer /* 21 bytes */)
{
size_t len = strlen(password);
unsigned char *pw = malloc(len * 2);
unsigned char *pw = len ? malloc(len * 2) : strdup("");

This comment has been minimized.

@jay

jay Nov 4, 2017

Member

I wonder if it would be better practice to just terminate the pw even though technically it doesn't need it

diff --git a/lib/curl_ntlm_core.c b/lib/curl_ntlm_core.c
index 5154949..0a825d8 100644
--- a/lib/curl_ntlm_core.c
+++ b/lib/curl_ntlm_core.c
@@ -557,12 +557,12 @@ CURLcode Curl_ntlm_core_mk_nt_hash(struct Curl_easy *data,
                                    unsigned char *ntbuffer /* 21 bytes */)
 {
   size_t len = strlen(password);
-  unsigned char *pw = malloc(len * 2);
+  unsigned char *pw = malloc((len + 1) * 2);
   CURLcode result;
   if(!pw)
     return CURLE_OUT_OF_MEMORY;
 
-  ascii_to_unicode_le(pw, password, len);
+  ascii_to_unicode_le(pw, password, len + 1);
 
   /*
    * The NT hashed password needs to be created using the password in the
@jay

jay Nov 4, 2017

Member

I wonder if it would be better practice to just terminate the pw even though technically it doesn't need it

diff --git a/lib/curl_ntlm_core.c b/lib/curl_ntlm_core.c
index 5154949..0a825d8 100644
--- a/lib/curl_ntlm_core.c
+++ b/lib/curl_ntlm_core.c
@@ -557,12 +557,12 @@ CURLcode Curl_ntlm_core_mk_nt_hash(struct Curl_easy *data,
                                    unsigned char *ntbuffer /* 21 bytes */)
 {
   size_t len = strlen(password);
-  unsigned char *pw = malloc(len * 2);
+  unsigned char *pw = malloc((len + 1) * 2);
   CURLcode result;
   if(!pw)
     return CURLE_OUT_OF_MEMORY;
 
-  ascii_to_unicode_le(pw, password, len);
+  ascii_to_unicode_le(pw, password, len + 1);
 
   /*
    * The NT hashed password needs to be created using the password in the

This comment has been minimized.

@bagder

bagder Nov 4, 2017

Member

Thanks! So you think so? I think that approach invites suspicions that the zero termination is needed there and cast doubts as to why all functions later on are passing in the length separately.

I actually prefer my approach. I tried making the zero-length path avoid the allocation completely but I think that just made the code worse.

@bagder

bagder Nov 4, 2017

Member

Thanks! So you think so? I think that approach invites suspicions that the zero termination is needed there and cast doubts as to why all functions later on are passing in the length separately.

I actually prefer my approach. I tried making the zero-length path avoid the allocation completely but I think that just made the code worse.

This comment has been minimized.

@jay

jay Nov 4, 2017

Member

fair enough

@jay

jay Nov 4, 2017

Member

fair enough

@bagder bagder closed this in 685ef13 Nov 4, 2017

@bagder bagder deleted the bagder/zero-length-ntlm-passwd branch Nov 6, 2017

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