945192 - (CVE-2015-0833) The updater.exe loads the bcrypt.dll and other dll's from the working and binary directory when not using the service (Application Update)

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Holger Fuhrmannek]

The updater.exe loads the bcrypt.dll from the update directory, for example while performing an update. That can lead to a privilege escalation while updating without the Mozilla Maintenance Service. I have tested this on win7 pro 32bit with Firefox 25.0.1. 

quick poc 1:
1.) copy the updater.exe from Firefox 25.0.1 and create a "bcrypt.dll" file with no content in the same location
2.) start the updater.exe
3.) a error message will occur, because the update tries to load the empty .dll

Also other .dlls are loaded from the working directory but only the bcrypt.dll is loaded while performing an update without the service from the high integrity updater process.

Comment 1

[Daniel Veditz [:dveditz]]

If this is only without using the maintenance service where does privilege escalation come in? Not disputing that this is unintended, just not sure how it gets used in an attack (but maybe I've misunderstood what you're describing)

Comment 2

[Holger Fuhrmannek]

Look at https://bugzilla.mozilla.org/show_bug.cgi?id=859072#c25, same problem here.

Comment 3

[Daniel Veditz [:dveditz]]

I'm still not getting how this is useful in an attack. If you can copy files around can't you copy or not copy any other file the updater needs or touches?

Comment 4

[Holger Fuhrmannek]

An attack can look as follows (using firefox):
- an attacker has already compromised a system and wants to get admin rights now
- the attacker puts the updater.exe from the ff directory,  an update.status (content: "pending") and update.version (content: "30.0") and a fake bcrypt.dll to 
C:\Users\<username>\AppData\Local\Mozilla\updates\308046B0AF4A39CB\updates\0
- the attacker sets "using the maintenance service for updating" in the firefox configuration to false  if it is enabled
- if the user starts firefox the next time, the update.exe will be execting and an UAC prompt from the updater.exe will appear
- if the user agrees, the attack will be successful, because the bcypt.dll will be executed.

Comment 5

[Brian R. Bondy [:bbondy]]

Hi ash,

Would it be possible to give us a dll that launches something like a cmd or calc.exe as high integrity?

We did some previous testing here: (accessible only internally unfortunately) 
https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking

And bcrypt.dll was found to be loaded, but found to not be an exploit because it doesn't launch an elevated cmd in a dll that simply launched a cmd.

Perhaps for example the dll is loaded as a resource dll and no code is executed from it.

Comment 6

[Holger Fuhrmannek]

I have created the exploit dll.  The exploit dll needs the original dll named "bcrypt_orig.dll" in the same directory because it forwards the calls to the original dll.
If the exploit dll is now placed in the "\updates\0" directory two cmd windows will appear during an update without the service. The first windows will not be elevated but the second. You will need to close the first cmd windows to see the second.

Comment 7

[Holger Fuhrmannek]

Attachment: exploit bcrypt.dll

Comment 8

[Brian R. Bondy [:bbondy]]

Is it possible to also provide source code? Thanks.

Comment 9

[Brian R. Bondy [:bbondy]]

Kamil I have a bit of a project for you or someone else on QA if your manager is OK with it.

Could you verify this dll? If it reproduces, which based on Comment 6 it will, we should retry all the dlls in the intranet link mentioned in Comment 5 with the new PoC dll. We basically need to redo everything in that intranet page but with the new PoC dll. 

For what it's worth nothing in the DLL Hijacking intranet link was tested wrong, I just think the PoC differs in some way that we didn't test.  So we should re-test with the new PoC for all Dlls and on all platforms.

Comment 10

[Holger Fuhrmannek]

The exploit dll need to export the same functions as the original dll. I think that's the difference between the exploit dll and your dll. My dll is auto generate using this tool http://www.codeproject.com/Articles/16541/Create-your-Proxy-DLLs-automatically, i only added system('cmd').

Comment 11

[Kamil Jozwiak [:kjozwiak]]

Brian, so I ran through the test case following the information added in comment #4 and comment #6 and reproduced the problem. Once selected "OK" under the UAC prompt, I received a second cmd.exe that was spawned in "High" integrity.

Comment 12

[Brian R. Bondy [:bbondy]]

Thanks ash and Kamil.

So Kamil, for testing I think we just need an updated list per platform for any dll that is loaded. If it crashes on startup even, then that counts as something we need to protect against. 
Using the new dll won't help with this compared to the old dll. 

Please go through the list that you and others created at ahughes's wiki and make a new list for things we should protect against.  This doesn't require you to go through every OS anymore. Just going through the main list.   

We should check all OS only after we have a fix to this.

Before we weren't considering things that didn't launch a high integrity process.
But as ash found, the calls can be forwarded to the real dll so the process doesn't crash, and yet can still run attack code.

Comment 13

[Kamil Jozwiak [:kjozwiak]]

I'm going to split this into three different tickets to keep it cleaner and easier to follow rather than just pasting everything in here and making it difficult to follow. This ticket will concentrate on the unknown DLL's that are loaded during the Installer Update process with app.update.services being disabled. The progress will be recorded in the following wiki:

https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking_via_Update

Bugs Created to track the regular Installer & Stub Installer:

- Bug #985057 - The updater.exe loads the bcrypt.dll from the working directory (Installer)
- Bug #985059 - The updater.exe loads the bcrypt.dll from the working directory (Stub Installer)

Comment 14

[Kamil Jozwiak [:kjozwiak]]

Quick Update:

- Installed all the needed Win OS's (both x64 and x86 versions)
- Went through each OS and listed all the unknown DLL's (went through this process twice to make sure nothing was missing)
- Once the entire unknown DLL list was created, went through each OS one more time to make sure nothing was missed
- Beginning to create the exploit DLL's using the utility/process mentioned in comment #10 (will be adding those steps in the wiki for future reference)

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

Robert, can you take a look in bbondy's absence?

Comment 16

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

Matt, according to comment #13 and https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking_via_Update it doesn't appear that all of the affected dll's have been identified yet but I might be wrong so I'll needinfo Kamil.

Kamil, have you completed the testing. I noticed that none of the dll's are bold.

Comment 17

[Kamil Jozwiak [:kjozwiak]]

I'm finishing up some things with telemetry experiments as the iteration is ending this Monday. I should be done the two telemetry tickets by the end of this weekend and then I'm planning on tackling this.

I have the environment all setup and just need to start creating the proxy DLL's, planning finishing this up at the end of next week.

Comment 18

[Kamil Jozwiak [:kjozwiak]]

