1423432 - (CVE-2017-7829) Mailsploit part 1: From addresses with encoded null character are cut off

Status: RESOLVED FIXED

(MailNews Core :: MIME, enhancement)

Description

[Ben Bucksch (:BenB)]

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: <###[e-mail]###>
DATA
From: =?utf-8?b?cG90dXNAd2hpdGVob3VzZS5nb3Y=?==?utf-8?Q?=00?==?utf-8?b?cG90dXNAd2hpdGVob3VzZS5nb3Y=?=@mailsploit.com
To: [e-mail]
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:
"[e-mail]
* If you click on the email address, you still see the same email address in the context menu.

Expected result:
* The From field displays:
[e-mail]
or even better:
[e-mail]

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.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: Picture showing correct decoding by JS Mime

When you import the message, it is displayed as 
"[e-mail][null][e-mail]"@mailsploit.com

That indicates that JS Mime is displaying it correctly. However, with the next repaint event, this is shortened to "[e-mail].

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

Comment 2

[Jorg K (CEST = GMT+2)]

Replying to the e-mail in question gives this in the composition:
  On 02/12/2017 19:16, "[e-mail]@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
  [e-mail] <>
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?

Comment 3

[Ben Bucksch (:BenB)]

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

Comment 4

[Ben Bucksch (:BenB)]

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.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: jsmime.patch - WIP

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.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: jsmime.patch (v2)

Final fix with test. Anyone can review ;-) First in, best dressed.

Comment 7

[Ben Bucksch (:BenB)]

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];

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[Ben Bucksch (:BenB)]

It obviously wasn't an important remark.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: jsmime.patch (v3)

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.

Comment 11

[Ben Bucksch (:BenB)]

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.

Comment 12

[Ben Bucksch (:BenB)]

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.

Comment 13

[Ben Bucksch (:BenB)]

(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.)

Comment 14

[Ben Bucksch (:BenB)]

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

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1423432-nulls.patch

This is what Joshua told me on IRC. Works too ;-)

Comment 16

[Ben Bucksch (:BenB)]

Attachment: Screenshot, after fix v3

Comment 17

[Ben Bucksch (:BenB)]

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.

Comment 18

[Jorg K (CEST = GMT+2)]

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\" <[e-mail]>",
+        [{name: "nullbyte", email: "[e-mail]"}]],
+      ["\"null=?UTF-8?B?AA==?=byte\" <[e-mail]>",
+        [{name: "nullbyte", email: "[e-mail]"}]],

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

Comment 19

[Jorg K (CEST = GMT+2)]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fef67283fe69
Strip null char from email addresses - p=JorgK, r=BenB

Comment 21

[Ben Bucksch (:BenB)]

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

Comment 22

[Ben Bucksch (:BenB)]

Requesting integration into TB 52.

Comment 23

[Ben Bucksch (:BenB)]

Attachment: Tree test failures from v3

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.

Comment 24

[Ben Bucksch (:BenB)]

See https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=fef67283fe69 (patch v3, many failures)
vs. https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=187b19434a76 (backout, much less failures)

Comment 25

[Jorg K (CEST = GMT+2)]

(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.

Comment 26

[Jorg K (CEST = GMT+2)]

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.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 28

[alta88]

(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, '')

Comment 29

[Jorg K (CEST = GMT+2)]

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.

Comment 30

[Ben Bucksch (:BenB)]

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 31

[Jorg K (CEST = GMT+2)]

I've shipped this off to a try run to avoid surprises:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1adecde50b0f1be5c11848da09566d0b6ff7c1c1

Comment 32

[Magnus Melin [:mkmelin]]

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/356b95d1856c
remove null characters from headers (idea by jcranmer). r=mkmelin

Comment 34

[Jorg K (CEST = GMT+2)]

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

Beta uplift coming.

Comment 35

[Jorg K (CEST = GMT+2)]

Beta (TB 58):
https://hg.mozilla.org/releases/comm-beta/rev/d75a75d200f707316d6894635462309f8698d0cc

Comment 36

[alta88]

(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.

Comment 37

[Jorg K (CEST = GMT+2)]

(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.

Comment 38

[Ben Bucksch (:BenB)]

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

[Adrian Zaugg]

In what version of TB will the patch appear?

Comment 40

[Wayne Mery (:wsmwk)]

(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.

Comment 41

[Wayne Mery (:wsmwk)]

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?

Comment 42

[Jorg K (CEST = GMT+2)]

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?

Comment 43

[Jorg K (CEST = GMT+2)]

TB 52.5.2 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/8bc93e746d8217a13c77f44565ad68a03da63882

Comment 44

[Bernard]

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.

Comment 45

[Jorg K (CEST = GMT+2)]

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.

Comment 46

[Bernard]

Attachment: Mailsploit attack as tested on Thunderbird v52.5.2, Windows x64

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

Comment 47

[Bernard]

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.

Comment 48

[Jorg K (CEST = GMT+2)]

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

[Bernard]

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.

Comment 50

[Jorg K (CEST = GMT+2)]

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

[Bernard]

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 ("[e-mail]"@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.

Comment 52

[Jorg K (CEST = GMT+2)]

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

[Bernard]

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!