Skip to content

Commit 18b0806

Browse files
tamasvajktamasvajk
authored
Merge pull request #5110 from porcupineyhairs/ssrfCsharp
C# : Add query to detect SSRF
2 parents 5d7b09a + f70d808 commit 18b0806

File tree

7 files changed

+388
-0
lines changed

7 files changed

+388
-0
lines changed

‎csharp/ql/src/experimental/CWE-918/RequestForgery.cs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
namespace RequestForgery.Controllers
2+
{
3+
public class SSRFController : Controller
4+
{
5+
[HttpPost]
6+
[ValidateAntiForgeryToken]
7+
public async Task<ActionResult> Bad(string url)
8+
{
9+
var request = new HttpRequestMessage(HttpMethod.Get, url);
10+
11+
var client = new HttpClient();
12+
await client.SendAsync(request);
13+
14+
return View();
15+
}
16+
17+
[HttpPost]
18+
[ValidateAntiForgeryToken]
19+
public async Task<ActionResult> Good(string url)
20+
{
21+
string baseUrl = "www.mysecuresite.com/";
22+
if (url.StartsWith(baseUrl))
23+
{
24+
var request = new HttpRequestMessage(HttpMethod.Get, url);
25+
var client = new HttpClient();
26+
await client.SendAsync(request);
27+
28+
}
29+
30+
return View();
31+
}
32+
}
33+
}

‎csharp/ql/src/experimental/CWE-918/RequestForgery.qhelp

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
5+
<overview>
6+
<p>Directly incorporating user input into a HTTP request without validating the input
7+
can facilitate Server Side Request Forgery (SSRF) attacks. In these attacks, the server
8+
may be tricked into making a request and interacting with an attacker-controlled server.
9+
</p>
10+
11+
</overview>
12+
<recommendation>
13+
14+
<p>To guard against SSRF attacks, it is advisable to avoid putting user input
15+
directly into the request URL. Instead, maintain a list of authorized
16+
URLs on the server; then choose from that list based on the user input provided.</p>
17+
18+
</recommendation>
19+
<example>
20+
21+
<p>The following example shows an HTTP request parameter being used directly in a forming a
22+
new request without validating the input, which facilitates SSRF attacks.
23+
It also shows how to remedy the problem by validating the user input against a known fixed string.
24+
</p>
25+
26+
<sample src="RequestForgery.cs" />
27+
28+
</example>
29+
<references>
30+
<li>
31+
<a href="https://owasp.org/www-community/attacks/Server_Side_Request_Forgery">OWASP SSRF</a>
32+
</li>
33+
34+
</references>
35+
</qhelp>

‎csharp/ql/src/experimental/CWE-918/RequestForgery.ql

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
/**
2+
* @name Uncontrolled data used in network request
3+
* @description Sending network requests with user-controlled data allows for request forgery attacks.
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @precision high
7+
* @id cs/request-forgery
8+
* @tags security
9+
* external/cwe/cwe-918
10+
*/
11+
12+
import csharp
13+
import RequestForgery::RequestForgery
14+
import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph
15+
16+
from RequestForgeryConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink
17+
where c.hasFlowPath(source, sink)
18+
select sink.getNode(), source, sink, "$@ flows to here and is used in a server side web request.",
19+
source.getNode(), "User-provided value"

