[PATCH v2 0/2] QGA installer fixes

[PATCH v2 0/2] QGA installer fixes

[Konstantin Kostiuk]

resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2167423
fixes: CVE-2023-0664

CVE Technical details: The cached installer for QEMU Guest Agent in c:\windows\installer
(https://github.com/qemu/qemu/blob/master/qga/installer/qemu-ga.wxs),
can be leveraged to begin a repair of the installation without validation
that the repair is being performed by an administrative user. The MSI repair
custom action "RegisterCom" and "UnregisterCom" is not set for impersonation
which allows for the actions to occur as the SYSTEM account
(LINE 137 AND 145 of qemu-ga.wxs). The custom action also leverages cmd.exe
to run qemu-ga.exe in line 134 and 142 which causes an interactive command
shell to spawn even though the MSI is set to be non-interactive on line 53.

v1: https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg05661.html
v1 -> v2:
  Add explanation into commit messages

Konstantin Kostiuk (2):
  qga/win32: Remove change action from MSI installer
  qga/win32: Use rundll for VSS installation

 qga/installer/qemu-ga.wxs | 11 ++++++-----
 qga/vss-win32/install.cpp |  9 +++++++++
 qga/vss-win32/qga-vss.def |  2 ++
 3 files changed, 17 insertions(+), 5 deletions(-)

--
2.25.1

[PATCH v2 1/2] qga/win32: Remove change action from MSI installer

[Konstantin Kostiuk]

Remove the 'change' button from "Programs and Features" because it does
not checks if a user is an admin or not. The installer has no components
to choose from and always installs everything. So the 'change' button is
not obviously needed but can create a security issue.

resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2167423
fixes: CVE-2023-0664

Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
Reviewed-by: Yan Vugenfirer <yvugenfi@redhat.com>
---
 qga/installer/qemu-ga.wxs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 51340f7ecc..feb629ec47 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -31,6 +31,7 @@
       />
     <Media Id="1" Cabinet="qemu_ga.$(var.QEMU_GA_VERSION).cab" EmbedCab="yes" />
     <Property Id="WHSLogo">1</Property>
+    <Property Id="ARPNOMODIFY" Value="yes" Secure="yes" />
     <MajorUpgrade
       DowngradeErrorMessage="Error: A newer version of QEMU guest agent is already installed."
       />
--
2.25.1

[PATCH v2 2/2] qga/win32: Use rundll for VSS installation

[Konstantin Kostiuk]

The custom action uses cmd.exe to run VSS Service installation
and removal which causes an interactive command shell to spawn.
This shell can be used to execute any commands as a SYSTEM user.
Even if call qemu-ga.exe directly the interactive command shell
will be spawned as qemu-ga.exe is a console application and used
by users from the console as well as a service.

As VSS Service runs from DLL which contains the installer and
uninstaller code, it can be run directly by rundll32.exe without
any interactive command shell.

Add specific entry points for rundll which is just a wrapper
for COMRegister/COMUnregister functions with proper arguments.

resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2167423
fixes: CVE-2023-0664

Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
Reviewed-by: Yan Vugenfirer <yvugenfi@redhat.com>
---
 qga/installer/qemu-ga.wxs | 10 +++++-----
 qga/vss-win32/install.cpp |  9 +++++++++
 qga/vss-win32/qga-vss.def |  2 ++
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index feb629ec47..46ae9e7a13 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -127,22 +127,22 @@
       </Directory>
     </Directory>

-    <Property Id="cmd" Value="cmd.exe"/>
+    <Property Id="rundll" Value="rundll32.exe"/>
     <Property Id="REINSTALLMODE" Value="amus"/>

     <?ifdef var.InstallVss?>
     <CustomAction Id="RegisterCom"
-              ExeCommand='/c "[qemu_ga_directory]qemu-ga.exe" -s vss-install'
+              ExeCommand='"[qemu_ga_directory]qga-vss.dll",DLLCOMRegister'
               Execute="deferred"
-              Property="cmd"
+              Property="rundll"
               Impersonate="no"
               Return="check"
               >
     </CustomAction>
     <CustomAction Id="UnRegisterCom"
-              ExeCommand='/c "[qemu_ga_directory]qemu-ga.exe" -s vss-uninstall'
+              ExeCommand='"[qemu_ga_directory]qga-vss.dll",DLLCOMUnregister'
               Execute="deferred"
-              Property="cmd"
+              Property="rundll"
               Impersonate="no"
               Return="check"
               >
diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index b57508fbe0..68662a6dfc 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -357,6 +357,15 @@ out:
     return hr;
 }

