287057 - Memory leak in CERT_FindCRLDistributionPoints

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Hanfei Yu]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.4) Gecko/20041214
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.4) Gecko/20041214


In certhigh/certhigh.c:

The field encodedExtenValue.data need to be freed before 
CERT_FindCRLDistributionPoints() is returned to the caller.

Here is the memory leak trace ...

  204       1    0x7bfc0  PR_Malloc < PORT_Alloc < SECITEM_CopyItem <
cert_FindExtensionByOID < cert_FindExtension < CERT_FindCRLDistributionPoints 


Reproducible: Always

Comment 1

[Wan-Teh Chang]

I think this memory leak bug report is correct, and
there are other similar memory leaks.

I already examined all *direct* callers of
cert_FindExtensionByOID and found that among them,
only cert_FindExtension needs to be pursued.  So
we should examine all callers of cert_FindExtension.
They need to call SECITEM_FreeItem(value, PR_FALSE) or
PORT_Free(value->data) to free the 'value' SECItem.

Comment 2

[EPerlade.spam]

here is a working correction:

CERTCrlDistributionPoints *
CERT_FindCRLDistributionPoints (CERTCertificate *cert)
{
    SECItem encodedExtenValue;
    SECStatus rv;
    CERTCrlDistributionPoints *crlDPs;
    encodedExtenValue.data = NULL;
    encodedExtenValue.len = 0;

    rv = cert_FindExtension(cert->extensions, SEC_OID_X509_CRL_DIST_POINTS,
			    &encodedExtenValue);
    if ( rv != SECSuccess ) {
	return (NULL);
    }
    crlDPs = CERT_DecodeCRLDistributionPoints (cert->arena,
					       &encodedExtenValue);
    SECITEM_FreeItem(&encodedExtenValue, PR_FALSE);
    return (crlDPs);
}

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch (checked in)

I've reviewed all (direct and indirect) callers of
cert_FindExtension.

Some of the leaks are only on the error paths.

Some of the changes are to pass NULL as the SECItem*
argument if we only want to know if the extension
exists but don't need its value.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 183944 [details] [diff] [review]
Proposed patch (checked in)

r=nelson

As an aside, part of this patch fixes code that has been dead
for about 5 years now.	Function CERT_HTMLCertInfo in 
lib/certhigh/certhtml.c is not exported and not called.
(The aparent call in jarver.c is ifdeffed "notdef").
I suspect the entire contents of that sourcce file is dead
code.  Please consider expunging any dead code found in it.

Comment 5

[Wan-Teh Chang]

Comment on attachment 183944 [details] [diff] [review]
Proposed patch (checked in)

Patch checked in on the NSS trunk for NSS 3.11
(with a minor change to the patch for certhigh.c).

Checking in certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.73; previous revision: 1.72
done
Checking in certdb/genname.c;
/cvsroot/mozilla/security/nss/lib/certdb/genname.c,v  <--  genname.c
new revision: 1.29; previous revision: 1.28
done
Checking in certhigh/certhigh.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.34; previous revision: 1.33
done
Checking in certhigh/certhtml.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhtml.c,v  <--  certhtml.c
new revision: 1.6; previous revision: 1.5
done
Checking in certhigh/crlv2.c;
/cvsroot/mozilla/security/nss/lib/certhigh/crlv2.c,v  <--  crlv2.c
new revision: 1.4; previous revision: 1.3
done
Checking in certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.21; previous revision: 1.20
done

Comment 6

[Wan-Teh Chang]

Attachment: Remove dead code ocsp_CertHasNoCheckExtension

This patch only removes ocsp_CertHasNoCheckExtension,
which is commented out.

I don't know if CERT_FindCRLDistributionPoints and
CERT_FindInvalidDateExten should be considered dead
code.  They are not exported and they are not used
by other NSS functions.

I'd like to ask Bob Relyea to look into removing
CERT_HTMLCertInfo because it was he who commented
out the CERT_HTMLCertInfo call in lib/jar/jarver.c.
There are many functions in lib/jar that aren't
being used, and I don't know if they should be
considered dead code.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 187869 [details] [diff] [review]
Remove dead code ocsp_CertHasNoCheckExtension

The code being excised here documents the existence of a cert 
extension whose purpose seems to be to avoid recursive OCSP
revocation checking on certs used as part of OCSP response singing.
However, such a check needs to be made inside the cert chain 
verification code, when validating a chain for OCSP responses,
not here in the OCSP code, IMO.  

So, perhaps when this dead code is removed, a comment should be
added to CERT_VerifyCert, or a bug about verifying cert chains
for OCSP responses should be filed, or a comment added to such
a bug if it already exists.  The comment should suggest the use
of this extension for this purpose.  

Julien, please comment on whether  CERT_FindCRLDistributionPoints 
and CERT_FindInvalidDateExten should be considered dead code.

CERT_HTMLCertInfo is a left-over from Netscape Communicator 4.
It was part of Communicator's certificate details display.
When we separated NSS from Communicator, we tried to separate
code that was specific to Commuicator from code that was common
to other apps, and keep only the common code in NSS.  This
function should not have been kept in NSS, IMO.  It anything,
it belongs in PSM today, not NSS.  If libjar was calling it,
that was, IMO, a layering violation.  It was libjar doing work
that belonged in the application.  A low level jar library
method should return a cert handle, and the application should
format the info from that cert, if desired.

Comment 8

[Wan-Teh Chang]

Marked the bug fixed because the memory leaks have been
fixed.  Removing dead code is just nice to have.