github / github/codeql

Possible false positive "Uncontrolled data used in path expression" still after stripping path from input.

Open
#18,225 0 comments 0 reactions 0 assignees View on GitHub
false-positive
Dominant language
CodeQL
Stars
10.1k
Forks
2.1k
Avg merge
2d 15h
Merged PRs (30d)
141

Description

The .NET `System.IO.Path.GetFileName` will take a path, relative or otherwise, and return only the filename part. I am using this to sanitize the input to a Download action that serves files from a hardcoded folder. The initial code looked like this, which is in fact vulnerable:

```
[HttpGet]
public IActionResult Download(string fileName)
{
string filesDir = "Files\\FilesToServe";
string filePath = Path.Combine(filesDir, fileName);

if (System.IO.File.Exists(filePath))
{
FileStream fs = new FileStream(filePath, FileMode.Open);

return File(fs, "text/plain", fileName);
}

return NotFound();
}
```

Corrective action was taken so that only a file name with no additional path will be used to look for the file:

```
[HttpGet]
public IActionResult Download(string fileName)
{
string strippedFileName = Path.GetFileName(fileName);
string filesDir = "Files\\FilesToServe";
string filePath = Path.Combine(filesDir, strippedFileName);

if (System.IO.File.Exists(filePath))
{
FileStream fs = new FileStream(filePath, FileMode.Open);

return File(fs, "text/plain", strippedFileName);
}

return NotFound();
}
```

This still detects "Uncontrolled data used in path expression (cs/path-injection)" for `filePath` even though it has been tested to no longer serve files outside of `filesDir` when passing in parameters such as "../../../FileThatShouldntBeServed.txt".

Contributor guide

Open the contributing guide

Research direction

Start by reproducing the C# examples in a CodeQL test case and inspect how the cs/path-injection query models Path.GetFileName. Compare results for the original vulnerable code and the stripped-file-name version; done means the sanitized example is not flagged while the original remains detected.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.