119418 - S/MIME opaque signed message with attachments displayed incorrectly.

Status: VERIFIED FIXED

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

Description

[Daniel Lanovaz]

I generated an opaquely signed e-mail message with two attachments
from Outlook 2002 version 10.3513.3501 SP-1 and viewed the message
in Mozilla 0.9.7 (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.7)
Gecko/20011221).

Two problems:

1. The "Attachments:" list on the right shows three attachments
where the third, "Part 1.1" is incorrect.

2. The message's security status shows "<Encrypted>" when it
should be "<Signed>"

I'll attach a screen shot and the original RFC 822 message.
Note that I manually had to change the messages Content-type:
from "application/pkcs7-mime" to "application/x-pkcs7-mine".
I've created a separate defect report for that.

Cheers,
Daniel

Comment 1

[Daniel Lanovaz]

Attachment: Screen shot of invalid attachments and security status.

Comment 2

[Daniel Lanovaz]

Attachment: RFC 822 message text that is incorrectly displayed.

Comment 3

[Daniel Lanovaz]

I traced through the code and figured out why there is a third
(Part 1.1) attachment.

The function mimemoz2.cpp>>CountTotalMimeAttachments() is
including the multipart/mixed contents (from the cryptographic
signature construction) as an attachment, rather than skipping
the cryptographic wrapping layer and going inside to the wrapped
multipart/alternative inner content.

Stack trace follows. In this case the attachment count is
actually 4, not 2.

I didn't look further to see if the defect was in the
attachment counting algorithm (which performs memory
allocations of attachment data) or the BuildAttachmentList()
function which seems to have its own algorithm for skipping over
inner content.

One other thing to watch out for. While poking around inside
the MimeContainer objects I noticed that if MimeContainer->children
is 0 that MimeContainer->nchildren can be arbitrary values, as
if it never gets initialized to 0 when the MimeContainer is first
created.

I hope this helps.

CountTotalMimeAttachments(MimeContainer * 0x01fc4030) line 220
MimeGetAttachmentList(MimeObject * 0x01f974d0, const char * 0x01f97248, 
nsMsgAttachmentData * * 0x0012fd48) line 518 + 9 bytes
mime_display_stream_complete(_nsMIMESession * 0x01f975d0) line 900 + 19 bytes
nsStreamConverter::OnStopRequest(nsStreamConverter * const 0x01f96540, 
nsIRequest * 0x00000000, nsISupports * 0x00000000, unsigned int 0x00000000) 
line 1030 + 13 bytes
ReceiveSMIMEMessage() line 252 + 40 bytes

Comment 4

[Mitchell Stoltz (not reading bugmail)]

->PSM S/MIME

Comment 5

[Antonio Lam]

The first problem is already taken care by bug 115169.

Comment 6

[Stephane Saux]

kai

Comment 7

[Kai Engert [:KaiE:]]

I'm confirming the bug.
The mime parser indeed handles this as an encrypted message, although it is not.

Comment 8

[Kai Engert [:KaiE:]]

RFC2311/2633 say, the mime types x-pkcs7-mime and pkcs7-mime are the same thing.
However, we are currently looking only for one type. While looking for both mime
types is an easy fix, it doesn't fix this bug - as you discovered by modifying
the raw message.

Currently our decryption engine seems to assume, it is an encrypted message when
the x-pkcs7-mime type is seen. But RFC2311/2633 clearly define, that it can be
also an encoded signed message, or even a message with certs only.

We need to extend our parser.

Comment 9

[Kai Engert [:KaiE:]]

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

Comment 10

[Stephane Saux]

David, you want to read this bug.

Comment 11

[Mark Shuttleworth]

While looking for the second mime type won't fix the display of attachments, it
will at least make such emails readable. Would there be any problem with getting
at least that part resolved? Right now, reading encrypted mail from some Outlook
users requires a rollback to Communicator 4.79.