‎csharp/ql/src/experimental/CWE-918/RequestForgery.qll

Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
import csharp
2+
3+
module RequestForgery {
4+
import semmle.code.csharp.controlflow.Guards
5+
import semmle.code.csharp.frameworks.System
6+
import semmle.code.csharp.frameworks.system.Web
7+
import semmle.code.csharp.frameworks.Format
8+
import semmle.code.csharp.security.dataflow.flowsources.Remote
9+
10+
/**
11+
* A data flow source for server side request forgery vulnerabilities.
12+
*/
13+
abstract private class Source extends DataFlow::Node { }
14+
15+
/**
16+
* A data flow sink for server side request forgery vulnerabilities.
17+
*/
18+
abstract private class Sink extends DataFlow::ExprNode { }
19+
20+
/**
21+
* A data flow BarrierGuard which blocks the flow of taint for
22+
* server side request forgery vulnerabilities.
23+
*/
24+
abstract private class BarrierGuard extends DataFlow::BarrierGuard { }
25+
26+
/**
27+
* A data flow configuration for detecting server side request forgery vulnerabilities.
28+
*/
29+
class RequestForgeryConfiguration extends DataFlow::Configuration {
30+
RequestForgeryConfiguration() { this = "Server Side Request forgery" }
31+
32+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
33+
34+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
35+
36+
override predicate isAdditionalFlowStep(DataFlow::Node prev, DataFlow::Node succ) {
37+
interpolatedStringFlowStep(prev, succ)
38+
or
39+
stringReplaceStep(prev, succ)
40+
or
41+
uriCreationStep(prev, succ)
42+
or
43+
formatConvertStep(prev, succ)
44+
or
45+
toStringStep(prev, succ)
46+
or
47+
stringConcatStep(prev, succ)
48+
or
49+
stringFormatStep(prev, succ)
50+
or
51+
pathCombineStep(prev, succ)
52+
}
53+
54+
override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
55+
guard instanceof BarrierGuard
56+
}
57+
}
58+
59+
/**
60+
* A remote data flow source taken as a source
61+
* for Server Side Request Forgery(SSRF) Vulnerabilities.
62+
*/
63+
private class RemoteFlowSourceAsSource extends Source {
64+
RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
65+
}
66+
67+
/**
68+
* An url argument to a `HttpRequestMessage` constructor call
69+
* taken as a sink for Server Side Request Forgery(SSRF) Vulnerabilities.
70+
*/
71+
private class SystemWebHttpRequestMessageSink extends Sink {
72+
SystemWebHttpRequestMessageSink() {
73+
exists(Class c | c.hasQualifiedName("System.Net.Http.HttpRequestMessage") |
74+
c.getAConstructor().getACall().getArgument(1) = this.asExpr()
75+
)
76+
}
77+
}
78+
79+
/**
80+
* An argument to a `WebRequest.Create` call taken as a
81+
* sink for Server Side Request Forgery(SSRF) Vulnerabilities. *
82+
*/
83+
private class SystemNetWebRequestCreateSink extends Sink {
84+
SystemNetWebRequestCreateSink() {
85+
exists(Method m |
86+
m.getDeclaringType().hasQualifiedName("System.Net.WebRequest") and m.hasName("Create")
87+
|
88+
m.getACall().getArgument(0) = this.asExpr()
89+
)
90+
}
91+
}
92+
93+
/**
94+
* An argument to a new HTTP Request call of a `System.Net.Http.HttpClient` object
95+
* taken as a sink for Server Side Request Forgery(SSRF) Vulnerabilities.
96+
*/
97+
private class SystemNetHttpClientSink extends Sink {
98+
SystemNetHttpClientSink() {
99+
exists(Method m |
100+
m.getDeclaringType().hasQualifiedName("System.Net.Http.HttpClient") and
101+
m.hasName([
102+
"DeleteAsync", "GetAsync", "GetByteArrayAsync", "GetStreamAsync", "GetStringAsync",
103+
"PatchAsync", "PostAsync", "PutAsync"
104+
])
105+
|
106+
m.getACall().getArgument(0) = this.asExpr()
107+
)
108+
}
109+
}
110+
111+
/**
112+
* An url argument to a method call of a `System.Net.WebClient` object
113+
* taken as a sink for Server Side Request Forgery(SSRF) Vulnerabilities.
114+
*/
115+
private class SystemNetClientBaseAddressSink extends Sink {
116+
SystemNetClientBaseAddressSink() {
117+
exists(Property p |
118+
p.hasName("BaseAddress") and
119+
p.getDeclaringType()
120+
.hasQualifiedName(["System.Net.WebClient", "System.Net.Http.HttpClient"])
121+
|
122+
p.getAnAssignedValue() = this.asExpr()
123+
)
124+
}
125+
}
126+
127+
/**
128+
* A method call which checks the base of the tainted uri is assumed
129+
* to be a guard for Server Side Request Forgery(SSRF) Vulnerabilities.
130+
* This guard considers all checks as valid.
131+
*/
132+
private class BaseUriGuard extends BarrierGuard, MethodCall {
133+
BaseUriGuard() { this.getTarget().hasQualifiedName("System.Uri", "IsBaseOf") }
134+
135+
override predicate checks(Expr e, AbstractValue v) {
136+
// we consider any checks against the tainted value to sainitize the taint.
137+
// This implies any check such as shown below block the taint flow.
138+
// Uri url = new Uri("whitelist.com")
139+
// if (url.isBaseOf(`taint1))
140+
(e = this.getArgument(0) or e = this.getQualifier()) and
141+
v.(AbstractValues::BooleanValue).getValue() = true
142+
}
143+
}
144+
145+
/**
146+
* A method call which checks if the Uri starts with a white-listed string is assumed
147+
* to be a guard for Server Side Request Forgery(SSRF) Vulnerabilities.
148+
* This guard considers all checks as valid.
149+
*/
150+
private class StringStartsWithBarrierGuard extends BarrierGuard, MethodCall {
151+
StringStartsWithBarrierGuard() {
152+
this.getTarget().hasQualifiedName("System.String", "StartsWith")
153+
}
154+
155+
override predicate checks(Expr e, AbstractValue v) {
156+
// Any check such as the ones shown below
157+
// "https://myurl.com/".startsWith(`taint`)
158+
// `taint`.startsWith("https://myurl.com/")
159+
// are assumed to sainitize the taint
160+
(e = this.getQualifier() or this.getArgument(0) = e) and
161+
v.(AbstractValues::BooleanValue).getValue() = true
162+
}
163+
}
164+
165+
private predicate stringFormatStep(DataFlow::Node prev, DataFlow::Node succ) {
166+
exists(FormatCall c | c.getArgument(0) = prev.asExpr() and c = succ.asExpr())
167+
}
168+
169+
private predicate pathCombineStep(DataFlow::Node prev, DataFlow::Node succ) {
170+
exists(MethodCall combineCall |
171+
combineCall.getTarget().hasQualifiedName("System.IO.Path", "Combine") and
172+
combineCall.getArgument(0) = prev.asExpr() and
173+
combineCall = succ.asExpr()
174+
)
175+
}
176+
177+
private predicate uriCreationStep(DataFlow::Node prev, DataFlow::Node succ) {
178+
exists(ObjectCreation oc |
179+
oc.getTarget().getDeclaringType().hasQualifiedName("System.Uri") and
180+
oc.getArgument(0) = prev.asExpr() and
181+
oc = succ.asExpr()
182+
)
183+
}
184+
185+
private predicate interpolatedStringFlowStep(DataFlow::Node prev, DataFlow::Node succ) {
186+
exists(InterpolatedStringExpr i |
187+
// allow `$"http://{`taint`}/blabla/");"` or
188+
// allow `$"https://{`taint`}/blabla/");"`
189+
i.getText(0).getValue().matches(["http://", "http://"]) and
190+
i.getInsert(1) = prev.asExpr() and
191+
succ.asExpr() = i
192+
or
193+
// allow `$"{`taint`}/blabla/");"`
194+
i.getInsert(0) = prev.asExpr() and
195+
succ.asExpr() = i
196+
)
197+
}
198+
199+
private predicate stringReplaceStep(DataFlow::Node prev, DataFlow::Node succ) {
200+
exists(MethodCall mc, SystemStringClass s |
201+
mc = s.getReplaceMethod().getACall() and
202+
mc.getQualifier() = prev.asExpr() and
203+
succ.asExpr() = mc
204+
)
205+
}
206+
207+
private predicate stringConcatStep(DataFlow::Node prev, DataFlow::Node succ) {
208+
exists(AddExpr a |
209+
a.getLeftOperand() = prev.asExpr()
210+
or
211+
a.getRightOperand() = prev.asExpr() and
212+
a.getLeftOperand().(StringLiteral).getValue() = ["http://", "https://"]
213+
|
214+
a = succ.asExpr()
215+
)
216+
}
217+
218+
private predicate formatConvertStep(DataFlow::Node prev, DataFlow::Node succ) {
219+
exists(Method m |
220+
m.hasQualifiedName("System.Convert",
221+
["FromBase64String", "FromHexString", "FromBase64CharArray"]) and
222+
m.getParameter(0) = prev.asParameter() and
223+
succ.asExpr() = m.getACall()
224+
)
225+
}
226+
227+
private predicate toStringStep(DataFlow::Node prev, DataFlow::Node succ) {
228+
exists(MethodCall ma |
229+
ma.getTarget().hasName("ToString") and
230+
ma.getQualifier() = prev.asExpr() and
231+
succ.asExpr() = ma
232+
)
233+
}
234+
}

