Open
Bug 626414
Opened 13 years ago
Updated 2 months ago
Bug 394919 fix can fail if app runs out of memory
Categories
(NSS :: Libraries, defect, P5)
NSS
Libraries
Tracking
(Not tracked)
NEW
People
(Reporter: matt, Unassigned)
References
Details
(Keywords: sec-moderate)
Attachments
(1 file, 1 obsolete file)
3.07 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Keywords: sec-moderate
Comment 1•8 years ago
|
||
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.
Assignee: nobody → franziskuskiefer
Attachment #8760587 -
Flags: review?(martin.thomson)
Reporter | ||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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;
Attachment #8760587 -
Flags: review?(martin.thomson)
Updated•8 years ago
|
Priority: -- → P5
Updated•6 years ago
|
Assignee: franziskuskiefer → nobody
Updated•2 years ago
|
Severity: normal → S3
Comment hidden (spam) |
Updated•2 months ago
|
Attachment #9385224 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•