340107 - UniversalBrowserRead should not permit access to chrome documents

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[shutdown]

By the nature of the UniversalBrowserRead privilege, scripts with
this privilege enabled can access any document in anywhere, and
be able to take control of the user's activity on the web site
(by reading login cookies -- the "sentisive browser data").

The problem is "anywhere" mentioned in the above includes the chrome.
Once the script with UBR enabled have access to a chrome JavaScript
object, it can do more than to "read sensitive browser data" by
elevating its privilege to full chrome level (see bug 313684).

Comment 1

[Daniel Veditz [:dveditz]]

Guessing mrbkap given link to 313684, but maybe this is in the DOM.

Do you have a testcase/PoC?

Comment 2

[Daniel Veditz [:dveditz]]

One spot that might be trouble is nsContentUtils::CanCallerAccess(). If the caller isn't chrome then it shouldn't be able to access a privileged DOMNode regardless of UniversalBrowserRead permission; accessing a chrome node should require UniversalXPConnect permission. nsContentUtils::IsTrustedForRead() might lead to the same issues but for now that's only used safely in nsGlobalWindow.
... or maybe xpconnect's use of CanAccess, or...

I guess we've got to catch this centrally in CAPS. In nsScriptSecurityManager::CheckPropertyAccessImpl chrome privileged callers bail out early so if we're calling CheckSameOriginDOMProp() we know we're in a non-privileged doc. Can we deny access if the object principal is chrome?

CheckPropertyAccessImpl might want to check for UniversalXPConnect privs early.. looks like you can't access a "noAccess" property even if you have that explicit privilege (signed code).

Comment 3

[Boris Zbarsky [:bzbarsky]]

So would it make sense to have nsIPrincipal::Subsumes or some such (to differentiate read and write) do the work here, have all the callers call that, and implement it once, correctly, in CAPS?

> Can we deny access if the object principal is chrome?

I think so.  Possibly even if the caller has UniversalXPConnect...  Should be pretty easy to do this part.

> looks like you can't access a "noAccess" property

At the moment you can, since we'll fall into the CheckXPCPermissions() code.  We should really clean this code up to be less confusing, or possibly document it better.  Or both.  We keep getting confused about the exact control flow, which is _so_ not good.

Comment 4

[Jay Patel [:jay]]

We need a fix for this.  Blake might have a lot on his plate right now, so if bz can take this one, that'll be great!  bz?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'm not sure I'm going to have time to really work on this until after the onsite.  :(  I _might_ have time before, but not to the extent that I can guarantee working on this...

Comment 6

[Boris Zbarsky [:bzbarsky]]

And anyone working on this would need some testcases to verify that their stuff works...

Comment 7

[shutdown]

Attachment: testcase

requested thing.

Comment 8

[Daniel Veditz [:dveditz]]

Attachment: Patch v1

There are two independent parts of this patch. The first change in nsScriptSecurityManager::CheckSameOriginDOMProp() is sufficient to stop the testcase in this bug. Punting on chrome objects is safe because access by the system principal is approved long before this point, and explicit UniversalXPConnect access is checked for and granted in CheckPropertyAccessImpl, the only caller of this routine.

You can verify the latter point by modifying the testcase to request UniversalXPConnect instead of UniversalBrowserRead -- it should work before and after the patch.

The second part of the patch covers nsContentUtils::CanCallerAccess which has the same problem, but will be harder to verify because we don't have a testcase to demonstrate the problem. It should be abusable by something like the XUL testcase in bug 313566 though I had no luck modifying it to actually do so.

It may also be more work to verify it doesn't regress anything since it's called in more places. It's called from
  nsGenericElement::doReplaceOrInsertBefore
  the VALIDATE_ACCESS macro all over nsRange (SetBegin, etc)
  nsXULCommandDispatcher::GetFocusedElement
  nsXULCommandDispatcher::GetFocusedWindow
  nsXULDocument::GetPopupNode
  nsXULDocument::GetTooltipNode

Comment 9

[Daniel Veditz [:dveditz]]

dmose added the check for UniversalBrowserRead to CanCallerAccess for bug 219848. That'd be one thing to check for regressions, except it looks like that testcase has regressed even before I try out this patch. Bug 275194 was marked a dupe of that one and might be an easier testcase to resurrect (haven't tried yet).

