sharpcompress icon indicating copy to clipboard operation
sharpcompress copied to clipboard

Entry is trying to write a file outside of the destination

Open crystalgreen opened this issue 9 months ago • 15 comments

I get the above mentioned error with this code

        using var reader = package.ExtractAllEntries();
        reader.WriteAllToDirectory(pkgWorkDir, new SharpCompress.Common.ExtractionOptions { PreserveFileTime = true, ExtractFullPath = true, Overwrite = true, PreserveAttributes = true });

when running as Windows Service and the LocalSystem account. I use pkgWorkDir that is constructed starting with Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData).

On my system, this returns C:\Windows\system32\config\systemprofile\AppData\Local. The code throws the error

Entry is trying to write a file outside of the destination directory: C:\Windows\system32\config\systemprofile\AppData\Local

As one can see, there is a difference between System32 and system32.

The lib code here uses StringComparison.Ordinal to compare the strings.

My issue could be fixed, if - under Windows OS - we use StringComparison.OrdinalIgnoreCase.

crystalgreen avatar Mar 21 '25 11:03 crystalgreen

This isn't an obvious fix because other OSes use case sensitive file systems. I guess the addition of an option based on OS is relevant

adamhathcock avatar Mar 21 '25 15:03 adamhathcock

I can't reproduce this with a simple example zip containing only one entry.

Logging the relevant paths on extract results in:

local application data: C:\WINDOWS\system32\config\systemprofile\AppData\Local
destinationFileName: C:\WINDOWS\system32\config\systemprofile\AppData\Local\sharpcompress test\filename.test
fullDestinationDirectoryPath: C:\WINDOWS\system32\config\systemprofile\AppData\Local\sharpcompress test\

As you can see, all of these use lowercase system32 and still work. Could you provide an example zip and full example code that reproduces this issue?

Morilli avatar Mar 21 '25 18:03 Morilli

This problem do not happen on each windows system. I got three environments and the problem only exist on one system. So there would be no specific example code which could be provided. The best would be as suggested by @adamhathcock to add an extra option or flag which can be set.

DoeWayne avatar Mar 24 '25 08:03 DoeWayne

or just write your own extraction code....the write to destination is just a shortcut method.

adamhathcock avatar Mar 24 '25 09:03 adamhathcock

I extract a 7z archive and the USAGE doc indicates it is more efficient to use ExtractAllEntries or WriteAllToDirectory. Is there another efficient way with more control over extraction?

crystalgreen avatar Mar 24 '25 10:03 crystalgreen

look and see what those methods do....they get the entries and stream copy basically

adamhathcock avatar Mar 24 '25 10:03 adamhathcock

This problem do not happen on each windows system. I got three environments and the problem only exist on one system.

What makes you so sure that the string comparison is the issue then? The fact that this code does work on some systems, even if the paths have casing different from what exists on the disk makes me believe that case differences are not the problem here.

Perhaps if you could compile the library while adding logs to the failing code it would help in tracking down the cause:

