Skip to content

Commit e6a5c50

Browse files
michaelnebelmichaelnebel
authored
Merge pull request github#14953 from rpmrmartin/issue/14952
C#: Fix a URL redirection from remote source false positive
2 parents 8ce4bbe + 8dcdda6 commit e6a5c50

File tree

6 files changed

+65
-27
lines changed

6 files changed

+65
-27
lines changed

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

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -115,14 +115,24 @@ class HttpServerTransferSink extends Sink {
115115
}
116116
}
117117

118-
private predicate isLocalUrlSanitizer(Guard g, Expr e, AbstractValue v) {
119-
g.(MethodCall).getTarget().hasName("IsLocalUrl") and
120-
e = g.(MethodCall).getArgument(0) and
118+
private predicate isLocalUrlSanitizerMethodCall(MethodCall guard, Expr e, AbstractValue v) {
119+
exists(Method m | m = guard.getTarget() |
120+
m.hasName("IsLocalUrl") and
121+
e = guard.getArgument(0)
122+
or
123+
m.hasName("IsUrlLocalToHost") and
124+
e = guard.getArgument(1)
125+
) and
121126
v.(AbstractValues::BooleanValue).getValue() = true
122127
}
123128

129+
private predicate isLocalUrlSanitizer(Guard g, Expr e, AbstractValue v) {
130+
isLocalUrlSanitizerMethodCall(g, e, v)
131+
}
132+
124133
/**
125-
* A URL argument to a call to `UrlHelper.isLocalUrl()` that is a sanitizer for URL redirects.
134+
* A URL argument to a call to `UrlHelper.IsLocalUrl()` or `HttpRequestBase.IsUrlLocalToHost()` that
135+
* is a sanitizer for URL redirects.
126136
*/
127137
class LocalUrlSanitizer extends Sanitizer {
128138
LocalUrlSanitizer() { this = DataFlow::BarrierGuard<isLocalUrlSanitizer/3>::getABarrierNode() }

‎csharp/ql/src/change-notes/2023-11-29-url-redirect-false-positive.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+
* Fixed a URL redirection from remote source false positive when guarding a redirect with `HttpRequestBase.IsUrlLocalToHost()`

‎csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.ql

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import semmle.code.csharp.security.dataflow.flowsources.Remote
22

33
from RemoteFlowSource source
4+
where source.getLocation().getFile().fromSource()
45
select source, source.getSourceType()

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System;
22
using System.Web;
33
using System.Web.Mvc;
4+
using System.Web.WebPages;
45

56
public class UrlRedirectHandler : IHttpHandler
67
{
@@ -48,6 +49,13 @@ public void ProcessRequest(HttpContext ctx)
4849

4950
// GOOD: request parameter is URL encoded
5051
ctx.Response.Redirect(HttpUtility.UrlEncode(ctx.Request.QueryString["page"]));
52+
53+
// GOOD: whitelisted redirect
54+
var url3 = ctx.Request.QueryString["page"];
55+
if (new HttpRequestWrapper(ctx.Request).IsUrlLocalToHost(url3))
56+
{
57+
ctx.Response.Redirect(url3);
58+
}
5159
}
5260

5361
// Implementation as recommended by Microsoft.

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

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
edges
2-
| UrlRedirect.cs:12:31:12:53 | access to property QueryString : NameValueCollection | UrlRedirect.cs:12:31:12:61 | access to indexer |
3-
| UrlRedirect.cs:22:22:22:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:22:22:22:52 | access to indexer : String |
4-
| UrlRedirect.cs:22:22:22:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:47:29:47:31 | access to local variable url |
5-
| UrlRedirect.cs:22:22:22:52 | access to indexer : String | UrlRedirect.cs:47:29:47:31 | access to local variable url |
6-
| UrlRedirect.cs:37:44:37:66 | access to property QueryString : NameValueCollection | UrlRedirect.cs:37:44:37:74 | access to indexer |
7-
| UrlRedirect.cs:38:47:38:69 | access to property QueryString : NameValueCollection | UrlRedirect.cs:38:47:38:77 | access to indexer |
2+
| UrlRedirect.cs:13:31:13:53 | access to property QueryString : NameValueCollection | UrlRedirect.cs:13:31:13:61 | access to indexer |
3+
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:23:22:23:52 | access to indexer : String |
4+
| 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:52 | access to indexer : String | UrlRedirect.cs:48:29:48:31 | access to local variable url |
6+
| UrlRedirect.cs:38:44:38:66 | access to property QueryString : NameValueCollection | UrlRedirect.cs:38:44:38:74 | access to indexer |
7+
| UrlRedirect.cs:39:47:39:69 | access to property QueryString : NameValueCollection | UrlRedirect.cs:39:47:39:77 | access to indexer |
88
| UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:16:22:16:26 | access to parameter value |
99
| UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:19:44:19:48 | call to operator implicit conversion |
1010
| UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:25:46:25:50 | call to operator implicit conversion |
@@ -17,15 +17,15 @@ edges
1717
| UrlRedirectCore.cs:45:51:45:55 | value : String | UrlRedirectCore.cs:56:31:56:35 | access to parameter value |
1818
| UrlRedirectCore.cs:53:40:53:44 | access to parameter value : String | UrlRedirectCore.cs:53:32:53:45 | object creation of type Uri |
1919
nodes
20-
| UrlRedirect.cs:12:31:12:53 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
21-
| UrlRedirect.cs:12:31:12:61 | access to indexer | semmle.label | access to indexer |
22-
| UrlRedirect.cs:22:22:22:44 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
23-
| UrlRedirect.cs:22:22:22:52 | access to indexer : String | semmle.label | access to indexer : String |
24-
| UrlRedirect.cs:37:44:37:66 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
25-
| UrlRedirect.cs:37:44:37:74 | access to indexer | semmle.label | access to indexer |
26-
| UrlRedirect.cs:38:47:38:69 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
27-
| UrlRedirect.cs:38:47:38:77 | access to indexer | semmle.label | access to indexer |
28-
| UrlRedirect.cs:47:29:47:31 | access to local variable url | semmle.label | access to local variable url |
20+
| UrlRedirect.cs:13:31:13:53 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
21+
| UrlRedirect.cs:13:31:13:61 | access to indexer | semmle.label | access to indexer |
22+
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
23+
| UrlRedirect.cs:23:22:23:52 | access to indexer : String | semmle.label | access to indexer : String |
24+
| UrlRedirect.cs:38:44:38:66 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
25+
| UrlRedirect.cs:38:44:38:74 | access to indexer | semmle.label | access to indexer |
26+
| UrlRedirect.cs:39:47:39:69 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
27+
| UrlRedirect.cs:39:47:39:77 | access to indexer | semmle.label | access to indexer |
28+
| UrlRedirect.cs:48:29:48:31 | access to local variable url | semmle.label | access to local variable url |
2929
| UrlRedirectCore.cs:13:44:13:48 | value : String | semmle.label | value : String |
3030
| UrlRedirectCore.cs:16:22:16:26 | access to parameter value | semmle.label | access to parameter value |
3131
| UrlRedirectCore.cs:19:44:19:48 | call to operator implicit conversion | semmle.label | call to operator implicit conversion |
@@ -41,10 +41,10 @@ nodes
4141
| UrlRedirectCore.cs:56:31:56:35 | access to parameter value | semmle.label | access to parameter value |
4242
subpaths
4343
#select
44-
| UrlRedirect.cs:12:31:12:61 | access to indexer | UrlRedirect.cs:12:31:12:53 | access to property QueryString : NameValueCollection | UrlRedirect.cs:12:31:12:61 | access to indexer | Untrusted URL redirection due to $@. | UrlRedirect.cs:12:31:12:53 | access to property QueryString | user-provided value |
45-
| UrlRedirect.cs:37:44:37:74 | access to indexer | UrlRedirect.cs:37:44:37:66 | access to property QueryString : NameValueCollection | UrlRedirect.cs:37:44:37:74 | access to indexer | Untrusted URL redirection due to $@. | UrlRedirect.cs:37:44:37:66 | access to property QueryString | user-provided value |
46-
| UrlRedirect.cs:38:47:38:77 | access to indexer | UrlRedirect.cs:38:47:38:69 | access to property QueryString : NameValueCollection | UrlRedirect.cs:38:47:38:77 | access to indexer | Untrusted URL redirection due to $@. | UrlRedirect.cs:38:47:38:69 | access to property QueryString | user-provided value |
47-
| UrlRedirect.cs:47:29:47:31 | access to local variable url | UrlRedirect.cs:22:22:22:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:47:29:47:31 | access to local variable url | Untrusted URL redirection due to $@. | UrlRedirect.cs:22:22:22:44 | access to property QueryString | user-provided value |
44+
| 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 |
45+
| 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 |
46+
| 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 |
47+
| 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 |
4848
| 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 |
4949
| 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 |
5050
| 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 |

‎csharp/ql/test/resources/stubs/System.Web.cs

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ public class Control
8181

8282
public class Page
8383
{
84-
public System.Security.Principal.IPrincipal User { get; }
84+
public System.Security.Principal.IPrincipal User { get; }
8585
public System.Web.HttpRequest Request { get; }
8686
}
8787

@@ -157,6 +157,11 @@ public class HttpRequest
157157
public HttpCookieCollection Cookies => null;
158158
}
159159

160+
public class HttpRequestWrapper : System.Web.HttpRequestBase
161+
{
162+
public HttpRequestWrapper(HttpRequest r) { }
163+
}
164+
160165
public class HttpResponse
161166
{
162167
public void Write(object o) { }
@@ -306,15 +311,16 @@ public class RequestContext
306311
{
307312
}
308313

309-
public class Route
314+
public class Route
310315
{
311316
}
312317

313-
public class RouteTable {
318+
public class RouteTable
319+
{
314320
public RouteCollection Routes { get; }
315321
}
316322

317-
public class RouteCollection
323+
public class RouteCollection
318324
{
319325
public Route MapPageRoute(string routeName, string routeUrl, string physicalFile, bool checkPhysicalUrlAccess) { return null; }
320326
}
@@ -369,6 +375,15 @@ public static void Validate() { }
369375
}
370376
}
371377

378+
namespace System.Web.WebPages
379+
{
380+
public static class RequestExtensions
381+
{
382+
public static bool IsUrlLocalToHost(this System.Web.HttpRequestBase request, string url) => throw null;
383+
}
384+
385+
}
386+
372387
namespace System.Web.Script.Serialization
373388
{
374389
// Generated from `System.Web.Script.Serialization.JavaScriptSerializer` in `System.Web.Extensions, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35`

0 commit comments

Comments
 (0)