Comment 12

[Stephane Saux]

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

Comment 13

[Stephane Saux]

kai, please decide on the nsbeta1+ status of this bug.

Comment 14

[Jean-Marc Desperrier]

I fully second comment #11.

I encounter messages I can't see in Mozilla because the type is pkcs7-mime
instead of x-pkcs7-mime every day.

If the severity of this bug is not high enough to treat it fast, I will open a
second bug for this specific issue.

Apparently, the code below is supposed to treat both cases as equal but it
doesn't work :

/mailnews/mime/cthandlers/smimestub/nsSMIMEStubFactory.cpp
117   { "MIME SMIMEStubed Mail Handler", NS_SMIME_CONTENT_TYPE_HANDLER_CID,
"@mozilla.org/mimecth;1?type=application/x-pkcs7-mime",
118     nsSMimeMimeContentTypeHandlerConstructor, },
119 
120   { "MIME SMIMEStubed Mail Handler", NS_SMIME_CONTENT_TYPE_HANDLER_CID,
"@mozilla.org/mimecth;1?type=application/pkcs7-mime",
121      nsSMimeMimeContentTypeHandlerConstructor, }


Both 
/netwerk/mime/public/nsMimeTypes.h, line 85
#define APPLICATION_XPKCS7_MIME "application/x-pkcs7-mime"
and 
/mailnews/mime/cthandlers/smimestub/nsSMIMEStub.h, line 46 
#define SMIME_CONTENT_TYPE "application/x-pkcs7-mime"

define a constant with the same value.

Comment 15

[Jean-Marc Desperrier]

I did not see the end of the comment of the bug reporter :
"I've created a separate defect report for that."

This was bug 117159.
But it's been marqued as a duplicate of this one bug. 
It had a detailled and very interesting report of where exactly the code is wrong.

Please either solve this problem as part of 119418, or reopen 117159.

Also this bug blocks bug 105774 .

Comment 16

[Kai Engert [:KaiE:]]

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

Comment 17

[Kai Engert [:KaiE:]]

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

Comment 18

[Kai Engert [:KaiE:]]

Stephane, my opinion is, this is definitively nsbeta1+, even if it needs new
parser logic to be written.

Comment 19

[Tom Mraz]

As to the fact that Moz displays status as encrypted on the opaque signed
messages, there is additional issue. If you click on the <encrypted> the mozilla
will crash (M0.9.9 Linux). No talkback window appears (shame).

Comment 20

[Tom Mraz]

Now (Linux nightly 2002032717) it doesn't crash but it still displays encrypted
status (key symbol) and doesn't display signed status (pen symbol).

Even when I receive signed and encrypted message it displays that it is
encrypted but not signed.

I think that it should be addressed as soon as possible.

We would like to help because we would like to use nsICMS and related interfaces
in our mozilla based project but by looking at them we've found that they aren't
completed (missing result checking and so on).

Comment 21

[Tom Mraz]

So there are probably two problems:

1. application/pkcs7-mime isn't treated the same as application/x-pkcs7-mime

2. The opaque message isn't parsed well in either of the two cases. The
x-pkcs7-mime messages are readable, but the signature isn't verified and the
message is incorrectly identified as encrypted.

I'll investigate the codepaths in the second problem later.

Comment 22

[Kai Engert [:KaiE:]]

Attachment: Patch / First attempt

I feared we need more mime parser logic, but now I think this does not seem to
be necessary.

WRT mime, it might be sufficient to extend the portions of mimemcms and mimcms
code, that look at the status of the CMS message.

With this patch, the attachments are no longer displayed as encrypted, but are
correctly indicated as signed.

However, we don't have code that verifies the signature, therefore the
application currently indicates an invalid signature -
nsCMSMessage::VerifySignature is not yet implemented.

In addition we need to export one more function from NSS, see patch.

Comment 23

[Kai Engert [:KaiE:]]

