Last Comment Bug 900552 - HTML code injection by renaming attached file
: HTML code injection by renaming attached file
Status: RESOLVED FIXED
: sec-low
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 26.0
Assigned To: Mark Banner (:standard8)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-01 09:20 PDT by Al Billings [:abillings]
Modified: 2013-09-10 04:01 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
24+
fixed


Attachments
PoC video (913.65 KB, video/mp4)
2013-08-01 09:20 PDT, Al Billings [:abillings]
no flags Details
Eml Test Case (849 bytes, text/plain)
2013-08-05 04:01 PDT, Mark Banner (:standard8)
no flags Details
Escape HTML in the attachment names (2.13 KB, patch)
2013-08-05 04:04 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Automated test case (1.10 KB, patch)
2013-08-05 04:05 PDT, Mark Banner (:standard8)
Pidgeot18: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
standard8: approval‑comm‑esr17+
Details | Diff | Splinter Review
Escape HTML in the attachment names v2 (2.13 KB, patch)
2013-08-05 04:35 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Escape HTML in the attachment names v2 (2.18 KB, patch)
2013-08-05 04:54 PDT, Mark Banner (:standard8)
neil: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
standard8: approval‑comm‑esr17+
Details | Diff | Splinter Review

Description Al Billings [:abillings] 2013-08-01 09:20:19 PDT
Created attachment 784428 [details]
PoC video

Security@ received the following email from the researcher, cc'd on the bug:

I have found a HTML Code Injection, to attach a file in a new mail, to rename the attached file this implies that you can insert HTML without being filtered properly. This vulnerability allows successful phishing attack.


Steps to reproduce:

 1) Compose a new mail
 2) Attach
 3) Select a file ... (.exe, .png, .html, etc, etc.)
 4) Rename the attached file  with HTML Code (right click, rename)
Comment 1 Olli Pettay [:smaug] 2013-08-01 09:24:20 PDT
Why Core: Editor? isn't this Thunderbird bug?
Comment 2 Fabian Cuchietti 2013-08-01 09:26:44 PDT
What mean CORE for this Bug?
Comment 3 Al Billings [:abillings] 2013-08-01 10:01:14 PDT
Olli, I'm fuzzy where the lines in some of this code is. If you think this is specific to Thunderbird and that Seamonkey, for example, is not vulnerable to this, please change the product and component.

Fabian, this is just where the code for the problem lives. It makes it easier to get this assigned to the right person to fix.
Comment 4 Fabian Cuchietti 2013-08-04 15:50:22 PDT
I also believe that there  no is a  problem in the editor.
Comment 5 Mark Banner (:standard8) 2013-08-05 02:58:40 PDT
I've just tried testing this. This does only affects

- Users with "Display attachments inline" turned on

and one of:

- Users who have allowed remote content to always be loaded from the sender
- Users who choose to load the remote content for the message
Comment 6 Mark Banner (:standard8) 2013-08-05 04:01:12 PDT
Created attachment 785698 [details]
Eml Test Case

Simple test case for manual reproduction.
Comment 7 Mark Banner (:standard8) 2013-08-05 04:04:19 PDT
Created attachment 785699 [details] [diff] [review]
Escape HTML in the attachment names

This ensures that we escape any html in the attachment names before they get injected into the HTML.
Comment 8 Mark Banner (:standard8) 2013-08-05 04:05:40 PDT
Created attachment 785701 [details] [diff] [review]
Automated test case

Joshua, this seemed to be the easiest place to test for the replacement, but I wondered if you knew of any other ways so that we could check the other cases.
Comment 9 neil@parkwaycc.co.uk 2013-08-05 04:23:54 PDT
Comment on attachment 785699 [details] [diff] [review]
Escape HTML in the attachment names

>+      UtilityWrite(escapedName);
>       mHTMLHeaders.Append(name);
Did you mean mHTMLHeaders.Append(escapedName); ?
Comment 10 Mark Banner (:standard8) 2013-08-05 04:35:53 PDT
Created attachment 785708 [details] [diff] [review]
Escape HTML in the attachment names v2

