Skip to content

Commit 9708073

Browse files
hvitvedhvitved
authored
Merge pull request github#3486 from h3ku/master
CSHARP: Add experimental query for tainted WebClient
2 parents 28c2aca + 66d77a4 commit 9708073

File tree

4 files changed

+194
-0
lines changed

4 files changed

+194
-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: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>The WebClient class provides a variety of methods for data transmission and
7+
communication with a particular URI. Despite of the class' naming convention,
8+
the URI scheme can also identify local resources, not only remote ones. Tainted
9+
by user-supplied input, the URI can be leveraged to access resources available
10+
on the local file system, therefore leading to the disclosure of sensitive
11+
information. This can be trivially achieved by supplying path traversal
12+
sequences (../) followed by an existing directory or file path.</p>
13+
14+
<p>Sanitization of user-supplied URI values using the
15+
<code>StartsWith("https://")</code> method is deemed insufficient in preventing
16+
arbitrary file reads. This is due to the fact that .NET ignores the protocol
17+
handler (https in this case) in URIs like the following:
18+
"https://../../../../etc/passwd".</p>
19+
20+
</overview>
21+
<recommendation>
22+
23+
<p>Validate user input before using it to ensure that is a URI of an external
24+
resource and not a local one.
25+
Potential solutions:</p>
26+
27+
<ul>
28+
<li>Sanitize potentially tainted paths using
29+
<code>System.Uri.IsWellFormedUriString</code>.</li>
30+
</ul>
31+
32+
</recommendation>
33+
<example>
34+
35+
<p>In the first example, a domain name is read from a <code>HttpRequest</code>
36+
and then this domain is requested using the method <code>DownloadString</code>.
37+
However, a malicious user could enter a local path - for example,
38+
"../../../etc/passwd" instead of a domain.
39+
In the second example, it appears that the user is restricted to the HTTPS
40+
protocol handler. However, a malicious user could still enter a local path,
41+
since as explained above the protocol handler will be ignored by .net. For
42+
example, the string "https://../../../etc/passwd" will result in the code
43+
reading the file located at "/etc/passwd", which is the system's password file.
44+
This file would then be sent back to the user, giving them access to all the
45+
system's passwords.</p>
46+
47+
<sample src="TaintedWebClient.cs" />
48+
49+
</example>
50+
<references>
51+
52+
<li>
53+
OWASP:
54+
<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
55+
</li>
56+
57+
</references>
58+
</qhelp>

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Uncontrolled data used in a WebClient
3+
* @description The WebClient class allows 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-023
12+
* external/cwe/cwe-036
13+
* external/cwe/cwe-073
14+
*/
15+
16+
import csharp
17+
import TaintedWebClientLib
18+
import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph
19+
20+
from TaintTrackingConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink
21+
where c.hasFlowPath(source, sink)
22+
select sink.getNode(), source, sink, "$@ flows to here and is used in a method of WebClient.",
23+
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 `IsWellFormedUriString` 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 = "TaintedWebClientLib" }
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 has 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)