Update: (see link in comment # 13 for DLL's that are exploitable)

* Windows Vista x86: [FOUND 4 DLL's]
* Windows Vista x64: [FOUND 3 DLL's]
* Windows 7 x86: [FOUND 2 DLL's]
* Windows 7 x64: [FOUND 2 DLL's]
* Windows 8 x86: [FOUND 3 DLL's]
* Windows 8 x64: [FOUND 3 DLL's]

Comment 19

[Kamil Jozwiak [:kjozwiak]]

Attachment: Win XP Pro DLL's (x86 & x64)

I finished going through each OS's unknown DLL's when using the "Update Route", I'll start on the Installer/Stub Installer tomorrow. Here are the remaining OS's:

* Windows XP SP3 x86: [FOUND 0 DLL's]
* Windows XP SP2 x64: [Found 5 POSSIBLE DLL's]
* Windows 8.1 x86: [FOUND 2 DLL's]
* Windows 8.1 x64: [FOUND 2 DLL's]

All the DLL's affected can be found in the following document:
- https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking_via_Update

** Note: Windows XP doesn't have integrity levels as it was added in Vista+. This makes it very difficult to know if the CMD that was spawned was in either high or medium integrity. I highlighted every DLL that spawned a CMD. These should be checked to ensure that they're not spawning in high integrity**

I've added 7zip files of all the unknown DLL proxies and their originals for future use. These can be used to reproduce the issue. Each zip file includes x86 and x64 files.

Comment 20

[Kamil Jozwiak [:kjozwiak]]

Attachment: Win Vista Ultimate DLL's (x86 & x64)

Comment 21

[Kamil Jozwiak [:kjozwiak]]

Attachment: Win 7 Pro DLL's (x86 & x64)

Comment 22

[Kamil Jozwiak [:kjozwiak]]

Attachment: Win 8 DLL's (x86 & x64)

Comment 23

[Kamil Jozwiak [:kjozwiak]]

Attachment: Win 8.1 DLL's (x86 & x64)

Comment 24

[Kamil Jozwiak [:kjozwiak]]

Progress:

- DLL Hijacking via Installer - Bug #985057 [COMPLETED]
- DLL Hijacking via Stub Installer - Bug #985059 [INCOMPLETE, SHOULD BE DONE TOMORROW]
- DLL Hijacking via Installer Update - Bug #945192 [COMPLETED]

Comment 25

[Kamil Jozwiak [:kjozwiak]]

Finished going through all the unknown DLL's, updated the appropriate wiki's that I created on the intra. The affected DLL's are highlighted in bold.

- DLL Hijacking via Stub Installer - Bug #985059 [COMPLETED]

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: dlls.diff

This is not for review, but I just wanted to flag you for feedback to see if you agree.

There's a total of 92 DLLs if you collapse each OS into a single directory. 
Each OS has a different combination of DLLs which are exploitable.

Kamil spent a lot of time generating exploitable DLLs for each OS (Each OS needs it's own copy of the original DLL to forward the calls to so it is exploitable instead of crashing). Thank you Kamil, this wouldn't be possible without that work.

The patch attached here fixes the problem on Windows 8.1 (54 DLLs).
Creating this list of DLLs in the patch isn't as simple as just adding a string to the array.
You need to be very careful about DLL load order and figure out how they should be ordered with the help of both Dependency Walker and procmon.

For example, if you load a DLL from c:\windows\system32 directly, but that has a DLL dependency, it will load that dependency from the current directory and not c:\windows\system32. So you always have to make sure to load the dependency first from c:\windows\system32.

For me to determine the ordering, I'd need to update a version of the patch on each of (winxpx86, winxpx64, vistax86, vistax64, win7x86, win7x64, win8x86, win8x64, win8.1x86, win8.1x64)

There seems to be a at least a few DLL difference for each OS release.  I'd expect this trend to continue for future OS releases. 

Every time you make any change to the loaddlls.cpp list, you need to re-test each and every DLL again and verify it with procmon because of the dependency problem described.  That means on the next Windows release we'd have a lot of work again.

---

So given all of that, I think the best course of action is to spend more time trying to find a one time fixes all solution.  I'm going to work on a prototype app that gets this working, and then assuming I eventually succeed I'll port that up into updater.exe code.  I'll update this ticket as I progress.

Comment 27

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

I've been thinking along the same lines. Perhaps removing or renaming dll's at the start and preloading only the dll's required to perform the removes or renames and if renamed renaming them back at the end. Perhaps there is a cleaner way?

Comment 28

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

Comment on attachment 8420726 [details] [diff] [review]
dlls.diff

Let's discuss other possibilities next week.

Comment 29

[Brian R. Bondy [:bbondy]]

Yep I'm going to play with a couple ways along those lines and we'll talk more. Thanks!

Comment 30

[Brian R. Bondy [:bbondy]]

Here's what I'm going to try going with:

This code would run just before executing the updater elevated (which is only when the maintenance service is not used).

1. Updater.exe (unelvated) creates a subdir inside updates\0\<randomname>
2. copies self to new dir 
3. Set ACL on the new DIR to be similar to program files
4. Verify that the dir has no DLLs in it, if it does error out.
5. Executes exe elevated in the new dir instead of at same path.
<Update from elevated updater happens>
6. Removes permissions on current dir for all users (The elevated updater probably has to do this)
7. Updater.exe unelevated deletes the dir.

This relies on updater dir being on an NTFS volume, but I think that's just about everyone.
I don't think there's cause to worry about the XP case because it doesn't have integrity levels and so we aren't worried about someone using DLL injection to elevate a medium integrity malware to high.

Comment 31

[Brian R. Bondy [:bbondy]]

Attachment: WIP v1

Looks like it's going to work. I attached a prototype that does the needed actions.

Note that we can also use this same process for the base installer exploits and the stub exploit.   Maybe we'd put the code in a 7zip stub and then wrap the stub inside of it. (We'll discuss that later after this bug is done)

Here's the new flow:
1. Create new directory inside the updater dir
C:\Users\BrianR.Bondy\AppData\Local\Mozilla\updates\EEFEA8717BC47F65\updates\0\<newdir>
2. Copy updater.exe to <newdir>
3. Lock the <newdir> path so that properties can't be changed on the directory and it can't be deleted.
4. Set ACLs on <newdir> to deny all writes for the EVERYONE special group
5. Verify that the only file inside the dir is updater.exe
6. ***DO THE UPDATE*** (by executing <newdir>/updater.exe elevated.
7. Revert back the ACLs to the old ones.
8. unlock the path.
9. delete the <newdir> path.

Comment 32

[Brian R. Bondy [:bbondy]]

Attachment: Securely update w/o the maintenance sevice - rev1.

This should be a catch all solution to stop all DLL attacks on updater.exe when the service is not used.  When the service is used, we already protect against that by copying the file to the maintenance service directory.

This works in a similar way by first copying updater.exe to a non-secure directory, then making it secure, then verifying that updater.exe is the only thing in that directory before executing it.  If anything else got in, then it fails the update.

Just requesting feedback first because after arriving at an f+ I want to request sec approval, and then push to oak for testing. I'll probably ask Kamil to help test too on other OS because I don't have the disk space currently for VMs.  Testing just requires making sure updates work on each of the supported OS when the service is disabled.

I did a local update and it worked, and was being elevated from the secure directory.

Comment 33

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

Comment on attachment 8424397 [details] [diff] [review]
Securely update w/o the maintenance sevice - rev1.

Looks pretty damn good!

nit: SECURE_ELEVATION_UPDATE_ERROR is a tad off. Perhaps SECURE_LOCATION_UPDATE_ERROR or similar.

Do you think tests can be added for this?

Looking forward to the final patch and thanks!

Comment 34

[Brian R. Bondy [:bbondy]]

this path is only hit when elevating, but I think maybe we can hack some kind of an env variable together to force doing that code without the elevation just to get it executing through tests. I think doing that is worth the extra effort.

Comment 35

[Brian R. Bondy [:bbondy]]

Comment on attachment 8424397 [details] [diff] [review]
Securely update w/o the maintenance sevice - rev1.

This request is for landing this and possibly follow up minor changes to the patch on oak to test. And if after testing that passes, and after r+, also for landing on mozilla-inbound. 

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
User needs to have physical unelevated access to the machine from a low integrity process at least via the same user account.  Constructing a DLL isn't too hard if you are experienced at doing so.  A typical developer doesn't know how to do that though.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The source code and comments do make it a bit obvious that we're protecting against dll injection attacks. 

Which older supported branches are affected by this flaw?
This issue has been present in some form forever.  We realized recently that if you construct DLLs with the same export table as the matching DLL name, that the problem is generalized to a lot more DLLs that we didn't know about before. This bug is only present when updating without the NT service.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch should be the same for all branches if we uplift.

How likely is this patch to cause regressions; how much testing does it need?
It's possible, but that's why I want to test thoroughly on oak. And ask for help from QA to help test other OS after initial testing.

Comment 36

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

Comment on attachment 8424397 [details] [diff] [review]
Securely update w/o the maintenance sevice - rev1.

sec-approval+ for trunk. Please make Aurora, Beta, and ESR24 patches to land once it is tested on trunk and trunk-like areas (as discussed) so we can have it fixed in all things.

Comment 37

[Brian R. Bondy [:bbondy]]

Updates working well on win8.1 

Pushed to try here:
https://tbpl.mozilla.org/?tree=Try&rev=59ae08661e59

Waiting on the request for help testing until I finish the env var changes that allow updates to go through this code path.

Comment 38

[Brian R. Bondy [:bbondy]]

Attachment: Securely update w/o the maintenance sevice - rev2.

Updated w/ review comments.

Comment 39

[Brian R. Bondy [:bbondy]]

Attachment: Patch for testing code path via env var - WIP

Comment 40

[Brian R. Bondy [:bbondy]]

Attachment: Patch for testing code path via env var - WIPv2

Comment 41

[Lukas Blakk [:lsblakk] use ?needinfo]

Whatever got sec-approval + here did not actually land to central and now we are already at the end of FF30 beta (only an RC next Monday for last minute - very low risk landings).  This looks like a wontfix to me for 30 without a patch already prepared and tested, landed to central. What is the risk to the user of waiting one more cycle?

Comment 42

[Brian R. Bondy [:bbondy]]

There's a second patch in progress for test coverage, so I don't think it'll be landed on inbound in time. The first patch should not land until this second patch is ready.

Comment 43

[Daniel Veditz [:dveditz]]

Yeah, don't land anything until the next cycle.

Comment 44

[Brian R. Bondy [:bbondy]]

Attachment: Patch for testing code path via env var - rev1

This makes it so all non service, Windows update tests, run through the code path of elevation. This includes things like creating the secure directory and copying in the updater binary. 

Instead of using the "runas" ShellExecute verb though, it just uses "open" to avoid the UAC prompt.

Comment 45

[Brian R. Bondy [:bbondy]]

Comment on attachment 8428989 [details] [diff] [review]
Securely update w/o the maintenance sevice - rev2.

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

You've taken a pass at this from the f+ before, but I'd like to go for a review now that the automated tests are passing.

I wanted to call out that if any of the security steps to securing the updater.exe into its own directory fail, the whole update will not be applied.
Let me know if you think this could be a problem with FAT32 or something like that. I don't think that kind of setup will exist in a non trivial way on Vista+, but I'm not 100% positive.

We could consider only doing this for NTFS filesystems and if we confirm the path to the updater is a local file like we do here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/common/updatehelper.cpp#693

Let me know your thoughts, and thanks for taking a look.

Comment 46

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

Comment on attachment 8428989 [details] [diff] [review]
Securely update w/o the maintenance sevice - rev2.

Will review remainder as time permits.

>diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json
>--- a/toolkit/components/telemetry/Histograms.json
>+++ b/toolkit/components/telemetry/Histograms.json
>@@ -3030,21 +3030,27 @@
>   "UPDATER_LAST_NOTIFY_INTERVAL_DAYS_NOTIFY": {
>     "expires_in_version": "never",
>     "kind": "exponential",
>     "n_buckets": 10,
>     "high": "60",
>     "description": "Updater: The interval in days between the previous and the current background update check when the check was timer initiated"
>   },
>   "UPDATER_STATUS_CODES": {
>-    "expires_in_version": "never",
>+    "expires_in_version": "32.0a1",
Just verifying that this do the "right thing" by using nsIVersionComparator.

Comment 47

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #46)
> Comment on attachment 8428989 [details] [diff] [review]
> Securely update w/o the maintenance sevice - rev2.
> 
> Will review remainder as time permits.
> 
> >diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json
> >--- a/toolkit/components/telemetry/Histograms.json
> >+++ b/toolkit/components/telemetry/Histograms.json
> >@@ -3030,21 +3030,27 @@
> >   "UPDATER_LAST_NOTIFY_INTERVAL_DAYS_NOTIFY": {
> >     "expires_in_version": "never",
> >     "kind": "exponential",
> >     "n_buckets": 10,
> >     "high": "60",
> >     "description": "Updater: The interval in days between the previous and the current background update check when the check was timer initiated"
> >   },
> >   "UPDATER_STATUS_CODES": {
> >-    "expires_in_version": "never",
> >+    "expires_in_version": "32.0a1",
> Just verifying that this do the "right thing" by using nsIVersionComparator.

I verified that this works correctly with a devtools chrome enabled scratchpad. But I'm going to update it to 34 since this will likely land sometime in 33. 32.0a1 was only applicable if this was landing in 32.

Comment 48

[Brian R. Bondy [:bbondy]]

Attachment: Patch for testing code path via env var - rev2

Added an ifdef to stub out the disabling privs logging since it was causing comparison problems for the re-launched updater (both not launched and elevated was appending logs for staged updates).  Was a test only problem.

Comment 49

[Brian R. Bondy [:bbondy]]

Attachment: Securely update w/o the maintenance sevice - rev3

Updated telemetry expiring version from the last patch.

Comment 50

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

Comment on attachment 8439667 [details] [diff] [review]
Patch for testing code path via env var - rev2

># HG changeset patch
># Parent ea1c16faa3b19bfa67c3b1c1deeff14916be6313
># User Brian R. Bondy <[e-mail]>
>
>diff --git a/toolkit/mozapps/update/common/uachelper.cpp b/toolkit/mozapps/update/common/uachelper.cpp
>--- a/toolkit/mozapps/update/common/uachelper.cpp
>+++ b/toolkit/mozapps/update/common/uachelper.cpp
>@@ -157,21 +157,25 @@ UACHelper::DisableUnneededPrivileges(HAN
>       return FALSE;
>     }
>     token = obtainedToken;
>   }
> 
>   BOOL result = TRUE;
>   for (size_t i = 0; i < count; i++) {
>     if (SetPrivilege(token, unneededPrivs[i], FALSE)) {
>+#ifdef UPDATER_LOG_PRIVS
>       LOG(("Disabled unneeded token privilege: %s.",
>            unneededPrivs[i]));
>+#endif
>     } else {
>+#ifdef UPDATER_LOG_PRIVS
>       LOG(("Could not disable token privilege value: %s. (%d)",
>            unneededPrivs[i], GetLastError()));
>+#endif
>       result = FALSE;
>     }
>   }
> 
>   if (obtainedToken) {
>     CloseHandle(obtainedToken);
>   }
>   return result;
>diff --git a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>--- a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>+++ b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>@@ -1503,25 +1503,28 @@ function runUpdate(aExpectedExitValue, a
>   }
>   logTestInfo("running the updater: " + updateBin.path + " " + args.join(" "));
> 
>   let env = AUS_Cc["@mozilla.org/process/environment;1"].
>             getService(AUS_Ci.nsIEnvironment);
>   if (gDisableReplaceFallback) {
>     env.set("MOZ_NO_REPLACE_FALLBACK", "1");
>   }
>+  env.set("MOZ_EMULATE_ELEVATION_PATH", "1");
> 
>   let process = AUS_Cc["@mozilla.org/process/util;1"].
>                 createInstance(AUS_Ci.nsIProcess);
>   process.init(updateBin);
>+
>   process.run(true, args, args.length);
> 
>   if (gDisableReplaceFallback) {
>     env.set("MOZ_NO_REPLACE_FALLBACK", "");
>   }
>+  env.set("MOZ_EMULATE_ELEVATION_PATH", "");
> 
>   let status = readStatusFile();
>   if (process.exitValue != aExpectedExitValue || status != aExpectedStatus) {
>     if (process.exitValue != aExpectedExitValue) {
>       logTestInfo("updater exited with unexpected value! Got: " +
>                   process.exitValue + ", Expected: " +  aExpectedExitValue);
>     }
>     if (status != aExpectedStatus) {
>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>...
>@@ -2770,17 +2767,19 @@ int NS_main(int argc, NS_tchar **argv)
>         } else {
>           lastFallbackError = FALLBACKKEY_LAUNCH_ERROR;
>         }
>       }
> 
>       // If the service can't be used when staging and update, make sure that
nit: please fix s/staging and update/staging an update/

>       // the UAC prompt is not shown! In this case, just set the status to
>       // pending and the update will be applied during the next startup.
>-      if (!useService && sStagedUpdate) {
>+      // If emulateElevation is set, we want to fall through to elevate, so
>+      // don't exist here.
nit:
// When emulateElevation is true fall through to the elevation code path.

Comment 51

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

Comment on attachment 8439670 [details] [diff] [review]
Securely update w/o the maintenance sevice - rev3

Throughout the new code
s/NULL/nullptr/

>diff --git a/toolkit/mozapps/update/common/uachelper.cpp b/toolkit/mozapps/update/common/uachelper.cpp
>--- a/toolkit/mozapps/update/common/uachelper.cpp
>+++ b/toolkit/mozapps/update/common/uachelper.cpp
>@@ -215,8 +216,96 @@ UACHelper::CanUserElevate()
>...

>+/**
>+ * Determines if the specified directory only has updater.exe inside of it,
>+ * and nothing else.
>+ *
>+ * @param inputPath the directory path to search
>+ * @return true if updater.exe is the only file in the directory
>+ */
>+bool
>+UACHelper::IsDirectorySafe(LPCWSTR inputPath)
>+{
>+  WIN32_FIND_DATAW findData;
>+  HANDLE findHandle = NULL;
>+
>+  WCHAR searchPath[MAX_PATH + 1] = { L'\0' };
>+  wsprintfW(searchPath, L"%s\\*.*", inputPath);
>+
>+  findHandle = FindFirstFileW(searchPath, &findData);
>+  if(findHandle == INVALID_HANDLE_VALUE) {
>+      return false;
nit: indentation

>+  }
>+
>+  // Enumerate the files and if we find anything other than the current
>+  // directory, the parent directory, or updater.exe. Then fail.
>+  do {
>+      if(wcscmp(findData.cFileName, L".") != 0 &&
>+         wcscmp(findData.cFileName, L"..") != 0 &&
>+         wcscmp(findData.cFileName, L"updater.exe") != 0) {
>+           FindClose(findHandle);
>+           return false;
>+      }
nit: indentation

>+  }
>+  while(FindNextFileW(findHandle, &findData));
} while(...) on same line per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

Comment 52

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

Comment on attachment 8439670 [details] [diff] [review]
Securely update w/o the maintenance sevice - rev3

s/NULL/nullptr/ for this as well


>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>@@ -2795,45 +2796,147 @@ int NS_main(int argc, NS_tchar **argv)
>...

>-      // using the service is because we are testing. 
>-      if (!useService && !noServiceFallback && 
>+      // using the service is because we are testing.
>+      if (!useService && !noServiceFallback &&
>           updateLockFileHandle == INVALID_HANDLE_VALUE) {
>-        SHELLEXECUTEINFO sinfo;
>-        memset(&sinfo, 0, sizeof(SHELLEXECUTEINFO));
>-        sinfo.cbSize       = sizeof(SHELLEXECUTEINFO);
>-        sinfo.fMask        = SEE_MASK_FLAG_NO_UI |
>-                             SEE_MASK_FLAG_DDEWAIT |
>-                             SEE_MASK_NOCLOSEPROCESS;
>-        sinfo.hwnd         = nullptr;
>-        sinfo.lpFile       = argv[0];
>-        sinfo.lpParameters = cmdLine;
>-        sinfo.lpVerb       = L"runas";
>-        sinfo.nShow        = SW_SHOWNORMAL;
>-
>-        bool result = ShellExecuteEx(&sinfo);
>-        free(cmdLine);
>-
>-        if (result) {
>-          WaitForSingleObject(sinfo.hProcess, INFINITE);
>-          CloseHandle(sinfo.hProcess);
>+
>+        // Get a unique directory name to secure
>+        RPC_WSTR guidString = RPC_WSTR(L"");
>+        GUID guid;
>+        HRESULT hr = CoCreateGuid(&guid);
>+        BOOL result = TRUE;
>+        bool safeToUpdate = true;
>+        WCHAR secureUpdaterPath[MAX_PATH + 1] = { L'\0' };
>+        WCHAR secureDirPath[MAX_PATH + 1] = { L'\0' };
>+        if (SUCCEEDED(hr)) {
>+          UuidToString(&guid, &guidString);
>+          result = PathGetSiblingFilePath(secureDirPath, argv[0],
>+                                          reinterpret_cast<LPCWSTR>(guidString));
>+          RpcStringFree(&guidString);
>         } else {
>-          WriteStatusFile(ELEVATION_CANCELED);
>+          // This should never happen, but just in case
>+          result = PathGetSiblingFilePath(secureDirPath, argv[0], L"tmp_update");
>         }
>+
>+        if (!result) {
>+          fprintf(stderr, "Could not obtain temp directory path");
nit: s/temp/secure update/

>+          safeToUpdate = false;
>+        }
>+
>+        // If it's still safe to update, create the directory
>+        if (safeToUpdate) {
>+          result = CreateDirectory(secureDirPath, NULL);
>+          if (!result) {
>+            fprintf(stderr, "Could not create temp directory");
nit: s/temp/secure update/

>+            safeToUpdate = false;
>+          }
>+        }
>+
>+        // If it's still safe to update, get the new updater path
>+        if (safeToUpdate) {
>+          wcsncpy(secureUpdaterPath, secureDirPath, MAX_PATH);
>+          result = PathAppendSafe(secureUpdaterPath, L"updater.exe");
>+          if (!result) {
>+            fprintf(stderr, "Could not obtain secure temp file name");
nit: s/temp/updater/

>+            safeToUpdate = false;
>+          }
>+        }
>+
>+        // If it's still safe to update, copy the file in
>+        if (safeToUpdate) {
>+          result = CopyFileW(argv[0], secureUpdaterPath, TRUE);
>+          if (!result) {
>+            fprintf(stderr, "Could not copy updater to new location");
nit: s/new/secure/

>+            safeToUpdate = false;
>+          }
>+        }
>+
>+        // If it's still safe to update, restrict access to the directory item
>+        // itself so that the directory cannot be deleted and re-created,
>+        // nor have its properties modified.  Note that this does not disallow
>+        // adding items inside the directory.
>+        HANDLE handle = INVALID_HANDLE_VALUE;
>+        if (safeToUpdate) {
>+          handle = CreateFileW(secureDirPath, GENERIC_READ, FILE_SHARE_READ,
>+                               NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS,
>+                               NULL);
>+          safeToUpdate = handle != INVALID_HANDLE_VALUE;
>+        }
>+
>+        // If it's still safe to update, deny write access completely to the
>+        // directory.
>+        PACL originalACL = nullptr;
>+        if (safeToUpdate) {
>+          safeToUpdate = UACHelper::DenyWriteACLOnPath(secureDirPath,
>+                                                       &originalACL);
>+        }
>+
>+        // If it's still safe to update, verify that there is only updater.exe
>+        // in the directory and nothing else.
>+        if (safeToUpdate) {
>+          if (!UACHelper::IsDirectorySafe(secureDirPath)) {
>+            safeToUpdate = false;
>+          }
>+        }
>+
>+        if (!safeToUpdate) {
>+          fprintf(stderr, "Could not copy path to secure location because it "
>+                          "is not safe to do so.");
The failure isn't really about copying a path

>+          WriteStatusFile(SECURE_LOCATION_UPDATE_ERROR);
>+        } else {
>+          SHELLEXECUTEINFO sinfo;
>+          memset(&sinfo, 0, sizeof(SHELLEXECUTEINFO));
>+          sinfo.cbSize       = sizeof(SHELLEXECUTEINFO);
>+          sinfo.fMask        = SEE_MASK_FLAG_NO_UI |
>+                              SEE_MASK_FLAG_DDEWAIT |
>+                              SEE_MASK_NOCLOSEPROCESS;
>+          sinfo.hwnd         = nullptr;
>+          sinfo.lpFile       = secureUpdaterPath;
>+          sinfo.lpParameters = cmdLine;
>+          sinfo.lpVerb       = L"runas";
>+          sinfo.nShow        = SW_SHOWNORMAL;
>+
>+          bool result = ShellExecuteEx(&sinfo);
>+          free(cmdLine);
>+
>+          if (result) {
>+            WaitForSingleObject(sinfo.hProcess, INFINITE);
>+            CloseHandle(sinfo.hProcess);
>+          } else {
>+            WriteStatusFile(ELEVATION_CANCELED);
>+          }
>+        }
>+
>+        // All done, revert back the permissions.
>+        if (originalACL) {
>+          SetNamedSecurityInfoW(const_cast<LPWSTR>(secureDirPath), SE_FILE_OBJECT,
>+                                DACL_SECURITY_INFORMATION, NULL, NULL,
>+                                originalACL, NULL);
>+          LocalFree(originalACL);
>+        }
>+
>+        // Done with the directory, no need to lock it.
>+        if (INVALID_HANDLE_VALUE != handle)
>+          CloseHandle(handle);
nit: braces


I'm a tad concerned about the possibility of the directory being left behind without the permissions being reset but I *think* that is just my paranoia speaking.

Comment 53

[Brian R. Bondy [:bbondy]]

Thanks for the review. 

> I'm a tad concerned about the possibility of the directory 
> being left behind without the permissions being reset but I *think* that is just my paranoia speaking.

Perhaps with a crash or something along those lines it would. Shouldn't cause a problem though because we use a new dir name each time.  From my manual testing though it always cleans up.

Comment 54

[Brian R. Bondy [:bbondy]]

Attachment: Securely update w/o the maintenance sevice - rev4

Implemented review comments.

Comment 55

[Brian R. Bondy [:bbondy]]

Attachment: Patch for testing code path via env var - rev3

Implemented review comments

Comment 56

[Brian R. Bondy [:bbondy]]

Getting some test timesouts only on win7 for the try push both Release and Debug:
https://tbpl.mozilla.org/?tree=Try&rev=61f2dc5043ce
Win8 and winxp work.

I'll investigate more, but I setup a local VM of Win7 and Win8 and all tests passed on both :(

Comment 57

[chris hofmann]

Hi Brian,  ever get this figured out?

Comment 58

[Brian R. Bondy [:bbondy]]

Still have that error, I think it would be ok to fix it by not using the env var that triggers this for that test, but I'd like to investigate more to be sure. I was pulled from my current activities to work on bug 1009816 though so will resume this once that's in review.

Comment 59

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

I'll try to figure out what is going on with the failures while Brian is working on bug 1009816.

Comment 60

[Brian R. Bondy [:bbondy]]

OK thanks, here's a bit of info:
Currently the failing test passes for me locally. But it fails on the talos machines. 
It currently fails on the apply staged update phase. 

The reason why I said in Comment 58 that I think it can be skipped is because I think that we will never do elevation for staged updates. 

It would be nice to figure out though.  I put a few patches with some extra logging on Oak if you want to see those changesets too.

Comment 61

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

Brian, the changes I emailed you about seems to have fixed the errors you were seeing. The LocalFree(sd); in DenyWriteACLOnPath was still causing a couple of the tests to fail locally for me (commented out in the changeset I sent to try) and I'll investigate that further.
https://tbpl.mozilla.org/?tree=Try&rev=0f1d2758f6ac

Comment 62

[Brian R. Bondy [:bbondy]]

Great news, thanks for digging into it!

Comment 63

[Brian R. Bondy [:bbondy]]

Hey Robert, I looked into this and the reason why is because the originalACL pointer actually points to a location within the buffer of sd.  So freeing it early causes problems when it is used. 
I'll upload a new patch with your fix and mine shortly and push to oak for testing.

Comment 64

[Brian R. Bondy [:bbondy]]

Attachment: Securely update w/o the maintenance sevice - rev5

Carrying forward r+. 
- Includes rstrong's fix for not freeing 
- Includes fix for returning / freeing sd at an appropriate time
This should not land until it is passes tests though. I'm pushing it to oak now. I didn't try locally because I updated the patch on OSX but if there's any issues I'll put a new patch up.

Comment 65

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

Comment on attachment 8450714 [details] [diff] [review]
Securely update w/o the maintenance sevice - rev5

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>...
>+        // If it's still safe to update, create the directory
>+        if (safeToUpdate) {
>+          result = CreateDirectory(secureDirPath, nullptr);
should be CreateDirectoryW

Comment 66

[Brian R. Bondy [:bbondy]]

Attachment: Securely update w/o the maintenance sevice - rev6

Updated to CreateDirectoryW

Comment 67

[Brian R. Bondy [:bbondy]]

Looks like automated tests are passing now. 

Kamil, would it be possible to test the Oak builds on the major Windows platforms?
XP SP2, Vista, Win7, Win8.1

The change is in this revision and later:
https://hg.mozilla.org/projects/oak/rev/9e099380262d

The types of tests that would be needed are:
- Try upgrading from the 2nd newest Oak nightly (with the changeset above) to the newest Oak nightly (with that same changeset or later)
  - Do this with service enabled
  - Do this with service disabled
  - Do this with service enabled and app.update.staging.enabled set to false 
  - Do this with service disabled and app.update.staging.enabled set to false 
- Try running a single exploit that you know already happens with one of your DLLs on each platform. You don't need to run each of the exploits, just a single one.
- Anything else you can think of testing

If the testing goes well then this is ready to land after pushing to try one last time.

Comment 68

[Sylvestre Ledru [:Sylvestre]]

Is there a plan to have a fix landed for 31?
If yes, it should land before Thursday (gtb of beta 9).

Comment 69

[Brian R. Bondy [:bbondy]]

Thanks for the heads up.  If the manual testing is done by then and it all passes then maybe ya. If not, then it's safer to just wait one more version. It may be safer to wait anyway without uplifting early with a little bake time because of the chance of affecting updates.

Comment 70

[Kamil Jozwiak [:kjozwiak]]

I'll get started this afternoon and post the results once I'm done. I should be done by Wednesday.

Comment 71

[Brian R. Bondy [:bbondy]]

I think we should probably wait anyway so don't kill yourself to get it done early.  More bake time on Nightly is probably better for this type of bug.

Comment 72

[Kamil Jozwiak [:kjozwiak]]

Sounds good Brian, I'll start either way and make sure there isn't anything major that stands out. But I agree, having this bake a bit longer on Nightly is probably the best route.

Comment 73

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

That said, if we don't get this in 31, we'll have to backport it to ESR31 later since it is a sec-high bug.

Comment 74

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

Understood and backporting would be the better route since there is a decent chance for edge cases with this method of dealing with the possibility of files being copied into the directory we run the updater on a system.

Comment 75

[Lukas Blakk [:lsblakk] use ?needinfo]

This definitely not going to get in 31, so marking wontfix, will leave the ? on ESR24 in case we want to consider it for the next ESR24 (and once flags are available we'll want this for the ESR31 that ships with FF32 as well) - finally, updating the FF33 flags as it doesn't appear this has landed on trunk yet.

Comment 76

[Lawrence Mandel [:lmandel] (use needinfo)]

Kamil - I don't see results posted. Did you finish testing?

Comment 77

[Kamil Jozwiak [:kjozwiak]]

Lawrence,

I haven't had the chance to go over this but I'll start it sometime next week. I'm currently in MTV for a work week and I'm really limited on time. Next week I'll be at a conference but I should have enough time to complete this by next week.

Comment 78

[Kamil Jozwiak [:kjozwiak]]

I haven't had any time to take a look at this as most of the talks/briefings at the conference end pretty late. I'll be flying out Friday morning and will tackle this first thing on Monday.

Comment 79

[Kamil Jozwiak [:kjozwiak]]

Progress Update:

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

Reproduced the original issue using the following builds & DLL's:
- Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-10-03-02-04-mozilla-central/
- DLL's: SHCore.dll (Reproduced), bcrypt.dll (Reproduced)

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/07/2014-07-03-19-17-16-oak/

-> ensured that regular updates are working
-> ensured that fx launches without any issues after being updated
-> ensured that once fx is updated, the "updater" doesn't find another update (ensured the latest version was installed)
-> once fx is updated, ensured that the contents under "\updates\0" are removed
-> ensured that SHCore.dll only spawns MEDIUM integrity CMD's (didn't spawn in some cases)
-> ensured that bcrypt.dll spawns only MEDIUM integrity CMD's

- app.update.service.enabled;true
- app.update.service.enabled;false
- app.update.service.enabled;true & app.update.staging.enabled;false
- app.update.service.enabled;false & app.update.staging.enabled;false

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

Reproduced the original issue using the following builds & DLL's:
- Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-10-03-02-04-mozilla-central/
- DLL's: bcryptprimitives.dll (Reproduced), bcrypt.dll (Reproduced)

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/07/2014-07-03-19-17-16-oak/

For each of the preferences listed below, I went through the following test cases:

-> ensured that regular updates are working
-> ensured that fx launches without any issues after being updated
-> ensured that once fx is updated, the "updater" doesn't find another update (ensured the latest version was installed)
-> once fx is updated, ensured that the contents under "\updates\0" are removed
-> ensured that bcryptprimitives.dll only spawns MEDIUM integrity CMD's
-> ensured that bcrypt.dll spawns only MEDIUM integrity CMD's

- app.update.service.enabled;true
- app.update.service.enabled;false
- app.update.service.enabled;true & app.update.staging.enabled;false
- app.update.service.enabled;false & app.update.staging.enabled;false

Comment 80

[Kamil Jozwiak [:kjozwiak]]

Progress Update:

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

Reproduced the original issue using the following builds & DLL's:
- Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-10-03-02-04-mozilla-central/
- DLL's: dwmapi.dll (Reproduced), SHCore.dll (Reproduced)

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/07/2014-07-03-19-17-16-oak/

-> ensured that regular updates are working
-> ensured that fx launches without any issues after being updated
-> ensured that once fx is updated, the "updater" doesn't find another update (ensured the latest version was installed)
-> once fx is updated, ensured that the contents under "\updates\0" are removed
-> ensured that dwmapi.dll only spawns MEDIUM integrity CMD's (didn't spawn in some cases)
-> ensured that SHCore.dll spawns only MEDIUM integrity CMD's (didn't spawn in some cases)

- app.update.service.enabled;true
- app.update.service.enabled;false
- app.update.service.enabled;true & app.update.staging.enabled;false
- app.update.service.enabled;false & app.update.staging.enabled;false

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

Reproduced the original issue using the following builds & DLL's:
- Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-10-03-02-04-mozilla-central/
- DLL's: dwmapi.dll (Reproduced), oleacc.dll (Reproduced)

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/07/2014-07-03-19-17-16-oak/

-> ensured that regular updates are working
-> ensured that fx launches without any issues after being updated
-> ensured that once fx is updated, the "updater" doesn't find another update (ensured the latest version was installed)
-> once fx is updated, ensured that the contents under "\updates\0" are removed
-> ensured that dwmapi.dll only spawns MEDIUM integrity CMD's (didn't spawn in some cases)
-> ensured that oleacc.dll spawns only MEDIUM integrity CMD's (didn't spawn in some cases)

- app.update.service.enabled;true
- app.update.service.enabled;false
- app.update.service.enabled;true & app.update.staging.enabled;false
- app.update.service.enabled;false & app.update.staging.enabled;false

Comment 81

[Kamil Jozwiak [:kjozwiak]]

Progress Update:

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

Reproduced the original issue using the following builds & DLL's:
- Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-10-03-02-04-mozilla-central/
- DLL's: bcrypt.dll (Reproduced), dwmapi.dll (Reproduced)

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/07/2014-07-03-19-17-16-oak/

-> ensured that regular updates are working
-> ensured that fx launches without any issues after being updated
-> ensured that once fx is updated, the "updater" doesn't find another update (ensured the latest version was installed)
-> once fx is updated, ensured that the contents under "\updates\0" are removed
-> ensured that bcrypt.dll only spawns MEDIUM integrity CMD's (didn't spawn in some cases)
-> ensured that dwmapi.dll spawns only MEDIUM integrity CMD's (didn't spawn in some cases)

- app.update.service.enabled;true
- app.update.service.enabled;false
- app.update.service.enabled;true & app.update.staging.enabled;false
- app.update.service.enabled;false & app.update.staging.enabled;false

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

Reproduced the original issue using the following builds & DLL's:
- Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-10-03-02-04-mozilla-central/
- DLL's: bcrypt.dll (Reproduced), dwmapi.dll (Reproduced)

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/07/2014-07-03-19-17-16-oak/

-> ensured that regular updates are working
-> ensured that fx launches without any issues after being updated
-> ensured that once fx is updated, the "updater" doesn't find another update (ensured the latest version was installed)
-> once fx is updated, ensured that the contents under "\updates\0" are removed
-> ensured that bcrypt.dll only spawns MEDIUM integrity CMD's (didn't spawn in some cases)
-> ensured that dwmapi.dll spawns only MEDIUM integrity CMD's (didn't spawn in some cases)

- app.update.service.enabled;true
- app.update.service.enabled;false
- app.update.service.enabled;true & app.update.staging.enabled;false
- app.update.service.enabled;false & app.update.staging.enabled;false

Comment 82

[Kamil Jozwiak [:kjozwiak]]

Progress Update:

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

Reproduced the original issue using the following builds & DLL's:
- Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-10-03-02-04-mozilla-central/
- DLL's: uxtheme.dll (Reproduced), userenv.dll (Reproduced)

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/07/2014-07-03-19-17-16-oak/

-> ensured that regular updates are working
-> ensured that fx launches without any issues after being updated
-> ensured that once fx is updated, the "updater" doesn't find another update (ensured the latest version was installed)
-> once fx is updated, ensured that the contents under "\updates\0" are removed
-> ensured that uxtheme.dll only spawns MEDIUM integrity CMD's (didn't spawn in some cases)
-> ensured that userenv.dll spawns only MEDIUM integrity CMD's (didn't spawn in some cases)

- app.update.service.enabled;true
- app.update.service.enabled;false
- app.update.service.enabled;true & app.update.staging.enabled;false
- app.update.service.enabled;false & app.update.staging.enabled;false

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

Reproduced the original issue using the following builds & DLL's:
- Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-10-03-02-04-mozilla-central/
- DLL's: msasn1.dll (Reproduced), userenv.dll (Reproduced)

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/07/2014-07-03-19-17-16-oak/

-> ensured that regular updates are working
-> ensured that fx launches without any issues after being updated
-> ensured that once fx is updated, the "updater" doesn't find another update (ensured the latest version was installed)
-> once fx is updated, ensured that the contents under "\updates\0" are removed
-> ensured that msasn1.dll only spawns MEDIUM integrity CMD's (didn't spawn in some cases)
-> ensured that userenv.dll spawns only MEDIUM integrity CMD's (didn't spawn in some cases)

- app.update.service.enabled;true
- app.update.service.enabled;false
- app.update.service.enabled;true & app.update.staging.enabled;false
- app.update.service.enabled;false & app.update.staging.enabled;false

Comment 83

[Kamil Jozwiak [:kjozwiak]]

Progress Update:

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

I couldn't reproduce the original issue because there was no DLL's that are hijackable. However I did go through several updates with the oak build to ensure everything was working

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/07/2014-07-03-19-17-16-oak/

-> ensured that regular updates are working
-> ensured that fx launches without any issues after being updated
-> ensured that once fx is updated, the "updater" doesn't find another update (ensured the latest version was installed)
-> once fx is updated, ensured that the contents under "\updates\0" are removed

- app.update.service.enabled;true
- app.update.service.enabled;false
- app.update.service.enabled;true & app.update.staging.enabled;false
- app.update.service.enabled;false & app.update.staging.enabled;false

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

Reproduced the original issue using the following builds & DLL's:
- Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-10-03-02-04-mozilla-central/
- DLL's: ws2_32.dll (Reproduced), ws2help.dll (Reproduced)

The issue with Win XP is that it doesn't list the integrity level of the CMD's...

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/07/2014-07-03-19-17-16-oak/

-> ensured that regular updates are working
-> ensured that fx launches without any issues after being updated
-> ensured that once fx is updated, the "updater" doesn't find another update (ensured the latest version was installed)
-> once fx is updated, ensured that the contents under "\updates\0" are removed
-> ws2_32.dll spawned CMD's but there's no way of checking their integrity level
-> ws2help.dll spawned CMD's but there's no way of checking their integrity level

- app.update.service.enabled;true
- app.update.service.enabled;false
- app.update.service.enabled;true & app.update.staging.enabled;false
- app.update.service.enabled;false & app.update.staging.enabled;false

Comment 84

[Kamil Jozwiak [:kjozwiak]]

Results:

- Windows 8.1 x86 & x64 [PASSED] - test cases in comment #79
- Windows 8 x86 & x64 [PASSED] - test cases in comment #80
- Windows 7 x86 & x64 [PASSED] - test cases in comment #81
- Windows Vista x86 & x64 [PASSED] - test cases in comment #82
- Windows XP x86 & x64 [PASSED BUT NEED INFO] - test cases in comment #83

Brian, for Windows XP there's no way of telling the integrity level so I'm not sure if the CMD's spawned MEDIUM/HIGH etc.. I did go through all the updates and everything worked without any issues.

I got one crash when I was testing Win XP but I don't think it's related, would you mind taking a quick look just in case?? The crash happened when I closed fx and quickly tried re-opening it.. (this didn't happen during the actual update process):

- https://crash-stats.mozilla.com/report/index/8c55d783-d7b4-416e-af2e-010d52140814

Comment 85

[Brian R. Bondy [:bbondy]]

yep not related, and yep you're correct about the integrity levels. Thanks so much for testing all of this!

Comment 86

[Lawrence Mandel [:lmandel] (use needinfo)]

After reading Kamil's reports above, I'm unclear whether additional work is needed or this patch is ready to land as is. If this is ready to land, can we get this in the tree for Monday so that it is in time for beta8?

Comment 87

[Brian R. Bondy [:bbondy]]

Looks like the Oak patches passed testing Lawrence but it is advisable to have some bake time on mozilla-central with Nightly builds before uplifting.  That probably means a natural train ride to aurora and then an uplift to the new beta 1.

Comment 88

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks Brian. I agree about testing on m-c. Generally we give a couple of days on m-c unless a patch is thought to be riskier. If you think a couple of days will be enough in this case, we can still target beta9. However, if there is enough risk, I agree that we should wait a couple of weeks for beta1.

Al/Dan - Are you OK from a security standpoint of holding this fix off until Firefox 33?

Comment 89

[Brian R. Bondy [:bbondy]]

Yes there is significantly more risk than a typical patch. 

Also I'm hesitant to land this *right* now because I won't have a lot of time available if there is fallout. I'd recommend waiting until after the next release, landing on m-c around beta-1, and then uplifting to aurora and beta after a couple weeks.  I'm not currently under contract right now so I can't guarantee availability for fallout right now.

I can land if someone feels strongly about it, but I don't think there's a significant security risk in waiting. This issue is only exploitable if the user has locally installed malware or access and most people are updating with the service which doesn't have this issue.

Comment 90

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm inclined to follow Brian's recommendation of landing after the merge (around 33 beta1 time frame) and considering for uplift around 33 beta 4/5. I will wait to hear from Dan or Al before confirming the plan.

Comment 91

[Brian R. Bondy [:bbondy]]

Awaiting their response too, but I do think that's the best plan forward independent of my availability. I think it's too late to have it on release channel so there'd be no benefit to landing on m-c before the next uplift anyway.

Comment 92

[Daniel Veditz [:dveditz]]

I agree, with only a beta or two before RC we should not cram it into Firefox 32 at this point in which case landing the first week in September (after the merge) is fine

Comment 93

[Lawrence Mandel [:lmandel] (use needinfo)]

I have marked Firefox 32 as won't fix. We'll get this in 33.

Comment 94

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

I agree. No reason to shove this in with the risk.

Comment 95

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

Fixing ESR flags.

Comment 96

[Brian R. Bondy [:bbondy]]

Update: 
Compared the patches in this bug to the ones in oak which was tested by Kamil. It's been a while since I've looked at this bug and I wasn't sure if they were the exact same, but they match up.

I pushed to try, if everything passes I'll be pushing to inbound as early as tomorrow.

Comment 97

[Kamil Jozwiak [:kjozwiak]]

Once the patch/changes make it into m-c, I'll go through the test cases from comment #79, comment #80, comment #81, comment #82 and comment #83 to double check and make sure everything is still working as expected.

Comment 98

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f8afdcdc9ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a1eb4df27b

Comment 99

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9f8afdcdc9ba
https://hg.mozilla.org/mozilla-central/rev/a7a1eb4df27b

Comment 101

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

Kamil, any estimate as to when you will have time to verify this bug? Thanks!

Comment 102

[Kamil Jozwiak [:kjozwiak]]

Robert, going to start today (in a few minutes here) and should probably be done by tomorrow afternoon.

Comment 103

[Sylvestre Ledru [:Sylvestre]]

Kamil, any progress on this?

Brian, could you fill an uplift request for aurora, beta and esr31? Thanks

Comment 104

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

I am concerned that this change caused a significant increase in the test failures that are recorded in bug 1027039.

Comment 105

[Brian R. Bondy [:bbondy]]

so it looks like the intermittent failure you mentioned used to happen even before this landed, but on the 10th and the 15th it had a few failures each. I'm not sure if that's because we only stared them on those days, or if there was something special that happened only on those days.  

Anyway I'm going to look into it more but we shouldn't uplift until we figure out if the intermittent update failure is tests only or if it can affect real users too.

Comment 106

[Brian R. Bondy [:bbondy]]

Looks like telemetry is showing that there are quite a few error code 51s (SECURE_LOCATION_UPDATE_ERROR) being returned as well. Since we haven't had any reports I assume this is an intermittent problem. But I think we should do a backout of this patch to be safe and re-land / test more on oak before re-uploading to m-c.  We'll possibly see that the issue in bug 1027039 goes down as well during that time and that would confirm for sure if that's related to this.

Comment 107

[Brian R. Bondy [:bbondy]]

Attachment: Merged backout patch for m-c with intent to re-land on oak for further testing

Comment 108

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

Comment on attachment 8489359 [details] [diff] [review]
Merged backout patch for m-c with intent to re-land on oak for further testing

If there is concern regarding the backout please send it to try first. Thanks!

Comment 109

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

Note: looks like the failure in bug 1027039 changed since the one time it happened around 3 months ago. The update.log back then contained data so I don't think it was the same failure and it has happened been reported 13 times since 9/10.

Comment 110

[Brian R. Bondy [:bbondy]]

I don't think there's a concern but I plan to push it to try first just to be safest.

Comment 111

[Kamil Jozwiak [:kjozwiak]]

I basically finished testing most of the OS's in m-c and never ran into an issue while going through the updating process. I tried on all the OS's via VM's and on several other physical machines without any problems. I completed about 35 updates per OS/architecture, about 350 updates in total.

I'll talk to Brian and see if there's anything we can do to try to reproduce this on oak once the backout from m-c is completed.

Comment 112

[Brian R. Bondy [:bbondy]]

Backed out here: 
https://hg.mozilla.org/integration/mozilla-inbound/rev/238c2d44c904

It looks like this fix is causing an intermittent problem in tests and a different intermittent problem outside of tests. This is a very risky type of change by nature, and we made every precaution possible before landing, but there must still be some edge cases left to resolve.

The in tests one shouldn't be too hard to track down.  The other one I'm not sure off hand how we can reproduce.  But it's safest to not have this on m-c and definitely we shouldn't let it ride an uplift to aurora and beyond.  

We made the right calls earlier in not landing this close to an uplift.  What makes it risky is that normally a change like this can be coded with a fallback in mind, where if it fails you fall back to the old functionality, but since this is to stop a security attack you can't do that.

Kamil thanks for doing so many tests on all the OS' so far, I'll re-ping you once this lands on m-c again.

Comment 113

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

Looks like bug 1027039 comment #11 is the first instance where this hit a tree other than m-c or m-c integration branches so unless I am badly mistaken this didn't cause bug 1027039. Both :) and :(

Comment 114

[Brian R. Bondy [:bbondy]]

Ya seen that just now. It's possible though that it's somehow related in some strange way because of the service. But I don't think so. The telemetry data showing the error is hit though is enough to warrant the backout alone.  Let's see with happens with the test failure in the next couple days.

Comment 115

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

Good point and thanks!

Comment 116

[Brian R. Bondy [:bbondy]]

By the way I started to look into the intermittent test failure problem earlier today (bug 1027039). The error it's throwing is only called from the maintenance service. We don't compare updates without the maintenance service.

This patch only changes (or should only change) the flow for non maintenance service updates.
But the patch from this bug does change the updater binary code, and maintenance service uses the updater, so who knows.

Comment 117

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

Perhaps the maintenance service changes I am making that have been pushed to try stuck around on the build systems and caused the failures. :(

Comment 118

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

I'll run the tests without my changes using the newer maintenance service again to test that.

Comment 119

[Brian R. Bondy [:bbondy]]

It could be, but shouldn't. But if there's an independent bug causing maintenance service to not be updated when running tests then either bug could have caused the esr test failures.

What should be happening is that the bootstrap service test should use whatever old service is currently installed (It gets reset on each reboot which I think is each test run) to replace the maintenance service to the one that's being tested.  Then the rest of the updater service tests use that new service.

Check if your try pushes were around the times of the failures. What's really strange to me if it's this bug is that there were no starred failures before the 10th, this landed on the 5th.  Also if there was some kind of problem comparing the updater binaries in bug 1027039 because of us locking something from this bug, well that locking code shouldn't execute on service updates where the verify compare code is run.

Comment 120

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

If it isn't due to my try pushes or possibly the pushes to oak carrying over to other runs then I suspect either an infrastructure change or possibly a change that has been uplifted everywhere. Looking at today's beta push log nothing stood out though.

Comment 121

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

So, I was able to reproduce the test failure using the new service and the old tests so I think it is from my try pushes. Once again both a :) and :(

Comment 122

[Phil Ringnalda (:philor)]

Indeed. I almost got there on my own, wanting to blame it on "a failure on trunk carries over to later runs on other branches," but I kept finding that the slaves failing on release branches hadn't had a starred failure on trunk, only unstarred failures on try. Or in some cases, unstarred greens, on your try pushes.

Don't suppose you know exactly what broken files in what locations on each version of Windows need to be removed to unbreak a slave? Inconveniently, what releng would usually do, just reimage the affected slaves, isn't possible at the moment because reimaging returns a slave that doesn't actually do anything at all right now, at least on Win8 and probably Win7.

Comment 123

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

The maintenanceservice.exe under Mozilla Maintenance Service\ in the x86 PROGRAMFILES needs to be restored to the one from the image.

Comment 124

[Kamil Jozwiak [:kjozwiak]]

Talked to Brian quickly and he suggested trying it on Win XP. I didn't get to Win XP when I was testing it in m-c but I did go through it in oak and never ran into an issue updating (comment #83). Here's the cases that I quickly went through via m-c:

For the below, I went through both Win XP x64 and x86 (VM's):

Builds updating from:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-09-01-09-10-08-mozilla-central/ (before fix)
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-09-06-03-02-04-mozilla-central/ (after fix)

- updated fx 5 different times with app.update.service.enabled;true without issues
- updated fx 5 different times with app.update.service.enabled;false without issues

I'll wait for more information from Brian's investigation into the issue and help wherever I can.

Comment 125

[Brian R. Bondy [:bbondy]]

Attachment: new patch to land to gather more data. v1.

So this still does all the same security steps as before.

But if any one step fails, it'll write a status failure with a code matching the exact failure.  And it'll allow the update to proceed. 

Upon loading post update processing, it'll log an extra telemetry entry for the status code, and it will reset itself to successful.

After review, I'll run this through try and on oak before pushing to m-c. 
The plan would be to keep this on m-c for a few days and then back it out.

Then hopefully we'll know more about why we have failures to make a new patch.
If we get to a newer fix, then it'll likely not make it on this train, but would be the next one.

Comment 126

[Kamil Jozwiak [:kjozwiak]]

Brian, would you like me to go through some updates on oak on the different OS's before we move this back into m-c for a few days?

Comment 127

[Brian R. Bondy [:bbondy]]

Sounds good after it's reviewed and I do basic testing on Oak. Thanks!

Comment 128

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

Comment on attachment 8495662 [details] [diff] [review]
new patch to land to gather more data. v1.

>diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json
>--- a/toolkit/components/telemetry/Histograms.json
>+++ b/toolkit/components/telemetry/Histograms.json
>@@ -3440,21 +3440,27 @@
>   "UPDATER_LAST_NOTIFY_INTERVAL_DAYS_NOTIFY": {
>     "expires_in_version": "40",
>     "kind": "exponential",
>     "n_buckets": 10,
>     "high": "60",
>     "description": "Updater: The interval in days between the previous and the current background update check when the check was timer initiated"
>   },
>   "UPDATER_STATUS_CODES": {
>-    "expires_in_version": "never",
>+    "expires_in_version": "35",
>     "kind": "enumerated",
>     "n_values": 50,
>     "description": "Updater: the status of the latest update performed"
>   },
>+  "UPDATER_ALL_STATUS_CODES": {
>+    "expires_in_version": "never",
Notcied that never has changed to default in a few places. Could you investigate that and change this if it should be?

>+    "kind": "enumerated",
>+    "n_values": 200,
>+    "description": "Updater: the status of the latest update performed"
>+  },
>   "UPDATER_UPDATES_ENABLED": {
>     "expires_in_version": "default",
>     "kind": "boolean",
>     "description": "Updater: Whether or not updates are enabled"
>   },
>   "UPDATER_UPDATES_METRO_ENABLED": {
>     "expires_in_version": "default",
>     "kind": "boolean",
>diff --git a/toolkit/mozapps/update/common/errors.h b/toolkit/mozapps/update/common/errors.h
>--- a/toolkit/mozapps/update/common/errors.h
>+++ b/toolkit/mozapps/update/common/errors.h
>@@ -64,16 +64,28 @@
> #define UNEXPECTED_FILE_OPERATION_ERROR 42
> #define FILESYSTEM_MOUNT_READWRITE_ERROR 43
> #define FOTA_GENERAL_ERROR 44
> #define FOTA_UNKNOWN_ERROR 45
> #define WRITE_ERROR_SHARING_VIOLATION_SIGNALED 46
> #define WRITE_ERROR_SHARING_VIOLATION_NOPROCESSFORPID 47
> #define WRITE_ERROR_SHARING_VIOLATION_NOPID 48
> 
>+#define FOTA_FILE_OPERATION_ERROR 49
>+#define FOTA_RECOVERY_ERROR 50
>+
>+#define SECURE_LOCATION_UPDATE_ERROR 51
>+#define SECURE_LOCATION_UPDATE_ERROR_2 52
>+#define SECURE_LOCATION_UPDATE_ERROR_3 53
>+#define SECURE_LOCATION_UPDATE_ERROR_4 54
>+#define SECURE_LOCATION_UPDATE_ERROR_5 55
>+#define SECURE_LOCATION_UPDATE_ERROR_6 56
>+#define SECURE_LOCATION_UPDATE_ERROR_7 57
>+#define SECURE_LOCATION_UPDATE_ERROR_8 58
Just to verify, the plan is for these extra error codes to away?

>diff --git a/toolkit/mozapps/update/common/uachelper.cpp b/toolkit/mozapps/update/common/uachelper.cpp
>--- a/toolkit/mozapps/update/common/uachelper.cpp
>+++ b/toolkit/mozapps/update/common/uachelper.cpp
>@@ -156,21 +157,25 @@ UACHelper::DisableUnneededPrivileges(HAN
>       return FALSE;
>     }
>     token = obtainedToken;
>   }
> 
>   BOOL result = TRUE;
>   for (size_t i = 0; i < count; i++) {
>     if (SetPrivilege(token, unneededPrivs[i], FALSE)) {
>+#ifdef UPDATER_LOG_PRIVS
>       LOG(("Disabled unneeded token privilege: %s.",
>            unneededPrivs[i]));
>+#endif
>     } else {
>+#ifdef UPDATER_LOG_PRIVS
>       LOG(("Could not disable token privilege value: %s. (%d)",
>            unneededPrivs[i], GetLastError()));
>+#endif
Is this just to clean up the log output?

>diff --git a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>--- a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>+++ b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>@@ -1503,25 +1503,28 @@ function runUpdate(aExpectedExitValue, a
>   }
>   logTestInfo("running the updater: " + updateBin.path + " " + args.join(" "));
> 
>   let env = AUS_Cc["@mozilla.org/process/environment;1"].
>             getService(AUS_Ci.nsIEnvironment);
>   if (gDisableReplaceFallback) {
>     env.set("MOZ_NO_REPLACE_FALLBACK", "1");
>   }
>+  env.set("MOZ_EMULATE_ELEVATION_PATH", "1");
> 
>   let process = AUS_Cc["@mozilla.org/process/util;1"].
>                 createInstance(AUS_Ci.nsIProcess);
>   process.init(updateBin);
>+
remove extra line

>   process.run(true, args, args.length);
> 
>   if (gDisableReplaceFallback) {
>     env.set("MOZ_NO_REPLACE_FALLBACK", "");
>   }
>+  env.set("MOZ_EMULATE_ELEVATION_PATH", "");

Since this won't likely make it this cycle and it will be introducing changes to updates on Windows at the same time that we are making changes to updates on Windows due to mac v2 signing please hold off on landing this again until a couple of days after mac v2 signing has landed.

Comment 129

[Brian R. Bondy [:bbondy]]

> Notcied that never has changed to default in a few places. Could you investigate that and change 
> this if it should be?

I think default has some set expiration from when it was introduced? Never sounds correct to me because we don't want it going away.


> Just to verify, the plan is for these extra error codes to away?

Correct.

> Is this just to clean up the log output?

This isn't new in this new patch, but I believe it was causing a test failure when comparing log output without it ifdef'ed out.  Because the test goes through the uac route so it calls it twice and the log gets appended.

> remove extra line

Will do.

> please hold off on landing this again until a couple of days after mac v2 signing has landed.

Will do.

Comment 130

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #128)
> Notcied that never has changed to default in a few places. Could you investigate that and change this 
> if it should be?
Looks like that was introduced here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1045108
To differentiate probes that have never had a default set from those that should actually never expire.

We definitely do want to never expire though for the status code.

Comment 131

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

Thanks for looking into it!

Comment 132

[Brian R. Bondy [:bbondy]]

Attachment: new patch to land to gather more data. v2

Carrying forward r+. Addressed nit.
Waiting to land on Oak for further testing until the Mac signing stuff is out of the way.  This is to avoid the chance of slowing down Mac signing work due to potential problems with this.

Comment 133

[Brian R. Bondy [:bbondy]]

By the way we can use a similar method as introduced in this patch for various MAR signing work when we're landing it so ensure potential problems introduced by that work doesn't stop updates.

Comment 134

[Brian R. Bondy [:bbondy]]

rstrong I think the mac stuff just landed right? Is it ok to land this on oak and then a couple days later on m-c?

Comment 135

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

Yes

Comment 136

[Benjamin Kerensa [:bkerensa]]

This is already to late for 33 so I’m going track for 34+ and Wontfix on 33.

Comment 137

[Brian R. Bondy [:bbondy]]

Kamil could you try a few updates from Oak.
Things look good to me for pushing to m-c.
 
In particular you don't need to test anything with bad dlls, just updates w/ and w/o the service on xp, vista, and win7 (or instead of win7 win8):

This is the build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-03-13-46-22-oak/firefox-35.0a1.en-US.win32.installer.exe

It should update to the latest on Oak.

If you run into any problems at all, please take a zip of the updater logs. 
Also after the browser starts back up, check about:telemetry for UPDATER_ALL_STATUS_CODES and screenshot it.  I'll attached an example screenshot above.

If everything works for you and you don't run into any errors yourself I'll push to m-c.

Comment 138

[Brian R. Bondy [:bbondy]]

Attachment: Screen Shot 2014-10-07 at 11.13.43 PM.png

Telemetry histogram to look for after the browser restarts after an update.  In particular if the update didn't apply.

Comment 139

[Kamil Jozwiak [:kjozwiak]]

I haven't forgotten about this Brian ;) Just finishing up the 31.2.0esr release. After that, this is next on my to do list. Thanks for all the info in comment #137, I'll start doing updates over the weekend and post finding/information here.

Comment 140

[Brian R. Bondy [:bbondy]]

I actually just had to back out the bug from oak for other testing. I'll ping you outside of this bug for when to continue. Should be within a few days.

Comment 141

[Lawrence Mandel [:lmandel] (use needinfo)]

Brian - Still looking to get this into 34?

Comment 142

[Brian R. Bondy [:bbondy]]

In the process of landing MAR file signing instead so this is on hold until that's landed. Hoping to get that landed within a couple weeks.

Comment 143

[Lawrence Mandel [:lmandel] (use needinfo)]

Given comment 142, I'm marking 34 as wontfix. We can consider this again for Firefox 35.

Comment 144

[Andrew McCreight [:mccr8]]

Brian, has the MAR file signing stuff landed yet?  If not, what bug is tracking that?  Can this bug move forward again?  This bug is almost a year old.  Thanks.

Comment 145

[Brian R. Bondy [:bbondy]]

Not yet, unfortunately the MAR signing is a really big project and I'm working through some hard to fix test problems right now.  I think it'll be landed in 2014 and then I can come back to this.  Tracking bug for that work is: bug 973933

Comment 146

[Lukas Blakk [:lsblakk] use ?needinfo]

We're getting close to final beta on FF35 and there's nothing new to consider here, wontfixing for 35 and putting esr31 tracking back to ? for now until we land this on mainline and know which ESR it can ride along with.

Comment 147

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

Brian, I spent some time coming up with an alternate approach so please don't work on this until I have a chance to finish it up and run it by you. Thanks!

Comment 148

[Brian R. Bondy [:bbondy]]

Sounds good, thanks!

Comment 149

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

Attachment: patch rev1 - alt approach

I tested both 32 and 64 bit on WinXP, WinVista, and Win7 using the dll's Kamil provided. The only issue is that both 32 and 64 bit WinVista crashed with apphelp.dll and had the 1 medium integrity cmd prompt. I suspect this is a bug in Vista and though it might possible to mitigate I am not too concerned with it... please let me know if you are. No other cmd prompts were created.

I verified that on Win 8.1 that SetDefaultDllDirectories does the right thing and doesn't load any of the dll's.

I haven't tested WinVista or Win7 with KB2533623 to make sure it does the right thing.

I'm going to add entries for 64 bit Firefox on Win7 in a followup patch if this approach makes sense hence why the f? vs. r?

Comment 150

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

Also haven't tested SetDefaultDllDirectories on Win8 yet.

Comment 151

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

Pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=f912a9ac41f4

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f912a9ac41f4

Comment 152

[Brian R. Bondy [:bbondy]]

Comment on attachment 8543599 [details] [diff] [review]
patch rev1 - alt approach

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

SetDefaultDllDirectories was not used originally because I thought/think (and MSDN says) that it only effects LoadLibrary based DLL loading and not implicitly loaded dependencies:
http://www.dependencywalker.com/help/html/dependency_types.htm
I'm pretty sure we tried using it before, but that could have been pre- construction of these new types of DLLs.

Comment 153

[Brian R. Bondy [:bbondy]]

Kamil are those DLLS linked above the ones created by constructing new special DLLs with fake exports which forward to the real exports when being run?
rstrong are you having the dlls all be extracted into the dir where the updater runs and for a whole update operation?

Comment 154

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

(In reply to Brian R. Bondy [:bbondy] from comment #153)
>...
> rstrong are you having the dlls all be extracted into the dir where the
> updater runs and for a whole update operation?
Yes... with staging and the service disabled. I also verified that without this patch the cmd prompts are displayed. I'll let Kamil verify but it does appear to me to be the case. I also tested updating with all dll's in place and when using the preload none of the other dll's launched a cmd prompt when using the LOAD_WITH_ALTERED_SEARCH_PATH flag whereas they did when that wasn't used. The same was true when using SetDefaultDllDirectories.

MSDN says, "The DLL search path is the set of directories that are searched for a DLL when a full path is not specified in a LoadLibrary or LoadLibraryEx function call, or when a full path to the DLL is specified but the system must search for dependent DLLs.

The standard DLL search path contains directories that can be vulnerable to a DLL pre-loading attack. An application can use the SetDefaultDllDirectories function to specify a default DLL search path for the process that eliminates the most vulnerable directories and limits the other directories that are searched. The process DLL search path applies only to the calling process and persists for the life of the process."

My interpretation of this is that when specifying LOAD_LIBRARY_SEARCH_SYSTEM32 then it will look in the system32 directory for the dll when a path is not specified. I tested this on Win8.1 and it didn't launch a cmd prompt and without it it did launch a cmd prompt.

Note that this patch no cmd prompts are launched for any of the dll's except for the one case on WinVista I mentioned.

Comment 155

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

For reference
http://msdn.microsoft.com/en-us/library/windows/desktop/hh310515%28v=vs.85%29.aspx

Comment 156

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

Brian, if you have an old type of dll I can also verify that it does the right thing. Thanks!

Comment 157

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

For comment #156

Comment 158

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #154)
> (In reply to Brian R. Bondy [:bbondy] from comment #153)
>...
> MSDN says, "The DLL search path is the set of directories that are searched
> for a DLL when a full path is not specified in a LoadLibrary or
> LoadLibraryEx function call, or when a full path to the DLL is specified but
> the system must search for dependent DLLs.
> 
> The standard DLL search path contains directories that can be vulnerable to
> a DLL pre-loading attack. An application can use the
> SetDefaultDllDirectories function to specify a default DLL search path for
> the process that eliminates the most vulnerable directories and limits the
> other directories that are searched. The process DLL search path applies
> only to the calling process and persists for the life of the process."
This doesn't cover it all by any means and the msdn page should be referenced.
http://msdn.microsoft.com/en-us/library/windows/desktop/hh310515%28v=vs.85%29.aspx

Comment 159

[Brian R. Bondy [:bbondy]]

OK I'd much prefer this approach as long as it works. I think the next step is to get some installers built for testing by Kamil to see if that works. And if so I'm fine with r+'ing this approach. Also since SetDefaultDllDirectories is win8+ only we'd need to test win7 and winvista separately. 

Regarding the medium integrity process that is started, that's not concerning to me.   Only high integrity is concerning. Although the crash could mean that there is some way not to crash that could lead to a high integrity possibly.  Not likely, but perhaps possible.

Comment 160

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

Is oak in shape for pushing the patch to create an installer?

Comment 161

[Brian R. Bondy [:bbondy]]

Not at the moment but I'm working on the windows related issue now. If there's a way to test on the try build let Kamil know, else within a few days hopefully on oak.

Comment 162

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

I took a try build and repackaged it after changing the update channel to xpcshell-test and making the precomplete a noop so it can be tested using the signed simple.mar. This requires a couple of more steps to test but it should suffice.

http://exchangecode.com/robert/work/bug945192-installer.exe

In the registry add under
HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\MaintenanceService\3932ecacee736d366d6436db0f55bce4\0
"name"="Mozilla Fake SPC"
"issuer"="Mozilla Fake CA"

After installing set
app.update.service.enabled=false
app.update.staging.enabled=false
devtools.errorconsole.enabled=true

Add the following new string pref
app.update.url.override=https://exchangecode.com/robert/work/snippets/bug945192.xml

Start an update by opening the about window.

You can get the path to the update directory by opening the error console under tools -> web developer -> error console and evaluating the following in the code box

const Cc = Components.classes; const Ci = Components.interfaces; var fileLocator = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties); var dir = fileLocator.get("UpdRootD", Ci.nsIFile); dir.path;

Place the dlls to check alongside the mar file in the updates\0 sub directory of the update directory found in the previous step.

Restart to update to test.

Comment 163

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

Attachment: updater.exe without changes

This is a try server updater without the changes from patch "rev1 - alt approach". By replacing the updater from the installer I linked to with this you can test the failure case with the dlls.

Comment 164

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

Kamil, are the above steps sufficient for you to test?

Comment 165

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

regarding comment #164

Comment 166

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #162)
>...
> Add the following new string pref
> app.update.url.override=https://exchangecode.com/robert/work/snippets/
> bug945192.xml
That should be an http url... so
app.update.url.override=http://exchangecode.com/robert/work/snippets/bug945192.xml

Comment 167

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

I verified that SetDefaultDllDirectories is called on WinVista x86 with KB2533623 installed and that updates work without launching a cmd prompt with all dll's except apphelp.dll.

I also verified that the apphelp.dll is causing the WinVista crash before AutoLoadSystemDependencies is even called so the mitigations won't prevent the crash.

Comment 168

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

Attachment: patch rev2 - alt approach

An early return was accidentally removed.

Comment 169

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

I'll update the installer after I get another try build

Comment 170

[Brian R. Bondy [:bbondy]]

This is not expected to help the problem on Vista and Windows 7 right? If so, we should keep this bug hidden when it is fixed and post another new bug to handle the Windows 7 and vista cases.  Also if so we can get Kamil to only test on Win8 to save time.

Comment 171

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

I believe it fixes the problem on all versions.

When SetDefaultDllDirectories is available (WinVista and Win7 with KB2533623 and Win8 and above) everything is fixed.

When SetDefaultDllDirectories is not available (WinVista and Win7 without KB2533623 and WinXP) it preloads dll's using LoadLibraryExW with the LOAD_WITH_ALTERED_SEARCH_PATH flag. From msdn:
"If this value is used and lpFileName specifies an absolute path, the system uses the alternate file search strategy discussed in the Remarks section to find associated executable modules that the specified module causes to be loaded."

"If lpFileName specifies an absolute path and dwFlags is set to LOAD_WITH_ALTERED_SEARCH_PATH, LoadLibraryEx uses the altered search path. The behavior is undefined when LOAD_WITH_ALTERED_SEARCH_PATH flag is set, and lpFileName specifiies a relative path."

In my testing, other dll's loaded by the dll loaded using LoadLibraryEx with the LOAD_WITH_ALTERED_SEARCH_PATH flag also use the altered search path.

Are there other cases that you know of?

Comment 172

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

Note: the WinVista apphelp.dll case is not fixed since it crashes and has a medium integrity cmd prompt as previously mentioned.

Comment 173

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

Note: the "altered search path" is defined at
http://msdn.microsoft.com/en-us/library/windows/desktop/ms682586%28v=vs.85%29.aspx#alternate_search_order_for_desktop_applications

"Alternate Search Order for Desktop Applications

The standard search order used by the system can be changed by calling the LoadLibraryEx function with LOAD_WITH_ALTERED_SEARCH_PATH. The standard search order can also be changed by calling the SetDllDirectory function.

Windows XP:  Changing the standard search order by calling SetDllDirectory is not supported until Windows XP with Service Pack 1 (SP1).

If you specify an alternate search strategy, its behavior continues until all associated executable modules have been located. After the system starts processing DLL initialization routines, the system reverts to the standard search strategy.

The LoadLibraryEx function supports an alternate search order if the call specifies LOAD_WITH_ALTERED_SEARCH_PATH and the lpFileName parameter specifies an absolute path.

Note that the standard search strategy and the alternate search strategy specified by LoadLibraryEx with LOAD_WITH_ALTERED_SEARCH_PATH differ in just one way: The standard search begins in the calling application's directory, and the alternate search begins in the directory of the executable module that LoadLibraryEx is loading.

If SafeDllSearchMode is enabled, the alternate search order is as follows:

    The directory specified by lpFileName.
    The system directory. Use the GetSystemDirectory function to get the path of this directory.
    The 16-bit system directory. There is no function that obtains the path of this directory, but it is searched.
    The Windows directory. Use the GetWindowsDirectory function to get the path of this directory.
    The current directory.
    The directories that are listed in the PATH environment variable. Note that this does not include the per-application path specified by the App Paths registry key. The App Paths key is not used when computing the DLL search path."

So, it uses the dll's directory since it is loaded using a full path and the LOAD_WITH_ALTERED_SEARCH_PATH flag is specified.

Comment 174

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

As long as all affected dll's are identified and preloaded on WinXP through Win7 the patch should fix all of the cases.

Comment 175

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

I updated the installer (includes the change in the latest patch) at:
http://exchangecode.com/robert/work/bug945192-installer.exe

Testing using the installer should be good to go. I verified that it fixed this bug on Win8.1 x64 as well as the failure case using the updater.exe attached to this bug.

Note: I had to disable smartscreen after replacing the updater.exe with the one attached to this bug when testing the failure case.

Comment 176

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #175)
>...
> Note: I had to disable smartscreen after replacing the updater.exe with the
> one attached to this bug when testing the failure case.
Likely due to it being marked as downloaded from the internet

Comment 177

[Kamil Jozwiak [:kjozwiak]]

(In reply to Brian R. Bondy [:bbondy] from comment #153)
> Kamil are those DLLS linked above the ones created by constructing new
> special DLLs with fake exports which forward to the real exports when being
> run?

Yup, the DLL's that I used/attached are proxy DLL's that are based on the exports of the original unknown DLL's that I found using WinObj and Process Monitor. For each OS, I added the original unknown DLL and the proxy DLL, example:

* apphelp.dll -> proxy DLL
* apphelp_orig.dll -> original DLL
* crypt32.dll -> proxy DLL
* crypt32_orig.dll -> original DLL

Place both the proxy and the original DLL into the "updates/0" folder to reproduce the issue.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #164)
> Kamil, are the above steps sufficient for you to test?

Steps look good, I'll test this first thing in the morning once I get into the office! I'll ping you on IRC if I run into any problems or have any questions.

Comment 178

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

I checked on WinVista and saw some issues when only using SetDefaultDllDirectories that were not present when using both before I added the early return so I reverted it back to the previous patch.

Comment 179

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

Just noticed on Vista when using SetDefaultDllDirectories that secur32.dll causes the update to fail though these is no cmd prompt. I might just not use SetDefaultDllDirectories on Vista since preloading works with secur32.dll. Need to check Win7 as well for similar issues.

Comment 180

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

FYI: I'm seeing weirdness on Vista that I didn't see before. I'm not seeing it elsewhere so it might just be my system.

Comment 181

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

I think I might be going on.

Kamil, please verify Win8 and Win7 to give me time to investigate. Note: Win7 might also be affected.

Comment 182

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #179)
> Just noticed on Vista when using SetDefaultDllDirectories that secur32.dll
> causes the update to fail though these is no cmd prompt. I might just not
> use SetDefaultDllDirectories on Vista since preloading works with
> secur32.dll. Need to check Win7 as well for similar issues.
s/secur32.dll/rsaenh.dll/

Comment 183

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

Verified that shimeng.dll tries to load apphelp.dll from the exe dir before AutoLoadSystemDependencies on Vista

Comment 184

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #181)
> I think I might be going on.
I think I might know what is going on.

> Kamil, please verify Win8 and Win7 to give me time to investigate. Note:
> Win7 might also be affected.
WinXP as well please.

Comment 185

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

Since I have this code on the brain I went ahead and tested WinXP and Win7 and they both suffer from the same problem as WinVista. For some reason wsock32.dll is still using rsaenh.dll from the app dir though it isn't launching a cmd prompt... I'm investigating.

Kamil, if you do check anything just check Win8 and Win8.1 until I track this down.

Comment 186

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

The issue with rasenh.dll is in part due to when crypto is used it reads the registry to find the dll to load and for some reason doesn't respect the altered path. On Vista (I assume xp as well) the ImagePath value is rsaenh.dll and on Win8 it is %SystemRoot%\system32\rsaenh.dll.

All other dll's do the right thing with the patch except for apphelp.dll on Vista which appears to load before we can do anything. Also, it crashes and elevation doesn't occur.

Brian, any suggestions on how to deal with this? I am sorely tempted to change the registry key to a full path before cert checking and backing out the change when it completes.

Comment 187

[Brian R. Bondy [:bbondy]]

I'm surprised that with loading via only "rsaenh.dll" that it doesn't use the already loaded module from loaddlls.cpp.  Did you try changing to an absolute path in the registry to verify that it does indeed fix the problem?

Comment 188

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

I have changed it and it worked without a problem... changed it back and it failed again. I suspect that it is doing its own loadlibrary call after reading the registry and that is using the default search path which would look in the app dir first since it doesn't have an absolute path.

Comment 189

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

which is lame! Just had to say that! :P

Comment 190

[Brian R. Bondy [:bbondy]]

I cant' think of another way to fix it, I don't like modifying the registry for things that aren't ours though, but I'd r+ it in the absence of a better solution.

Comment 191

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

I hate to ask but is it any more possible today to use NSS on Windows? I ask in part since there is the desire to have SHA2 support as well. Just weighing options though I think I'd prefer just changing the reg key.

Comment 192

[Brian R. Bondy [:bbondy]]

I think it was always possible but just not desired by others. Don't remember who for sure, but maybe it was bsmith who didn't want to make app updates dependent on nss in case nss breaks.  There could be other reasons too which I don't know of, but as I recall that was the reasoning. It would probably be not a lot of config changes to do it since we already have it working for Linux.  Another option is just doing sha2 on the platforms that support it for minimal risk/changes, and since eventually all platforms will support it. But that gets into a lot of other discussions so I'll cut it off there :)

Comment 193

[Kamil Jozwiak [:kjozwiak]]

* installed bug945192-installer.exe from comment # 162
* changed the "name" and "issuer" keys via HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\MaintenanceService\a4948786b022cc1a376991b44efb838c\0
* restarted the VM machine
* set "app.update.service.enabled=false"
* set "app.update.staging.enabled=false"
* set "devtools.errorconsole.enabled=true"
* added "app.update.url.override=https://exchangecode.com/robert/work/snippets/bug945192.xml"
* changed the "default" update channel to "xpcshell-test" via channel-prefs.js

I kept receiving the following error under the error console so I added https://exchangecode.com into the security exception list and continued with the update.

exchangecode.com:443 uses an invalid security certificate.
The certificate is not trusted because no issuer chain was provided.
The certificate expired on 5/15/2008 8:00 PM.
(Error code: sec_error_unknown_issuer)

Once I configured everything using the above steps, I selected "About" and received an update which added "update.mar", "update.status" and "update.version" into "C:\Users\Kamil Jozwiak\AppData\Local\Mozilla\updates\EEFEA8717BC47F65\updates\0". At that point, I added "bcrypt.dll" and "bcrypt_orig.dll" into the "updates\0" folder and selected "Restart Nightly to Update".

I tried this using the new updater.exe that comes with the installable from comment # 162 and the updater.exe from comment # 175:

- updater.exe from the installable in comment # 162 didn't launch any CMD windows (using bcrypt.dll)
- updater.exe from comment # 175 launched a bunch of CMD's in High Integrity (using bcrypt.dll)

However, once fx restarted, I always receive a prompt indicating that "The Update could not be installed (patch apply failed)".

Robert, does this look like it's correct? I'm not sure if fx should actually be updating or is the failure expected? If this is correct, I'll go through the rest of the DLL's on Win 8 x86/x64 and Win 8.1 x86/x64.

Comment 194

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

(In reply to Kamil Jozwiak [:kjozwiak] from comment #193)
> * installed bug945192-installer.exe from comment # 162
> * changed the "name" and "issuer" keys via
> HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\MaintenanceService\a4948786b022cc1a376991
> b44efb838c\0
> * restarted the VM machine
> * set "app.update.service.enabled=false"
> * set "app.update.staging.enabled=false"
> * set "devtools.errorconsole.enabled=true"
> * added
> "app.update.url.override=https://exchangecode.com/robert/work/snippets/
> bug945192.xml"
> * changed the "default" update channel to "xpcshell-test" via
> channel-prefs.js
> 
> I kept receiving the following error under the error console so I added
> https://exchangecode.com into the security exception list and continued with
> the update.
Use http and not https instead. ( comment #166 )

> exchangecode.com:443 uses an invalid security certificate.
> The certificate is not trusted because no issuer chain was provided.
> The certificate expired on 5/15/2008 8:00 PM.
> (Error code: sec_error_unknown_issuer)
> 
> Once I configured everything using the above steps, I selected "About" and
> received an update which added "update.mar", "update.status" and
> "update.version" into "C:\Users\Kamil
> Jozwiak\AppData\Local\Mozilla\updates\EEFEA8717BC47F65\updates\0". At that
> point, I added "bcrypt.dll" and "bcrypt_orig.dll" into the "updates\0"
> folder and selected "Restart Nightly to Update".
> 
> I tried this using the new updater.exe that comes with the installable from
> comment # 162 and the updater.exe from comment # 175:
> 
> - updater.exe from the installable in comment # 162 didn't launch any CMD
> windows (using bcrypt.dll)
> - updater.exe from comment # 175 launched a bunch of CMD's in High Integrity
> (using bcrypt.dll)
> 
> However, once fx restarted, I always receive a prompt indicating that "The
> Update could not be installed (patch apply failed)".
> 
> Robert, does this look like it's correct? I'm not sure if fx should actually
> be updating or is the failure expected? If this is correct, I'll go through
> the rest of the DLL's on Win 8 x86/x64 and Win 8.1 x86/x64.
Thanks Kamil! That appears to be due to the rdaenh.dll issue. What OS were you using?

Comment 195

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

s/rdaenh.dll/rsaenh.dll/

Comment 196

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

BTW: I also see the rsaenh.dll issue with builds without this patch

Comment 197

[Kamil Jozwiak [:kjozwiak]]

> Thanks Kamil! That appears to be due to the rdaenh.dll issue. What OS were you using?

I was using Win 8.1 x64. I went through it one more time using the http:// link rather than the https:// and received "The Update could not be installed (patch apply failed)".

(OS's below are up to date with the latest MS updates)

- Win 8 x64 -> received the prompt
- Win 8.1 x64 -> received the prompt

> BTW: I also see the rsaenh.dll issue with builds without this patch

So should I continue and test all the DLL's on the Win 8 machines (VM's) or wait?

Comment 198

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

No need to continue testing at this time until there is a resolution to rsaenh.dll preventing applying the update.

Could I get the value of the following in the registry on the system you tested?
HKEY_LOCAL_MACHINE

SOFTWARE\Wow6432Node\Microsoft\Cryptography\Defaults\Provider\Microsoft Enhanced Cryptographic Provider v1.0

Image Path

Comment 199

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

I think I missed something. Kamil, you didn't have rsaenh.dll in the directory when you tested?

Comment 200

[Kamil Jozwiak [:kjozwiak]]

I was using bcrypt.dll on Win 8.1 x64 and cryptsp.dll on Win 8 x64. I just picked them randomly from the 7zip archive I attached earlier to quickly test and make sure I was doing it correctly before running through all the other DLL's. Once the updater pulled update.mar, update.status, update.version, I added bcrypt.dll and bcrypt_orig.dll into "\updates\0".

* Win 8.1 x64:
** placed bcrypt.dll and bcrypt_orig.dll into "\updates\0" once update.mar, update.status, update.version was pulled

* Win 8 x64
** placed cryptsp.dll and cryptsp_orig.dll into "\updates\0" once update.mar, update.status, update.version was pulled

> Could I get the value of the following in the registry on the system you tested?

Win 8 x64 -> %SystemRoot%\system32\rsaenh.dll
Win 8.1 x64 -> %SystemRoot%\system32\rsaenh.dll

Comment 201

[Kamil Jozwiak [:kjozwiak]]

> I think I missed something. Kamil, you didn't have rsaenh.dll in the directory when you tested?

Nope, I was using bcrypt.dll/bcrypt_orig.dll and cryptsp.dll/cryptsp_orig.dll (comment #200 for details). Let me know if I'm doing something wrong! Was I supposed to be using rsaenh.dll?

Comment 202

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

You aren't dong anything wrong... it is just strange that I didn't see that as well on my system.

Comment 203

[Kamil Jozwiak [:kjozwiak]]

I'm using VM's but not sure if that's the cause? When I get home tonight I can try it on my dedicated Win 8.1 x64 desktop and see if I see the same thing happening. I also have a X1 Carbon with Win 8.1 that I can try it with.

Comment 204

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

I've been using hardware to test Win8.1 and I've been meaning to test it in a VM so I'll go ahead and do that.

Comment 205

[Kamil Jozwiak [:kjozwiak]]

I ran through the steps on my Desktop & X1 Carbon which are both running Win 8.1 x64 and got the same "The Update could not be installed (patch apply failed)" prompt. Used bcrypt.dll and bcrypt_orig.dll as the DLL's.

Both machines had Image Path set as --> %SystemRoot%\system32\rsaenh.dll

Comment 206

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

Since incorrect reg entries can also cause this, were you able to successfully update without the dll's?

Comment 207

[Kamil Jozwiak [:kjozwiak]]

Attachment: last-update.log

I get the same error when I try to update without adding the DLL's into the update directory. I've attached the "last-update.log" in case it sheds some light. Let me know if I can attach anything else that will help you!

Comment 208

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

Attachment: cert.reg

Try it again after importing these reg entries by double clicking the file, etc.

Comment 209

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

Kamil, I suspect it is due to the reg entries being incorrect especially since it works for me on Win 8.1 x64 on hardware and Win 8 x86 on a VM. The attachment should resolve it for you. Thanks

Comment 210

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

After testing on all Windows versions though not all SPs it appears that the rsaenh.dll issue only affects WinXP and WinVista. I also checked the registry on the systems and only WinXP and WinVista don't specify the path to rsaenh.dll. Since other versions of Windows specify the full path and this only affecting older versions of Windows I'm much more ok with changing the registry when possible.

Comment 211

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

Kamil, you might want to delete the reg entries parent before importing the reg file

Comment 212

[Kamil Jozwiak [:kjozwiak]]

Robert, that definitely did it. I wasn't aware that I need to add the exact reg for this to work and was editing the one my machine created. I quickly tried it by applying the .reg from comment # 209 and it updated without displaying the error prompt. Apologies for the back and forth :/ Learned something new though!

I'll go through all the DLL's on Win 8 x64/x86 and Win 8.1 x64/x86 first thing tomorrow morning.

Comment 213

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

Kamil, not a problem whatsoever and I'm glad everything worked out!

You can test Win7 and above with 32 bit Firefox. After I get a hack in for the registry entry WinXP and WinVista should be good to test.

Would you be able to get the list of DLL's on Win7 with Firefox 64 bit as you did for Firefox 32 bit? Only Win7 is needed since we only support Firefox on Win7 and above and the call to SetDefaultDllDirectories fixes this bug on Win8 and above. Thanks!

Comment 214

[Kamil Jozwiak [:kjozwiak]]

Using the updater.exe from comment # 163, I reproduced the original issue. Once reproduced, I went through all the DLL's using the following OS's:

* Quick Note: used fx x86 for the below tests

- Win 8 x64 - PASSED (no CMD prompts appearing)
- Win 8.1 x64 - PASSED (no CMD prompts appearing)
- Win 8 x86 - PASSED (no CMD prompts appearing)
- Win 8.1 x86 - PASSED (no CMD prompts appearing)
- Win 7 SP1 x64 - PASSED (no CMD prompts appearing)
- Win 7 SP1 x86 - PASSED (no CMD prompts appearing)

> Would you be able to get the list of DLL's on Win7 with Firefox 64 bit as you did for Firefox 32 bit? 
> Only Win7 is needed since we only support Firefox on Win7 and above and the call to
> SetDefaultDllDirectories fixes this bug on Win8 and above. Thanks!

I'll try getting it done sometime this week but it will be pretty hard as I'm doing fx35 and ESR verifications (release week). I can get it done Monday/Tuesday of next week. Let me know if you need it sooner.

Comment 215

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

Attachment: patch w/ registry hack

I didn't add comments since that would make the local exploit obvious

Comment 216

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

Also, pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=11b93804f541

Kamil, thanks for everything and when you can get to it would be great!

Comment 217

[Brian R. Bondy [:bbondy]]

Comment on attachment 8545187 [details] [diff] [review]
patch w/ registry hack

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

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2134,5 @@
>  
>  #ifdef MOZ_VERIFY_MAR_SIGNATURE
>      if (rv == OK) {
> +#ifdef XP_WIN
> +      HKEY baseKey;

Please initialize this, baseKey may not be set to NULL always on all failure cases in RegOpenKeyExW.

Comment 218

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

Attachment: patch w/ registry hack comments addressed

Comment 219

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

New installer for testing Firefox 32 bit
http://exchangecode.com/robert/work/bug945192-installer-v2.exe

I've included the change to app.update.url.override in this installer so it doesn't need to be added during testing.

Comment 220

[Brian R. Bondy [:bbondy]]

Comment on attachment 8545438 [details] [diff] [review]
patch w/ registry hack comments addressed

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

Thanks and thanks for searching for other instances of this as well.

Comment 221

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #219)
> New installer for testing Firefox 32 bit
> http://exchangecode.com/robert/work/bug945192-installer-v2.exe
> 
> I've included the change to app.update.url.override in this installer so it
> doesn't need to be added during testing.
Turns out this doesn't cover everything so I need to do more investigation. :(

Comment 222

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

Attachment: patch w/ registry hack comments addressed

Fixed typo.

New try run to generate signed files
https://tbpl.mozilla.org/?tree=Try&rev=64e60a206537

Comment 223

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

New installer for testing Firefox 32 bit using the latest patch
http://exchangecode.com/robert/work/bug945192-installer-v2.exe

I've included the change to app.update.url.override in this installer so it doesn't need to be added during testing.

I've tested this on WinXP SP2, Vista x86 and x64 with and without KB2533623, Windows 8 x86, and Windows 8.1 x64 and it addresses everything for Firefox x86 except the crash due to apphelp.dll on Vista x86 and x64 which crashes before elevation, a non-elevated cmd prompt is launched, and no elevated cmd prompt is launched.

I think the patch as it stands covers everything for Firefox x86. The only thing left is getting the DLL's for Firefox x64 on Win7.

Kamil, I wouldn't bother testing until after Firefox x64 is addressed.

Comment 224

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

I also tested Win7 x86 and x64 with and without KB2533623 and it addressed this bug as well.

Comment 225

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

Comment on attachment 8545679 [details] [diff] [review]
patch w/ registry hack comments addressed

Let's get this reviewed and I'll create a separate patch for x64.

Comment 226

[Kamil Jozwiak [:kjozwiak]]

> I think the patch as it stands covers everything for Firefox x86. The only thing left is getting the
> DLL's for Firefox x64 on Win7.

I'll start that sometime this weekend and should be done by Monday. Robert, should I go through all the DLL's in Vista/XP like I did in comment # 214 for the x86 patch?

Comment 227

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

Kamil, you can wait until I get a Firefox x64 version in case more changes are needed so you won't have to repeatedly test.

Comment 228

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

Besides, imo it is too late in the cycle to land this so my only rush is to get this off my plate. :)

Comment 229

[Kamil Jozwiak [:kjozwiak]]

Great, sounds good! I'll attach the x64 DLL's sometime this weekend/Monday.

Comment 230

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #224)
> I also tested Win7 x86 and x64 with and without KB2533623 and it addressed
> this bug as well.
Also on WinXP SP2 x64 and it addressed this bug as well. So... yay!

Comment 231

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

dveditz, if / when this lands it is going to be pretty obvious what is being addressed. How would you like to handle it and specifically around timing during the next cycle and code comments that will just make it easier what is being addressed?

Comment 232

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #231)
> dveditz, if / when this lands it is going to be pretty obvious what is being
> addressed. How would you like to handle it and specifically around timing
> during the next cycle and code comments that will just make it easier what
> is being addressed?
s/make it easier what/make it easier to understand what/

Comment 233

[Brian R. Bondy [:bbondy]]

Comment on attachment 8545679 [details] [diff] [review]
patch w/ registry hack comments addressed

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

Sorry for the delay, looks good.

Comment 234

[Daniel Veditz [:dveditz]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #231)
> if / when this lands it is going to be pretty obvious what is being
> addressed. How would you like to handle it and specifically around timing
> during the next cycle and code comments that will just make it easier to
> understand what is being addressed?

Assuming we're confident in the patch enough to land all branches (beta + ESR in particular) how much testing do you need? We're at the beginning of a cycle--can you get enough testing if you land now and uplift to branches in the next week or so?

It's fairly obvious what's going on from the patch, but it's a local exploit rather than remote so that takes a lot of the heat off. And no one wants to break the updater! So it'd be ideal to get it into the same release as the cycle you land in (that is, if you land now get it into Firefox 36) but apart from that land as early as you need to get sufficient testing.

Above is provisional, please request sec-approval? on the patch and answer the questions just in case some detail changes the picture. But "soon" is probably fine.

Comment 235

[Kamil Jozwiak [:kjozwiak]]

Attachment: wrappit.cpp

Robert, so I went through the entire process on Win 7 x64 using fx x64 and generated all the proxy DLL's using the same method as before. However, I couldn't reproduce the original issue using the DLL's I've listed below.. The CMD prompts never appeared. I tried several times, created the proxy DLL's a few times to make sure I was doing it correctly but still couldn't get any CMD prompts to appear.

I think the program that I'm using is always creating x86 proxy DLL's rather than x64 DLL's hence not being able to reproduce the problem on x64. Either that or fx x64 is doing something differently than fx x86?

I've attached the source of the program that I'm using, could you take a look and see if that's the case? If it is, would you be able to modify it so it spits out x64 DLL's?? (if that's even the issue)

Program being used: http://www.codeproject.com/Articles/16541/Create-your-Proxy-DLLs-automatically

Unknown DLL's (FX x64 on Win 7 x64):

C:\Windows\System32\wsock32.dll
C:\Windows\System32\cryptsp.dll
C:\Windows\System32\cryptbase.dll
C:\Windows\System32\secur32.dll
C:\Windows\System32\ws2help.dll
C:\Windows\System32\apphelp.dll
C:\Windows\System32\bcryptprimitives.dll
C:\Windows\System32\bcrypt.dll
C:\Windows\System32\uxtheme.dll
C:\Windows\System32\propsys.dll
C:\Windows\System32\ntmarta.dll
C:\Windows\System32\shdocvw.dll
C:\Windows\System32\version.dll
C:\Windows\System32\sspicli.dll
C:\Windows\System32\api-ms-win-downlevel-advapi32-l2-1-0.dll
C:\Windows\System32\mpr.dll
C:\Windows\System32\rsaenh.dll
C:\Windows\System32\dwmapi.dll

Comment 236

[Kamil Jozwiak [:kjozwiak]]

After doing a quick search, using "dumpbin.exe /headers name.dll", reveals that the proxy DLL's being generated have "x86" under "FILE HEADER VALUES"

Comment 237

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

I went through the dll's loaded and limited the preloads to just the dlls loaded from the update directory which I believe will solve this. I'll verify over the weekend.
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8b431b099e47/

Comment 238

[Kamil Jozwiak [:kjozwiak]]

Attachment: bcrypt high integrity cmd.exe on Win 7 x64

Robert, I pulled an all nighter and found out that the original creator of wrappit (the cmd tool used to create the x86 DLL's) also created wrappit2 that supports x64 DLL's. Unfortunately there was barely any documentation so I spent the night hacking away at it and finally figured it out :) Learned a lot about the entire linking/compiling process! I double checked and made sure that the proxy is x64 and reproduced the cmd.exe in high integrity on my VM Win 7 x64.

Could you check it out on your end and see if you can also reproduce the high integrity cmd.exe? If it works for you, I'll start creating all the other proxy x64 DLLs (shouldn't take that long as I have the process down)

Used the following STR:

- Win 7 Pro SP1 x64
- Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-01-13-03-02-05-mozilla-central/
- Once installed --> app.update.service.enabled;false (restarted the browser)
- add bcrypt.dll and bcryptold.dll into "updates\0"

Comment 239

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

I was able to reproduce using your bcrypt.dll. Awesome!

Comment 240

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

Kamil, could you please create a Vista x64 proxy apphelp.dll as well?

Comment 241

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

So, I went with preloading the dll's that are loaded from the updates/0 directory based on procmon to create the following Firefox x86 and x64 builds
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8b431b099e47/

Everything looks good on Firefox x64 on Win 7 x64 through Win8.1 x64 and on WinXP through Win8.1 both x86 and x64 with the following exceptions.

Firefox x86 - apphelp.dll is loaded on Win Vista x86 and x64 before the preloading code.
Firefox x86 - wow64log.dll is loaded on Win XP x64 before the preloading code.

I'm going to try to come up with a way to prevent these 2 dll's from loading over the next couple of days.

Comment 242

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

Attachment: patch - only preload dll's that need it and Firefox x64 support

Comment 243

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

Attachment: patch - only preload dll's that need it and Firefox x64 support

Forgot to clean it up of the dll's I was testing

Comment 244

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

Try build for the latest patches
https://tbpl.mozilla.org/?tree=Try&rev=49f1e116b014

Comment 245

[Kamil Jozwiak [:kjozwiak]]

Attachment: Win 7 Pro FX x64.7z

Went through each of the following DLL's and ensured that they're all x64 DLL's. I also went through each DLL twice using my Win 7 Pro SP1 x64 VM and ensured I got the same results:

C:\Windows\System32\wsock32.dll --> PASSED
C:\Windows\System32\cryptsp.dll --> PASSED
C:\Windows\System32\cryptbase.dll --> PASSED
C:\Windows\System32\secur32.dll --> PASSED
C:\Windows\System32\ws2help.dll --> PASSED
C:\Windows\System32\apphelp.dll --> PASSED
C:\Windows\System32\bcryptprimitives.dll --> PASSED
C:\Windows\System32\bcrypt.dll --> FAILED [1 medium & 1 high cmd.exe]
C:\Windows\System32\uxtheme.dll --> PASSED
C:\Windows\System32\propsys.dll --> PASSED [1 medium cmd.exe]
C:\Windows\System32\ntmarta.dll --> PASSED [1 medium cmd.exe]
C:\Windows\System32\shdocvw.dll --> PASSED
C:\Windows\System32\version.dll --> PASDED [1 medium cmd.exe]
C:\Windows\System32\sspicli.dll --> PASSED [1 medium cmd.exe]
C:\Windows\System32\api-ms-win-downlevel-advapi32-l2-1-0.dll --> PASSED [1 medium cmd.exe]
C:\Windows\System32\mpr.dll --> PASSED [1 medium cmd.exe]
C:\Windows\System32\rsaenh.dll --> PASSED
C:\Windows\System32\dwmapi.dll --> FAILED (1 high cmd.exe)

Comment 246

[Kamil Jozwiak [:kjozwiak]]

I've also added the results to the intra wiki:
* https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking_via_Update

I'll add all the steps on how to create a x64 proxy DLL's sometime this week when I have some time.

Comment 247

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

The try builds are at
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-49f1e116b014/

Everything passes except for the previous mentioned exceptions using the Firefox x86 build on WinXP, WinVista, Win7, Win8, and Win8.1 on Windows x86 and x64.

Everything passes using the Firefox x64 build on Win7, Win8, and Win8.1 on Windows x64.

So close...

Note: I used process monitor to find the attempts to load dlls in the same directory as the updater.exe and preloaded only those. After creating the build with only those pre-loaded I verified that no additional dll load attempts were made in that directory.

Comment 248

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

Comment on attachment 8551010 [details] [diff] [review]
patch - only preload dll's that need it and Firefox x64 support

Brian, I'll look into how the exceptions can be handled and if I come up with a way I'll submit a separate patch.

Comment 249

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

Attachment: Combined patch

Carrying forward r+

Comment 250

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

The patch as it stands I believe covers loading all dll's placed in the directory alongside the updater.exe except for the following cases using Firefox x86:
Windows Vista x86 and x64 - apphelp.dll
Windows XP x64 - wow64log.dll

Firefox x64 is fixed.

To fix this for the two cases above the directory where the updater.exe would need to be secured which we can't guarantee with our current policy of allowing users to install into a directory of their own choosing and the fact that the user can change the permissions of the directory.

To address these two cases I am planning on running the updater from the application directory when not using the service. This would require not staging updates when not using the service which I don't see as a major issue since the vast majority of users use the maintenance service with less than 5% not using it according to telemetry. The visible affect of not using staging is it taking longer to apply an update during startup.

Making app update use the updater.exe from the application directory will take time to implement and I am unsure of the amount of time it will take. Since the affected systems after the current patch lands are a very small subset of the Windows install base we need to decide whether or not we should hold off on landing the current patch until this additional work is completed.

Comment 251

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

dveditz, I'd like your take on comment #250

Comment 252

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

Note: on Vista apphelp.dll is loaded due to the app compat database shimming *update* for various values (e.g. filename, original filename, etc.).

Comment 253

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

Investigating... on Vista, adding an updater.exe.Local directory checks for a sub-directory of
x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.6002.18005_none_5cb72f96088b0de0

Comment 254

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

Although the documentation states that an embedded manifest disables .Local redirection my testing shows that it doesn't disable it entirely when using a <exe_full_name>.Local directory in that it attempts to load the comctl32.dll SxS from within a sub-directory of the <exe_full_name>.Local directory and it does so before the preloading code. A couple of examples:

Win Vista
updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.6002.18005_none_5cb72f96088b0de0

Win 7
updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.7601.17514_none_41e6975e2bd6f2b2

There are no other attempts to load DLLs from this location.

Redirection does not occur with an updater.exe.Local file.

Comment 255

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

Kamil, could you create a Win Vista x86 proxy dll for
C:\Windows\winsxs\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.6002.18005_none_5cb72f96088b0de0\comctl32.dll

and a Win 7 x86 proxy dll for
C:\Windows\winsxs\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.7601.17514_none_41e6975e2bd6f2b2\comctl32.dll

Thanks!

Comment 256

[Kamil Jozwiak [:kjozwiak]]

Attachment: comctl32 DLL.7z

Attached the DLL's, apologies it took so long! I had to get Vista up and running and it's the slowest to install :/

Let me know if you need anything else!

Comment 257

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

On Win 7 x86 commctl32.dll opened one cmd prompt without elevation.

On Win Vista x86 commctl32.dll opened two cmd prompts... 1 without elevation and 1 with elevation.

Locations of the dlls had to be as follows

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

The original dll had to be prefixed with a '.' and located in the following directory
updates\0\.comctl32_orig.dll

Win Vista
updates\0\updater.exe.Local\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.6002.18005_none_5cb72f96088b0de0\comctl32.dll

The original dll had to be prefixed with a '.' and located in the following directory
updates\0\.comctl32_orig.dll

Brian, what do you think of the approach I outlined in comment #250?

Comment 258

[Brian R. Bondy [:bbondy]]

I think it would be good to land without the additional work but just keep this bug private if that's possible.  That's probably a call for someone on the sec team (I see you have dveditz flagged already for more info for that).

I agree with running the update from the application directory and not using bgupdates. We should minimize this to only on the OS' where it is needed and keep existing flows without the service the same.  If that doesn't work for some reason we could always fall back to the original patch for this, but it was failing when landed for some machines for an unknown reason via telemetry stats so I'd rather avoid that original approach that I did if possible.

Comment 259

[Daniel Veditz [:dveditz]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #251)
> dveditz, I'd like your take on comment #250

250+ comments! MS has really made a mess of this, haven't they?

I'd go ahead and land the great improvements you've already got: it solves the problem for people who have purchased their computer in the last 5 years, and that number is only going to grow. Make them safe and then we can worry about Win XP/Vista separately.

Comment 260

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

Not surprised to see that Win XP also has the updater.exe.Local SxS bug
updater.exe.Local\WOW64_Microsoft.Windows.Common-Controls_6595b64144ccf1df_6.0.3790.3959_x-ww_5FA17F4E\comctl32.dll

I verified again that Win7 doesn't load the updater.exe.Local\*\comctl32.dll in the elevated process.

I'll check whether Win8 and Win8.1 also don't load the updater.exe.Local\*\comctl32.dll in the elevated process and I think that will be it for this approach and I'll work on implementing comment #250 hopefully soon after.

Comment 261

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

Win 8 and Win 8.1 aren't affected by having an updater.exe.Local file... weak woot.

Comment 262

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

Attachment: bug945192-2

This should be the last preloading patch. If comctl32.dll is not delay loaded that it does load in the elevated process on Win 7 and that if updater.exe crashes it will try to load dwmapi.dll from the app dir.

Try builds
https://treeherder.mozilla.org/#/jobs?repo=try&revision=325801eb5dc0

https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-325801eb5dc0/

Comment 263

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

Attachment: combined patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
As long as there is local file system access fairly easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I did add one comment of "+    Note: dwmapi.dll is preloaded since a crash will try to load it from the application's directory." but I think the code as is makes it fairly obvious.

Which older supported branches are affected by this flaw?
All

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I think it will apply cleanly but if not it is trivial to create backports.

How likely is this patch to cause regressions; how much testing does it need?
Very 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 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.

Comment 264

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

Comment on attachment 8556256 [details] [diff] [review]
combined patch

OMG, so many comments.

sec-approval+ for trunk. Can we get Aurora, Beta, and ESR31 patches made and nominated as well (for after it gets on trunk)?

Comment 265

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

Attachment: esr31 combined patch

Only an esr31 patch was needed since the existing patch applies cleanly to aurora and beta.

Comment 266

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

For the complete fix as outlined in comment #250 I'd like to use a new bug. Are you ok with my doing this?

Comment 267

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

Comment on attachment 8556256 [details] [diff] [review]
combined patch

[Triage Comment]

Comment 268

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

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/8f65c2be0e01

Filed bug 1127481 for followup as outlined in comment #250

Comment 269

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a04793749000
https://hg.mozilla.org/releases/mozilla-beta/rev/cf270a9a66ad
https://hg.mozilla.org/releases/mozilla-esr31/rev/2d48f64fe6d5

Comment 270

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #263)
>...
> How likely is this patch to cause regressions; how much testing does it need?
> Very 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.
I didn't realize this was going to be uplifted before it had been on Nightly a couple of days as I stated. I think it will be ok *crosses fingers*

Kamil, I went ahead and used procmon to see all attempts to load dll's from the update directory instead of going with unknown dll's. I suggest going with your existing method where you use proxy dll's as well as using procmon to find any dll load attempts. When doing that you should create a directory named updater.exe.Local alongside the updater.exe to find the SxS dll directory it tries to load on Win 7 and below. Then run it again with procmon and the updater.exe.Local\<sxs directory name>. In my tests it will try to load comctl32.dll from this directory. See comment #257 for more information.

I think in the end you'll end up with
Win XP - x86 and x64
Firefox x86 - wow64log.dll and the updater.exe.Local\<sxs directory name>\comctl32.dll both medium and elevated

Win Vista - x86 and x64
Firefox x86 - apphelp.dll and the updater.exe.Local\<sxs directory name>\comctl32.dll both medium and elevated

Win 7 - x86 and x64
Firefox x86 - updater.exe.Local\<sxs directory name>\comctl32.dll only medium

Win 7 - x64
Firefox x64 - updater.exe.Local\<sxs directory name>\comctl32.dll only medium

Win 8 and Win 8.1 - x86 and x64
Firefox x86 - none

Win 8 and Win 8.1 - x64
Firefox x64 - none

Please also verify that an updater.exe.Local file doesn't try to load sxs dll's from the directory or any other dll's besides wow64log.dll on WinXP and apphelp.dll on Win Vista.

It would be good to check the affect of KB2533623 on Win Vista and Win 7 as well.

Comment 271

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f65c2be0e01

Comment 272

[Ryan VanderMeulen [:RyanVM]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #270)
> I didn't realize this was going to be uplifted before it had been on Nightly
> a couple of days as I stated. I think it will be ok *crosses fingers*

Sorry, I didn't see that amongst all the other comments in this bug. I really wish things wouldn't be approved until they're ready to land. :-\

Comment 273

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #272)
> 
> Sorry, I didn't see that amongst all the other comments in this bug. I
> really wish things wouldn't be approved until they're ready to land. :-\

Yeah but then we can lose track of approval requests... I figure that if no one adds the checkin-needed keyword, things don't just get checked in magically without prompting or work from the dev.

Comment 274

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

Kamil, after testing nightly with a real update I suspect that the updater.exe.Local\<sxs directory name>\comctl32.dll may still be exploitable on all Windows versions. :(

Comment 275

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

Brian, for the case where there is an updater.exe.Local directory alongside the updater.exe what do you think about not displaying the ui and thereby avoid loading the SxS comctl32.dll? It is simple enough and would cover all cases for Win7 and above completely.

Comment 276

[Brian R. Bondy [:bbondy]]

We may have to load whatever APIs that use SxS comctl32.dll dynamically or else it might lead to SxS comctl32.dll loading even if the UI is not shown.  But I think that approach is genius and definitely worth trying out.

Comment 277

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

My initial testing shows that this approach fixes it. I'll file a new bug for this additional patch if further testing is successful.

Kamil, please hold off on verifying this bug until I have finished the testing so you don't have to go duplicate effort.

Comment 278

[Kamil Jozwiak [:kjozwiak]]

> Kamil, please hold off on verifying this bug until I have finished the
> testing so you don't have to go duplicate effort.

Sounds good, just let me know whenever I'm needed :)

Comment 279

[Kamil Jozwiak [:kjozwiak]]

Marking this as verified as testing was completed here and in bug # 1129209. The remaining issues will be fixed in bug # 1127481

Comment 280

[Justin Wood (:Callek)]

:rstrong, :abillings,

This broke MSVC2010 (on win2k3) for SeaMonkey builds, I'm loathe to backout a sec fix to produce our builds, but I don't see a way around it at a glance, this Function is not available in the SDK we have at present.

Note MSVC2010 is still supported on beta/esr31 at present, even though Firefox's official builds use MSVC 2013.

e:/builds/slave/rel-c-beta-w32-bld/build/mozilla/toolkit/mozapps/update/updater/loaddlls.cpp(24) : error C2065: 'SetDefaultDllDirectories' : undeclared identifier
e:/builds/slave/rel-c-beta-w32-bld/build/mozilla/toolkit/mozapps/update/updater/loaddlls.cpp(25) : error C2065: 'SetDefaultDllDirectories' : undeclared identifier
e:/builds/slave/rel-c-beta-w32-bld/build/mozilla/toolkit/mozapps/update/updater/loaddlls.cpp(27) : error C2065: 'LOAD_LIBRARY_SEARCH_SYSTEM32' : undeclared identifier

Any ideas that allows us to proceed?

Comment 281

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

Either try adding an #ifndef for that and then define it or update the build system.

Comment 282

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

Otherwise I'll look at it after the holiday

Comment 283

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

Nothing for me to add here.

Comment 284

[[e-mail]]

Comment on attachment 8556620 [details] [diff] [review]
esr31 combined patch

>+      decltype(SetDefaultDllDirectories)* setDefaultDllDirectories =
>+        (decltype(SetDefaultDllDirectories)*) GetProcAddress(module, "SetDefaultDllDirectories");
FYI process_mitigations.cc uses
typedef BOOL (WINAPI *SetDefaultDllDirectoriesFunction)(DWORD DirectoryFlags);
SetDefaultDllDirectoriesFunction set_default_dll_directories = reinterpret_cast<SetDefaultDllDirectoriesFunction>(::GetProcAddress(module, "SetDefaultDllDirectories"));

Comment 285

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

Attachment: Followup esr31 patch

Followup patch to support SDK's that don't have SetDefaultDllDirectories or LOAD_LIBRARY_SEARCH_SYSTEM32

Comment 286

[Justin Wood (:Callek)]

Comment on attachment 8565217 [details] [diff] [review]
Followup esr31 patch

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

imo we should land on beta as well, since we do support old sdk's there. (Its just not P1) -- either way SeaMonkey will take the reviewed patch on our beta train.

Comment 287

[Justin Wood (:Callek)]

Comment on attachment 8565217 [details] [diff] [review]
Followup esr31 patch

given the beta->release merge is scheduled for tomorrow, I'm requesting approval-beta here. Ahead of the review

Approval Request Comment
[Feature/regressing bug #]: This One
[User impact if declined]: Unable to build with MSVC2010 + older SDK's
[Describe test coverage new/current, TreeHerder]: Minimal, current code uses newer SDKs that won't trip without this code, however Current code trees do exercise these files.
[Risks and why]: Low, just defining features in newer SDKs so older SDKs can build
[String/UUID change made/needed]: None

Comment 288

[Brian R. Bondy [:bbondy]]

Comment on attachment 8565217 [details] [diff] [review]
Followup esr31 patch

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

Great!

Comment 289

[Sylvestre Ledru [:Sylvestre]]

Justin, could you open a new bug for that? To be sure that the sheriff see the uplift request, I would have to change the "status-firefoxXY" flags to affected.
Moreover, it is a different issue by itself.
Thanks

Comment 290

[Justin Wood (:Callek)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #289)
> Justin, could you open a new bug for that? To be sure that the sheriff see
> the uplift request, I would have to change the "status-firefoxXY" flags to
> affected.
> Moreover, it is a different issue by itself.
> Thanks

I'm of the mindset of keeping landings/bugs with the patches that got reviewed.

I don't mind owning the landings if they get approval, especially if before the beta->release uplift.

Comment 291

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

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

Comment 292

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-beta/rev/cce919848572

Comment 293

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

Just to make sure comment 263 was noticed:
Note: since this is not a complete fix 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.

That will happen in bug 1127481