1005552 - (CVE-2014-8644) <marquee> event handlers run when JS disabled (e.g. in Thunderbird mail)

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Veditz [:dveditz]]

+++ 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?

Comment 1

[Giorgio Maone [:ma1]]

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

Comment 2

[Giorgio Maone [:ma1]]

(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

[nico]

Do you filed a new apps bug for Noscript? Or it's not important to filed it?

Comment 5

[Daniel Veditz [:dveditz]]

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

[nico]

OK. Thx

Comment 7

[nico]

hi,
just a question
when Is the bug is assigned?
How long does it take?

Comment 8

[Bobby Holley (:bholley)]

I hope to get to this sometime this week or next.

Comment 9

[nico]

ok thx you

Comment 10

[nico]

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

Comment 11

[nico]

assigned, sorry

Comment 12

[Bobby Holley (:bholley)]

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

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

Comment 13

[nico]

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.

Comment 14

[Bobby Holley (:bholley)]

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

Comment 15

[Bobby Holley (:bholley)]

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.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Stop binding marquee event handlers. v1

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.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Bobby Holley (:bholley)]

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

Comment 21

[Bobby Holley (:bholley)]

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.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. Please create and nominate patches for Aurora, Beta, and ESR24 after trunk is green.

Comment 23

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be9514701bd5

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/be9514701bd5

Comment 25

[nico]

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

Comment 26

[Bobby Holley (:bholley)]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b4656a446d65
https://hg.mozilla.org/releases/mozilla-beta/rev/e09ccb39a409

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/e09ccb39a409

Comment 31

[Bobby Holley (:bholley)]

Looks like we need to waive window before accessing the Function constructor, rather than after, because bug 933681 landed in FF28.

Comment 32

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-esr24/rev/e807ff18a7d7
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/b12b92450b2b

Comment 33

[Kamil Jozwiak [:kjozwiak]]

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!

Comment 34

[Bobby Holley (:bholley)]

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

Comment 35

[Kamil Jozwiak [:kjozwiak]]

Attachment: Using file rather than data:URI

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

Comment 36

[Bobby Holley (:bholley)]

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.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Boris, is that really the situation? 

That's correct.  See also bug 409737 and bug 822213, which WebIDL event listeners fixed.

Comment 38

[Boris Zbarsky [:bzbarsky]]

What we did do is do checks for script-enabled before _compiling_ event listeners.  Could we do that for marquee on ESR24?

Comment 39

[Bobby Holley (:bholley)]

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

Comment 40

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 41

[Bobby Holley (:bholley)]

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

Comment 42

[Bobby Holley (:bholley)]

Attachment: Fuss around with marquee some more on esr24. v1

This seems to do the trick.

Comment 43

[Boris Zbarsky [: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.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/96a1ab166184
https://hg.mozilla.org/releases/mozilla-esr24/rev/8806859eda66

Comment 45

[Bobby Holley (:bholley)]

Kamil, can you try verifying again on esr24?

Comment 46

[Kamil Jozwiak [:kjozwiak]]

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)

Comment 47

[nico]

it run on firefox 29.0.1 for android to.

Comment 48

[Bobby Holley (:bholley)]

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.

Comment 49

[nico]

ok :)

Comment 50

[Giorgio Maone [:ma1]]

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

Comment 51

[Bobby Holley (:bholley)]

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

Comment 52

[Giorgio Maone [:ma1]]

@nico: could you please check 2.6.8.27rc3 from https://secure.informaction.com/download/betas/noscript-2.6.8.27rc3.xpi ?
Thanks!

Comment 53

[Bobby Holley (:bholley)]

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

Comment 54

[Kamil Jozwiak [:kjozwiak]]

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.

Comment 55

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-release/rev/fd0adbe7ef4a

Comment 57

[Daniel Veditz [:dveditz]]

Assigning CVE-2014-8644 to this bug.