Attachment: Updated patch, now verifies signature

Verifying the signature was easier than I thought. I just cloned function
VerifyDetachedSignature, but remove the call to manually set the digest.

I tested this patch with sample messages, that I got from the reported
problems, see duplicates of this bug.

The verification reasons make sense, it succesfully verified a message where I
trust the signer's root, does not verify where the root is not trusted, and I
see a broken signature, if I manually modify a byte in raw opaque message.

The sender's S/Mime cert arrive in self's encryption database.

The only problem I see, the "message security info" window is unable to display
the sender's cert, because nsCMS::GetSignerCert does not succeed. But I'd say
this is a separate bug.

I suggest we take this patch, because it adds supports for displaying message
sent using the opaque type, and it fixes the issue that messages are wrongly
displayed as encrypted.

Comment 24

[Kai Engert [:KaiE:]]

Nominating [adt1], because it's a security issue and we have a patch.

Comment 25

[David P. Drinan]

Comment on attachment 78267 [details] [diff] [review]
Updated patch, now verifies signature

Patch looks good to me. r=ddrinan.

Comment 26

[Jean-Francois Ducarroz]

it's fine with me too. R=ducarroz

Comment 27

[Darin Fisher]

r=darin for the nsMimeTypes.h change

Comment 28

[Kai Engert [:KaiE:]]

Alec, can you please superreview?

Comment 29

[Wan-Teh Chang]

Attachment: Export NSS_CMSMessage_IsEncrypted from NSS's smime3 shared library

Comment 30

[Kai Engert [:KaiE:]]

Comment on attachment 78267 [details] [diff] [review]
Updated patch, now verifies signature

Please hold off superreviewing, I found a problem with my patch.
Sorry.

Comment 31

[Kai Engert [:KaiE:]]

Attachment: Updated patch, has wtc's patch, has fix for 122721

Sorry that I asked for review before I did excessive testing.
I crashed with my patch trying to dereference a null pointer, if I did not have
the decryption key for an encrypted message.

I carefully rewrote the new logic to avoid that risk.

After updating the patch, and testing all kind of messages and key existance
scenarios, I think the patch is now correct.

Unchanged are the portions to netwerk, so Darin's r= is still valid.

Comment 32

[Kai Engert [:KaiE:]]

David, Jean-Francois, while I now have to ask you for review again, I'm trying
to minimize the overhead.
I was working on bug 122721 in parallel and that bug only requires a small
change, and I have included it with this new patch.

Unchanged are the previous changes to nsCMS.cpp, no need to review again.

But I implemented two more functions in nsCMS.cpp, so please review
  nsCMSMessage::GetSignerEmailAddress
  nsCMSMessage::GetSignerCommonName
They should be correct, because they mostly use logic that is already in use in
GetSignerCert.

Please review the changes to file mimecms.cpp again, it changed completely.

In mimecms.cpp, the code to compare email header and certificate no longer
retrieves the cert's common name, that is not necessary.

In both mimemcms and mimcms, I fixed another bug, the actual and max wanted
nesting levels used the wrong comparison operator...
This did not cause a problem yet, because the comparison included the == case,
and the called function checks the nesting level, too, and does it correctly.

I updated my previous change to NSS with Wan-Teh's patch, and I give r=kaie on
that change to NSS, because it works for me.

Comment 33

[Jean-Francois Ducarroz]

Comment on attachment 78482 [details] [diff] [review]
Updated patch, has wtc's patch, has fix for 122721

Looks fine to me. I have just one request:
can you share the common part of functions nsCMSMessage::GetSignerEmailAddress
& nsCMSMessage::GetSignerCommonName instead of duplicating the code?

Comment 34

[Kai Engert [:KaiE:]]

Attachment: Updated patch, does what ducarroz requested

> Looks fine to me. I have just one request:
> can you share the common part of functions
nsCMSMessage::GetSignerEmailAddress
> & nsCMSMessage::GetSignerCommonName instead of duplicating the code?

