User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0.1 Build ID: 20120713134347 Steps to reproduce: Reader mode copies the article into a document with chrome privileges via innerHTML without doing much sanitization. http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js?force=1#294

Confirmed locally. Clicking a link of <a href="javascript:alert(Components.stack);">link</a> shows we have chrome privileges in that page, and event handlers (for example, I tested onclick) don't get stripped from at least links. This feature does not appear to have requested or gotten a security review. Adding a chrome-privileged page with web/user-derived content is about the most dangerous thing we do and should not have skipped that step. about: urls should only be privileged as a last resort -- see the no end of troubles we've had with the feed page. It's not always the fault of the about: page either, it's just a convenient place to exploit any xpconnect wrapper problems we happen to have. innerHTML is a screaming red flag of danger. We should be using the common sanitizer we already have and not writing a new one. They're trouble and hard to get right (see again the feed page).

[mid-aired with myself in the middle of a comment. Interesting] ...The only safe way is to use the same parser used by the browser itself. Look into nsIParserUtils, or ask the content folks what to do if that's not suitable (particularly hsivonen). Parsing by regexp is another red flag. There are zillions of ways to encode HTML to evade parsers, see all the XSS even on sites that are trying to be careful. The right sanitizer would have stripped out the event handlers. I shouldn't have been able to add a javascript: link. Whitelist http/https and strip links that use any other scheme. As long as the page is privileged there must be absolutely no embeds. Even if we trust youtube and vimeo to re-encode all videos and remove scripts, unless it's an https: URL we can't trust that the content is actually from those sites. Even if it is https: the content may not be safe in the presence of a MITM proxy that many people have to use to get access to the internet. It's one thing to trust your employer MITM not to mess with your sites (you can avoid the sensitive ones), it's another to have to trust it not to compromise your browser/machine. Consider creating a document (sub frame?) with an explicit nsNullPricipal and then adding the content DOM to that.

Here's one approach - this uses a chrome page with a null principal inside of an iframe as suggested in comment 2. I don't know how to make an unprivileged chrome URL other than by specifying it in AboutRedirector.js, so I did that by creating an about:readercontent entry. There are a couple of problems with this patch as-is: 1) Scrolling is a bit broken for some reason. When scrolling from the top or bottom of the page, scrolling abruptly stops after a small distance, requiring a second scroll. 2) The correct font isn't used. Since aboutReaderContent.html cannot refer to any chrome:// URLs, I haven't figured out a way to fix this yet.

Also, I just realized that we'll have to resize the iframe whenever the font size or margins change.

Weird that unprivileged about URIs can't access chrome:// URLs. Bunch of about URIs on Desktop seem to use it https://mxr.mozilla.org/mozilla-central/source/browser/components/certerror/content/aboutCertError.xhtml Is there anything on Mobile that would cause this not to work? also, an easier trick *might* be to make the main (parent) frame unprivileged, and create a 0px iframe that is privileged to do the coding part for you by postmessaging each other. Alternatively, CSS tricks should make the unprivileged iframe completely 'seamless' http://stackoverflow.com/a/5632609

