4592-RestartManager-AddPriviledges #189

Merged
merged 3 commits into from Jan 3, 2015

Projects

None yet

2 participants

@phillHgl
Contributor
phillHgl commented Dec 9, 2014

If the named process is running under another user, then OpenProcess must be called using a process token which includes SeDebugPriviledge. To set SeDebugPriviledge the caller must have elevated privileges. If cannot set SeDebugPriviledge, log a message and continue. Let caller handle a reboot request rather than returning a failure.

Tested these changes on Win 7 SP1 x64 Prof, Win 8 Prof x64, Win 7 SP1 Ultimate N German x64, Win 7 SP1 Ultimate N Korean x86, WS2008R2 SP1 Standard, WS2012R2 Korean.

@phillHgl phillHgl 4592-RestartManager-AddPriviledges
If the named process is running under another user, then OpenProcess must be called using a process token which includes SeDebugPriviledge.  To set SeDebugPriviledge the caller must have elevated privledges.  If cannot set SeDebugPriviledge, log a message and continue.  Let caller handle a reboot request rather than returning a failure.
f743a7e
@phillHgl
Contributor
phillHgl commented Dec 9, 2014

Tested these changes on Win 7 SP1 x64 Prof, Win 8 Prof x64, Win 7 SP1 Ultimate N German x64, Win 7 SP1 Ultimate N Korean x86, WS2008R2 SP1 Standard, WS2012R2 Korean.

@barnson barnson and 1 other commented on an outdated diff Dec 9, 2014
src/ext/ca/wixca/dll/RestartManager.cpp
@@ -148,8 +149,17 @@ extern "C" UINT __stdcall WixRegisterRestartResources(
case etApplication:
WcaLog(LOGMSG_VERBOSE, "Registering process name %ls with the Restart Manager.", wzResource);
hr = RmuAddProcessesByName(pSession, wzResource);
- ExitOnFailure(hr, "Failed to register the process name with the Restart Manager session.");
- break;
+ if (E_NOTFOUND == hr)
@barnson
barnson Dec 9, 2014 Member

Tabs are inherently evil. :) Rule 1 at http://wixtoolset.org/development/code-style/.

@phillHgl
phillHgl Dec 10, 2014 Contributor

Thanks. I found vs option to insert spaces..

@barnson barnson commented on an outdated diff Dec 9, 2014
src/ext/ca/wixca/dll/RestartManager.cpp
@@ -148,8 +149,17 @@ extern "C" UINT __stdcall WixRegisterRestartResources(
case etApplication:
WcaLog(LOGMSG_VERBOSE, "Registering process name %ls with the Restart Manager.", wzResource);
hr = RmuAddProcessesByName(pSession, wzResource);
- ExitOnFailure(hr, "Failed to register the process name with the Restart Manager session.");
- break;
+ if (E_NOTFOUND == hr)
+ {
+ //At least one instance of this process, running under another user returned access denied. Since other instances may have been registered, continue this setup.
@barnson
barnson Dec 9, 2014 Member

Space after comment marker.

@barnson barnson and 1 other commented on an outdated diff Dec 9, 2014
src/libs/dutil/rmutil.cpp
-
- ::EnterCriticalSection(&pSession->cs);
- fLocked = TRUE;
-
- hr = RmuApplicationArrayAlloc(&pSession->rgApplications, &pSession->cApplications, dwProcessId, CreationTime);
- ExitOnFailure(hr, "Failed to add the application to the array.");
+ HANDLE hToken = NULL;
+ TOKEN_PRIVILEGES priv = { 0 };
+ TOKEN_PRIVILEGES* pPrevPriv = NULL;
+ DWORD cbPrevPriv = 0;
+
+ BOOL fElevated = FALSE;
+ ProcElevated(::GetCurrentProcess(), &fElevated);
+
+ // Must be elevated to adjust process privileges
+ if (TRUE == fElevated) {
@barnson
barnson Dec 9, 2014 Member

BOOLs and pointers don't use comparison operators -- if (fBar) or if (!pFoo). Multiple instances.

@phillHgl
phillHgl Dec 10, 2014 Contributor

I had (fElevated) originally but changed it to use == after searching wix src to see if I could determine what was the prefered style. Happy to change back.

@barnson barnson commented on an outdated diff Dec 9, 2014
src/libs/dutil/rmutil.cpp
+ // Calling process needs SeDebugPrivilege if the process to be opened running under a different user account.
+ hProcess = ::OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, dwProcessId);
+ if (TRUE == fElevated)
+ {
+ ExitOnNullWithLastError(hProcess, hr, "Failed to open the process ID %d.", dwProcessId);
+ if (!::GetProcessTimes(hProcess, &CreationTime, &ExitTime, &KernelTime, &UserTime))
+ {
+ ExitWithLastError(hr, "Failed to get the process times for process ID %d.", dwProcessId);
+ }
+
+ ::EnterCriticalSection(&pSession->cs);
+ fLocked = TRUE;
+
+ hr = RmuApplicationArrayAlloc(&pSession->rgApplications, &pSession->cApplications, dwProcessId, CreationTime);
+ ExitOnFailure(hr, "Failed to add the application to the array.");
+
@barnson
barnson Dec 9, 2014 Member

Extra whitespace.

@barnson barnson commented on an outdated diff Dec 9, 2014
src/libs/dutil/rmutil.cpp
+ {
+ ExitWithLastError(hr, "Failed to get process token.");
+ }
+
+ priv.PrivilegeCount = 1;
+ priv.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;
+ if (!::LookupPrivilegeValueW(NULL, L"SeDebugPrivilege", &priv.Privileges[0].Luid))
+ {
+ ExitWithLastError(hr, "Failed to get debug privilege LUID.");
+ }
+
+ cbPrevPriv = sizeof(TOKEN_PRIVILEGES);
+ pPrevPriv = static_cast<TOKEN_PRIVILEGES*>(MemAlloc(cbPrevPriv, TRUE));
+ ExitOnNull(pPrevPriv, hr, E_OUTOFMEMORY, "Failed to allocate memory for empty previous privileges.");
+
+ if (!::AdjustTokenPrivileges(hToken, FALSE, &priv, cbPrevPriv, pPrevPriv, &cbPrevPriv))
@barnson
barnson Dec 9, 2014 Member

We need to restore the original privileges. MSI reuses custom action host processes so you'd be leaking the debug privilege.

@barnson
Member
barnson commented Dec 9, 2014

Please add message to History.md. Check for "privilege" typos. :)

phillHgl added some commits Dec 12, 2014
@phillHgl phillHgl 4592-RestartManager
Implemented pull request feedback.  Try to add SeDebugPrivilege, if
successful remove the privilege when the function returns.  If
OpenProcess() returns AccessDenied, log a message and continue the
install, rather than a fatal error.
a8bdfd9
@phillHgl phillHgl 4592-RestartManager-history
Added comment to history.md
d88039c
@barnson barnson merged commit d88039c into wixtoolset:develop Jan 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment