343475 - Bug 340107 broke UniversalBrowserRead (and maybe other) permissions

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[James Ross]

The following check-in:

2006-06-26 17:56	dveditz%cruzio.com 	mozilla/caps/src/nsScriptSecurityManager.cpp 	1.307 	6/0  	bug 340107 save wasted cycles checking permissions if we're just going to deny access anyway. r=mrbkap, sr=sicking

broke granting pages UniversalBrowserRead [1]. They can still be granted it, but it fails read operations with a permissions error even though it should allow the reads. Using UniversalXPConnect works, but this is a regression in what permissions are needed.

I have backed out this check-in locally and confirmed that the permissions error goes away.

signed.applets.codebase_principal_support is false. This is a default configuration security-wise.

Note: the default QA Contact for Core: Security: CAPS is set as [e-mail], but this does not seem right. Please correct it if you know what it should be.

Note 2: this bug should block bug 340107 but Bugzilla sucks and wont let me.

[1] In my case, they are file: pages, but I suspect this applies to any that can otherwise ask for permissions.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #0)
> Note: the default QA Contact for Core: Security: CAPS is set as
> [e-mail], but this does not seem right. Please correct it if
> you know what it should be.

Fixing this is covered by bug 334211.

Comment 2

[James Ross]

OK, so despite the "testcase" on bug 340107 not actually working (ever), it seems the change in behavior so that UniversalBrowserRead does NOT grant universal read access was deliberate and is unlikely to be backtracked.

