search

oleg-shilo / wixsharp

Framework for building a complete MSI or WiX source code by using script files written with C# syntax.
MIT License
1.1k stars 174 forks source link

When the installer rolls back, the SE_DACL_AUTO_INHERITED is removed from the C:\ drive #1336

Closed julien-lebot closed 8 months ago

julien-lebot commented 1 year ago

Bug description

When the installer rolls back SE_DACL_AUTO_INHERITED is removed from the C:\ drive.

Steps to reproduce

Start from the Registry sample code. Add the following bit of code to import the WIXFAILWHENDEFERRED custom action:

project.WixSourceGenerated += (doc) =>
{
    project.IncludeWixExtension(WixExtension.Util);
    doc
        .Select("Wix/Product")
        .AddElement("CustomActionRef", "Id=WixFailWhenDeferred");
};

Check the ACLs on the C:\ drive:

get-acl C:\ | fl

Path   : Microsoft.PowerShell.Core\FileSystem::C:\
Owner  : NT SERVICE\TrustedInstaller
Group  : NT SERVICE\TrustedInstaller
Access : CREATEUR PROPRIETAIRE Allow  268435456
         AUTORITE NT\Système Allow  FullControl
         BUILTIN\Administrateurs Allow  FullControl
         BUILTIN\Utilisateurs Allow  AppendData
         BUILTIN\Utilisateurs Allow  CreateFiles
         BUILTIN\Utilisateurs Allow  ReadAndExecute, Synchronize
Audit  :
Sddl   : O:S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464G:S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464D:PAI(A;OICIIO;GA;;;CO)(A;OICI;FA;;;SY)(A;OIC
         I;FA;;;BA)(A;CI;LC;;;BU)(A;CIIO;DC;;;BU)(A;OICI;0x1200a9;;;BU)

Note the AI flag for the auto inherited DACL.

Run the installer as follows:

msiexec /i registry_sample.msi WIXFAILWHENDEFERRED=1

Check the ACLs on the C:\ drive:

get-acl C:\ | fl

Path   : Microsoft.PowerShell.Core\FileSystem::C:\
Owner  : NT SERVICE\TrustedInstaller
Group  : NT SERVICE\TrustedInstaller
Access : CREATEUR PROPRIETAIRE Allow  268435456
         AUTORITE NT\Système Allow  FullControl
         BUILTIN\Administrateurs Allow  FullControl
         BUILTIN\Utilisateurs Allow  AppendData
         BUILTIN\Utilisateurs Allow  CreateFiles
         BUILTIN\Utilisateurs Allow  ReadAndExecute, Synchronize
Audit  :
Sddl   : O:S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464G:S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464D:P(A;OICIIO;GA;;;CO)(A;OICI;FA;;;SY)(A;OICI;
         FA;;;BA)(A;CI;LC;;;BU)(A;CIIO;DC;;;BU)(A;OICI;0x1200a9;;;BU)

The AI flag was removed.

clarkb7 commented 1 year ago

New debugging info.

This happens during CreateFolder rollback. Here's a callstack from procmon. image

You can reproduce clearing the AI flag with python

import win32security
sd=win32security.GetFileSecurity("C:\\", win32security.DACL_SECURITY_INFORMATION)
win32security.SetFileSecurity("C:\\", win32security.DACL_SECURITY_INFORMATION, sd)

I think that the WiX# AutoElements code is generating elements that cause C:\\ and other directories to be put in the MSIs CreateFolder table. Then any directories in that table are subject to this Windows Installer bug.

WiX# shouldn't be generating Component/CreateFolder/RemoveFolder elements for Directories that are outside of the installation, right?

oleg-shilo commented 1 year ago

Txs for reporting. Currently investigating it...

oleg-shilo commented 1 year ago

WiX# shouldn't be generating Component/CreateFolder/RemoveFolder elements for Directories that are outside of the installation, right?

Yes and it seems like it does not. This is the unmodified WXS content of your sample:

