437349 - updater.exe lacks elevation manifest and fails to start when installer detection is disabled

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[snaury]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

When installer detection is disabled (HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Policies\System\EnableInstallerDetection is set to 0), updater.exe is no longer detected as an installer and is not automatically elevated. This is because updater.exe lacks trustInfo in the manifest resource. The relevant part of updater.exe's manifest should read as follows:

<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
  <security>
    <requestedPrivileges>
      <requestedExecutionLevel level="asAdministrator"  />
    </requestedPrivileges>
  </security>
</trustInfo>

Workaround: user can manually set "Run as administrator" in the Compatibility sheet in updater.exe properties.

P.S. I'm not sure how many people actually disable installer detection, but I can imagine almost anyone who uses cygwin has to either disable UAC completely, or disable installer detection in particular. Installer detection is extremely dumb and prevents execution of standard install.exe utility under cygwin.

Reproducible: Always

Comment 1

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

What directory did you install into? The app will request elevation when launching updater.exe if it is installed into Program Files on Vista. We don't want to require run as admin for the case where it is installed into a location that doesn't require elevation for users that don't have admin rights, etc.

Comment 2

[snaury]

I have it installed in Program Files. I think that "Program Files" thing could also be part of the installer detection mechanism, which I have turned off. When it offered me an update to rc2 it didn't show any elevation prompts, it just failed.

However. I just tried to turn installer detection back on, rebooted and made some experiments. A very simple c program:

    int main(int argc, char** argv) { return 1; }

When compiled as instal.exe, setup.exe, or update.exe (you can add any letters before or after those names, UAC just searches for a substring) and executed from my *home* folder, it asks to elevate. So your point about firefox being installed into location not requiring to elevate doesn't seem to work. In my experiments it tries to elevate anyway, just because its name contains some very unlucky sequence of characters.

If you worry about non-administrator users, then maybe it should be highestAvailable instead of requireAdministrator. Then if user doesn't have administrator privileges it won't try to elevate, but I'm not sure.

Again, I personally consider installer detection implemented by Microsoft in Vista (which is based on *substring* of the filename!) to be extremely wrong, and it is a feature that in my opinion potentially breaks more applications than fixes. There have always been administrator-aware installers that worked fine without administrator privileges, with installer detection turned on there's just no simple way you can run it *without* administrator privileges (ok, of course sometimes you can rename it, other times you can manually unpack it and rename setup.exe that's inside, but that's not the point). And of course it breaks cygwin.

Relying on heuristics is a bad idea anyway, these heuristics are supposed to help "legacy" applications that just don't support Vista, and these heuristics can suddenly change without notice. Especially when it's not even easy to find a detailed description on what exactly those heuristics detect. :-/

Comment 3

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

note: it appears that WinLaunchChild is not using shellexecute as it should be and is falling back to createprocess which then relies on heuristics.

Comment 4

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

Attachment: Patch - in progress

Comment #3 is not correct please ignore.

We do need to add requestedExecutionLevel asInvoker to the existing updater.exe manifest. To get around the issues with launching the updater.exe elevated such as bug 386826 which causes environment vars to not be retained. This patch is also the first step in allowing an option to specify credentials to update with an administrator account when using a non admin account.

Still need to do some cleanup of the patch so holding off on reviews.

Comment 5

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

Attachment: Patch rev1

Jim, could you take a look at this?

Comment 6

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

Comment on attachment 325665 [details] [diff] [review]
Patch rev1

>Index: toolkit/mozapps/update/src/updater/updater.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/update/src/updater/updater.cpp,v
>retrieving revision 1.36
>diff -u -8 -r1.36 updater.cpp
>--- toolkit/mozapps/update/src/updater/updater.cpp	12 Mar 2008 11:13:10 -0000	1.36
>+++ toolkit/mozapps/update/src/updater/updater.cpp	
...

Please ignore the following... I use this to make testing the patch easier and it won't be included in the final patch.
>   rv = list.Prepare();
>+  if (rv == UNEXPECTED_ERROR)
>+    return OK;
>   if (rv)
>     return rv;

Comment 7

[Jim Mathies [:jimm]]

