1427382 - Mailsploit side-issue: TB feature to only show the display name in the thread pane can be used for spoofing

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1423432 +++

As was pointed out in bug 1423432 comment #51 (and earlier, but not as clearly stated), the full Mailsploit vulnerability isn't fixed yet.

Go to https://www.mailsploit.com/ and "Try The Demo"

Sending all 14 "payloads", TB will only handle five of them, and in nine of them, the From address is still truncated somewhere.

Comment 1

[Jorg K (CEST = GMT+2)]

Trying the "other" patch for bug 1423432, attachment 8935176 [details] [diff] [review] also only solves 5 of the 14 problems.

I should add that all 14 test messages show the correct From address in the header pane but only 5 show the correct from address in the thread pane/message list where more massaging takes place :-(

Comment 2

[Jorg K (CEST = GMT+2)]

OK, looking at this a little further, the issue is not that something gets truncated at a null-byte. The issue is that TB only shows the display part of the From header in the thread pane, so:
  a) From: "[e-mail]" <[e-mail]>
  b) From: "Klaus Peter" <[e-mail]>
  c) From: "Don T <[e-mail]>" <[e-mail]>
only shows
  a) [e-mail]
  b) Klaus Peter
  c) Don T <[e-mail]>

So that's a TB feature which is being exploited here. The full address is shown in the header pane.

The nine test messages obscuring the real sender address are crafted along those lines.

Magnus, do you have an opinion here?

Comment 3

[Magnus Melin [:mkmelin]]

I think we should check if the name includes @, and if it does, show the full address for unknown people. 
https://dxr.mozilla.org/comm-central/rev/4469c475c6b473927956f509a1ef78da8db5e48c/mailnews/base/src/nsMsgDBView.cpp#425

Comment 4

[Jorg K (CEST = GMT+2)]

I agree. And we have a taker!

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1427382_mailsploit_threadpane.patch

Not that @ signs in the display name necessarily are scams. Look at Bugzilla@Mozilla

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939671 [details] [diff] [review]
bug1427382_mailsploit_threadpane.patch

I think this is too simplistic and destroys the UX. There is nothing wrong with "Bugzilla@Mozilla" or "Men @ Work".

At least we should test for a dot after the @ and if it present, display name + email. Let me attach something more sophisticated ;-)

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: bug1427382_mailsploit_threadpane.patch (v2)

I think this is nicer.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: bug1427382_mailsploit_threadpane.patch (v2b)

Sorry about the noise, move the shorter block up.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: bug1427382_mailsploit_threadpane.patch (v2c)

Sorry, even more noise, I moved another shorter branch up.

Now the thread pane (aka message list) looks the same as the header pane and that's a good thing, no?

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 8939686 [details] [diff] [review]
bug1427382_mailsploit_threadpane.patch (v2c)

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

::: mailnews/base/src/nsMsgDBView.cpp
@@ +429,5 @@
> +      if ((atPos = name.FindChar('@')) == kNotFound ||
> +          name.FindChar('.', atPos) == kNotFound) {
> +        aSenderString = name;
> +      } else {
> +        // Found @ followed by a dot, so this looks like a spoofing case.

nit: that's not what you tested for. The order can be anything

Comment 11

[Jorg K (CEST = GMT+2)]

No, the second search is from name.FindChar('.', atPos)
------------------------------------------------ ^^^^^

Short-circuit processing makes sure this works. Don't you agree?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8473687592d8
don't use header display name straight off, in case it contains @ (analysis by Magnus Melin). r=mkmelin

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939686 [details] [diff] [review]
bug1427382_mailsploit_threadpane.patch (v2c)

Too late for TB 58 beta 3 which is being built today.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/32e98bb5f25c
Follow-up: fix compile error (signed/unsigned). rs=bustage-fix

Comment 15

[Jorg K (CEST = GMT+2)]

TB 52.6 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/1ea604524f95c68209534cbcee72cd0bfb1796b5
(2nd bustage-fix changeset already integrated into 1st changeset)

Comment 16

[Jorg K (CEST = GMT+2)]

Magnus, you you think we should also fix the recipient string
https://dxr.mozilla.org/comm-central/rev/2b56d89a8792fc1af6ae66be5f38cd9d26ecb7e1/mailnews/base/src/nsMsgDBView.cpp#542

so it receives the same treatment as the sender:
https://dxr.mozilla.org/comm-central/rev/2b56d89a8792fc1af6ae66be5f38cd9d26ecb7e1/mailnews/base/src/nsMsgDBView.cpp#421

I noticed the unequal treatment when I did the uplift.

Comment 17

[Ben Bucksch (:BenB)]

yes