289940 - Chrome code needs to be protected from untrusted events.

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Johnny Stenback (:jst)]

Right now our chrome code generally excpects its event handlers to be called
onyl from the code that normally fires the events in question, but lots of event
handlers listen for events that can be synthesized by untrusted content, and
we're thus vulnerable to security problems, or at least unexpected code
execution. We have a solution for this problem, and that is to check if the
event in question is trusted or not, but way too often this check is omitted and
it's by now unlikely to expect this problem to be fixable by adding those
checks, and not forgetting those checks in new code etc.

The safer fix for this problem is to simply stop delivering untrusted events to
chrome code. This way, if people forget something related to this, their code
won't work rather than having security problems creep into the code. The
potential problem with this approach is that existing code that does rely on
untrusted events in chrome code is broken by this change. But I think that's
inevitable, and I'm hoping the breakage won't be too broad. Because of this, we
do need an alternate apporach that the code that does indeed need to see
untrusted events from chrome code can use. That will be done by an optional
additional argument to the addEventListener() method that tells whether the
listener wants untrusted events or not. By default content code will get them,
chrome code won't.

The big part of this change is to mark all our internal events as trusted, where
applicable. This will be done by changing the constructors of the various
ns*Event classes such that the C++ compiler will require you to specify if an
event is trusted or not. Patch coming up (tested only on Win32 and Gtk2 builds
for now).

(marking security sensitive to keep the visibility of this problem down for now)

Comment 1

[Johnny Stenback (:jst)]

Attachment: Make ns*Event ctor's take a "trusted" arg, and make chrome not get untusted events by default (WIP still)

Comment 2

[Boris Zbarsky [:bzbarsky]]

/mozilla/widget/src/gtk/nsWindow.cpp:3179: no 
   matching function for call to `nsMouseEvent::nsMouseEvent(int, int, 
   nsWindow*&)'
/mozilla/widget/src/gtk/nsWindow.cpp:3318: no 
   matching function for call to `nsMouseEvent::nsMouseEvent(int, int, 
   nsWindow*&)'

With those fixed, widget/ builds.

I'll do a build of the whole tree tonight and see whether I can give it a spin.

Comment 3

[Boris Zbarsky [:bzbarsky]]

More build bustage, only if SVG is enabled:

/mozilla/content/xml/document/src/nsXMLContentSink.cpp:1008: no
   matching function for call to `nsEvent::nsEvent(int)'

Comment 4

[Boris Zbarsky [:bzbarsky]]

And more:

/mozilla/content/svg/content/src/nsSVGElement.cpp:234: no
   matching function for call to `nsDerivedSafe<nsIEventListenerManager>::
   AddScriptEventListener(nsIContent*, nsIAtom*&, const nsAString&, int)'
/mozilla/content/svg/content/src/nsSVGElement.cpp:668: no
   matching function for call to `nsMutationEvent::nsMutationEvent(int, 
   nsCOMPtr<nsIDOMEventTarget>&)'
/mozilla/content/svg/content/src/nsSVGScriptElement.cpp:242: no
   matching function for call to `nsScriptErrorEvent::nsScriptErrorEvent(int)'
