684555 - (CVE-2012-0454) Exploitable "use after free" @ SHLWAPI!IUnknown_QueryService 0x3b

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Blair Strang]

Attachment: POC files to reproduce bug, along with some screenshots

Under windows 7 32-bit, a crash can be caused with the attached code.  

The simplest POC code includes a parent and one child window.  The parent window spawns the child and after a short delay invokes a 'file open' dialog from the child.  Then the parent closes the child window.  This causes an exploitable crash. 

Pre-requisites:

* JavaScript enabled
* Ability to create at least one pop-up window (not a tab)
       [browser.link.open_newwindow -> 2]  [or a popup blocker bypass]
* Firefox 5, 6 or Nightly [other versions not tested]

In the basic POC, the faulting instruction is a CALL to freed memory:

SHLWAPI!IUnknown_QueryService+0x3b:
76d842a1 ff11            call    dword ptr [ecx]
ds:0023:feeefeee=????????

Note that 0xfeeefeee is what is placed into Windows memory which has been HeapFree'd.  

We are also attaching an exploit POC; this uses two child windows.

Due to certain allocation sequences, it turns out that 0x10693AEA ends up being in ECX about 30% of the time.  Believe it or not, this address comes from the binary representation of the GUID for "my computer" {20d04fe0-3aea-1069-a2d8-08002b30309d} which ends up in the memory where ECX gets loaded.  So a stable pointer value turns up in ECX quite often.  On 32-bit Windows 7, a heap spray can arrange for page-aligned data to turn up at 0x10693AEA.

With more work on heap feng shui (i.e, using javascript to control memory allocations) it may be possible to get 0x10693AEA into ECX more reliably or in fact directly control the value which goes into ECX.

The exploit POC should show 0x0c0c0c0c at the top of the call stack.  We have got code execution on Windows 7 with DEP turned off; it is also possible to get this to work on stock  Windows 7 using a heap-based ROP.

We were only able to reproduce this issue on Windows 7 32-bit (XP and Win7 64 did not crash).  Non-windows operating systems should not be vulnerable.

Comment 1

[Blair Strang]

Could someone with permission please add [e-mail] and [e-mail] to the CC list?  We are the reporters. Thanks.

Comment 2

[Benjamin Smedberg]

Do you have crash report IDs? [e-mail] does not appear to have a bugzilla account.

Comment 3

[Blair Strang]

Hi, Scott has created a bugzilla account.  

I submitted "b01bd2db-fcb7-481f-b1aa-037102110904" from the simple POC. The IUnknown_QueryService signature has a reasonable number of crashes and I would hazard a guess that they all have the same root cause.  The comments refer to file save/open dialogs and the crash only happens on 32-bit Windows 7.

Comment 4

[Daniel Veditz [:dveditz]]

Guessing sg:critical if we can reproduce this. Start with the DOM team because form/file upload controls are involved.

Comment 6

[Mounir Lamouri (:mounir)]

So I can't reproduce this on GNU/Linux (with GTK) and MacOS X. It seems reproducible on Windows 7. Though, to reproduce this bug you have to enable the popup blocker and change browser.link.open_newwindow pref to '2'. It seems to reduce the seriousness of the issue.

Likely, the crash cause is somewhere in Windows widget code instead of the DOM code.

Comment 7

[Scott Bell]

Attachment: PoC (works with pop-up blocker enabled)

This PoC works with 'browser.link.open_newwindow' set to 3 (open in tab by default) and with pop-up blocker enabled.

Comment 8

[Scott Bell]

I have attached another PoC which works with 'browser.link.open_newwindow -> 3' (default, open new links in tab) and with the pop-up blocker enabled.  The original setting where 'browser.link.open_newwindow -> 2' turns out not to be required.  We have also reproduced this on Win7-64 bit.

Comment 9

[Mounir Lamouri (:mounir)]

Jst, I believe it would be better to assign this to someone who knows Windows Widget code. I'm unassigning myself so the bug will likely shows up in some list. Though, if you still want me to work on it, feel free to re-assign me.

Comment 10

[Daniel Veditz [:dveditz]]

Jim or Brian: is this Windows security bug something one of you can look into?

Comment 11

[Brian R. Bondy [:bbondy]]

I can reproduce, I'll take a look.

Comment 12

[Jim Mathies [:jimm]]

Looks like this has been around for some time:

