[PATCH 0/2] QGA installer fixes

[PATCH 0/2] QGA installer fixes

[Konstantin Kostiuk]

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 1/2] qga/win32: Remove change action from MSI installer

[Konstantin Kostiuk]

resolves: rhbz#2167436
fixes: CVE-2023-0664

Signed-off-by: Konstantin Kostiuk <kkostiuk@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 2/2] qga/win32: Use rundll for VSS installation

[Konstantin Kostiuk]

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

resolves: rhbz#2167436
fixes: CVE-2023-0664

Signed-off-by: Konstantin Kostiuk <kkostiuk@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 2/2] qga/win32: Use rundll for VSS installation

[Yan Vugenfirer]

Reviewed-by: Yan Vugenfirer <yvugenfi@redhat.com>


On Mon, Feb 20, 2023 at 7:41 PM Konstantin Kostiuk <kkostiuk@redhat.com> wrote:
>
> Add specific an entry points for rundll which is
> just a wrapper for COMRegister/COMUnregister functions.
>
> resolves: rhbz#2167436
> fixes: CVE-2023-0664
>
> Signed-off-by: Konstantin Kostiuk <kkostiuk@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 1/2] qga/win32: Remove change action from MSI installer

[Yan Vugenfirer]

Reviewed-by: Yan Vugenfirer <yvugenfi@redhat.com>

On Mon, Feb 20, 2023 at 7:41 PM Konstantin Kostiuk <kkostiuk@redhat.com> wrote:
>
> resolves: rhbz#2167436
> fixes: CVE-2023-0664
>
> Signed-off-by: Konstantin Kostiuk <kkostiuk@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
>

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

[Philippe Mathieu-Daudé]

On 20/2/23 18:41, Konstantin Kostiuk wrote:
> resolves: rhbz#2167436

"You are not authorized to access bug #2167436."

> fixes: CVE-2023-0664

This commit description is rather scarce...

I understand you are trying to fix a CVE, but we shouldn't play
the "security by obscurity" card. How can the community and
distributions know this security fix is enough with the bare
"Remove change action from MSI installer" justification?
Can't we do better?

> Signed-off-by: Konstantin Kostiuk <kkostiuk@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
> 
> 

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

[Mauro Matteo Cascella]

Hi Philippe,

On Tue, Feb 21, 2023 at 9:15 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 20/2/23 18:41, Konstantin Kostiuk wrote:
> > resolves: rhbz#2167436
>
> "You are not authorized to access bug #2167436."

Please refer to https://bugzilla.redhat.com/show_bug.cgi?id=2167423.
It should now be accessible.

> > fixes: CVE-2023-0664
>
> This commit description is rather scarce...
>
> I understand you are trying to fix a CVE, but we shouldn't play
> the "security by obscurity" card. How can the community and
> distributions know this security fix is enough with the bare
> "Remove change action from MSI installer" justification?
> Can't we do better?

CCing Brian Wiltse, who originally found and reported this issue.

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

> > Signed-off-by: Konstantin Kostiuk <kkostiuk@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
> >
> >
>

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

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

[Konstantin Kostiuk]

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

On Tue, Feb 21, 2023 at 10:15 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 20/2/23 18:41, Konstantin Kostiuk wrote:
> > resolves: rhbz#2167436
>
> "You are not authorized to access bug #2167436."
>
> > fixes: CVE-2023-0664
>
> This commit description is rather scarce...
>
> I understand you are trying to fix a CVE, but we shouldn't play
> the "security by obscurity" card. How can the community and
> distributions know this security fix is enough with the bare
> "Remove change action from MSI installer" justification?
> Can't we do better?
>

This patch is part of the fix. I remove the 'change' button because
the installer has no components to choose from and the installer
always installs everything.

The second patch removes the interactive command shell.


>
> > Signed-off-by: Konstantin Kostiuk <kkostiuk@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
> >
> >
>
>

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

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

[Daniel P. Berrangé]

On Tue, Feb 21, 2023 at 09:15:15AM +0100, Philippe Mathieu-Daudé wrote:
> On 20/2/23 18:41, Konstantin Kostiuk wrote:
> > resolves: rhbz#2167436
> 
> "You are not authorized to access bug #2167436."
> 
> > fixes: CVE-2023-0664
> 
> This commit description is rather scarce...
> 
> I understand you are trying to fix a CVE, but we shouldn't play
> the "security by obscurity" card. How can the community and
> distributions know this security fix is enough with the bare
> "Remove change action from MSI installer" justification?
> Can't we do better?

Yes, commit messages should always describe the problem being
solved directly. Bug trackers usually make people wade through
piles of irrelevant comments & potentially misleading blind
alleys during the back & forth of triage. The important info
needs to be distilled down and put in the commit message,
concisely describing the problem faced. Bug tracker links have
been known to bit-rot too.

The commit message needs to focus on /why/ the change was made,
much more than describing /what/ was changed.

> > Signed-off-by: Konstantin Kostiuk <kkostiuk@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."
> >         />

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|