Review web endpoints building file paths from their parameters

Rule ND3129 Security

Matches public actions of MVC controllers, API controllers and Razor page models that take a string parameter, build a file path with it (Path.Combine(), concatenation, interpolation) and then read or write the file system, without calling Path.GetFileName(), Path.GetFullPath() or Path.IsPathRooted() first.

A segment such as ..\..\appsettings.json or an absolute path then escapes the intended directory: path traversal. Note that Path.Combine() makes this worse, since it discards everything before an absolute second argument.

This is CWE-22 Improper Limitation of a Pathname to a Restricted Directory (OWASP Top 10 A01 Broken Access Control).

Flagged Code
public class DownloadController : ControllerBase {
   [HttpGet("download")]
   public IActionResult Download(string fileName) {
      var path = Path.Combine(_root, fileName);  // flagged: "..\..\web.config" escapes _root
      return File(System.IO.File.ReadAllBytes(path), "application/octet-stream");
   }
}
Fixed Code
public class DownloadController : ControllerBase {
   [HttpGet("download")]
   public IActionResult Download(string fileName) {
      var safeName = Path.GetFileName(fileName);  // drops any directory segment
      var path = Path.Combine(_root, safeName);
      return File(System.IO.File.ReadAllBytes(path), "application/octet-stream");
   }
}

How to fix

Reduce the caller's segment to a file name with Path.GetFileName(), or resolve the combined path with Path.GetFullPath() and verify it still starts with the intended root directory before touching it. Better still, map the caller's value to an identifier (a Guid, a database row) and build the path from trusted data only.

Suppress the issue when the string parameter cannot reach the path, for instance when it only names a format or a culture.

CQLinq source

This rule is written in CQLinq, C# LINQ over the NDepend code model. It ships with every new NDepend project: you can read it, adapt it, or use it as a starting point for your own custom rules.

// <Name>Review web endpoints building file paths from their parameters</Name>
// <Id>ND3129:ReviewWebEndpointsBuildingFilePathsFromTheirParameters</Id>

warnif count > 0

// Third-party types are part of the code model only when the application uses them.
let controllerBaseTypes = ThirdParty.Types.WithFullNameIn(
  "Microsoft.AspNetCore.Mvc.ControllerBase",
  "Microsoft.AspNetCore.Mvc.RazorPages.PageModel",
  "System.Web.Mvc.Controller",
  "System.Web.Http.ApiController").ToArray()
where controllerBaseTypes.Any()

let fileSystemMembers = ThirdParty.Methods.Where(m =>
   m.ParentType.FullName.StartsWithAny(
     "System.IO.File", "System.IO.Directory", "System.IO.FileStream",
     "System.IO.FileInfo", "System.IO.DirectoryInfo", "System.IO.StreamReader")).ToHashSetEx()

// The path is assembled rather than picked from a closed set of known paths.
let pathBuildingMembers = ThirdParty.Methods.Where(m =>
   (m.ParentType.FullName == "System.IO.Path" && m.SimpleNameEqualsAny("Combine", "Join")) ||
   (m.ParentType.FullName == "System.String" && m.SimpleNameEqualsAny("Concat", "Format")) ||
    m.ParentType.FullName == "System.Runtime.CompilerServices.DefaultInterpolatedStringHandler").ToHashSetEx()

// Calling any of these means the segment is being reduced or resolved before use.
let pathSanitizingMembers = ThirdParty.Methods.Where(m =>
    m.ParentType.FullName == "System.IO.Path" &&
    m.SimpleNameEqualsAny("GetFileName", "GetFullPath", "IsPathRooted", "IsPathFullyQualified",
                           "GetInvalidFileNameChars", "GetInvalidPathChars")).ToHashSetEx()

from t in Application.Types.ThatDeriveFromAny(controllerBaseTypes)
where !t.IsGeneratedByCompiler

from m in t.Methods
where m.IsPublic && !m.IsConstructor && !m.IsGeneratedByCompiler &&

      // The endpoint receives a string from the caller...
      m.Name.Contains("String") &&

      // ...assembles a path with it and reaches the file system...
      m.MethodsCalled.Intersect(pathBuildingMembers).Any() &&
      m.MethodsCalled.Intersect(fileSystemMembers).Any() &&

      // ...without reducing the segment first.
     !m.MethodsCalled.Intersect(pathSanitizingMembers).Any()

select new {
  m,
  fileSystemMembersUsed = m.MethodsCalled.Intersect(fileSystemMembers).ToArray(),
  Debt = 20.ToMinutes().ToDebt(),
  Severity = Severity.High
}

// <Desc>
// Matches public actions of MVC controllers, API controllers and Razor
// page models that take a string parameter, build a file path with it
// (*Path.Combine()*, concatenation, interpolation) and then read or write
// the file system, without calling *Path.GetFileName()*,
// *Path.GetFullPath()* or *Path.IsPathRooted()* first.
//
// A segment such as *..\..\appsettings.json* or an absolute path then
// escapes the intended directory: **path traversal**. Note that
// *Path.Combine()* makes this worse, since it discards everything before
// an absolute second argument.
//
// This is CWE-22 *Improper Limitation of a Pathname to a Restricted
// Directory* (OWASP Top 10 A01 Broken Access Control).
//
// **Flagged Code**
// ```csharp
// public class DownloadController : ControllerBase {
//    [HttpGet("download")]
//    public IActionResult Download(string fileName) {
//       var path = Path.Combine(_root, fileName);  // flagged: "..\..\web.config" escapes _root
//       return File(System.IO.File.ReadAllBytes(path), "application/octet-stream");
//    }
// }
// ```
//
// **Fixed Code**
// ```csharp
// public class DownloadController : ControllerBase {
//    [HttpGet("download")]
//    public IActionResult Download(string fileName) {
//       var safeName = Path.GetFileName(fileName);  // drops any directory segment
//       var path = Path.Combine(_root, safeName);
//       return File(System.IO.File.ReadAllBytes(path), "application/octet-stream");
//    }
// }
// ```
// </Desc>

//<Expl>
// The web endpoint *{0}* builds a file path from a string it receives and
// calls *{1}*, without reducing the segment first. A caller sending
// *..\..\* or an absolute path can reach files outside the intended directory.
//</Expl>

// <HowToFix>
// Reduce the caller's segment to a file name with *Path.GetFileName()*, or
// resolve the combined path with *Path.GetFullPath()* and verify it still
// starts with the intended root directory before touching it. Better
// still, map the caller's value to an identifier (a Guid, a database row)
// and build the path from trusted data only.
//
// Suppress the issue when the string parameter cannot reach the path, for
// instance when it only names a format or a culture.
// </HowToFix>

Open in the interactive Rules Explorer · All default rules

Security: all 39 items