50823 - Support certificates with multiple listed email addresses

Status: VERIFIED FIXED

(MailNews Core :: Security: S/MIME, enhancement, P3)

Description

[bill]

Got an email cert from Thawte Freemail.  it was bound to "[e-mail]".

Then, went back to Thawte and got another cert, but bound this one to
"[e-mail]".

Thawte's freemail system lets you have multiple email addresses in one cert,
which it did for this certificate.  The problem was that when I tried to set my
"from" address to be "[e-mail]", my recipients got "invalid
signature" because the "from" and signer didn't match.

I suspect that PSM is only parsing the certificate and looking at the first
email address in the cert, rather than also checking the other email addresses.

I found this error in Communicator 4.74 on Solaris (no PSM), Mac Netscape 6
(build 00082504)+ PSM and Communicator 4.74/Solaris + PSM.

Comment 1

[John Unruh]

Changing QA contact to nitinp

Comment 2

[John Unruh]

Mass reassigning nitinp's bugs to me.

Comment 3

[John Unruh]

Moving this to bugsplat 527088 since Netscape 6 doesn't support S/MIME.
https://scopus.mcom.com/bugsplat/show_bug.cgi?id=527088

Comment 4

[John Unruh]

Reopening per javi's request.

Comment 5

[John Unruh]

Mass assigning QA to ckritzer.

Comment 6

[John Unruh]

Changing OS to all, QA > junruh.

Comment 7

[Stephane Saux]

This bug is relevant today.

Comment 8

[John Unruh]

Qa > alam

Comment 9

[Kai Engert [:KaiE:]]

This bug is a request for enhancement.
Indeed, Communicator and Mozilla only look at the first mentioned email address
in a certificate.
I agree it would be nice to allow one cert to be used for all addresses.

Comment 10

[Kai Engert [:KaiE:]]

Changing subject.

Comment 11

[Stephane Saux]

S/MIME bugs are automatically nsbeta1 candidates. (this is a bulk update - there
may be some adjustment of the list).

Comment 12

[Jaime Rodriguez, Jr.]

removing nsbeta1, as this bug has been plussed.

Comment 13

[Charles Rosendahl]

*** Bug 138380 has been marked as a duplicate of this bug. ***

Comment 14

[Doug Balmer]

I experience similar probs when my certificate has multiple CN (common name)
fields in it.

Comment 15

[Charles Rosendahl]

*** Bug 151533 has been marked as a duplicate of this bug. ***

Comment 16

[Charles Rosendahl]

*** Bug 176451 has been marked as a duplicate of this bug. ***

Comment 17

[Charles Rosendahl]

*** Bug 179499 has been marked as a duplicate of this bug. ***

Comment 18

[Kai Engert [:KaiE:]]

cc'ing ddrinan FYI

Comment 19

[Kai Engert [:KaiE:]]

Attachment: Patch v1

This patch makes it work.

It contains changes to the signature verification code, and will be happy if
sender/from address matches against any address contained in the signature
certificate.

No changes are required to make sending encrypted messages work.
This works already, since Mozilla recently started to use a new version of NSS,
that supports multiple email addresses in certificates.
If the recipient certificate is stored in the database, the S/Mime code will do
a lookup with the email address as the query parameter, and NSS will find the
cert, even if the address is a secondy address in a certificate.

Comment 20

[Kai Engert [:KaiE:]]

Comment on attachment 113136 [details] [diff] [review]
Patch v1

Javi, can you please review?

Comment 21

[Javier Delgadillo]

Comment on attachment 113136 [details] [diff] [review]
Patch v1

is the nsIX509Cert interface not frozen for embedding purposes?

If not, r=javi

nit:
+// XXX kaie: this is bad. We return a non localizable hardcoded string.

Why not replace "(no nickname)" with something from a properties string.

Comment 22

[Kai Engert [:KaiE:]]

> is the nsIX509Cert interface not frozen for embedding purposes?

Not yet, see bug 168452.


> Why not replace "(no nickname)" with something from a properties string.

I'll file a separate bug requesting to do that.

Comment 23

[Alec Flett]

you might be interested in bug 173601 then - a way to get a list of strings..

until that is implemented, I think a more appropriate mechanism would be to use
IDL arrays:

void getEmailAddresses(out unsigned long length, [retval, array,
size_is(length)] out string addresses);

which should give you the signature:

NS_IMETHOD GetEmailAddresses(PRUint32 *aLength, char*** aAddresses);

Comment 24

[Kai Engert [:KaiE:]]

> void getEmailAddresses(out unsigned long length, [retval, array,
> size_is(length)] out string addresses);

will that work with wstring, too?
Note that we are dealing with Unicode strings here.

Comment 25

[Alec Flett]

yep! The only trick is it doesn't work with AString/ACString... but that
shouldn't be a huge deal...

Comment 26

[Kai Engert [:KaiE:]]

Attachment: Patch v3

Updated patch, use "out array wstring" as Alec suggested.

Comment 27

[Kai Engert [:KaiE:]]

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

Carrying forward r=javi.

Alec, can you please review?

Comment 28

[Alec Flett]

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