<?xml version="1.0" encoding="utf-8"?>
<Wix xmlns="http://schemas.microsoft.com/wix/2006/wi">
  <Product Id="6f330b47-2577-43ad-9095-1861ca25889c" Name="MyProduct" Language="1033" Codepage="Windows-1252" Version="1.0.0.0" UpgradeCode="6f330b47-2577-43ad-9095-1861ba25889b" Manufacturer="oleg.shilo">
    <Package InstallerVersion="200" Compressed="yes" SummaryCodepage="Windows-1252" Languages="1033" />
    <Media Id="1" Cabinet="MyProduct.cab" EmbedCab="yes" />

    <Directory Id="TARGETDIR" Name="SourceDir">
      <Directory Id="ProgramFilesFolder" Name="ProgramFilesFolder">
        <Directory Id="My20Company" Name="My Company">
          <Directory Id="INSTALLDIR" Name="My Product">

            <Component Id="Component.readme.txt_1240382565" Guid="6f330b47-2577-43ad-9095-18615005cac4">
              <File Id="readme.txt_1240382565" Source="readme.txt" />
            </Component>

            <Component Id="RemoveRegistryKey" Guid="6f330b47-2577-43ad-9095-18615e65d734" KeyPath="yes">
              <RemoveRegistryKey Id="RemoveRegistryKey" Root="HKLM" Key="Software\My Company\My Product" Action="removeOnUninstall" />

              <CreateFolder />
              <RemoveFolder Id="INSTALLDIR" On="uninstall" />
            </Component>

          </Directory>

          <Component Id="My20Company" Guid="6f330b47-2577-43ad-9095-186154b4b41d" KeyPath="yes">
            <RemoveFolder Id="My20Company" On="uninstall" />
          </Component>

        </Directory>

        <Component Id="Registry.1" Guid="6f330b47-2577-43ad-9095-1861b742561a" KeyPath="yes">
          <RegistryKey Root="HKLM" Key="Software\My Company\My Product">
            <RegistryValue Id="LICENSE_KEY" Type="binary" KeyPath="no" Value="01020304" Name="LICENSE_KEY">
              <Permission Read="yes" User="usr" />
            </RegistryValue>
          </RegistryKey>

          <CreateFolder />
          <RemoveFolder Id="ProgramFilesFolder" On="uninstall" />
        </Component>

        <Component Id="Registry.2" Guid="6f330b47-2577-43ad-9095-1861b743561a">
          <RegistryKey Root="HKLM" Key="Software\My Company\My Product">
            <RegistryValue Id="Message" Type="string" KeyPath="yes" Value="Hello" Name="Message" />
          </RegistryKey>

          <CreateFolder />
        </Component>

        <Component Id="Registry.3" Guid="6f330b47-2577-43ad-9095-1861b744561a">
          <RegistryKey Root="HKLM" Key="Software\My Company\My Product">
            <RegistryValue Id="Count" Type="integer" KeyPath="yes" Value="777" Name="Count" />
          </RegistryKey>

          <CreateFolder />
        </Component>

        <Component Id="Registry.4" Guid="6f330b47-2577-43ad-9095-1861b73d561a">
          <RegistryKey Root="HKLM" Key="Software\My Company\My Product">
            <RegistryValue Id="Index" Type="integer" KeyPath="yes" Value="333" Name="Index" />
          </RegistryKey>

          <CreateFolder />
        </Component>

        <Component Id="Registry.5" Guid="6f330b47-2577-43ad-9095-1861b73e561a">
          <RegistryKey Root="HKCR" Key="test\shell\open\command">
            <RegistryValue Id="RegValue" Type="expandable" KeyPath="yes" Value="&quot;[INSTALLDIR]test.exe&quot; &quot;%1&quot;" />
          </RegistryKey>

          <CreateFolder />
        </Component>

        <Component Id="Registry.6" Guid="6f330b47-2577-43ad-9095-1861b73f561a">
          <RegistryKey Root="HKCR" Key="test\shell\open\command2">
            <RegistryValue Id="RegValue.1" Type="expandable" KeyPath="yes" Value="&quot;[CommonAppDataFolder]test.exe&quot; &quot;%1&quot;" />
          </RegistryKey>

          <CreateFolder />
        </Component>

      </Directory>

      <Component Id="TARGETDIR" Guid="6f330b47-2577-43ad-9095-18612df5f80e" KeyPath="yes">
        <RemoveFolder Id="TARGETDIR" On="uninstall" />
      </Component>

    </Directory>

    <Feature Id="MyApp20Binaries" Title="MyApp Binaries" Absent="allow" Level="1">
      <ComponentRef Id="Component.readme.txt_1240382565" />
      <ComponentRef Id="Registry.1" />
      <ComponentRef Id="Registry.2" />
      <ComponentRef Id="Registry.3" />
      <ComponentRef Id="Registry.4" />
      <ComponentRef Id="Registry.5" />
      <ComponentRef Id="Registry.6" />
      <ComponentRef Id="RemoveRegistryKey" />
      <ComponentRef Id="My20Company" />
      <ComponentRef Id="TARGETDIR" />
    </Feature>

    <CustomActionRef Id="WixFailWhenDeferred" />

  </Product>
