Skip to content

Commit 2be1ee8

Browse files
michaelnebelmichaelnebel
authored
Merge pull request #15394 from michaelnebel/csharp/urlredirect-sanitizer
C#: Add more santizers to the `cs/web/unvalidated-url-redirection` query.
2 parents 3c8b093 + 10be0de commit 2be1ee8

File tree

5 files changed

+75
-2
lines changed

5 files changed

+75
-2
lines changed

‎csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ class ValidFormatString extends StringLiteral {
134134
result = this.getValue().regexpFind(getValidFormatTokenRegex(), _, outPosition)
135135
}
136136

137-
/**Gets the insert number at the given position in the string. */
137+
/** Gets the insert number at the given position in the string. */
138138
int getInsert(int position) {
139139
result = this.getToken(position).regexpCapture(getFormatInsertRegex(), 1).toInt()
140140
}

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import csharp
66
private import semmle.code.csharp.security.dataflow.flowsources.Remote
77
private import semmle.code.csharp.controlflow.Guards
8+
private import semmle.code.csharp.frameworks.Format
89
private import semmle.code.csharp.frameworks.system.Web
910
private import semmle.code.csharp.frameworks.system.web.Mvc
1011
private import semmle.code.csharp.security.Sanitizers
@@ -163,6 +164,36 @@ class ConcatenationSanitizer extends Sanitizer {
163164
}
164165
}
165166

167+
/**
168+
* A string interpolation expression, where the first part (before any inserts) of the
169+
* expression contains the character "?".
170+
*
171+
* This is considered a sanitizer by the same reasoning as `ConcatenationSanitizer`.
172+
*/
173+
private class InterpolationSanitizer extends Sanitizer {
174+
InterpolationSanitizer() {
175+
this.getExpr().(InterpolatedStringExpr).getText(0).getValue().matches("%?%")
176+
}
177+
}
178+
179+
/**
180+
* A call to `string.Format`, where the format expression (before any inserts)
181+
* contains the character "?".
182+
*
183+
* This is considered a sanitizer by the same reasoning as `ConcatenationSanitizer`.
184+
*/
185+
private class StringFormatSanitizer extends Sanitizer {
186+
StringFormatSanitizer() {
187+
exists(FormatCall c, Expr e, int index, string format |
188+
c = this.getExpr() and e = c.getFormatExpr()
189+
|
190+
format = e.(StringLiteral).getValue() and
191+
exists(format.regexpFind("\\{[0-9]+\\}", 0, index)) and
192+
format.substring(0, index).matches("%?%")
193+
)
194+
}
195+
}
196+
166197
/** A call to an URL encoder. */
167198
class UrlEncodeSanitizer extends Sanitizer {
168199
UrlEncodeSanitizer() { this.getExpr() instanceof UrlSanitizedExpr }

‎csharp/ql/src/change-notes/2024-01-22-url-redirect-sanitizer.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 string interpolation expressions and `string.Format` as possible sanitizers for the `cs/web/unvalidated-url-redirection` query.

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

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public void ProcessRequest(HttpContext ctx)
4141
// GOOD: Redirecting to the RawUrl only reloads the current Url
4242
ctx.Response.Redirect(ctx.Request.RawUrl);
4343

44-
// GOOD: The attacker can only control the parameters, not the locaiton
44+
// GOOD: The attacker can only control the parameters, not the location
4545
ctx.Response.Redirect("foo.asp?param=" + url);
4646

4747
// BAD: Using Transfer with unvalidated user input
@@ -56,6 +56,24 @@ public void ProcessRequest(HttpContext ctx)
5656
{
5757
ctx.Response.Redirect(url3);
5858
}
59+
60+
// GOOD: The attacker can only control the parameters, not the location
61+
ctx.Response.Redirect($"foo.asp?param={url}");
62+
63+
// BAD: The attacker can control the location
64+
ctx.Response.Redirect($"{url}.asp?param=foo");
65+
66+
// GOOD: The attacker can only control the parameters, not the location
67+
ctx.Response.Redirect(string.Format("foo.asp?param={0}", url));
68+
69+
// BAD: The attacker can control the location
70+
ctx.Response.Redirect(string.Format("{0}.asp?param=foo", url));
71+
72+
// GOOD: The attacker can only control the parameters, not the location
73+
ctx.Response.Redirect(string.Format("foo.asp?{1}param={0}", url, url));
74+
75+
// BAD: The attacker can control the location
76+
ctx.Response.Redirect(string.Format("{1}.asp?{0}param=foo", url, url));
5977
}
6078