/mozilla/content/svg/content/src/nsSVGScriptElement.cpp:283: no
   matching function for call to `nsEvent::nsEvent(int)'

Comment 5

[Boris Zbarsky [:bzbarsky]]

With those 7 spots fixed, a GTK1 build compiles and runs.  I tested some things
(clicking on links, the link toolbar, that sort of thing) and they worked.

Comment 6

[Jesse Ruderman]

*** Bug 265729 has been marked as a duplicate of this bug. ***

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Make ns*Event ctor's take a "trusted" arg, and make chrome not get untusted events by default (WIP still)

This merges attachment 180396 [details] [diff] [review] to the trunk (changes to nsEventStateManager),
makes the changes per comment 2 (untested, since built on Mac), comment 3, and
comment 4, and makes some changes to nsMenuX.cpp and nsMenuBarX.cpp in
widget/src/mac/ to get this to build Mac firefox and to nsMenuX.cpp in
widget/src/cocoa/ for building Camino (my Camino build isn't done yet, but
it's past widget; my Mac firefox build failed linking libgklayout.dylib
for reasons related to cairo).

Comment 8

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

I've got this up and running locally, still reviewing though...

Comment 9

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

I feel a bit uneasy about the iscallerchrome check in
RegisterScriptEventListener, but it's probably ok. I need to look at it some more.

In @@ -1620,32 +1632,44 @@ nsresult nsEventListenerManager::HandleE
+        if (!NS_IS_TRUSTED_EVENT(aEvent) ||
+            ls->mFlags & NS_PRIV_EVENT_UNTRUSTED_PERMITTED) {
+          int a = 4;
+        }
...
+        } else {
+          int a = 5;
         }

Debug code?


In @@ -524,19 +524,19 @@ nsEventStateManager::PreHandleEvent(nsPr

-            nsEvent blurevent(NS_BLUR_CONTENT);
+            nsEvent blurevent(NS_IS_TRUSTED_EVENT(aEvent), NS_BLUR_CONTENT);

Couldn't this always be trusted? Even if the focusevent was untrusted we're
still really blurring the old element, right? Same in other places in this file.

Hmm.. is this a problem in general, that 'fake' events can have real effects
that chrome is interested in. Like if a javascript clickevent is dispatched on a
link.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Links actually ignore all untrusted click events in current builds...

Comment 11

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

So are there no untrusted events we want to react to? How about clicks on
sumbitbuttons? And can focus affect the UI?


In @@ -1267,21 +1267,24 @@ nsEventStateManager::FireContextClick()
+        // XXX: Should this be a trusted event?
+        nsMouseEvent event(PR_TRUE, NS_CONTEXTMENU,
+                           mCurrentTarget->GetWindow(),
+                           nsMouseEvent::eReal);
Why not?

Comment 12

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

Somewhat related to this bug, but should be a separate patch. Is there a reason
we couldn't make sure that all eventclasses enforce that event.target is always
a real node (rather then a js object).

Comment 13

[Boris Zbarsky [:bzbarsky]]

> So are there no untrusted events we want to react to? 

I want to say "no", but I'm not sure that's true....

> How about clicks on sumbitbuttons?

That's a tough one (since the page can just call submit() on the form, which
does a similar but not identical thing).

> And can focus affect the UI?

Probably. :(  ccing some UI folks for help.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #12)
> Somewhat related to this bug, but should be a separate patch. Is there a reason
> we couldn't make sure that all eventclasses enforce that event.target is always
> a real node (rather then a js object).

Have a look at nsEventStateManager::DispatchNewEvent and its caller.  I *think*
this is already done.

Comment 15

[Johnny Stenback (:jst)]

(In reply to comment #13)
> > So are there no untrusted events we want to react to? 
> 
> I want to say "no", but I'm not sure that's true....

I can't say I'm sure either, but I must say I'd be surprised if this wasn't the
case.

> > How about clicks on sumbitbuttons?
> 
> That's a tough one (since the page can just call submit() on the form, which
> does a similar but not identical thing).

Not really, IMO. No matter how a submit happens, the onsubmit event should be
trusted since it's an event that tells you that we're now submitting, no matter
what triggerd the submit action (i.e. a synthetic click event on a submit button
should fire a trusted onsubmit event, just like calling form.submit() should).

Comment 16

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

In @@ -1629,19 +1638,20 @@ nsHTMLInputElement::HandleDOMEvent(nsPre
           if (mForm) {
-            nsFormEvent event((mType == NS_FORM_INPUT_RESET) ?
+            // XXX: Should this always be trusted?
+            nsFormEvent event(PR_TRUE, (mType == NS_FORM_INPUT_RESET) ?
                               NS_FORM_RESET : NS_FORM_SUBMIT);

This is IMHO inconsistent. Content calling Reset() will give an untrusted
reset/submit event, but content dispatching a DOMActivate will get a trusted
one. I think I agree that these could be trusted.

Same inconsistency in xulelement::click/docommand vs. html*element::click, but
here I think a CallerIsChrome check is the right thing to do.

In @@ -668,19 +665,20 @@ nsXULElement::AddScriptEventListener(nsI

-    return manager->AddScriptEventListener(target, aName, aValue, defer);
+    return manager->AddScriptEventListener(target, aName, aValue, defer,
+                                           /* XXX */ PR_TRUE);

Shouldn't this have the SchemeIs("chrome") check?


In @@ -373,19 +373,21 @@ nsXULCommandDispatcher::UpdateCommands(c

+      // XXX: Do we need to check if we're called from chrome?
+      nsEvent event(PR_TRUE, NS_XUL_COMMAND_UPDATE);

It'd be good if someone that knew the command architecture better then me
answered this question.


You'll have to walk me through the classinfo magic. I get most of it, but not all.


In @@ -3495,19 +3495,20 @@ nsresult nsPluginInstanceOwner::Dispatch

-      nsGUIEvent focusEvent(theEvent->message); // we only care about the
message in ProcessEvent
+      // we only care about the message in ProcessEvent
+      nsGUIEvent focusEvent(PR_FALSE, theEvent->message, nsnull);

Why false?

Are you sure it's ok to remove the nullcheck in nsMenuFrame::Execute?


Ok, it's getting too late here. Finishing the rest tomorrow.

Comment 17

[[e-mail]]

(In reply to comment #13)
>>And can focus affect the UI?
>Probably. :(  ccing some UI folks for help.
I'm probably commenting at completely the wrong time of day, so all I can think
of is that we currently update the status bar with a link's target as it gets
focus (although but we don't clear it when it blurs...)

Comment 18

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

I wonder if we should always make focus events that actually affect focus
trusted too? Just like submit and reset.


In @@ -1507,19 +1508,19 @@ void nsGfxScrollFrameInner::CurPosAttrib

-        nsScrollbarEvent event(NS_SCROLL_EVENT);
+        nsScrollbarEvent event(PR_FALSE, NS_SCROLL_EVENT, nsnull);

Why false?


Shouldn't the code in nsButtonBoxFrame::MouseClicked default to untrusted event
if there is no aEvent. Similar question in other MouseClicked functions.

Dbaron/bz, does MouseClicked (with or without an aEvent argument) ever get
called unless the user actually clicked on the frame using the mouse or through
accessibility interfaces?

Maybe we could use check for js on the stack and make it untrusted unless the
top js is chrome?

Comment 19

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

The added space in @@ -2638,20 +2638,20 @@ nsChildView::Idle() seems accidental

Whitespace in @@ -1548,30 +1550,32 @@ PRBool nsMacEventHandler::HandleMouseDow
is whacky

You claim all eventtargets should implement nsIDOMNSEventTarget, but you're not
adding that interface to any classes?


Ok, that's it!

Comment 20

[Johnny Stenback (:jst)]

(In reply to comment #19)
> The added space in @@ -2638,20 +2638,20 @@ nsChildView::Idle() seems accidental

Yeah, fixed.

> Whitespace in @@ -1548,30 +1550,32 @@ PRBool nsMacEventHandler::HandleMouseDow
> is whacky

Yeah, whitespace in that whole file is whacky. Nice mixture of spaces and tabs.
I won't mess with that now.

> You claim all eventtargets should implement nsIDOMNSEventTarget, but you're not
> adding that interface to any classes?

Oops, yeah, that was the intention, but I never wrote that code. I've got it now
tho... new patch coming up where all your comments have been addressed, and some
more issues too.

Comment 21

[Johnny Stenback (:jst)]

Attachment: Same thing, but fix CreateEvent() users too, and remove unused MouseClicked() methods in some layout code, and implement nsIDOMNSEventTarget::AddEventListener() on all targets.

I believe this one's money.

Comment 22

[Johnny Stenback (:jst)]

Reviewers, FYI there's a missing '!' in front of the isChromeElement argument in
nsXULElement::AddScriptEventListener(), the code should be:

    return manager->AddScriptEventListener(target, aName, aValue, defer,
                                           !isChromeElement);

Comment 23

[chris hofmann]

Comment on attachment 181923 [details] [diff] [review]
Same thing, but fix CreateEvent() users too, and remove unused MouseClicked() methods in some layout code, and implement nsIDOMNSEventTarget::AddEventListener() on all targets.

hoping we get review overnight and can get this approved on thursday if it
looks good.

Comment 24

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

Comment on attachment 181923 [details] [diff] [review]
Same thing, but fix CreateEvent() users too, and remove unused MouseClicked() methods in some layout code, and implement nsIDOMNSEventTarget::AddEventListener() on all targets.

In @@ -343,43 +348,48 @@ nsHTMLButtonElement::HandleDOMEvent(nsPr

Should the last event in this block always be trusted? Or should we promote the
event to a trusted one sometime later on if it in fact causes a
submission/reset (does it ever not?).


For the various click functions, I think it might be a good idea to implement a
nsContentUtils::IsCallerNativeOrChrome(). There might be embedders that want to
call these functions.


So is nsEventListenerManager::RegisterScriptEventListener the function that is
called when you set  myElem.onfoo = <jscode> ?	Please add some documentation
to the headerfile that explains that this function will do a iscallerchrome
check.

r=me with that

Comment 25

[Peter Van der Beken [:peterv]]

Comment on attachment 181923 [details] [diff] [review]
Same thing, but fix CreateEvent() users too, and remove unused MouseClicked() methods in some layout code, and implement nsIDOMNSEventTarget::AddEventListener() on all targets.

>Index: content/events/public/nsIPrivateDOMEvent.h
>===================================================================

>+class nsEvent;

>+NS_NewDOMEvent(nsIDOMEvent** aInstancePtrResult, nsPresContext* aPresContext, class nsEvent *aEvent);

Don't need the class keyword here.

>Index: content/events/src/nsDOMUIEvent.cpp
>===================================================================

>+  : nsDOMEvent(aPresContext, aEvent ?
>+               NS_STATIC_CAST(nsEvent *, aEvent) :
>+               NS_STATIC_CAST(nsEvent *, new nsUIEvent(PR_FALSE, 0, 0)))

Are these casts needed?

>Index: content/events/src/nsEventListenerManager.cpp
>===================================================================

> nsListenerStruct*
> nsEventListenerManager::FindJSEventListener(EventArrayType aType)
> {
>   nsVoidArray *listeners = GetListenersByType(aType, nsnull, PR_FALSE);
>   if (listeners) {
>-    //Run through the listeners for this IID and see if a script listener is registered
>+    // Run through the listeners for this type and see if a script
>+    // listener is registered
>     nsListenerStruct *ls;
>-    for (int i=0; i<listeners->Count(); i++) {
>+    for (int i=0; i < listeners->Count(); i++) {

While you're here: s/int/PRInt32/

>@@ -1620,32 +1632,44 @@ nsresult nsEventListenerManager::HandleE
>       PRInt32 count = listeners->Count();
>       nsVoidArray originalListeners(count);
>       originalListeners = *listeners;
> 
>       nsAutoPopupStatePusher popupStatePusher(nsDOMEvent::GetEventPopupControlState(aEvent));
> 
>       for (int k = 0; !mListenersRemoved && listeners && k < count; ++k) {
>         nsListenerStruct* ls = NS_STATIC_CAST(nsListenerStruct*, originalListeners.FastElementAt(k));
>         // Don't fire the listener if it's been removed
>-        if (listeners->IndexOf(ls) != -1 && ls->mFlags & aFlags && ls->mGroupFlags == currentGroup) {
>+
>+        if (!NS_IS_TRUSTED_EVENT(aEvent) ||
>+            ls->mFlags & NS_PRIV_EVENT_UNTRUSTED_PERMITTED) {
>+          int a = 4;
>+        }

Remove this.

>           // If it doesn't implement that, call the generic HandleEvent()
>           if (!hasInterface && (ls->mSubType == NS_EVENT_BITS_NONE ||
>-                                ls->mSubType & dispData->bits))
>+                                ls->mSubType & dispData->bits)) {
>             HandleEventSubType(ls, *aDOMEvent, aCurrentTarget,
>                                dispData ? dispData->bits : NS_EVENT_BITS_NONE,
>                                aFlags);
>+          }
>+        } else {
>+          int a = 5;

Remove this.

>Index: content/events/src/nsEventStateManager.cpp
>===================================================================


>@@ -3985,28 +3987,31 @@ nsEventStateManager::SendFocusBlur(nsPre
>         nsCOMPtr<nsIViewManager> kungFuDeathGrip;
>         nsIPresShell *shell = doc->GetShellAt(0);
>         if (shell) {
>           kungFuDeathGrip = shell->GetViewManager();
> 
>           nsCOMPtr<nsPresContext> oldPresContext = shell->GetPresContext();
> 
>           //fire blur
>           nsEventStatus status = nsEventStatus_eIgnore;
>-          nsEvent event(NS_BLUR_CONTENT);
>+          nsEvent event(nsContentUtils::IsCallerChrome(), NS_BLUR_CONTENT);
> 
>           EnsureDocument(presShell);
> 
>-          // Make sure we're not switching command dispatchers, if so, surpress the blurred one
>+          // Make sure we're not switching command dispatchers, if so,
>+          // surpress the blurred one
>           if(gLastFocusedDocument && mDocument) {
>             nsIFocusController *newFocusController = nsnull;
>             nsIFocusController *oldFocusController = nsnull;
>-            nsCOMPtr<nsPIDOMWindow> newWindow = do_QueryInterface(mDocument->GetScriptGlobalObject());
>-            nsCOMPtr<nsPIDOMWindow> oldWindow = do_QueryInterface(gLastFocusedDocument->GetScriptGlobalObject());
>+            nsCOMPtr<nsPIDOMWindow> newWindow =
>+              do_QueryInterface(mDocument->GetScriptGlobalObject());
>+            nsCOMPtr<nsPIDOMWindow> oldWindow =
>+              do_QueryInterface(gLastFocusedDocument->GetScriptGlobalObject());
>             if(newWindow)
>               newFocusController = newWindow->GetRootFocusController();
>             if(oldWindow)
>               oldFocusController = oldWindow->GetRootFocusController();
>             if(oldFocusController && oldFocusController != newFocusController)
>               oldFocusController->SetSuppressFocus(PR_TRUE, "SendFocusBlur Window Switch");
>           }
> 
>           nsCOMPtr<nsIEventStateManager> esm;
>@@ -4040,21 +4045,22 @@ nsEventStateManager::SendFocusBlur(nsPre
>     nsCOMPtr<nsIScriptGlobalObject> globalObject;
> 
>     if(gLastFocusedDocument)
>       globalObject = gLastFocusedDocument->GetScriptGlobalObject();
> 
>     EnsureDocument(presShell);
> 
>     if (gLastFocusedDocument && (gLastFocusedDocument != mDocument) && globalObject) {
>       nsEventStatus status = nsEventStatus_eIgnore;
>-      nsEvent event(NS_BLUR_CONTENT);
>+      nsEvent event(nsContentUtils::IsCallerChrome(), NS_BLUR_CONTENT);
> 
>-      // Make sure we're not switching command dispatchers, if so, surpress the blurred one
>+      // Make sure we're not switching command dispatchers, if so,
>+      // surpress the blurred one
>       if (mDocument) {
>         nsIFocusController *newFocusController = nsnull;
>         nsIFocusController *oldFocusController = nsnull;
>         nsCOMPtr<nsPIDOMWindow> newWindow = do_QueryInterface(mDocument->GetScriptGlobalObject());
>         nsCOMPtr<nsPIDOMWindow> oldWindow = do_QueryInterface(gLastFocusedDocument->GetScriptGlobalObject());
> 
>         if (newWindow)
>           newFocusController = newWindow->GetRootFocusController();
>         oldFocusController = oldWindow->GetRootFocusController();
>@@ -4124,19 +4130,19 @@ nsEventStateManager::SendFocusBlur(nsPre
>     //to that element while the first focus is still ongoing.
>     PRBool clearFirstFocusEvent = PR_FALSE;
>     if (!mFirstFocusEvent) {
>       mFirstFocusEvent = aContent;
>       clearFirstFocusEvent = PR_TRUE;
>     }
> 
>     //fire focus
>     nsEventStatus status = nsEventStatus_eIgnore;
>-    nsEvent event(NS_FOCUS_CONTENT);
>+    nsEvent event(nsContentUtils::IsCallerChrome(), NS_FOCUS_CONTENT);
> 
>     if (nsnull != mPresContext) {
>       nsCxPusher pusher(aContent);
>       aContent->HandleDOMEvent(mPresContext, &event, nsnull, NS_EVENT_FLAG_INIT, &status);
>     }
> 
>     nsAutoString tabIndex;
>     aContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::tabindex, tabIndex);
>     PRInt32 ec, val = tabIndex.ToInteger(&ec);
>@@ -4145,19 +4151,19 @@ nsEventStateManager::SendFocusBlur(nsPre
>     }
> 
>     if (clearFirstFocusEvent) {
>       mFirstFocusEvent = nsnull;
>     }
>   } else if (!aContent) {
>     //fire focus on document even if the content isn't focusable (ie. text)
>     //see bugzilla bug 93521
>     nsEventStatus status = nsEventStatus_eIgnore;
>-    nsEvent event(NS_FOCUS_CONTENT);
>+    nsEvent event(nsContentUtils::IsCallerChrome(), NS_FOCUS_CONTENT);
> 
>     if (nsnull != mPresContext && mDocument) {
>       nsCxPusher pusher(mDocument);
>       mDocument->HandleDOMEvent(mPresContext, &event, nsnull, NS_EVENT_FLAG_INIT, &status);
>     }
>   }
> 
>   if (mBrowseWithCaret)
>     SetContentCaretVisible(presShell, aContent, PR_TRUE);

I don't understand why we check for IsCallerChrome in these.

>Index: dom/public/idl/events/nsIDOMNSEventTarget.idl
>===================================================================

>+ * The nsIDOMNSEventTarget interface an extension to the standard

... interface *is* an ...

>+   * lets callers controll whether or not they want to receive

control

>+   *                         they're truested

trusted

>Index: dom/src/base/nsDOMClassInfo.cpp
>===================================================================

> nsresult
> nsEventReceiverSH::RegisterCompileHandler(nsIXPConnectWrappedNative *wrapper,
>                                           JSContext *cx, JSObject *obj,
>                                           jsval id, PRBool compile,
>                                           PRBool remove,
>-                                          PRBool *did_compile)
>+                                          PRBool *did_define)

>   if (!IsEventName(id)) {
>+    if (id == sAddEventListener_id) {
>+      JSString *str = JSVAL_TO_STRING(id);
>+      JSFunction *fnc =
>+        ::JS_DefineFunction(cx, obj, ::JS_GetStringBytes(str),
>+                            AddEventListenerHelper, 0, JSPROP_ENUMERATE);
>+
>+      *did_define = PR_TRUE;
>+
>+      return fnc ? NS_OK : NS_ERROR_UNEXPECTED;
>+
>+    }
>+
>     return NS_OK;
>   }

I'd move this into NewResolve

Please file a followup bug on the XUL command stuff and the XForms stuff, so we
can make sure we're doing the right thingthere.

Comment 26

[timeless]

silver says nsEvent.h declares nsEvent as a struct. please don't forward declare
it incorrectly, that'll trigger a warning.

Comment 27

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

I wonder if these too should always be trusted:

@@ -577,19 +577,19 @@ nsEventStateManager::PreHandleEvent(nsPr
@@ -4124,19 +4130,19 @@ nsEventStateManager::SendFocusBlur(nsPre
@@ -4145,19 +4151,19 @@ nsEventStateManager::SendFocusBlur(nsPre
@@ -255,19 +255,19 @@ nsHTMLLabelElement::HandleDOMEvent(nsPre

Not really sure about the last one...

Comment 28

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

Ugh, and these:

@@ -524,19 +524,19 @@ nsEventStateManager::PreHandleEvent(nsPr
@@ -675,19 +675,19 @@ nsEventStateManager::PreHandleEvent(nsPr
@@ -831,19 +831,19 @@ nsEventStateManager::PreHandleEvent(nsPr
@@ -3985,28 +3987,31 @@ nsEventStateManager::SendFocusBlur(nsPre
@@ -4040,21 +4045,22 @@ nsEventStateManager::SendFocusBlur(nsPre

Comment 29

[Johnny Stenback (:jst)]

(In reply to comment #25)
> >+  : nsDOMEvent(aPresContext, aEvent ?
> >+               NS_STATIC_CAST(nsEvent *, aEvent) :
> >+               NS_STATIC_CAST(nsEvent *, new nsUIEvent(PR_FALSE, 0, 0)))
> 
> Are these casts needed?

Yeha, w/o the casts I get:

../../../../mozilla/content/events/src/nsDOMUIEvent.cpp:58: error: conditional 
   expression between distinct pointer types `nsGUIEvent*' and `nsUIEvent*' 
   lacks a cast

