Bug 851586 (CVE-2013-1716)

URI_SAFE_FOR_UNTRUSTED_CONTENT is apparently ignored for custom about modules

RESOLVED FIXED in Firefox 23

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mgoodwin, Assigned: Gavin)

Tracking

({addon-compat, csectype-priv-escalation, sec-moderate})

unspecified
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox20 wontfix, firefox21 affected, firefox22 affected, firefox23 fixed, firefox-esr17 wontfix, b2g18 disabled)

Details

(Whiteboard: [adv-main23+] embargo until ESR-17 EOL)

Attachments

(1 attachment, 3 obsolete attachments)

Issue:
The comments in nsAboutRedirector.cpp (and documentation) seem to imply that setting URI_SAFE_FOR_UNTRUSTED_CONTENT in flags causes the about page to drop privilege as well as being allowed in untrusted content. This is not the case; it seems the AboutModule must explicitly set a different principal on its channel.

I'm not sure if the bug is in the code or the docs but this seems wrong.
A quick look at available addons shows almost no-one is doing the check.

See also:
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsAboutRedirector.cpp#24
https://developer.mozilla.org/en-US/docs/Code_snippets/JS_XPCOM#XPCOMUtils_-_About_protocol_handler
See http://pastebin.mozilla.org/2220028 line 46 / 47 for more info
The comments in the about redirector are true by definition: they apply to the about redirector.

The comments in nsIAboutModule are pretty clear, I thought: it's a way for the _module_ to communicate to necko that the URI is safe to link to, and a claim that the module is guaranteeing such safety.  It's still up to the module to guarantee said safety.

The documentation at https://developer.mozilla.org/en-US/docs/Code_snippets/JS_XPCOM#XPCOMUtils_-_About_protocol_handler is flat-out insane.  :(

I suppose we could change the setup to force a non-system principal on the channel in nsAboutProtocolHandler::NewChannel if the URI claims to be safe.  If this won't break any of our internal stuff, I'm probably fine doing that.  Might break some addons, but I don't exactly see a better option here short of them fixing their code.
(In reply to Boris Zbarsky (:bz) from comment #2)
> Might break some addons, but I don't exactly see a better option here short
> of them fixing their code.

There are a few cases where people are setting URI_SAFE_FOR_UNTRUSTED_CONTENT for xul about pages; if we took the option of forcing a non-system principal I'm fairly sure we'll cause breakages here (am I right in thinking that non-system principals force the remoteXUL checks to fail?).
I suspect non-system principals do in fact mean no XUL.  As I see it, we have the following options:

1) Not making those about pages linkable from untrusted content (basically ignoring the
   URI_SAFE_FOR_UNTRUSTED_CONTENT flag).  This will break addons that use the flag
   properly.
2) Forcing everything with URI_SAFE_FOR_UNTRUSTED_CONTENT into a non-system principal.
3) Blacklisting the addons in question.
4) Leaving the likely security holes those addons introduce.

In any case, I've fixed the documentation to be saner.  :(
Flags: needinfo?(jorge)
(In reply to Boris Zbarsky (:bz) from comment #4)
> 2) Forcing everything with URI_SAFE_FOR_UNTRUSTED_CONTENT into a non-system
> principal.

This sounds like the best option to me.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> (In reply to Boris Zbarsky (:bz) from comment #4)
> > 2) Forcing everything with URI_SAFE_FOR_UNTRUSTED_CONTENT into a non-system
> > principal.
> 
> This sounds like the best option to me.

If we go this way, this means developers have a choice between using XUL and privileged code, or keeping URI_SAFE_FOR_UNTRUSTED_CONTENT, right? My guess is that most devs using this flag wouldn't have any problem removing it.

There are 37 matches on the MXR: https://mxr.mozilla.org/addons/search?string=URI_SAFE_FOR_UNTRUSTED_CONTENT
Flags: needinfo?(jorge)
> If we go this way, this means developers have a choice between using XUL and privileged
> code, or keeping URI_SAFE_FOR_UNTRUSTED_CONTENT, right?

