420212 - Empty cert DNs handled badly, display as "!INVALID AVA!"

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

RFC 3280 allows certificate to have empty subject DNs and empty issuer DNs,
provided that subject alternate name exists (and maybe issuer alternate name,
too).  

When NSS encounters a cert with empty Subject and/or Issuer DNs, it prints
"!Invalid AVA!".  That's correct behavior for an individual AVA in a DN, 
and I would argue that it's correct when a non-empty DN contains unparsable
AVAs, but when the entire DN is empty, some other behavior seems more desirable.
Perhaps it should print "(empty)" instead.

The fix should go into function SECU_PrintName in nss/cmd/lib/secutil.c
The decision on whether to print "(empty)" should be made BEFORE the call to CERT_NameToAscii, and should be based on the content of the CERTName, not on 
the output of CERT_NameToAscii.  I'm not exactly sure what the right test is
yet.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Kai, please review.

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 306413 [details] [diff] [review]
patch v1

> {
>     char *nameStr;
>     char *str;
>     SECItem my;
> 
>-    str = nameStr = CERT_NameToAscii(name);
>+    if (!name) {
>+	PORT_SetError(SEC_ERROR_INVALID_ARGS);
>+	return;
>+    }
>+    if (!name->rdns || !name->rdns[0]) {
>+	str = "(empty)";


Shouldn't yet set nameStr, too?


>+    } else {
>+	str = nameStr = CERT_NameToAscii(name);
>+    }

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #2)
> (From update of attachment 306413 [details] [diff] [review])

> Shouldn't yet set nameStr, too?

No, the difference between str and namestr is that namestr is non-NULL
if and only if str was allocated from the heap.  Its only purpose is to 
facilitate freeing that memory after the contents is copied out of it.  
The variable str points to the string to be copied out, and namestr
(if non-NULL) is the memory to be freed after the copy.
Since this new string is not allocated from the heap, its address should
not be in namestr.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Kai, perhaps you didn't see my reply in comment 3 to the questions you asked 
in comment 2.  Hopefully that comment resolves the issue.

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 306413 [details] [diff] [review]
patch v1

(In reply to comment #4)
> Kai, perhaps you didn't see my reply in comment 3 to the questions you asked 
> in comment 2.  Hopefully that comment resolves the issue.

Indeed. It appears I did not get that bugzilla message!

Thanks for explaining that this is not an oversight.

r=kengert

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Checking in secutil.c; new revision: 1.86; previous revision: 1.85

Thanks, Kai

Comment 7

[Tuukka Tolvanen (sp3000)]

In function ‘SECU_PrintName’:
http://mxr.mozilla.org/mozilla/source/security/nss/cmd/lib/secutil.c#2302
warning: ‘nameStr’ may be used uninitialized in this function