The specific check-in which caused part of the breakage (which I'll go into detail on in a sec) can be worked around (not very satisfactorily) by granting the code the UniversalXPConnect permission instead.

There is another problem, which feels related, but I can't work out exactly how. The following two errors are the result of something that changed with respect to security in the last week. I may be morphing this bug into their issue, as there is no way anyone will backtrack on the original issue.

[Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "Output%20Window.js Line: 120"]

[Exception... "Access to restricted URI denied"  code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)"  location: "Output%20Window.js Line: 399"]

Comment 3

[James Ross]

So what is it that has actually broken? ChatZilla's custom output windows, basically.

Normally, ChatZilla loads a chrome URI HTML document into its display area, and calls into it, passing various information, and generally engaging in a critical two-way communication with it to operate it.

It also supports loading a different URI which, for permissions purposes, should always be a file: URI. Obviously, the page's script has to ask for permissions and let the user grant them. This used to be UniversalBrowserRead, and it all used to work fine.

With the nsScriptSecurityManager change from bug 340107, it must request and be granted UniversalXPConnect. That is, although very annoying and not following common sense, not fatal. The problem is that the DOM is preventing the display window from inserting the message table. Line 120 of output%20window.js is:
    com.messages.chat.appendChild(view.messages);

com.messages.chat is a DOM element the page's own code has located. view is the object passed in to the code by ChatZilla, and view.messages is the HTML Table element created by ChatZilla for the display. That is when it throws the first of the exceptions I mentioned. I'm waiting for a debug build to figure out what's causing this.

The second exception I think is caused by a case that now needs permissions but did not used to.

Comment 4

[Daniel Veditz [:dveditz]]

(In reply to comment #2)
> OK, so despite the "testcase" on bug 340107 not actually working (ever),

What do you mean? The testcase in that bug demonstrates an exploit in Firefox 1.5.0.4 and "works" just fine. 

> seems the change in behavior so that UniversalBrowserRead does NOT grant
> universal read access was deliberate and is unlikely to be backtracked.

UBR was intended to let one web page access another. Chrome is not a web page, it's far more dangerous.

> The specific check-in which caused part of the breakage (which I'll go into
> detail on in a sec) can be worked around (not very satisfactorily) by granting
> the code the UniversalXPConnect permission instead.

Why is that not satisfactory? That is what's required to do Chrome things. We may have naively thought that a web page from chrome was just a web page, but it's not, it is in fact chrome. If you're worried that UniversalXPConnect grants too much power then pre-340107 you were actually granting just as much power with UniversalBrowserRead, you just didn't know it, and the recipient would have to work a little for it.

> The following two errors are the result of something that changed with
> respect to security in the last week.

That might be other things, see the 1.5.0.5 checkins for other security-related checkins in that timeframe: http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla1.8.0

> [Exception... "Security error"  code: "1000" nsresult: "0x805303e8
> (NS_ERROR_DOM_SECURITY_ERR)"  location: "Output%20Window.js Line: 120"]
> 
> [Exception... "Access to restricted URI denied"  code: "1012" nsresult:
> "0x805303f4 (NS_ERROR_DOM_BAD_URI)"  location: "Output%20Window.js Line: 399"]

where can I find the "Output Window.js" code you're using? I want the whole thing, not just the one line you quote in comment 3. Better yet would be instructions in how to get Chatzilla to load this file so I can debug it.

Is this with the chatzilla in the Mozilla cvs, or do I need to get a newer version of the source somewhere?

Comment 5

[James Ross]

(In reply to comment #4)
> (In reply to comment #2)
> > OK, so despite the "testcase" on bug 340107 not actually working (ever),
> 
> What do you mean? The testcase in that bug demonstrates an exploit in Firefox
> 1.5.0.4 and "works" just fine. 

Error: uncaught exception: A script from "https://bugzilla.mozilla.org" was denied UniversalBrowserRead privileges.

It requires the "insecure" codebase principals preference to be set, and does not demonstrate anything otherwise.

Comment 6

[James Ross]

This bug can't really be about bug 340107 any more, as that wont get anywhere, but I'll be damned if I know what it is about.

If you want to see the mess, get any relatively recent ChatZilla on a current trunk build, download all the files in http://twpol.dyndns.org/temp/cz_ow/ to a local directory, and do this command in ChatZilla [1]:

  /pref outputWindowURL file:///local/path/to/Output%20Window.html

It will prompt for UniversalXPConnect (used to be UBR, but we know what happened to that). Grant it. Watch the JavaScript^WError Console.

Note: you will not see the second exception I mentioned in comment 2, due to the fact it is caught and logged by ChatZilla itself. To see it, enable the dump pref and watch the real console.

Because the display is now broken, here is the command to reset it:

  /pref outputWindowURL -

[1] Don't need to be connected anywhere, and probably better if you aren't, as that causes much brokenness with the privilege prompt window.

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to comment #5)
> It requires the "insecure" codebase principals preference to be set, and does
> not demonstrate anything otherwise.

It demonstrates that if you grant UBR you get chrome privileges. How do you get UBR? The "codebase" pref (the lazy way) is perfectly fine for a PoC. In real life it would be signed. How the heck are you getting your code to ask for it? I suspect that's a security hole right there: if you're already chrome you wouldn't need to ask; if you're not you should need to be signed or enable the codebase pref yourself, apparently neither of which is true.

Do you have any better luck using a resource: output window? It'd have to be in the client install directory or below, but would be an interesting test since in some cases resource: is treated differently from file:.

Another approach would be to treat these chatzilla customizations as chrome things by "installing" them in your profile chrome or extensions directory. Then you could use a chrome: url and everything would be happy. It's another boilerplate file or two to create, but looking at the complexity that's already in your example that's nothing to anyone who could handle this in the first place. (The new package could be "flat" so you don't have to deal with .zip files)

Comment 8

[James Ross]

(In reply to comment #7)
> How the heck are you getting your code to ask for it?

Is that a genuine question? Did it not ask you without the codebase principals?

Comment 9

[Daniel Veditz [:dveditz]]

> > How the heck are you getting your code to ask for it?
> 
> Is that a genuine question? Did it not ask you without the codebase principals?

It is a genuine question, not directed at you. That should absolutely not ever be possible, and I'm afraid there is probably future breakage in your future as this seems like another security bug. Your unsigned code should fail without the codebase pref set just as the testcase in bug 340107 does.

We're not going to be able to "fix" this in 1.5.0.5

Comment 10

[Daniel Veditz [:dveditz]]

Another approach would be to read the specified output window code and then write the code into your own window (data: url maybe, document.write, several choices). Would break relative URLs for your subsidiary script/style files, so those would have to use full URLs or be included inline. That's why I preferred the chromelet approach, but though I should mention this.

Comment 11

[James Ross]

(In reply to comment #9)
> Your unsigned code should fail without the codebase pref set just as the
> testcase in bug 340107 does.

I feared as much. :(

(In reply to comment #10)
> Another approach would be to read the specified output window code and then
> write the code into your own window (data: url maybe, document.write, several
> choices). Would break relative URLs for your subsidiary script/style files, so
> those would have to use full URLs or be included inline. That's why I preferred
> the chromelet approach, but though I should mention this.

Yeah, I've pondered doing document.write from a fixed chrome page to insert the user's chosen page - the best part of this feature is that you can just set the URL and no more. What it really needs is some way for the outside code to grant it privileges on it behalf, but I don't think that's possible.

Comment 12

[Daniel Veditz [:dveditz]]

Did adding an enablePrivilege call fix your second exception? It looks like the patch for bug 340107 doesn't take UniversalXPConnect fully into account. The patch to nsScriptSecurityManager.cpp does, but nsContentUtils::CanCallerAccess doesn't, and it doesn't look like anywhere up the chain does either.

Comment 13

[Daniel Veditz [:dveditz]]

Attachment: Allow UniversalXPConnect in CanCallerAccess

This is what I mean. I don't think there will be a performance impact because before the fix for 340107 this case would have checked for UniversalBrowserRead/Write anyway (in addition to the CheckSameOrigin call).

This does not "fix" this bug, I'm not quite sure what this bug actually is.

Comment 14

[James Ross]

(In reply to comment #12)
> Did adding an enablePrivilege call fix your second exception?

No, it didn't.

Comment 15

[Daniel Veditz [:dveditz]]

You're probably hitting the thing this patch fixes then.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 228352 [details] [diff] [review]
Allow UniversalXPConnect in CanCallerAccess

>+      NS_ENSURE_SUCCESS(rv, PR_FALSE);
>+      return enabled;

alternativly: return NS_SUCCEEDED(rv) && enabled;

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 228352 [details] [diff] [review]
Allow UniversalXPConnect in CanCallerAccess

r=bzbarsky, with sicking's proposed change.

dveditz, the reason that James doesn't need the codebase pref set is that file:// URIs don't require that to request permissions...

James, view.messages is a DOM node Chatzilla created, right?  Does it help to import that node into the display document before passing to the display code?  With any luck, that would eliminate the need for expanded permissions here completely...  (Note that importNode returns the imported node; that's what you'd pass along.)

Comment 18

[James Ross]

(In reply to comment #17)
> James, view.messages is a DOM node Chatzilla created, right?

Yeah, the chrome JS creates it. I'll have a go at importing it.

It's been years since I wrote those privilege calls, but I don't think importing the node will stop some of the code needing some extra perms, though, as the code has to touch other objects that belong to the chrome code.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Note also that old versions of Gecko didn't support importNode, so you'll want a try/catch around that call and just use the node as-is if importNode is unsupported.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 228352 [details] [diff] [review]
Allow UniversalXPConnect in CanCallerAccess

Requesting branch approval with the style change suggested by sicking and bzbarsky.

(I was "when in Rome-ing" -- do you want me to fix the other similar places in that file?)

Comment 21

[Mike Connor [:mconnor]]

Comment on attachment 228352 [details] [diff] [review]
Allow UniversalXPConnect in CanCallerAccess

a=mconnor on behalf of drivers for checkin to 1.8.1 branch

Comment 22

[Daniel Veditz [:dveditz]]

Fixed on trunk and 1.8 branch

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 228352 [details] [diff] [review]
Allow UniversalXPConnect in CanCallerAccess

approved for 1.8.0 branch, a=dveditz for drivers

Comment 24

[Daniel Veditz [:dveditz]]

Fix checked into 1.8.0 branch

Comment 25

[Jay Patel [:jay]]

Adding [need testcase] in case anyone has a simplified test we can use to verify this fix.  If not, let me know if the steps in comment #6 are sufficient and exactly what to look for.

If anyone else has been able to reproduce this (James?), can you please verify this with the latest 1.8.0 and 1.8.1 nightlies?  Thanks!

Comment 26

[Daniel Veditz [:dveditz]]

Needed on old branches if bug 340107 is taken