146666 - S/MIME mail with multiple recipients only can be read by first one

Status: VERIFIED DUPLICATE of bug 153243

(MailNews Core :: Security: S/MIME, defect)

Description

[Lapo Luchini]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; it-IT; rv:1.0.0+)
Gecko/20020523
BuildID:    2002052308

When an encrypted S/MIME e-mail is sent to more than one person it is encrypted
only for the first one.
Well I'm not sure, but I'm sure only first one can read it.
I tried with two friends, befeore "A,B" and only A read it, after "B,A" and only
B read it.
A uses Mozilla-laetst-trunk as me, B uses Outlook so it's definitely a "sending"
bug and not receiving/reading/decrypting bug.

Reproducible: Always
Steps to Reproduce:
1.new mail message (^M)
2.To: person1@domain
3.To: person2@domain
4.Security->Always Encrypt
5.send

Actual Results:  person1 can read it, person 2 can't

Expected Results:  both of the receive and read it

We are all using Thawte keys.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

->PSM S/MIME

Comment 2

[Charles Rosendahl]

How do you know that only the first one can read it?  Are you using return
receipts by any chance, or are you physically verifying the inability to view
the message?

Thanks,
Charles

Comment 3

[Charles Rosendahl]

Works for me on all builds using outlook, outlook express, communicator, and
netscape 7.0

Comment 4

[Charles Rosendahl]

.

Comment 5

[Lapo Luchini]

I'm just asking two friends and doing some test sending a message encrypted to
both. As of just before 1.0 RC3 release only first recipient could read it.
I'll do another test with 1.0 RC3.

Comment 6

[Lapo Luchini]

Today I send 4 more "proble" messages, all signed and encrypted, to my two
friends, with each of my two keys.
I used Mozilla latest-1.0.0 build 2002061106.

Friend A uses Mozilla 1.0.0 release (20020530) with "broken key" and, clicking
on it, error "Message has no digital signature [...] Message cannot be decryptes
[..] there are unknown problems with this encrypted message."

Friend B now uses theBat as a reader: he also can read only the message in which
is the first but theBat also give him some more infos about the serial number of
the key which can read the message (info that would be useful to have in
Mozilla, btw)

In the messages with order A, B: s/n 07593F 0777DA
In the messages with order B, A: s/n 06594B 07593F

Comment 7

[Lapo Luchini]

I also made some test with Cc and Bcc: I confirm that "only the first To" can
receive the message (BTW: in previous comment they also where all in To).

Comment 8

[Lapo Luchini]

More test with three recipients (L - myself, O, and A), all in "To:", I report
the s/n that decrypt as seen from theBat (recipient A, notice that in comment it
was B).

LOA: 07593F 07593F 07593F
AOL: 06594B 06594B 06594B 07593F
ALO: 06594B 06594B 06594B 07593F
OLA: 07593F 0777DA 0777DA 0777DA 

It seems that it encrypts always for the first recipient but for the correct
number of time as are the recipients. Moreover if I'm the first ercipient it
doesn't add a "fourth" recipient to let me read the written message, while it
does if I'm not the first.

Comment 9

[Lapo Luchini]

Please notice that 07593F is the correct s/n for my certificate (L as Lapo).

Sorry for the spam but I had always more testing ideas just after sending the bug...

Comment 10

[Lapo Luchini]

Just a wild guess (this is my VERY FIRST reading of Mozilla sources) but in the
MimeCryptoHackCerts function:
http://lxr.mozilla.org/mozilla1.0/source/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp#854
if line 924 uses mailbox_lowercase (which is defined in line 918, out of the
'for') then line 963 shouldn't be followed by a line with the followind data, to
increment ALSO the lower case version of the string?

961       // To understand this loop, especially the "+= strlen +1", look at the
documentation
962       // of ParseHeaderAddresses. Basically, it returns a list of zero
terminated strings.
963       mailbox += strlen(mailbox) + 1;
NEW       mailbox_lowercase += strlen(mailbox_lowercase) + 1;
964     }

