Bug 1005552 (CVE-2014-8644)

<marquee> event handlers run when JS disabled (e.g. in Thunderbird mail)

VERIFIED FIXED in Firefox 30

Status

()

VERIFIED FIXED
5 years ago
7 days ago

People

(Reporter: dveditz, Assigned: bholley)

Tracking

(Blocks: 1 bug, {sec-high})

unspecified
mozilla32
sec-high
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox29 wontfix, firefox30+ verified, firefox31+ verified, firefox32+ verified, firefox-esr2430+ verified, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, seamonkey2.26 fixed)

Details

(Whiteboard: [embargo until esr24 eol][adv-main30-][adv-esr24.6-])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #1004697 +++

Splitting the <marquee> issue from multi-issue bug 1004697

The <marquee> element implements 3 event handlers in XBL, onstart, onfinish, and onbounce. These will run even when javascript is disabled, as it is in Thunderbird mail.


Firefox testcase:
1. disable javascript
2. load the following URL
   data:text/html,<marquee%20onstart=alert(1)></marquee>

Thunderbird testscase:
1. compose HTML mail to yourself
2. from the Insert menu select HTML...
3. Add the following 
   <marquee onstart=alert(document.body.innerHTML)>.</marquee>
4. send the mail to yourself and open in Thunderbird

The script will execute in both the composition window and the message window when receiving the mail.

I have not yet tested but I assume this bypasses NoScript protection.