diff --git a/src/SharpCompress/Common/ExtractionMethods.cs b/src/SharpCompress/Common/ExtractionMethods.cs
index 799d47b3..00850d77 100644
--- a/src/SharpCompress/Common/ExtractionMethods.cs
+++ b/src/SharpCompress/Common/ExtractionMethods.cs
@@ -66,6 +66,8 @@ internal static class ExtractionMethods
         {
             destinationFileName = Path.GetFullPath(destinationFileName);

+            Console.WriteLine($"destinationFileName: {destinationFileName}");
+            Console.WriteLine($"fullDestinationDirectoryPath: {fullDestinationDirectoryPath}");
             if (
                 !destinationFileName.StartsWith(
                     fullDestinationDirectoryPath,

Morilli avatar Mar 24 '25 13:03 Morilli

Perhaps if you could compile the library while adding logs to the failing code it would help in tracking down the cause:

We did this already: Entry is trying to write a file outside of the destination directory: fullDestinationDirectoryPath: C:\Windows\system32\config\systemprofile\AppData\Local\AppPkgs\XL9j\7OLtRyxbLNavRSbmtk3UQ_tmp
destinationFileName: C:\Windows\System32\config\systemprofile\AppData\Local\AppPkgs\XL9j\7OLtRyxbLNavRSbmtk3UQ_tmp\js\7cf1.js

DoeWayne avatar Mar 24 '25 14:03 DoeWayne

Something weird must be going on then because the only operations performed on the path are Path.Combine which doesn't even touch the paths and Path.GetFullPath which should generally also not modify the paths passed to it, however I could see some weird things happening if windows' short paths are used somehow.

Using case-insensitive comparison should definitely not be the fix here in any case as most file systems are case sensitive, including NTFS.

Morilli avatar Mar 24 '25 16:03 Morilli

Here some more test we did

        if (options.ExtractFullPath)
        {
            var folder = Path.GetDirectoryName(entry.Key.NotNull("Entry Key is null"))
                .NotNull("Directory is null");
            var destdir = Path.GetFullPath(Path.Combine(fullDestinationDirectoryPath, folder));
            Console.WriteLine($"folder: {folder}");
            Console.WriteLine($"Check fulldesdirpath: {Path.Combine(fullDestinationDirectoryPath, folder)}");
            Console.WriteLine($"destdir: {destdir}");
            Console.WriteLine($"file: {file}");
            if (!Directory.Exists(destdir))
            {
                if (!destdir.StartsWith(fullDestinationDirectoryPath, StringComparison.Ordinal))
                {
                    throw new ExtractionException(
                        "Entry is trying to create a directory outside of the destination directory."
                    );
                }

                Directory.CreateDirectory(destdir);
            }
            Console.WriteLine($"1 destdir: {destdir}");
            destinationFileName = Path.Combine(destdir, file);
            Console.WriteLine($"2 destdir: {destdir}");
            Console.WriteLine($"2 destinationFileName: {destinationFileName}");
        }
        else
        {
            destinationFileName = Path.Combine(fullDestinationDirectoryPath, file);
        }

        if (!entry.IsDirectory)
        {
            destinationFileName = Path.GetFullPath(destinationFileName);
            Console.WriteLine($"destinationFileName: {destinationFileName}");
            Console.WriteLine($"fullDestinationDirectoryPath: {fullDestinationDirectoryPath}");
            if (
                !destinationFileName.StartsWith(
                    fullDestinationDirectoryPath,
                    StringComparison.Ordinal
                )
            )
            {
                throw new ExtractionException(
                    $"Entry is trying to write a file outside of the destination directory: {fullDestinationDirectoryPath} file: {destinationFileName} "
                );
            }
            write(destinationFileName, options);
        }
        else if (options.ExtractFullPath && !Directory.Exists(destinationFileName))
        {
            Directory.CreateDirectory(destinationFileName);
        }
    }
folder: pwa\js
Check fulldesdirpath: C:\Windows\system32\config\systemprofile\Apple\pwa\js
destdir: C:\Windows\system32\config\systemprofile\Apple\pwa\js
file: babel~app.41f17cf1.js
1 destdir: C:\Windows\system32\config\systemprofile\Apple\pwa\js
2 destdir: C:\Windows\system32\config\systemprofile\Apple\pwa\js
2 destinationFileName: C:\Windows\system32\config\systemprofile\Apple\pwa\js\babel~app.41f17cf1.js
destinationFileName: C:\Windows\System32\config\systemprofile\Apple\pwa\js\babel~app.41f17cf1.js
fullDestinationDirectoryPath: C:\Windows\system32\config\systemprofile\Apple\

So it looks like that the

Path.GetFullPath(destinationFileName);

is changing it. We created a small example and if we run it in that environment it failed as well. But as mentioned not in all environments:

using System;
using System.Collections.Generic;
using System.IO;
using System.Text;

namespace ConsoleApp2
{
    internal class Program
    {
        static void Main(string[] args)
        {
            string fullDestinationDirectoryPath = "C:\\Windows\\system32\\config\\systemprofile\\AppData\\Local";
            Console.WriteLine($"{Path.Combine(fullDestinationDirectoryPath, "babel~app.41f17cf1.js")}");
            Console.WriteLine($"{Path.GetFullPath(Path.Combine(fullDestinationDirectoryPath, "babel~app.41f17cf1.js"))}");
        }
    }
}

Failed on Windows 2016 1607 Build 14393.7876

DoeWayne avatar Mar 25 '25 15:03 DoeWayne

Odd...doing a search and the AI says the method will "normalize" casing but haven't found anywhere saying it yet

Image

adamhathcock avatar Mar 25 '25 17:03 adamhathcock

Tilde in the filename seems to trigger this behavior, As @Morilli suspected it's due to short path (8.3) handling, but it's not even a short name really, it happens with any tilde.

Image

jdpurcell avatar Mar 25 '25 23:03 jdpurcell

As for a workaround you could try this before calling WriteAllToDirectory:

pkgWorkDir = Path.GetDirectoryName(Path.GetFullPath(Path.Combine(pkgWorkDir, "~")));

jdpurcell avatar Mar 26 '25 01:03 jdpurcell

This unfortunately appears to be a badly documented quirk of Path.GetFullPath: The documentation states If you pass in a short file name, it is expanded to a long file name, however

  • as seen here, paths containing ~ that aren't short paths are also expanded, and
  • short path that do not contain ~ are never expanded, even though they should be

So in effect, the function may call GetLongPathNameW on (part) of the passed in path, which may change casing (of part of the path). In my opinion this is a bug in the .NET runtime library.

Morilli avatar Mar 26 '25 16:03 Morilli

@jdpurcell Your workaround looks promising, thanks.

This issue can be closed unless you consider adding the workaround to the lib.

crystalgreen avatar Mar 27 '25 09:03 crystalgreen