Merge pull request #3486 from h3ku/master

CSHARP: Add experimental query for tainted WebClient

csharp/ql/src/experimental/CWE-099/TaintedWebClient.cs

@@ -0,0 +1,31 @@
+using System;
+using System.IO;
+using System.Web;
+using System.Net;
+
+public class TaintedWebClientHandler : IHttpHandler
+{
+    public void ProcessRequest(HttpContext ctx)
+    {
+        String url = ctx.Request.QueryString["domain"];
+
+        // BAD: This could read any file on the filesystem. (../../../../etc/passwd)
+		using(WebClient client = new WebClient()) {
+			ctx.Response.Write(client.DownloadString(url));
+        }
+
+        // BAD: This could still read any file on the filesystem. (https://../../../../etc/passwd)
+        if (url.StartsWith("https://")){
+            using(WebClient client = new WebClient()) {
+                ctx.Response.Write(client.DownloadString(url));
+            }
+        }
+
+        // GOOD: IsWellFormedUriString ensures that it is a valid URL
+        if (Uri.IsWellFormedUriString(url, UriKind.Absolute)){
+            using(WebClient client = new WebClient()) {
+                ctx.Response.Write(client.DownloadString(url));
+            }
+        }
+    }
+}

csharp/ql/src/experimental/CWE-099/TaintedWebClient.qhelp

@@ -0,0 +1,58 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+<p>The WebClient class provides a variety of methods for data transmission and
+communication with a particular URI. Despite of the class' naming convention,
+the URI scheme can also identify local resources, not only remote ones. Tainted
+by user-supplied input, the URI can be leveraged to access resources available
+on the local file system, therefore leading to the disclosure of sensitive
+information. This can be trivially achieved by supplying path traversal
+sequences (../) followed by an existing directory or file path.</p>
+
+<p>Sanitization of  user-supplied URI values using the
+<code>StartsWith("https://")</code> method is deemed insufficient in preventing
+arbitrary file reads. This is due to the fact that .NET ignores the protocol
+handler (https in this case) in URIs like the following:
+"https://../../../../etc/passwd".</p>
+
+</overview>
+<recommendation>
+
+<p>Validate user input before using it to ensure that is a URI of an external
+resource and not a local one. 
+Potential solutions:</p>
+
+<ul>
+<li>Sanitize potentially tainted paths using
+<code>System.Uri.IsWellFormedUriString</code>.</li>
+</ul>
+
+</recommendation>
+<example>
+
+<p>In the first example, a domain name is read from a <code>HttpRequest</code>
+and then this domain is requested using the method <code>DownloadString</code>.
+However, a malicious user could enter a local path - for example,
+"../../../etc/passwd" instead of a domain. 
+In the second example, it appears that the user is restricted to the HTTPS
+protocol handler. However, a malicious user could still enter a local path,
+since as explained above the protocol handler will be ignored by .net. For
+example, the string "https://../../../etc/passwd" will result in the code
+reading the file located at "/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>
+
+<sample src="TaintedWebClient.cs" />
+
+</example>
+<references>
+
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
+</li>
+
+</references>
+</qhelp>

csharp/ql/src/experimental/CWE-099/TaintedWebClient.ql

@@ -0,0 +1,23 @@
+/**
+ * @name Uncontrolled data used in a WebClient
+ * @description The WebClient class allows developers to request resources,
+ *              accessing resources influenced by users can allow an attacker to access local files.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id cs/webclient-path-injection
+ * @tags security
+ *       external/cwe/cwe-099
+ *       external/cwe/cwe-023
+ *       external/cwe/cwe-036
+ *       external/cwe/cwe-073
+ */
+
+import csharp
+import TaintedWebClientLib
+import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph
+
+from TaintTrackingConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink
+where c.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "$@ flows to here and is used in a method of WebClient.",
+  source.getNode(), "User-provided value"

csharp/ql/src/experimental/CWE-099/TaintedWebClientLib.qll

@@ -0,0 +1,82 @@
+import csharp
+import semmle.code.csharp.frameworks.system.Net
+import semmle.code.csharp.frameworks.System
+import semmle.code.csharp.security.dataflow.flowsources.Remote
+import semmle.code.csharp.security.Sanitizers
+
+//If this leaves experimental this should probably go in semmle.code.csharp.frameworks.system.Net
+/** The `System.Net.WebClient` class. */
+class SystemNetWebClientClass extends SystemNetClass {
+  SystemNetWebClientClass() { this.hasName("WebClient") }
+
+  /** Gets the `DownloadString` method. */
+  Method getDownloadStringMethod() { result = this.getAMethod("DownloadString") }
+}
+
+//If this leaves experimental this should probably go in semmle.code.csharp.frameworks.System
+//Extend the already existent SystemUriClass to not touch the stdlib.
+/** The `System.Uri` class. */
+class SystemUriClassExtra extends SystemUriClass {
+  /** Gets the `IsWellFormedUriString` method. */
+  Method getIsWellFormedUriStringMethod() { result = this.getAMethod("IsWellFormedUriString") }
+}
+
+//If this leaves experimental this should probably go in semmle.code.csharp.frameworks.system
+/**
+ * A data flow source for uncontrolled data in path expression vulnerabilities.
+ */
+abstract class Source extends DataFlow::Node { }
+
+/**
+ * A data flow sink for uncontrolled data in path expression vulnerabilities.
+ */
+abstract class Sink extends DataFlow::ExprNode { }
+
+/**
+ * A sanitizer for uncontrolled data in path expression vulnerabilities.
+ */
+abstract class Sanitizer extends DataFlow::ExprNode { }
+
+/**
+ * A taint-tracking configuration for uncontrolled data in path expression vulnerabilities.
+ */
+class TaintTrackingConfiguration extends TaintTracking::Configuration {
+  TaintTrackingConfiguration() { this = "TaintedWebClientLib" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+}
+
+/** A source of remote user input. */
+class RemoteSource extends Source {
+  RemoteSource() { this instanceof RemoteFlowSource }
+}
+
+/**
+ * A path argument to a `WebClient` method call that has an address argument.
+ */
+class WebClientSink extends Sink {
+  WebClientSink() {
+    exists(Method m | m = any(SystemNetWebClientClass f).getAMethod() |
+      this.getExpr() = m.getACall().getArgumentForName("address")
+    )
+  }
+}
+
+/**
+ * A call to `System.Uri.IsWellFormedUriString` that is considered to sanitize the input.
+ */
+class RequestMapPathSanitizer extends Sanitizer {
+  RequestMapPathSanitizer() {
+    exists(Method m | m = any(SystemUriClassExtra uri).getIsWellFormedUriStringMethod() |
+      this.getExpr() = m.getACall().getArgument(0)
+    )
+  }
+}
+
+private class SimpleTypeSanitizer extends Sanitizer, SimpleTypeSanitizedExpr { }
+
+private class GuidSanitizer extends Sanitizer, GuidSanitizedExpr { }