418546 - reference leak in CERT_PKIXVerifyCert

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

I get a leak on every successful cert verification when using vfychain with PKIX :

[jp96085@monstre]/net/monstre/export/home/julien/nss/tip/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 154 % ./vfychain -p -d . -u 4 ee.der l1ca.der
Chain is good!
Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:120
Abort (core dumped)

Comment 1

[Julien Pierre]

In this specific case, I was testing certs from bug 399152 . I added root I and root II to the DB as trusted . And I was verifying the EE and adding the intermediate on the command line.

Comment 2

[Julien Pierre]

Looks like the leak is specific to CERT_PKIXVerifyCert. When using libpkix through CERT_VerifyCertificate and the environment variable, I don't get the reference leak assertion. So, this is a leak in CERT_PKIXVerifyCert.

[jp96085@monstre]/net/monstre/export/home/julien/nss/tip/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 185 % setenv NSS_ENABLE_PKIX_VERIFY 1
[jp96085@monstre]/net/monstre/export/home/julien/nss/tip/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 186 % ./vfychain -d . -u 4 ee.der
Chain is good!
[jp96085@monstre]/net/monstre/export/home/julien/nss/tip/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 187 %

And there is no leak while going through cert_VerifyCertChainPkix .

So, I'm reassigning this bug to Bob.

Comment 3

[Robert Relyea]

Attachment: Fix memory leaks in pkix glue code.

Comment 4

[Alexei Volkov]

Comment on attachment 307745 [details] [diff] [review]
Fix memory leaks in pkix glue code.

1. >+    if (error && error != PKIX_ALLOC_ERROR()) {

Unnecessary check for PKIX_ALLOC_ERROR. It is safe to call decref without doing it.
 
2. In function cert_PKIXMakeOIDList error now get decrefed twice.

3. >@@ -1444,11 +1461,17 @@ cert_pkixSetParam(PKIX_ProcessingParams 
Please don't do changes in cert_pkixSetParam. They will collide with patch https://bugzilla.mozilla.org/attachment.cgi?id=307775

4.     error = PKIX_ProcessingParams_Create(anchors, &procParams, plContext);
>     if (error != NULL) {              /* need pkix->nss error map */
>         PORT_SetError(SEC_ERROR_CERT_NOT_VALID);
>@@ -1677,13 +1724,21 @@ SECStatus CERT_PKIXVerifyCert(
> 
> 
>     certSelector = cert_GetTargetCertConstraints(cert, plContext);
>+    if (certSelector != NULL) {
>+        PORT_SetError(SEC_ERROR_IO);    /* need pkix->nss error map */
>+        goto cleanup;
>+    }
We have the map. But need to have PKIX_Error object to be able to convert. The best is to set error at cert_GetTargetCertConstraints function level.

Comment 5

[Robert Relyea]

Attachment: Updated patch taking account the review comments

Note: interdiff won't work on this patch. That is because the original file was very far out of date and the original patch conflicted with lots of changes (as alexi pointed out in his review).

the new patch is small enough to be reviewed on it's own merits, so a diff between it and the original is not really useful anyway.

bob

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 308047 [details] [diff] [review]
Updated patch taking account the review comments


>@@ -1703,13 +1732,19 @@ SECStatus CERT_PKIXVerifyCert(
> 
> 
>     certSelector = cert_GetTargetCertConstraints(cert, plContext);
>+    if (certSelector != NULL) {

That test seems exactly backwards.  certSelector is not an error
object, it's the object you want to use in the next call below.
I'm pretty sure you don't want to pass NULL in the next call.

>+        goto cleanup;
>+    }
>     error = PKIX_ProcessingParams_SetTargetCertConstraints
>         (procParams, certSelector, plContext);
>     if (error != NULL) {
>         goto cleanup;
>     }

Comment 7

[Robert Relyea]

Attachment: Use correct failure test

This patch interdiffs with patch 308047 if for ease in a quick review

Comment 8

[Robert Relyea]

Attachment: Patch for checkin...

This patch is the same as 309445 except it was made against the current trunk. (cvs updated)

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 309446 [details] [diff] [review]
Patch for checkin...

All throughout cert_pkixSetParam we see code in error paths that 
sets an error code that may not accurately reflect the underlying
error.  Here are two examples of many:

>             policyOIDList = cert_PKIXMakeOIDList(param->value.array.oids,
>                                 param->value.arraySize,plContext);
>+	    if (policyOIDList == NULL) {
>+		r = SECFailure;
>+		PORT_SetError(SEC_ERROR_INVALID_ARGS);


>                 error = PKIX_PL_Date_Create_UTCTime(NULL, &date, plContext);
>+                if (error != NULL) {
>+                    errCode = SEC_ERROR_INVALID_TIME;

The problem is that the underlying cause may be out of memory, and reporting 
that the input arguments were wrong, when they weren't, disserves the caller.
But it's beyond the scope of this patch to correct that throughout this function.
So, r=nelson

Comment 10

[Robert Relyea]

Checking in certvfypkix.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfypkix.c,v  <--  certvfypkix.c
new revision: 1.16; previous revision: 1.15
done

Comment 11

[Julien Pierre]

I verified the fix.