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

RESOLVED FIXED in Thunderbird 59.0

Status

defect
--
major
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jorgk, Assigned: mkmelin)

Tracking

(Blocks 1 bug, {csectype-spoof, sec-low})

Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr5258+ fixed, thunderbird59 fixed)

Details

(Whiteboard: TB 52.6 ESR)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
+++ 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.
(Reporter)

Comment 1

a year ago
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 :-(
(Reporter)

Comment 2

a year ago
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
(Assignee)

Comment 3

a year ago
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)
(Reporter)

Comment 4

a year ago
I agree. And we have a taker!
(Assignee)

Comment 5

a year ago
Not that @ signs in the display name necessarily are scams. Look at Bugzilla@Mozilla
Attachment #8939671 - Flags: review?(jorgk)
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
(Reporter)

Comment 6

a year ago
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)
(Reporter)

Comment 7

a year ago
I think this is nicer.
Attachment #8939684 - Flags: review?(mkmelin+mozilla)
(Reporter)

Comment 8

a year ago
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)
(Reporter)

Comment 9

a year ago
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)
(Assignee)

Comment 10

a year ago
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+
(Reporter)

Comment 11

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

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

Comment 12

a year ago
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
Last Resolved: a year ago
Resolution: --- → FIXED
(Reporter)

Updated

a year ago
Target Milestone: --- → Thunderbird 59.0
(Reporter)

Comment 13

a year ago
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?

Comment 14

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/32e98bb5f25c
Follow-up: fix compile error (signed/unsigned). rs=bustage-fix
(Reporter)

Updated

a year ago
Attachment #8939686 - Flags: approval-comm-esr52? → approval-comm-esr52+
(Reporter)

Comment 15

a year ago
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
(Reporter)

Comment 16

a year ago
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
(Reporter)

Updated

a year ago
Blocks: 1433792
(Reporter)

Updated

a year ago
Flags: needinfo?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.