https://crash-stats.mozilla.com/report/list?product=Firefox&platform=windows&query_search=signature&query_type=contains&query=IUnknown_QueryService&reason_type=contains&date=09%2F28%2F2011%2008%3A28%3A00&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=IUnknown_QueryService

Comment 13

[Brian R. Bondy [:bbondy]]

From what I've seen is that the crash happens on DispatchMessag for a message of WM_TIMER.  I'm still investigating if this is accurate or not and if so which window.

Comment 14

[Johnny Stenback (:jst)]

Brian, any updates here?

Comment 15

[Brian R. Bondy [:bbondy]]

I've been deep in some silent update service work, I'll take a break on that Monday and work on this instead if that's ok.

Comment 16

[Brian R. Bondy [:bbondy]]

err Tuesday

Comment 17

[Brian R. Bondy [:bbondy]]

Spent about a day on this on Tuesday and getting much closer but not yet fixed.  Will fit another day in this coming week and hopefully finish.

Comment 18

[Daniel Veditz [:dveditz]]

Can't reproduce using 3.6.23 on a 32-bit Windows 7 VM.

Comment 19

[Johnny Stenback (:jst)]

This is looking like it won't be fixed in time for 8.

Brian, any updates on this?

Comment 20

[Brian R. Bondy [:bbondy]]

I spent another night on it after work this week but did not make much progress.
I don't think I'll have much time to look at it before Nov 8 because I'm working on getting a huge 20 patch task (bug 481815) into v10 for silent update.

If Jim or anyone else would like to pick up this task in the meantime that would be great.
I'll send a follow up email to Jim asking him specificaly if he has any time to pick it up.

Here are some notes I collected while debugging:

- The crash happens on DispatchMessage 
- There is no window title on the HWND that it is being dispatched to.
- The window class is WorkerW
- There are 4 messages of exactly WM_APP sent, and one WM_TIMER (wParam 1, lParam 0) before the crash, no other messages are sent to that window.
- The crash happens on a 6th message of type WM_TIMER which has (wParam 2, lParam 0)
- There is a special case inside the remarks of the documentation of DispatchMessage that WM_TIMER with lParam non NULL will call the function of the lParam.  But in our crashing case lParam is non NULL.
- IsWindow returns TRUE just before the crash on that handle
- It doesn't seem to be anything we're doing obviously wrong, except maybe some kind of memory corruption like freeing twice. 
- One way to fix might be to manually close all children window on Destroy() of nsWindow.

Comment 21

[Jim Mathies [:jimm]]

I'll poke at this tomorrow, see what I can come up with.

Comment 22

[Brian R. Bondy [:bbondy]]

Great, thanks Jim

Comment 23

[Jim Mathies [:jimm]]

This seems similar to bug 487700. There we had focus problems with alerts (before we removed the native version) created by popups that were closed. The window tear down happens async, leaving the windows dialog around.

Just starting to look at this specific case, will post what I find.

Comment 24

[Jim Mathies [:jimm]]

Regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e1d55bbd1d1d&tochange=6e3f6d18c124

Bug 130078 most likely, but that doesn't help much.

Comment 25

[Brian R. Bondy [:bbondy]]

I'm setting assigned to you for now Jim since you will be ( or are )  looking at this.  If you don't have time I can re-look at this after silent updates.

Comment 26

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #25)
> I'm setting assigned to you for now Jim since you will be ( or are ) 
> looking at this.  If you don't have time I can re-look at this after silent
> updates.

I haven't found a fix for this yet, and now I'm side tracked on something for the rest of this week. will get back to it next week.

Comment 27

[Johnny Stenback (:jst)]

Jim, any updates here?

Comment 28

[Jim Mathies [:jimm]]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #27)
> Jim, any updates here?

Not yet. I was hoping to get back to this this week but haven't gotten there yet, Maybe tomorrow or Monday. I think it's fixable, it's just a matter of figuring out what tweak we need to avoid it.

Comment 29

[Jim Mathies [:jimm]]

sorry for the delay, I'm back on this as of now.

Comment 30

[Jim Mathies [:jimm]]

Some notes -

We make the api call to display the dialog and after entering the internal modal event loop within the call we call DestroyWindow on the parent.

