Closed Bug 334274 Opened 15 years ago Closed 15 years ago

double free in CRMF_EncryptedKeyGetEncryptedValue

Categories

(NSS :: Libraries, defect, P2)

3.11
All
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.2

People

(Reporter: timeless, Assigned: alvolkov.bgs)

References

()

Details

(4 keywords, Whiteboard: CID 673)

Attachments

(1 file, 3 obsolete files)

found by coverity

crmf_destroy_encrypted_value
crmf_copy_encryptedvalue
CRMF_EncryptedKeyGetEncryptedValue

crmf_destroy_encrypted_value
CRMF_DestroyEncryptedValue
CRMF_EncryptedKeyGetEncryptedValue
Assignee: kengert → nobody
Severity: blocker → critical
Priority: -- → P2
Target Milestone: --- → 3.11.1
Version: 4.0 → 3.11
Hardware: PC → All
Attached patch fix (obsolete) — Splinter Review
crmf_destroy_encrypted_value called from crmf_copy_encryptedvalue should not free dest root struct.
Assignee: nobody → alexei.volkov.bugs
Status: NEW → ASSIGNED
Attachment #219640 - Flags: review?(nelson)
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.
Attachment #219640 - Flags: review?(nelson) → review-
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.
Attachment #219640 - Attachment is obsolete: true
Attachment #219702 - Flags: review?(nelson)
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.
Attachment #219702 - Flags: review?(nelson) → review-
Attached patch wrong patch (obsolete) — Splinter Review
Also fix for a couple potential places for mem leaks.
Attachment #219702 - Attachment is obsolete: true
Attachment #219967 - Flags: review?(nelson)
Attachment #219967 - Attachment description: functions mem dealocation clean up → wrong patch
Attachment #219967 - Attachment filename: 334274c.patch → wrong.patch
Attachment #219967 - Attachment is obsolete: true
Attachment #219967 - Flags: review?(nelson)
Also fix for a couple potential places for mem leaks.
Attachment #219968 - Flags: review?(nelson)
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.
Attachment #219968 - Flags: review?(nelson) → review+
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
Target Milestone: 3.11.1 → 3.11.2
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
CID 673
Whiteboard: CID 673
should this fix get picked up for the 2.0.0.4 firefox release?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4?
Group: security
You need to log in before you can comment on or make changes to this bug.