what is the urgency here? Could we wait for bug 173601, to avoid double-looping
through the list?
(kai and I talked about this over IRC, but didn't finish the discussion)

Another option is to just add a new method in YOUR library,
CERT_GetEmailAddressCount() which tells you how many e-mail addresses there
are. Is there anyting to stop you from adding that new API?

Comment 29

[Kai Engert [:KaiE:]]

> what is the urgency here? Could we wait for bug 173601, to avoid
> double-looping through the list?

This is a very helpful feature, people are waiting for it, the earlier we get it
in, the more testing we get.


> Another option is to just add a new method in YOUR library,
> CERT_GetEmailAddressCount() which tells you how many e-mail addresses there
< are. Is there anyting to stop you from adding that new API?

As far as I understand, the implementation of the GetEmailCount in the NSS
library would require the same kind of looping.

Comment 30

[Alec Flett]

but even so why burden the consumers? It seems like they will be able to loop
much faster internally than you will by calling GetNextEmail over and over. 

Personally I think that if this can't wait for nsIStringArray (which is slated
for 1.4alpha, we have a few other people demanding it as well for the editor API
freeze) and thus you are forced to loop twice, at least make one of the loops
happen inside NSS by adding that new API. 

If someone is going to stop you from adding to the API that's a whole other
issue, but I'm certainly willing to be a reviewer there, and you can do it as a
part of this bug.

Comment 31

[Kai Engert [:KaiE:]]

> If someone is going to stop you from adding to the API that's a whole other
> issue, but I'm certainly willing to be a reviewer there, and you can do it as a
> part of this bug.

NSS is an external library. NSS has a completely different release schedule than
Mozilla. We are currently using a branch of NSS that is the maintained snapshot
for us. It would be a separate process and a separate bug to implement an
additional API in the external library, and we would need to delay this feature
until we make the transition to move Mozilla to the next larger version of NSS.

Comment 32

[Alec Flett]

ok, I've got a mostly working version of an nsIStringEnumerator in bug 173601..
I hope to land it early in 1.4alpha. the implementation may change, but the
interfaces should stay the same, if you want to try applying it to your tree.

Comment 33

[Alec Flett]

ok, the nsISimpleEnumerator bug has been fixed - wanna give it a try with that?
All you need is an nsCStringArray filled with e-mail addresses.

Comment 34

[Alec Flett]

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

minusing the sr because we have a string enumerator now that is really easy to
use.

Comment 35

[Jean-Marc Desperrier]

Requesting nsenterprise flag.
Some enterprises use an organization with an internal email that is different
from the email seen in the outside world.
Support for this feature would be really interesting for them.

This feature is now supported in Outlook XP.

Comment 36

[Alec Flett]

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

sorry for the delay in getting back to this after our AIM conversation

please file a bug against the non-localizable strings you use here...

sr=alecf

Comment 37

[Kai Engert [:KaiE:]]

Checked in, marking fixed.

Comment 38

[Jean-Marc Desperrier]

Verified/Fixed with 2003040408 (Linux)

Good job, but still some work to do on the subject unfortunately.

This fix does not cover the encryption side of the thing, ie after importing
someone's else cert with two email email1, email2, you will only be able to
encrypt to email1, not to email2. Bug 190738 says the backend part needed for
this in the db is fixed already in nss 3.7.
There's a bug for that already somewhere, I'll include a link.

When cliking to see details of the signature of the mail, the email displayed is
always the first of the list of emails for the cert even if it does not match
the "From:". I filled bug 200710 about this.

Comment 39

[Kai Engert [:KaiE:]]

> This fix does not cover the encryption side of the thing, ie after importing
> someone's else cert with two email email1, email2, you will only be able to
> encrypt to email1, not to email2.

Did you test it?
It does work for me correctly!

If you want me to, I can send you a signed mail from my private address, using a
certificate from Thawte that contains multiple email addresses. I can send send
encrypted mail to any address listed in that cert.

Comment 40

[Cathleen]

libpipnss.so grew by 1.4k.  anyway to optimize this more?

libpipnss.so
  	Total:	      +1456 (+1464/-8)
  	Code:	      +1392 (+1392/+0)
  	Data:	        +64 (+72/-8)
  	      +1392 (+1392/+0)	T (CODE)
  		      +1392 (+1392/+0)	UNDEF:libpipnss.so:T
  			       +748	nsNSSCertificate::ContainsEmailAddress(nsAString const &, int *)
  			       +632	nsNSSCertificate::GetEmailAddresses(unsigned int *, unsigned
short ***)
  			        +12	cmmf_decode_process_certified_key_pair
  	        +64 (+72/-8)	D (DATA)
  		        +64 (+72/-8)	UNDEF:libpipnss.so:D
  			        +32	nsIX509Cert virtual table
  			        +32	nsNSSCertificate virtual table
  			         +8	nsPKCS12Blob virtual table
  			         -8	nsStringComparator virtual table

Comment 41

[Jean-Marc Desperrier]

> Did you test it?
> It does work for me correctly!

I did test it, it was failing, but I just tried again with another install and
this time it worked (an older Mozilla install unfortunately).
I wonder if the failing case was not after importing the multiple email cert,
but before opening a message that has been signed using it. I really thought I
had checked after opening the message too, but ... I see it working now.

I'll try to verify if the failure came from that, and if so, then it's not a
problem in real use.

Comment 42

[Charles Rosendahl]

I have verified that this works using thawte certs on the 20030423 builds.

Jean-Marc, I want to mark this bug as verified, however as you have something
you want to look at, please mark it verified if you are satisifed.

Comment 43

[Josh Birnbaum]

*** Bug 204537 has been marked as a duplicate of this bug. ***

Comment 44

[Charles Rosendahl]

Verified

Comment 45

[Kai Engert [:KaiE:]]

*** Bug 206409 has been marked as a duplicate of this bug. ***

Comment 46

[Kai Engert [:KaiE:]]

*** Bug 30883 has been marked as a duplicate of this bug. ***