Closed Bug 1427382 Opened 3 years ago Closed 3 years ago

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

Categories

(MailNews Core :: MIME, defect)

defect
Not set
major

Tracking

(thunderbird_esr5258+ fixed, thunderbird59 fixed)

RESOLVED FIXED
Thunderbird 59.0
Tracking Status
thunderbird_esr52 58+ fixed
thunderbird59 --- fixed

People

(Reporter: jorgk-bmo, Assigned: mkmelin)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-spoof, sec-low, Whiteboard: TB 52.6 ESR)

Attachments

(2 files, 2 obsolete files)

+++ 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.
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 :-(
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: "huhu@huhu.com" <hihi@hihi.com>
  b) From: "Klaus Peter" <huhu@huhu.com>
  c) From: "Don T <president@whitehouse.gov>" <huhu@huhu.com>
only shows
  a) huhu@huhu.com
  b) Klaus Peter
  c) Don T <president@whitehouse.gov>

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?
Flags: needinfo?(mkmelin+mozilla)
Summary: Mailsploit part 1: From addresses with encoded null character are cut off (take 2) → Mailsploit side-issue: TB feature to only show the display name in the thread pane can be used for spoofing
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
Assignee: nobody → mkmelin+mozilla
Flags: needinfo?(mkmelin+mozilla)
I agree. And we have a taker!
Not that @ signs in the display name necessarily are scams. Look at Bugzilla@Mozilla
Attachment #8939671 - Flags: review?(jorgk)
Status: NEW → ASSIGNED
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 ;-)
Attachment #8939671 - Flags: review?(jorgk)
I think this is nicer.
Attachment #8939684 - Flags: review?(mkmelin+mozilla)
Sorry about the noise, move the shorter block up.
Attachment #8939684 - Attachment is obsolete: true
Attachment #8939684 - Flags: review?(mkmelin+mozilla)
Attachment #8939685 - Flags: review?(mkmelin+mozilla)
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?
Attachment #8939685 - Attachment is obsolete: true
Attachment #8939685 - Flags: review?(mkmelin+mozilla)
Attachment #8939686 - Flags: review?(mkmelin+mozilla)
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
Attachment #8939686 - Flags: review?(mkmelin+mozilla) → review+
No, the second search is from name.FindChar('.', atPos)
------------------------------------------------ ^^^^^

Short-circuit processing makes sure this works. Don't you agree?
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
Comment on attachment 8939686 [details] [diff] [review]
bug1427382_mailsploit_threadpane.patch (v2c)

Too late for TB 58 beta 3 which is being built today.
Attachment #8939686 - Flags: approval-comm-esr52?
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/32e98bb5f25c
Follow-up: fix compile error (signed/unsigned). rs=bustage-fix
Attachment #8939686 - Flags: approval-comm-esr52? → approval-comm-esr52+
TB 52.6 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/1ea604524f95c68209534cbcee72cd0bfb1796b5
(2nd bustage-fix changeset already integrated into 1st changeset)
Whiteboard: TB 52.6 ESR
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.
Flags: needinfo?(mkmelin+mozilla)
yes
Blocks: 1433792
Flags: needinfo?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.