Last Comment Bug 497780 - onmouseout's MouseEvent.relatedTarget is a chrome element and is completely inaccessible.
: onmouseout's MouseEvent.relatedTarget is a chrome element and is completely i...
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- major with 2 votes (vote)
: ---
Assigned To: Olli Pettay [:smaug] (reviewing overload)
:
:
Mentors:
http://kellegous.com/moz-bugs/ff35-mo...
Depends on:
Blocks: 101197 475864
  Show dependency treegraph
 
Reported: 2009-06-11 18:08 PDT by Kelly Norton
Modified: 2009-08-24 09:09 PDT (History)
12 users (show)
mbeltzner: blocking1.9.1-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?


Attachments
not properly tested (923 bytes, patch)
2009-06-12 01:49 PDT, Olli Pettay [:smaug] (reviewing overload)
no flags Details | Diff | Splinter Review
better (4.14 KB, patch)
2009-06-12 03:28 PDT, Olli Pettay [:smaug] (reviewing overload)
mrbkap: review-
Details | Diff | Splinter Review
better (4.45 KB, patch)
2009-06-26 02:55 PDT, Olli Pettay [:smaug] (reviewing overload)
mrbkap: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
+comments (4.51 KB, patch)
2009-06-26 12:46 PDT, Olli Pettay [:smaug] (reviewing overload)
no flags Details | Diff | Splinter Review
+comments (5.25 KB, patch)
2009-06-26 12:50 PDT, Olli Pettay [:smaug] (reviewing overload)
no flags Details | Diff | Splinter Review

Description Kelly Norton 2009-06-11 18:08:36 PDT
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) 2009-06-12 01:22:24 PDT
(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] (reviewing overload) 2009-06-12 01:49:14 PDT
Created attachment 382932 [details] [diff] [review]
not properly tested
Comment 3 Olli Pettay [:smaug] (reviewing overload) 2009-06-12 01:52:10 PDT
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] (reviewing overload) 2009-06-12 02:25:13 PDT
One thing I'm not sure yet is what happens with videocontrols.xml.
It uses .relatedTarget.
Comment 5 Olli Pettay [:smaug] (reviewing overload) 2009-06-12 02:53:06 PDT
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] (reviewing overload) 2009-06-12 03:28:26 PDT
Created attachment 382938 [details] [diff] [review]
better

Maybe something like this.
Comment 7 Olli Pettay [:smaug] (reviewing overload) 2009-06-12 04:07:28 PDT
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] (reviewing overload) 2009-06-12 04:32:35 PDT
Tryserver builds will appear here https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-relatedTarget_2/
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2009-06-12 07:31:33 PDT
So this is a duplicate of bug 101197, right?
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2009-06-12 07:33:26 PDT
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] 2009-06-12 07:44:58 PDT
Don't see any reason why we would block release on this; feel free to renominate with rationale
Comment 12 Olli Pettay [:smaug] (reviewing overload) 2009-06-12 07:47:13 PDT
(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] (reviewing overload) 2009-06-12 07:49:31 PDT
(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] (still a bit busy) 2009-06-12 07:52:09 PDT
> 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 2009-06-12 08:43:52 PDT
(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] (still a bit busy) 2009-06-12 08:59:47 PDT
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] (reviewing overload) 2009-06-12 09:02:14 PDT
On FF3.0 we throw later, when using native anonymous element.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2009-06-12 09:06:45 PDT
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) 2009-06-12 11:03:32 PDT
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) 2009-06-17 17:30:34 PDT
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 2009-06-24 11:42:29 PDT
Any update on this issue?  Will it be fixed before the final release?
Comment 22 Olli Pettay [:smaug] (reviewing overload) 2009-06-26 02:55:51 PDT
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] (reviewing overload) 2009-06-26 06:42:11 PDT
https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-relatedTarget_3/
Comment 24 Olli Pettay [:smaug] (reviewing overload) 2009-06-26 11:03:45 PDT
Comment on attachment 385345 [details] [diff] [review]
better

Bz, do you think this would be ok for the branch?
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2009-06-26 12:23:22 PDT
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] (reviewing overload) 2009-06-26 12:33:59 PDT
> 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] (still a bit busy) 2009-06-26 12:45:29 PDT
Yeah, ok.  Add comments here and in that code that the two must match?
Comment 28 Olli Pettay [:smaug] (reviewing overload) 2009-06-26 12:46:07 PDT
Created attachment 385450 [details] [diff] [review]
+comments

Well, aContent was used for the assertion, but sure, not really needed.
Comment 29 Olli Pettay [:smaug] (reviewing overload) 2009-06-26 12:50:57 PDT
Created attachment 385451 [details] [diff] [review]
+comments
Comment 30 Olli Pettay [:smaug] (reviewing overload) 2009-06-28 15:20:42 PDT
http://hg.mozilla.org/mozilla-central/rev/bfa072ef0065
Comment 31 Mike Beltzner [:beltzner, not reading bugmail] 2009-07-22 16:15:29 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.