349985 - about: needs privileges to xpconnect

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P4)

Description

[Axel Hecht [:Pike]]

Looking at 
http://lxr.mozilla.org/mozilla/source/netwerk/protocol/about/src/nsAboutRedirector.cpp#148
and
http://lxr.mozilla.org/mozilla/source/caps/src/nsScriptSecurityManager.cpp#1554
it seems that plain about: does not have the permissions set to at least unconditionally do what we expect it to from bug 348076.

All those code places only support about:foo urls with explicit modules, but not about:.

The impl of about: is in http://lxr.mozilla.org/mozilla1.8.0/source/xpfe/appshell/src/nsAbout.cpp

Comment 1

[Reed Loden [:reed]]

Will this fix the "Error: uncaught exception: Permission denied to get property UnnamedClass.classes" I'm getting every time I go to about:?

Comment 2

[Mike Connor [:mconnor]]

not going to block on this.  about: is a hidden feature, not user-facing at all, and all this fixes is the release notes link, which is accesible from browser chrome.

Comment 3

[Reed Loden [:reed]]

(In reply to comment #2)
> not going to block on this.  about: is a hidden feature, not user-facing at
> all, and all this fixes is the release notes link, which is accessible from
> browser chrome.

Uh, the release notes link isn't the only thing this breaks. The user agent is no longer displayed at the bottom of the page. This causes a regression of bug 345993 making testers not happy, as it's difficult to get the full 10-digit build id otherwise.

Comment 4

[Adam Guthrie]

As discussed on IRC we can do a couple of things:

1. Just give about.xhtml chrome privileges like we do for SeaMonkey. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/about/src/nsAboutRedirector.cpp&rev=1.22.2.3&mark=71#55

2. Gracefully handle the error with a try/catch block so we actually get to the code that gets the buildid. 

I see two as a hack, since we're just wallpapering over something that will never work, but I don't know the security implications of doing option 1.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 1. Just give about.xhtml chrome privileges like we do for SeaMonkey.

We don't give it chrome privileges for Seamonkey.

> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/about/src/nsAboutRedirector.cpp&rev=1.22.2.3&mark=71#55

That's "about:about", not "about:".  Try them -- they're different.

"about:" is allowed to execute script (per nsAbout::GetURIFlags).  There seems to be some confusion in comment 0 about which code belongs with which about: module...

ccing Jesse, if the question is security implications.  I'm assuming everyone involved has read the bug that initially took away chrome privileges from varions about: URLs, right?  If not, please go read it so we're all on the same page.

Comment 6

[Daniel Veditz [:dveditz]]

(In reply to comment #3)
> This causes a regression of bug 345993 making testers not happy,
> as it's difficult to get the full 10-digit build id otherwise.

Bookmark "javascript:alert(navigator.buildID);void(0);" and give it a keyword, back to being easy. Anyone who can't handle that doesn't need the full 10 digits anyway.

Comment 7

[Steffen Wilberg]

The build identifier part could be fixed by adding a try...catch around the url formatter call, see comment 4. JS works just fine in the about: page.

But the url formatter call needed for the release notes url fails with "Error: uncaught exception: Permission denied to get property UnnamedClass.classes".
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/about.xhtml&rev=1.8.8.10&mark=75-78#74

Comment 8

[Mike Connor [:mconnor]]

This is not a blocker.  If the problem is "about:" doesn't work quite right, file a Firefox bug on that.  But even that isn't a blocker, as this is not user-facing.  We should catch the error, but that's about it.  Given that the release notes are linked in the Help menu, there's no need to do that here as well, so we might as well remove it, instead of making a change with unknown security implications.

Comment 9

[Zach Lipton [:zach]]

*** Bug 350849 has been marked as a duplicate of this bug. ***

Comment 10

[Robert Kaiser]

Could we get a solution for trunk here, so that the new toolkit-based SeaMonkey generation can use this about: page and still have release notes linked there?

Our only problem is that urlformatter cannot be called from about: at the moment, but it's needed for getting the relnotes URL.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So what exactly is needed here?  Would passing the release notes url as an argument in the document URI or something like that work?

Comment 12

[Robert Kaiser]

Anything that gets the document to know about the urlformatted relnotes URl should be fine.
As we don't have any external content in about: and it is actually a chrome content doc, I guess it would be safe to assign chrome privs to that doc though and the code that's already there would work fine. This would be easy to set for a nsAboutRedirector doc, but for about: itself I'm lost to where to do it.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> but for about: itself I'm lost to where to do it.

Remove the SetOwner call in nsAbout::NewChannel (and whatever other code can be removed if you do that).  Please get that sort of change OKed by dveditz before working on it, though.

Comment 14

[Tony Mechelynck [:tonymec]]

(In reply to comment #6)
> (In reply to comment #3)
> > This causes a regression of bug 345993 making testers not happy,
> > as it's difficult to get the full 10-digit build id otherwise.
> 
> Bookmark "javascript:alert(navigator.buildID);void(0);" and give it a keyword,
> back to being easy. Anyone who can't handle that doesn't need the full 10
> digits anyway.
> 

Alternately, the "MR Tech Local Install" extension includes (among others) a Tools menu item to copy the useragent string and/or the 10-digit build ID to the clipboard, and a context menu item to insert them (together) into the current textarea. This is what the latter does for me:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.2pre) Gecko/20070110 BonEcho/2.0.0.2pre - Build ID: 2007011004

The pseudo-useragent-string on my about: page does include the 10-digit build ID (unlike the useragent string displayed on the Help -> About popup and seen, for instance, by Bugzilla when I create a new bug). The "release notes" link, OTOH, is not clickable.

Comment 15

[Robert Kaiser]

(In reply to comment #13)
> Please get that sort of change OKed by dveditz before
> working on it, though.

IIRC, dveditz wasn't really happy with that on IRC back when I asked him about this.

We're repeatedly running into class of internal pages that need a certain small set of components that are not critical per se but are unavailable to content by themselves and might loading stuff that comes from chrome or profile, like stringbundles or the urlformatter (which read from a pref). We were running into the stringbundle problem in about:plugins (and it unfortunately runs with chrome privs since that time), we're seeing this again here with about:, I vaguely remember we might have had some discussion in that direction with the MailNews start page. Is there some way we could give those pages the resources they need without compromising security? Maybe there might be a way to execute a script with chrome privs loading the values the pages need, dropping those privs and leaving just the values in some form for in-page script handling before anything in the page is executed? Or are they safe enough that they can run with chrome privs, being loaded with about: or chrome: URLs?

It would be nice if we'd come to a good conclusion there.

Comment 16

[Jesse Ruderman]

I like bz's idea from bug 349943 comment 16 to not load things like about:plugins in the web browser content area at all.

Comment 17

[Robert Kaiser]

As long as it _looks_ like a web browser content area to the user (and bz's comment there seems to imply that to me), this sounds like a really good idea to me.

Comment 18

[Jesse Ruderman]

Why do you want it to look like web browser content area?  It's browser UI, really.

Comment 19

[Robert Kaiser]

about:plugins needs to be real HTML, as some plugins like injecting HTML there, sometimes even with an <embed> calling the plugin itself and offering their plugin settings UI through that (AFAIK, some Flash versions at least used to do that).

The other reason is that people who are using those pages are used to them looking like web pages, at least them being shown right inside the browser window. After all, they are often enough calling them through typing a (special) URL in the urlbar.

Comment 20

[Boris Zbarsky [:bzbarsky]]

We can have "real HTML" in a chrome document too.  But really, we can also just use a type="chrome" tab in the tabbrowser, with a bit of work on the browser/tabbrowser bindings.

Comment 21

[Robert Kaiser]

Yes, this sounds good as well, I guess.

Comment 22

[Robert Kaiser]

Is anyone working on this? We'd really like to have some kind of solution here for the Gecko 1.9 cycle...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Well.  First a solution needs to be decided on.  Then someone needs to implement it.  I'm certainly not working on this....

For what it's worth, one could also do this entirely in the app UI: catch when about: loads and fill in stuff from browser.js or eqivalent.  It's a hack, but it might happen for 1.9, unlike serious core changes as described in comment 15.  Some of the other proposed changes here are a little safer, so might happen for 1.9.  But we're in somewhat-serious code freeze now, so time is running out very fast on any core changes.

Comment 24

[Tony Mechelynck [:tonymec]]

(In reply to comment #18)
> Why do you want it to look like web browser content area?  It's browser UI,
> really.
> 
(In reply to comment #19)
> about:plugins needs to be real HTML, as some plugins like injecting HTML there,
> sometimes even with an <embed> calling the plugin itself and offering their
> plugin settings UI through that (AFAIK, some Flash versions at least used to do
> that).
> 
> The other reason is that people who are using those pages are used to them
> looking like web pages, at least them being shown right inside the browser
> window. After all, they are often enough calling them through typing a
> (special) URL in the urlbar.
> 
An additional reason is that some people (including me) have CSS rules in userContent.css specifically meant for about: family URLs. If these become "browser UI" (to be styled exclusively by userChrome.css) instead of "browser content" (handled by userContent.css) it's pennies get you pounds that you'll get showered by "regression" bugs about CSS rules in userContent having stopped functioning in about:<something> pages.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Frankly, I don't think that particular concern should affect our security decisions.

Comment 26

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041001 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

No error.


Then:
bug 415498 got fixed :->


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041102 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Moving bug from (1.8.1) branch to (1.9) trunk,
even if it's likely to miss that release too :-/

Steps:
1. Load <about:>.
1r.
{{
Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]
}}
(I'm assuming this is this bug too ?? At least, same steps !)

If you had opened JS.D and tracing errors/exceptions:
{{
Error ``Permission denied to get property XPCComponents.classes'' [x-] in file ``about:'', line 79, character 0.

Exception ``Error: Permission denied to get property XPCComponents.classes'' thrown from function (null)() in <about:> line 79.

[e] message = [string] "Permission denied to get property XPCComponents.classes"
}}
That's
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/content/about.xhtml&rev=1.26&mark=79-80,89#68>

Comment 27

[Serge Gautherie (:sgautherie)]

(In reply to comment #26)
> [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041102
> SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041107 Minefield/3.0pre] (nightly) (W2Ksp4)

Firefox too. (confirming.)

> {{
> Error: [Exception... "'Component is not available' when calling method:
> [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111
> (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]
> }}
> (I'm assuming this is this bug too ?? At least, same steps !)

Well, once upon a time, I had filed bug 395228 about this exception...

Comment 28

[Mike Schroepfer]

Given the age of this bug and the fact that it is in 1.8.1 we are not going to block FF3.  Doesn't mean we wouldn't take a fix in a dot or future release...

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

XUL Application that run on XULRunner are given the version of XULRunner, not
their own version in about:

We could use nsIXULAppInfo to get the app's version and replace it in the XHTML
- except about: would need privileges to do that.

see bug 428765

Comment 30

[Robert Kaiser]

(In reply to comment #26)
> No error.

Sure, we catch the errors and therefore 1) don't display the release notes link (which is a regression over 1.8.x/XPFE for SeaMonkey) as well as 2) don't change the link on the logo from mozilla.org to whatever the app has set.
For both of those, we'd need exactly this bug fixed (or do a suite-specific nasty workaround from within SeaMonkey UI code).

Comment 31

[timeless]

one very nice feature from my perspective of about:plugins is that i can take them and use them in random browsers: http://viper.haque.net/~timeless/blog/23/plugs.html

I don't like the idea of breaking this....

the nsIHandlerService nonsense is entirely unrelated.

Axel: personally I think that this is your regression and you should back out your changes. changes based on an assumption that wasn't valid which result in bustage are not something we should keep. Further, changing security at this stage is not good practice.

Note that exposing stringbundles today to content of any kind is sg:not-acceptable. If you need to know why, contact me and I'll explain.

Comment 32

[Axel Hecht [:Pike]]

It's not my regresssion, and it's not going to get backed out.

Right now, about: just doesn't link to release notes. Which isn't a drama to me either.

If someone decides to WONTFIX this, that's fine, I guess that'd call for a follow-up bug to do the code part in C++, in http://mxr.mozilla.org/mozilla/source/xpfe/appshell/src/nsAbout.cpp#53, and to pass the values as query params. Something like chrome://global/content/about.xhtml?vendor=mozilla.org&releasenotes=http://www.mozilla.org/projects/firefox/3.0pre/releasenotes/. To be blunt, not my bug either.

Comment 33

[Robert Kaiser]

(In reply to comment #32)
> Right now, about: just doesn't link to release notes. Which isn't a drama to me
> either.

Not for you, but for SeaMonkey, which uses this page for the "About SeaMonkey" menu item. We may need to hack in some browser-chrome-side page manipulation for our next release, which is ugly, to say it mildly.

Comment 34

[Mike Hommey [:glandium]]

By the way is there a reason why about: is not handled by nsAboutRedirector from toolkit nowadays ?

Comment 35

[Mike Hommey [:glandium]]

The rationale to drop chrome privileges from about:, in bug #88087, was security of document.write()s happening there. The current about.xhtml doesn't contain any document.write() and uses the DOM API instead. I fail to see how the current about.xhtml code could be a security risk.

Comment 36

[Mike Hommey [:glandium]]

Attachment: possible patch

What about this ? It both moves about: handling to nsAboutRedirector and allows the ReleaseNotes script to work.

Comment 37

[Boris Zbarsky [:bzbarsky]]

So why does this change things?  Basically because we're ditching the SetOwner() call that we used to have in nsAbout::NewChannel?  That seems like the only substantive change...

I'm not sure how happy I am with relying on the empty string thing in the redirector.  Seems like it would be too easy to cause confusion by injecting null bytes, no?

Comment 38

[Boris Zbarsky [:bzbarsky]]

Still waiting for an answer to my questions in comment 37.

Comment 39

[Mike Hommey [:glandium]]

(In reply to comment #37)
> So why does this change things?  Basically because we're ditching the
> SetOwner() call that we used to have in nsAbout::NewChannel?  That seems like
> the only substantive change...

Yes, it also removes code duplication.
 
> I'm not sure how happy I am with relying on the empty string thing in the
> redirector.  Seems like it would be too easy to cause confusion by injecting
> null bytes, no?

Do you mean something like <a href="about:\0config"> ?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Maybe I wasn't clear.  My first question was which part of this patch is actually needed to fix this bug.

And yes, like \0. Or %00 or whatever escaping is needed to get a null byte in there.

The real question is whether it's possible to somehow create a page that links to about: and which the CheckLoadURI check allows and which will therefore link to a page with chrome privileges.

Comment 41

[Mike Hommey [:glandium]]

Attachment: test

I verified that you can't fool it with attached file, at least.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Are you sure the null byte is actually present in the DOM in this once the parser is done with this input stream?  Looks to me like it gets replaced by U+FFFD in this case, as expected.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 321987 [details] [diff] [review]
possible patch

r- pending answer to the question in comment 42

Comment 44

[Robert Kaiser]

Mike, any chance to move forward here?

Comment 45

[Reed Loden [:reed]]

Can we get somebody to help Mike out here? He got it started... can somebody help finish it up?

Comment 46

[Johnny Stenback (:jst)]

Not blocking on this, but if a safe working patch emerges we'll consider it...

Comment 47

[Mike Hommey [:glandium]]

(In reply to comment #42)
> Are you sure the null byte is actually present in the DOM in this once the
> parser is done with this input stream?  Looks to me like it gets replaced by
> U+FFFD in this case, as expected.

That's the whole point, how is a null byte supposed to get there ?

Comment 48

[Robert Kaiser]

From what I'm seeing in http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsAboutRedirector.cpp#66, about:about, about:plugins, about:config, about:crashes, and about:memory all run with chrome privs.
Additionally, the Firefox-specific http://mxr.mozilla.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp#69 defines about:privatebrowsing, about:sessionrestore and about:support as having chrome privs.

From the comparing what they are doing with what about: is doing, I can't see why it might be considered a security problem to have about: run with chrome privs as well, actually.

I know that there are ideas for a reasonable solution here, but no progress on them in years, and that's tiring.

I'd propose to just give about: chrome privs for now and file an own bug for the ideas to solve this the "right" way.

Comment 50

[Steffen Wilberg]

So if the only concern is whether the empty string in this part

+++ b/docshell/base/nsAboutRedirector.cpp
 static RedirEntry kRedirMap[] = {
+    { "", "chrome://global/content/about.xhtml",
+      nsIAboutModule::ALLOW_SCRIPT },

can be exploited, we could just keep nsAbout.cpp and remove the GetCodebasePrincipal call, i.e. backout this:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsAbout.cpp&branch=&root=/cvsroot&subdir=mozilla/xpfe/appshell/src&command=DIFF_FRAMESET&rev1=1.11&rev2=1.12 (bug 88087).

That would be the minimal patch to fix this bug, and I'd be happy to write that.

Comment 51

[Johnny Stenback (:jst)]

Not blocking on this, but if a safe patch appears we'll certainly consider it.

Comment 52

[Steffen Wilberg]

Attachment: remove the SetOwner call

This is the minimal patch I described in comment 50.

In about.xhtml, I removed the try/catch, which isn't needed anymore. The rest is adding a couple of comments, and reindentation.

In nsAbout::NewChannel, do I need the
  NS_ENSURE_SUCCESS(rv, rv);
before returning rv?

Comment 53

[Steffen Wilberg]

Attachment: qdiff -w

Same as before, but without whitespace changes.

Done by using https://developer.mozilla.org/en/Mercurial_FAQ#How_do_I.c2.a0get_a_diff_-w.3f and doing "hg diffw -r -2".

Comment 54

[Mike Hommey [:glandium]]

Very frankly, rereading netwerk/protocol/about/src/nsAboutProtocolHandler.cpp, I fail to see what is supposedly unsafe with my original patch.

Comment 55

[Mike Hommey [:glandium]]

Comment on attachment 321987 [details] [diff] [review]
possible patch

To be more specific than comment 54, both the contractid lookup and the table lookup in nsAboutRedirectory end up using NS_GetAboutModuleName from the original uri for the about:* requests. I see no way both lookups will resolve to something different, and I see no way something could be faking an empty string that wouldn't already get the about: contractid with the current code.
As such, I'm re-requesting review for this patch.

Comment 56

[Boris Zbarsky [:bzbarsky]]

> That's the whole point, how is a null byte supposed to get there ?

 "about:%00config", for example.

Or do the set in JS instead of relying on the HTML parser's handling of null bytes.

I'm not saying the original patch _is_ unsafe.  I was just asking whether you had tested the various attack scenarios to make sure it's safe.

Seeing as I never got a straight answer I just had to spend several hours reading through this code carefully and doing some testing myself to make sure things are ok.  Thanks.

Comment 57

[Boris Zbarsky [:bzbarsky]]

> So if the only concern is whether the empty string in this part

Yes, that was the concern.

Comment 58

[Robert Kaiser]

(In reply to comment #56)
>  "about:%00config", for example.

We seem to catch that in some other way as my SeaMonkey trunk nightly says that URL is invalid and cannot be loaded.

Comment 59

[Boris Zbarsky [:bzbarsky]]

And the reason the null thing ends up being not a concern is that nsSimpleURI::SetSpec will escape null to %00 while NS_GetAboutModule doesn't unescape.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 321987 [details] [diff] [review]
possible patch

r=bzbarsky

Comment 61

[Steffen Wilberg]

Comment on attachment 321987 [details] [diff] [review]
possible patch

Does this need super-review?

Comment 62

[Steffen Wilberg]

Comment on attachment 321987 [details] [diff] [review]
possible patch

Per http://www.mozilla.org/hacking/reviewers.html, all changes involving security require super-review.

Comment 63

[Steffen Wilberg]

Comment on attachment 321987 [details] [diff] [review]
possible patch

The requirement to get super-review for "all changes involving security" has been removed today, so this is ready to land.

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 321987 [details] [diff] [review]
possible patch

That means that the requirement that all security _fixes_ get sr is gone.  In any case, reviewers can request sr at their discretion and given my r+ here this bug still needs an sr from dveditz or maybe mrbkap or jst.

Comment 65

[Steffen Wilberg]

Attachment: "possible patch", unbitrotted

This is Mike's "possible patch", updated to trunk, and with the comments around the release notes link in about.xhtml removed, which is the point of this bug.

Comment 66

[Robert Kaiser]

dveditz, ping?

Comment 67

[Steffen Wilberg]

Attachment: unbitrotted again

Adapted to bug 568691 (data-driven-compreg).

Comment 68

[[e-mail]]

There may be another way, which involves no security changes.

Instead of having the about: page obtain the release notes and vendor URLs, simply define new about: pages for those links, and use those.

The backend code that fulfils those new about: pages just needs to redirect the channel to the URL as looked up in preferences.

Comment 69

[Mike Hommey [:glandium]]

(In reply to comment #68)
> There may be another way, which involves no security changes.
> 
> Instead of having the about: page obtain the release notes and vendor URLs,
> simply define new about: pages for those links, and use those.

Been there, done that. I don't remember the details but iirc that still requires this security change.

Comment 70

[Daniel Veditz [:dveditz]]

Comment on attachment 455761 [details] [diff] [review]
unbitrotted again

sr=dveditz

Comment 71

[Mike Hommey [:glandium]]

Comment on attachment 455761 [details] [diff] [review]
unbitrotted again

This patch finally allows the release notes link in about: to work.

Comment 72

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/01c8100757c4