Closed Bug 210240 Opened 21 years ago Closed 17 years ago

alert() in onblur triggers onblur with XULDocument target

Categories

(Core :: Security, defect, P4)

Product:
Core
Component:
Security
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: jruderman, Assigned: emaijala+moz)

References

Details

(Whiteboard: [sg:investigate])

Attachments

(2 files, 2 obsolete files)

automated testcase that doesn't require any user action
495 bytes, text/html
Details
Stack to the erroneous focus unsuppression
12.61 KB, text/plain
Details 
Steps to reproduce:
1. Go to
2. Paste this code into the top frame:

<body>
<script>
window.onblur = function(e) 
{
  if(("" + e.target).indexOf("XUL")!=-1)
    alert("!!! " + e.target)
  else
    alert("" + e.target);
}
</script>

3. Click on the bottom frame.
4. Click on the top frame.

Result: two alerts appear, the second showing that the script got a reference to
a XULDocument.  This XULDocument seems to be part of the browser.

Expected: only one alert should appear.

This is probably the cause of bug 112294, "calling an alert box from
window.onblur triggers the event multiple times".

Tested in Firebird 05/22 and Firebird 06/21 on Windows XP.
cc bryner so he can see this bug
Still happens in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3)
Gecko/20041021 Firefox/1.0.  I have to click the inner frame and then the outer
frame in the testcase.
Is this a vulnerability requiring the confidential flag? CAPS seems to protect
getting or setting any of the XULDocument properties I poked at. Definitely a
bug though, and might suggest places where chrome objects are similarly leaked.
Whiteboard: [sg:fix]
Deer park: 
-On mac got three alerts -- html document, null, html document.
-On winXP got the original two alerts: html document, xul document.
Whiteboard: [sg:fix] → [sg:investigate]
Flags: testcase+
Depends on: 339470
The testcase is crashing now after the first alert in current trunk builds, I filed bug 339470 for that.
Assignee: security-bugs → dveditz
QA Contact: carosendahl → toolkit
Flags: in-testsuite+ → in-testsuite?
I can't reproduce on Mac, but this needs to be retested on Windows before it can be marked as WFM.
I crash with a 2007-03-01 build, but with a 2007-03-02 build, I don't crash anymore and I don't see the XULDocument alerting anymore.
So I guess this was indeed fixed on windows somehow. Not sure if it was fixed by the patch from bug 339470.

But in current windows trunk builds, no alert fires at all, I filed bug 392148 for that.
Depends on: 392148
Should this be a blocker? Sounds scary.

Smaug or Ere, do you have any ideas here?
Flags: blocking1.9?
Ere, please see above comment.
Sounds similar to the other symptom in bug 392148. Maybe it should be actually moved here. Here it is: 

If you make the blur event fire by clicking another window you enter an endless loop of popups. It's not a mouse trailer issue as I originally thought but something going seriously wrong. With the test case from bug 392148 I also get this assertion: 

###!!! ASSERTION: Attempt to decrement focus controller's suppression when no
suppression active!

Could someone mark bug 392148 security sensitive?
Assignee: dveditz → emaijala
Flags: blocking1.9? → blocking1.9+
Ere, would you be able to have a stab at this one? Or would Smaug be a better candidate? It sounds scary, so it would be great to have fixed.
I'll take a look, but Smaug might have more insight on this. 
The attached testcase also shows the other problem where the alerts just duplicate if they're triggered by focusing another app.
Seems to work as badly in Linux.
OS: Windows XP → All
Here's a call stack leading to the erroneous focus unsuppression. As far as I can tell it seems the following happens:

1. Focusing another window fires the first blur event.
2. The blur event creates the alert dialog.
3. When the alert dialog is loaded and gets focus, second blur is fired for the old content, which is already processing the first blur.

Not sure about the above either, so don't take it as granted. I'll do some more digging, but Smaug, feel free to help here too as I'm not sure I can get much further in any reasonable amount of time.
I'll look at this too, hopefully within the next few days.
On Linux/trunk I never get alert with "XULDocument". The target of the event is
either the window or the document of iframe. To get the alerts I need to, for example, first focus iframe and then the main document and then open a new window and focus that.
Attached patch Patch to break the loop (obsolete) — — Splinter Review 
This patch, modeled along the way the same is done for GOTFOCUS and LOSTFOCUS in nsEventStateManager::SendFocusBlur, prevents the eternal alert loop when switching to another app. Doesn't fix non-firing of events or the focus suppression alert.
The code path causing the assertion:

1. Processing of DEACTIVATE begins
2. It causes another window to ACTIVATE
3. When ACTIVATE is done, nsEventStateManager::PreHandleEvent unsuppresses until not suppressed
4. When deactivate is processed, it unsuppresses unconditionally causing the assertion

Shall we make DEACTIVATE unsuppression conditional too?
If there are ways to actually exploit this we need to up the priority, please let us know if so. (we'll take your word for it, no testcases needed).

Ere, that said, is this patch ready for review?
Priority: -- → P4
I'll add some code to fix the assertion and post a new patch.
This patch contains the same loop-breaker as the previous one and removes the assertion from focus controller. I first thought I'd make the unsuppression conditional in event state manager, but there were other cases that could have triggered it too (such as SendFocusBlur using a nsFocusSuppressor and causing activation) so I just removed the assertion.
Attachment #283915 - Attachment is obsolete: true
Attachment #287853 - Flags: review?
Comment on attachment 287853 [details] [diff] [review]
Patch to fix the loop and remove the assertion

I think you wanted someone to review this.
Attachment #287853 - Flags: review? → review?(Olli.Pettay)
Comment on attachment 287853 [details] [diff] [review]
Patch to fix the loop and remove the assertion

Uh.. that's what I tried, but I always miss the message about multiple matches...
There seems to be an additional problem that causes a modal dialog loop without a dialog if the alert is caused by closing the last window. The previous patch is still valid, but this problem needs to be addressed too.
As I said in comment 19, I don't see XULDocument as target. Does that happen
somehow on Windows?
The patch fixes possible loop, but I don't know how to reproduce that on linux 
using the testcase.
(I tried having several ff windows or ff+some other app, etc.)

The patch as such makes sense (expect mFirstDocumentBlurEvent should be
renamed), but is it really for this bug? This bug is about "alert() in onblur triggers onblur with XULDocument target ", and that is something scary.
Having a loop/DoS isn't nearly as bad.

I originally thought the loop and the XULDocument as target might have something in common, that's why it's here. I can split it and the other DoS stuff into a new bug if you think that's better. I can't reproduce the XUL target either, so there must have been something else too affecting this. This was indeed confirmed by Martijn in comment #8. Such as mess..
New bug with a testcase and a patch would be great. I could review that patch.
This could be marked WFM, I think, unless someone finds the bug that fixed this.
When fixing bug 392148 it is perhaps better to make sure that this one doesn't regress.
Attachment #287853 - Attachment is obsolete: true
Attachment #287853 - Flags: review?(Olli.Pettay)
There's actually bug 112294. I'll continue there. Marking this WFM.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → WORKSFORME
It would be great if we got a testcase for this checked in. This is especially true since it's a potential security problem that we don't really know how it got fixed and so might regress.
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: