Bug 1423432 (CVE-2017-7829)

Mailsploit part 1: From addresses with encoded null character are cut off

RESOLVED FIXED in Thunderbird 59.0

Status

enhancement
--
major
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: BenB, Assigned: jorgk)

Tracking

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

Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr5257+ fixed, thunderbird58 fixed, thunderbird59 fixed)

Details

Attachments

(4 attachments, 5 obsolete attachments)

Reporter

Description

2 years ago
Part of bug 1423430
https://www.mailsploit.com

Reproduction:
1. Send yourself an email with SMTP:
$ netcat ###YOUR.SMTP.SERVER### 25
MAIL FROM:<=?utf-8?b?cG90dXNAd2hpdGVob3VzZS5nb3Y=?==?utf-8?Q?=00?==?utf-8?b?cG90dXNAd2hpdGVob3VzZS5nb3Y=?=@mailsploit.com>
RCPT TO: <###your.email.address@your.own.provider.not.gmail.and.co###>
DATA
From: =?utf-8?b?cG90dXNAd2hpdGVob3VzZS5nb3Y=?==?utf-8?Q?=00?==?utf-8?b?cG90dXNAd2hpdGVob3VzZS5nb3Y=?=@mailsploit.com
To: moron@example.com
Date: Wed, 6 December 2017 22:00:00 +0200
Subject: mailsploit

Yeah, right.

.
QUIT

2. Open the email in Thunderbird

Actual result:
* The From field displays:
"potus@whitehouse.gov
* If you click on the email address, you still see the same email address in the context menu.

Expected result:
* The From field displays:
-potus-whitehouse.gov@mailsploit.com
or even better:
invalid@mailsploit.com

Analysis:
I think the bug here is that we cut the string off at an embedded null \0 character.

That might be in libmime or in JSMime. We need to find the place, and before that happens, we should strip the null character.
Reporter

Updated

2 years ago
Severity: normal → major
Assignee

Comment 1

2 years ago
When you import the message, it is displayed as 
"potus@whitehouse.gov[null]potus@whitehouse.gov"@mailsploit.com

That indicates that JS Mime is displaying it correctly. However, with the next repaint event, this is shortened to "potus@whitehouse.gov.

The task it to find where this repaint happens and why the address is truncated then.
Assignee

Comment 2

2 years ago
Replying to the e-mail in question gives this in the composition:
  On 02/12/2017 19:16, "potus@whitehouse.govpotus@whitehouse.gov"@mailsploit.com wrote:

However, the text can't be copied in full since it seems to contain an invisible x\00. BTW, the reply address is
  potus@whitehouse.gov <>
and sending with this will fail, so there is a reduced chance that anyone will disclose information to the attacker.

Given that the null-byte is mis-handled on at least two locations, during the repaint and during the copy/paste, I've come to the conclusion that JS Mime should not hand on the null-byte but should replace it with [x\00].

Comments?
Assignee

Updated

2 years ago
Group: mail-core-security
Reporter

Updated

2 years ago
Keywords: sec-low
Reporter

Updated

2 years ago
Group: mail-core-security
Reporter

Comment 3

2 years ago
@Jörg, as I said, we should simply strip the null byte. Not replace it.
Reporter

Comment 4

2 years ago
If we replace it, I would consider it an active attack, and I'd replace the *entire* local part with "[invalid]". No legitimate email address contains null bytes, so we should proactively protect the user.
Assignee

Comment 5

2 years ago
Posted patch jsmime.patch - WIP (obsolete) — Splinter Review
Here's your fix.

Note that is doesn't work when the message is already in the mail store since the old header will be served by Mork from the DB to the UI.

I need to add some tests.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8935130 - Flags: review?(mkmelin+mozilla)
Attachment #8935130 - Flags: review?(ben.bucksch)
Assignee

Comment 6

2 years ago
Posted patch jsmime.patch (v2) (obsolete) — Splinter Review
Final fix with test. Anyone can review ;-) First in, best dressed.
Attachment #8935130 - Attachment is obsolete: true
Attachment #8935130 - Flags: review?(mkmelin+mozilla)
Attachment #8935130 - Flags: review?(ben.bucksch)
Attachment #8935141 - Flags: review?(mkmelin+mozilla)
Attachment #8935141 - Flags: review?(ben.bucksch)
Attachment #8935141 - Flags: review?(acelists)
Reporter

