From 59792808d423dd6c62e8cb81de248de7154987cc Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 13 Feb 2024 12:37:29 +0100 Subject: [PATCH 1/7] add new url-redirect test file --- .../CWE-601/UrlRedirect/UrlRedirect.expected | 4 ++++ .../CWE-601/UrlRedirect/UrlRedirect2.cs | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs diff --git a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect.expected b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect.expected index 33e634145589..b36b30253e0c 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect.expected +++ b/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 | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs new file mode 100644 index 000000000000..855dfe952515 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs @@ -0,0 +1,19 @@ +using System; +using System.Web; +using System.Web.Mvc; +using System.Web.WebPages; +using System.Collections.Generic; + +public class UrlRedirectHandler2 : IHttpHandler +{ + private const String VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.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"]); + + List VALID_REDIRECTS = new List{ "http://cwe.mitre.org/data/definitions/601.html", "http://cwe.mitre.org/data/definitions/79.html" }; + + } +} From 3f8de82ea3b932116624be3b2f2f2afa664c0521 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 13 Feb 2024 12:45:07 +0100 Subject: [PATCH 2/7] add a sanitizer for `List.Contains()` in url-redirect --- .../security/dataflow/UrlRedirectQuery.qll | 22 +++++++++++++++++++ .../CWE-601/UrlRedirect/UrlRedirect2.cs | 6 +++++ 2 files changed, 28 insertions(+) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll index 034972b5f228..149e980bab91 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll @@ -139,6 +139,28 @@ class LocalUrlSanitizer extends Sanitizer { LocalUrlSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() } } +/** + * A argument to a call to `List.Contains()` that is a sanitizer for URL redirects. + */ +private predicate isContainsUrlSanitizer(Guard guard, Expr e, AbstractValue v) { + exists(MethodCall method | method = guard | + exists(Method m | m = method.getTarget() | + m.hasName("Contains") and + e = method.getArgument(0) + ) and + v.(AbstractValues::BooleanValue).getValue() = true + ) +} + +/** + * A URL argument to a call to `List.Contains()` that is a sanitizer for URL redirects. + */ +class ContainsUrlSanitizer extends Sanitizer { + ContainsUrlSanitizer() { + this = DataFlow::BarrierGuard::getABarrierNode() + } +} + /** * A call to the getter of the RawUrl property, whose value is considered to be safe for URL * redirects. diff --git a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs index 855dfe952515..1bc778b9fe55 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs @@ -14,6 +14,12 @@ public void ProcessRequest(HttpContext ctx) ctx.Response.Redirect(ctx.Request.QueryString["page"]); List VALID_REDIRECTS = new List{ "http://cwe.mitre.org/data/definitions/601.html", "http://cwe.mitre.org/data/definitions/79.html" }; + 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); + } } } From f4dd3e9aa16ffd862fbb4714af5f02b936dc7db3 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 13 Feb 2024 13:13:18 +0100 Subject: [PATCH 3/7] treat relative URLs as safe for url-redirects --- .../security/dataflow/UrlRedirectQuery.qll | 21 +++++++++++++++++++ .../CWE-601/UrlRedirect/UrlRedirect2.cs | 6 ++++++ 2 files changed, 27 insertions(+) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll index 149e980bab91..909c2c966f77 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll @@ -161,6 +161,27 @@ class ContainsUrlSanitizer extends Sanitizer { } } +/** + * A check that the URL is relative, and therefore safe for URL redirects. + */ +private predicate isRelativeUrlSanitizer(Guard guard, Expr e, AbstractValue v) { + exists(PropertyAccess access | access = guard | + access.getProperty().getName() = "IsAbsoluteUri" and + // TOOD: type = URL? + 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::getABarrierNode() + } +} + /** * A call to the getter of the RawUrl property, whose value is considered to be safe for URL * redirects. diff --git a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs index 1bc778b9fe55..cd429c8db0dc 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs @@ -20,6 +20,12 @@ public void ProcessRequest(HttpContext ctx) // 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()); + } } } From 4dae8d0bb4fe261a8979530c0b8c4abbb59222cd Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 13 Feb 2024 13:08:08 +0100 Subject: [PATCH 4/7] add host comparisons as a sanitizer for url-redirect --- .../security/dataflow/UrlRedirectQuery.qll | 25 +++++++++++++++++++ .../CWE-601/UrlRedirect/UrlRedirect2.cs | 6 ++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll index 909c2c966f77..8429362f575a 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll @@ -182,6 +182,31 @@ class RelativeUrlSanitizer extends Sanitizer { } } +/** + * 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) { + exists(EqualityOperation comparison | comparison = guard | + exists(PropertyAccess access | access = comparison.getAnOperand() | + access.getProperty().getName() = "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::getABarrierNode() + } +} + /** * A call to the getter of the RawUrl property, whose value is considered to be safe for URL * redirects. diff --git a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs index cd429c8db0dc..96da2b6e773d 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs @@ -26,6 +26,10 @@ public void ProcessRequest(HttpContext ctx) // 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()); + } } } From d31bfc06c237a499248a3951b11cbe7c99846116 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 13 Feb 2024 13:10:38 +0100 Subject: [PATCH 5/7] add type requirement to the new Uri sanitizers --- .../semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll index 8429362f575a..02693107dde3 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll @@ -167,7 +167,7 @@ class ContainsUrlSanitizer extends Sanitizer { private predicate isRelativeUrlSanitizer(Guard guard, Expr e, AbstractValue v) { exists(PropertyAccess access | access = guard | access.getProperty().getName() = "IsAbsoluteUri" and - // TOOD: type = URL? + access.getQualifier().getType().getFullyQualifiedName() = "System.Uri" and e = access.getQualifier() and v.(AbstractValues::BooleanValue).getValue() = false ) @@ -190,6 +190,7 @@ private predicate isHostComparisonSanitizer(Guard guard, Expr e, AbstractValue v exists(EqualityOperation comparison | comparison = guard | exists(PropertyAccess access | access = comparison.getAnOperand() | access.getProperty().getName() = "Host" and + access.getQualifier().getType().getFullyQualifiedName() = "System.Uri" and e = access.getQualifier() ) and if comparison instanceof EQExpr From a2bd45d0cb37663980090bce0ae70cef4e14b6d7 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Wed, 14 Feb 2024 13:50:27 +0100 Subject: [PATCH 6/7] apply suggestions from code review --- .../security/dataflow/UrlRedirectQuery.qll | 54 ++++++++++--------- .../CWE-601/UrlRedirect/UrlRedirect2.cs | 3 +- 2 files changed, 30 insertions(+), 27 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll index 02693107dde3..53b4cceb9602 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll @@ -140,20 +140,24 @@ class LocalUrlSanitizer extends Sanitizer { } /** - * A argument to a call to `List.Contains()` that is a sanitizer for URL redirects. + * An argument to a call to `List.Contains()` that is a sanitizer for URL redirects. */ private predicate isContainsUrlSanitizer(Guard guard, Expr e, AbstractValue v) { - exists(MethodCall method | method = guard | - exists(Method m | m = method.getTarget() | - m.hasName("Contains") and - e = method.getArgument(0) - ) and - v.(AbstractValues::BooleanValue).getValue() = true - ) + guard = + any(MethodCall method | + exists(Method m | m = method.getTarget() | + m.hasName("Contains") and + e = method.getArgument(0) + ) and + v.(AbstractValues::BooleanValue).getValue() = true + ) } /** - * A URL argument to a call to `List.Contains()` that is a sanitizer for URL redirects. + * 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() { @@ -165,12 +169,12 @@ class ContainsUrlSanitizer extends Sanitizer { * A check that the URL is relative, and therefore safe for URL redirects. */ private predicate isRelativeUrlSanitizer(Guard guard, Expr e, AbstractValue v) { - exists(PropertyAccess access | access = guard | - access.getProperty().getName() = "IsAbsoluteUri" and - access.getQualifier().getType().getFullyQualifiedName() = "System.Uri" and - e = access.getQualifier() and - v.(AbstractValues::BooleanValue).getValue() = false - ) + guard = + any(PropertyAccess access | + access.getProperty().hasFullyQualifiedName("System", "Uri", "IsAbsoluteUri") and + e = access.getQualifier() and + v.(AbstractValues::BooleanValue).getValue() = false + ) } /** @@ -187,16 +191,16 @@ class RelativeUrlSanitizer extends Sanitizer { * E.g. `url.Host == "example.org"` */ private predicate isHostComparisonSanitizer(Guard guard, Expr e, AbstractValue v) { - exists(EqualityOperation comparison | comparison = guard | - exists(PropertyAccess access | access = comparison.getAnOperand() | - access.getProperty().getName() = "Host" and - access.getQualifier().getType().getFullyQualifiedName() = "System.Uri" and - e = access.getQualifier() - ) and - if comparison instanceof EQExpr - then v.(AbstractValues::BooleanValue).getValue() = true - else v.(AbstractValues::BooleanValue).getValue() = false - ) + 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 + ) } /** diff --git a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs index 96da2b6e773d..83f499ea048d 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-601/UrlRedirect/UrlRedirect2.cs @@ -6,14 +6,13 @@ public class UrlRedirectHandler2 : IHttpHandler { - private const String VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html"; + private List VALID_REDIRECTS = new List{ "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"]); - List VALID_REDIRECTS = new List{ "http://cwe.mitre.org/data/definitions/601.html", "http://cwe.mitre.org/data/definitions/79.html" }; var redirectUrl = ctx.Request.QueryString["page"]; if (VALID_REDIRECTS.Contains(redirectUrl)) { From 7c2465e7b78acafaf4dbfa7c361d4c051e1ea40b Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Wed, 14 Feb 2024 13:53:43 +0100 Subject: [PATCH 7/7] add change-note --- csharp/ql/src/change-notes/2024-02-14-url-sanitizers.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/src/change-notes/2024-02-14-url-sanitizers.md diff --git a/csharp/ql/src/change-notes/2024-02-14-url-sanitizers.md b/csharp/ql/src/change-notes/2024-02-14-url-sanitizers.md new file mode 100644 index 000000000000..1ac35ed0e899 --- /dev/null +++ b/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. \ No newline at end of file