Good idea. The same code was even used in additional locations, I made them all
call the same common function.

I also created a shared implementation for the signature verification, as the
new function was mostly identical to the existing one.

Comment 35

[Tom Mraz]

Sorry but to correctly resolve the bug it would also be necessary to fix the NSS
functions which look for the cert in the opaque signed data.
Because even with the latest patch if you don't have the sender's certificate in
the Other people database it shows broken pen and that the message couldn't be
verified. (Unknown problems....1028) 
This would certainly fix the GetSignerCert too.
I think it shouldn't be too hard to fix it...investigating.

Comment 36

[Kai Engert [:KaiE:]]

I agree this patch is not the end of the story.
I already mentioned the problem in comment #23, and it is not yet fixed.

But this patch fixes the most serious issue, in my opinion. It makes sure that
messages, that have only been signed, will not be incorrectly labeled as being
encrypted.

I think this kind of correctness is a number 1 priority, to make sure people
trust the application, and we don't make the people feel safe, where they should
not feel safe.

This patch is the best I currently have, and because of the limited time until
1.0, I'd rather land this patch than leaving the current situation.

Fixing the ability to verify the signature can be done in the next step.

Comment 37

[Kai Engert [:KaiE:]]

We have an additional option, for discussion.

In one of the duplicates of this bug I said, because we currently only display
messages having application/x-pkcs7-mime, we avoid displaying opaque signed
messages, and are thereby avoiding displaying messages as being encrypted,
although they are only signed.

While that seems to be true for messages sent from recent versions of OE (which
uses application/pkcs7-mime), I have learned that my assumption is not 100% true.

In the newsgroups I saw complaints, that Mozilla is already displaying signed
messages as being encrypted.

I draw the conclusion that there must be some S/Mime clients, that do send
opaque signed message using application/x-pkcs7-mime, which cause the wrong display.

As you noted, this patch is not yet perfect, as it has problems verifying some
messages - although the worst that happens, an error is shown that the signature
is invalid.

The option we have: If we wanted to avoid confusion, we could leave support for
application/pkcs7-mime still disabled for now, and only use the portions of the
fix that make the encryption bug go away.

That would mean, opaque signed messages from other recent S/Mime applications
are still not displayed at all with Mozilla, and we avoid displaying incorrect
signature status, until we have implemented that completely.

That's just another option we have, but personally I'd say, ignore this comment
and let's use the patch.

Comment 38

[Kai Engert [:KaiE:]]

I have provided test builds with this patch, for both Linux and Windows,
downloadable at http://www.kuix.de/mozilla/bug119418/ .

Please help testing and give feedback here in the bug, to raise chances that
this fix will become accepted for inclusion with 1.0.

We need to confirm:
- no crashes when using S/Mime
- no regressions with S/Mime messages sent from Mozilla to Mozilla
- messages sent from OE are readable with Mozilla
  (if you can, please test various versions of OE, old and new)
- signed only S/Mime messages from OE are never indicated as being encrypted,
  although they might be displayed as having an invalid signature.
  (please test messages with and without attachments)

Thanks!

Comment 39

[David P. Drinan]

Comment on attachment 78522 [details] [diff] [review]
Updated patch, does what ducarroz requested

r=ddrinan

Comment 40

[Daniel Lanovaz]

I ran Kai's test build through its paces, using the following
test matrix:

+ Incoming messages: clear signed, opaque signed, clear signed and
  encrypted, opaque signed and encrypted, encrypted. All the previous
  with attachments.

+ Outgoing messages: same set as incoming messages.

Problems
========

I found two problems.

1. The signatory is not displayed for opaque signed message, only
clear signed messages. The same is true for encrypted and signed
(clear or opaque). I've attached two screen shots, opaqueSigned.jpg
and clearSigned.jpg that show the differences.

