Re: [PATCH v2 0/2] QGA installer fixes

[Philippe Mathieu-Daudé]

Hi Brian, Konstantin,

On 28/2/23 23:48, Brian Wiltse wrote:
> Microsoft has a list of best practices for MSI creation which covers 
> custom actions 
> https://learn.microsoft.com/en-us/windows/win32/msi/windows-installer-best-practices#if-you-use-custom-actions-follow-good-custom-action-practices <https://learn.microsoft.com/en-us/windows/win32/msi/windows-installer-best-practices#if-you-use-custom-actions-follow-good-custom-action-practices>, The change to the custom action from an interactive command shell to a silent invocation of rundll32.exe keeps the interactive shell from being easily caught and abused, but this does not fully solve the repair from being triggered from a non admin user. There is still the potential for abuse indirectly via attacks like the Mitre documented Hijack Execution Flow technique - Path Interception by PATH Environment Variable (https://attack.mitre.org/techniques/T1574/007/ <https://attack.mitre.org/techniques/T1574/007/>), or even the abuse of potential arbitrary folder creates, file writes and deletes in user-controlled areas such as C:\ProgramData.
>
> The Change button was removed from "Programs and Features", but the 
> cached installer in c:\windows\installer can be leveraged directly to 
> start a privileged repair with msiexec.exe as a non-administrative user. 
> Ideally, the MSI would be compiled with the Privileged property 
> https://learn.microsoft.com/en-us/windows/win32/msi/privileged 
> <https://learn.microsoft.com/en-us/windows/win32/msi/privileged> or 
> AdminUser property 
> https://learn.microsoft.com/en-us/windows/win32/msi/adminuser 
> <https://learn.microsoft.com/en-us/windows/win32/msi/adminuser> or 
> InstallPrivileges="Elevated" 
> https://wixtoolset.org/docs/v3/xsd/wix/package/ 
> <https://wixtoolset.org/docs/v3/xsd/wix/package/> or similar privilege 
> check that which would help ensure the user has proper privileges to 
> perform the repair or change action. However, since the QEMU build 
> process leverages WiXL from msitools, many of the WiX property types are 
> not currently supported to leverage as solutions ( i.e. (wixl:1077): 
> GLib-GObject-WARNING **: 17:49:05.477: g_object_set_is_valid_property: 
> object class 'WixlWixPackage' has no property named 'InstallPrivileges' 
> ). This similar to wixl issue 40 
> https://gitlab.gnome.org/GNOME/msitools/-/issues/40 
> <https://gitlab.gnome.org/GNOME/msitools/-/issues/40>.
>
> I do see that Wixl appears to support the custom action JScriptCall. 
> This might provide for a facility for a script could be run to check if 
> the user has the proper privileges before privileged actions are taken 
> in the repair process, but this is not an ideal solution.

Does that mean this patchset is, although "not ideal", sufficient
to fix CVE-2023-0664? Or does this need more work?
(IOW, do we feel happy enough and want to merge this and forget about it?)

Konstantin, you use "Fixes: CVE-2023-0664" in two different patches.
I'm worried a downstream distrib only pick one and feel safe. Maybe
use something like "Fixes: CVE-2023-0664 (part 1 of 2)".