C#: Improve the cs/path-injection QHelp

[erik-krogh]

I took the QHelp from Java, and modified it to fit C#.
Everything except the <examples> section is directly copy-pasted from the Java version.

I'm not that comfortable in C#, so the new code-snippets should be thoroughly checked.

The reference to HttpRequest.MapPath was purposely removed, as it seems to be very rarely used in the wild.

See the backref for an evaluation and a bit more context.

[github-actions]

QHelp previews:

csharp/ql/src/Security Features/CWE-022/TaintedPath.qhelp

Uncontrolled data used in path expression

Accessing paths controlled by users can allow an attacker to access unexpected resources. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.

Paths that are naively constructed from data controlled by a user may be absolute paths, or may contain unexpected special characters such as "..". Such a path could point anywhere on the file system.

Recommendation

Validate user input before using it to construct a file path.

Common validation methods include checking that the normalized path is relative and does not contain any ".." components, or checking that the path is contained within a safe folder. The method you should use depends on how the path is used in the application, and whether the path should be a single path component.

If the path should be a single path component (such as a file name), you can check for the existence of any path separators ("/" or "\"), or ".." sequences in the input, and reject the input if any are found.

Note that removing "../" sequences is not sufficient, since the input could still contain a path separator followed by "..". For example, the input ".../...//" would still result in the string "../" if only "../" sequences are removed.

Finally, the simplest (but most restrictive) option is to use an allow list of safe patterns and make sure that the user input matches one of these patterns.

Example

In this example, a user-provided file name is read from a HTTP request and then used to access a file and send it back to the user. However, a malicious user could enter a file name anywhere on the file system, such as "/etc/passwd" or "../../../etc/passwd".

using System;
using System.IO;
using System.Web;

public class TaintedPathHandler : IHttpHandler
{
    public void ProcessRequest(HttpContext ctx)
    {
        string filename = ctx.Request.QueryString["path"];
        // BAD: This could read any file on the filesystem.
        ctx.Response.Write(File.ReadAllText(filename));
    }
}

If the input should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.

using System;
using System.IO;
using System.Web;

public class TaintedPathHandler : IHttpHandler
{
    public void ProcessRequest(HttpContext ctx)
    {
        string filename = ctx.Request.QueryString["path"];
        // GOOD: ensure that the filename has no path separators or parent directory references
        if (filename.Contains("..") || filename.Contains("/") || filename.Contains("\\"))
        {
            ctx.Response.StatusCode = 400;
            ctx.Response.StatusDescription = "Bad Request";
            ctx.Response.Write("Invalid path");
            return;
        }
        ctx.Response.Write(File.ReadAllText(filename));
    }
}

If the input should be within a specific directory, you can check that the resolved path is still contained within that directory.

using System;
using System.IO;
using System.Web;

public class TaintedPathHandler : IHttpHandler
{
    public void ProcessRequest(HttpContext ctx)
    {
        string filename = ctx.Request.QueryString["path"];
        
        string user = ctx.User.Identity.Name;
        string publicFolder = Path.GetFullPath("/home/" + user + "/public");
        string filePath = Path.GetFullPath(Path.Combine(publicFolder, filename));

        // GOOD: ensure that the path stays within the public folder
        if (!filePath.StartsWith(publicFolder + Path.DirectorySeparatorChar))
        {
            ctx.Response.StatusCode = 400;
            ctx.Response.StatusDescription = "Bad Request";
            ctx.Response.Write("Invalid path");
            return;
        }
        ctx.Response.Write(File.ReadAllText(filename));
    }
}

References

• OWASP: Path Traversal.
• Common Weakness Enumeration: CWE-22.
• Common Weakness Enumeration: CWE-23.
• Common Weakness Enumeration: CWE-36.
• Common Weakness Enumeration: CWE-73.
• Common Weakness Enumeration: CWE-99.

csharp/ql/src/Security Features/CWE-022/ZipSlip.qhelp

Arbitrary file access during archive extraction ("Zip Slip")

Extracting files from a malicious zip file, or similar type of archive, is at risk of directory traversal attacks if filenames from the archive are not properly validated.

Zip archives contain archive entries representing each file in the archive. These entries include a file path for the entry, but these file paths are not restricted and may contain unexpected special elements such as the directory traversal element (..). If these file paths are used to create a filesystem path, then a file operation may happen in an unexpected location. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.

For example, if a zip file contains a file entry ..\sneaky-file, and the zip file is extracted to the directory c:\output, then naively combining the paths would result in an output file path of c:\output\..\sneaky-file, which would cause the file to be written to c:\sneaky-file.

Recommendation

Ensure that output paths constructed from zip archive entries are validated to prevent writing files to unexpected locations.

The recommended way of writing an output file from a zip archive entry is to:

1. Use Path.Combine(destinationDirectory, archiveEntry.FullName) to determine the raw output path.
2. Use Path.GetFullPath(..) on the raw output path to resolve any directory traversal elements.
3. Use Path.GetFullPath(destinationDirectory + Path.DirectorySeparatorChar) to determine the fully resolved path of the destination directory.
4. Validate that the resolved output path StartsWith the resolved destination directory, aborting if this is not true.
   Another alternative is to validate archive entries against a whitelist of expected files.

Example

In this example, a file path taken from a zip archive item entry is combined with a destination directory. The result is used as the destination file path without verifying that the result is within the destination directory. If provided with a zip file containing an archive path like ..\sneaky-file, then this file would be written outside the destination directory.

using System.IO;
using System.IO.Compression;

class Bad
{
    public static void WriteToDirectory(ZipArchiveEntry entry,
                                        string destDirectory)
    {
        string destFileName = Path.Combine(destDirectory, entry.FullName);
        entry.ExtractToFile(destFileName);
    }
}

To fix this vulnerability, we need to make three changes. Firstly, we need to resolve any directory traversal or other special characters in the path by using Path.GetFullPath. Secondly, we need to identify the destination output directory, again using Path.GetFullPath, this time on the output directory. Finally, we need to ensure that the resolved output starts with the resolved destination directory, and throw an exception if this is not the case.

using System.IO;
using System.IO.Compression;

class Good
{
    public static void WriteToDirectory(ZipArchiveEntry entry,
                                        string destDirectory)
    {
        string destFileName = Path.GetFullPath(Path.Combine(destDirectory, entry.FullName));
        string fullDestDirPath = Path.GetFullPath(destDirectory + Path.DirectorySeparatorChar);
        if (!destFileName.StartsWith(fullDestDirPath)) {
            throw new System.InvalidOperationException("Entry is outside the target dir: " +
                                                                                 destFileName);
        }
        entry.ExtractToFile(destFileName);
    }
}

References

• Snyk: Zip Slip Vulnerability.
• OWASP: Path Traversal.
• Common Weakness Enumeration: CWE-22.

[michaelnebel]

Suggested change

	String filename = ctx.Request.QueryString["path"];
	string filename = ctx.Request.QueryString["path"];

[michaelnebel]

User is undefined.
Maybe it should be

string user = ctx.User.Identity.Name;

[michaelnebel]

I don't think the query as it is catches this as "good" (It produces an alert).

[erik-krogh]

I think you're right. I've seen some false positives that shouldn't have been flagged.

But that is a problem with the query, and that should probably be fixed.

[michaelnebel]

Yes, but then giving the query an overhaul will block the update of the documentation.
I will open an issue.

[michaelnebel]

Wait, isn't this a true postive? We haven't checked that filename doesn't contain e.g. ../ which could remove the "public" folder part when normalized. Did you find the example somewhere specific or did you construct it from scratch?

[erik-krogh]

Wait, isn't this a true postive?

No.
Path.GetFullPath normalizes the path so filePath cannot contain ../ elements.

I constructed this example based on the examples from java/path-injection.
Other languages also have similar examples, because sometimes this pattern is the correct fix.

[michaelnebel]

Right. I will open an issue for this.

[michaelnebel]

Suggested change

	String filename = ctx.Request.QueryString["path"];
	string filename = ctx.Request.QueryString["path"];

[michaelnebel]

LGTM!