Skip to content

C#: Fix a URL redirection from remote source false positive #14953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
michaelnebel merged 6 commits into from
Dec 5, 2023
Merged

C#: Fix a URL redirection from remote source false positive #14953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
66b456d
C#: Fix a URL redirection from remote source false positive
rpmrmartin Nov 29, 2023
6807fd1
C#: Add some stubs, a testcase and update the expected output without…
michaelnebel Dec 4, 2023
d160890
C#: Re-factor to avoid multiple explicit casts.
michaelnebel Dec 4, 2023
b9833fc
C#: Updated expected test output.
michaelnebel Dec 4, 2023
2fc7e51
C#: Only include source code (and not stubs) in the remote flow sourc…
michaelnebel Dec 4, 2023
8dcdda6
C#: Address review comments.
michaelnebel Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,24 @@ class HttpServerTransferSink extends Sink {
}
}

private predicate isLocalUrlSanitizer(Guard g, Expr e, AbstractValue v) {
g.(MethodCall).getTarget().hasName("IsLocalUrl") and
e = g.(MethodCall).getArgument(0) and
private predicate isLocalUrlSanitizerMethodCall(MethodCall guard, Expr e, AbstractValue v) {
exists(Method m | m = guard.getTarget() |
m.hasName("IsLocalUrl") and
e = guard.getArgument(0)
or
m.hasName("IsUrlLocalToHost") and
e = guard.getArgument(1)
) and
v.(AbstractValues::BooleanValue).getValue() = true
}

private predicate isLocalUrlSanitizer(Guard g, Expr e, AbstractValue v) {
isLocalUrlSanitizerMethodCall(g, e, v)
}

/**
* A URL argument to a call to `UrlHelper.isLocalUrl()` that is a sanitizer for URL redirects.
* A URL argument to a call to `UrlHelper.IsLocalUrl()` or `HttpRequestBase.IsUrlLocalToHost()` that
* is a sanitizer for URL redirects.
*/
class LocalUrlSanitizer extends Sanitizer {
LocalUrlSanitizer() { this = DataFlow::BarrierGuard<isLocalUrlSanitizer/3>::getABarrierNode() }
Expand Down
4 changes: 4 additions & 0 deletions csharp/ql/src/change-notes/2023-11-29-url-redirect-false-positive.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Fixed a URL redirection from remote source false positive when guarding a redirect with `HttpRequestBase.IsUrlLocalToHost()`
1 change: 1 addition & 0 deletions csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import semmle.code.csharp.security.dataflow.flowsources.Remote

from RemoteFlowSource source
where source.getLocation().getFile().fromSource()
select source, source.getSourceType()
8 changes: 8 additions & 0 deletions csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Web;
using System.Web.Mvc;
using System.Web.WebPages;

public class UrlRedirectHandler : IHttpHandler
{
Expand Down Expand Up @@ -48,6 +49,13 @@ public void ProcessRequest(HttpContext ctx)

// GOOD: request parameter is URL encoded
ctx.Response.Redirect(HttpUtility.UrlEncode(ctx.Request.QueryString["page"]));

// GOOD: whitelisted redirect
var url3 = ctx.Request.QueryString["page"];
if (new HttpRequestWrapper(ctx.Request).IsUrlLocalToHost(url3))
{
ctx.Response.Redirect(url3);
}
}

// Implementation as recommended by Microsoft.
Expand Down
38 changes: 19 additions & 19 deletions csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
edges
| UrlRedirect.cs:12:31:12:53 | access to property QueryString : NameValueCollection | UrlRedirect.cs:12:31:12:61 | access to indexer |
| UrlRedirect.cs:22:22:22:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:22:22:22:52 | access to indexer : String |
| UrlRedirect.cs:22:22:22:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:47:29:47:31 | access to local variable url |
| UrlRedirect.cs:22:22:22:52 | access to indexer : String | UrlRedirect.cs:47:29:47:31 | access to local variable url |
| UrlRedirect.cs:37:44:37:66 | access to property QueryString : NameValueCollection | UrlRedirect.cs:37:44:37:74 | access to indexer |
| UrlRedirect.cs:38:47:38:69 | access to property QueryString : NameValueCollection | UrlRedirect.cs:38:47:38:77 | access to indexer |
| UrlRedirect.cs:13:31:13:53 | access to property QueryString : NameValueCollection | UrlRedirect.cs:13:31:13:61 | access to indexer |
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:23:22:23:52 | access to indexer : String |
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:48:29:48:31 | access to local variable url |
| UrlRedirect.cs:23:22:23:52 | access to indexer : String | UrlRedirect.cs:48:29:48:31 | access to local variable url |
| UrlRedirect.cs:38:44:38:66 | access to property QueryString : NameValueCollection | UrlRedirect.cs:38:44:38:74 | access to indexer |
| UrlRedirect.cs:39:47:39:69 | access to property QueryString : NameValueCollection | UrlRedirect.cs:39:47:39:77 | access to indexer |
| UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:16:22:16:26 | access to parameter value |
| UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:19:44:19:48 | call to operator implicit conversion |
| UrlRedirectCore.cs:13:44:13:48 | value : String | UrlRedirectCore.cs:25:46:25:50 | call to operator implicit conversion |
Expand All @@ -17,15 +17,15 @@ edges
| UrlRedirectCore.cs:45:51:45:55 | value : String | UrlRedirectCore.cs:56:31:56:35 | access to parameter value |
| UrlRedirectCore.cs:53:40:53:44 | access to parameter value : String | UrlRedirectCore.cs:53:32:53:45 | object creation of type Uri |
nodes
| UrlRedirect.cs:12:31:12:53 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
| UrlRedirect.cs:12:31:12:61 | access to indexer | semmle.label | access to indexer |
| UrlRedirect.cs:22:22:22:44 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
| UrlRedirect.cs:22:22:22:52 | access to indexer : String | semmle.label | access to indexer : String |
| UrlRedirect.cs:37:44:37:66 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
| UrlRedirect.cs:37:44:37:74 | access to indexer | semmle.label | access to indexer |
| UrlRedirect.cs:38:47:38:69 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
| UrlRedirect.cs:38:47:38:77 | access to indexer | semmle.label | access to indexer |
| UrlRedirect.cs:47:29:47:31 | access to local variable url | semmle.label | access to local variable url |
| UrlRedirect.cs:13:31:13:53 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
| UrlRedirect.cs:13:31:13:61 | access to indexer | semmle.label | access to indexer |
| UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
| UrlRedirect.cs:23:22:23:52 | access to indexer : String | semmle.label | access to indexer : String |
| UrlRedirect.cs:38:44:38:66 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
| UrlRedirect.cs:38:44:38:74 | access to indexer | semmle.label | access to indexer |
| UrlRedirect.cs:39:47:39:69 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
| UrlRedirect.cs:39:47:39:77 | access to indexer | semmle.label | access to indexer |
| UrlRedirect.cs:48:29:48:31 | access to local variable url | semmle.label | access to local variable url |
| UrlRedirectCore.cs:13:44:13:48 | value : String | semmle.label | value : String |
| UrlRedirectCore.cs:16:22:16:26 | access to parameter value | semmle.label | access to parameter value |
| UrlRedirectCore.cs:19:44:19:48 | call to operator implicit conversion | semmle.label | call to operator implicit conversion |
Expand All @@ -41,10 +41,10 @@ nodes
| UrlRedirectCore.cs:56:31:56:35 | access to parameter value | semmle.label | access to parameter value |
subpaths
#select
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
Expand Down
23 changes: 19 additions & 4 deletions csharp/ql/test/resources/stubs/System.Web.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public class Control

public class Page
{
public System.Security.Principal.IPrincipal User { get; }
public System.Security.Principal.IPrincipal User { get; }
public System.Web.HttpRequest Request { get; }
}

Expand Down Expand Up @@ -157,6 +157,11 @@ public class HttpRequest
public HttpCookieCollection Cookies => null;
}

public class HttpRequestWrapper : System.Web.HttpRequestBase
{
public HttpRequestWrapper(HttpRequest r) { }
}

public class HttpResponse
{
public void Write(object o) { }
Expand Down Expand Up @@ -306,15 +311,16 @@ public class RequestContext
{
}

public class Route
public class Route
{
}

public class RouteTable {
public class RouteTable
{
public RouteCollection Routes { get; }
}

public class RouteCollection
public class RouteCollection
{
public Route MapPageRoute(string routeName, string routeUrl, string physicalFile, bool checkPhysicalUrlAccess) { return null; }
}
Expand Down Expand Up @@ -369,6 +375,15 @@ public static void Validate() { }
}
}

namespace System.Web.WebPages
{
public static class RequestExtensions
{
public static bool IsUrlLocalToHost(this System.Web.HttpRequestBase request, string url) => throw null;
}

}

namespace System.Web.Script.Serialization
{
// Generated from `System.Web.Script.Serialization.JavaScriptSerializer` in `System.Web.Extensions, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35`
Expand Down