Fixed the rest, including sickings latest findings (except the
nsHTMLLableElement case which we decided should remain as is).

Comment 30

[chris hofmann]

Comment on attachment 181923 [details] [diff] [review]
Same thing, but fix CreateEvent() users too, and remove unused MouseClicked() methods in some layout code, and implement nsIDOMNSEventTarget::AddEventListener() on all targets.

a=chofmann

Comment 31

[Johnny Stenback (:jst)]

Attachment: Patch that's being checked in.

Comment 32

[Johnny Stenback (:jst)]

Patch checked in on the trunk.

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: fix for qt build bustage (checked in 2005-04-29 15:29 PDT)

Comment 34

[Johnny Stenback (:jst)]

Comment on attachment 182207 [details] [diff] [review]
fix for qt build bustage (checked in 2005-04-29 15:29 PDT)

r+sr=jst

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 182207 [details] [diff] [review]
fix for qt build bustage (checked in 2005-04-29 15:29 PDT)

a=asa

Comment 36

[Johnny Stenback (:jst)]

Marking FIXED.

Comment 37

[Daniel Veditz [:dveditz]]

*** Bug 294323 has been marked as a duplicate of this bug. ***

Comment 38

[Jay Patel [:jay]]

jst:  Is the Trunk patch safe for the branch?  If so, let's get this checked in
for 1.0.5.  Otherwise, let's get a new patch with reviews.

