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

RESOLVED FIXED in mozilla1.9.1a1

Status

()

defect
--
major
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: snaury, Assigned: rstrong)

Tracking

(Blocks 1 bug, {fixed1.9.0.2, fixed1.9.1})

1.9.0 Branch
mozilla1.9.1a1
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.1 -
wanted1.9.0.x +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

11 years ago
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
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.
Reporter

Comment 2

11 years ago
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. :-/
note: it appears that WinLaunchChild is not using shellexecute as it should be and is falling back to createprocess which then relies on heuristics.
Posted patch Patch - in progress (obsolete) — Splinter Review
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.
Assignee: nobody → robert.bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: blocking-firefox3.1?
Posted patch Patch rev1 (obsolete) — Splinter Review
Jim, could you take a look at this?
Attachment #325064 - Attachment is obsolete: true
Attachment #325665 - Flags: review?(jmathies)
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;
> 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.  

Updated

11 years ago
Attachment #325665 - Flags: review?(jmathies) → review+
Posted patch patch rev2Splinter Review
Ted, could you take the review on this?
Attachment #325665 - Attachment is obsolete: true
Attachment #325826 - Flags: review?(ted.mielczarek)
Attachment #325826 - Flags: review?(ted.mielczarek) → review+
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.
Flags: in-litmus?
Checked in to mozilla-central

changeset:   15484:9737aa363037
tag:         tip
user:        Robert Strong <robert.bugzilla@gmail.com>
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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?
The reliance on that code path was fixed by Bug 390366 which was landed as part of Bug 370571
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
What specifically breaks with Win2K Server 2003 SP1 SDK?

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.
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
Checked in followup patch attachment #326511 [details] [diff] [review] to mozilla-central

changeset:   15501:bb0f90b6b18e
tag:         tip
user:        Robert Strong <robert.bugzilla@gmail.com>
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
Is this ready to get nominated for approval1.9.0.1?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1+
I'm holding off until after tomorrow's update
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
Attachment #326757 - Flags: review+
Attachment #326757 - Flags: approval1.9.0.1?

Updated

11 years ago
Duplicate of this bug: 386826
Attachment #326757 - Flags: approval1.9.0.1? → approval1.9.0.2?
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?
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.
Depends on: 404541
(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.
(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.
(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 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
Attachment #326757 - Flags: approval1.9.0.2? → approval1.9.0.2+
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
Keywords: fixed1.9.0.2
Depends on: 448351
Product: Firefox → Toolkit
Target Milestone: --- → mozilla1.9.1a1
Version: unspecified → 1.9.0 Branch
You need to log in before you can comment on or make changes to this bug.