156546 - Forwarded inline: Incorrect signature/encryption status shown in header area

Status: VERIFIED FIXED

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

Description

[Kai Engert [:KaiE:]]

There are multiple ways to see the bug.

-
Send a signed message to yourself.
Receive and forward that message inline to yourself, this time using encryption.
The received message will be shown as being both signed and encrypted.

-
Ensure that new signed messages you send to yourself are displayed as having a
valid signature.
Receive an opaquely signed mail from someboy, where the signature is not
verified, and an invalid signature symbol is shown.
Forward that message inline to yourself, and sign that message.
When receiving that message, an invalid signature will be shown, and worse, when
you view the message's security info, you'll not see your own cert, but you'll
see the cert of the signer of the forwarded message.

-
The previous case could be reversed, and that's probably the worst scenario. If
you forward a signed message, that can be verified by the recipient, but the
recipient is not able to verify your outermost signature, the recipient will see
it as a valid signed message.

-
Obtain an encrypted message that you can not decrypt (like an older message
encrypted to you, where you have no longer the private key to decrypt). That
message displays an invalid encryption.
Forward inline that message to yourself, encrypt it.
When receiving that message, you'll an invalid encryption icon, although you
should see a good encryption icon. (Since you were able to decrypt the outermost
message.)

Comment 1

[Kai Engert [:KaiE:]]

The cause of this bug is in the mime parser for crypto mime types.
When we parse mime messages, only the crypto attributes of the outermost message
should result in setting UI attributes.

The current logic does not correctly the correct nesting of mime messages. For
some messages, it incorrectly assumes that inner message parts are still the
toplevel message.

The current logic increases the nesting level count, whenever a mimeMessage
object is seen, making the assumption that such an object were present for any
included sub message. However, that assumption is wrong for messages forwarded
inline.

Comment 2

[Kai Engert [:KaiE:]]

Attachment: Patch v1

This fix changes the code to use the following logic instead:

When walking up the chain of mime objects, in order to count the nesting level,
count every mime type except the crypto mime types.

The crypto mime types "encrypted/signed cms message" and "multipart signed cms
message" are viewed as being transparent to content nesting.


While this patch seems to work already for my test cases, before we start
reviewing, I'd like to check something. I wonder whether I should not check for
the special CMS classes, but rather for the more generic signature and
encrypted mime types.

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Indeed, this patch just uses the superclasses of the cms special classes, and
still works.

Comment 4

[Jean-Francois Ducarroz]

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

R=ducarroz. Maybe you could remove those extern delacation not needed instead
of commenting them out.

Comment 5

[Kai Engert [:KaiE:]]

Attachment: Patch v3

Actually, commenting it out made most sense in my patch v1. In v2, where I no
longer need the cms types, but only the base types, I now think I should avoid
adding the includes. This is a new patch that adds an "extern" declaration to
one of the files, no longer adds includes, and no longer comments out the other
extern declaration.

The patch behaves identically.

Comment 6

[Jean-Francois Ducarroz]

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

R=ducarroz

Comment 7

[Kai Engert [:KaiE:]]

Alec, can you please review?

Comment 8

[Stephane Saux]

cc ddrinan.

Comment 9

[Alec Flett]

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

sr=alecf

Comment 10

[Kai Engert [:KaiE:]]

Checked in to trunk, marking fixed.

Nominating.

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Patch v3 after bustage fix

The previous patch compiled on all platforms but Windows. I was required to
check in a bustage fix.

(Technical details: The failure were unresolved externals. The "extern"
declarations did not work on Windows. Therefore I had to go back to using
#include.)

For documentation purposes, this is the adjusted version of the previous patch,
having the bustage fix, as checked in to the trunk.

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 90790 [details] [diff] [review]
Patch v3 after bustage fix

Carring forward reviews to bustage fixed patch.

Comment 13

[scottputterman]

Did this make today's trunk build?  If so, let's get it verified today.

Comment 14

[Kai Engert [:KaiE:]]

Yes, today's trunk build contains the fix. I checked it in right before the
1.1beta freeze at midnight.

Comment 15

[Stephane Saux]

Charles promised to verify this on the trunk.

Comment 16

[Charles Rosendahl]

I have verified this on the trunk on OSX, win2k, and linux7.3.

However, bug 156715 is really pretty nasty on windows - it prevents me from
using s/mime functionality in a normal use model when composing messages.  I had
to compose all my messages using my linux client and then view them through my
windows client.

Marking verified, as bug 156715 already exists to track the pref problem on
windows trunk.

Comment 17

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch. pls check
this in asap, the add the fixed1.0.1 keyword. thanks!

Comment 18

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 19

[Kai Engert [:KaiE:]]

Checked in to 1.0 branch.

Comment 20

[Charles Rosendahl]

Verified on 20020715 Branch builds, with the exceptions noted in a separate bug
related specifically to issues forwarding messages inline on the Mac only.

Question:  when I mark these bugs verified1.0.1, should I yank out all of the
other keywords, such as approval, nsbeta1+, etc?