497780 - onmouseout's MouseEvent.relatedTarget is a chrome element and is completely inaccessible.

Status: RESOLVED FIXED

Description

[Kelly Norton]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_7; en-us) AppleWebKit/531.0+ (KHTML, like Gecko) Version/4.0 Safari/530.17
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4

Wiring up an onmouseout to an element with scrollbar, the MouseEvent that is delivered when you move the mouse off of the scroll bar has a chrome element as the relatedTarget. Accessing any property on that element throws "Permission denied to access property 'nodeName' from non-chrome context"

The element (Firebug is able to display it) is one of <thumb/> or <slider/>.

Reproducible: Always

Steps to Reproduce:
1. Go to the URL.
2. Mouse over the scrollbar, then mouseout.

Actual Results:  
An error is thrown when the onmouseout callback tries to execute 'event.relatedTarget.nodeName'

Expected Results:  
The relatedTarget should be a accessible element from the pages DOM.

Comment 1

[Ria Klaassen (not reading all bugmail)]

(On windows at least) I see it only when I move my mouse above the scrollbar, and then the regression range is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1a1611bb1063&tochange=e167d6ca2023

Comment 2

[Olli Pettay [:smaug]]

Created attachment 382932 [details] [diff] [review]
not properly tested

Comment 3

[Olli Pettay [:smaug]]

We do retargeting for .target, but not for .relatedTarget.
When mouse moves from native anon to non-native anon, mouseover/out messages
are suppressed. So as far as I see, .relatedTarget is the problematic case with
mouse events.

Comment 4

[Olli Pettay [:smaug]]

One thing I'm not sure yet is what happens with videocontrols.xml.
It uses .relatedTarget.

Comment 5

[Olli Pettay [:smaug]]

when using the patch and moving mouse over videocontrols does cause .relatedTarget retargeting - as I excepted.

Blake, is there some easy way to detect if caller JS can access
native anon content? If yes, that could be used and not nsContentUtils::IsCallerChrome()
If not, we could add .mozOriginalRelatedTarget.
(I know, that is an ugly solution.)

Comment 6

[Olli Pettay [:smaug]]

Created attachment 382938 [details] [diff] [review]
better

Maybe something like this.

Comment 7

[Olli Pettay [:smaug]]

Comment on attachment 382938 [details] [diff] [review]
better

Could we do this, especially for branch. Using .relatedTarget is common. Though, I expect that it doesn't work too well always with FF3.0 either.

Comment 8

[Olli Pettay [:smaug]]

Tryserver builds will appear here https://build.mozilla.org/tryserver-builds/[e-mail]-relatedTarget_2/

Comment 9

[Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-)]

So this is a duplicate of bug 101197, right?

Comment 10

[Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-)]

The patch doesn't make sense to me.  Why not just check for the capability and then if missing look for chrome:// or some such?  The capability check is wrong, since it needs to walk up the stack...

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Don't see any reason why we would block release on this; feel free to renominate with rationale

Comment 12

[Olli Pettay [:smaug]]

(In reply to comment #10)
> The patch doesn't make sense to me. 
I copied the check from bug 475864.
IMHO the checks should be the same for the wrapper and for retargeting.

Comment 13

[Olli Pettay [:smaug]]

(In reply to comment #9)
> So this is a duplicate of bug 101197, right?
Sort of. Nowadays we throw an exception in a bit different place.

Comment 14

[Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-)]

> I copied the check from bug 475864.

Mmm... yeah.  That check doesn't make sense to me either: it requires an enablePrivilege in the function doing the access, not just in any caller.  How come?

I agree that the code should match (which to me implies it should either use the existing secman API or be refactored in some other way; copy-paste is not the right way to get things to match, imo).

> Sort of. Nowadays we throw an exception in a bit different place.

Sure; the real bug is that we hand out native anon content via relatedTarget in the first place.

Comment 15

[Kelly Norton]

(In reply to comment #11)
> Don't see any reason why we would block release on this; feel free to
> renominate with rationale

FWIW, I filed this because we're getting reports in GWT of frontends throwing exceptions in ff3.5b4. It's a hard bug to work around since there already exists a lot of code that gets a reference to relatedTarget and then calls other DOM related functions with it. Adding pessimistic checks everywhere in our DOM library to guard against the possibility of a toxic element (one that denies access to every property) would be a horrible outcome and would also be a pretty sad performance/size hit for Firefox users.

I have to imagine that this effects other libraries as well since most of them make use of relatedTarget.

Comment 16

[Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-)]

All those cases should also throw in Fx3.0, no?  We certainly hand back native anon relatedTarget there, and don't have quickstubs to make it sometimes work...

Comment 17

[Olli Pettay [:smaug]]

On FF3.0 we throw later, when using native anonymous element.

Comment 18

[Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-)]