Yes.

> This sounds like the best option to me.

OK.  You want to do it, or should I?  ;)
Flags: needinfo?(gavin.sharp)
Posted patch untested WIP (obsolete) — Splinter Review
I don't really have time at the moment to take this bug and write a test, but I think this should do it. Also, I don't quite understand the about:feeds-specific code in browser/components/about/AboutRedirector.cpp, so I didn't touch it, but maybe it can be simplified/removed.
Attachment #726624 - Flags: feedback?
Flags: needinfo?(gavin.sharp)
That actually changes the feed code, in fact: it'll now get a null principal instead of the codebase principal.

The feed code is the way it is to fix bug 786009 (which I think this patch would regress)...

Maybe we should only set the owner to null if the channel principal is system?
Attachment #726624 - Flags: feedback?
(In reply to Boris Zbarsky from comment #9)
> That actually changes the feed code, in fact: it'll now get a null principal
> instead of the codebase principal.
> 
> The feed code is the way it is to fix bug 786009 (which I think this patch
> would regress)...

XBL scopes may help here, see bug 846763.
"sec-high" on the theory that this is contributing to exploitable issues in 30-some add-ons and we need to deal with it.
Keywords: sec-high
(In reply to Boris Zbarsky (:bz) from comment #9)
> The feed code is the way it is to fix bug 786009 (which I think this patch
> would regress)...

Yeah, I never really understood that bug. Perhaps we can land its long-forgotten "alternative patch" to unstuck us here?
Posted patch patch (obsolete) — Splinter Review
This doesn't regress bug 786009 in my testing.
Attachment #731289 - Flags: feedback?(neil)
Attachment #731289 - Flags: feedback?(bzbarsky)
(In reply to comment #10)
> (In reply to Boris Zbarsky from comment #9)
> > That actually changes the feed code, in fact: it'll now get a null principal
> > instead of the codebase principal.
> > 
> > The feed code is the way it is to fix bug 786009 (which I think this patch
> > would regress)...
> 
> XBL scopes may help here, see bug 846763.

I did some testing and the problem with using XBL scopes is that they get turned off when you whitelist XUL, so if you visit a site with XUL whitelisted but have script disabled then the codebase principal change is the only thing left keeping the feed preview working.

(Not that XUL makes much sense with script disabled...)
Comment on attachment 731289 [details] [diff] [review]
patch

Presumably once this lands I can remove lines 46-47 from suite/common/src/nsAbout.js?

> Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> Components.utils.import("resource://gre/modules/debug.js");
>+Components.utils.import('resource://gre/modules/Services.jsm');
Ah yes, this was copied from sicking's patch, presumably created independently from attachment 674441 [details] [diff] [review], which would explain a) the different placement of the import of Services.jsm b) the single quotes.

>+        chromeChannel.owner =
>+          Services.scriptSecurityManager.getNoAppCodebasePrincipal(chromeURI);
So, I take it the reason you set this here is that the about protocol handler will now stomp on the owner that the about module tried to set?
Attachment #731289 - Flags: feedback?(neil) → feedback+
Comment on attachment 731289 [details] [diff] [review]
patch

This is changing the principal about:feeds gets, from the codebase for about:feeds to the codebase for the chrome:// URI, no?  I think we want to keep the old behavior....

Otherwise, looks fine.
Attachment #731289 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky from comment #16)
> This is changing the principal about:feeds gets, from the codebase for
> about:feeds to the codebase for the chrome:// URI, no?  I think we want to
> keep the old behavior....
No, it's about:feeds - I'm not sure why the variable is called chromeURI, presumably historically it was the actual chrome URI but the URI got changed for security reasons?
Gah.  That's terrible variable naming. ;)
Posted patch patch (obsolete) — Splinter Review
Changes since the last patch:
- added a comment to nsIAboutModule.idl
- added a test
- changed the confusing variable name in FeedConverter.js
- added an XXX comment in nsAboutProtocolHandler.cpp, would appreciate your thoughts on whether it's something to be worried about. We could try to address it by having the about: protocol handler enforce an originalURI on the created channel, but I'm not sure whether that would break too many legitimate uses.
Attachment #726624 - Attachment is obsolete: true
Attachment #731289 - Attachment is obsolete: true
Attachment #732529 - Flags: review?(bzbarsky)
Comment on attachment 732529 [details] [diff] [review]
patch

The comment in the idl isn't quite right: the principal will be based on the originalURI or the URI depending on the channel flags.

You're right about the XXX comment... I really wish I knew what extensions actually want out of their about: modules.  :(
Attachment #732529 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #20)
> The comment in the idl isn't quite right: the principal will be based on the
> originalURI or the URI depending on the channel flags.

I figured that distinction wasn't important since about: channels will likely never be LOAD_REPLACE, as I understand it. But I can tweak the comment.

> You're right about the XXX comment... I really wish I knew what extensions
> actually want out of their about: modules.  :(

I guess I'll just leave it in there as a precautionary note, then.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #21)
> > You're right about the XXX comment... I really wish I knew what extensions
> > actually want out of their about: modules.  :(
> 
> I guess I'll just leave it in there as a precautionary note, then.

At least all of the in-tree nsIAboutModule's set originalURI to the about: URI, so if things are cargo-culted it'll work out.
[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Not easily; there aren't any "easy exploits" here. This bug allows content pages to easily cause chrome-privileged pages to load when running some vulnerable extensions, which facilitates some privilege escalation attacks. I'm not sure a "sec-high" rating is justified TBH.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Somewhat. Firefox alone isn't vulnerable so it's a bit ambiguous.

> Which older supported branches are affected by this flaw?

All.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I suppose I could backport, but given the nature of the issue (belt-and-suspenders precaution) and some risk for compatibility issues, I don't think that's worth doing.

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely to affect Firefox or other m-c apps, but could negatively impact add-ons relying on this behavior.
Assignee: nobody → gavin.sharp
Attachment #732529 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #732565 - Flags: sec-approval?
(In reply to neil@parkwaycc.co.uk from comment #15)
> Presumably once this lands I can remove lines 46-47 from
> suite/common/src/nsAbout.js?

Yep.

> Ah yes, this was copied from sicking's patch, presumably created
> independently from attachment 674441 [details] [diff] [review], which would
> explain a) the different placement of the import of Services.jsm b) the
> single quotes.

Yep, fixed the quotes in the latest patch.

> >+        chromeChannel.owner =
> >+          Services.scriptSecurityManager.getNoAppCodebasePrincipal(chromeURI);
> So, I take it the reason you set this here is that the about protocol
> handler will now stomp on the owner that the about module tried to set?

Yes.
(In reply to :Gavin Sharp from comment #23)
> I'm not sure a "sec-high" rating is justified TBH.

That's fair:
 * A user would have to install a vulnerable add-on
 * attacker still needs an additional bug to inject code
   into the privileged content, either a general SOP bypass
   or an XSS bug in the add-on.

I'll lower this to sec-moderate.

> Unlikely to affect Firefox or other m-c apps, but could negatively
> impact add-ons relying on this behavior.

Worrying. We should reach out to the authors of the add-ons we know about and make sure they test on Nightly after this patch lands. Landing early in the cycle (as you are) is best for this kind of change.

sec-approval=dveditz
Attachment #732565 - Flags: sec-approval? → sec-approval+
Blocks: 857426
https://hg.mozilla.org/integration/mozilla-inbound/rev/248bfb2dcc99
Flags: in-testsuite+
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/248bfb2dcc99
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
wontfixing on ESR17 due to add-on compatibility worries.
Whiteboard: embargo until ESR-17 EOL
Whiteboard: embargo until ESR-17 EOL → [adv-main23+] embargo until ESR-17 EOL
Alias: CVE-2013-1716
Group: core-security
You need to log in before you can comment on or make changes to this bug.