Last Comment Bug 420212 - Empty cert DNs handled badly, display as "!INVALID AVA!"
: Empty cert DNs handled badly, display as "!INVALID AVA!"
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.0
: All All
: P2 normal (vote)
: 3.12
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
https://bugzilla.mozilla.org/attachme...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-28 17:06 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2008-03-23 04:59 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (819 bytes, patch)
2008-02-28 17:24 PST, Nelson Bolyard (seldom reads bugmail)
kaie: review+
Details | Diff | Review

Description Nelson Bolyard (seldom reads bugmail) 2008-02-28 17:06:22 PST
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) 2008-02-28 17:24:10 PST
Created attachment 306413 [details] [diff] [review]
patch v1

Kai, please review.
Comment 2 Kai Engert (:kaie) 2008-02-29 12:12:21 PST
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) 2008-02-29 12:22:54 PST
(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) 2008-03-02 23:56:41 PST
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) 2008-03-03 06:25:52 PST
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) 2008-03-03 11:44:06 PST
Checking in secutil.c; new revision: 1.86; previous revision: 1.85

Thanks, Kai
Comment 7 Tuukka Tolvanen (sp3000) 2008-03-23 04:59:29 PDT
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

Note You need to log in before you can comment on or make changes to this bug.