Yes I did, bad c&p.
Comment 11 Mark Banner (:standard8) 2013-08-05 04:54:32 PDT
Created attachment 785714 [details] [diff] [review]
Escape HTML in the attachment names v2

This time with the change included.
Comment 12 Fabian Cuchietti 2013-08-05 06:06:36 PDT
What sec-??? is considered?
Comment 13 Mark Banner (:standard8) 2013-08-05 06:20:12 PDT
I think this is probably sec-low or maybe sec-moderate, but I'd value input from Dan here as he's got more experience than me with these.
Comment 14 Fabian Cuchietti 2013-08-05 06:24:59 PDT
As you may consider it an HTML injection to sec-low?
Comment 15 Mark Banner (:standard8) 2013-08-05 06:49:07 PDT
(In reply to Fabian Cuchietti from comment #14)
> As you may consider it an HTML injection to sec-low?

Sorry, yes, I should have explained that. It is an html injection, but javascript is not available (I've just confirmed that). So from a user perspective, they attacker could spoof some html, but it'd appear in the message reader section and they could just do that via writing an html message.

Although the attacker could cause remote content to be loaded, a) we ask the user by default and b) javascript isn't available even within the remote content.

Therefore, this seems to fit the "Identification of users by profiling browsing behavior." category of sec-low.
Comment 16 Joshua Cranmer [:jcranmer] 2013-08-05 08:25:36 PDT
Comment on attachment 785701 [details] [diff] [review]
Automated test case

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

The way this is implemented may be surprising to some API consumers, but I don't think it will matter that much in practice.
Comment 17 Fabian Cuchietti 2013-08-05 16:17:11 PDT
This issue is considered for a Bounty?
Comment 18 Matt Wobensmith [:mwobensmith][:matt:] 2013-08-07 10:43:37 PDT
As per Dan, sec-low, unintended but valid bug, minimal effect because we accept HTML content in the mail body and therefore filter dangerous content.
Comment 19 Fabian Cuchietti 2013-08-07 12:15:07 PDT
(In reply to Matt Wobensmith from comment #18)
> As per Dan, sec-low, unintended but valid bug, minimal effect because we
> accept HTML content in the mail body and therefore filter dangerous content.

I don't understand. Is valid for a bounty?
Comment 20 Mark Banner (:standard8) 2013-08-07 13:03:48 PDT
The policy is available here which will give you the general guidelines for when bounties are awarded:

http://www.mozilla.org/security/bug-bounty.html
Comment 21 Fabian Cuchietti 2013-08-07 16:01:49 PDT
But you do not talk about sec-low (only sec-critical/high) too in wiki.mozilla.org
Comment 22 Matt Wobensmith [:mwobensmith][:matt:] 2013-08-07 16:18:05 PDT
Fabian, only bugs that have been rated high/critical will receive a bounty. Therefore, to answer your question, this bug will not receive one.
Comment 23 Joe Sabash [:JoeS1] 2013-08-07 20:34:21 PDT
I was a little concerned that this might interfere with native ogg/audio or the use of iframe in the html body.
Those functions work normally, and the code injection in the attachment header fix seems to work as designed.
tested with  http://hg.mozilla.org/mozilla-central/rev/45128af17739
with this patch imported to a local build.
Comment 24 Fabian Cuchietti 2013-08-08 07:34:54 PDT
(In reply to Matt Wobensmith from comment #22)
> Fabian, only bugs that have been rated high/critical will receive a bounty.
> Therefore, to answer your question, this bug will not receive one.

My previous bounty was to "sec-moderate."
Comment 26 Mark Banner (:standard8) 2013-08-12 12:18:20 PDT
Comment on attachment 785714 [details] [diff] [review]
Escape HTML in the attachment names v2

[Triage Comment]
Taking onto branches to get some testing before next releases.
Comment 28 Mark Banner (:standard8) 2013-09-10 03:29:54 PDT
Comment on attachment 785701 [details] [diff] [review]
Automated test case

[Triage Comment]
No complaints afaik, so we'll take this to esr 17 as well.

Note You need to log in before you can comment on or make changes to this bug.