> if (result || ((int)sinfo.hInstApp) > 32) {

I'd probably not check the hInstApp value, if result is false I'd consider that an absolute. Just my 2 cents.  

Comment 8

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

Attachment: patch rev2

Ted, could you take the review on this?

Comment 9

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

Litmus test 5086 essentially tests this already though it might be good to have a general test that the UAC prompt is displayed when installed into program files.

Comment 10

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

Checked in to mozilla-central

changeset:   15484:9737aa363037
tag:         tip
user:        Robert Strong <[e-mail]>
date:        Mon Jun 23 12:06:37 2008 -0700
summary:     Bug 437349 - updater.exe lacks elevation manifest and fails to start when installer detection is disabled r=jmathies r=ted.mielczarek

Comment 11

[Masatoshi Kimura [:emk]]

This fix will cause bug 367540 again.
> -      rv = LaunchChild(nativeApp, appInitiatedRestart, upgraded ? -1 : 0);
> +      rv = LaunchChild(nativeApp, appInitiatedRestart);
should not be applied. Am I wrong?

Comment 12

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

The reliance on that code path was fixed by Bug 390366 which was landed as part of Bug 370571

Comment 13

[Axel Hecht [:Pike]]

This breaks building with Windows Server 2003 SP1 SDK and --disable-vista-sdk-requirements, and ted said:

<ted>	hm, yeah, it's not in the SDK that ships with VC8 Pro either
<ted>	but it's in the vista sdk
<ted>	Pike: wanna comment on the bug and let him know?
<ted>	should be easy enough to #ifndef / #define

Comment 14

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

What specifically breaks with Win2K Server 2003 SP1 SDK?

Comment 15

[(not currently active) Ted Mielczarek]

SEE_MASK_NOASYNC is new, and not in the Win2k3 SDK. It replaces SEE_MASK_FLAG_DDEWAIT, which has the exact same value but is now considered deprecated apparently. You could easily work around that with an #ifndef.

Comment 16

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

damn, I meant to change that to SEE_MASK_FLAG_DDEWAIT and change it when we change SEE_MASK_FLAG_DDEWAIT in the tree... fix coming up

Comment 17

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

Attachment: fix for the Win2K Server 2003 SP1 SDK

Comment 18

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

Checked in followup patch attachment #326511 [details] [diff] [review] to mozilla-central

changeset:   15501:bb0f90b6b18e
tag:         tip
user:        Robert Strong <[e-mail]>
date:        Tue Jun 24 10:45:24 2008 -0700
summary:     Follow up for Bug 395891 to fix compiling with the Win2K Server 2003 SP1 SDK

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this ready to get nominated for approval1.9.0.1?

Comment 20

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

I'm holding off until after tomorrow's update

Comment 21

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

Attachment: patch - includes fix for the Win2K Server 2003 SP1 SDK

carrying forward Ted's and Jim's reviews

Litmus test 5086 essentially covers this. Since this displays the UAC dialog which prevents UI automation there is no way to automate a test for this.

Requesting 1.9.0.1

Comment 23

[Mike Kaply [:mkaply]]

Why is this marked approval 1.9.0.2? did it already miss the 1.9.0.1 boat? is there a 1.9.0.1 boat?

Comment 24

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

Drivers have been triaging for 1.9.0.1 for a quick turn around release and I suspect they wanted to mitigate risk by not taking this for 1.9.0.1... beltzner should be able to provide an authoritative answer.

Comment 25

[Masatoshi Kimura [:emk]]

(In reply to comment #12)
> The reliance on that code path was fixed by Bug 390366 which was landed as part
> of Bug 370571
Unfortunately the UAC NSIS plugin isn't helpful for "Run as Administrator" from Standard User scenario.

Comment 26

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

(In reply to comment #25)
> (In reply to comment #12)
> > The reliance on that code path was fixed by Bug 390366 which was landed as part
> > of Bug 370571
> Unfortunately the UAC NSIS plugin isn't helpful for "Run as Administrator" from
> Standard User scenario.
Of course it isn't and we shouldn't be overriding a user choice to explicitly "run as administrator" or "run as" whether it is from the installer or from the application itself.

Comment 27

[Samuel Sidler (old account; do not CC)]

(In reply to comment #9)
> Litmus test 5086 essentially tests this already though it might be good to have
> a general test that the UAC prompt is displayed when installed into program
> files.

It'd be good to have a general Litmus test added as mentioned here, but that won't gate approval since the current test mostly covers it.

Comment 28

[Samuel Sidler (old account; do not CC)]

Comment on attachment 326757 [details] [diff] [review]
patch - includes fix for the Win2K Server 2003 SP1 SDK

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 29

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

Checked in for 1.9.0.2 / Firefox 3.0.2

Checking in mozilla/toolkit/mozapps/update/src/updater/updater.exe.manifest;
/cvsroot/mozilla/toolkit/mozapps/update/src/updater/updater.exe.manifest,v  <--
 updater.exe.manifest
new revision: 1.4; previous revision: 1.3
done
Checking in mozilla/toolkit/mozapps/update/src/updater/updater.cpp;
/cvsroot/mozilla/toolkit/mozapps/update/src/updater/updater.cpp,v  <--  updater.
cpp
new revision: 1.37; previous revision: 1.36
done
Checking in mozilla/toolkit/xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.212; previous revision: 1.211
done
Checking in mozilla/toolkit/xre/nsUpdateDriver.cpp;
/cvsroot/mozilla/toolkit/xre/nsUpdateDriver.cpp,v  <--  nsUpdateDriver.cpp
new revision: 1.26; previous revision: 1.25
done