340217 - Coverity 543, leak after OOM in CMMF_POPODecKeyChallContDecryptChallenge

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

In CMMF_POPODecKeyChallContDecryptChallenge, if the allocation of 
decryptedRand succeeds, but the subsequent allocation of 
decryptedRand->data fails, the function goes to loser, and returns 
without freeing decryptedRand, hence leaking it.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This function is unbelievably broken.  One cannot tell what it was supposed 
to do, but one can be sure it doesn't do that, whatever that was. 

Please write a patch that replaces the entire body of this function with 
something like this:
{
    /* This unused function had many flaws, and has been deimplemented. */
    PORT_SetError(PR_NOT_IMPLEMENTED_ERROR);
    return SECFailure;
}

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

This URL shows one big problem I see in this function.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/crmf/challcli.c&rev=1.5&mark=141,161,167-168#140

Bob, you and I have worked on this code before.  
It surely didn't seem as obviously broken to me that last time I worked on it
as it does now.  Please review this code (as it is) and tell me if I'm missing
something and it's really OK.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to eliminate code in this bad function

Comment 4

[Alexei Volkov]

Comment on attachment 224835 [details] [diff] [review]
patch to eliminate code in this bad function

r=alexei.volkov

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: try to make it work as intended

I wasn't really happy with the prospect of eliminating this function,
so I attempted to figure out what it was really trying to do, and 
reimplemented much of it to do that.  

In reviewing this, please make sure that it seems to make sense. :)

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2, don't use OCSP error code for CMMF

This patch is the same as the previous one, except that I set different
error codes.

Comment 7

[Alexei Volkov]

Comment on attachment 225166 [details] [diff] [review]
patch v2, don't use OCSP error code for CMMF

r=alexei.volkov

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Coverity bug 340217. Fix leak, crash, and failure to set error codes in
CMMF_POPODecKeyChallContDecryptChallenge. r=alexei.volkov

Checking in challcli.c; new revision: 1.6;      previous revision: 1.5
Checking in challcli.c; new revision: 1.5.28.1; previous revision: 1.5

Resolved / fixed