334274 - double free in CRMF_EncryptedKeyGetEncryptedValue

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

found by coverity

crmf_destroy_encrypted_value
crmf_copy_encryptedvalue
CRMF_EncryptedKeyGetEncryptedValue

crmf_destroy_encrypted_value
CRMF_DestroyEncryptedValue
CRMF_EncryptedKeyGetEncryptedValue

Comment 1

[Alexei Volkov]

Attachment: fix

crmf_destroy_encrypted_value called from crmf_copy_encryptedvalue should not free dest root struct.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219640 [details] [diff] [review]
fix

r-
This patch may solve one double free, but I believe it 
creates leaks in some other paths that call these functions.

I think it will be neccessary to review all the calls to 
crmf_destroy_encrypted_value and crmf_copy_encryptedvalue,
and perhaps to change the arugments to one or both functions
and to change several of their callers.  This crmf code is
clearly a mess and needs to be cleaned up.

Comment 3

[Alexei Volkov]

Attachment: make copy function non destructive

Found two leaks in callers of crmf_destroy_encryptedvalue by checking direct and indirect caller-functions code. I believe the code should be changed only in the mentioned function, as it should not be destructive for its arguments.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219702 [details] [diff] [review]
make copy function non destructive

This patch is better, but there's still a problem to be corrected.
See below.


>Index: respcmn.c
>===================================================================

>@@ -322,20 +322,21 @@
> 	break;
>     case cmmfEncryptedCert:
>         dest->cert.encryptedCert = encVal = (poolp == NULL) ?
> 	                             PORT_ZNew(CRMFEncryptedValue) :
> 				     PORT_ArenaZNew(poolp, CRMFEncryptedValue);
> 	if (encVal == NULL) {
> 	    return SECFailure;
> 	}
>         rv = crmf_copy_encryptedvalue(poolp, src->cert.encryptedCert, encVal);
> 	if (rv != SECSuccess) {
>+	    crmf_destroy_encrypted_value(encVal, PR_TRUE);
> 	    return rv;
> 	}

We've destroyed encVal, but we've left dest->cert.encryptedCert still
pointing to it.  The fix is not to copy the value of encVal into 
dest->cert.encryptedCert unless and until crmf_copy_encryptedvalue succeeds. 

>@@ -355,20 +356,21 @@
> 
> 	encVal = dest->privateKey = (poolp == NULL) ?
> 	                             PORT_ZNew(CRMFEncryptedValue) :
>                                      PORT_ArenaZNew(poolp, CRMFEncryptedValue);
> 	if (encVal == NULL) {
> 	    return SECFailure;
> 	}
>         rv = crmf_copy_encryptedvalue(poolp, src->privateKey, 
> 				      dest->privateKey);
> 	if (rv != SECSuccess) {
>+	    crmf_destroy_encrypted_value(encVal, PR_TRUE);
> 	    return rv;
> 	}

Here again, we've destroyed encVal and left dest->privateKey 
pointing to it.  The fix is not to set dest->privateKey to the 
value of encVal until after crmf_copy_encryptedvalue succeeds.

These crmf source files are FULL of this same mistake.  Please plan to
fix all the places where this mistake is made, in at least the crmf files
touched by this patch.

Comment 5

[Alexei Volkov]

Attachment: wrong patch

Also fix for a couple potential places for mem leaks.

Comment 6

[Alexei Volkov]

Attachment: functions mem dealocation clean up

Also fix for a couple potential places for mem leaks.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219968 [details] [diff] [review]
functions mem dealocation clean up

Yes, this looks good to me.  r=nelson
We should test it with the crmftest program in nss/cmd and also have Kai Engert test it in the browsers.

Comment 8

[Alexei Volkov]

tip:
/cvsroot/mozilla/security/nss/lib/crmf/crmfcont.c,v  <--  crmfcont.c
new revision: 1.8; previous revision: 1.7
/cvsroot/mozilla/security/nss/lib/crmf/respcmn.c,v  <--  respcmn.c
new revision: 1.11; previous revision: 1.10

3.11:
/cvsroot/mozilla/security/nss/lib/crmf/crmfcont.c,v  <--  crmfcont.c
new revision: 1.7.28.1; previous revision: 1.7
/cvsroot/mozilla/security/nss/lib/crmf/respcmn.c,v  <--  respcmn.c
new revision: 1.10.28.1; previous revision: 1.10

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

CID 673

Comment 10

[chris hofmann]

should this fix get picked up for the 2.0.0.4 firefox release?