586957 - CERT_FormatName leaks things if properties exist multiple times

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[timeless]

I presume this should never happen, but I'd rather we:
* ignore them
* fail them
* warn about them

than have the potential for leaking.

At least in the case of email, it looks fairly easy for this to happen:

105     SECItem * email     = 0;
118     SECItem * orgunit[MAX_OUS];

121     /* Loop over name components and gather the interesting ones */
122     rdns = name->rdns;
123     while ((rdn = *rdns++) != 0) {
124         avas = rdn->avas;
125         while ((ava = *avas++) != 0) {
126             tag = CERT_GetAVATag(ava);
127             switch(tag) {
...
here we ignore cases with too many SEC_OID_AVA_ORGANIZATIONAL_UNIT_NAMEs:
170               case SEC_OID_AVA_ORGANIZATIONAL_UNIT_NAME:
171                 if (ou_count < MAX_OUS) {
... work is done here
177                 }
178                 break;

here, it should be trivial for us to reach both cases:
188               case SEC_OID_PKCS9_EMAIL_ADDRESS:
189               case SEC_OID_RFC1274_MAIL:
the first time through we allocate email.
the second time through we leak the previous allocation of email when we replace it:
190                 email = CERT_DecodeAVAValue(&ava->value);
191                 if (!email) {
192                         goto loser;
193                 }
194                 len += email->len;
195                 break;

Comment 1

[timeless]

Attachment: patch

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Timeless,  
This is yet another bug about the seldom-used function CERT_FormatName.
As I look at this function, I see that it has some big logic flaws, 
based on some flawed assumptions about certificate names.  Even if we 
fix all its leaks, it is still flawed, IMO.  Among the flaws I see are:

- the assumption that a cert name contains no attributes other than these:
  - cn
  - email address
  - organization unit name   (*)
  - domain qualifier
  - organization name
  - domain component         (*)
  - locality  (city)
  - state 
  - country

- the assumption that a cert name contains at most ONE of any of the above
named attributes, except for the ones marked with a '*'.

- The attributes will always be given in the certificate in (the reverse of) the order shown above, and should always be displayed in the order given 
above.

We know these assumptions are false.  For names that do not hold to those 
assumptions, the function will not display the correct name as given in 
the certificate.  It will list at most one of each of the attributes (except for the ones marked with '*') and will not list ANY attributes not shown above.

There is another function in NSS that has the same signature as this function (except for its name, of course), and that seems to be intended to serve a very similar purpose, but which (IMO) does the job correctly.  It correctly lists all the name attributes in the proper order, regardless of whether that order matches any pre-conceived idea of the "proper order" or not.  That function is 
CERT_NameToAscii.  

However, there is one major difference between the two functions.  CERT_NameToAscii outouts a single string with a comma-separated list of "Attribute Value Assertions", that is expressions of the form 
"attribute_name=value".  In contrast to that, CERT_FormatName outputs a string that is intended to be html.  It lists only attribute values, not the attribute names, and it separates them with '<br>' rather than commas.

I wish we could replace the body of CERT_FormatName with a call to CERT_NameToAscii.  If we cannot do that, perhaps we can leverate the correct logic of CERT_NameToAscii to correct the flaws in CERT_FormatName.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 465627 [details] [diff] [review]
patch

This patch accomplishes its objective, which is to plug leaks.  
It does not correct the flaws due to fundamental mistaken design assumptions.
But given that I suspect this function is dead code anyway, I'm willing to 
accept this patch to fix it.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Checking in certhigh/certhtml.c; new revision: 1.10; previous revision: 1.9

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Mass checkin of Timeless's coverity fixes on 3.12 branch:

cmd/lib/secutil.c;           new revision: 1.99.2.1; previous revision: 1.99
cmd/lib/secutil.h;           new revision: 1.32.2.1; previous revision: 1.32
cmd/certutil/certutil.c;     new revision: 1.149.2.1; previous revision: 1.149
lib/certhigh/certhtml.c;     new revision: 1.8.66.1; previous revision: 1.8
lib/certhigh/certreq.c;      new revision: 1.8.56.1; previous revision: 1.8
lib/jar/jar.h;               new revision: 1.6.4.1; previous revision: 1.6
lib/smime/cmssiginfo.c;      new revision: 1.32.2.1; previous revision: 1.32
lib/pk11wrap/debug_module.c; new revision: 1.15.2.1; previous revision: 1.15
lib/crmf/servget.c;          new revision: 1.5.66.1; previous revision: 1.5