</Wix>

Note, the solution creates TARGETDIR\ProgramFilesFolder\My Company\My Product dir. The CreateFolder element is auto-inserted only for ProgramFilesFolder, My Company and My Product. Thus I am not sure even why C:\ is in the picture at all.

There are questionable RemoveFolder in TARGETDIR and in ProgramFilesFolder but they are only exercised on uninstall. These dirs will not be removed at runtime anyway as they are always non-empty.

I am completely puzzled.

BTW this is the routine that inserts CreateFolder for any component of the Dir without files: https://github.com/oleg-shilo/wixsharp/blob/7601893a5e9e14408ce9ef63415290d5c56254a6/Source/src/WixSharp/AutoElements.cs#L655C28-L655C28.

I can only suggest that you try removing CreateFolder/RemoveFolder and see if it impacts the behaviour:

project.WixSourceGenerated += (doc) =>
             {
                 doc.FindAll("CreateFolder")
                    .ForEach(x => x.Remove());
                 doc.FindAll("RemoveFolder")
                    .ForEach(x => x.Remove());
             };
julien-lebot commented 1 year ago

Thanks for your input Oleg.

Thus I am not sure even why C:\ is in the picture at all.

If you look at the call stack Branden provided, I believe it could be the roll back action of CreateFolder on TARGETDIR.

I can only suggest that you try removing CreateFolder/RemoveFolder and see if it impacts the behaviour:

Yes, removing the entries in the CreateFolder table as a post-processing step is our approach as well. Wix# also seems to be auto-generating components for the TARGETDIR, ProgramFiles64Folder and the installation root which end up creating entries in the CreateFolder table, so we had to remove them as well, see https://github.com/DataDog/datadog-agent/pull/19298/files#diff-c15f6fd4e6dba7d135a55a39a62d22db6386a879bf0d11c4dde2edf2d1426a13R253-R266

oleg-shilo commented 1 year ago

Interesting.

If you look at the call stack Branden provided, I believe it could be the roll back action of CreateFolder on TARGETDIR.

Actually, the wxs I provided does not have CreateFolder on TARGETDIR.

Wix# also seems to be auto-generating components for the TARGETDIR, ProgramFiles64Folder and the installation root which end up creating entries in the CreateFolder table.

Let's bring it all together

dir component CreateFolder RemoveFolder
TARGETDIR yes no yes
ProgramFiles64Folder yes yes yes
<root> yes yes yes

TARGETDIR

TARGETDIR is not the suspect. Interestingly enough I tried to suspend the generation of the component element for TARGETDIR but it triggered the compile error error LGHT0094 : Unresolved reference to symbol 'Component:TARGETDIR'.

