Skip to content

Commit 7c05572

Browse files
erik-krogherik-krogh
authored
Merge pull request github#15596 from erik-krogh/url-san
C#: Add a few more sanitizers to `cs/web/unvalidated-url-redirection`
2 parents 0643184 + 7c2465e commit 7c05572

File tree

4 files changed

+115
-0
lines changed

4 files changed

+115
-0
lines changed

‎csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,79 @@ class LocalUrlSanitizer extends Sanitizer {
139139
LocalUrlSanitizer() { this = DataFlow::BarrierGuard<isLocalUrlSanitizer/3>::getABarrierNode() }
140140
}
141141

142+
/**
143+
* An argument to a call to `List.Contains()` that is a sanitizer for URL redirects.
144+
*/
145+
private predicate isContainsUrlSanitizer(Guard guard, Expr e, AbstractValue v) {
146+
guard =
147+
any(MethodCall method |
148+
exists(Method m | m = method.getTarget() |
149+
m.hasName("Contains") and
150+
e = method.getArgument(0)
151+
) and
152+
v.(AbstractValues::BooleanValue).getValue() = true
153+
)
154+
}
155+
156+
/**
157+
* An URL argument to a call to `.Contains()` that is a sanitizer for URL redirects.
158+
*
159+
* This `Contains` method is usually called on a list, but the sanitizer matches any call to a method
160+
* called `Contains`, so other methods with the same name will also be considered sanitizers.
161+
*/
162+
class ContainsUrlSanitizer extends Sanitizer {
163+
ContainsUrlSanitizer() {
164+
this = DataFlow::BarrierGuard<isContainsUrlSanitizer/3>::getABarrierNode()
165+
}
166+
}
167+
168+
/**
169+
* A check that the URL is relative, and therefore safe for URL redirects.
170+
*/
171+
private predicate isRelativeUrlSanitizer(Guard guard, Expr e, AbstractValue v) {
172+
guard =
173+
any(PropertyAccess access |
174+
access.getProperty().hasFullyQualifiedName("System", "Uri", "IsAbsoluteUri") and
175+
e = access.getQualifier() and
176+
v.(AbstractValues::BooleanValue).getValue() = false
177+
)
178+
}
179+
180+
/**
181+
* A check that the URL is relative, and therefore safe for URL redirects.
182+
*/
183+
class RelativeUrlSanitizer extends Sanitizer {
184+
RelativeUrlSanitizer() {
185+
this = DataFlow::BarrierGuard<isRelativeUrlSanitizer/3>::getABarrierNode()
186+
}
187+
}
188+
189+
/**
190+
* A comparison on the `Host` property of a url, that is a sanitizer for URL redirects.
191+
* E.g. `url.Host == "example.org"`
192+
*/
193+
private predicate isHostComparisonSanitizer(Guard guard, Expr e, AbstractValue v) {
194+
guard =
195+
any(EqualityOperation comparison |
196+
exists(PropertyAccess access | access = comparison.getAnOperand() |
197+
access.getProperty().hasFullyQualifiedName("System", "Uri", "Host") and
198+
e = access.getQualifier()
199+
) and
200+
if comparison instanceof EQExpr
201+
then v.(AbstractValues::BooleanValue).getValue() = true
202+
else v.(AbstractValues::BooleanValue).getValue() = false
203+
)
204+
}
205+
206+
/**
207+
* A comparison on the `Host` property of a url, that is a sanitizer for URL redirects.
208+
*/
209+
class HostComparisonSanitizer extends Sanitizer {
210+
HostComparisonSanitizer() {
211+
this = DataFlow::BarrierGuard<isHostComparisonSanitizer/3>::getABarrierNode()
212+
}
213+
}
214+
142215
/**
143216
* A call to the getter of the RawUrl property, whose value is considered to be safe for URL
144217
* redirects.

‎csharp/ql/src/change-notes/2024-02-14-url-sanitizers.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Added sanitizers for relative URLs, `List.Contains()`, and checking the `.Host` property on an URI to the `cs/web/unvalidated-url-redirection` query.

‎csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
edges
2+
| UrlRedirect2.cs:14:31:14:53 | access to property QueryString : NameValueCollection | UrlRedirect2.cs:14:31:14:61 | access to indexer | provenance | |
23
| UrlRedirect.cs:13:31:13:53 | access to property QueryString : NameValueCollection | UrlRedirect.cs:13:31:13:61 | access to indexer | provenance | |
34
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:23:22:23:52 | access to indexer : String | provenance | |
45
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:48:29:48:31 | access to local variable url | provenance | |
@@ -28,6 +29,8 @@ edges
2829
| UrlRedirectCore.cs:45:51:45:55 | value : String | UrlRedirectCore.cs:56:31:56:35 | access to parameter value | provenance | |
2930
| UrlRedirectCore.cs:53:40:53:44 | access to parameter value : String | UrlRedirectCore.cs:53:32:53:45 | object creation of type Uri | provenance | |
3031
nodes
32+
| UrlRedirect2.cs:14:31:14:53 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
33+
| UrlRedirect2.cs:14:31:14:61 | access to indexer | semmle.label | access to indexer |
3134
| UrlRedirect.cs:13:31:13:53 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
3235
| UrlRedirect.cs:13:31:13:61 | access to indexer | semmle.label | access to indexer |
3336
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
@@ -58,6 +61,7 @@ nodes
5861
| UrlRedirectCore.cs:56:31:56:35 | access to parameter value | semmle.label | access to parameter value |
5962
subpaths
6063
#select
64+
| UrlRedirect2.cs:14:31:14:61 | access to indexer | UrlRedirect2.cs:14:31:14:53 | access to property QueryString : NameValueCollection | UrlRedirect2.cs:14:31:14:61 | access to indexer | Untrusted URL redirection due to $@. | UrlRedirect2.cs:14:31:14:53 | access to property QueryString | user-provided value |
6165
| UrlRedirect.cs:13:31:13:61 | access to indexer | UrlRedirect.cs:13:31:13:53 | access to property QueryString : NameValueCollection | UrlRedirect.cs:13:31:13:61 | access to indexer | Untrusted URL redirection due to $@. | UrlRedirect.cs:13:31:13:53 | access to property QueryString | user-provided value |
6266
| UrlRedirect.cs:38:44:38:74 | access to indexer | UrlRedirect.cs:38:44:38:66 | access to property QueryString : NameValueCollection | UrlRedirect.cs:38:44:38:74 | access to indexer | Untrusted URL redirection due to $@. | UrlRedirect.cs:38:44:38:66 | access to property QueryString | user-provided value |
6367
| UrlRedirect.cs:39:47:39:77 | access to indexer | UrlRedirect.cs:39:47:39:69 | access to property QueryString : NameValueCollection | UrlRedirect.cs:39:47:39:77 | access to indexer | Untrusted URL redirection due to $@. | UrlRedirect.cs:39:47:39:69 | access to property QueryString | user-provided value |

‎csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
using System;
2+
using System.Web;
3+
using System.Web.Mvc;
4+
using System.Web.WebPages;
5+
using System.Collections.Generic;
6+
7+
public class UrlRedirectHandler2 : IHttpHandler
8+
{
9+
private List<string> VALID_REDIRECTS = new List<string>{ "http://cwe.mitre.org/data/definitions/601.html", "http://cwe.mitre.org/data/definitions/79.html" };
10+
11+
public void ProcessRequest(HttpContext ctx)
12+
{
13+
// BAD: a request parameter is incorporated without validation into a URL redirect
14+
ctx.Response.Redirect(ctx.Request.QueryString["page"]);
15+
16+
var redirectUrl = ctx.Request.QueryString["page"];
17+
if (VALID_REDIRECTS.Contains(redirectUrl))
18+
{
19+
// GOOD: the request parameter is validated against set of known fixed strings
20+
ctx.Response.Redirect(redirectUrl);
21+
}
22+
23+
var url = new Uri(redirectUrl, UriKind.RelativeOrAbsolute);
24+
if (!url.IsAbsoluteUri) {
25+
// GOOD: The redirect is to a relative URL
26+
ctx.Response.Redirect(url.ToString());
27+
}
28+
29+
if (url.Host == "example.org") {
30+
// GOOD: The redirect is to a known host
31+
ctx.Response.Redirect(url.ToString());
32+
}
33+
}
34+
}

0 commit comments

Comments
 (0)