Comment 39

[Johnny Stenback (:jst)]

Jay, dveditz is working on porting the patch to the branch.

Comment 40

[Daniel Veditz [:dveditz]]

Attachment: preliminary aviary patch

This is a preliminary aviary-branch patch. Still making a couple tweaks

Comment 41

[Daniel Veditz [:dveditz]]

jst: nsXBLPrototypeHandler::BindingAttached and
nsXBLPrototypeHandler::BindingDetached call CreateEvent -- should those have the
trusted bits set?

Comment 42

[Johnny Stenback (:jst)]

dveditz, yeah, both of those should fire trusted events.

Comment 43

[Daniel Veditz [:dveditz]]

(In reply to comment #40)
> Created an attachment (id=186648)
> This is a preliminary aviary-branch patch. Still making a couple tweaks

I've completed the patch and incorporated bug 292326 and bug 292464 (295210 was
checked in independently). I'm not attaching it because it doesn't actually work
-- the "blocked" bugs are still exploitable on the branch, for example. Diffing
against the trunk now to see what I flipped the wrong way. Could be my
replacement for the trunk's GetOwnerDoc() was wrong :-( (nsGenericElement and a
couple others).

Comment 44

[Daniel Veditz [:dveditz]]

Attachment: Aviary branch patch

Found the missing lines doing a careful diff of diffs between the trunk and
branch patches. Patch now does what it's supposed to.

