Path.Combine and Uri violate Principle of least suprise

[MarcusWichelmann]

The Path.Combine (string path1, string path2) method and the Uri constructor Uri (Uri baseUri, string relativeUri) behave very unexpected when the second parameter (the relative path) is an absolute path.

A very common use-case for these handy methods is, that the first parameter is an absolute path like C:\RootDir\ and the second parameter is a user-defined relative path like foo\bar.txt. In this case the methods behave as expected and are very popular tool.
But a thing that most people do not expect and never think of is, that these methods prioritise the second parameter if it is an absolute path and completely skip the first parameter then.

Examples:

Path.Combine(@"C:\WebDir", @"C:\Passwords\SecretFile.txt")
// => "C:\\Passwords\\SecretFile.txt"
new Uri(new Uri(@"C:\WebDir"), @"C:\Passwords\SecretFile.txt")
// => [file:///C:/Passwords/SecretFile.txt]

Because of this the use of these methods in environments where the second parameter is user-defined (e.g. a Webserver) might be very dangerous and could cause serious security issues.
Even though this behaviour is visible at the end of the documentation (https://msdn.microsoft.com/de-de/library/fyy7a5kt(v=vs.110).aspx), I don't think any developer will read the full documentation of every API method he uses and therefore needs to have the trust that these methods do what he expects them to do.
See: https://en.wikipedia.org/wiki/Principle_of_least_astonishment

And this misconception doesn't only exist in theory.
I've already found multiple bugs in source codes that use Path.Combine and would make users able to read or write to any file on the filesystem by adding "C:\somefile.txt" to a HTTP URL or opening malicious ZIP files in applications.

Some examples are:

• Big Security Issue! StaticFilesModule allows access to ANY file on harddrive. unosquare/embedio#68
• Be careful with using Path.Combine() FubarDevelopment/WebDavServer#32
• Example code introduces very big security flaw because of Path.Combine() icsharpcode/SharpZipLib#180

How do you think about this?
Is it possible to do an API change to support developers in writing more secure code in future?

Thank you and kind regards,
Marcus Wichelmann

[filipnavara]

This is something that historically made a lot of sense. The common use case is Path.Combine(Environment.CurrentDirectory, someFileNameInArgument) or new Uri(myCurrentWebPageUri, anUriSpecifiedInAHref). Also, you can escape the first absolute path by merely using ../../whatever.txt relative path as the second argument and that doesn't sound at all surprising.

The APIs probably cannot be changed for compatibility reasons, but it's a good example of something that could be addressed by some new convenience APIs. There's a code in CoreFX (https://github.com/dotnet/corefx/blob/master/src/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.cs) that accomplishes what you want for files:

string fileDestinationPath = Path.GetFullPath(Path.Combine(destinationDirectoryFullPath, source.FullName));
if (!fileDestinationPath.StartsWith(destinationDirectoryFullPath, PathInternal.StringComparison))
     throw new IOException(SR.IO_ExtractingResultsInOutside);

Something like that could possibly be extracted to some new helper API.

[karelz]

cc @rmkerr @JeremyKuhne

For Uri, you can use Uri(string, UriKind.Relative) if you know what you should be getting, then combine it via Uri(Uri, Uri)

Due to compat we won't be able to change behavior of existing APIs (definitely not on .NET Framework) and in .NET Core we should maintain compat with .NET Framework (for libraries).

I wonder if it would help to add Uri(Uri, string, UriKind) overload and deprecate the Uri(Uri, string) which I agree is somewhat dangerous ...

cc @morganbr @GrabYourPitchforks @bartonjs for the security improvement angle ...

[JeremyKuhne]

The "surprising" behavior of Path.Combine is precisely why I added Path.Join in 2.1.

Docs are still catching up, but Path.Join (and TryJoin, which joins in a provided char buffer) simply combine inputs, ensuring that there is at least one separator between them. Rooting isn't considered.

[karelz]

@JeremyKuhne did you consider obsoleting Path.Combine in some way? (e.g. in ref assemblies, or via Platform Compat Roslyn analyzer)

[JeremyKuhne]

did you consider obsoleting Path.Combine in some way?

Yes. I just opened an issue to track getting docs to point you to the right place. https://github.com/dotnet/docs/issues/7379

In some cases the rooting behavior is desirable, so obsoleting it is a little tricky. The intended behavior is to match what happens when you "cd" through a number of directories. Even that isn't quite right as it doesn't handle volume changes the way a command prompt does (volume context doesn't change for the current location when you change the current directory in another volume). Weirdness aside, some people rely on the current behavior (MSBuild for example).

[filipnavara]

I don't think Path.Join solves the security concern because .. is still a valid relative path. AFAICT there's no API that enforces that the resulting path stays within bounds of the absolute path, which is the real problem security-wise (eg. https://snyk.io/research/zip-slip-vulnerability).

[danmoseley]

@filipnavara given reparse points and their equivalent security decisions cannot confidently be made by examining path strings alone. It is necessary to rely on OS security features such as ACLs.

[filipnavara]

@danmosemsft How does Path.GetFullPath handle reparse points?

My argument is that Path.Combine is not insecure because of treatment of absolute path in second parameter. It is insecure for certain usage because the second parameter can escape from the absolute path in the first parameter. I, personally, do expect that from the API and do not find that surprising at all.

What I find useful though is some kind of API that would allow you to combine two paths where you are still bound by the context of the first path. I don't necessarily intend this to be replacement for ACLs, but more of a defence mechanism. The use case that it is trying to solve is something like extracting a .zip file containing ../autoexec.bat, ../../autoexec.bat, ../../../autoexec.bat or c:\autoexec.bat and inadvertently planting files on system in an unexpected way (substitute autoexec.bat for whatever you want).

If Path.GetFullPath doesn't handle reparse points then you may already have a problem in the current CoreFX code:

string fileDestinationPath = Path.GetFullPath(Path.Combine(destinationDirectoryFullPath, source.FullName));
if (!fileDestinationPath.StartsWith(destinationDirectoryFullPath, PathInternal.StringComparison))
     throw new IOException(SR.IO_ExtractingResultsInOutside);

[MarcusWichelmann]

@filipnavara Even when the behaviour of Path.Combine has a historical background and is justified, the practice has shown, that this method actually gets misunderstood by developers. I've found uses of it that might cause serious security issues in at least three libraries, and I haven't even searched for them. I'm sure there are far more libraries or even web-services affected by this.

I didn't know of Path.Join, but that looks very interesting. IMHO pointing developers to this alternative method or another one that also checks for "../../" would be a good solution, at least for the Path.Combine-related issue.

[JeremyKuhne]

I've added more to the issue I opened above about documenting best practices with handling user/untrusted input paths. There is already an active issue that I can't find at the moment that recommends to never pass a local path to Uri without calling GetFullPath on it first.

[steevcoco]

Er ... I also stumbled onto this "bug"; and I'd like to note that looking around and using URI when you're working with local file paths is a bad direction (you should not be blindly crossing into a URI just to "clean" some directory separator issue). And for sure programmers should lead right to Path if they are trying to build some path.

I had to write helper methods to combine paths:

    public static string PathCombine(
            string rootPath,
            string relativePath,
            bool throwIfRelativePathNull = false,
            bool throwIfRelativePathEmpty = false)
    {
        if (rootPath == null)
            throw new ArgumentNullException(nameof(rootPath));
        return Path.Combine(
                rootPath,
                PathHelper.RemoveLeadingSeparators(
                        relativePath,
                        throwIfRelativePathNull,
                        throwIfRelativePathEmpty));
    }

    public static string RemoveLeadingSeparators(string path, bool throwIfNull = false, bool throwIfEmpty = false)
    {
        if (path == null) {
            if (throwIfNull)
                throw new ArgumentNullException(nameof(path));
            return string.Empty;
        }
        do {
            if (string.IsNullOrEmpty(path)) {
                if (throwIfEmpty)
                    throw new ArgumentException(nameof(path));
                return string.Empty;
            }
            // Typically removing only zero or one character: no StringBuilder is used
            if ((path[0] == Path.DirectorySeparatorChar)
                    || (path[0] == Path.AltDirectorySeparatorChar)
                    || (path[0] == Path.PathSeparator)
                    || (path[0] == Path.VolumeSeparatorChar))
                path = path.Substring(1);
            else
                return path;
        } while (true);
    }

[danmoseley]

Given Path.Join exists is there more work here - should this be closed now.

[JeremyKuhne]

To summarize, to avoid grief with path combining:

1. Use Path.Join over Path.Combine.
2. Never pass a local path to Uri without calling Path.GetFullPath() on it first.