334448 - oom Crash in crmf_copy_cert_req_msg

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

found by coverity

Comment 1

[timeless]

Attachment: free arena if object isn't constructed

Comment 2

[Daniel Veditz [:dveditz]]

Comment on attachment 218791 [details] [diff] [review]
free arena if object isn't constructed

>     newReqMsg = PORT_ArenaZNew(poolp, CRMFCertReqMsg);
>     newReqMsg->poolp = poolp;
>     if (newReqMsg == NULL) {
>         goto loser;

As long as you're here, check for null /before/ dereferencing newReqMsg->poolp.

>     newReqMsg->pop = crmf_copy_pop(poolp, srcReqMsg->pop);
>     if (newReqMsg->pop == NULL) {
>         goto loser;

This is fine here, but looking in crmf_copy_pop it looks like it leaks newPOP if it hits any of the three "goto loser" cases.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218791 [details] [diff] [review]
free arena if object isn't constructed


>     newReqMsg = PORT_ArenaZNew(poolp, CRMFCertReqMsg);
>     newReqMsg->poolp = poolp;
>     if (newReqMsg == NULL) {

Dan's right.  That crash has to be fixed.  This patch didn't address that.

>  loser:
>     if (newReqMsg != NULL) {
>-        CRMF_DestroyCertReqMsg(newReqMsg);
>+        if (newReqMsg->certReq) {
>+            CRMF_DestroyCertReqMsg(newReqMsg);
>+        } else {
>+            PORT_FreeArena(poolp, PR_TRUE);
>+        }

The right way to destroy a CertReqMsg is to call CRMF_DestroyCertReqMsg, 
even if the CertReqMsg is incomplete.  If CRMF_DestroyCertReqMsg crashes
when the CertReqMsg is incomplete, then the patch should fix THAT, rather
than incorporate some of the logic that belongs in CRMF_DestroyCertReqMsg
into every one of its callers.

Comment 4

[Alexei Volkov]

> >     newReqMsg->pop = crmf_copy_pop(poolp, srcReqMsg->pop);
> >     if (newReqMsg->pop == NULL) {
> >         goto loser;
> 
> This is fine here, but looking in crmf_copy_pop it looks like it leaks newPOP
> if it hits any of the three "goto loser" cases.
> 
Yes, the newPOP object leaks in crmf_copy_pop. 

PORT_ArenaMark, PORT_ArenaUnmark, PORT_ArenaRelease should be used in the function.

Comment 5

[Alexei Volkov]

Attachment: fix for crash and some leaks

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219376 [details] [diff] [review]
fix for crash and some leaks

r-
I see no calls to PORT_ArenaUnmark here.

Every ArenaMark has to be either Released or Unmarked.  
On some platforms, in some builds, a Mark acquires a lock.
That lock is release by calling either Release or Unmark.
Unmark removes the mark, and leaves the arena intact, and 
clears the lock.  Release frees anything allocating since the 
mark was made, removes the mark, and clears the lock.

Comment 7

[Alexei Volkov]

Attachment: Be more conservative with pool.

Modification to the previous patch: PORT_ArenaUnmark is added.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219767 [details] [diff] [review]
Be more conservative with pool.

This patch is fine, as far as it goes, but it doesn't address the
original OOM crash.  Here's the change that's needed for that.

>     newReqMsg = PORT_ArenaZNew(poolp, CRMFCertReqMsg);
>-    newReqMsg->poolp = poolp;
>     if (newReqMsg == NULL) {
>         goto loser;
>+    newReqMsg->poolp = poolp;

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219767 [details] [diff] [review]
Be more conservative with pool.

r- because it doesn't prevent the oom crash.
Otherwise it's OK.

Comment 10

[Alexei Volkov]

I think moving pool assignment below "if" is not enough. When failure occures at 517, we still have crash in CRMF_DestroyCertReqMsg function.

517     newReqMsg->certReq = crmf_copy_cert_request(poolp, srcReqMsg->certReq);
518     if (newReqMsg->certReq == NULL) {
519         goto loser;
520     }


This is why the third patch has changes to CRMF_DestroyCertReqMsg function.

Comment 11

[Alexei Volkov]

Attachment: address the root issue of the bug

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 220067 [details] [diff] [review]
address the root issue of the bug

r=nelson for the 3.11 branch (not 3.11.1 branch) and the trunk.
I think this patch is no less correct than the old code in all
respects, but as I review the patched code, there are still a few
areas where the original code seems, uh, suspicious.  

So, IMO, it's ok to check in these changes (on the branches 
indicated above), but before marking the bug resolved/fixed there
might be a few more changes worth consideration.


>Index: crmfreq.c

> SECStatus 
> CRMF_DestroyCertReqMsg(CRMFCertReqMsg *inCertReqMsg)
> {
>     PORT_Assert(inCertReqMsg != NULL && inCertReqMsg->poolp != NULL);

That assertion should be followed by an if statement that detects 
those conditions and deal with them gracefully in non-debug builds,
e.g. set an error code and return SECFailure.

>-    if (!inCertReqMsg->isDecoded) {
>+    if (!inCertReqMsg->isDecoded && inCertReqMsg->certReq) {
>         if (inCertReqMsg->certReq->certTemplate.extensions != NULL) {
> 	    PORT_Free(inCertReqMsg->certReq->certTemplate.extensions);
> 	}
> 	if (inCertReqMsg->certReq->controls != NULL) {
> 	    PORT_Free(inCertReqMsg->certReq->controls);
> 	}
>     }
>     PORT_FreeArena(inCertReqMsg->poolp, PR_TRUE);
>     return SECSuccess;
> }

The above block is a part that seems suspicious.  Cert request messages
can be either (a) locally generated (to be DER encoded and then sent off 
to a remote server) or (b) received in the server from a remote client and 
then DER decoded.  IOW, the message can be either input to encoder or output
of decoder. The "isDecoded" flag serves to tell us which case we're dealing 
with on this stack.  

The decoded case always implies that the decoded parts are in an arena pool 
(because the decoder always decodes into a pool).  The code here seems to be 
saying: if we're in a pool, and we're decoded, then explicitly free the 
extensions (rather than implicitly freeing them when the pool is freed).  
That's suspicious.  It might be right (in which case it must be that the 
extensions and controls were not allocated from the pool), but there should 
be some comments explaining that, if so.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

CID 212

Comment 14

[chris hofmann]

should this be considered for fixing in firefox 2.0.0.4?