Closed Bug 1423432 (CVE-2017-7829) Opened 3 years ago Closed 3 years ago

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

Categories

(MailNews Core :: MIME, enhancement)

enhancement
Not set
major

Tracking

(thunderbird_esr5257+ fixed, thunderbird58 fixed, thunderbird59 fixed)

RESOLVED FIXED
Thunderbird 59.0
Tracking Status
thunderbird_esr52 57+ fixed
thunderbird58 --- fixed
thunderbird59 --- fixed

People

(Reporter: BenB, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-spoof, sec-low)

Attachments

(4 files, 5 obsolete files)

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.
Severity: normal → major
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.
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?
Group: mail-core-security
Keywords: sec-low
Group: mail-core-security
@Jörg, as I said, we should simply strip the null byte. Not replace it.
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.
Attached 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)
Attached 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)
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 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)
Attachment #8935141 - Flags: review-
It obviously wasn't an important remark.
Attached 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)
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)
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.
(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.)
I've compiled TB trunk with the patch v3, and it works fine for me and fixes the bug.
Attached 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)
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+
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 :-(
Test needs to go into test_nsIMsgHeaderParser5.js but not at 2:30 AM any more.
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: 3 years ago
Resolution: --- → FIXED
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
Requesting integration into TB 52.
Attached 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.
(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 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)
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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, '')
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.
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+
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: 3 years ago3 years ago
Resolution: --- → FIXED
Attachment #8935176 - Attachment is obsolete: true
Attachment #8935176 - Flags: review?(Pidgeot18)
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+
Target Milestone: --- → Thunderbird 59.0
(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.
(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.
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.
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?
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?
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
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)
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)
Old test, as generated on v52.5.0 earlier in December, can be seen below and renders exactly the same.
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.
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.
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.
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.
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.
Blocks: 1427382
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.
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.