A lot of our about: pages are privileged, and it seems like some of them don't need to be (e.g. about:certerror, about:blocked). I'm also unsure why we're not using the URI_SAFE_FOR_UNTRUSTED_CONTENT flag for unprivileged pages. I think it would be worthwhile to just make sure we're being as safe as can be here.
Not using URI_SAFE_FOR_UNTRUSTED_CONTENT for unprivileged pages is weird, but isn't a security issue (it just means none of the pages are linkable from untrusted content). Having pages that can be loaded (indirectly) by content be privileged *is* a security issue, though. about:home (via window.home()), about:certerror/blocked (via embedding a phishing/malware page, or page with a bad cert) are in this category.
Created attachment 647578 [details] [diff] [review] don't make about: pages privileged if they don't need to be I looked in blockedSite.xhtml, aboutCertError.xhtml, and aboutHome.xhtml, and none of these pages try to make any privileged calls. Unlike desktop, our about:/about:firefox pages have the update install stuff, but that's okay because external pages can't link to it.
Assignee: nobody → margaret.leibovic
Attachment #647578 - Flags: review?(mark.finkle)
Created attachment 647579 [details] [diff] [review] don't make about: pages privileged if they don't need to be Whoops wrong patch.
Comment on attachment 647579 [details] [diff] [review] don't make about: pages privileged if they don't need to be blockedSite and aboutCertError use "click" events handled in browser.js, which should still work fine with this change.
Attachment #647579 - Flags: review?(mark.finkle) → review+
Target Milestone: --- → Firefox 17
Comment on attachment 647579 [details] [diff] [review] don't make about: pages privileged if they don't need to be [Approval Request Comment] Bug caused by (feature/regressing bug #): n/a User impact if declined: possible security risk from content indirectly loading privileged about pages Testing completed (on m-c, etc.): just landed on inbound Risk to taking this patch (and alternatives if risky): low-risk, changing the privilege level of some pages to match desktop String or UUID changes made by this patch: n/a
https://hg.mozilla.org/mozilla-central/rev/89fb39735ee3 Is it possible to test this?
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(In reply to Ryan VanderMeulen from comment #7) > https://hg.mozilla.org/mozilla-central/rev/89fb39735ee3 > > Is it possible to test this? We don't have any automated test for this AFAIK. On desktop you can use the web console to try to execute privileged code while on an about: page (e.g. try running |Components.stack|), but this patch only affects mobile, so I'm not sure of the best way to test it. I think we can trust that setting this privileged flag does the right thing for these pages. Or otherwise we'd have a more serious problem! Gavin, do you have an idea of how to test this manually?
I don't think there's an easy way to test this manually in Fennec, short of exploiting one of our open security vulnerabilities :)
We might be able to create iframes containing the about pages, and use their windows as a sandbox context. On some chrome:// page, try this: // no error let iframe = document.createElement("iframe"); iframe.setAttribute("src", "about:config"); document.body.appendChild(iframe); let sandbox = Components.utils.Sandbox(iframe.contentWindow); Components.utils.evalInSandbox("Components.classes", sandbox); // Error: Permission denied for <moz-safe-about:blank> to get property XPCComponents.classes iframe = document.createElement("iframe"); iframe.setAttribute("src", "about:blank"); document.body.appendChild(iframe); let sandbox = Components.utils.Sandbox(iframe.contentWindow); Components.utils.evalInSandbox("Components.classes", sandbox);
Oh, yes - I was assuming a "without changing code" constraint. It's certainly possible to test that the page isn't privileged by changing its code (or injecting code into it programmatically, etc.), but I assumed Margaret had done that locally already.
tracking-fennec: ? → ---
status-firefox15: --- → fixed
status-firefox16: --- → fixed
You need to log in before you can comment on or make changes to this bug.