Comment 10

[Daniel Veditz [:dveditz]]

I missed transformix, which has its own URIUtils::CanCallerAccess on the 1.8 branch but uses nsContentUtils on the trunk. It'll get fixed on the trunk, is there any way transformix can get hold of chrome nodes? Sicking, what do you think?

http://lxr.mozilla.org/mozilla1.8.0/search?string=URIUtils::CanCallerAccess

The trunk also has additional CanCallerAccess callers
  nsDOMSerializer::SerializeToString
  nsDOMSerializer::SerializeToStream
  nsXULDocument::GetPopupRangeParent
  nsXULDocument::GetPopupRangeOffset

Comment 11

[Daniel Veditz [:dveditz]]

Attachment: xslt patch -- 1.8(.0) branch only

On the trunk xslt uses nsContentUtils::CanCallerAccess, but if we're worried about it on the 1.8 branch we need this patch as well.

Comment 12

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

Comment on attachment 227046 [details] [diff] [review]
xslt patch -- 1.8(.0) branch only

The patch only seems to move the same code a bit further down (as well as duplicate the systemPrincipal check).

Comment 13

[Daniel Veditz [:dveditz]]

The first systemPrincipal check was for the subject (the running code). If it's systemPrincipal it can do anything so we return TRUE right then.

The second systemPrincipal check is on the argument node. If it is systemPrincipal then we do not want to let non-privileged code touch it. Privileged callers already returned TRUE so anyone getting this far must be non-privileged. We should return FALSE here.

The CheckSameOrigin bit has to be moved after we know and checn the principal of the object node or else UBR effectively grants permission to manipulate chrome nodes. That's what this bug is about.

(Arguably if nodePrincipal == systemPrincipal we should check whether UniversalXPConnect is enabled before bailing out, but on the trunk xslt uses nsContentUtils::CanCallerAccess and doesn't make that check. I didn't want to add a feature to the branch and then take it away on the trunk. If we want to change the trunk it should go in a new bug, and wouldn't hold up 1.8.0.5)

Comment 14

[Jay Patel [:jay]]

Comment on attachment 227034 [details] [diff] [review]
Patch v1

approved for 1.8.0 branch, a=jay for drivers.

Comment 15

[Jay Patel [:jay]]

Comment on attachment 227046 [details] [diff] [review]
xslt patch -- 1.8(.0) branch only

a=jay for 1.8.0 branch

Comment 16

[Daniel Veditz [:dveditz]]

Fix checked into the 1.8.0 branch

Comment 17

[Daniel Veditz [:dveditz]]

Fix checked into trunk

Comment 18

[Darin Fisher]

Please land this on the 1.8 branch ASAP so this does not hold up the FF2 beta1 release.  Or, explain why this should not be included in FF2 beta1.  Thanks!

Comment 19

[Daniel Veditz [:dveditz]]

Fix checked into 1.8 branch

Comment 20

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.5) Gecko/20060706 Firefox/1.5.0.5, access blocked:
uncaught exception: A script from "https://bugzilla.mozilla.org" was denied UniversalBrowserRead privileges.

Comment 21

[Daniel Veditz [:dveditz]]

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

I don't think that's a valid verification. You need to set the signed.applets.codebase_principal_support pref to true first, then you'll get a prompt asking for permission, grant it, *then* this security fix kicks in.

If you're not seeing the security prompt then you've not gotten far enough to test this bug. After the permission grant you should see an exception in the console like "Permission denied to get property Function.__proto__" rather than an alert showing Components.classes.

Comment 22

[Jay Patel [:jay]]

Thanks Dan for clarifying the steps.  I retested with the pref set and see exactly what you mentioned:

uncaught exception: Permission denied to get property Function.__proto__
initconsoleBindings.x... (line 86)
nullconsoleBindings.x... (line 267)

Returning to v.fixed.  Please change back if we are waiting for a more thorough verification.

Comment 23

[Alexander Sack]

Attachment: for 1.0.x

Comment 24

[Jay Patel [:jay]]

v.fixed on 1.8.1 branch: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060830 BonEcho/2.0b2