Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inno setup incompatible with winget when using PrivilegesRequired #254

Open
dmex opened this issue May 20, 2020 · 6 comments
Open

Inno setup incompatible with winget when using PrivilegesRequired #254

dmex opened this issue May 20, 2020 · 6 comments
Labels
Issue-Feature This is a feature request for the Windows Package Manager client.

Comments

@dmex
Copy link

dmex commented May 20, 2020

winget does not properly support Inno setup installers due the LowIL of the binary before they're executed.

Inno setup installers use the PrivilegesRequired directive for requesting UAC elevation which is implemented internally by Inno Setup with ShellExecuteEx and the RunAs verb:
https://jrsoftware.org/ishelp/index.php?topic=setup_privilegesrequired

AppInstallerCLI downloads binaries into the WinGet directory and it's inheriting both directory and file permissions from the %localappdata%\app_container_name\TempState\ directory which includes the Low integrity level RID:
https://i.imgur.com/MBTzvkr.png

ShellExecuteEx doesn't allow processes to use the RunAs verb for binaries with LowIL and this prevents winget from properly executing Inno setup installers.

The InstallCommand::ExecuteInternal function needs to include an extra step which removes the RID for the downloaded binary so that ShellExecute will correctly use the integrity level of the calling process token instead of the integrity level of the file.

VerifyInstallerHash
RemoveFileIL <-- winget needs this before calling ShellExecute
ExecuteInstaller

For this code:

Workflow::DownloadInstaller <<
Workflow::VerifyInstallerHash <<
Workflow::ExecuteInstaller <<
Workflow::RemoveInstaller;

Alternatively winget should use a directory which doesn't inherit LowIL permissions. (Edit: See second github post below for more information)

You can also reproduce the incorrect behaviour by navigating to the \TempState\ directory and manually executing the Inno setup installers via Windows Explorer which also fails due to winget not removing the LowIL from the file:
image

Expected behavior

winget install vlc - successfully shows elevation prompt

Actual behavior

winget install processhacker - elevation prompt failure (see pull request 373 on winget-pkgs repro)

Environment

Windows: Windows.Desktop v10.0.19041.264
Package: Microsoft.DesktopAppInstaller v1.0.41331.0

@dmex
Copy link
Author

dmex commented May 20, 2020

Related to #189

@KevinLaMS KevinLaMS added the Issue-Bug It either shouldn't be doing this or needs an investigation. label May 21, 2020
@ghost ghost added the Needs-Tag-Fix label May 21, 2020
@KevinLaMS KevinLaMS added this to the Package Manager v0.2 milestone May 21, 2020
@dmex
Copy link
Author

dmex commented May 21, 2020

Alternatively winget should use a directory which doesn't inherit LowIL permissions.

Expanding on this point. Winget already supports using an alternate cache directory with already has the correct permissions but the current checks are incorrect since the winget store package has the runFullTrust capability.

AppInstaller::Runtime::GetPathToTemp() function checks if the process has 'identity' by calling IsRunningInPackagedContext() here:

if (IsRunningInPackagedContext())
{
result.assign(winrt::Windows::Storage::ApplicationData::Current().TemporaryFolder().Path().c_str());
}
else
{
wchar_t tempPath[MAX_PATH + 1];
DWORD tempChars = GetTempPathW(ARRAYSIZE(tempPath), tempPath);
result.assign(std::wstring_view{ tempPath, static_cast<size_t>(tempChars) });
}

The function queries the current process package using GetPackageFamilyName which internally uses the "WIN://SYSAPPID" token attribute:

UINT32 length = 0;
LONG result = GetPackageFamilyName(GetCurrentProcess(), &length, nullptr);
return (result != APPMODEL_ERROR_NO_PACKAGE);

winget does not execute with AppContainer restrictions since its using the runFullTrust capability and should instead be using GetTempPathW since it doesn't inherit LowIL permissions.

The AppInstaller::Runtime::GetPathToTemp() function should be calling GetTokenInformation(GetCurrentProcessToken(), TokenIsAppContainer) and use GetTempPathW when TokenIsAppContainer==0

@denelon denelon modified the milestones: Package Manager v0.2.x, Package Manager v0.3.x Aug 21, 2020
@wjk
Copy link

wjk commented Jan 10, 2021

I’m seeing this with my Burn-based installer too. It will only install if I run winget from an elevated command prompt. Otherwise, it dies with the HRESULT for Access Denied. A solution would be appreciated. Thanks!

@doctordns
Copy link

I’m seeing this with my Burn-based installer too. It will only install if I run winget from an elevated command prompt. Otherwise, it dies with the HRESULT for Access Denied. A solution would be appreciated. Thanks!

Isn't the solution just to use an elevated powershel/cmd prompt assuming you are allowed? In my experience, installers need elevated permissions to write to protected registry areas or protected folders. Winget should not be driving a coach and horses through enterprise security.

@wjk
Copy link

wjk commented Jan 11, 2021

@doctordns I’ve seen winget work from from a normal command prompt many times; the installer being run calls for elevation itself. I don’t know why some packages can elevate themselves, and others require the caller to be elevated.

@denelon
Copy link
Contributor

denelon commented Apr 20, 2021

There are several different features related to required installer privileges and UAC prompts:
#218
#271
#281

We will need to look at adding this as a feature for Inno installers (and possibly other installer types). So the client can be properly informed about the required permissions, and the user can be informed as well.

@denelon denelon added Issue-Feature This is a feature request for the Windows Package Manager client. Area-Manifest This may require a change to the manifest and removed Issue-Bug It either shouldn't be doing this or needs an investigation. labels Apr 20, 2021
@denelon denelon modified the milestones: Package Manager v0.3.x, Package Manager Backlog May 3, 2021
@denelon denelon removed the Area-Manifest This may require a change to the manifest label Jun 23, 2021
@denelon denelon removed this from the Backlog-Client milestone Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature This is a feature request for the Windows Package Manager client.
Projects
None yet
Development

No branches or pull requests

6 participants