The dialog call then exits, the parent nsWindow and the file picker get torn down, and that's it until a spurious message is delivered to a child window that should be in the now destroyed picker dialog. Note though, the dialog call exit is actually an exception in Windows code caught by the try/catch block we have this call wrapped in. If I remove the try/catch trap shell32 generates an exception much sooner, fx crashes, and that's it. 

I've done some experimenting to isolate all the buffer allocation work we do in file picker to be sure that isn't part of the problem. This didn't help, so it doesn't look like our quick tear down of the file picker is the issue.

So it seems what we are doing is unanticipated by Windows and there's not much we can do in file picker to prevent this.

Comment 31

[Jim Mathies [:jimm]]

Attachment: widget fix v.1

I spent some time looking for ways to avoid this at a higher level but didn't find anything. So I decided to put this together. This is a bit of a hack but it works. I'm going to do a bit more investigating tomorrow, if I don't find a better way I'll kick off reviews on this patch.

Comment 32

[Jim Mathies [:jimm]]

After some additional experimenting I think the first patch is the solution we should go with for now. Caveat here is that we are trying to address a security related issue, so a minor corner case trade off is acceptable. The side effect of this patch is that it doesn’t automatically close the picker with the underlying browser window – the picker hangs around until the user closes it. Since this is a corner case which would involve bad content design it’s not something users would run into, and it’s easily overcome by simply closing the picker. The other negative side effect is that without auto-closing the picker I can’t write an automated test for this. The good news is it prevents the exception from happening mitigating the vulnerability.

There is some additional good news - we can fix the auto-close picker problem, but the fix relies on the fix for bug 661991.  The core problem here is that we don’t have a way to get at the HWND of the picker from the hidden shim child or browser window we know about due to idiosyncrasies in the way Windows handles parenting of these dialogs. There’s a clean way of getting at the HWND we need but it requires the use of callback procedures built into these dialogs. Unfortunately due to some issues with the older calls we make, we can’t leverage these callbacks on Vista and up. bug 661991 is all about upgrading our calls so we can start using those procedures, in which case we can add auto-close functionality and a test to our work there.

Depending on our plans for landing of this fix the auto-close problem may not be an issue - I’m going to push the priority of bug 661991 up so that we get it out in the Dec. 20th merge. If we plan on running this fix through a full release cycle, we’ll be able to fix the remaining issues before then. If however we were thinking of pushing this out to aurora it’ll have to stand on it’s own for at least one release cycle.

I need to do some cleanup and run this through try, then I'll kick off a review from bbondy and an sr from roc.

Comment 33

[Brian R. Bondy [:bbondy]]

I haven't looked at the patch yet, but does this keep the file picker open even when you right click on the taskbbar and click to close that way? I.e. when the filepicker is already shown? If so that seems like a pretty big problem.

Also it seems we can try to make a hook callback fix in Windows XP as of this patch, we just can't use it for Vista and later.  For bug 661991 that will only help Vista and later.

Comment 34

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #33)
> I haven't looked at the patch yet, but does this keep the file picker open
> even when you right click on the taskbbar and click to close that way? I.e.
> when the filepicker is already shown? If so that seems like a pretty big
> problem.
> 

If the window with the form is visible and has a file picker open, windows prevents you from closing it via the taskbar. (This from testing using the thumbnail close button on Win7 for the browser window.) Generally the picker is modal, so you can't usually get to the underlying window. Except of course through script.

> Also it seems we can try to make a hook callback fix in Windows XP as of
> this patch, we just can't use it for Vista and later.  For bug 661991 that
> will only help Vista and later.

Thought about adding this but decided it wasn't worth adding to the complexity of this fix just to address the issue on XP.

Comment 35

[Jim Mathies [:jimm]]

Also, I should add, if you close the main window (in this poc, the window with 'Click Me' in it) and let script close the form browser, you are left with the file picker and nothing in the taskbar to click on. That's the one ugly side effect from a user's perspective if perchance they somehow landed on a site that resulted in this happening.

Comment 36

[Brian R. Bondy [:bbondy]]

I probably don't fully understand the scope of the side effect yet until I review the patch, but will we want the side effect to live in XP forever? 

I thought something crazy like > 50% of our users are XP.  If the side effect is only in cases similar to steps to reproduce for this bug (i.e. only via script closing), then I don't think it's a big deal.

Comment 37

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #36)
> I probably don't fully understand the scope of the side effect yet until I
> review the patch, but will we want the side effect to live in XP forever? 
> 
> I thought something crazy like > 50% of our users are XP.  If the side
> effect is only in cases similar to steps to reproduce for this bug (i.e.
> only via script closing), then I don't think it's a big deal.

