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
Created attachment 382932 [details] [diff] [review] not properly tested
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.
One thing I'm not sure yet is what happens with videocontrols.xml. It uses .relatedTarget.
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.)
Created attachment 382938 [details] [diff] [review] better Maybe something like this.
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.
Tryserver builds will appear here https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-relatedTarget_2/
So this is a duplicate of bug 101197, right?
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
(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.
(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.
> 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.
(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.
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...
On FF3.0 we throw later, when using native anonymous element.
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).
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.
Any update on this issue? Will it be fixed before the final release?
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.
https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-relatedTarget_3/
Comment on attachment 385345 [details] [diff] [review] better Bz, do you think this would be ok for the branch?
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.
> Why "chrome://global/" and not just "chrome://"? Seems weird to lock > extensions out here... is that on purpose? Because that is what XPCSystemOnlyWrapper does.
Yeah, ok. Add comments here and in that code that the two must match?
Created attachment 385450 [details] [diff] [review] +comments Well, aContent was used for the assertion, but sure, not really needed.
Created attachment 385451 [details] [diff] [review] +comments
http://hg.mozilla.org/mozilla-central/rev/bfa072ef0065
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.
or
