1094492 - pk11_GenerateNewParamWithKeyLen corrupts memory if there is an error

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[John Dennis]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141015093046

Steps to reproduce:

pk11_GenerateNewParamWithKeyLen has a logic error which cause it to call free on uninitalized memory.  At the top of the function a SECItem is allocated:

mech = (SECItem *) PORT_Alloc(sizeof(SECItem));

If an error occurs the final action in the function is to free the mech SECItem

    if (rv !=  SECSuccess) {
	SECITEM_FreeItem(mech,PR_TRUE);
	return NULL;
    }

The PR_TRUE passed to SECItem_FreeItem means it's supposed to also free the contents of the SECItem but the SECItem may never have been written into thus the data pointer inside the SECItem is garbage and the attempt to call free on it results in memory corruption.

The very simple fix is to change PORT_Alloc at the top of the function to PORT_ZAlloc.

Comment 1

[Ryan Sleevi]

Attachment: Port_Alloc -> PORT_ZAlloc

Comment 2

[Ryan Sleevi]

Bob: Ping

Comment 3

[Wan-Teh Chang]

Comment on attachment 8529033 [details] [diff] [review]
Port_Alloc -> PORT_ZAlloc

r=wtc.

Comment 4

[Wan-Teh Chang]

Comment on attachment 8529033 [details] [diff] [review]
Port_Alloc -> PORT_ZAlloc

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

I probably would manually initialize the members, which is done
in the related pk11_ParamFromIVWithLen function in the same file:

diff --git a/lib/pk11wrap/pk11mech.c b/lib/pk11wrap/pk11mech.c
--- a/lib/pk11wrap/pk11mech.c
+++ b/lib/pk11wrap/pk11mech.c
@@ -1379,30 +1379,30 @@ pk11_GenerateNewParamWithKeyLen(CK_MECHA
     SECStatus rv;
 
 
     mech = (SECItem *) PORT_Alloc(sizeof(SECItem));
     if (mech == NULL) return NULL;
 
     rv = SECSuccess;
     mech->type = siBuffer;
+    mech->data = NULL;
+    mech->len = 0;
     switch (type) {
     case CKM_RC4:
     case CKM_SEED_ECB:
     case CKM_CAMELLIA_ECB:
     case CKM_AES_ECB:
     case CKM_DES_ECB:
     case CKM_DES3_ECB:
     case CKM_IDEA_ECB:
     case CKM_CDMF_ECB:
     case CKM_CAST_ECB:
     case CKM_CAST3_ECB:
     case CKM_CAST5_ECB:
-	mech->data = NULL;
-	mech->len = 0;
 	break;
     case CKM_RC2_ECB:
 	rc2_ecb_params = (CK_RC2_PARAMS *)PORT_Alloc(sizeof(CK_RC2_PARAMS));
 	if (rc2_ecb_params == NULL) {
 	    rv = SECFailure;
 	    break;
 	}
 	/* NOTE PK11_GetKeyLength can return -1 if the key isn't and RC2, RC5,
@@ -1440,18 +1440,16 @@ pk11_GenerateNewParamWithKeyLen(CK_MECHA
 	rv = pk11_GenIV(type,&iv);
 	if (rv != SECSuccess) {
 	    break;
 	}
         PORT_Free(mech);
 	return PK11_ParamFromIV(type,&iv);
     default:
 	if (pk11_lookup(type)->iv == 0) {
-	    mech->data = NULL;
-	    mech->len = 0;
 	    break;
 	}
     case CKM_SEED_CBC:
     case CKM_CAMELLIA_CBC:
     case CKM_AES_CBC:
     case CKM_DES_CBC:
     case CKM_DES3_CBC:
     case CKM_IDEA_CBC:

Comment 5

[Wan-Teh Chang]

I noticed a lot of code duplication between pk11_GenerateNewParamWithKeyLen
and pk11_ParamFromIVWithLen. It seems that with negligible differences
pk11_GenerateNewParamWithKeyLen can be implemented in terms of
pk11_ParamFromIVWithLen:

SECItem *
pk11_GenerateNewParamWithKeyLen(CK_MECHANISM_TYPE type, int keyLen)
{
    SECStatus rv;
    SECItem iv;
    SECItem *param;

    rv = pk11_GenIV(type, &iv);
    if (rv != SECSuccess) {
        return NULL;
    }
    param = pk11_ParamFromIVWithLen(type, &iv, keyLen);
    PORT_Free(iv.data);
    return param;
}

I am not suggesting we make this change in this bug. I am including
the code snippet just for future reference.

Comment 6

[Ryan Sleevi]

Attachment: patch2.diff

Updated patch to wtc's suggestion in Comment #4 and carrying the r+ forward

Comment 7

[Ryan Sleevi]

http://hg.mozilla.org/projects/nss/rev/f7b7800ac9e9

Comment 8

[Kai Engert [:KaiE:]]

mass change target milestone to 3.17.4