851586 - (CVE-2013-1716) URI_SAFE_FOR_UNTRUSTED_CONTENT is apparently ignored for custom about modules

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Mark Goodwin [:mgoodwin]]

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

Comment 1

[Mark Goodwin [:mgoodwin]]

See http://pastebin.mozilla.org/2220028 line 46 / 47 for more info

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Mark Goodwin [:mgoodwin]]

(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?).

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to :Gavin Sharp (use [e-mail] 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

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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?  ;)

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: untested WIP

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[[e-mail]]

(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.

Comment 11

[Daniel Veditz [:dveditz]]

"sec-high" on the theory that this is contributing to exploitable issues in 30-some add-ons and we need to deal with it.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(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?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This doesn't regress bug 786009 in my testing.

Comment 14

[[e-mail]]

(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 15

[[e-mail]]

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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[[e-mail]]

(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?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Gah.  That's terrible variable naming. ;)

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

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.

Comment 20

[Boris Zbarsky [: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.  :(

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch with comments addressed

[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.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to [e-mail] 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.

Comment 25

[Daniel Veditz [:dveditz]]

(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

Comment 26

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/248bfb2dcc99

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/248bfb2dcc99

Comment 28

[Daniel Veditz [:dveditz]]

wontfixing on ESR17 due to add-on compatibility worries.