626414 - Bug 394919 fix can fail if app runs out of memory

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Matt McCutchen]

As described in bug 394919 comment 55, the fix to bug 394919 can fail if the application runs out of memory during the call to CERT_GetCommonName from CERT_GetConstrainedCertificateNames, so that CERT_GetCommonName returns NULL and the name constraints are not checked.

This is based on code inspection.  I have not attempted to reproduce the bug in a real process.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: fix-constrained-cert-names.patch

I don't think we ever ran into this, but it seems worth being safe here and always return NULL from CERT_GetConstrainedCertificateNames if something goes wrong.

Comment 2

[Matt McCutchen]

Comment on attachment 8760587 [details] [diff] [review]
fix-constrained-cert-names.patch

Review of attachment 8760587 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, this patch does not appear to fix the specific case I identified in comment #0, where CERT_GetCommonName runs out of memory and returns NULL and the certificate passes name constraint verification even if it contains a subject common name usable for SSL that violates the name constraint.  We'd somehow need to distinguish "out of memory" from "certificate contains no subject common name".

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8760587 [details] [diff] [review]
fix-constrained-cert-names.patch

Review of attachment 8760587 [details] [diff] [review]:
-----------------------------------------------------------------

I can't tell from context if you have addressed Matt's concerns, but you should try to do that.  The mark and free on the arena seems like a fine improvement, but I'd like to understand why this helps.

::: lib/certdb/certdb.c
@@ +1771,1 @@
>      return rv;

I've recently concluded that "return rv" is a sign that we have a problem.  Why not change the block so that you return SECFailure at the point that the failure occurs (way up there in the context I can't see)?

::: lib/certdb/genname.c
@@ +1142,5 @@
>                  rv = SECITEM_CopyItem(arena, &CN->name.other, &cnItem);
>                  if (rv == SECSuccess) {
>                      DN = cert_CombineNamesLists(DN, CN);
> +                } else {
> +                    PORT_Free(cn);

This is horrible code.  A better pattern is:

CERTGeneralName *CN = NULL;
...
CN = CERT_NewGeneralName(arena, certDNSName);
if (!CN) { goto loser; }
...
rv = SECITEM_CopyItem(arena, &CN->name.other, &cnItem);
if (rv != SECSuccess) { goto loser; }
...

return SECSuccess;
loser:
if (CN) PORT_Free(cn);
PORT_ArenaRelease(arena, mark);
return NULL;