633378 - CERT_DupCertificate may not always increment reference count before returning shared object

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

> CERTCertificate *
> CERT_DupCertificate(CERTCertificate *c)
> {
>    if (c) {
>        NSSCertificate *tmp = STAN_GetNSSCertificate(c);
>        nssCertificate_AddRef(tmp);
>    }
>    return c;
> }

STAN_GetNSSCertificate() will return NULL if it failed to create the NSSCertificate object for any reason. nssCertificate_AddRef does nothing if its argument is NULL.

CERT_DupCertificate must check the return value of STAN_GetNSSCertificate, and if it is NULL, it must return NULL. (It looks like all callers of GetNSSCertificate do this except for CERT_DupCertificate).

All callers of CERT_DupCertificate must check the result against NULL. A quick check shows that some callers inside NSS do not make this check (e.g. in crl.c, ssl3con.c, and probably others). All calls to CERT_DupCertificate need to be reviewed and fixed as necessary.

nssCertificate_AddRef should abort if its argument is NULL.

Comment 1

[Wan-Teh Chang]

Thank you for the bug report.

I agree that some NSS code protects against null input unnecessarily,
which could make it harder to track down a crash.

Re: your proposal

Too much code has been written that assumes CERT_DupCertificate
is an AddRef operation, which cannot fail.  So we need to preserve
that, rather than changing the callers to check a NULL return value.

This means two things:

1. Every CERTCertificate passed to CERT_DupCertificate should
already have the nssCertificate member created, so that the
STAN_GetNSSCertificate(c) call cannot possibly fail.

2. In the unlikely event that STAN_GetNSSCertificate(c) returns
NULL, CERT_DupCertificate should crash the program, rather than
either returning NULL or failing to do AddRef silently.