Background Link to heading

During a security review this year I stumbled across some C# that at a glance looked fine. Turns out however it actually contained path traversal which was by design (of the language). After asking some of my friends who are familiar with C# they also went ‘huh thats weird’ so I figured I’d make this and share with you all.

TLDR Link to heading

If you are reviewing C# and come across Path.Combine that uses user input at some stage, it is potentially vulnerable to a niche path traversal where starting any argument with / ignores all previously arguments.

Consider using Path.Join instead. Also pass user input through Path.GetFileName first.

The Post Link to heading

Take the following code as an example and guess at the output.

String base_path = "/home/skelmis/tmp/csharp_post/ConsoleApp1";
String media_root = "media";
String user_input = "cat.png";
String output_file_path = Path.Combine(base_path, media_root, user_input);
Console.WriteLine(Path.GetFullPath(output_file_path));

If you said /home/skelmis/tmp/csharp_post/ConsoleApp1/media/cat.png then you would have been correct. So far this meets expectations and looks like a reasonable use case. Further to that, if we change the user_input variable to ../cat.png then we observe the expected path traversal in the output string: /home/skelmis/tmp/csharp_post/ConsoleApp1/cat.png.

Now imagine the code does some home brew path traversal checks here, let’s say not allowing ../. This can be seen below:

String base_path = "/home/skelmis/tmp/csharp_post/ConsoleApp1";
String media_root = "media";

String user_input = "../cat.png";
if (user_input.Contains("../"))
{
    throw new Exception("Path traversal detected!");
}

String output_file_path = Path.Combine(base_path, media_root, user_input);
Console.WriteLine(Path.GetFullPath(output_file_path));

In this example, the following exception is thrown on poor user input. Unhandled exception. System.Exception: Path traversal detected!. Often this may seem ‘good enough’, but specifically with the Path.Combine method we have a sneaky way as an attacker to still achieve an easy path traversal. All we need to do is make user_input equal to /cat.png. Seriously. This will also remove all the prefixed directories.

Here is the example the code:

String base_path = "/home/skelmis/tmp/csharp_post/ConsoleApp1";
String media_root = "media";

String user_input = "/cat.png";
if (user_input.Contains("../"))
{
    throw new Exception("Path traversal detected!");
}

String output_file_path = Path.Combine(base_path, media_root, user_input);
Console.WriteLine(Path.GetFullPath(output_file_path));

And here is the output: /cat.png

Why? Link to heading

Well technically this is explicitly defined behaviour, but at a glance during security reviews we often won’t go read into all of the methods our clients are using. As such this was an aha gotcha moment you may be able to replicate on your next C# review.

Here is an excerpt from the method docstring:

… if an argument other than the first contains a rooted path, any previous path components are ignored, and the returned string begins with that rooted path component.

…

Important: This method assumes that the first argument is an absolute path and that the following argument or arguments are relative paths.

Fix Recommendation Link to heading

  1. All user input for files should be first run through Path.GetFileName
  2. Directory concatenation should be done using Path.Join

Here is our example again:

String base_path = "/home/skelmis/tmp/csharp_post/ConsoleApp1";
String media_root = "/media";
String user_input = Path.GetFileName("../cat.png");
String output_file_path = Path.Join(base_path, media_root, user_input);
Console.WriteLine(Path.GetFullPath(output_file_path));

This now produces the expected output and also doesn’t mind absolute paths as arguments. Output: /home/skelmis/tmp/csharp_post/ConsoleApp1/media/cat.png.