Comment 7

2 years ago
Looks good to me.
For readability, could you please put the .replace() in its own line above the return, like so:

+ decoded = decoded.replace(/\x00/g, ''); // strip null chars
  return [decoded, ''];

and

+ var decoded = atob(sanitize);
+ decoded = decoded.replace(/\x00/g, ''); // strip null chars
- return [decoded, buffer];
Assignee

Comment 8

2 years ago
Comment on attachment 8935141 [details] [diff] [review]
jsmime.patch (v2)

Actually: No. This is readable. We don't need extra variables, not to mention that 'let' should be used instead of 'var'.

Explaining what code does is bad style, if anything, one could add a comment as to the why. But that would be:
  Strip null characters because hell will break loose if we let them through.
Also not useful.
Attachment #8935141 - Flags: review?(ben.bucksch)
Reporter

Updated

2 years ago
Attachment #8935141 - Flags: review-
Reporter

Comment 9

2 years ago
It obviously wasn't an important remark.
Assignee

Comment 10

2 years ago
Posted patch jsmime.patch (v3) (obsolete) — Splinter Review
I've talked with Joshua on IRC. JS Mime can also decode attachment data, like binary image data, although it's not used in TB so far.

So removing nulls should only be done when decoding RFC2047 tokens. This patch does that.

However, Joshua said on IRC:
01:42:36 - jcranmer: my gut would say to kill it from the access methods in mimeJSComponents.js

So this patch might be the wrong approach altogether.
Attachment #8935141 - Attachment is obsolete: true
Attachment #8935141 - Flags: review?(mkmelin+mozilla)
Attachment #8935141 - Flags: review?(acelists)
Reporter

Comment 11

2 years ago
Comment on attachment 8935176 [details] [diff] [review]
jsmime.patch (v3)

Looks good to me.

I'd still appreciate the "// strip null chars" comment, because I don't consider hex escape sequences to be readable, but alas...

Just a small request:

> removeNulls = false

sanitize = true

a) rename param: We might need to strip other chars later
b) the default should be safe. If callers want the raw binary data, they should explicitly say so. I think the majority of callers need the sanitized version.

Aside from that, looks good.

A third set of eyes can't hurt, so it would be nice to get jcranmer's review, too, if he's around anyways.
Attachment #8935176 - Flags: review?(Pidgeot18)
Reporter

Comment 12

2 years ago
the flag is ugly, that's true. but...

> my gut would say to kill it from the access methods in mimeJSComponents.js

my gut says to strip it as low level as possible. Less can go wrong, if you add another function that uses the decoder and the developer doesn't know about it.
Reporter

Comment 13

2 years ago
(One of the base rules of secure programming is to sanitize data as early as possible, as soon as it comes in. *Not* higher up / later on / when it's used.)
Reporter

Comment 14

2 years ago
I've compiled TB trunk with the patch v3, and it works fine for me and fixes the bug.
Assignee

Comment 15

2 years ago
Posted patch 1423432-nulls.patch (obsolete) — Splinter Review
This is what Joshua told me on IRC. Works too ;-)
Attachment #8935176 - Attachment is obsolete: true
Attachment #8935176 - Flags: review?(Pidgeot18)
Attachment #8935184 - Flags: review?(Pidgeot18)
Reporter

Comment 16

2 years ago
Reporter

Comment 17

2 years ago
Comment on attachment 8935176 [details] [diff] [review]
jsmime.patch (v3)

Please land this.

As explained above, this is better than the higher level fix, because there might be other functions using the decoder, now or in the future.

Secure coding rules demand to sanitize as soon as the data comes in, not later or higher up.
Attachment #8935176 - Attachment is obsolete: false
Attachment #8935176 - Flags: review+
Assignee

Comment 18

2 years ago
Oh, I forgot the test in mailnews/mime/jsmime/test/test_header.js:

+      // Bug 1423432: Encoded strings with null bytes,
+      // in base64 a single null byte can be encoded as AA== to AP==.
+      ["\"null=?UTF-8?Q?=00?=byte\" <nullbyte@example.com>",
+        [{name: "nullbyte", email: "nullbyte@example.com"}]],
+      ["\"null=?UTF-8?B?AA==?=byte\" <nullbyte@example.com>",
+        [{name: "nullbyte", email: "nullbyte@example.com"}]],

