From 9a97114f595562c91b0833b4a800dd51e9df65e9 Mon Sep 17 00:00:00 2001
From: Guillaume Bougard <gbougard@teclib.com>
Date: Tue, 19 Mar 2024 17:11:21 +0100
Subject: [PATCH] fix: Fix critical security issue on windows MSI packaging

---
 Changes                                      |  7 +++++++
 Makefile.PL                                  |  2 +-
 contrib/windows/glpi-agent-packaging.pl      | 20 +++++++++++++++++---
 contrib/windows/packaging/MSI_main-v2.wxs.tt | 13 +++++++++++++
 lib/GLPI/Agent/Version.pm                    |  4 ++--
 5 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/Changes b/Changes
index 24d38eccb..3d59db11c 100644
--- a/Changes
+++ b/Changes
@@ -1,5 +1,12 @@
 Revision history for GLPI agent
 
+1.7.2 not yet released
+
+packaging:
+* [SECURITY] Fix CVE-2024-28241: A local user could modify the GLPI-Agent installation
+  to gain higher privileges, but only when GLPI Agent is not installed in the default
+  installation folder
+
 1.7.1 Fri, 22 Dec 2023
 
 core:
diff --git a/Makefile.PL b/Makefile.PL
index 01fdd76c1..d45d02791 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -14,7 +14,7 @@ include 'Module::AutoInstall';
 abstract 'GLPI unified Agent for UNIX, Linux, Windows and MacOSX';
 license 'gpl';
 repository 'https://github.com/glpi-project/glpi-agent';
-version '1.7.1';
+version '1.7.2-dev';
 perl_version '5.008';
 authors 'Teclib Editions';
 