I was thinking we could fix both, since on XP we have the callbacks and can use them to address the problem. I just didn't want to add that complexity to the patch in this bug.

Comment 38

[Brian R. Bondy [:bbondy]]

OK cool. The XP fix can be posted as a separate bug and not being blocked by anything in that case.

Comment 39

[Jim Mathies [:jimm]]

Attachment: patch v.1

This passed try fine.

Comment 40

[Brian R. Bondy [:bbondy]]

Comment on attachment 578239 [details] [diff] [review]
patch v.1

Review of attachment 578239 [details] [diff] [review]:
-----------------------------------------------------------------

> If the window with the form is visible and has a file picker open, windows prevents 
> you from closing it via the taskbar. (This from testing using the thumbnail close 
> button on Win7 for the browser window.) Generally the picker is modal, so you 
> can't usually get to the underlying window. Except of course through script.

Not fully true, see below.

If the "Show tabpreviews in the Windows taskbar" is on, you can close the parent window without the child filepicker even know it is Modal in 2 ways:
1) By using the close button on the preview.
2) By right clicking on the preview and selecting close.

So that problem can happen even outside of scripting.  
This is not on by default in FF though, so it's not a huge deal.  A lot of people use it though.
I think the trade off is fine and I think it will probably be a non issue anyway since the other bug will likely land before the 20th.

::: widget/src/windows/nsFilePicker.cpp
@@ +112,5 @@
>    }
>  }
>  
> +// Callback hook which will ensure that the window is visible. Currently
> +// only in use on XP.

nit: XP and below.

@@ +136,5 @@
>  }
>  
>  
>  // Callback hook which will dynamically allocate a buffer large
> +// enough for the file picker dialog.  Currently only in use on XP.

nit: XP and below.

