940678 - "Sender" can spoof signature for "From"

Status: RESOLVED FIXED

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

Description

[Florian Bender]

There's a German article (http://www.heise.de/open/meldung/Thunderbird-gibt-falschem-Absender-das-Echtheits-Siegel-2044405.html) explaining a security flaw with singed emails in TB24 ESR (and earlier), effectively allowing to spoof a signature resp. leading a user to believe that an Email comes from a legitimate sender while it actually does not. I could not find a bug for it, so I filed this one. I also set the security flag though as the issue is publicly known (per the article linked above) this is probably not necessary. 

So the article says that the issue comes from the "Sender" header which signed the Email. TB only displays the data from the "From" header while showing the signature icon, leading the user to believe the message comes from the "From" sender and is signed by him, while in fact the message comes from the "Sender" sender and is signed by the "Sender" (which is not shown). 

Here's how this could be fixed: 

1. a) Make it clear to whom the signature applies: "From" or "Sender".
   b) Don't mark an Email as signed when the signee is the "Sender" not the "From".
2. Always show "Sender" when set, i.e. set mailnews.headers.showSender=true.


Bug 332639 may solve part of this issue.

Comment 1

[Benjamin Smedberg]

If it's in the press, there's no reason for this to be a private bug.

Comment 2

[Florian Bender]

So turns out Bug 332639 has some comments about this, too, starting from Bug 332639 Comment 8. 

Though I'm not sure if this is a DUPE of Bug 332639, the latter is rather one possible solution which should be combined with on of the points above. Thus, I leave it as a dependancy for now.

Comment 3

[Kaspar]

Attachment: Only fall back to Sender header if From header is missing/empty

Maybe it's simply a question of not falling back to the "Sender" header if a "From" header is present. RFC 5750 is relatively vague about what a user agent should do exactly:

                 Receiving agents MUST check that the address in the
   From or Sender header of a mail message matches an Internet mail
   address, if present, in the signer's certificate, if mail addresses
   are present in the certificate.  A receiving agent SHOULD provide
   some explicit alternate processing of the message if this comparison
   fails, which may be to display a message that shows the recipient the
   addresses in the certificate or other certificate details.

If we read that first "or" as an XOR, then something like the attached patch should help.

Comment 4

[Florian Bender]

If the spec is unclear, go for the most foolproof design. 

Your patch (AFAICS) could help the most serious issue, so that's good. Bug 332639 improves the usability, then.

Comment 5

[Magnus Melin [:mkmelin]]

I'd think the spec does mean or, not xor. With your patch the Sender can't usefully sign stuff. Not that I think it's common, but still...

Comment 6

[Kaspar]

(In reply to Magnus Melin from comment #5)
> With your patch the Sender can't usefully sign stuff.

He can, but if his address is different from the From address, Tb will show the question-mark icon ("it is unknown whether sender and signer are the same person" etc.). I think that's a reasonable way of making the user aware of the difference between the From and Sender address. (Otherwise, with the patch from attachment 8336306 [details] [diff] [review], he might not realize this, unless he explicitly compares the addresses from both header fields.)

Comment 7

[Magnus Melin [:mkmelin]]

You're right. Tried the patch and i think it makes sense.
Please set the review flag for the patch to move it forwards.

Comment 8

[Kaspar]

Comment on attachment 8335181 [details] [diff] [review]
Only fall back to Sender header if From header is missing/empty

(In reply to Magnus Melin from comment #7)
> Please set the review flag for the patch to move it forwards.

Here we go. Looking a bit further back, it seems that "mailnews.headers.showSender" wasn't perhaps the best name for a pref with this intended meaning:

  "[...] we want to show the Sender: header when it's not the same as
   the From: header, so that a recipient gets a hint that the e-mail was
   handled by a delegate. This also helps with spoofing, with servers that
   fill in the sender: header. That's controlled by the pref
   mailnews.headers.showSender, defaulted to false (but we might want
   to change that default)" (bug 285474)

Seamonkey still implements this behavior (cf. also bug 308988), but for Tb, bug 482048 tried to "fix Sender header as well" (attachment 367434 [details] [diff] [review])... with some collateral damage, unfortunately.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8335181 [details] [diff] [review]
Only fall back to Sender header if From header is missing/empty

Review of attachment 8335181 [details] [diff] [review]:
-----------------------------------------------------------------

(I'm not a reviewer for the /mailnews code)

Comment 10

[Florian Bender]

ping?

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8335181 [details] [diff] [review]
Only fall back to Sender header if From header is missing/empty

Review of attachment 8335181 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in getting to this. Having tested it locally, I think this is fine and gives the right behaviors. Thanks for the patch.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/0d5a6b079e1a

Thanks for the patch! One request, please make sure that future patches include proper commit information. Makes life much easier for those landing on your behalf. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F