snaury Reporter
	Description • 16 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

	Robert Strong (they/them - no direct email) Assignee
	Comment 1 • 16 years ago

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.

	snaury Reporter
	Comment 2 • 16 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. :-/

	Robert Strong (they/them - no direct email) Assignee
	Comment 3 • 16 years ago

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

	Robert Strong (they/them - no direct email) Assignee
	Comment 4 • 16 years ago

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.

	Robert Strong (they/them - no direct email) Assignee
	Comment 5 • 16 years ago

Jim, could you take a look at this?

	Robert Strong (they/them - no direct email) Assignee
	Comment 6 • 16 years ago

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;

	Jim Mathies [:jimm]
	Comment 7 • 16 years ago

> 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.  

	Robert Strong (they/them - no direct email) Assignee
	Comment 8 • 16 years ago

Ted, could you take the review on this?

	Robert Strong (they/them - no direct email) Assignee
	Comment 9 • 16 years ago

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.

	Robert Strong (they/them - no direct email) Assignee
	Comment 10 • 16 years ago

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

	Masatoshi Kimura [:emk]
	Comment 11 • 16 years ago

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?

	Robert Strong (they/them - no direct email) Assignee
	Comment 12 • 16 years ago

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

	Axel Hecht [:Pike]
	Comment 13 • 16 years ago

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

	Robert Strong (they/them - no direct email) Assignee
	Comment 14 • 16 years ago

What specifically breaks with Win2K Server 2003 SP1 SDK?

	(not currently active) Ted Mielczarek
	Comment 15 • 16 years ago

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.

	Robert Strong (they/them - no direct email) Assignee
	Comment 16 • 16 years ago

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

	Robert Strong (they/them - no direct email) Assignee
	Comment 18 • 16 years ago

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

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 19 • 16 years ago

Is this ready to get nominated for approval1.9.0.1?

	Robert Strong (they/them - no direct email) Assignee
	Comment 20 • 16 years ago

I'm holding off until after tomorrow's update

	Robert Strong (they/them - no direct email) Assignee
	Comment 21 • 16 years ago

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

	Mike Kaply [:mkaply]
	Comment 23 • 16 years ago

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?

	Robert Strong (they/them - no direct email) Assignee
	Comment 24 • 16 years ago

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.

	Masatoshi Kimura [:emk]
	Comment 25 • 16 years ago

(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.

	Robert Strong (they/them - no direct email) Assignee
	Comment 26 • 16 years ago

(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.

	Samuel Sidler (old account; do not CC)
	Comment 27 • 16 years ago

(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.

	Samuel Sidler (old account; do not CC)
	Comment 28 • 16 years ago

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

	Robert Strong (they/them - no direct email) Assignee
	Comment 29 • 16 years ago

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