@@ +210,5 @@
> +  
> +bool nsFilePicker::GetFileName(OPENFILENAMEW* ofn, PickerType aType)
> +{
> +  if (!ofn)
> +    return false;

nit: NS_ENSURE_ARG_POINTER(ofn);

@@ +217,5 @@
> +  nsWindow* win = static_cast<nsWindow*>(mParentWidget.get());
> +  MOZ_SEH_TRY {
> +    if (win) {
> +      win->PickerOpen();
> +    }

nit: Move PickerOpen() above the SEH try.

@@ +225,5 @@
> +    else if (aType == PICKER_TYPE_SAVE)
> +      result = ::GetSaveFileNameW(ofn);
> +
> +  } MOZ_SEH_EXCEPT(true) {
> +    NS_ERROR("nsFilePicker GetFileName win32 calls generated an exception! This is bad!");

nit: calls -> call
nit2: I think you should be setting result to false here.

@@ +387,4 @@
>        if (mMode == modeOpen) {
>          // FILE MUST EXIST!
>          ofn.Flags |= OFN_FILEMUSTEXIST;
> +        result = GetFileName(&ofn, PICKER_TYPE_OPEN);

nit: There is no reason for all of this to be inside of a surrounding { }.  Pls remove and unindent.

Comment 41

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #40)
> ::: widget/src/windows/nsFilePicker.cpp
> @@ +210,5 @@
> >        if (mMode == modeOpen) {
> >          // FILE MUST EXIST!
> >          ofn.Flags |= OFN_FILEMUSTEXIST;
> > +        result = GetFileName(&ofn, PICKER_TYPE_OPEN);
> 
> nit: There is no reason for all of this to be inside of a surrounding { }. 
> Pls remove and unindent.

I didn't want to blow away blame, but since I've ended up reformatting the whole thing in bug 661991 it doesn't make any difference. Will update.

Comment 42

[Brian R. Bondy [:bbondy]]

OK cool, I wasn't sure if the code would survive as is for the XP case.  Gavin recently commented on one of my bugs that I should never let hg blame stop me from fixing whitespace here: https://bugzilla.mozilla.org/show_bug.cgi?id=699567#c15

Comment 43

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1236e039a9e3

Comment 44

[Ian Melven :imelven]

via edmorley: 

https://hg.mozilla.org/mozilla-central/rev/1236e039a9e3

marking fixed with target milestone of FF11

Comment 45

[Johnny Stenback (:jst)]

Jim, what are your thoughts on landing this for 9? We'd love to fix this ASAP, but I fear we're too late to get appropriate testing for this before release. I'd love to hear your thoughts on the risk/reward of this change?

Comment 46

[Jim Mathies [:jimm]]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #45)
> Jim, what are your thoughts on landing this for 9? We'd love to fix this
> ASAP, but I fear we're too late to get appropriate testing for this before
> release. I'd love to hear your thoughts on the risk/reward of this change?

The patch is pretty safe, it involves some innocuous trickery in widget when we open the picker. It would be better to roll this out with the patches in bug 661991 though if we could. See comment 32 for the detail as to why.

Comment 47

[Jim Mathies [:jimm]]

Attachment: picker close crash test

Posting this here. This can land after the patches in bug 661991 do.

Comment 48

[Jim Mathies [:jimm]]

Bah, that had a couple tabs in the html. I've cleaned that up locally.

Comment 49

[Jim Mathies [:jimm]]

Comment on attachment 581023 [details] [diff] [review]
picker close crash test

This failed on one out of four test runs (xp debug). I need to figure out how to make this more reliable.

Comment 50

[Brian R. Bondy [:bbondy]]

k I looked at the patch already, it looked good but I wanted to run it and do another pass.  Pls re- r? when ready.

Comment 51

[Jim Mathies [:jimm]]

Attachment: picker close crash test v.2

improved test

Comment 52

[Jim Mathies [:jimm]]

Comment on attachment 581337 [details] [diff] [review]
picker close crash test v.2

This version of the test passed a couple try runs fine.

Comment 53

[Brian R. Bondy [:bbondy]]

Comment on attachment 581337 [details] [diff] [review]
picker close crash test v.2

As discussed on IRC pls post a rollup or wait until the dependencies on the test are in on m-c .  Pls re r? me after one of those.

Comment 54

[Brian R. Bondy [:bbondy]]

Comment on attachment 581337 [details] [diff] [review]
picker close crash test v.2

Review of attachment 581337 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Tested it and it works, and code looks good.

nit: Pls fix the Windows line endings.

You can put this in your c:\users\username\mercurial.ini :

> [hooks]
> # Reject commits which would introduce windows-style text" files
> pretxncommit.crlf = python:hgext.win32text.forbidcrlf

And it won't allow you to create patches nor commit anything with crlf.

Comment 55

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/5131c0b1982f

Comment 56

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on http://hg.mozilla.org/releases/mozilla-esr10/by Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.

Comment 57

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage comment]
Confirmed with dveditz that esr10 is affected by this bug and requires this patch to go to build - can someone please land today or tomorrow?  Let me know if there are any concerns we should be aware of.

Comment 58

[Jim Mathies [:jimm]]

I can.

Comment 59

[Jim Mathies [:jimm]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/9cc3cf1e6c36
https://hg.mozilla.org/releases/mozilla-esr10/rev/de4a17c2292a

Comment 60

[Al Billings [:abillings - ex-MoCo]]

I've verified this is fixed in beta 4 of Firefox 11 and nightly Aurora (Firefox 12) from 2/27 with the second PoC. The original PoC wasn't causing a visible issue on Win 7 SP1 32-bit. Firefox 10.0.2 still crashes with the second PoC with bp-1ddce430-2ad1-48a5-bce6-2ddfa2120228.

I've also verified this as fixed in the trunk build from 2/27.

Comment 61

[Al Billings [:abillings - ex-MoCo]]

Latest 10esr build was built from before this so I couldn't check there.

Comment 62

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Al Billings [:abillings] from comment #61)
> Latest 10esr build was built from before this so I couldn't check there.

can you check here: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-esr10/

Comment 63

[Jim Mathies [:jimm]]

Bah, per comment 32 I should not have landed that test on esr. I'll back that out.

Comment 64

[Jim Mathies [:jimm]]

patch backed out:

https://hg.mozilla.org/releases/mozilla-esr10/rev/de4a17c2292a

landings:

https://hg.mozilla.org/releases/mozilla-esr10/rev/794ff89ae0c9
https://hg.mozilla.org/releases/mozilla-esr10/rev/933538545cce

Comment 65

[Al Billings [:abillings - ex-MoCo]]

No crash with the current ESR build post landing (Mozilla/5.0 (Windows NT 6.1; rv:10.0.2) Gecko/20120302 Firefox/10.0.2).