Could this have anything to do with why in bug 987222 errors from marquee events are mis-identified as coming from system JS? And if so does that patch help?
Flags: sec-bounty?
(In reply to Daniel Veditz [:dveditz] from comment #0)

> I have not yet tested but I assume this bypasses NoScript protection.

Yes it does, just checked :(
(In reply to Giorgio Maone from comment #1)
> (In reply to Daniel Veditz [:dveditz] from comment #0)
> 
> > I have not yet tested but I assume this bypasses NoScript protection.
> 
> Yes it does, just checked :(

... and worked around in 2.6.8.21, http://noscript.net/getit#direct - thank you.

Comment 4

5 years ago
Do you filed a new apps bug for Noscript? Or it's not important to filed it?
(Reporter)

Comment 5

5 years ago
NoScript is a 3rd party add-on and does not track its bugs at bugzilla.mozilla.org. According to comment 2 the latest version has a workaround for this bug.

Comment 6

5 years ago
OK. Thx

Comment 7

5 years ago
hi,
just a question
when Is the bug is assigned?
How long does it take?
(Assignee)

Comment 8

5 years ago
I hope to get to this sometime this week or next.
Assignee: nobody → bobbyholley

Comment 9

5 years ago
ok thx you

Comment 10

5 years ago
just a question, bug is not assignated to the reporter?

Comment 11

5 years ago
assigned, sorry
(Assignee)

Comment 12

5 years ago
(In reply to nico from comment #11)
> assigned, sorry

"Assigned" refers to the person that writes the patch to fix the bug.

Comment 13

5 years ago
Ok sorry. It was what I thought but I wasn't sure. Sorry for questions that may seem strange for you. But this is my first bug on bugzilla and I don't still mastered the procedure. Thx.
(Assignee)

Comment 14

5 years ago
(In reply to nico from comment #13)
> Ok sorry. It was what I thought but I wasn't sure. Sorry for questions that
> may seem strange for you. But this is my first bug on bugzilla and I don't
> still mastered the procedure. Thx.

No problem at all! Thanks for reporting this. :-)
(Assignee)

Comment 15

5 years ago
The issue here is that we're currently binding event event handlers (which began in bug 871887 comment 5). This causes |realCallback| in CallSetup to be privileged, and allows it to bypass script disabling.

We can just remove this whole dance now. I'll attach a patch.
(Assignee)

Comment 16

5 years ago
These functions get invoked as event listeners, so we'll automatically get the
proper |this|. The reason for the existing shenanigans was to work around
bug 872772, which has now been fixed.
Attachment #8423279 - Flags: review?(bzbarsky)
(Assignee)

Comment 17

5 years ago
Posted patch Tests. v1Splinter Review
Attachment #8423280 - Flags: review?(bzbarsky)
Comment on attachment 8423279 [details] [diff] [review]
Stop binding marquee event handlers. v1

r=me
Attachment #8423279 - Flags: review?(bzbarsky) → review+
Comment on attachment 8423280 [details] [diff] [review]
Tests. v1

>+    m.setAttribute("onstart", "ok(false, 'Shouldnt call inline event handler');");

"Shouldn't", and same in the next line.

r=me
Attachment #8423280 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #19)
> Comment on attachment 8423280 [details] [diff] [review]
> Tests. v1
> 
> >+    m.setAttribute("onstart", "ok(false, 'Shouldnt call inline event handler');");
> 
> "Shouldn't", and same in the next line.

This was intentional, because of the double-quote nesting. But I'll just make it "Should not".
(Assignee)

Updated

5 years ago
Flags: in-testsuite?
(Assignee)

Comment 21

5 years ago
Comment on attachment 8423279 [details] [diff] [review]
Stop binding marquee event handlers. v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It would be very difficult to determine the security issue by inspecting this patch. If the security issue was determined, an exploit (NoScript bypass) would be trivial.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The tests yes, but I will delay landing them.

Which older supported branches are affected by this flaw?

22 and up.

If not all supported branches, which bug introduced the flaw?

bug 871887.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Backporting to 22 and 23 would be hard, but I don't think we need it. Backporting to 24 and above is trivial, because we have bug 872772.

How likely is this patch to cause regressions; how much testing does it need?

Our internal test coverage for <marquee> is almost non-existent, so testing would be good.
Attachment #8423279 - Flags: sec-approval?
sec-approval+ for trunk. Please create and nominate patches for Aurora, Beta, and ESR24 after trunk is green.
status-firefox29: --- → wontfix
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox-esr24: --- → affected
tracking-firefox30: --- → +
tracking-firefox31: --- → +
tracking-firefox32: --- → +
tracking-firefox-esr24: --- → +
Attachment #8423279 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/be9514701bd5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox32: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla32

Comment 25

5 years ago
it's not fixed on Thunderbird...
What is this bug appear in my reports? in my dashboard? ( 1004697 & 1005552)
thx 
:)
(Assignee)

Comment 26

5 years ago
Comment on attachment 8423279 [details] [diff] [review]
Stop binding marquee event handlers. v1

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

Flagging this patch for branch approval.

A note on the uplift - there will be conflicts on beta and esr24. It should be ok to just remove the old thing and use the lines added in this patch. Flagging verifyme to make sure that somebody runs the mochitest here on branches, even though we don't want to check it in.
Attachment #8423279 - Flags: approval-mozilla-esr24?
Attachment #8423279 - Flags: approval-mozilla-beta?
Attachment #8423279 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Keywords: verifyme
Flags: sec-bounty? → sec-bounty+
Attachment #8423279 - Flags: approval-mozilla-esr24?
Attachment #8423279 - Flags: approval-mozilla-esr24+
Attachment #8423279 - Flags: approval-mozilla-beta?
Attachment #8423279 - Flags: approval-mozilla-beta+
Attachment #8423279 - Flags: approval-mozilla-aurora?
Attachment #8423279 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e913c50067f2
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/0586f2989c54
https://hg.mozilla.org/releases/mozilla-esr24/rev/00af762ed273
status-b2g-v1.2: --- → fixed
status-b2g-v1.3: --- → fixed
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → fixed
status-firefox-esr24: affected → fixed
status-b2g-v1.3T: affected → fixed
(Assignee)

Comment 31

5 years ago
Looks like we need to waive window before accessing the Function constructor, rather than after, because bug 933681 landed in FF28.
Reproduced the original issue using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-01-03-02-02-mozilla-central/

Firefox Test Cases being used:

- Downloaded & install FX
- Inserted "data:text/html,<marquee%20onstart=alert(1)></marquee>" into the URL Bar and ensure a prompt appears
- Under about:config, changed: "javascript.enabled;false"
- Inserted "data:text/html,<marquee%20onstart=alert(1)></marquee>" into the URL Bar and ensure there's no prompt

Went through the verification process using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-26-03-02-02-mozilla-central/ [Build ID: 20140526030202]
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-26-00-40-04-mozilla-aurora/ [Build ID: 20140526004004]
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/mac/en-US/ [Build ID: 20140522105902]

I also downloaded NoScript 2.6.8.25 and ensured that the original issue isn't reproducible [Nightly, Aurora, BETA]

However, I can still reproduce the original issue with the latest ESR build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-26-00-05-02-mozilla-esr24/
- Built from https://hg.mozilla.org/releases/mozilla-esr24/rev/133851640781 (Sat May 24 03:13:49 2014)

The fix was pushed on Tue May 20 20:31:34 2014 in comment # 32. This should have been fixed in the build that I was using correct? Just making sure I didn't miss something!
status-firefox30: fixed → verified
status-firefox31: fixed → verified
status-firefox32: fixed → verified
(Assignee)

Comment 34

5 years ago
(In reply to Kamil Jozwiak [:kjozwiak] from comment #33)
> The fix was pushed on Tue May 20 20:31:34 2014 in comment # 32. This should
> have been fixed in the build that I was using correct? Just making sure I
> didn't miss something!

Yes, it should. Can you try verifying with a page (rather than a data:URI) to see if that's the issue? Let me know if it still reproduces, and then I'll take a look.
(In reply to Bobby Holley (:bholley) from comment #34) 
> Yes, it should. Can you try verifying with a page (rather than a data:URI)
> to see if that's the issue? Let me know if it still reproduces, and then
> I'll take a look.

I went through the tests again this time using the HTML file that I've attached rather than using data:URI. It looks like it's still failing under ESR:

- Disabled JS via "javascript.enabled;false" (message box still appearing)
- Installed NoScript v2.6.8.26 (message box still appearing)

It works correctly on Nightly, Aurora and BETA. Once JS is disabled or NoScript is installed, the message box doesn't appear when opening the HTML file or going through data:URI.

Bobby, let me know if there's anything else I can try!
(Assignee)

Comment 36

5 years ago
Hm, so I think the issue here is that esr24 was before bug 840488 and before event handlers were on WebIDL.

Looking back at that code, and running some tests, it looks like we never actually did scriptability checks for event listeners. In particular, I can load a page with script disabled, use eval to add an event listener (via the web console), and watch that event listener fire.

Boris, is that really the situation? If so, I think we should just embargo this on esr24.
Flags: needinfo?(bzbarsky)
> Boris, is that really the situation? 

That's correct.  See also bug 409737 and bug 822213, which WebIDL event listeners fixed.
Flags: needinfo?(bzbarsky)
What we did do is do checks for script-enabled before _compiling_ event listeners.  Could we do that for marquee on ESR24?
(Assignee)

Comment 39

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #38)
> What we did do is do checks for script-enabled before _compiling_ event
> listeners.  Could we do that for marquee on ESR24?

The problem is that the marquee code basically converts event handlers to event listeners, which it installs via addEventListener. IIUC, the esr24 strategy involves assuming that pages with script disabled can't addEventListener in the first place. There don't appear to be any checks in either addEventListener or event dispatch for this. :-(

I wonder if we do something clever with setTimeout. I'll give that a shot.
status-firefox-esr24: fixed → affected
> There don't appear to be any checks in either addEventListener or event dispatch for this.

Correct.  Can we just have the marquee binding itself check whether script is enabled before doing the addEventListener bits?
status-b2g-v1.2: fixed → affected
(Assignee)

Comment 41

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #40)
> Correct.  Can we just have the marquee binding itself check whether script
> is enabled before doing the addEventListener bits?

Not explicitly, because the marquee binding isn't chrome privileged. We can detect this by doing a setTimeout on the contentWindow though, I think. 2 ways to do this:

(1) Dispatched compiled event handlers with setTimeout. This should do the right thing, but means that event handler dispatch has to round-trip through the event loop where it didn't before.

(1) In the <constructor> use setTimeout to sniff whether script is enabled, and store that as a global variable. This gets rid of the extra round-trip, but also can get messy if any event handlers are supposed to fire between the <constructor> and the completion of the first round-trip. Also, it makes the security issue here much more obvious.

I'm going to give (1) a shot unless bz has other ideas.
(Assignee)

Comment 42

5 years ago
This seems to do the trick.
Attachment #8430261 - Flags: review?(bzbarsky)
Comment on attachment 8430261 [details] [diff] [review]
Fuss around with marquee some more on esr24. v1

r=me, but only because I assume in practice no one relies on this stuff...  Doing this async is not how events are supposed to work.
Attachment #8430261 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 45

5 years ago
Kamil, can you try verifying again on esr24?
Flags: needinfo?(kamiljoz)
Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-30-00-05-02-mozilla-esr24/

- Ensured that the prompt appears when "javascript.enabled;true"
- Ensured that the prompt doesn't appear when "javascript.enabled;false"
- Checked against about:about, about:rights, about:home, about:newtab, about:memory, about:support, about:telemetry, about:crashes
- Used both "data:text/html,<marquee onstart=alert(1)></marquee>" and the test.html file that I've attached in comment # 35

Tested on the following OS's:
- Win 8.1, Ubuntu 13.10 and OSX 10.9.3

I did run into an issue while using NoScript 2.6.8.26, when you attempt to open test.html or use "data:text/html,<marquee onstart=alert(1)></marquee>" under about:home or in about:rights, the prompt message will appear either way. On some other about: pages such as about:newtab, about:crashes, about:about, the prompt will NOT be displayed which is expected. Used the following pref's:

- noscript.allowURLBarJS;true
- javascript.enabled;true

This only happens under ESR, when I tried it on m-c, aurora or beta, the prompt will never appear once NoScript 2.6.8.26 is installed.

Bobby, do we contact the addons owner in this case or? (not really sure as this isn't our issue)
status-firefox-esr24: fixed → verified
Flags: needinfo?(kamiljoz)

Comment 47

5 years ago
it run on firefox 29.0.1 for android to.
(Assignee)

Comment 48

5 years ago
Nice detective work!

It doesn't sound too dire - really, the biggest concern here is that thunderbird uses ESR24 and expects to have javascript disabled. I'll needinfo Giorgio to see if he has any ideas and thinks this is worth investigating.
Flags: needinfo?(g.maone)

Comment 49

5 years ago
ok :)
(In reply to Kamil Jozwiak [:kjozwiak] from comment #46)
> Went through verification using the following build:
> -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-30-00-05-02-
> mozilla-esr24/
> 

> I did run into an issue while using NoScript 2.6.8.26
[...]
> This only happens under ESR, when I tried it on m-c, aurora or beta, the
> prompt will never appear once NoScript 2.6.8.26 is installed.
> 
NoScript falls back to CAPS to block scripts in ESR, while it uses Cu.blockScriptsForGlobal() wherever available.
It seems your fix doesn't cover CAPS-based script blocking.
Whether you're willing to fix this or not, I'm gonna backport my original work-around to ESR ASAP :(
Flags: needinfo?(g.maone)
(Assignee)

Comment 51

5 years ago
(In reply to Giorgio Maone from comment #50)
> NoScript falls back to CAPS to block scripts in ESR, while it uses
> Cu.blockScriptsForGlobal() wherever available.
> It seems your fix doesn't cover CAPS-based script blocking.

This doesn't really make sense at first glance. This fix just routes marquee event handlers through setTimeout (as a WebIDL callback), which uses the same script blocking mechanism as everything else (calling into CAPS).

So this would be worth debugging. If the problem is limited to NoScript (and regular JS disabling works) and to esr24, this may not be a high enough priority for me to look at though.
Whiteboard: [embargo until esr24 eol]
@nico: could you please check 2.6.8.27rc3 from https://secure.informaction.com/download/betas/noscript-2.6.8.27rc3.xpi ?
Thanks!
(Assignee)

Comment 53

5 years ago
(In reply to Giorgio Maone from comment #52)
> @nico: could you please check 2.6.8.27rc3 from
> https://secure.informaction.com/download/betas/noscript-2.6.8.27rc3.xpi ?
> Thanks!

I think you want Kamil.
Flags: needinfo?(kamiljoz)
Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-06-02-00-05-02-mozilla-esr24/

Giorgio,

Yup, looks like that did the trick! I went through all the "about:" pages that are being listed under "about:about" using both methods and couldn't reproduce the issue mentioned in comment # 46. (once NoScript was installed and "noscript.allowURLBarJS;true", the prompts weren't appearing)

Test Cases:
- loading "data:text/html,<marquee%20onstart=alert(1)></marquee>" into each "about:" page while NoScript was installed
- loading the "test.html" file attached in comment # 35 into each "about:" page while NoScript was installed

Marking this as verified as it was targeted/tested against Mozilla32.
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
Keywords: verifyme
tracking-firefox-esr24: + → 30+
Whiteboard: [embargo until esr24 eol] → [embargo until esr24 eol][adv-main30-]
Whiteboard: [embargo until esr24 eol][adv-main30-] → [embargo until esr24 eol][adv-main30-][adv-esr24.6-]
status-seamonkey2.26: --- → affected
(Reporter)

Updated

4 years ago
Group: core-security
(Reporter)

Comment 57

3 years ago
Assigning CVE-2014-8644 to this bug.
Alias: CVE-2014-8644
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.