Note: There are a few cases of duplicates in user autocompletion which are being worked on.

reference leak in CERT_PKIXVerifyCert

VERIFIED FIXED in 3.12

Status

NSS
Libraries
P1
normal
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Julien Pierre, Assigned: Robert Relyea)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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)
(Reporter)

Updated

10 years ago
Priority: -- → P1
Target Milestone: --- → 3.12
(Reporter)

Comment 1

10 years ago
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.
(Reporter)

Comment 2

10 years ago
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.
Assignee: alexei.volkov.bugs → rrelyea
(Reporter)

Updated

10 years ago
Summary: reference leak in PKIX → reference leak in CERT_PKIXVerifyCert
(Assignee)

Comment 3

10 years ago
Created attachment 307745 [details] [diff] [review]
Fix memory leaks in pkix glue code.
Attachment #307745 - Flags: review?(alexei.volkov.bugs)

Comment 4

10 years ago
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.
Attachment #307745 - Flags: review?(alexei.volkov.bugs) → review-
(Assignee)

Comment 5

10 years ago
Created attachment 308047 [details] [diff] [review]
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
Attachment #307745 - Attachment is obsolete: true
Attachment #308047 - Flags: review?(alexei.volkov.bugs)

Updated

10 years ago
Attachment #308047 - Flags: review?(alexei.volkov.bugs) → review+
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;
>     }
Attachment #308047 - Flags: superreview-
(Assignee)

Comment 7

10 years ago
Created attachment 309445 [details] [diff] [review]
Use correct failure test

This patch interdiffs with patch 308047 if for ease in a quick review
Attachment #308047 - Attachment is obsolete: true
(Assignee)

Comment 8

10 years ago
Created attachment 309446 [details] [diff] [review]
Patch for checkin...

This patch is the same as 309445 except it was made against the current trunk. (cvs updated)
Attachment #309446 - Flags: review?(nelson)
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
Attachment #309446 - Flags: review?(nelson) → review+
(Assignee)

Comment 10

10 years ago
Checking in certvfypkix.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfypkix.c,v  <--  certvfypkix.c
new revision: 1.16; previous revision: 1.15
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 11

10 years ago
I verified the fix.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.