1

How could I solve this problem in that code. I've tried some approaches, but I couldn't pass the checkmarx test (system used to perform the scan)

FinalUploadFolder comes from the WebConfig file, which is where the files are saved

public FileResult Index(string attachedFile)
   {
       string rootPath = System.Configuration.ConfigurationManager.AppSettings.Get("FinalUploadFolder");
       byte[] file= System.IO.File.ReadAllBytes(string.Format(Path.Combine(rootPath, attachedFile.ToString())));
       return File(file, System.Net.Mime.MediaTypeNames.Application.Octet, attachedFile.ToString());         
   }
AllPower
  • 175
  • 1
  • 4
  • 16
  • `attachedFile` might be `\windows\system32\...` or `..\other...`. https://stackoverflow.com/questions/48777564/path-traversal-warning-when-using-path-combine – Jeremy Lakeman Feb 24 '21 at 02:19
  • See also: [How to prevent Path Traversal in .NET](https://blog.mindedsecurity.com/2018/10/how-to-prevent-path-traversal-in-net.html) and [From Path Traversal to Source Code in Asp.NET MVC Applications](https://blog.mindedsecurity.com/2018/10/from-path-traversal-to-source-code-in.html) – Fildor Feb 24 '21 at 16:12

1 Answers1

1

Validating and sanitizing input is a secure coding best practice. There are plenty of "sanitizers" that Checkmarx looks out for and Path.GetFilename is one of them.

Also, I believe the attachedFile is what Checkmarx is more likely concerned at, and it is possible that malicious input could be passed into the parameter. So try to change your code with the following:

public FileResult Index(string attachedFile)
   {
       attachedFile = Path.GetFileName(attachedFile);
       string rootPath = System.Configuration.ConfigurationManager.AppSettings.Get("FinalUploadFolder");
       byte[] file= System.IO.File.ReadAllBytes(string.Format(Path.Combine(rootPath, attachedFile.ToString())));
       return File(file, System.Net.Mime.MediaTypeNames.Application.Octet, attachedFile.ToString());         
   }
 
securecodeninja
  • 2,497
  • 3
  • 16
  • 22