752056 - Maintainence service executes updater.exe from a directory that doesn't (by default) require admin privileges to write to

Status: RESOLVED WORKSFORME

(Toolkit :: Application Update, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #750850 +++

See bug 750850 and bug 748948. Both of those bugs have other mitigations, and in some cases those mitigations may even be more effective than executing updater.exe out of a protected directory. But, it is also true that both bugs would have been prevented in the vast majority of cases by requiring updater.exe to be in an administrator-only-write-access directory before executing it. IMO, that is good enough evidence to convince us that we should make this change, if only to protect ourselves from bugs we don't even know about yet. 

Thanks, James, for the awesome bug reports!

Comment 1

[Brian R. Bondy [:bbondy]]

I think this is a good thing but a couple notes:

1) The report in Bug 750850 is specifically about the service but it also applies to updater.exe without the service.  This is probably partially what you were referring to with "those mitigations may even be more effective".

2) Future security bug reports that are yet to be discovered for the cases this bug will fix, will likely affect both the service and updater.exe.  Putting updater.exe in a secured location before executing it is only possible with the service.  So those security bugs will still be present in updater.exe.

Comment 3

[Brian R. Bondy [:bbondy]]

James Forshaw also made a good point that this bug is important in case AV software inject their DLL into a process.  

> James wrote on 2012-05-17 07:50:04 PDT
>
> Well take for example Sophos AV, which forces sophos_detoured.dll into all
> processes (even running as admin/system). Now it loads it by an absolute path so
> the DLL itself isn't a problem, but it has a dependency on psapi.dll which is
> not a known DLL. This means that any process (well that loads user32.dll)
> started under any account will always try and first load psapi.dll from the
> application directory (at least on XP, haven't double checked on Win7 where it
> does some things to protect against bad InitDLLs). Therefore when you load
> updater.exe on a system with sophos you could get priv escalation using a dummy
> psapi.dll in with updater.exe even though you don't ever directly load it.
> 
> Of course this is their problem rather than anyone elses, but normally it isn't
> an issue because most system/admin processes run from secure locations.
> 
> My point was not that it happens (it will) it was that pre-loading the DLLs you
> know you are using is not enough to cover all bases, as you do not know the
> dependencies of anything loading implicitly or explicitly into your process.
> Therefore I considered it a very brittle way of fixing the issue, however I do
> not believe this needs a separate bug open, as the way to fix it is to not run
> updater.exe from the untrusted location.

Comment 4

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

We now launch the updater when using the maintenance service from
C:\Program Files (x86)\Mozilla Maintenance Service\update\

We now launch the updater when NOT using the maintenance service from
<path to install dir>\