But that now doesn't pass any more :-(
Assignee

Comment 19

2 years ago
Test needs to go into test_nsIMsgHeaderParser5.js but not at 2:30 AM any more.

Comment 20

2 years ago
Pushed by mozilla.BenB@bucksch.org:
https://hg.mozilla.org/comm-central/rev/fef67283fe69
Strip null char from email addresses - p=JorgK, r=BenB
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Reporter

Comment 21

2 years ago
Given that v3 works, has a test, is QA tested, and fixes the bug fine, and is IMHO also the right approach, and time is important here, I landed this.

https://hg.mozilla.org/comm-central/rev/fef67283fe69
FIXED
Reporter

Comment 22

2 years ago
Requesting integration into TB 52.
Reporter

Comment 23

2 years ago
Posted image Tree test failures from v3 (obsolete) —
Unfortunately, this patch v3 caused many test failures with the tree burning.
So, I backed it out for now. https://hg.mozilla.org/comm-central/rev/187b19434a76
Maybe Jörg look at it tomorrow calmly.
Assignee

Comment 25

2 years ago
(In reply to Ben Bucksch (:BenB) from comment #23)
> Unfortunately, this patch v3 caused many test failures with the tree burning.
Yes, because you mixed it with unrelated changes from your local tree.
Please don't land patches withdrawn by the author in the future.
Assignee

Comment 26

2 years ago
Comment on attachment 8935176 [details] [diff] [review]
jsmime.patch (v3)

Joshua, this is not your preferred solution. I'll add the test to the other patch and then you can choose one.
Attachment #8935176 - Flags: review+ → review?(Pidgeot18)
Assignee

Comment 27

2 years ago
Joshua, this is what you suggested on IRC. I've added the test to test_nsIMsgHeaderParser5.js as you had suggested.
Attachment #8935184 - Attachment is obsolete: true
Attachment #8935241 - Attachment is obsolete: true
Attachment #8935184 - Flags: review?(Pidgeot18)
Attachment #8935289 - Flags: review?(Pidgeot18)
Assignee

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 28

2 years ago
(In reply to Jorg K (GMT+1) from comment #27)
> Created attachment 8935289 [details] [diff] [review]
> 1423432-nulls.patch (v1 + test)
> 
> Joshua, this is what you suggested on IRC. I've added the test to
> test_nsIMsgHeaderParser5.js as you had suggested.

This patch is pretty clearly the better way to go. But is there any reason all unprintable ascii is not being removed as well (aside from \n\r\t which need to go to the next level). Similar to the feed sanitization bug. Meaning regex like:
.replace(/[\x00-\x08\x0B-\x0C\x0E-\x1F\x7F]+/g, '')
Assignee

Comment 29

2 years ago
Thanks for stopping by ;-)

(In reply to alta88 from comment #28)
> This patch is pretty clearly the better way to go. But is there any reason
> all unprintable ascii is not being removed as well (aside from \n\r\t which
> need to go to the next level).
I asked myself the same question.
Reporter

Comment 30

2 years ago
I concur. That's what bug 1423439 is about. This bug here is about a quick obvious hotfix about the null char, while bug 1423439 addresses the broader issue.

Regarding patch v3 vs. nulls patch: Please consider that there might be other places that use the JSMime decoders and that expect strings, not binary data. These should be fixed as well, not just email headers. Then again, this is just a hotfix, so any reasonable patch that fixes the issue is fine for now.
Comment on attachment 8935289 [details] [diff] [review]
1423432-nulls.patch (v1 + test)

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

Yes let's use this one. 
I agree you should be able to drop the other ascii control characters too (except \n\r\t)

::: mailnews/mime/src/mimeJSComponents.js
@@ +224,5 @@
>  // XPIDL output.
>  function fixArray(addresses, preserveGroups, count) {
>    function resetPrototype(obj, prototype) {
>      let prototyped = Object.create(prototype);
> +    for (var key of Object.getOwnPropertyNames(obj)) {

nit: let instead of var while you're here
Attachment #8935289 - Flags: review+

Comment 33

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/356b95d1856c
remove null characters from headers (idea by jcranmer). r=mkmelin
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Assignee

Updated

2 years ago
Attachment #8935176 - Attachment is obsolete: true
Attachment #8935176 - Flags: review?(Pidgeot18)
Assignee

Comment 34

2 years ago
Comment on attachment 8935289 [details] [diff] [review]
1423432-nulls.patch (v1 + test)

Beta uplift coming.
Attachment #8935289 - Flags: review?(Pidgeot18)
Attachment #8935289 - Flags: approval-comm-esr52?
Attachment #8935289 - Flags: approval-comm-beta+
Assignee

Updated

2 years ago
Target Milestone: --- → Thunderbird 59.0

Comment 36

2 years ago
(In reply to Jorg K (GMT+1) from comment #29)
> Thanks for stopping by ;-)
> 
> (In reply to alta88 from comment #28)
> > This patch is pretty clearly the better way to go. But is there any reason
> > all unprintable ascii is not being removed as well (aside from \n\r\t which
> > need to go to the next level).
> I asked myself the same question.

Well, despite that and 2 people including the reviewer asking to include all non ascii as well, why didn't that land?

For the record, all these other mailsploit bugs are completely WONTFIX. Evicting longtime spec compliant chars due to spoofing concerns is ill-considered and kneejerk and should not be entertained further. There is a clear prior art solution in what Fx and others do in the urlbar for homograph spoofs. The solution is to highlight the valid email, @, and domain while reducing the importance of any localpart string and middle cropping it, with a tooltip for the whole string, and ensuring the valid parts are always visible and visually enhanced.
Assignee

Comment 37

2 years ago
(In reply to alta88 from comment #36)
> Well, despite that and 2 people including the reviewer asking to include all
> non ascii as well, why didn't that land?
There are many reasons:
We, or let's say *I*, had little time. As you can see, a patch was landed here pre-maturely and backed out. So I wanted to fix the damage asap.

We're talking about the non-printable characters 0x00 to 0x1F. I'd have to see how an e-mail with these characters behaves. Having had a quick look now, they show as illegal characters, so little square boxes with two rows of numbers in them. Is that so bad? I'd rather show that the header has garbage than sanitising it so it looks valid when it isn't.

Also, I trusted comment #30 saying that bug 1423439 is about this. But it's not. That bug wants so remove much more (that IMHO will cause a stream of user complaints).

So if anyone wants to file a bug and argue in favour of removal of those characters, they can go ahead. It's pretty simple to write a patch including a test. The feed sanitization (bug 1411699) on the other hand was done to fix a security issue.
Reporter

Comment 38

2 years ago
alta88:
> include all non ascii as well, why didn't that land?

Because this bug is about the null character only, and a direct fix for the concrete problem we know. The point was to fix this particular known bug, and fix it quickly.

We should then take a little time to discuss what further additional protections we will add. But that doesn't need to be done in the middle of the night.

I would like to ask everybody to try to avoid accusations of any sort, including "Why did you [not] do ABC?". Let's help each other.

Comment 39

2 years ago
In what version of TB will the patch appear?
(In reply to Adrian Zaugg from comment #39)
> In what version of TB will the patch appear?

The current patch will be in 58 beta 2, currently being tested.  If the current patch flies further, it will likely be in 52.5.1 at a time yet to be determined.
Given that this is rated sec-low (and whatever other considerations exist), is it essential for this patch to go out in a 52.x before Dec 25?
Assignee

Comment 42

2 years ago
It's in TB 58 beta 2 and hasn't caused any problems, so why not ship it in the next ESR, whenever that's going out?
Assignee

Updated

2 years ago
Attachment #8935289 - Flags: approval-comm-esr52? → approval-comm-esr52+
Alias: CVE-2017-7829
Summary: Mailsploit part 1: From addresses with null character are cut off → Mailsploit part 1: From addresses with encoded null character are cut off

Comment 44

2 years ago
Mailsploit test (https://www.mailsploit.com/) renders exactly the same in v52.5.2 as in v52.5.0 on Windows 7 x64.
Most importantly, the 'From' column of the mailboxes till shows the forged (cut-off prefix) email address.

In the preview pane of by opening the email content, no display change is to be noticed.

Seeing no difference at all since v52.5.2, I hardly consider thix bug as 'fixed'.
Moreover, the 'Low' criticality label is debatable.
Flags: needinfo?(jorgk)
Assignee

Comment 45

2 years ago
I didn't classify this as "low", but that's apparently the rating if it's spoofing since From addresses are generally not reliable.

The fix works, but not for messages which are already received; for those, the header is served from the database (.msf) file. So repair the folder or drag the message out to the desktop and then re-import it.
Flags: needinfo?(jorgk)

Comment 46

2 years ago
Old test, as generated on v52.5.0 earlier in December, can be seen below and renders exactly the same.

Comment 47

2 years ago
It was implied but apparently not understood nor even considered that a new test was issued on the Mailsploit website after having updated Thunderbird to v52.5.2, thus I clarify that here and now...

The render of the new test is equally rendered in every aspect to the old test done on v52.5.0.
Assignee

Comment 48

2 years ago
I don't understand what you're saying.

https://docs.google.com/spreadsheets/d/1jkb_ZybbAoUA43K902lL-sB7c1HMQ78-fhQ8nowJCQk/edit#gid=0
says: GOOD NEWS! Mozilla fixed the issue in Thunderbird. https://bugzilla.mozilla.org/show_bug.cgi?id=1423432

I've just tried 52.5.2 again, when importing a sample e-mail, I see the full sender address, no truncation takes place.

Comment 49

2 years ago
If you look at the screenshot I uploaded, you will notice the headers in the mail preview could be considered 'OK' as the 'mailsploit.com' suffix appears.

However, in the mail list on top, you can clearly see the truncated, spoofed, email address is shown. IMHO, the mail list 'From' address is of greater importance, since it represents the entry point for people looking at their email messages.

The Google spreadsheet you share merely quote this bug report, especially its claim of having the problem solved. Could you specify how this document helps addressing the current issue?

At best, I think it would be most appropriate to consider the resolution of the issue WIP or partially solved, not resolved.
Assignee

Comment 50

2 years ago
The situation is this:

For newly received or imported messages, both the thread pane (message list) and the header pane show the full address.

For messages received with TB 52.5.0 or earlier, TB 52.5.2 will only show the full address in the header pane but not in the thread pane. The display in the thread pane comes from the message database, the .msf file, as mentioned in comment #45.

I was wrong in saying that repairing the folder (via Properties, Repair Folder) will fix the truncated display. To fix the truncated display in the thread pane a) delete the folder's .msf file or b) save/drag the message to the desktop or a folder and then re-import it.

Sadly we can't regenerate all databases containing truncated headers.

Comment 51

2 years ago
Seriously? I do not know how to phrase that better... That will be my last attempt, though, I won't fight one who won't listen.
Read the following slowly:

What my screenshot shows highlighted is a newly received message (2017-12-28T07:43:**+01:00) on Thunderbird v52.5.2.
The message *underneath* (not selected) is a remnant from the *previous* test in early December, done on Thunderbird v52.5.0.

*Both* messages show a corrected, non-truncated, email address in the headers parts of the opened message ("potus@whitehouse.gov"@mailsploit.com). Even the message received at the time on v52.5.0 shows the corrected output.
*Both* messages show exactly the same, so I do not know anything about your database stuff, but it does not seem to relate to the problem I am mentioning here.

What I see which is *wrong* is the truncated, spoofed, email address in the 'From' column from the messages list.

Sum-up of the problem at hand here:
A message received from the mailsploit.com website test tool on *v52.5.2* still improperly shows the spoofed email address in  the preview of the email headers in the email messages list.

Please have a slow, thorough look at the screenshot I uploaded once again.
Assignee

Updated

2 years ago
Blocks: 1427382
Assignee

Comment 52

2 years ago
Thanks for your persistence. I filed bug 1427382 to address the remaining issues.

Your comments preceding comment #51 weren't as clear. You could have summarised your concern in two lines:

Go to https://www.mailsploit.com/ and "Try The Demo"
Send all 14 "payloads", TB will only handle five of them. In nine of them, the From address is still truncated somewhere.

Comment 53

2 years ago
Thank you for bearing with me. Genuinely happy to see this leads somewhere!

I'll for sure try to be clearer and more concise in the future, here and anywhere else.
Cheers for new year's eve, and happy Thunderbird 2018!
You need to log in before you can comment on or make changes to this bug.