Ah, so if you just pass it to some DOM method (as opposed to trying to get props off it) it works?  That really sucks, actually.  :(

Comment 19

[Blake Kaplan (:mrbkap)]

Only if the caller of the DOM method passes the security check -- see XPCWrapper::Unwrap (and XPCWrapper::UnwrapSOW; in particular, the call to AllowedToAct).

Comment 20

[Blake Kaplan (:mrbkap)]

Comment on attachment 382938 [details] [diff] [review]
better

>+PRBool
>+nsContentUtils::CanAccessNativeAnon(nsIContent* aContent)
>+{
>+  NS_ASSERTION(aContent && aContent->IsInNativeAnonymousSubtree(),
>+               "Fix the caller!");
>+  JSContext* cx = nsnull;
>+  sThreadJSContextStack->Peek(&cx);
>+  if (!cx) {
>+    return PR_TRUE;
>+  }
>+  JSStackFrame* fp;
>+  nsIPrincipal* principal =
>+    sSecurityManager->GetCxSubjectPrincipalAndFrame(cx, &fp);
>+  NS_ENSURE_TRUE(principal, PR_FALSE);
>+  void* annotation = JS_GetFrameAnnotation(cx, fp);
>+  PRBool privileged;
>+  if (fp &&
>+      NS_SUCCEEDED(principal->IsCapabilityEnabled("UniversalXPConnect",
>+                                                  annotation,
>+                                                  &privileged)) &&
>+      privileged) {
>+    // UniversalXPConnect things are allowed to touch us.
>+    return PR_TRUE;
>+  }

This differs subtly from the code in XPCSystemOnlyWrapper.cpp. In particular, its handling of no JS code running and only native code running differs. Also, don't you need to null check fp here?

Other than that, I can't come up with a better solution here. My only thought would be to have the "system only wrappers" do the re-targeting done here if they notice invalid access. That would make them less general purpose, but would centralize the re-targeting. Not sure if that's necessarily "better", though.

Comment 21

[acquaflash12]

Any update on this issue?  Will it be fixed before the final release?

Comment 22

[Olli Pettay [:smaug]]

Created attachment 385345 [details] [diff] [review]
better

because I'm hoping to get this to 1.9.1.x, I didn't move the code to
script security manager (just copied it). That could be done later on trunk.

Comment 23

[Olli Pettay [:smaug]]

https://build.mozilla.org/tryserver-builds/[e-mail]-relatedTarget_3/

Comment 24

[Olli Pettay [:smaug]]

Comment on attachment 385345 [details] [diff] [review]
better

Bz, do you think this would be ok for the branch?

Comment 25

[Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-)]

Comment on attachment 385345 [details] [diff] [review]
better

The new function needs documentation.

Its aContent argument is unused; get rid of it.

Why "chrome://global/" and not just "chrome://"?  Seems weird to lock extensions out here... is that on purpose?

With those nits, looks ok.  Not sure about branch yet; I _think_ it should be safe, but let's get trunk baking and security banging on it first.

Comment 26

[Olli Pettay [:smaug]]

> Why "chrome://global/" and not just "chrome://"?  Seems weird to lock
> extensions out here... is that on purpose?
Because that is what XPCSystemOnlyWrapper does.

Comment 27

[Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-)]

Yeah, ok.  Add comments here and in that code that the two must match?

Comment 28

[Olli Pettay [:smaug]]

Created attachment 385450 [details] [diff] [review]
+comments

Well, aContent was used for the assertion, but sure, not really needed.

Comment 29

[Olli Pettay [:smaug]]

Created attachment 385451 [details] [diff] [review]
+comments

Comment 30

[Olli Pettay [:smaug]]

http://hg.mozilla.org/mozilla-central/rev/bfa072ef0065

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 385451 [details] [diff] [review]
+comments

Has the security banging referred to in comment 25 happened? If so, please post results and renominate for branch.