There is a small chance that it is RemoveFolder folder that triggers it indirectly. This is easy to test by simply removing it before the compilation. Interestingly enough WiX4 completely ditched TARGETDIR concept.

ProgramFiles64Folder

I argue that ProgramFiles64Folder as any other standard dir should have CreateFolder as it can be some standard dir that does not exist on the target system during the installation. Though it's debatable.

<root>

This is the case when the CreateFolder has all the reasons to exist.

Thus I am still not sure why C:\ is impacted.

BTW, you can always suppress the auto-generation of the CreateFolder element:

AutoElements.SupportEmptyDirectories = CompilerSupportState.Disabled;
clarkb7 commented 1 year ago

Actually, the wxs I provided does not have CreateFolder on TARGETDIR. Thus I am still not sure why C:\ is impacted.

I've been using Orca to examine the CreateFolder MSI table. The CreateFolder element is not the only thing that causes folders to be added to the CreateFolder MSI table. We also have to remove the Component elements that are created to host the CreateFolder and RemoveFolder elements for each directory. It seems that Component elements with KeyPath="yes" set also add the parent Directory element to the CreateFolder MSI table, regardless of if there is a CreateFolder, RemoveFolder, or any other element inside that component.

TARGETDIR is not the suspect. Interestingly enough I tried to suspend the generation of the component element for TARGETDIR but it triggered the compile error error LGHT0094 : Unresolved reference to symbol 'Component:TARGETDIR'.

There is a ComponentRef element under the Feature element that must also be removed.

BTW, you can always suppress the auto-generation of the CreateFolder element:

We tried this and it does remove all of the CreateFolder elements, but there's still something else that is auto-generating a Component for TARGETDIR along with a RemoveFolder. This also puts TARGETDIR in the CreateFolder MSI table.

I argue that ProgramFiles64Folder as any other standard dir should have CreateFolder as it can be some standard dir that does not exist on the target system during the installation. <root>: This is the case when the CreateFolder has all the reasons to exist.

Won't the Windows Installer recursively create the directory tree necessary to place files that it is installing, without needing an explicit entry in the CreateFolder table? I thought it was only necessary for creating empty directories.

oleg-shilo commented 1 year ago

Won't the Windows Installer recursively create the directory tree necessary to place files that it is installing, without needing an explicit entry in the CreateFolder table? I thought it was only necessary for creating empty directories.

No, if the folders are empty (no files). And support for empty folders is a feature that was requested and later added to WixSharp.

...There is a ComponentRef element under the Feature element that must also be removed. ...but there's still something else that is auto-generating a Component for TARGETDIR along with a RemoveFolder.

Then it explains. IMHO the concept of component is fundamentally flawed. Instead of being just a group of deployment artifacts, it plays the role of the triggers for certain runtime behaviour (e.g. creating a folder). Then "CreatFolder" instead of encoding only folder creation triggers changes in the security context of the file system elements. WiX is full of these antipatterns. And WiXShar is an attempt to at least minimize the diversity of the combinations of these antipatterns.

Anyway, I would prefer to implement the countermeasures so let me have a look at the TARGETDIR business. I think I can clean it and have the problem addressed. Basically what you are reporting is a new (previously unknown) side-effect of WixSharp auto-generation. So the auto-generation algorithm needs to be reviewed.

oleg-shilo commented 1 year ago

I have found the trigger for creating entries in the CreateFolder table when WXS does not ask for it (that Registry sample).

It is the RemoveRegistryKey element. In contrast to RegistryKey, it requires the corresponding entry of its parent dir in the CreateFolder. This is achieved via KeyPath attribute:

LGHT0204 : ICE18: KeyPath for Component: 'RemoveRegistryValue' is Directory: 
'ProgramFilesFolder'. The Directory/Component pair must be listed in the CreateFolders table.

However if you remove 'RemoveRegistryValue' from the sample it will not even create a CreateFolder table.