6179
// Implementation as recommended by Microsoft.

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,20 @@ edges
22
| UrlRedirect.cs:13:31:13:53 | access to property QueryString : NameValueCollection | UrlRedirect.cs:13:31:13:61 | access to indexer |
33
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:23:22:23:52 | access to indexer : String |
44
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:48:29:48:31 | access to local variable url |
5+
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:64:31:64:52 | $"..." |
6+
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:70:66:70:68 | access to local variable url : String |
7+
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:76:69:76:71 | access to local variable url : String |
8+
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:76:74:76:76 | access to local variable url : String |
59
| UrlRedirect.cs:23:22:23:52 | access to indexer : String | UrlRedirect.cs:48:29:48:31 | access to local variable url |
10+
| UrlRedirect.cs:23:22:23:52 | access to indexer : String | UrlRedirect.cs:64:31:64:52 | $"..." |
11+
| UrlRedirect.cs:23:22:23:52 | access to indexer : String | UrlRedirect.cs:70:66:70:68 | access to local variable url : String |
12+
| UrlRedirect.cs:23:22:23:52 | access to indexer : String | UrlRedirect.cs:76:69:76:71 | access to local variable url : String |
13+
| UrlRedirect.cs:23:22:23:52 | access to indexer : String | UrlRedirect.cs:76:74:76:76 | access to local variable url : String |
614
| UrlRedirect.cs:38:44:38:66 | access to property QueryString : NameValueCollection | UrlRedirect.cs:38:44:38:74 | access to indexer |
715
| UrlRedirect.cs:39:47:39:69 | access to property QueryString : NameValueCollection | UrlRedirect.cs:39:47:39:77 | access to indexer |
16+
| UrlRedirect.cs:70:66:70:68 | access to local variable url : String | UrlRedirect.cs:70:31:70:69 | call to method Format |
17+
| UrlRedirect.cs:76:69:76:71 | access to local variable url : String | UrlRedirect.cs:76:31:76:77 | call to method Format |
18+
| UrlRedirect.cs:76:74:76:76 | access to local variable url : String | UrlRedirect.cs:76:31:76:77 | call to method Format |
819
| UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:16:22:16:26 | access to parameter value |
920
| UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:19:44:19:48 | call to operator implicit conversion |
1021
| UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:25:46:25:50 | call to operator implicit conversion |
@@ -26,6 +37,12 @@ nodes
2637
| UrlRedirect.cs:39:47:39:69 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
2738
| UrlRedirect.cs:39:47:39:77 | access to indexer | semmle.label | access to indexer |
2839
| UrlRedirect.cs:48:29:48:31 | access to local variable url | semmle.label | access to local variable url |
40+
| UrlRedirect.cs:64:31:64:52 | $"..." | semmle.label | $"..." |
41+
| UrlRedirect.cs:70:31:70:69 | call to method Format | semmle.label | call to method Format |
42+
| UrlRedirect.cs:70:66:70:68 | access to local variable url : String | semmle.label | access to local variable url : String |
43+
| UrlRedirect.cs:76:31:76:77 | call to method Format | semmle.label | call to method Format |
44+
| UrlRedirect.cs:76:69:76:71 | access to local variable url : String | semmle.label | access to local variable url : String |
45+
| UrlRedirect.cs:76:74:76:76 | access to local variable url : String | semmle.label | access to local variable url : String |
2946
| UrlRedirectCore.cs:13:44:13:48 | value : String | semmle.label | value : String |
3047
| UrlRedirectCore.cs:16:22:16:26 | access to parameter value | semmle.label | access to parameter value |
3148
| UrlRedirectCore.cs:19:44:19:48 | call to operator implicit conversion | semmle.label | call to operator implicit conversion |
@@ -45,6 +62,9 @@ subpaths
4562
| 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 |
4663
| 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 |
4764
| UrlRedirect.cs:48:29:48:31 | access to local variable url | UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:48:29:48:31 | access to local variable url | Untrusted URL redirection due to $@. | UrlRedirect.cs:23:22:23:44 | access to property QueryString | user-provided value |
65+
| UrlRedirect.cs:64:31:64:52 | $"..." | UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:64:31:64:52 | $"..." | Untrusted URL redirection due to $@. | UrlRedirect.cs:23:22:23:44 | access to property QueryString | user-provided value |
66+
| UrlRedirect.cs:70:31:70:69 | call to method Format | UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:70:31:70:69 | call to method Format | Untrusted URL redirection due to $@. | UrlRedirect.cs:23:22:23:44 | access to property QueryString | user-provided value |
67+
| UrlRedirect.cs:76:31:76:77 | call to method Format | UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:76:31:76:77 | call to method Format | Untrusted URL redirection due to $@. | UrlRedirect.cs:23:22:23:44 | access to property QueryString | user-provided value |
4868
| UrlRedirectCore.cs:16:22:16:26 | access to parameter value | UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:16:22:16:26 | access to parameter value | Untrusted URL redirection due to $@. | UrlRedirectCore.cs:13:44:13:48 | value | user-provided value |
4969
| UrlRedirectCore.cs:19:44:19:48 | call to operator implicit conversion | UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:19:44:19:48 | call to operator implicit conversion | Untrusted URL redirection due to $@. | UrlRedirectCore.cs:13:44:13:48 | value | user-provided value |
5070
| UrlRedirectCore.cs:25:46:25:50 | call to operator implicit conversion | UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:25:46:25:50 | call to operator implicit conversion | Untrusted URL redirection due to $@. | UrlRedirectCore.cs:13:44:13:48 | value | user-provided value |

0 commit comments

Comments
 (0)