7

Can anyone please confirm, is Path Traversal Vulnerabilities is possible in my below code snippet? if yes then what changes I should make.

[RedirectingAction]
public ActionResult Download(string fileName)
{
    byte[] fileBytes = System.IO.File.ReadAllBytes(Server.MapPath("~/ClientDocument/") + fileName);
    return File(fileBytes, System.Net.Mime.MediaTypeNames.Application.Octet, fileName);
}
Mr Lister
  • 45,515
  • 15
  • 108
  • 150
Kundan Rai
  • 75
  • 1
  • 1
  • 7

2 Answers2

15

Yes, it is vulnerable.

Just to prove it, I set up a new MVC project called WebApplication1.sln

The following request downloads the solution file:

http://localhost:56548/Home/Download?fileName=../../WebApplication1.sln

You can write a naive check:

private static readonly char[] InvalidFilenameChars = Path.GetInvalidFileNameChars();
public ActionResult Download(string fileName)
{
    if (fileName.IndexOfAny(InvalidFilenameChars) >= 0)
        return new HttpStatusCodeResult(HttpStatusCode.BadRequest);

    var rootPath = Server.MapPath("~/ClientDocument/");
    byte[] fileBytes = System.IO.File.ReadAllBytes(Path.Combine(rootPath, fileName));
    return File(fileBytes, System.Net.Mime.MediaTypeNames.Application.Octet, fileName);
}

Which will check that the fileName argument is a valid file name. This excludes directory separator characters, so they cannot pass a path as a filename.

However, the only way to be completely safe, is to restrict the permissions your application has. Only grant it permission to your virtual directory, and nothing else.

Rob
  • 26,989
  • 16
  • 82
  • 98
  • `Path.GetDirectoryName` can also be used to verify that the directory is the one expected. – SilverlightFox May 24 '16 at 09:24
  • 1
    Not quite that vulnerable. This only works because the project runs on a development server, using the developer's credentials. An application pool account doesn't have access outside a site's root by default – Panagiotis Kanavos Oct 27 '17 at 08:42
  • That doesn't mean it's OK though - AccessDenied exceptions cost in CPU, which means they cost in hosting, cloud money. Never mind the time wasted checking error logs. File checks should be performed to reduce log noise, and the cost of servicing bad requests – Panagiotis Kanavos Oct 27 '17 at 08:45
  • @PanagiotisKanavos "By default" is a pretty broad claim, and in fact entirely depends on which web server you're using... Running dotnet core, for example, runs under the user your invoke it as, which very well may have access to files outside of the web directory. – Rob Oct 27 '17 at 08:46
  • @Rob in this case one shouldn't be combining paths with user input anyway. [Accept an ID or other indicator](https://www.filipekberg.se/2013/07/12/are-you-serving-files-insecurely-in-asp-net/) and find the proper path in the code itself – Panagiotis Kanavos Oct 27 '17 at 08:52
3

In concept what you should do to alleviate a Path Traversal vulnerability is to evaluate your basePath to its real path, and likewise do the same to your basePath plus the fileName. If the resulting file of the second operation is still within the folder from your basePath, you know that Path Traversial has not taken place.

I'm using a much later version of .NET so Server.MapPath is not valid. As a result, I'm not sure if this will run for you; but this at least demonstrates how to fix it in concept:

[RedirectingAction]
public ActionResult Download(string fileName)
{
    var baseFolder = Path.GetFullPath(Server.MapPath("~/ClientDocument/"));
    var targetFile = Path.GetFullPath(Path.Combine(baseFolder, fileName));
    if(targetFile.StartsWith(baseFolder){
      byte[] fileBytes = System.IO.File.ReadAllBytes(Server.MapPath("~/ClientDocument/") + fileName);
      return File(fileBytes, System.Net.Mime.MediaTypeNames.Application.Octet, fileName);
    } else {
      //Don't do the download and do something else.
    }  
}
Doug
  • 6,446
  • 9
  • 74
  • 107