AutoElements.SupportEmptyDirectories = CompilerSupportState.Disabled;

var project =
    new Project("MyProduct",
        new Dir(@"%ProgramFiles%\My Company\My Product",
            new File(fullSetup, @"readme.txt")),
        new RegValue(fullSetup, RegistryHive.LocalMachine, @"Software\My Company\My Product", "LICENSE_KEY", "01020304") { AttributesDefinition = "Type=binary", Permissions = new[] { new Permission { User = "usr", Read = true } }         },
        new RegValue(fullSetup, RegistryHive.ClassesRoot, @"test\shell\open\command", "", "\"[INSTALLDIR]test.exe\" \"%1\""),
        new RegValue(fullSetup, RegistryHive.ClassesRoot, @"test\shell\open\command2", "", "\"[CommonAppDataFolder]test.exe\" \"%1\""),

The remaining question is how to avoid modifying C:\ drive attributes if you have to use RemoveRegistryValue. I found that the presence of TARGETDIR in CreateFolder table seems to have no impact so I can easily remove it during the build.

image

Can you please help me to test the theory that the C:\ problem is caused by a very specific entry TARGETDIR in CreateTable, not any other dir.

Can you please rebuild your msi with this modification and then test in your environment?

project.WixSourceGenerated += (doc) =>
{
    doc.FindAll("ComponentRef")
       .Where(x => x.HasAttribute("Id", "TARGETDIR"))
       .ForEach(x => x.Remove());

    doc.FindAll("Component")
       .Where(x => x.HasAttribute("Id", "TARGETDIR"))
       .ForEach(x => x.Remove());
};

Note, we are trying to find a workaround but not a solution. The solution is for the CMsiOpExecute::CreateFolder to stop messing with the drive attributes. It is obvious, we have no control over it.

Interesting reading about KeyPath here: https://stackoverflow.com/questions/2003043/what-is-the-wix-keypath-attribute It again confirms that WiX is just a more manageable interface to a barely manageable MSI db schema. And if one wants to be effective he/she still often needs to think about deployment not as an algorithm but a collection of the MSI tables 😞

oleg-shilo commented 8 months ago

It looks like there is no further info is available from the issue author. So closing it for now.

clarkb7 commented 1 month ago

I have a short and disappointing update.

We contacted Microsoft Support (on 09/2023) and they confirmed that it is indeed a bug in MSI.DLL. At first they said they were going to fix it (on 02/2024) but they later changed their mind (on 06/2024). They need additional business and impact justification in order to prioritize it.

The underlying root cause is that MSI.DLL uses the obsolete SetFileSecurityW API, as described in https://github.com/oleg-shilo/wixsharp/issues/1336#issuecomment-1703610582, instead of the newer SetNamedSecurityInfoW API which supports ACE inheritance.

So all we can do is implement the workarounds described above to try to avoid the scenarios in which MSI.DLL will call SetFileSecurityW. It's not as much of an issue if it only affects folders that are going to be removed on rollback, or if your installer also modifies the permissions of those folders on install. But it becomes a larger issue if system folders like C:\ or parent folders that linger or are shared with other MSIs make their way into the CreateFolder table or become KeyPaths, and thus become a target of SetFileSecurityW during rollback.

If anyone comes across this issue in the future and contacts Microsoft about it, you can reference our support case number 2309110010002832.

iglendd commented 1 month ago

They need additional business and impact justification in order to prioritize it.

Would not a lurking security hole and high chances for exploitation, when otherwise well protected and unsuspected installed product, can be exploited accidently or on purpose, is a very significant “business and impact justification”, considering that it is a Microsoft bug? Investigation will link it to the bug Microsoft knew about but decided to ignore due to the lack of “business justification”?

oleg-shilo commented 1 month ago

@clarkb7, Brandon, thank you so much for sharing such useful detailed feedback. Much appreciated

clarkb7 commented 1 month ago

@oleg-shilo thank you for your in depth assistance!