It really seems that the orignal-case mailbox is used as address and the
lowercase is used for cert retrieval, but the latter is never incremented =)

Moreover the thing that "my own address" is added onlt when I'm not the first
recipient leads to more investigation...
no, it's the same bug: in line 955 it checks self cert with fetched cert, so if
the cert is fetched wrong, it is of course different from self cert.

That single new line should solve it all, if I'm not wrong.

And ni that case BIG CREDIT to Mozilla source's clearness: it's not common to
see readable enough sources that let you find a bug in a source never seen
before... =)

(unfortunately bug 134113 prevents me from trying to patch the code, as I have
not a license for VisualC)

Comment 11

[Omero]

I've given a look at the code, and the bug fix proposal by Lapo seems to be
right, as far as I can see, and given my ZERO knowledge of mozzi sources.

Two versions of mailboxes are mantained, the original case one and the "forced
lowercase", but only the first one gets incremented, and that seems to be the
reason of mozzi encrypting the mail N times (N = number of total receivers, TO,
CC, BCC included) with always the first TO key...

The other side-bug (adding the unneeded sender key) is because of the same
reason, it checks if the sender is already in the list, if not it adds the
sender key, but this check is done ONLY on the first TO key, and not on the
others, and that's because it adds the key when the sender is NOT the first TO
(where it should not add the sender key if the sender is ANYWHERE in the
TO/CC/BCC list).

I don't visual C as well, so I leave this to others: please have a try ;)

Comment 12

[Charles Rosendahl]

Kaie, is this different than the fix you just placed into the trunk to correct
bug 130692?

Here are the original changes made in 130692:
Index: mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp,v
retrieving revision 1.12.2.1
diff -u -d -r1.12.2.1 nsMsgComposeSecure.cpp
--- mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp	10 Apr 2002 03:05:14 -0000	1.12.2.1
+++ mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp	30 Apr 2002 19:12:10 -0000
@@ -38,6 +38,7 @@
 #include "nsIServiceManager.h"
 #include "nsIMsgIdentity.h"
 #include "nsIMsgCompFields.h"
+#include "nsReadableUtils.h"
 
 // String bundle for smime. Class static.
 nsCOMPtr<nsIStringBundle> nsMsgComposeSecure::mSMIMEBundle = nsnull;
@@ -908,12 +909,14 @@
   /* If the message is to be encrypted, then get the recipient certs */
   if (aEncrypt) {
 	  mailbox = mailbox_list;
+
  nsCString mailbox_lowercase;
+
  ToLowerCase(nsDependentCString(mailbox), mailbox_lowercase);
 
     PRBool already_added_self_cert = PR_FALSE;
 
 	  for (; count > 0; count--) {
       nsCOMPtr<nsIX509Cert> cert;
-
	  certdb->GetCertByEmailAddress(nsnull, mailbox, getter_AddRefs(cert));
+      certdb->GetCertByEmailAddress(nsnull, mailbox_lowercase.get(),
getter_AddRefs(cert));
 
	  if (!cert) {
         // failure to find an encryption cert is
         // fatal for now. We won't be able to encrypt anyway

lapo, can you wait until next week and pull down a build, say on the 25th to
reconfirm?  We have a patch going in at the moment which fixes this problem.

Comment 13

[Kai Engert [:KaiE:]]

Indeed, this looks exactly like what I fixed with bug 153243. It should now work
with the trunk build from this morning. If you could help testing, that would be
great. Thanks!


*** This bug has been marked as a duplicate of 153243 ***

Comment 14

[Charles Rosendahl]

I will leave this as resolved until lapo (real name?) verifies that this has
been fixed in the 06/25 or later builds.

Comment 15

[Omero]

Good to know guys, me and lapo will pull down the 25/06 build and check it out
for sure... thanks :)

Btw, the real name of Lapo is Lapo, he is Lapo Luchini, Lapo is a typical name
of the Florence area in Italy.

Comment 16

[Lapo Luchini]

Verified as solved by other bug's patch, as of latest-1.0 2002062506

Comment 17

[Charles Rosendahl]

.