(In reply to Devdatta Akhawe from comment #6) > Weird that unprivileged about URIs can't access chrome:// URLs. Bunch of > about URIs on Desktop seem to use it > https://mxr.mozilla.org/mozilla-central/source/browser/components/certerror/ > content/aboutCertError.xhtml > > Is there anything on Mobile that would cause this not to work? I noticed that too. I'm not sure what the difference is, but trying to include the same stylesheet in aboutReaderContent.html gives this error: "Content at about:readercontent may not load or link to chrome://browser/skin/aboutCertError.css". > also, an easier trick *might* be to make the main (parent) frame > unprivileged, and create a 0px iframe that is privileged to do the coding > part for you by postmessaging each other. One problem with this approach might be the reader toolbar, which is a div in the page containing buttons to perform certain actions (share, change font size, etc.). With the reader content being in its own frame, the reader toolbar can stay in the privileged parent page without any issues. If the unprivileged page is the parent frame, I think we'd have to put the toolbar in that same page, which means the content could access it. > Alternatively, CSS tricks should make the unprivileged iframe completely > 'seamless' http://stackoverflow.com/a/5632609 Yeah, I think that's essentially what I did (no border or padding on the frame). The problem is having to resize the frame when its contents change, but I don't think that part can be fixed with CSS. Originally, I just made the iframe style="position: absolute; width: 100%; height: 100%" so that the frame took up the entire viewport. This made the frame scroll rather than its parent. But this had several undesirable side effects on mobile: scrolling was noticeably choppier, there was no "bounce" overscroll effect, and the scrollbar doesn't appear (since on mobile, the top-level document is the only frame with a scrollbar).

> One problem with this approach might be the reader toolbar, which is a div > in the page containing buttons to perform certain actions (share, change > font size, etc.). With the reader content being in its own frame, the reader > toolbar can stay in the privileged parent page without any issues. If the > unprivileged page is the parent frame, I think we'd have to put the toolbar > in that same page, which means the content could access it. Are the buttons the only place where the privileges are needed? The about pages on Desktop have all the click handlers for about:certerror / about:blocked etc. in browser.js, so that those pages could run unprivileged.

Listens for the MozScrolledAreaChanged event to resize the iframe whenever the scroll area changes.

(In reply to Devdatta Akhawe from comment #8) > Are the buttons the only place where the privileges are needed? The about > pages on Desktop have all the click handlers for about:certerror / > about:blocked etc. in browser.js, so that those pages could run unprivileged. Yeah, the buttons and the readability parsing itself are the only things that need chrome privileges. I did want to inject these things from browser.js, but the lack of chrome:// URL support was a problem since there was no obvious way to include the many chrome:// images we have in aboutReader.css.

According to https://developer.mozilla.org/en/DOM/element.scrollHeight, scrollHeight does not include margins. We had margins at the top and bottom of aboutReaderContent.html, so iframe height was being set to a value slightly smaller than its full height. This explains why I was seeing broken scrolling behavior: the first scroll just scrolled the iframe the remaining overflow amount, and the second scroll was what actually scrolled the page. Here's a build with these changes: http://dl.dropbox.com/u/35559547/fennec-readability-iframe.apk Now I think the only thing left is fixing the font.

Comment on attachment 648247 [details] [diff] [review] Move reader document to unprivileged iframe, v1.2 This looks good to me from a high level. We should get Dan to look at the iframe, loading a non-privileged about: page. Lucas can review for the reader functionality side too.

This should fix the security issues by sandboxing reader content.

Splits the stylesheet for each frame, makes chrome://browser/skin contentaccessible so the content frame can use it, and moves the @font-face declaration to content.css so it can be accessed in the unprivileged frame.

Automatically resizes the iframe whenever we receive a MozScrollAreaChanged or DOMSubtreeModified event. Apparently, MozScrollAreaChanged wasn't enough since it only fired when increasing the font size; decreasing the font size kept the page the same height (and the event didn't fire).

Makes about:reader hidden on about:about and other small fixes.

I've updated the build (http://dl.dropbox.com/u/35559547/fennec-readability-iframe.apk) with these latest changes. AFAICT, it's functionally/aesthetically identical to our existing implementation.

Comment on attachment 648544 [details] [diff] [review] Part 1: Split about:reader into privileged and unprivileged frames Review of attachment 648544 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to see a new cleaned up patch before giving r+ ::: mobile/android/chrome/content/aboutReader.html @@ +24,5 @@ > </div> > </li> > </ul> > > + <script type="application/javascript;version=1.8" src="chrome://browser/content/aboutReader.js"></script> Unrelated change, but ok. ::: mobile/android/chrome/content/aboutReader.js @@ +33,5 @@ > > this._article = null; > > dump("Feching toolbar, header and content notes from about:reader"); > + this._frame = document.getElementById("frame"); Given that you only use this._frame pretty much get its inner document, maybe you should just do: this._frameDocument = document.getElementById("frame").contentDocument; and use it everywhere. @@ +43,5 @@ > > this._scrollOffset = window.pageYOffset; > > + this._frame.contentDocument.addEventListener("touchstart", this, false); > + this._frame.contentDocument.addEventListener("click", this, false); The event listener used to be added to the body of the document, not the document itself. Is that intentional? @@ +159,5 @@ > if (this._marginSize === newMarginSize) > return; > > this._marginSize = Math.max(5, Math.min(25, newMarginSize)); > + this._frame.contentDocument.body.style.marginLeft = this._marginSize + "%"; Assign body style to a temporary variable, then use use on both assignment here. @@ +202,3 @@ > > this._colorScheme = newColorScheme; > bodyClasses.add(this._colorScheme); Do you still need to set the color scheme on bodyClasses? ::: mobile/android/chrome/content/aboutReaderContent.html @@ +2,5 @@ > +<html> > + > +<head> > + <!-- make links loads outside of frame --> > + <base target="_parent" /> Is that what UX wants? Just wondering.

Comment on attachment 648561 [details] [diff] [review] Part 2: Split about:reader stylesheet for each frame Review of attachment 648561 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: mobile/android/themes/core/content.css @@ +11,5 @@ > +@font-face { > + font-family: 'OpenSansRegular'; > + src: url('chrome://browser/skin/fonts/opensans-regular.ttf') format('truetype'); > + font-weight: normal; > + font-style: normal; App-wide definition of the font? Nice.

Comment on attachment 648567 [details] [diff] [review] Part 4: Miscellaneous about:reader fixes Review of attachment 648567 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/components/AboutRedirector.js @@ +61,5 @@ > }, > reader: { > uri: "chrome://browser/content/aboutReader.html", > + privileged: true, > + hide: true What does this hide accomplish exactly?

(In reply to Lucas Rocha (:lucasr) from comment #20) > Comment on attachment 648567 [details] [diff] [review] > Part 4: Miscellaneous about:reader fixes > > Review of attachment 648567 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/components/AboutRedirector.js > @@ +61,5 @@ > > }, > > reader: { > > uri: "chrome://browser/content/aboutReader.html", > > + privileged: true, > > + hide: true > > What does this hide accomplish exactly? Makes it so about:reader doesn't show up in about:about. In general, we don't show about: sites on that page if they require a query string to work correctly.

(In reply to Lucas Rocha (:lucasr) from comment #18) > ::: mobile/android/chrome/content/aboutReader.html > @@ +24,5 @@ > > </div> > > </li> > > </ul> > > > > + <script type="application/javascript;version=1.8" src="chrome://browser/content/aboutReader.js"></script> > > Unrelated change, but ok. Oops, I'll move this to patch 4 (or just remove it, not really a big deal). > > ::: mobile/android/chrome/content/aboutReader.js > @@ +33,5 @@ > > > > this._article = null; > > > > dump("Feching toolbar, header and content notes from about:reader"); > > + this._frame = document.getElementById("frame"); > > Given that you only use this._frame pretty much get its inner document, > maybe you should just do: > > this._frameDocument = document.getElementById("frame").contentDocument; > > and use it everywhere. We still need the frame itself to resize it in patch 3. If you prefer, we could have both _frame and _frameDocument. > > @@ +43,5 @@ > > > > this._scrollOffset = window.pageYOffset; > > > > + this._frame.contentDocument.addEventListener("touchstart", this, false); > > + this._frame.contentDocument.addEventListener("click", this, false); > > The event listener used to be added to the body of the document, not the > document itself. Is that intentional? Yeah. I noticed that increasing the margins made it non-clickable if you clicked outside the body. I figured we wanted these to cover the whole page. > > @@ +202,3 @@ > > > > this._colorScheme = newColorScheme; > > bodyClasses.add(this._colorScheme); > > Do you still need to set the color scheme on bodyClasses? Yeah, the parent frame still contains the background (and the content frame's background is invisible). > > ::: mobile/android/chrome/content/aboutReaderContent.html > @@ +2,5 @@ > > +<html> > > + > > +<head> > > + <!-- make links loads outside of frame --> > > + <base target="_parent" /> > > Is that what UX wants? Just wondering. I didn't ask, but I think we need this. Otherwise, links will open inside of the readability content frame, which we definitely don't want. If the link points to a standard web page, that would mean they'd be browsing inside of about:reader, which would be weird.

Comment on attachment 648773 [details] [diff] [review] Part 1: Split about:reader into privileged and unprivileged frames, v2 Review of attachment 648773 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/chrome/content/aboutReader.js @@ +33,5 @@ > > this._article = null; > > dump("Feching toolbar, header and content notes from about:reader"); > + this._frame = document.getElementById("frame"); Ok, then do a "let frameDocument = this._frame.contentDocument" here and use it everywhere in the init() method.

Comment on attachment 648564 [details] [diff] [review] Part 3: Automatically resize reader frame with contents Seems ok

Comment on attachment 648561 [details] [diff] [review] Part 2: Split about:reader stylesheet for each frame Lucas is enough for me. Looks right.

Comment on attachment 648561 [details] [diff] [review] Part 2: Split about:reader stylesheet for each frame Review of attachment 648561 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/themes/core/jar.mn @@ +4,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > > chrome.jar: > +% skin browser classic/1.0 %skin/ contentaccessible=yes Does this work? According to comments in the manifest parser, contentaccessible is only used on "content". This is bolstered by the fact that ManifestLocale() and ManifestSkin() in nsChromeRegistryChrome.cpp don't do anything with the contentaccessible param. Maybe you're inheriting the content setting from /browser/base/jar.mn, or does Fennec have it's own copy in /mobile ? But then you're unlikely to have added it if it wasn't broken without it. Is something mysterious and broken in the chrome registry/parser?

(In reply to Daniel Veditz [:dveditz] from comment #26) > Comment on attachment 648561 [details] [diff] [review] > Part 2: Split about:reader stylesheet for each frame > > Does this work? According to comments in the manifest parser, > contentaccessible is only used on "content". This is bolstered by the fact > that ManifestLocale() and ManifestSkin() in nsChromeRegistryChrome.cpp don't > do anything with the contentaccessible param. Maybe you're inheriting the > content setting from /browser/base/jar.mn, or does Fennec have it's own copy > in /mobile ? Thanks - that shouldn't have made it into the patch. I think that was left over from when I tried various things to make the font import work.

Or at least I thought it was unintentional until I saw my comment 14. Either way, though, I removed that line and it still works fine.

Comment on attachment 648773 [details] [diff] [review] Part 1: Split about:reader into privileged and unprivileged frames, v2 Review of attachment 648773 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/aboutReader.js @@ +35,5 @@ > > dump("Feching toolbar, header and content notes from about:reader"); > + this._frame = document.getElementById("frame"); > + this._titleElement = this._frame.contentDocument.getElementById("reader-header"); > + this._contentElement = this._frame.contentDocument.getElementById("reader-content"); I'm still uneasy about this direct manipulation of unprivileged content from a chrome context. I'd prefer something more indirect, like running scripts using EvalInSandbox or sending the data to the child frame using postMessage(). Our wrappers are supposed to save us, but if they fail this could be wide open. On the other hand, at this point in reader there is no web content in the unprivileged frame, and if the readability scripts end before any content gets to run then we are probably OK. Need to talk to the wrapper gurus (mrbkap, bholley, jst).

CC'ing mrbkap, bholley, and jst - any problems with the approach in this patch?

(In reply to Brian Nicholson (:bnicholson) from comment #30) > CC'ing mrbkap, bholley, and jst - any problems with the approach in this > patch? Can you clarify the approach, the concern, and the potential alternatives? In general, it's pretty safe to rely on Xray wrappers to do their job when accessing native properties (as long as you don't turn them off with .wrappedJSObject / XPCNativeWrapper.unwrap). You get a clean view of the DOM, though content is free to manipulate the DOM in confusing ways. And as a general principle, defense-in-depth is a good thing. :-)

(In reply to Bobby Holley (:bholley) from comment #31) > (In reply to Brian Nicholson (:bnicholson) from comment #30) > > CC'ing mrbkap, bholley, and jst - any problems with the approach in this > > patch? > > Can you clarify the approach, the concern, and the potential alternatives? On mobile, we have a feature called Reader Mode that performs sanitization on the document to remove any content not relevant to the article being viewed; see [1] for a screenshot. The sanitization is performed using a custom JS script [2] that's run inside of a web worker. Currently, we simply take the innerHTML of the parsed result, and set the innerHTML to a div in about:reader to that result. about:reader is a privileged chrome page, so this introduces a gaping security hole by allowing JS from the web page to run in a privileged context. The approach here creates an unprivileged iframe on about:reader to contain the parsed document. This is done by creating another about: page - about:readercontent - which is unprivileged (I don't know of another way to include an unprivileged chrome document inside of a privileged one). The iframe's document is manipulated directly from the parent document, e.g.: + this._frame = document.getElementById("frame"); + this._contentElement = this._frame.contentDocument.getElementById("reader-content"); + let bodyStyle = this._frame.contentDocument.body.style; + this._frame.contentDocument.addEventListener("click", this, false); From comment 29, I think Dan's concern is that accessing these properties/methods is potentially dangerous if the wrappers somehow fail since we are directly accessing and calling them from a privileged context. As for alternatives, we could do something like postMessage() to the frame to avoid these direct manipulations, as Dan mentioned. [1] https://farm8.staticflickr.com/7017/6560941471_5153493ffa_b.jpg [2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js

(In reply to Brian Nicholson (:bnicholson) from comment #32) > From comment 29, I think Dan's concern is that accessing these > properties/methods is potentially dangerous if the wrappers somehow fail > since we are directly accessing and calling them from a privileged context. This is totally fine. Our story here is really good. Even if you turn off Xray vision (with .wrappedJSObject), the content code still runs with its own privileges. The principal of a function is computed from the compartment in which it is defined, so chrome can freely call into content JS. > The approach here creates an unprivileged iframe on about:reader to contain > the parsed document. This is done by creating another about: page - > about:readercontent - which is unprivileged (I don't know of another way to > include an unprivileged chrome document inside of a privileged one). Do you mean to say that you're loading untrusted content into a type=chrome iframe? Because that's...not good.

(In reply to Bobby Holley (:bholley) from comment #33) > (In reply to Brian Nicholson (:bnicholson) from comment #32) > > > The approach here creates an unprivileged iframe on about:reader to contain > > the parsed document. This is done by creating another about: page - > > about:readercontent - which is unprivileged (I don't know of another way to > > include an unprivileged chrome document inside of a privileged one). > > Do you mean to say that you're loading untrusted content into a type=chrome > iframe? Because that's...not good. It's an HTML iframe (both the parent and child frames are HTML documents), so there's no type attribute set. Since the subdocument is from a chrome URL, it (and the untrusted content) would typically have chrome privileges, but by adding the page to aboutRedirector.js [1] with "privileged: false", these privileges are lost. [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js

On a related note, I tried using a XUL iframe like this: <iframe xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" type="content" src="chrome://..."> </iframe> But the subdocument still had chrome privileges. Any idea why?

(In reply to Brian Nicholson (:bnicholson) from comment #34) > It's an HTML iframe (both the parent and child frames are HTML documents), > so there's no type attribute set. Since the subdocument is from a chrome > URL, it (and the untrusted content) would typically have chrome privileges, > but by adding the page to aboutRedirector.js [1] with "privileged: false", > these privileges are lost. Ah, interesting. I hadn't seen this hack before. I hope we have tests to make sure the pages really are unprivileged? That seems like just the sort of thing I'd break while cleaning up CAPS. ;-) (In reply to Brian Nicholson (:bnicholson) from comment #35) > On a related note, I tried using a XUL iframe like this: > > <iframe > xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > type="content" > src="chrome://..."> > </iframe> > > But the subdocument still had chrome privileges. Any idea why? Because the principal of a page is determined by the document URI, and modulo hacks like the above, a chrome:// URI is converted into nsSystemPrincipal. The docshell type is used for different things.

(In reply to Bobby Holley (:bholley) from comment #36) > (In reply to Brian Nicholson (:bnicholson) from comment #34) > Ah, interesting. I hadn't seen this hack before. I hope we have tests to > make sure the pages really are unprivileged? That seems like just the sort > of thing I'd break while cleaning up CAPS. ;-) AFAIK, we do not - I filed bug 783566 to take care of this.

Here is a test case for the chrome xss using Dan's comment STR 1. Visit about:reader?url=<path to test case> 2. Scroll down and click on the link that says "Click" 3. You should get an alert popup Sorry for how hacky the testcase is. I'm unfamiliar with the exact format of reader mode articles.

Comment on attachment 648773 [details] [diff] [review] Part 1: Split about:reader into privileged and unprivileged frames, v2 Let's land on Nightly ASAP

I discussed with mfinkle and mgoodwin just now (and Gavin yesterday as well). I think the right thing to do here is : 1. land this fix asap - it's sec-critical and was apparently found via code inspection and isn't a hard issue to find. there's a manual test case which is blocked by the fix in the bug according to bnicholson. 2. uplift the fix to Aurora - same rationale as #1 3. schedule a proper sec review for this feature - mgoodwin is going to file a bug for this - and we should make sure dveditz is involved. 4. try to harden the feature more fully in the near future, this could involve using iframe sandbox or perhaps other approaches, but discussing and having a considered approach 5. we should really do the testing in bug 783566 to make sure things are the way we think they are with the approach we're using overall (adding privileged:false in aboutRedirector.js) This is assuming that Dan's constraints in comment 29 : "On the other hand, at this point in reader there is no web content in the unprivileged frame, and if the readability scripts end before any content gets to run then we are probably OK." are true. I believe the first part is at least (links open outside the frame), can someone confirm the second part is also ? FWIW, I'm not so worried about holes in our wrapper implementation since we rely on those all over and that would be a bigger issue than this bug.

(In reply to Ian Melven :imelven from comment #40) > This is assuming that Dan's constraints in comment 29 : "On the other hand, > at this point in reader there is no web content in the unprivileged frame, > and if the readability scripts end before any content gets to run then we > are probably OK." are true. I believe the first part is at least (links open > outside the frame), can someone confirm the second part is also ? I confirm that we don't inject any content until the readability script has completely finished. But I think it is possible for web content to be run in the unprivileged frame. We set 'base target="_parent"' in the content frame HTML, so any scripts that run on the content page could override this. Won't the wrappers save us here, though?

http://hg.mozilla.org/integration/mozilla-inbound/rev/be2dc5ca1acb http://hg.mozilla.org/integration/mozilla-inbound/rev/73f018c8fbea http://hg.mozilla.org/integration/mozilla-inbound/rev/63f9b653db9d http://hg.mozilla.org/integration/mozilla-inbound/rev/254a6ae6cf2a

Backed out for native android R3 failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/535465bc2005

Apparently, the R3 failures are caused by patch 2: https://tbpl.mozilla.org/?tree=Try&rev=292a4aa6537b The only parts of the patch not specific to about:reader are the font declarations. It's not obvious why they would be causing any failures, but one possibility suggested by mbrubeck is that they are affecting the load timings of the page. I pushed these patches to try with the font declarations removed from content.css to verify that they are, in fact, the cause of the failure: https://tbpl.mozilla.org/?tree=Try&rev=11806d2e2211.

Here's a workaround that avoids adding the fonts to content.css. Not an ideal solution, but it appears to work, and hopefully avoids the reftest failures. I'll talk to dbaron to see if we can find a proper fix, but this is hopefully sufficient for the Monday merge deadline. Try: https://tbpl.mozilla.org/?tree=Try&rev=8adc3cd871a6

Alternate fix that uses the @font-face declarations for only about:readercontent. Try: https://tbpl.mozilla.org/?tree=Try&rev=7e541c40db6a

Removed summary for landing

Removed summary for landing

Removed summary for landing

Removed summary for landing

Looks like this passed try, so we should go with this one. Marking old one as obsolete.

Comment on attachment 654955 [details] [diff] [review] Part 5: Use @document condition for @font-face declarations, v2 I'll just land this now; mfinkle said he'd give r+ over IRC.

http://hg.mozilla.org/integration/mozilla-inbound/rev/950379788e33 http://hg.mozilla.org/integration/mozilla-inbound/rev/e106316f41e4 http://hg.mozilla.org/integration/mozilla-inbound/rev/726f7f71fd45 http://hg.mozilla.org/integration/mozilla-inbound/rev/ee35e4c5e26c http://hg.mozilla.org/integration/mozilla-inbound/rev/3559149341e0

Comment on attachment 654955 [details] [diff] [review] Part 5: Use @document condition for @font-face declarations, v2 Yes, r+ to this patch

Comment on attachment 654950 [details] [diff] [review] Part 1: Split about:reader into privileged and unprivileged frames, v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): new feature User impact if declined: security risk Testing completed (on m-c, etc.): green tests Risk to taking this patch (and alternatives if risky): medium String or UUID changes made by this patch: none

Comment on attachment 654952 [details] [diff] [review] Part 2: Split about:reader stylesheet for each frame, v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): new feature User impact if declined: security risk Testing completed (on m-c, etc.): green tests Risk to taking this patch (and alternatives if risky): medium String or UUID changes made by this patch: none

The patches that landed in m-c to fix this issue caused unintended side-effects: scrolling inside an iframe is not as smooth as before and the new code caused a few regressions (image positioning, unwanted margins, scrolling bugs, etc). I'd like to propose a different approach to fix this bug which is both simpler and less intrusive. It involves simply turning about:reader into an unprivileged page and handling interaction with the page in browser.js. For instance, this is the same approach used for about:certerror page. The only major issue I found is a bug that prevents us from using history.pushState() on the page directly from chrome. The patch has a workaround. Let me know if you know of a better way to do it.

(In reply to Brian Nicholson (:bnicholson) from comment #53) > http://hg.mozilla.org/integration/mozilla-inbound/rev/950379788e33 > http://hg.mozilla.org/integration/mozilla-inbound/rev/e106316f41e4 > http://hg.mozilla.org/integration/mozilla-inbound/rev/726f7f71fd45 > http://hg.mozilla.org/integration/mozilla-inbound/rev/ee35e4c5e26c > http://hg.mozilla.org/integration/mozilla-inbound/rev/3559149341e0 https://hg.mozilla.org/mozilla-central/rev/950379788e33 https://hg.mozilla.org/mozilla-central/rev/e106316f41e4 https://hg.mozilla.org/mozilla-central/rev/726f7f71fd45 https://hg.mozilla.org/mozilla-central/rev/ee35e4c5e26c https://hg.mozilla.org/mozilla-central/rev/3559149341e0

Comment on attachment 655239 [details] [diff] [review] Turn about:reader into an unprivileged page Can you put this into a new bug? Could even be a public bug now that the security issue is fixed.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #59) > Comment on attachment 655239 [details] [diff] [review] > Turn about:reader into an unprivileged page > > Can you put this into a new bug? Could even be a public bug now that the > security issue is fixed. Is it considered "fixed" even if aurora (currently shipping reader mode) doesn't have this fix yet?

Here's a new version of the patch that protects chrome against untrusted events coming from reader UI.

Comment on attachment 655461 [details] [diff] [review] Turn about:reader into an unprivileged page Would like another set of eyes on this, and some security peeps too.

The patch seems to be doing the "right stuff" to protect chrome code from bad interactions with content code. I see event.isTrusted checks for example. I would like to know if the workaround used from bug 682296 is considered safe though.

Comment on attachment 655461 [details] [diff] [review] Turn about:reader into an unprivileged page As discussed on IRC, one issue with this patch is the page's ability to manipulate the reader toolbar, so we may want to use Gecko's sanitization utilities before adding the content to the page. But this at least fixes the main issue - the page getting chrome privileges - while also avoiding the regressions from the iframe-based patch.

(In reply to Brian Nicholson (:bnicholson) from comment #64) > Comment on attachment 655461 [details] [diff] [review] > Turn about:reader into an unprivileged page > > As discussed on IRC, one issue with this patch is the page's ability to > manipulate the reader toolbar, so we may want to use Gecko's sanitization > utilities before adding the content to the page. Filed bug 785992.

(In reply to Mark Finkle (:mfinkle) from comment #63) > The patch seems to be doing the "right stuff" to protect chrome code from > bad interactions with content code. I see event.isTrusted checks for example. > > I would like to know if the workaround used from bug 682296 is considered > safe though. re attachment 655461 [details] [diff] [review] + this._pushStateScript = doc.createElement('script'); + this._pushStateScript.type = "text/javascript"; + this._pushStateScript.innerHTML = 'history.pushState({ dropdown: 1 }, document.title);'; that looks ok to me, it's sticking a static string that can't be influenced by content in the history.

(In reply to Brian Nicholson (:bnicholson) from comment #64) > Comment on attachment 655461 [details] [diff] [review] > Turn about:reader into an unprivileged page > > As discussed on IRC, one issue with this patch is the page's ability to > manipulate the reader toolbar, so we may want to use Gecko's sanitization > utilities before adding the content to the page. But this at least fixes the > main issue - the page getting chrome privileges - while also avoiding the > regressions from the iframe-based patch. right, i read through this and the crucial bit looks like : reader: { uri: "chrome://browser/content/aboutReader.html", - privileged: true + privileged: false, + hide: true which means no parts of Reader mode have chrome privs at all. This sounds like a solid approach. However, even if about:reader doesn't have chrome privs, can it still call into other chrome: things via URL injection etc ? i would hope not, but this is something we should look at as bug 783566 says. as i said in the comments for 785922 you might want to look into iframe sandbox if you want to block running scripts, plugins, etc. I did see that one reason you wanted to avoid using an iframe is due to regressions though, so those might need addressing first :/ I noticed the issue described by comment 64 while reviewing the patch, I suppose that a page could XSS itself (or hosted content could get your cookie on the hosting site)? but also see the concerns above about calling into other chrome: URL's. This is less than ideal but sanitization is difficult to get right as has been discussed. Also I will state again that this feature really needs a proper secreview and most likely, some security testing. mgoodwin has filed bug 785077 for this.

Are the original patches still desired for Beta 16, given the regressions we've found? Or can we wait for the final solution?

From my perspective, we should fix the chrome XSS on beta. I'm OK with either patch although I think Lucas' no chrome privilege patch is a nice way of reducing attack surface. Then the Security Assurance team can get a secreview scheduled, we can discuss approaches towards the overall solution, and maybe they can do some security testing as well of this.

(In reply to Ian Melven :imelven from comment #69) > From my perspective, we should fix the chrome XSS on beta. I'm OK with > either patch although I think Lucas' no chrome privilege patch is a nice way > of reducing attack surface. > > Then the Security Assurance team can get a secreview scheduled, we can > discuss approaches towards the overall solution, and maybe they can do some > security testing as well of this. Agreed. Tuesday, Lucas can backout the previous iframe-based patch from nightly and aurora, and land the new approach. Once things look OK, we can also uplift to beta. Sound OK?

(In reply to Mark Finkle (:mfinkle) from comment #70) > Agreed. Tuesday, Lucas can backout the previous iframe-based patch from > nightly and aurora, and land the new approach. Once things look OK, we can > also uplift to beta. > > Sound OK? This sounds like a good approach.

(In reply to Mark Finkle (:mfinkle) from comment #70) > Agreed. Tuesday, Lucas can backout the previous iframe-based patch from > nightly and aurora, and land the new approach. Once things look OK, we can > also uplift to beta. > > Sound OK? Yep, but let's just get the final solution into beta 2 and skip resolving this for beta 1. That'll give us plenty of time to converge.

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/145fedfb046a (revert iframe patches) https://hg.mozilla.org/integration/mozilla-inbound/rev/c447eff920db

https://hg.mozilla.org/mozilla-central/rev/145fedfb046a https://hg.mozilla.org/mozilla-central/rev/c447eff920db

Merge conflict confused me (feedback page landed) and I unintentionally removed the contentaccessible=yes flag from chrome's jar.nm. Reader is currently broken in nightly because of that. This patch fixes it.

Comment on attachment 655461 [details] [diff] [review] Turn about:reader into an unprivileged page [Approval Request Comment] User impact if declined: We're posting untrusted content on a privileged chrome page. We can't do that. Testing completed (on m-c, etc.): Landed in m-c, no regressions found. Risk to taking this patch (and alternatives if risky): Relatively low as this patch is just moving calling points to allow about:reader to become an unprivileged page. String or UUID changes made by this patch: None

Comment on attachment 656411 [details] [diff] [review] Add contentaccessible=yes flag [Approval Request Comment] User impact if declined: This patch fixes reader mode in Nightly. This should be folded with the main patch. Testing completed (on m-c, etc.): Landed in m-c, no regressions found. Risk to taking this patch (and alternatives if risky): Low, just an obvious fix on the main patch. String or UUID changes made by this patch: None

Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/3c09109243fe https://hg.mozilla.org/releases/mozilla-aurora/rev/813ce1e0bc77 Pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/60d7baaf6ba2

Fixing flags.