Merge pull request github#15519 from erik-krogh/cs-path

C#: Improve the `cs/path-injection` QHelp

Author: erik-krogh

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

@@ -7,34 +7,53 @@
 can result in sensitive information being revealed or deleted, or an attacker being able to influence
 behavior by modifying unexpected files.</p>
 
-<p>Paths that are naively constructed from data controlled by a user may contain unexpected special characters,
-such as "..". Such a path may potentially point to any directory on the file system.</p>
+<p>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.</p>
 
 </overview>
 <recommendation>
 
-<p>Validate user input before using it to construct a file path. Ideally, follow these rules:</p>
+<p>Validate user input before using it to construct a file path.</p>
 
-<ul>
-<li>Do not allow more than a single "." character.</li>
-<li>Do not allow directory separators such as "/" or "\" (depending on the file system).</li>
-<li>Do not rely on simply replacing problematic sequences such as "../". For example, after applying this filter to
-".../...//" the resulting string would still be "../".</li>
-<li>Use a whitelist of known good patterns.</li>
-<li>Sanitize potentially tainted paths using <code>HttpRequest.MapPath</code>.</li>
-</ul>
+<p>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.
+</p>
+
+<p>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.
+</p>
+
+<p>
+Note that removing "../" sequences is <i>not</i> 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.
+</p>
+
+<p>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.</p>
 
 </recommendation>
 <example>
 
-<p>In the first example, a file name is read from a <code>HttpRequest</code> and then used to access a file. However, a
-malicious user could enter a file name which is an absolute path - for example, "/etc/passwd". In the second example, it
-appears that the user is restricted to opening a file within the "user" home directory. However, a malicious user could
-enter a filename which contains special characters. For example, the string "../../etc/passwd" will result in the code
-reading the file located at "/home/[user]/../../etc/passwd", which is the system's password file. This file would then be
-sent back to the user, giving them access to all the system's passwords.</p>
+<p>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".</p>
+
+<sample src="examples/TaintedPath.cs" />
+
+<p>
+If the input should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.
+</p>
+
+<sample src="examples/TaintedPathGoodNormalize.cs" />
+
+<p>
+If the input should be within a specific directory, you can check that the resolved path 
+is still contained within that directory.
+</p>
 
-<sample src="TaintedPath.cs" />
+<sample src="examples/TaintedPathGoodFolder.cs" />
 
 </example>
 <references>

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

@@ -50,7 +50,7 @@ the result is within the destination directory. If provided with a zip file cont
 path like <code>..\sneaky-file</code>, then this file would be written outside the destination
 directory.</p>
 
-<sample src="ZipSlipBad.cs" />
+<sample src="examples/ZipSlipBad.cs" />
 
 <p>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 <code>Path.GetFullPath</code>.
@@ -59,7 +59,7 @@ Secondly, we need to identify the destination output directory, again using
 the resolved output starts with the resolved destination directory, and throw an exception if this
 is not the case.</p>
 
-<sample src="ZipSlipGood.cs" />
+<sample src="examples/ZipSlipGood.cs" />
 
 </example>
 <references>

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

@@ -0,0 +1,13 @@
+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));
+    }
+}

csharp/ql/src/Security Features/CWE-022/examples/TaintedPath.cs

@@ -0,0 +1,25 @@
+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));
+    }
+}

csharp/ql/src/Security Features/CWE-022/examples/TaintedPathGoodFolder.cs

@@ -0,0 +1,20 @@
+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));
+    }
+}