2. To test encryption I imported a PKCS #12 key pair from a
Windows XP box. The import succeeded but the key pair was labeled
"Other People's" rather than one of mine. See the pkcs12Import.jpg
image for a screen shot.

The screen shot shows that the private key is present since I
successfully decrypted an incoming encrypted message.

I could not test outgoing S/MIME messages because I could not
configure a default signing and encryption certificate. See
the security.jpg image. Note the disabled buttons. I'm guessing
this is due to the errant PKCS #12 import?

I hope this helps.

Daniel

Comment 41

[Daniel Lanovaz]

Attachment: Opaque signed message.

Comment 42

[Daniel Lanovaz]

Attachment: Clear signed message.

Comment 43

[Daniel Lanovaz]

Attachment: PKCS #12 import

Comment 44

[Kai Engert [:KaiE:]]

Daniel, thanks for your help!

(I noticed that server with test builds was offline, it should be back online
now. Ping me if you can't access.)

> 1. The signatory is not displayed for opaque signed message, only
> clear signed messages. The same is true for encrypted and signed
> (clear or opaque). I've attached two screen shots, opaqueSigned.jpg
> and clearSigned.jpg that show the differences.

The difference I see is, in one message you are able to view the details of the
signer cert, in the other message, you are not. This is a known issue, not yet
solved, and must be addressed with a separate bug, I'll open it.

But in both cases, you seem to be able to successfully read the message, that's
good.


> 2. To test encryption I imported a PKCS #12 key pair from a
> Windows XP box. The import succeeded but the key pair was labeled
> "Other People's" rather than one of mine. See the pkcs12Import.jpg
> image for a screen shot.
>
> The screen shot shows that the private key is present since I
> successfully decrypted an incoming encrypted message.
> 
> I could not test outgoing S/MIME messages because I could not
> configure a default signing and encryption certificate. See
> the security.jpg image. Note the disabled buttons. I'm guessing
> this is due to the errant PKCS #12 import?

That's interesting. Indeed, it is a bug that your cert appears in the wrong tab.
I didn't tell you, but this test build actually includes another patch, the one
in bug 128586.

I think what you see is an effect caused by that bug/patch, but not a problem
with the patch from this bug. You do not have other p12 files for testing, do you?

If these are the only issues you found, that makes me confident that the patch
works.

If anybody else can help testing, please do.

Comment 45

[Kai Engert [:KaiE:]]

Alec, now it's time to ask again, can you please superreview the patch?


I filed bug 136791, to track the new issue, that extracting certs from opaque
signed messages does not work yet.

Comment 46

[Charles Rosendahl]

I've verified that clear and opaque signed messages, some combined with
encrytpion, are viewable within mozilla.  I've also verified replies and
forwarding of these messages are correct in form in both moz and OE.

- Attachments (and list) show up correctly
- Messages appear as signed when signed amd sign/encrypted (icons are correct)

Other bugs are in separate defect reports.

Comment 47

[Tom Mraz]

I've verified various possibilities (but only with messages from Mozilla and
Outlook 2000) and it works fine with the latest patch except the bug 136791.
I've found and reported another bug 136814, but this one isn't connected to the
patch.

Comment 48

[Kai Engert [:KaiE:]]

Attachment: Updated patch, slightly rearranged code

I found the solution for bug 136791!

The previous patch tried to retrieve the cert from the NSS data structure,
before we called the signature verification code. Simply moving that call after
the verification step, and bug 136791 is fixed.

The reason is that the verification signature code includes a call to import
the certificates from the message. After that has been executed, retrieving the
signer's cert from the NSS data structures succeeds.

Comment 49

[Tom Mraz]

Good work! I've just tried your patch and it works fine in all circumstances
I've tried the previous patch with and the certificate is now viewable.

Comment 50

[Kai Engert [:KaiE:]]

I'm currently uploading an updated test build to
http://www.kuix.de/mozilla/bug119418/ . It should be up in about 20 minutes.