‎csharp/ql/test/experimental/CWE-918/RequestForgery.cs

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// semmle-extractor-options: ${testdir}/../../resources/stubs/System.Web.cs /r:System.Threading.Tasks.dll /r:System.Collections.Specialized.dll /r:System.Runtime.dll /r:System.Private.Uri.dll
2+
3+
using System;
4+
using System.Threading.Tasks;
5+
using System.Web.Mvc;
6+
using System.Net.Http;
7+
8+
namespace RequestForgery.Controllers
9+
{
10+
public class SSRFController : Controller
11+
{
12+
[HttpPost]
13+
[ValidateAntiForgeryToken]
14+
public async Task<ActionResult> Bad(string url)
15+
{
16+
var request = new HttpRequestMessage(HttpMethod.Get, url);
17+
18+
var client = new HttpClient();
19+
await client.SendAsync(request);
20+
21+
return View();
22+
}
23+
24+
[HttpPost]
25+
[ValidateAntiForgeryToken]
26+
public async Task<ActionResult> Good(string url)
27+
{
28+
string baseUrl = "www.mysecuresite.com/";
29+
if (url.StartsWith(baseUrl))
30+
{
31+
var request = new HttpRequestMessage(HttpMethod.Get, url);
32+
var client = new HttpClient();
33+
await client.SendAsync(request);
34+
35+
}
36+
37+
return View();
38+
}
39+
}
40+
}
41+
// Missing stubs:
42+
namespace System.Net.Http
43+
{
44+
public class HttpClient
45+
{
46+
public async Task SendAsync(HttpRequestMessage request) => throw null;
47+
}
48+
49+
public class HttpRequestMessage
50+
{
51+
public HttpRequestMessage(HttpMethod method, string requestUri) => throw null;
52+
}
53+
54+
public class HttpMethod
55+
{
56+
public static readonly HttpMethod Get;
57+
}
58+
}

‎csharp/ql/test/experimental/CWE-918/RequestForgery.expected

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
edges
2+
| RequestForgery.cs:14:52:14:54 | url : String | RequestForgery.cs:16:66:16:68 | access to parameter url |
3+
nodes
4+
| RequestForgery.cs:14:52:14:54 | url : String | semmle.label | url : String |
5+
| RequestForgery.cs:16:66:16:68 | access to parameter url | semmle.label | access to parameter url |
6+
subpaths
7+
#select
8+
| RequestForgery.cs:16:66:16:68 | access to parameter url | RequestForgery.cs:14:52:14:54 | url : String | RequestForgery.cs:16:66:16:68 | access to parameter url | $@ flows to here and is used in a server side web request. | RequestForgery.cs:14:52:14:54 | url | User-provided value |

‎csharp/ql/test/experimental/CWE-918/RequestForgery.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/CWE-918/RequestForgery.ql

0 commit comments

Comments
 (0)