Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED

Status

()

Core
DOM
--
major
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Kelly Norton, Assigned: smaug)

Tracking

({regression, testcase})

Trunk
x86
Mac OS X
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -

Firefox Tracking Flags

(status1.9.1 ?)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(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
Component: General → DOM
Keywords: regression, testcase
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9.1?
(Assignee)

Comment 2

8 years ago
Created attachment 382932 [details] [diff] [review]
not properly tested
(Assignee)

Comment 3

8 years ago
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.

Updated

8 years ago
Blocks: 475864
(Assignee)

Comment 4

8 years ago
One thing I'm not sure yet is what happens with videocontrols.xml.
It uses .relatedTarget.
(Assignee)

Comment 5

8 years ago
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.)
(Assignee)

Comment 6

8 years ago
Created attachment 382938 [details] [diff] [review]
better

Maybe something like this.
(Assignee)

Comment 7

8 years ago
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.
Attachment #382938 - Flags: review?(mrbkap)
(Assignee)

Comment 8

8 years ago
Tryserver builds will appear here https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-relatedTarget_2/

Comment 9

8 years ago
So this is a duplicate of bug 101197, right?

Comment 10

8 years ago
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...
Don't see any reason why we would block release on this; feel free to renominate with rationale
Flags: wanted1.9.1.x?
Flags: blocking1.9.1?
Flags: blocking1.9.1-
(Assignee)

Comment 12

8 years ago
(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.
(Assignee)

Comment 13

8 years ago
(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

8 years ago
> 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.
(Reporter)

Comment 15

8 years ago
(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

8 years ago
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...
(Assignee)

Comment 17

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

Comment 18

8 years ago
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.  :(
Only if the caller of the DOM method passes the security check -- see XPCWrapper::Unwrap (and XPCWrapper::UnwrapSOW; in particular, the call to AllowedToAct).

Updated

8 years ago
Attachment #382938 - Flags: review?(mrbkap) → review-
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

8 years ago
Any update on this issue?  Will it be fixed before the final release?
(Assignee)

Comment 22

8 years ago
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.
Assignee: nobody → Olli.Pettay
Attachment #385345 - Flags: review?(mrbkap)
(Assignee)

Comment 23

8 years ago
https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-relatedTarget_3/

Updated

8 years ago
Attachment #385345 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 24

8 years ago
Comment on attachment 385345 [details] [diff] [review]
better

Bz, do you think this would be ok for the branch?
Attachment #385345 - Flags: superreview?(bzbarsky)

Updated

8 years ago
Attachment #385345 - Flags: superreview?(bzbarsky) → superreview+

Comment 25

8 years ago
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.
(Assignee)

Comment 26

8 years ago
> 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

8 years ago
Yeah, ok.  Add comments here and in that code that the two must match?
(Assignee)

Comment 28

8 years ago
Created attachment 385450 [details] [diff] [review]
+comments

Well, aContent was used for the assertion, but sure, not really needed.
Attachment #382932 - Attachment is obsolete: true
Attachment #382938 - Attachment is obsolete: true
Attachment #385345 - Attachment is obsolete: true
(Assignee)

Comment 29

8 years ago
Created attachment 385451 [details] [diff] [review]
+comments
Attachment #385450 - Attachment is obsolete: true
(Assignee)

Comment 30

8 years ago
http://hg.mozilla.org/mozilla-central/rev/bfa072ef0065
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Attachment #385451 - Flags: approval1.9.1.1?
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.
Attachment #385451 - Flags: approval1.9.1.1?
status1.9.1: --- → ?
Flags: wanted1.9.1.x?

Updated

8 years ago
Blocks: 101197
You need to log in before you can comment on or make changes to this bug.