+STDAPI_(void) CALLBACK DLLCOMRegister(HWND, HINSTANCE, LPSTR, int)
+{
+    COMRegister();
+}
+
+STDAPI_(void) CALLBACK DLLCOMUnregister(HWND, HINSTANCE, LPSTR, int)
+{
+    COMUnregister();
+}

 static BOOL CreateRegistryKey(LPCTSTR key, LPCTSTR value, LPCTSTR data)
 {
diff --git a/qga/vss-win32/qga-vss.def b/qga/vss-win32/qga-vss.def
index 927782c31b..ee97a81427 100644
--- a/qga/vss-win32/qga-vss.def
+++ b/qga/vss-win32/qga-vss.def
@@ -1,6 +1,8 @@
 LIBRARY      "QGA-PROVIDER.DLL"

 EXPORTS
+	DLLCOMRegister
+	DLLCOMUnregister
 	COMRegister		PRIVATE
 	COMUnregister		PRIVATE
 	DllCanUnloadNow		PRIVATE
--
2.25.1

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

[Philippe Mathieu-Daudé]

On 21/2/23 12:21, Konstantin Kostiuk wrote:
> resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2167423
> fixes: CVE-2023-0664
> 
> CVE Technical details: The cached installer for QEMU Guest Agent in c:\windows\installer
> (https://github.com/qemu/qemu/blob/master/qga/installer/qemu-ga.wxs),
> can be leveraged to begin a repair of the installation without validation
> that the repair is being performed by an administrative user. The MSI repair
> custom action "RegisterCom" and "UnregisterCom" is not set for impersonation
> which allows for the actions to occur as the SYSTEM account
> (LINE 137 AND 145 of qemu-ga.wxs). The custom action also leverages cmd.exe
> to run qemu-ga.exe in line 134 and 142 which causes an interactive command
> shell to spawn even though the MSI is set to be non-interactive on line 53.
> 
> v1: https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg05661.html

Per 
https://lore.kernel.org/qemu-devel/CAA8xKjUQFBVgDVJ059FvGoSjkv+kZ5jB1gfMNz+ao-twH7FDRg@mail.gmail.com/:

Reported-by: Brian Wiltse <brian.wiltse@live.com>

> v1 -> v2:
>    Add explanation into commit messages

Thanks, much appreciated!

> Konstantin Kostiuk (2):
>    qga/win32: Remove change action from MSI installer
>    qga/win32: Use rundll for VSS installation
> 
>   qga/installer/qemu-ga.wxs | 11 ++++++-----
>   qga/vss-win32/install.cpp |  9 +++++++++
>   qga/vss-win32/qga-vss.def |  2 ++
>   3 files changed, 17 insertions(+), 5 deletions(-)
> 
> --
> 2.25.1
> 

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

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

ping

On Tue, Feb 21, 2023 at 1:41 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 21/2/23 12:21, Konstantin Kostiuk wrote:
> > resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2167423
> > fixes: CVE-2023-0664
> >
> > CVE Technical details: The cached installer for QEMU Guest Agent in
> c:\windows\installer
> > (https://github.com/qemu/qemu/blob/master/qga/installer/qemu-ga.wxs),
> > can be leveraged to begin a repair of the installation without validation
> > that the repair is being performed by an administrative user. The MSI
> repair
> > custom action "RegisterCom" and "UnregisterCom" is not set for
> impersonation
> > which allows for the actions to occur as the SYSTEM account
> > (LINE 137 AND 145 of qemu-ga.wxs). The custom action also leverages
> cmd.exe
> > to run qemu-ga.exe in line 134 and 142 which causes an interactive
> command
> > shell to spawn even though the MSI is set to be non-interactive on line
> 53.
> >
> > v1:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg05661.html
>
> Per
>
> https://lore.kernel.org/qemu-devel/CAA8xKjUQFBVgDVJ059FvGoSjkv+kZ5jB1gfMNz+ao-twH7FDRg@mail.gmail.com/
> :
>
> Reported-by: Brian Wiltse <brian.wiltse@live.com>
>
> > v1 -> v2:
> >    Add explanation into commit messages
>
> Thanks, much appreciated!
>
> > Konstantin Kostiuk (2):
> >    qga/win32: Remove change action from MSI installer
> >    qga/win32: Use rundll for VSS installation
> >
> >   qga/installer/qemu-ga.wxs | 11 ++++++-----
> >   qga/vss-win32/install.cpp |  9 +++++++++
> >   qga/vss-win32/qga-vss.def |  2 ++
> >   3 files changed, 17 insertions(+), 5 deletions(-)
> >
> > --
> > 2.25.1
> >
>
>

[-- Attachment #2: Type: text/html, Size: 2719 bytes --]

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

[Brian Wiltse]

[-- Attachment #1: Type: text/plain, Size: 4836 bytes --]

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, 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/), 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 or AdminUser property https://learn.microsoft.com/en-us/windows/win32/msi/adminuser or InstallPrivileges="Elevated" 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.

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.

Thanks,
Brian

________________________________
From: Konstantin Kostiuk <kkostiuk@redhat.com>
Sent: Monday, February 27, 2023 2:18 AM
To: Philippe Mathieu-Daudé <philmd@linaro.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; Daniel P . Berrangé <berrange@redhat.com>; Bin Meng <bin.meng@windriver.com>; Stefan Weil <sw@weilnetz.de>; Yonggang Luo <luoyonggang@gmail.com>; Markus Armbruster <armbru@redhat.com>; Alex Bennée <alex.bennee@linaro.org>; Peter Maydell <peter.maydell@linaro.org>; Gerd Hoffmann <kraxel@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Thomas Huth <thuth@redhat.com>; Marc-André Lureau <marcandre.lureau@redhat.com>; Michael Roth <michael.roth@amd.com>; Mauro Matteo Cascella <mcascell@redhat.com>; Yan Vugenfirer <yvugenfi@redhat.com>; Evgeny Iakovlev <eiakovlev@linux.microsoft.com>; Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>; Xuzhou Cheng <xuzhou.cheng@windriver.com>; Brian Wiltse <brian.wiltse@live.com>
Subject: Re: [PATCH v2 0/2] QGA installer fixes

ping

On Tue, Feb 21, 2023 at 1:41 PM Philippe Mathieu-Daudé <philmd@linaro.org<mailto:philmd@linaro.org>> wrote:
On 21/2/23 12:21, Konstantin Kostiuk wrote:. For example
> resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2167423
> fixes: CVE-2023-0664
>
> CVE Technical details: The cached installer for QEMU Guest Agent in c:\windows\installer
> (https://github.com/qemu/qemu/blob/master/qga/installer/qemu-ga.wxs),
> can be leveraged to begin a repair of the installation without validation
> that the repair is being performed by an administrative user. The MSI repair
> custom action "RegisterCom" and "UnregisterCom" is not set for impersonation
> which allows for the actions to occur as the SYSTEM account
> (LINE 137 AND 145 of qemu-ga.wxs). The custom action also leverages cmd.exe
> to run qemu-ga.exe in line 134 and 142 which causes an interactive command
> shell to spawn even though the MSI is set to be non-interactive on line 53.
>
> v1: https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg05661.html

Per
https://lore.kernel.org/qemu-devel/CAA8xKjUQFBVgDVJ059FvGoSjkv+kZ5jB1gfMNz+ao-twH7FDRg@mail.gmail.com/:

Reported-by: Brian Wiltse <brian.wiltse@live.com<mailto:brian.wiltse@live.com>>

> v1 -> v2:
>    Add explanation into commit messages

Thanks, much appreciated!

> Konstantin Kostiuk (2):
>    qga/win32: Remove change action from MSI installer
>    qga/win32: Use rundll for VSS installation
>
>   qga/installer/qemu-ga.wxs | 11 ++++++-----
>   qga/vss-win32/install.cpp |  9 +++++++++
>   qga/vss-win32/qga-vss.def |  2 ++
>   3 files changed, 17 insertions(+), 5 deletions(-)
>
> --
> 2.25.1
>


[-- Attachment #2: Type: text/html, Size: 8648 bytes --]

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

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

[Brian Wiltse]

[-- Attachment #1: Type: text/plain, Size: 4862 bytes --]

Hello,

I think this patch is sufficient to remediate the priv ledge escalation via the repair and catching the VSS com registration boxes that were being invoked frivolously.


Long term the repair function not validating if the user has admin should be addressed as well since their is still a potential for abuse. I dont see any other easy privledge elevation vulns at the moment other then an potential arbitrary file create where the creation of C:\programdata\qemu\qemu-ga.pid could be potentially be redirected via symbolic links to another file, but I have not been able to find time to fully prove that out. If we could get wixl support for the user privilege checks this would close abuse via the installer repair.


Thanks,

Brian

________________________________
From: Philippe Mathieu-Daudé <philmd@linaro.org>
Sent: Thursday, March 2, 2023 5:06 AM
To: Brian Wiltse <brian.wiltse@live.com>; Konstantin Kostiuk <kkostiuk@redhat.com>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; Daniel P . Berrangé <berrange@redhat.com>; Bin Meng <bin.meng@windriver.com>; Stefan Weil <sw@weilnetz.de>; Yonggang Luo <luoyonggang@gmail.com>; Markus Armbruster <armbru@redhat.com>; Alex Bennée <alex.bennee@linaro.org>; Peter Maydell <peter.maydell@linaro.org>; Gerd Hoffmann <kraxel@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Thomas Huth <thuth@redhat.com>; Marc-André Lureau <marcandre.lureau@redhat.com>; Michael Roth <michael.roth@amd.com>; Mauro Matteo Cascella <mcascell@redhat.com>; Yan Vugenfirer <yvugenfi@redhat.com>; Evgeny Iakovlev <eiakovlev@linux.microsoft.com>; Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>; Xuzhou Cheng <xuzhou.cheng@windriver.com>
Subject: Re: [PATCH v2 0/2] QGA installer fixes

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

[-- Attachment #2: Type: text/html, Size: 9761 bytes --]