Merge pull request #15596 from erik-krogh/url-san

C#: Add a few more sanitizers to `cs/web/unvalidated-url-redirection`

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

@@ -139,6 +139,79 @@ class LocalUrlSanitizer extends Sanitizer {
   LocalUrlSanitizer() { this = DataFlow::BarrierGuard<isLocalUrlSanitizer/3>::getABarrierNode() }
 }
 
+/**
+ * An argument to a call to `List.Contains()` that is a sanitizer for URL redirects.
+ */
+private predicate isContainsUrlSanitizer(Guard guard, Expr e, AbstractValue v) {
+  guard =
+    any(MethodCall method |
+      exists(Method m | m = method.getTarget() |
+        m.hasName("Contains") and
+        e = method.getArgument(0)
+      ) and
+      v.(AbstractValues::BooleanValue).getValue() = true
+    )
+}
+
+/**
+ * An URL argument to a call to `.Contains()` that is a sanitizer for URL redirects.
+ *
+ * This `Contains` method is usually called on a list, but the sanitizer matches any call to a method
+ * called `Contains`, so other methods with the same name will also be considered sanitizers.
+ */
+class ContainsUrlSanitizer extends Sanitizer {
+  ContainsUrlSanitizer() {
+    this = DataFlow::BarrierGuard<isContainsUrlSanitizer/3>::getABarrierNode()
+  }
+}
+
+/**
+ * A check that the URL is relative, and therefore safe for URL redirects.
+ */
+private predicate isRelativeUrlSanitizer(Guard guard, Expr e, AbstractValue v) {
+  guard =
+    any(PropertyAccess access |
+      access.getProperty().hasFullyQualifiedName("System", "Uri", "IsAbsoluteUri") and
+      e = access.getQualifier() and
+      v.(AbstractValues::BooleanValue).getValue() = false
+    )
+}
+
+/**
+ * A check that the URL is relative, and therefore safe for URL redirects.
+ */
+class RelativeUrlSanitizer extends Sanitizer {
+  RelativeUrlSanitizer() {
+    this = DataFlow::BarrierGuard<isRelativeUrlSanitizer/3>::getABarrierNode()
+  }
+}
+
+/**
+ * A comparison on the `Host` property of a url, that is a sanitizer for URL redirects.
+ * E.g. `url.Host == "example.org"`
+ */
+private predicate isHostComparisonSanitizer(Guard guard, Expr e, AbstractValue v) {
+  guard =
+    any(EqualityOperation comparison |
+      exists(PropertyAccess access | access = comparison.getAnOperand() |
+        access.getProperty().hasFullyQualifiedName("System", "Uri", "Host") and
+        e = access.getQualifier()
+      ) and
+      if comparison instanceof EQExpr
+      then v.(AbstractValues::BooleanValue).getValue() = true
+      else v.(AbstractValues::BooleanValue).getValue() = false
+    )
+}
+
+/**
+ * A comparison on the `Host` property of a url, that is a sanitizer for URL redirects.
+ */
+class HostComparisonSanitizer extends Sanitizer {
+  HostComparisonSanitizer() {
+    this = DataFlow::BarrierGuard<isHostComparisonSanitizer/3>::getABarrierNode()
+  }
+}
+
 /**
  * A call to the getter of the RawUrl property, whose value is considered to be safe for URL
  * redirects.

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

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* 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

@@ -1,4 +1,5 @@
 edges
+| UrlRedirect2.cs:14:31:14:53 | access to property QueryString : NameValueCollection | UrlRedirect2.cs:14:31:14:61 | access to indexer | provenance |  |
 | UrlRedirect.cs:13:31:13:53 | access to property QueryString : NameValueCollection | UrlRedirect.cs:13:31:13:61 | access to indexer | provenance |  |
 | UrlRedirect.cs:23:22:23:44 | access to property QueryString : NameValueCollection | UrlRedirect.cs:23:22:23:52 | access to indexer : String | provenance |  |
 | 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
 | UrlRedirectCore.cs:45:51:45:55 | value : String | UrlRedirectCore.cs:56:31:56:35 | access to parameter value | provenance |  |
 | UrlRedirectCore.cs:53:40:53:44 | access to parameter value : String | UrlRedirectCore.cs:53:32:53:45 | object creation of type Uri | provenance |  |
 nodes
+| UrlRedirect2.cs:14:31:14:53 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
+| UrlRedirect2.cs:14:31:14:61 | access to indexer | semmle.label | access to indexer |
 | 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 |
@@ -58,6 +61,7 @@ nodes
 | UrlRedirectCore.cs:56:31:56:35 | access to parameter value | semmle.label | access to parameter value |
 subpaths
 #select
+| 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 |
 | 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 |

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

@@ -0,0 +1,34 @@
+using System;
+using System.Web;
+using System.Web.Mvc;
+using System.Web.WebPages;
+using System.Collections.Generic;
+
+public class UrlRedirectHandler2 : IHttpHandler
+{
+    private List<string> VALID_REDIRECTS = new List<string>{ "http://cwe.mitre.org/data/definitions/601.html", "http://cwe.mitre.org/data/definitions/79.html" };
+
+    public void ProcessRequest(HttpContext ctx)
+    {
+        // BAD: a request parameter is incorporated without validation into a URL redirect
+        ctx.Response.Redirect(ctx.Request.QueryString["page"]);
+
+        var redirectUrl = ctx.Request.QueryString["page"];
+        if (VALID_REDIRECTS.Contains(redirectUrl))
+        {
+            // GOOD: the request parameter is validated against set of known fixed strings
+            ctx.Response.Redirect(redirectUrl);
+        }
+
+        var url = new Uri(redirectUrl, UriKind.RelativeOrAbsolute);
+        if (!url.IsAbsoluteUri) {
+            // GOOD: The redirect is to a relative URL
+            ctx.Response.Redirect(url.ToString());
+        }
+
+        if (url.Host == "example.org") {
+            // GOOD: The redirect is to a known host
+            ctx.Response.Redirect(url.ToString());
+        }
+    }
+}