It will contain the latest patch from this bug. It will no longer contain the
patch for bug 128586.

Comment 51

[Jaime Rodriguez, Jr.]

Can you land this on the trunk today, if possible. The sooner the better.

Comment 52

[Alec Flett]

Comment on attachment 78697 [details] [diff] [review]
Updated patch, slightly rearranged code

sr=alecf

Comment 53

[Kai Engert [:KaiE:]]

Checked in to the trunk. Leaving open for branch landing.

Comment 54

[Jaime Rodriguez, Jr.]

If this is fixed on the trunk, then you can mark it as fixed. Once it has landed
on the 1.0 branch, pls add the keyword fixed1.0.0, so we know it made it there, too.

Comment 55

[scottputterman]

Please update this bug when it has been tested.

Comment 56

[Kai Engert [:KaiE:]]

Marking fixed, needs to be verified, in preparation for branch landing.

Comment 57

[Kai Engert [:KaiE:]]

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

Comment 58

[Kai Engert [:KaiE:]]

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

Comment 59

[Jean-Marc Desperrier]

Verified fixed with 2002041503. 
I will include the message I used for the test as an attachment as I could not
see any usable one in the previous attachments.
Encrypted messages from OE are also OK.

But I'm sightly annoyed by the patch.
It implements a function nsCMSMessage::GetSignerEmailAddress that assumes that a
certificate can only have one email adress. 
This is not true, and a certificate can have several adresses that will be
stored in the alternative name extension.

Whilst it's not very much used yet, they are use scenario for certificates where
it's very useful to have a client mail application that suppports this.

Comment 60

[Kai Engert [:KaiE:]]

Thanks for verifying.

> It implements a function nsCMSMessage::GetSignerEmailAddress that assumes that a
> certificate can only have one email adress. 
> This is not true, and a certificate can have several adresses that will be
> stored in the alternative name extension.
> 
> Whilst it's not very much used yet, they are use scenario for certificates where
> it's very useful to have a client mail application that suppports this.

Right, I agree, and I would appreciate that feature too.
But right now, we only support the first email address listed in the cert.
There is already bug 50823 which tracks that feature request.
I think it is a separate task to implement bug 50823, because it might need many
changes across the security codebase - but that's just a guess.

Comment 61

[Charles Rosendahl]

Agree with Kai's last comment - the multiple email address support (Thawte for
instance) is a separate bug that needs to be resolved most likely in a point
release.

Verified fixed in the 0416 trunk build.  Signed/encrypted, opaque/non-opaque
combinatorics used to confirm correct functionality.

Please add 'fixed' to the keyword list after this defect lands on the branch.

Comment 62

[Jaime Rodriguez, Jr.]

only the adt can add adt1.0.0+ on a bug. pls wait for our approval, should come
shortly. thanks!

Comment 63

[Kai Engert [:KaiE:]]

> only the adt can add adt1.0.0+ on a bug. pls wait for our approval, should come
> shortly. thanks!

Argh sorry, my intention was to add the keyword with the plus :(

Comment 64

[Kai Engert [:KaiE:]]

Please ignore the previous comment.
What I really wanted to say was:

  "Actually, my intention was to add the keyword WITHOUT the plus."

To summarize, Jaime, thanks for correcting my mistake!

(comment proofread this time)

Comment 65

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's beahlf) for checkin into the 1.0 branch. Pls check this in
today, and add fixed1.0.0. Once, QA has verified the fix on the branch, pls add
the verified 1.0.0 keyword. thanks again kaie!

Comment 66

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 78697 [details] [diff] [review]
Updated patch, slightly rearranged code

a=[e-mail] for branch checkin.	Marking previously gibven r='s

Comment 67

[Kai Engert [:KaiE:]]

Checked in to the branch.

Comment 68

[Kai Engert [:KaiE:]]

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