The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in 3.12

Status

NSS
Tools
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

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.
Created attachment 306413 [details] [diff] [review]
patch v1

Kai, please review.
Assignee: nobody → nelson
Status: NEW → ASSIGNED
Attachment #306413 - Flags: review?(kengert)

Comment 2

9 years ago
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);
>+    }
(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.
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.
(Assignee)

Updated

9 years ago
Priority: -- → P2
Target Milestone: --- → 3.12

Comment 5

9 years ago
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
Attachment #306413 - Flags: review?(kengert) → review+
Checking in secutil.c; new revision: 1.86; previous revision: 1.85

Thanks, Kai
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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
You need to log in before you can comment on or make changes to this bug.