392790 - In certificate selection dialogs, also show e-mail addresses from the subjectAltName extension

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Kaspar]

Attachment: Proposed patch

Currently, PSM does not display e-mail addresses from the subjectAltName extension when it prompts the user to select a cert (e.g. when choosing one for Web authentication, when selecting a signing/encryption cert for S/MIME, or with crypto.signText()).

The attached patch addresses this by using nsNSSCertificate::GetEmailAddresses() to retrieve the e-mail addresses from the subjectAltName extension.

While it's true that most certificates continue to use PKCS#9 emailAddress attributes as RDNs in the subject name, this has actually been deprecated long ago already (see e.g. section 3 in RFC 2632, "S/MIME v3 Certificate Handling", June 1999). This patch makes sure that for a certificate adhering to these recommendations, the user is still able to see the e-mail address(es).

Comment 1

[Kaspar]

Attachment: Screenshots illustrating the differences when applying the proposed patch

See also bug 278689 for sample certificates which allow to reproduce the behavior (current and with patch applied).

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 277287 [details] [diff] [review]
Proposed patch

looks mostly good, however:

- please free the data returned by GetEmailAddresses(). You must loop through the list and destroy each item (it was allocated using ToNewUnicode) and finally destroy the array of pointers (which got allocated using nsMemory::Alloc>

- in this code:
+      while (num > 0) {
+        details.AppendLiteral(",");
+        details.Append(*emailAddr++);

I think it would be good to have a space after the comma, in other words, please use ", "

- In expression:
    *emailAddr++;
  I know it's not required, but I'd prefer to use () to indicate what action (* or ++) is supposed to happen first.

- You say an address could potentially be contained twice in the list. Why is that limited to the first two entries in the list? Hmm, I guess because the subject DN always contains a single address only. But what happens if the extension contains multiple addresses, could the DN name address appear at any position in the extension?

Comment 3

[Kaspar]

Attachment: Patch v2

(In reply to comment #2)

Thanks for the review, Kai.

> - please free the data returned by GetEmailAddresses(). You must loop through
> the list and destroy each item (it was allocated using ToNewUnicode) and
> finally destroy the array of pointers (which got allocated using
> nsMemory::Alloc>

True, memory management is something I should care about in C/C++ land... hopefully I got it right this time (instead of adding a loop at the end, I'm just freeing things on the spot in the while loop - or is that bad style?).

> - in this code:
> +      while (num > 0) {
> +        details.AppendLiteral(",");
> +        details.Append(*emailAddr++);
> 
> I think it would be good to have a space after the comma, in other words,
> please use ", "

I left it out because the DNs don't have an space after the comma either, but am fine with changing this. Added with v2.

> - In expression:
>     *emailAddr++;
>   I know it's not required, but I'd prefer to use () to indicate what action
> (* or ++) is supposed to happen first.

In this statement, * was actually pointless, so I dropped it. In the other cases, I'm now doing this in separate steps (since I'm freeing it in between), so there's no more "*emailAddr++".

> - You say an address could potentially be contained twice in the list. Why is
> that limited to the first two entries in the list?

Mostly due to an NSS peculiarity (see below).

> Hmm, I guess because the subject DN always contains a single address only.

That's what you'd expect, yes, but there are some CAs that do it differently (look at one of your Thawte Freemail certs ;-).

> But what happens if the
> extension contains multiple addresses, could the DN name address appear at any
> position in the extension?

nsNSSCertificate::GetEmailAddresses() will always only return the first PKCS#9 emailAddress from the subject DN (the first in the ASN.1 SEQUENCE, to be precise). That's due to the way NSS's cert_GetCertificateEmailAddresses pulls it from the subject:

1129     rawEmailAddr = CERT_GetNameElement(tmpArena, &cert->subject,
1130                                        SEC_OID_PKCS9_EMAIL_ADDRESS);

(cf. http://lxr.mozilla.org/mozilla/ident?i=cert_GetCertificateEmailAddresses)

Now we could file this as a separate NSS "bug"/RFE and try to have it changed, but it's really an exceptional case, I would say (remember that PKCS# emailAddress is a legacy thing). The only situation where my "suppress duplicates" code doesn't work is for a certificate where

a) more than one rfc822Name entry is present in the subjectAltName extension
AND
b) at least one PKCS#9 emailAddress is included in the subject DN
AND
c) the first e-mail address in the subject DN and the first one in the subjectAltName are different (i.e., the ordering in the subject and in the subjectAltName extension is not the same e.g.)

Really a corner case, IMO, and it should not prevent this patch from being landed. (If you like, we can also drop the block doing the "firstEmail" thingy, the only consequence is that an address might appear twice on the "Email:" line).

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 283811 [details] [diff] [review]
Patch v2

(In reply to comment #3)
> > - please free the data returned by GetEmailAddresses(). You must loop through
> > the list and destroy each item (it was allocated using ToNewUnicode) and
> > finally destroy the array of pointers (which got allocated using
> > nsMemory::Alloc>
> 
> True, memory management is something I should care about in C/C++ land...
> hopefully I got it right this time (instead of adding a loop at the end, I'm
> just freeing things on the spot in the while loop - or is that bad style?).

If you guarantee the pointers will only every get accessed once, and you won't loop through the list again, I think it's ok to delete them during the loop.
For added safety, you could assign the "pointer = null" each time you free something, so we don't store dangling pointers.

Regarding the "free", I see you're using PORT_Free. I should have been clearer in my first comment, sorry. We must use a call that is equivalent to the allocation call, which is nsMemory::Free().


r=kengert if you make that change.


Regarding the duplicate address filtering, it seems you are listing several scenarios where the filtering will not succeed. In general I'd prefer that we do it right, but if your argument is, you're filtering out the most common case, then I'm ok.

Would it make sense to compare all other entries with the first entry? That is, remove the special code in front of the loop, and inside the loop, only append if it's not equal?

Comment 5

[Kaspar]

Attachment: Patch v3

(In reply to comment #4)
> If you guarantee the pointers will only every get accessed once, and you won't
> loop through the list again, I think it's ok to delete them during the loop.
> For added safety, you could assign the "pointer = null" each time you free
> something, so we don't store dangling pointers.

I guess you meant "= nsnull"... I've added that, except in the case of firstEmail, since that goes out of scope right after the nsMemory::Free() line.

> Regarding the duplicate address filtering, it seems you are listing several
> scenarios where the filtering will not succeed. In general I'd prefer that we
> do it right, but if your argument is, you're filtering out the most common
> case, then I'm ok.

What I was trying to say is that *all* of the above conditions (a/b/c) must be true at the same time (that's what the AND was supposed to indicate), so the code would only fail for corner cases. But -

> Would it make sense to compare all other entries with the first entry? That
> is, remove the special code in front of the loop, and inside the loop, only
> append if it's not equal?

That's a good idea in any case - thanks for the suggestion. I've rearranged the code and verified that it now also does the "right thing" for a certificate where the subject has e.g. "E=[e-mail],E=[e-mail],E=[e-mail]" and the subjectAltName has the three entries "rfc822Name=[e-mail], rfc822Name=[e-mail], rfc822Name=[e-mail]" (i.e., different ordering of the addresses in the subject and in the subjectAltName extension).

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 284277 [details] [diff] [review]
Patch v3

Kaspar, thanks a lot for your changes. r=kengert

Comment 7

[Kaspar]

Comment on attachment 284277 [details] [diff] [review]
Patch v3

Requesting sr. (Bob: yes, here's another case where we have to cope with some NSS peculiarity [the other recent one is in ExportAsCMS] - just in case this looks familiar... :-)

Comment 8

[Robert Relyea]

Comment on attachment 284277 [details] [diff] [review]
Patch v3

r+. (note: the Free(emalAddr) ouside the if shouldn't be necessary [it could be brought into the if], but is safe since you initialize emailAddr to zero.

Also, it won't hurt to write a bug against NSS's double email address issue. We can hash out whether or not it's reasonable (is here any reason the list should contain an email address twice just because it may be in the certificate twice)?

bob

Comment 9

[Kaspar]

(In reply to comment #8)

Thanks for the review, Bob.

> r+. (note: the Free(emalAddr) ouside the if shouldn't be necessary

Ok - I just wanted to make sure that emailAddr would be free'd
even if num isn't > 0.

> Also, it won't hurt to write a bug against NSS's double email address issue.
> We can hash out whether or not it's reasonable (is here any reason the list
> should contain an email address twice just because it may be in the
> certificate twice)?

Maybe something like GetUniqueEmailAddresses() would make sense (but still in PSM)? Currently, GetEmailAddresses() relies on CERT_GetFirstEmailAddress() and CERT_GetNextEmailAddress(), and the issue that would have to be addressed by NSS is the fact that it only picks the first PKCS#9 emailAddress RDN from the subject.

Comment 10

[Kaspar]

Comment on attachment 284277 [details] [diff] [review]
Patch v3

Requesting approval1.9

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Several comments and a request for Kaspar.

As you know, a DN is a sequence of RDNs, ordered (in DER) from most general 
first (e.g. country) to most specific last (e.g. common name, or given name).
When displayed, the order is commonly reversed, shown from most specific 
first to most general last.  When you write "it only picks the first",
I don't know if you mean (as DER encoded - most general) or as displayed
(most specific).  (I guess I could look).

The use of Common Names and PKCS9 email addresses in DNs for host names
(SSL) or email addresses (S/MIME) is a legacy feature.  For https, 
RFC 2818 is quite specific that only the most specific CN is to be used 
as the host name (and then only when no subject alt name exists).  

I'm not sure what (if any) RFC governs the selection of email address(es)
from subject common names, but I'd think that we either should use only the 
most specific, or else use them all, but certainly we should not use only 
the most general (least specific) PKCS9 address.  Is that what we're doing?

As far as NSS's routines that make these addresses available, I think NSS
should make available to the caller ALL email addresses that the CA is 
certifying belong to the subject of this particular cert, even if one or
more of those addresses appears multiple times.  If some application (such 
as PSM) wishes to eliminate duplicates, it may do so.

Comment 12

[Kaspar]

(In reply to comment #11)
> I'm not sure what (if any) RFC governs the selection of email address(es)
> from subject common names, but I'd think that we either should use only the 
> most specific, or else use them all, but certainly we should not use only 
> the most general (least specific) PKCS9 address.  Is that what we're doing?

Yes, currently NSS is doing that (in alg1485.c):

               rawEmailAddr = CERT_GetNameElement(tmpArena,
                                          &current->name.directoryName, 
                                          SEC_OID_PKCS9_EMAIL_ADDRESS);

CERT_GetNameElement takes the most general one, while CERT_GetLastNameElement would take the most specific one.

The behavior with multiple PKCS#9 email addresses in the subject is likely quite underspecified RFC-wise, I guess (similar to multiple CNs for server names - or is the situation better there?).

> As far as NSS's routines that make these addresses available, I think NSS
> should make available to the caller ALL email addresses that the CA is 
> certifying belong to the subject of this particular cert, even if one or
> more of those addresses appears multiple times.  If some application (such 
> as PSM) wishes to eliminate duplicates, it may do so.

That's effectively what I was suggesting in comment #9, so I'm fine with this. Given the fact that those CAs allowing you to put in more than one address into a certificate will most likely also put all of them into the subjectAltName extension, a fix in NSS should only be a (very-)low-priority thing, IMO.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Kaspar,
I'll file a bug against the many lines of code like the one cited in comment 12.

RFC 2818 says exactly what to do with multiple CNs for server names. 
Paraphrasing here, it says that when SAN exists and has DNS names, 
those names MUST be used exclusively for host name matching, but 
otherwise the most specific subject common name MAY be used.  

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

I filed Bug 399690: 
NSS uses least specific email address from subject DN, not most specific

Comment 15

[Reed Loden [:reed]]

Checking in security/manager/locales/en-US/chrome/pipnss/pipnss.properties;
/cvsroot/mozilla/security/manager/locales/en-US/chrome/pipnss/pipnss.properties,v  <--  pipnss.properties
new revision: 1.27; previous revision: 1.26
done
Checking in security/manager/ssl/src/nsNSSCertificate.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCertificate.cpp,v  <--  nsNSSCertificate.cpp
new revision: 1.130; previous revision: 1.129
done