Skip to content

Commit a022086

Browse files
h3kuh3ku
committed
Add experimental query for Tainted WebClient
1 parent b08de6c commit a022086

File tree

4 files changed

+186
-0
lines changed

4 files changed

+186
-0
lines changed

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
using System;
2+
using System.IO;
3+
using System.Web;
4+
using System.Net;
5+
6+
public class TaintedWebClientHandler : IHttpHandler
7+
{
8+
public void ProcessRequest(HttpContext ctx)
9+
{
10+
String url = ctx.Request.QueryString["domain"];
11+
12+
// BAD: This could read any file on the filesystem. (../../../../etc/passwd)
13+
using(WebClient client = new WebClient()) {
14+
ctx.Response.Write(client.DownloadString(url));
15+
}
16+
17+
// BAD: This could still read any file on the filesystem. (https://../../../../etc/passwd)
18+
if (url.StartsWith("https://")){
19+
using(WebClient client = new WebClient()) {
20+
ctx.Response.Write(client.DownloadString(url));
21+
}
22+
}
23+
24+
// GOOD: IsWellFormedUriString ensures that it is a valid URL
25+
if (Uri.IsWellFormedUriString(url, UriKind.Absolute)){
26+
using(WebClient client = new WebClient()) {
27+
ctx.Response.Write(client.DownloadString(url));
28+
}
29+
}
30+
}
31+
}

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>The WebClient class provices common methods for sending data to and receiving data from a resource identified by a URI.
7+
Even that the name of the class is WebClient the support is not only limited to WebResources but also local resources. This
8+
can result in sensitive information being revealed.</p>
9+
10+
<p>URIs that are naively constructed from data controlled by a user may contain local paths with unexpected special characters,
11+
such as "..". Such a path may potentially point to any directory on the file system.</p>
12+
13+
</overview>
14+
<recommendation>
15+
16+
<p>Validate user input before using it to ensure that is a URI of an external resource and not a local one.
17+
Pontetial solutions:</p>
18+
19+
<ul>
20+
<li>Sanitize potentially tainted paths using <code>System.Uri.IsWellFormedUriString</code>.</li>
21+
</ul>
22+
23+
</recommendation>
24+
<example>
25+
26+
<p>In the first example, a domain name is read from a <code>HttpRequest</code> and then used to request this domain. However, a
27+
malicious user could enter a local path - for example, "../../../etc/passwd". In the second example, it
28+
appears that user is restricted to the HTTPS protocol handler. However, a malicious user could
29+
still enter a local path. For example, the string "../../../etc/passwd" will result in the code
30+
reading the file located at "/etc/passwd", which is the system's password file. This file would then be
31+
sent back to the user, giving them access to all the system's passwords.</p>
32+
33+
<sample src="TaintedWebClient.cs" />
34+
35+
</example>
36+
<references>
37+
38+
<li>
39+
OWASP:
40+
<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
41+
</li>
42+
<li>
43+
CWE-099:
44+
<a href="https://cwe.mitre.org/data/definitions/99.html">Resource Injection</a>.
45+
</li>
46+
47+
</references>
48+
</qhelp>

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/**
2+
* @name Uncontrolled data used in a WebClient
3+
* @description The WebClient class allow developers to request resources,
4+
* accessing resources influenced by users can allow an attacker to access local files.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id cs/webclient-path-injection
9+
* @tags security
10+
* external/cwe/cwe-099
11+
* external/cwe/cwe-022
12+
* external/cwe/cwe-023
13+
* external/cwe/cwe-036
14+
* external/cwe/cwe-073
15+
* external/cwe/cwe-022
16+
*/
17+
18+
import csharp
19+
import TaintedWebClientLib
20+
import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph
21+
22+
from TaintTrackingConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink
23+
where c.hasFlowPath(source, sink)
24+
select sink.getNode(), source, sink, "$@ flows to here and is used in a method of WebClient.",
25+
source.getNode(), "User-provided value"

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

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import csharp
2+
import semmle.code.csharp.frameworks.system.Net
3+
import semmle.code.csharp.frameworks.System
4+
import semmle.code.csharp.security.dataflow.flowsources.Remote
5+
import semmle.code.csharp.security.Sanitizers
6+
7+
//If this leaves experimental this should probably go in semmle.code.csharp.frameworks.system.Net
8+
/** The `System.Net.WebClient` class. */
9+
class SystemNetWebClientClass extends SystemNetClass {
10+
SystemNetWebClientClass() { this.hasName("WebClient") }
11+
12+
/** Gets the `DownloadString` method. */
13+
Method getDownloadStringMethod() { result = this.getAMethod("DownloadString") }
14+
}
15+
16+
//If this leaves experimental this should probably go in semmle.code.csharp.frameworks.System
17+
//Extend the already existent SystemUriClass to not touch the stdlib.
18+
/** The `System.Uri` class. */
19+
class SystemUriClassExtra extends SystemUriClass {
20+
/** Gets the `DownloadString` method. */
21+
Method getIsWellFormedUriStringMethod() { result = this.getAMethod("IsWellFormedUriString") }
22+
}
23+
24+
//If this leaves experimental this should probably go in semmle.code.csharp.frameworks.system
25+
/**
26+
* A data flow source for uncontrolled data in path expression vulnerabilities.
27+
*/
28+
abstract class Source extends DataFlow::Node { }
29+
30+
/**
31+
* A data flow sink for uncontrolled data in path expression vulnerabilities.
32+
*/
33+
abstract class Sink extends DataFlow::ExprNode { }
34+
35+
/**
36+
* A sanitizer for uncontrolled data in path expression vulnerabilities.
37+
*/
38+
abstract class Sanitizer extends DataFlow::ExprNode { }
39+
40+
/**
41+
* A taint-tracking configuration for uncontrolled data in path expression vulnerabilities.
42+
*/
43+
class TaintTrackingConfiguration extends TaintTracking::Configuration {
44+
TaintTrackingConfiguration() { this = "TaintedPath" }
45+
46+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
47+
48+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
49+
50+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
51+
}
52+
53+
/** A source of remote user input. */
54+
class RemoteSource extends Source {
55+
RemoteSource() { this instanceof RemoteFlowSource }
56+
}
57+
58+
/**
59+
* A path argument to a `WebClient` method call that have an address argument.
60+
*/
61+
class WebClientSink extends Sink {
62+
WebClientSink() {
63+
exists(Method m | m = any(SystemNetWebClientClass f).getAMethod() |
64+
this.getExpr() = m.getACall().getArgumentForName("address")
65+
)
66+
}
67+
}
68+
69+
/**
70+
* A call to `System.Uri.IsWellFormedUriString` that is considered to sanitize the input.
71+
*/
72+
class RequestMapPathSanitizer extends Sanitizer {
73+
RequestMapPathSanitizer() {
74+
exists(Method m | m = any(SystemUriClassExtra uri).getIsWellFormedUriStringMethod() |
75+
this.getExpr() = m.getACall().getArgument(0)
76+
)
77+
}
78+
}
79+
80+
private class SimpleTypeSanitizer extends Sanitizer, SimpleTypeSanitizedExpr { }
81+
82+
private class GuidSanitizer extends Sanitizer, GuidSanitizedExpr { }

0 commit comments

Comments
 (0)