1129209 - The updater.exe loads the SxS comctl32.dll from the updater.exe.Local directory when not using the service (Application Update)

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

For example, on Win 8.1 it will load
updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.9600.17031_none_a9efdb8b01377ea7\comctl32.dll

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

I still need to perform additional testing but so far the testing shows that this fixes this bug.

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 8558804 [details] [diff] [review]
patch rev1

Doesn't fix Vista and visual styles aren't used on XP with this patch :(

Comment 3

[Robert Strong (they/them - no direct email)]

I think I figured out what is going on with XP.

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

Fixes visual styles on WinXP.

With this patch the SxS comctl32.dll under an updater.exe.Local directory alongside updater.exe is not loaded on Win8 and is only loaded in the unelevated process on Win7. WinXP and WinVista are still loaded.

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 8558890 [details] [diff] [review]
patch rev2

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

::: toolkit/mozapps/update/updater/progressui_win.cpp
@@ +250,5 @@
> +  if (!GetModuleFileNameW(nullptr, appPath, MAX_PATH)) {
> +    return -1;
> +  }
> +
> +  NS_tstrcat(appPath, L".Local");

Pls use PathAppendSafe from updatehelper.h on false return, then return -1.

@@ +274,5 @@
> +  // This is needed only for Win XP but doesn't cause a problem with other
> +  // versions of Windows.
> +  actx.lpSource = appPath;
> +  // 17 is the first unreserved manifest resource.
> +  actx.lpResourceName = MAKEINTRESOURCE(17);

More common to put this as a define in a resource header and use the name here.

Also I don't think making the filename have a 17 in it, has any value to people reading it, and makes them think: "Why does this have a \"-17\" in it?".  So they go spend time and find out, oh it's because the resource number is 17, ok. But then they can do nothing with that extra knowledge.  Could be avoided with a more descriptive filename altogether.

Comment 6

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> Comment on attachment 8558890 [details] [diff] [review]
> patch rev2
> 
> Review of attachment 8558890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/updater/progressui_win.cpp
> @@ +250,5 @@
> > +  if (!GetModuleFileNameW(nullptr, appPath, MAX_PATH)) {
> > +    return -1;
> > +  }
> > +
> > +  NS_tstrcat(appPath, L".Local");
> 
> Pls use PathAppendSafe from updatehelper.h on false return, then return -1.
From the msdn page on PatheAppendW which is used by PathAppendSafe.
"This function automatically inserts a backslash between the two strings, if one is not already present."

I'll add a check similar to PathAppendSafe to get the same result.

Comment 7

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #6)
> (In reply to Brian R. Bondy [:bbondy] from comment #5)
> > Comment on attachment 8558890 [details] [diff] [review]
> > patch rev2
> > 
> > Review of attachment 8558890 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/mozapps/update/updater/progressui_win.cpp
> > @@ +250,5 @@
> > > +  if (!GetModuleFileNameW(nullptr, appPath, MAX_PATH)) {
> > > +    return -1;
> > > +  }
> > > +
> > > +  NS_tstrcat(appPath, L".Local");
> > 
> > Pls use PathAppendSafe from updatehelper.h on false return, then return -1.
> From the msdn page on PatheAppendW which is used by PathAppendSafe.
> "This function automatically inserts a backslash between the two strings, if
> one is not already present."
> 
> I'll add a check similar to PathAppendSafe to get the same result.

The main motivation for that was just about buffer size checking.

Comment 8

[Robert Strong (they/them - no direct email)]

Loading of comctl32.dll under updater.exe.Local directory with most OS's with the patch applied:
Win XP x64 with Firefox x86 running as admin since UAC is not available - loaded
Win Vista x64 with Firefox x86 - loaded in the unelevated and elevated process
Win 7 x86 with Firefox x86 - only loaded in the unelevated process
Win 7 x64 with Firefox x86 and Firefox x64 - only loaded in the unelevated process
Win 8 x86 with Firefox x86 - not loaded
Win 8.1 x64 with Firefox x86 and Firefox x64 - not loaded

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch for beta and aurora (already landed on nightly)

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 8559383 [details] [diff] [review]
patch for beta and aurora (already landed on nightly)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Somewhat easy though less so than the patch for the similar issue in bug 945192

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Somewhat though once again less so than bug 945192

Which older supported branches are affected by this flaw?
All

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I haven't checked if it cleanly applies yet but it is trivial to create backports.

How likely is this patch to cause regressions; how much testing does it need?
It is unlikely to create regressions. I'd like this to be on Nightly for a few days before uplifting just in case though I have very few concerns about it regressing.

Note: since this is not a complete fix like bug 945192 I'd like to not open this bug to the public until after I have changed the update process to run out of the application directory when not using the service in bug 1127481.

Comment 11

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

Comment on attachment 8559383 [details] [diff] [review]
patch for beta and aurora (already landed on nightly)

sec-approval+. I'd like to see this on Aurora and Beta in time for next week's beta if possible. I assume we should take this on ESR31?

Comment 12

[Robert Strong (they/them - no direct email)]

(In reply to Al Billings [:abillings] from comment #11)
> Comment on attachment 8559383 [details] [diff] [review]
> patch
> 
> sec-approval+. I'd like to see this on Aurora and Beta in time for next
> week's beta if possible. I assume we should take this on ESR31?
Yes to both and I can land this over the weekend.

Comment 13

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/6fcfaa95e84a

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch esr31

Only had to remove the test change which isn't needed.

Comment 15

[Robert Strong (they/them - no direct email)]

I just tested all OS's except the Win10 preview with the following results:

Win XP x86 - Firefox x86
updates\0\LPK.DLL - loads before preloading - elevated since there is no UAC
updates\0\USP10.dll - loads before preloading - elevated since there is no UAC
updates\0\updater.exe.Local\x86_Microsoft.Windows.Common-Controls_6595b64144ccf1df_6.0.2600.6028_x-ww_61e65202\comctl32.dll - elevated since there is no UAC

Win XP x64 - Firefox x86
updates\0\wow64log.dll - loads before preloading - elevated since there is no UAC
updates\0\updater.exe.Local\WOW64_Microsoft.Windows.Common-Controls_6595b64144ccf1df_6.0.3790.3959_x-ww_5FA17F4E\comctl32.dll - elevated since there is no UAC

Win Vista x86 - Firefox x86
updates\0\apphelp.dll - loads before preloading - unelevated and elevated
updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.6002.18005_none_5cb72f96088b0de0\comctl32.dll - unelevated and elevated

Win Vista x64 - Firefox x86
updates\0\apphelp.dll - loads before preloading - unelevated and elevated
updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.6002.18005_none_5cb72f96088b0de0\comctl32.dll - unelevated and elevated

Win 7 x86 - Firefox x86
updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.7601.17514_none_41e6975e2bd6f2b2\comctl32.dll - only unelevated

Win 7 x64 - Firefox x86
updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.7601.17514_none_41e6975e2bd6f2b2\comctl32.dll - only unelevated

Win 7 x64 - Firefox x64
updates\0\updater.exe.Local\amd64_microsoft.windows.common-controls_6595b64144ccf1df_6.0.7601.17514_none_fa396087175ac9ac\comctl32.dll - only unelevated

Win 8 x86 - Firefox x86
none

Win 8 x64 - Firefox x86
none

Win 8 x64 - Firefox x64
none

Win 8.1 x86 - Firefox x86
none

Win 8.1 x64 - Firefox x86
none

Win 8.1 x64 - Firefox x64
none

I'm going to test on the Win 10 tech preview as well

Comment 16

[Robert Strong (they/them - no direct email)]

As expected, I got the same results with KB2533623 applied to Win Vista and Win 7.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fcfaa95e84a

Comment 18

[Robert Strong (they/them - no direct email)]

Win 10 Tech Preview x86 - Firefox x86
none

Win 10 Tech Preview x64 - Firefox x86
none

Win 10 Tech Preview x64 - Firefox x64
none

Comment 19

[Robert Strong (they/them - no direct email)]

I tested all of the OS's using an updater.exe.Local file as well and the only difference is that the comctl32.dll was only loaded from the windows dir.

Comment 20

[Robert Strong (they/them - no direct email)]

Kamil, the results are detailed in comment #15 through comment #19.

I'm especially interested in if proxy dll's for apphelp.dll and comctl32.dll on Win Vista launches both an unelevated and elevated cmd prompt on Vista and that a proxy dll for comctl32.dll only launches an unelevated cmd prompt on Win 7.

Comment 21

[Robert Strong (they/them - no direct email)]

Also, you can verify bug 945192 at the same time as this bug.

Comment 22

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0a33db91743

Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/e07bd3f1fd4b

Pushed to mozilla-esr31
https://hg.mozilla.org/releases/mozilla-esr31/rev/a28919844cc4

Comment 23

[Kamil Jozwiak [:kjozwiak]]

Updated to the latest m-c from the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-02-09-03-02-04-mozilla-central/

Win XP Pro SP3 x86
==================

* C:\Documents and Settings\Administrator\Local Settings\Application Data\Mozilla\Firefox\Nightly\updates\0\updater.exe.Local\x86_Microsoft.Windows.Common-Controls_6595b64144ccf1df_6.0.2600.6028_x-ww_61e65202\comctl32.dll
** spawned three cmd prompts but there's no way of checking the integrity level in Windows XP

Win XP Pro SP2 x64:
===================

* C:\Documents and Settings\Administrator\Local Settings\Application Data\Mozilla\Firefox\Nightly\updates\0\SETUPAPI.dll
** no cmd's spawned
* C:\Documents and Settings\Administrator\Local Settings\Application Data\Mozilla\Firefox\Nightly\updates\0\NETAPI32.dll
** no cmd's spawned
* C:\Documents and Settings\Administrator\Local Settings\Application Data\Mozilla\Firefox\Nightly\updates\0\WINSTA.dll
** no cmd's spawned
* C:\Documents and Settings\Administrator\Local Settings\Application Data\Mozilla\Firefox\Nightly\updates\0\wow64log.dll
* C:\Documents and Settings\Administrator\Local Settings\Application Data\Mozilla\Firefox\Nightly\updates\0\updater.exe.Local\WOW64_Microsoft.Windows.Common-Controls_6595b64144ccf1df_6.0.3790.5190_x-ww_B8C2A5B7\comctl32.dll
** spawned one cmd prompt but there's no way of checking the integrity level in Windows XP

Win Vista Ultimate SP2 x86
==========================

* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\Firefox\Nightly\updates\0\apphelp.dll
** crashed the updater and spawns 1 medium integrity cmd prompt (fx doesn't update)
* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\Firefox\Nightly\updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.6002.18305_none_5cb72f2a088b0ed3\comctl32.dll
** spawned 3 medium and 2 high integrity cmd prompts

Win Vista Ultimate SP2 x64
==========================

* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\Firefox\Nightly\updates\0\apphelp.dll
** crashed the updater and spawns 1 medium integrity cmd prompt (fx doesn't update)
* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\Firefox\Nightly\updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.6002.18305_none_5cb72f2a088b0ed3\comctl32.dll
** spawned 3 medium and 2 high integrity cmd prompts

Win 7 Pro SP1 x86
=================

* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\updates\A3710B8EBB50CD3\updates\0\version.DLL
** spawned 1 medium integrity cmd prompt
* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\updates\A3710B8EBB50CD3\updates\0\SSPICLI.DLL
** spawned 1 medium integrity cmd prompt
* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\updates\A3710B8EBB50CD3\updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.7601.17514_none_41e6975e2bd6f2b2\comctl32.dll
** spawned 1 medium integrity cmd prompt

Win 7 Pro SP1 x64
=================

FX x86:
* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\updates\EEFEA8717BC47F65\updates\0\version.DLL
** spawned 1 medium integrity cmd prompt
* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\updates\EEFEA8717BC47F65\updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.7601.17514_none_41e6975e2bd6f2b2\comctl32.dll
** spawned 1 medium integrity cmd prompt

FX x64:
* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\updates\A3710B8EBB50CD3\updates\0\version.DLL
** spawned 1 medium integrity cmd prompt
* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\updates\A3710B8EBB50CD3\updates\0\SSPICLI.DLL
** spawned 1 medium integrity cmd prompt
* C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\updates\A3710B8EBB50CD3\updates\0\updater.exe.Local\amd64_microsoft.windows.common-controls_6595b64144ccf1df_6.0.7601.17514_none_fa396087175ac9ac\comctl32.dll
** spawned 1 medium integrity cmd prompt

Win 8 x86:
==========

* didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

Win 8 x64:
==========

FX x86:
* didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

FX x64:
* didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

Win 8.1 x86:
============

* didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

Win 8.1 x64:
============

FX x86:
* didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

FX x64:
* didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

Comment 24

[Kamil Jozwiak [:kjozwiak]]

Results:

Win XP Pro SP2 x86: [Possible PASSED]

- comctl32.dll launched three cmd prompts but there's no way of checking the integrity level

Win XP Pro SP2 x64: [Possible PASSED]

- Using procmon, I found SETUPAPI.dll, NETAPI32.dll, WINSTA.dll being loaded alongside "Nightly\updates\0\". Created proxy DLL's but didn't receive any cmd prompts while using them
- I couldn't find wow64log.dll anywhere on the machine (tried Win 7 as well without any luck)
- comctl32.dll launched one cmd prompt but there's no way of checking the integrity level in Windows XP (reproduced three times)

Win Vista Ultimate SP2 x86: [FAILED] <- Spawned high integrity cmd prompts

- apphelp.dll crashed the updater and spawns 1 medium integrity cmd prompt (fx doesn't end up updating)
- comctl32.dll spawned 3 medium and 2 high integrity cmd prompts (reproduced three times)

Win Vista Ultimate SP2 x64: [FAILED] <- Spawned high integrity cmd prompts

- apphelp.dll crashed the updater and spawns 1 medium integrity cmd prompt (fx doesn't end up updating)
- comctl32.dll spawned 3 medium and 2 high integrity cmd prompts (reproduced three times)

Win 7 Pro SP1 x86: [PASSED]

- version.DLL spawned 1 medium integrity cmd prompt
- comctl32.dll spawned 1 medium integrity cmd prompt
- comctl32.dll spawned 1 medium integrity cmd prompt

Win 7 Pro SP1 x64: [PASSED]

FX x86:
- version.DLL spawned 1 medium integrity cmd prompt
- comctl32.dll spawned 1 medium integrity cmd prompt

FX x64:
- version.DLL spawned 1 medium integrity cmd prompt
- SSPICLI.DLL spawned 1 medium integrity cmd prompt
- comctl32.dll spawned 1 medium integrity cmd prompt

Win 8 x86: [PASSED]

- didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

Win 8 x64: [PASSED]

FX x86:
- didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

FX x64:
- didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

Win 8.1 x86: [PASSED]

- didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

Win 8.1 x64: [PASSED]

FX x86:
- didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

FX x64:
- didn't find comctl32.dll nor any dlls that are being loaded along side "\updates\0\"

Comment 25

[Robert Strong (they/them - no direct email)]

Thanks Kamil! That is about all of the dll's that I thought bug 945192 and this bug would fix. The remainder will be fixed by bug 1127481.

Comment 26

[Kamil Jozwiak [:kjozwiak]]

Attachment: proxy DLL's (comment # 23)

Robert,

I went through all the OS's using m-c and listed the dll's that I found in comment #23. I've added the results in comment #24 to keep it a bit cleaner.

- Win XP spawns cmd prompts but there's no way of knowing the integrity level as it was introduced in Vista
- apphelp.dll crashes both x86 and x64 Vista and spawns a medium integrity cmd prompt
- I'm still getting high integrity cmd prompts in both x86 and x64 Vista using comctl32.dll
- Win 7 spawns medium integrity cmd's but I'm assuming that's not an issue

Not sure if this is expected on Vista, let me know and I'll quickly finish up Aurora/BETA/ESR as I have all the DLL's and paths so it should be a lot faster now.

I attached the new proxy DLL's that I created for this and also updated the documentation in case we ever need to do this again:
- https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking_via_Update

(using 7zip as the compression is lot better than zip which goes over the 10MB limit)

Comment 27

[Kamil Jozwiak [:kjozwiak]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #25)
> Thanks Kamil! That is about all of the dll's that I thought bug 945192 and
> this bug would fix. The remainder will be fixed by bug 1127481.

Thanks Robert, I'll finish going through Aurora/BETA/ESR tomorrow and test the remaining issues in Bug # 1127481 once completed.

Comment 28

[Robert Strong (they/them - no direct email)]

That is known on Vista per my testing. I don't know of a decent way to fix it on Vista beyond bug 1127481 so that is what will fix Vista as well as all other cases including all medium and high integrity cmd prompts.

I wouldn't be surprised if there is a way to also exploit via apphelp.dll.

Regarding wow64log.dll I created one a while back with info on found via google and there is a sample at http://waleedassar.blogspot.com/2013/01/wow64logdll.html

Comment 29

[Kamil Jozwiak [:kjozwiak]]

Used the following Aurora build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-02-09-00-40-26-mozilla-aurora/

Win XP Pro SP3 x86 --> same results as before
Win XP Pro SP2 x64 --> same results
Win Vista Ultimate SP2 x86 --> same results as before (spawned 2 high integrity cmds)
Win Vista Ultimate SP2 x64 --> same results as before (spawned 2 high integrity cmds)
Win 7 Pro SP1 x86 --> same results as before
Win 7 Pro SP1 x64 --> same results as before

Comment 30

[Kamil Jozwiak [:kjozwiak]]

Used the following BETA build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/36.0b8-candidates/

Win XP Pro SP3 x86 --> same results as before
Win XP Pro SP2 x64 --> same results
Win Vista Ultimate SP2 x86 --> same results as before (spawned 2 high integrity cmds)
Win Vista Ultimate SP2 x64 --> same results as before (spawned 2 high integrity cmds)
Win 7 Pro SP1 x86 --> same results as before
Win 7 Pro SP1 x64 --> same results as before

Comment 31

[Kamil Jozwiak [:kjozwiak]]

Used the following ESR build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-02-09-00-06-35-mozilla-esr31/

Win XP Pro SP3 x86 --> same results as before
Win XP Pro SP2 x64 --> same results 
Win Vista Ultimate SP2 x86 --> same results as before (spawned 2 high integrity cmds)
Win Vista Ultimate SP2 x64 --> same results as before (spawned 2 high integrity cmds)
Win 7 Pro SP1 x86 --> same results as before
Win 7 Pro SP1 x64 --> same results as before

As Robert mentioned in comment # 25, the remaining issues will be fixed in bug # 1127481

Comment 32

[Robert Strong (they/them - no direct email)]

Just to make sure comment 10 was noticed:
Note: since this is not a complete fix like bug 945192 I'd like to not open this bug to the public until after I have changed the update process to run out of the application directory when not using the service in bug 1127481.