diff --git a/contrib/windows/glpi-agent-packaging.pl b/contrib/windows/glpi-agent-packaging.pl
index 70e999df7..60bf4b3a8 100644
--- a/contrib/windows/glpi-agent-packaging.pl
+++ b/contrib/windows/glpi-agent-packaging.pl
@@ -398,7 +398,16 @@ sub _tree2xml {
         # see: http://stackoverflow.com/questions/10358989/wix-using-keypath-on-components-directories-files-registry-etc-etc
         $feat = $self->_get_dir_feature($dir_id);
         $result .= $ident ."  ". qq[<Component Id="$component_id" Guid="{$component_guid}" KeyPath="yes" Feature="$feat">\n];
-        $result .= $ident ."  ". qq[    <CreateFolder />\n];
+        if ($dir_id eq 'd_install') {
+                $result .= $ident ."    ". qq[  <CreateFolder>\n];
+                $result .= $ident ."    ". qq[    <util:PermissionEx GenericAll="yes" User="CREATOR OWNER" />\n];
+                $result .= $ident ."    ". qq[    <util:PermissionEx GenericAll="yes" User="LocalSystem" />\n];
+                $result .= $ident ."    ". qq[    <util:PermissionEx GenericAll="yes" User="Administrators" />\n];
+                $result .= $ident ."    ". qq[    <util:PermissionEx GenericWrite="no" GenericExecute="yes" GenericRead="yes" User="AuthenticatedUser" />\n];
+                $result .= $ident ."    ". qq[  </CreateFolder>\n];
+        } else {
+                $result .= $ident ."  ". qq[    <CreateFolder />\n];
+        }
         if ($dir_id eq 'd_var') {
             $result .= $ident ."  ". qq[    <util:RemoveFolderEx On="uninstall" Property="UNINSTALL_VAR" />\n];
         } elsif ($dir_id eq 'd_etc') {
@@ -411,11 +420,16 @@ sub _tree2xml {
         $result .= $ident ."  ". qq[</Component>\n];
         # Also add virtual folder properties under d_install
         if ($dir_id eq 'd_install') {
-            foreach my $id (qw(_LOCALDIR)) {
+            foreach my $id (qw(LOCAL)) {
                 $result .= $ident ."  ". qq[<Directory Id="$id">\n];
                 ($component_id, $component_guid) = $self->_gen_component_id(lc($id).".create");
                 $result .= $ident ."    ". qq[<Component Id="$component_id" Guid="{$component_guid}" KeyPath="yes" Feature="$feat">\n];
-                $result .= $ident ."    ". qq[  <CreateFolder />\n];
+                $result .= $ident ."    ". qq[  <CreateFolder>\n];
+                $result .= $ident ."    ". qq[    <util:PermissionEx GenericAll="yes" User="CREATOR OWNER" />\n];
+                $result .= $ident ."    ". qq[    <util:PermissionEx GenericAll="yes" User="LocalSystem" />\n];
+                $result .= $ident ."    ". qq[    <util:PermissionEx GenericAll="yes" User="Administrators" />\n];
+                $result .= $ident ."    ". qq[    <util:PermissionEx GenericWrite="no" GenericExecute="yes" GenericRead="yes" User="AuthenticatedUser" />\n];
+                $result .= $ident ."    ". qq[  </CreateFolder>\n];
                 $result .= $ident ."    ". qq[  <RemoveFolder Id="rm.] .lc($id). qq[" On="uninstall" />\n];
                 $result .= $ident ."    ". qq[</Component>\n];
                 $result .= $ident ."  ". qq[</Directory>\n];
diff --git a/contrib/windows/packaging/MSI_main-v2.wxs.tt b/contrib/windows/packaging/MSI_main-v2.wxs.tt
index eba3aa442..44c467c8f 100644
--- a/contrib/windows/packaging/MSI_main-v2.wxs.tt
+++ b/contrib/windows/packaging/MSI_main-v2.wxs.tt
@@ -436,6 +436,12 @@
     <CustomAction Id="SetTaskDaily" Property="TaskDaily" Value="&quot;[SystemFolder]schtasks.exe&quot; /f /tn &quot;$(var.AgentName)&quot; /create /ru system /tr '&quot;[#f_glpiagent]&quot;' /sc daily /mo [TASK_DAILY_MODIFIER]" Execute="immediate" />
     <CustomAction Id="TaskDaily" BinaryKey="WixCA" DllEntry="WixQuietExec" Execute="deferred" Return="ignore" Impersonate="no"/>
 
+    <CustomAction Id="SetFixInstallDir" Property="FixInstallDir" Value="&quot;[SystemFolder]icacls.exe&quot; &quot;[INSTALLDIR].&quot; /inheritance:r" Execute="immediate" />
+    <CustomAction Id="FixInstallDir" BinaryKey="WixCA" DllEntry="WixQuietExec" Execute="deferred" Return="check" Impersonate="no"/>
+    <CustomAction Id="UpdateLocalDir" Property="LOCAL" Value="[LOCAL]\" Execute="immediate" />
+    <CustomAction Id="SetFixLocalDir" Property="FixLocalDir" Value="&quot;[SystemFolder]icacls.exe&quot; &quot;[LOCAL].&quot; /inheritance:r" Execute="immediate" />
+    <CustomAction Id="FixLocalDir" BinaryKey="WixCA" DllEntry="WixQuietExec" Execute="deferred" Return="check" Impersonate="no"/>
+
     <InstallExecuteSequence>
       <!-- InstallExecuteSequence is used to define Custom Actions that fire after the UI is finished and the install is starting to execute -->
       <Custom Action="CA_SetARPInstallLoc"    Before="RegisterProduct" />
@@ -448,6 +454,13 @@
       <AppSearch Sequence="100" />
       <LaunchConditions Sequence="200" />
 
+      <Custom Action="SchedSecureObjects_x64" After="CreateFolders"><![CDATA[NOT REMOVE~="ALL"]]></Custom>
+      <Custom Action="SetFixInstallDir" After="SchedSecureObjects"><![CDATA[NOT REMOVE~="ALL"]]></Custom>
+      <Custom Action="FixInstallDir" After="SetFixInstallDir"><![CDATA[NOT REMOVE~="ALL"]]></Custom>
+      <Custom Action="UpdateLocalDir" Before="CostFinalize"><![CDATA[LOCAL<>"" AND NOT LOCAL>>"\" AND NOT REMOVE~="ALL"]]></Custom>
+      <Custom Action="SetFixLocalDir" After="SchedSecureObjects"><![CDATA[LOCAL<>"" AND NOT REMOVE~="ALL"]]></Custom>
+      <Custom Action="FixLocalDir" After="SetFixLocalDir"><![CDATA[LOCAL<>"" AND NOT REMOVE~="ALL"]]></Custom>
+
       <!-- Schedule custom action to always remove windows task if exists -->
       <Custom Action="SetEndTask" Before="EndTask" />
       <Custom Action="EndTask" Before="StopServices" />
diff --git a/lib/GLPI/Agent/Version.pm b/lib/GLPI/Agent/Version.pm
index 0dbd6cee6..918dedf1f 100644
--- a/lib/GLPI/Agent/Version.pm
+++ b/lib/GLPI/Agent/Version.pm
@@ -3,7 +3,7 @@ package GLPI::Agent::Version;
 use strict;
 use warnings;
 
-our $VERSION = "1.7.1";
+our $VERSION = "1.7.2-dev";
 our $PROVIDER = "GLPI";
 our $COMMENTS = [];
 
@@ -31,5 +31,5 @@ agent issue is reported.
 One very useful information should be first defined like in that example:
 
 our $COMMENTS = [
-    "Based on GLPI Agent 1.7.1"
+    "Based on GLPI Agent 1.7.2-dev"
 ];