Closed
Bug 1423432
(CVE-2017-7829)
Opened 7 years ago
Closed 7 years ago
Mailsploit part 1: From addresses with encoded null character are cut off
Categories
(MailNews Core :: MIME, enhancement)
Tracking
(thunderbird_esr5257+ fixed, thunderbird58 fixed, thunderbird59 fixed)
RESOLVED
FIXED
Thunderbird 59.0
People
(Reporter: BenB, Assigned: jorgk-bmo)
References
Details
(Keywords: csectype-spoof, sec-low)
Attachments
(4 files, 5 obsolete files)
5.53 KB,
image/png
|
Details | |
21.91 KB,
image/png
|
Details | |
2.73 KB,
patch
|
mkmelin
:
review+
jorgk-bmo
:
approval-comm-beta+
jorgk-bmo
:
approval-comm-esr52+
|
Details | Diff | Splinter Review |
32.49 KB,
image/png
|
Details |
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•7 years ago
|
Severity: normal → major
Assignee | ||
Comment 1•7 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•7 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•7 years ago
|
Group: mail-core-security
Reporter | ||
Updated•7 years ago
|
Group: mail-core-security
Reporter | ||
Comment 3•7 years ago
|
||
@Jörg, as I said, we should simply strip the null byte. Not replace it.
Reporter | ||
Comment 4•7 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•7 years ago
|
||
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•7 years ago
|
||
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•7 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•7 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•7 years ago
|
Attachment #8935141 -
Flags: review-
Reporter | ||
Comment 9•7 years ago
|
||
It obviously wasn't an important remark.
Assignee | ||
Comment 10•7 years ago
|
||
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•7 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•7 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•7 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•7 years ago
|
||
I've compiled TB trunk with the patch v3, and it works fine for me and fixes the bug.
Assignee | ||
Comment 15•7 years ago
|
||
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•7 years ago
|
||
Reporter | ||
Comment 17•7 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•7 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•7 years ago
|
||
Test needs to go into test_nsIMsgHeaderParser5.js but not at 2:30 AM any more.
Comment 20•7 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: 7 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 21•7 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•7 years ago
|
||
Requesting integration into TB 52.
tracking-thunderbird_esr52:
--- → ?
Reporter | ||
Comment 23•7 years ago
|
||
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.
Reporter | ||
Comment 24•7 years ago
|
||
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)
Assignee | ||
Comment 25•7 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•7 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•7 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•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 28•7 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•7 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•7 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.
Assignee | ||
Comment 31•7 years ago
|
||
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•7 years ago
|
||
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•7 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: 7 years ago → 7 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•7 years ago
|
status-thunderbird58:
--- → affected
status-thunderbird59:
--- → fixed
status-thunderbird_esr52:
--- → affected
Assignee | ||
Updated•7 years ago
|
Attachment #8935176 -
Attachment is obsolete: true
Attachment #8935176 -
Flags: review?(Pidgeot18)
Assignee | ||
Comment 34•7 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•7 years ago
|
Target Milestone: --- → Thunderbird 59.0
Assignee | ||
Comment 35•7 years ago
|
||
Comment 36•7 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•7 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•7 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•7 years ago
|
||
In what version of TB will the patch appear?
Comment 40•7 years ago
|
||
(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•7 years ago
|
||
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•7 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•7 years ago
|
Attachment #8935289 -
Flags: approval-comm-esr52? → approval-comm-esr52+
Assignee | ||
Comment 43•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Updated•7 years ago
|
Keywords: csectype-spoof
Updated•7 years ago
|
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•7 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•7 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•7 years ago
|
||
Old test, as generated on v52.5.0 earlier in December, can be seen below and renders exactly the same.
Comment 47•7 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•7 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•7 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•7 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•7 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 | ||
Comment 52•7 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•7 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.
Description
•