1192020 - MSan: use-of-uninitialized-value in nssutil_DupnCat()

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: call_stack

This seems to be triggered by a call to NSS_NoDB_Init(NULL).

I'm not sure about the security implications here so I'm making this as a sec bug. Feel free to change it if need be.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Mass cc to get some NSS eyes on these bugs.

Comment 2

[Robert Relyea]

--- a/lib/util/utilmod.c
+++ b/lib/util/utilmod.c
@@ -87,17 +87,19 @@ nssutil_DupnCat(char *baseString, const 
 
     len += str_len;
     newString = (char *) PORT_Realloc(baseString,len);
     if (newString == NULL) {
 	PORT_Free(baseString);
 	return NULL;
     }
     if (baseString == NULL) *newString = 0;
-    return PORT_Strncat(newString,str, str_len);
+    newString[len]=0; /* Strncat will not copy the NULL in str. Make sure the
+		       * resulting string terminates */
+    return PORT_Strncat(newString, str, str_len);
 }
 
 /* Same as nssutil_DupnCat except it concatenates the full string, not a
  * partial one */
 static char *
 nssutil_DupCat(char *baseString, const char *str)
 {


I don't think this has a security exploit unless the user has access to pkcs11.txt, which is the source of the module strings, but it should get fixed.

Comment 3

[Robert Relyea]

Attachment: Make sure the string is null terminated on return from nssutil_DupnCat().

Comment 4

[Wan-Teh Chang]

Comment on attachment 8647748 [details] [diff] [review]
Make sure the string is null terminated on return from nssutil_DupnCat().

Review of attachment 8647748 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/util/utilmod.c
@@ +91,5 @@
>  	PORT_Free(baseString);
>  	return NULL;
>      }
>      if (baseString == NULL) *newString = 0;
> +    newString[len]=0; /* Strncat will not copy the NULL in str. Make sure the

1. Nit: add spaces around the '=' sign.

2. The comment on line 78 is wrong:

   * The first parameter is the source string. If it's null, we

"source" should be changed to "destination".

3. Nit: This function can use memcpy instead of strncat so that it
doesn't scan baseString twice. This should work:

static char *
nssutil_DupnCat(char *baseString, const char *str, int str_len)
{
    int baseStringLen = baseString ? PORT_Strlen(baseString) : 0;
    int len = baseStringLen + 1;
    char *newString;

    len += str_len;
    newString = (char *) PORT_Realloc(baseString,len);
    if (newString == NULL) {
        PORT_Free(baseString);
        return NULL;
    }
    PORT_Memcpy(&newString[baseStringLen], str, str_len);
    newString[len] = 0;
    return newString;
}

Comment 5

[Robert Relyea]

I would r+ wtc's patch. It is actually clearer to read that the existing function, and removes the hackish test to set the first byte of newString to zero if baseStringLen is zero.

Comment 6

[Wan-Teh Chang]

Attachment: Make sure the string is null terminated on return from nssutil_DupnCat(). v2

Bob: thank you for the support! Here is the patch.

Comment 7

[Robert Relyea]

Comment on attachment 8647800 [details] [diff] [review]
Make sure the string is null terminated on return from nssutil_DupnCat(). v2

Review of attachment 8647800 [details] [diff] [review]:
-----------------------------------------------------------------

r+ rrelyea

Comment 8

[Robert Relyea]

actually the patch should be:

newString[len-1] = 0;

newString[len] is beyond the array.

Alternately we can skip adding 1 to the len and add it only in the malloc.

bob

Comment 9

[Wan-Teh Chang]

Attachment: Make sure the string is null terminated on return from nssutil_DupnCat(). v3

Bob: I made the fix you described and checked in the patch. Thanks!

https://hg.mozilla.org/projects/nss/rev/49af81a7869b