Closed
Bug 1005552
(CVE-2014-8644)
Opened 11 years ago
Closed 11 years ago
<marquee> event handlers run when JS disabled (e.g. in Thunderbird mail)
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla32
People
(Reporter: dveditz, Assigned: bholley)
Details
(Keywords: reporter-external, sec-high, Whiteboard: [embargo until esr24 eol][adv-main30-][adv-esr24.6-])
Attachments
(4 files)
1.78 KB,
patch
|
bzbarsky
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
approval-mozilla-esr24+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
2.67 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
52 bytes,
text/html
|
Details | |
995 bytes,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
+++ 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?
Comment 1•11 years ago
|
||
(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•11 years ago
|
||
(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.
Do you filed a new apps bug for Noscript? Or it's not important to filed it?
Reporter | ||
Comment 5•11 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.
hi, just a question when Is the bug is assigned? How long does it take?
Assignee | ||
Comment 8•11 years ago
|
||
I hope to get to this sometime this week or next.
Assignee: nobody → bobbyholley
Assignee | ||
Comment 12•11 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•11 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•11 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•11 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•11 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)
![]() |
||
Comment 18•11 years ago
|
||
Comment on attachment 8423279 [details] [diff] [review] Stop binding marquee event handlers. v1 r=me
Attachment #8423279 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 19•11 years ago
|
||
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•11 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•11 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 21•11 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?
Comment 22•11 years ago
|
||
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:
--- → +
Updated•11 years ago
|
Attachment #8423279 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/be9514701bd5
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/be9514701bd5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 25•11 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•10 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?
Updated•10 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•10 years ago
|
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+
Comment 28•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b4656a446d65 https://hg.mozilla.org/releases/mozilla-beta/rev/e09ccb39a409
Comment 29•10 years ago
|
||
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
Updated•10 years ago
|
Assignee | ||
Comment 31•10 years ago
|
||
Looks like we need to waive window before accessing the Function constructor, rather than after, because bug 933681 landed in FF28.
Assignee | ||
Comment 32•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr24/rev/e807ff18a7d7 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/b12b92450b2b
Comment 33•10 years ago
|
||
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!
Assignee | ||
Comment 34•10 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.
Comment 35•10 years ago
|
||
(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•10 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)
![]() |
||
Comment 37•10 years ago
|
||
> Boris, is that really the situation? That's correct. See also bug 409737 and bug 822213, which WebIDL event listeners fixed.
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 38•10 years ago
|
||
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•10 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.
![]() |
||
Comment 40•10 years ago
|
||
> 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?
Updated•10 years ago
|
Assignee | ||
Comment 41•10 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•10 years ago
|
||
This seems to do the trick.
Attachment #8430261 -
Flags: review?(bzbarsky)
![]() |
||
Comment 43•10 years ago
|
||
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+
Comment 44•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/96a1ab166184 https://hg.mozilla.org/releases/mozilla-esr24/rev/8806859eda66
Assignee | ||
Comment 45•10 years ago
|
||
Kamil, can you try verifying again on esr24?
Flags: needinfo?(kamiljoz)
Comment 46•10 years ago
|
||
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)
Flags: needinfo?(kamiljoz)
Assignee | ||
Comment 48•10 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 50•10 years ago
|
||
(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•10 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]
Comment 52•10 years ago
|
||
@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•10 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)
Comment 54•10 years ago
|
||
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.
Updated•10 years ago
|
Updated•10 years ago
|
Whiteboard: [embargo until esr24 eol] → [embargo until esr24 eol][adv-main30-]
Updated•10 years ago
|
Whiteboard: [embargo until esr24 eol][adv-main30-] → [embargo until esr24 eol][adv-main30-][adv-esr24.6-]
Updated•10 years ago
|
status-seamonkey2.26:
--- → affected
Reporter | ||
Updated•9 years ago
|
Group: core-security
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Updated•3 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•