453185 - nsIProcess run fails with NS_ERROR_FILE_EXECUTION_FAILED when running an exe with a manifest that specifies requestedExecutionLevel level="requireAdministrator" on Vista

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

This is due to nsIProcess using CreateProcess where the app has a manifest with requestedExecutionLevel level="asInvoker" and the system has UAC turned on. nsIProcess should likely use ShellExecuteEx

Comment 1

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

Attachment: simple exe for testing

Comment 2

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

script I've been using for testing... change path for the test exe to where ever you saved the exe. You can also specify an exe that doesn't have a manifest that specifies requestedExecutionLevel level="requireAdministrator" such as notepad to verify that it does work for the other cases

var exePath = "C:\\test.exe";
var process = Components.classes["@mozilla.org/process/util;1"].
                createInstance(Components.interfaces.nsIProcess);
var file = Components.classes["@mozilla.org/file/local;1"].
             createInstance(Components.interfaces.nsILocalFile);
file.initWithPath(exePath);
file.QueryInterface(Components.interfaces.nsIFile);
try {
  process.init(file);
  process.run(true, [], 0);
} catch (e) {
  alert("bleh " + e + "\n\n");
}

Comment 3

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

Attachment: patch rev1 - use ShellExecuteEx

Since the scenario that launches the binary will bring up the UAC dialog on a Vista system there is no way I know of to test this... if there were we would have likely found a major security hole in the UAC implementation. Are there any tests you think should be added that can be implemented?

Comment 4

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

Comment on attachment 336377 [details] [diff] [review]
patch rev1 - use ShellExecuteEx

Jim, could you also take a look at this? Thanks!

Comment 5

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

Comment on attachment 336377 [details] [diff] [review]
patch rev1 - use ShellExecuteEx

>diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp
>--- a/xpcom/threads/nsProcessCommon.cpp
>+++ b/xpcom/threads/nsProcessCommon.cpp
>...
>+    // Sets hProcess to the process handle.
>+    if (blocking)
>+        sinfo.fMask += SEE_MASK_NOCLOSEPROCESS;
should be |=

Comment 6

[Jim Mathies [:jimm]]

Looks good to me, pretty heavy change though to a key chunk of code. I assume this will be baking for a while?

Comment 7

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

(In reply to comment #6)
> Looks good to me, pretty heavy change though to a key chunk of code. I assume
> this will be baking for a while?
Most likely

Comment 8

[Benjamin Smedberg]

Comment on attachment 336377 [details] [diff] [review]
patch rev1 - use ShellExecuteEx

This looks good, though I desperately want some tests...

Comment 9

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

(In reply to comment #8)
> (From update of attachment 336377 [details] [diff] [review])
> This looks good, though I desperately want some tests...
We can't test the UAC from what I've been able to find. For what it is worth there is a test I landed yesterday (bug 451085) that uses nsIProcess so it at lease gets tested elsewhere for the general case where elevation isn't required.

Comment 10

[Dave Townsend [:mossop]]

Cleaning this patch up to work with the current state of nsIProcess...

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This patch applies on top of the patch in bug 442393.

This is mostly the same stuff but has had to undergo some rearrangement to cope with changes introduced by other patches. It switches to using ShellExecuteEx to launch the process and just keeps a hold of the process HANDLE that comes back. This allows us to kill etc. the process when run non-blocking. It also adds support for getting the exit value of a process that was run in a non-blocking manner. This only works on windows though, I can't see a way to make it work elsewhere.

There is one problem, ShellExecuteEx doesn't give us the PID of the process. The API call to get a PID from a process HANDLE is only available on XP SP1 and later. I've implemented it such that it works there and later. If we wanted getting the PID to work everywhere then we might be able to cope by trying CreateProcess first then using ShellExecuteEx if that doesn't work. However I don't think being able to get the PID is such a critical need that we should complicate things like that for now. Getting the PID was only implemented a short time ago in bug 442393 so we aren't really regressing anything in a release.

As Rob says there doesn't seem to be a way that we can test the UAC response to this automatically. We do now have a few different tests of nsIProcess that this all passes which I'm hoping will be enough to allow checking this in this time. I have verified that the UAC does appear using the testcase in this bug report on Windows 7, both running blocking and non-blocking, and in both cases we get an appropriate exitValue (0 or 2 depending on if you click yes or no in the dialog). For both blocking and non-blocking we still throw NS_ERROR_FILE_EXECUTION_FAILED out of the run call if the user denies the UAC request.

I don't have access to a Vista box to verify it works for sure there, Rob maybe you might be able to give it a spin for me?

Comment 12

[Benjamin Smedberg]

Comment on attachment 367797 [details] [diff] [review]
patch rev 2

>diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp

>+    HMODULE kernelDLL = ::LoadLibraryW(L"Kernel32.dll");

I slightly prefer all-lowercase here.

Comment 13

[Dave Townsend [:mossop]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/1f3d5e332a44

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: wince bustage fix

This push busted windows ce, this patch gets us building

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/8fc57e8196a5

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 367797 [details] [diff] [review]
patch rev 2

We need to take this on 1.9.1 if we want to be able to use nsIProcess to run installers that require elevation on Vista. The existing nsIProcess tests verify that it does not break anything, but we are unable to use automated testing to show that elevation works, however that has be tested manually.

Comment 17

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

Comment on attachment 367797 [details] [diff] [review]
patch rev 2

a191=beltzner for this and the WINCE fix

Comment 18

[Dave Townsend [:mossop]]

Landed with the bustage fix on branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/726d6e0d61de