Comment 45

[Johnny Stenback (:jst)]

Looks like the new file nsIDOMNSEventTarget.idl is missing from the diff (should
be able to just copy that from the trunk).

Comment 46

[Johnny Stenback (:jst)]

The implementation of the method nsContentUtils::IsChromeDoc() is missing a
return value type declaration, it should look like:

PRBool
nsContentUtils::IsChromeDoc(nsIDocument *aDocument)

Comment 47

[Daniel Veditz [:dveditz]]

(In reply to comment #45)
> Looks like the new file nsIDOMNSEventTarget.idl is missing

Forgot -N on the diff, it's in there though.

(In reply to comment #46)
> The implementation of the method nsContentUtils::IsChromeDoc() is missing a
> return value type declaration

Thanks.

I'm going to check this in so QA can start beating on tomorrow's builds.

Comment 48

[Daniel Veditz [:dveditz]]

Fix checked in to mozilla 1.7 and aviary101 branches.

Comment 49

[Daniel Veditz [:dveditz]]

Adding distributors

Comment 50

[Daniel Veditz [:dveditz]]

Security advisories published

Comment 51

[Georges-Etienne Legendre]

Maybe the Bug 300852 is related to this one?

Comment 52

[Juha-Matti Laurio]

This is new MFSA2005-45
http://www.mozilla.org/security/announce/mfsa2005-45.html and SA16043's
vulnerability #1; see http://secunia.com/advisories/16043/ . Alias field is not
yet updated to contain SA16043.

Comment 53

[Pooya Karimian]

This patched disabled ability the catch to key events and changing them to
another one, which as I far as I know can only be done using initkeyevent, check
the second example on this page:

http://www.pooyak.com/blog/archives/000146.shtml

I know that this is something that potentially can cause many security problems
but at the same time it is something very useful for web application. For
example a popular usage is to make the tab key working in a text area, as I did
on the edit page of Uniwakka, a scientific Wiki. I remember the old version of
Yahoo Mail also had functionality (IE Only), but I couldn’t find a reference to
it. There tab is used for indentation:

http://www.istitutocolli.org/uniwakka/SandBox/edit

I also use that to port a popular script for changing the keyboard layout to
Firefox. This script allows users to type in Persian language in text inputs
without having any special software installed. The IE version of it is very
popular in Persian community:

http://www.pooyak.com/p/persianjavascript/

In Internet Explorer it is done by either changing keyCode property of the event
(which as I remember is read-only in Mozilla) or using some IE specific text
selection functions to find the cursor position and put the new character there.

I'm looking forward either for getting back this functionality or knowing
alternate solutions to it.

Thanks
More info on request: me at pooyak.com

Comment 54

[Dennis Jacobfeuerborn]

Could this have caused Bug 305970 ? It falls right into the timeframe that bug
occured.

Comment 55

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

Comment on attachment 186722 [details] [diff] [review]
Aviary branch patch

Resetting old request flags

Comment 56

[Boris Zbarsky [:bzbarsky]]

This caused bug 303713