Merge pull request #21140 from owen-mc/csharp/mad-barriers

C#: Allow MaD barriers and barrier guards, and convert some existing ones

csharp/ql/lib/ext/System.Web.model.yml

@@ -1,4 +1,10 @@
 extensions:
+  - addsTo:
+      pack: codeql/csharp-all
+      extensible: barrierModel
+    data:
+      # The RawUrl property is considered to be safe for URL redirects
+      - ["System.Web", "HttpRequest", False, "get_RawUrl", "()", "", "ReturnValue", "url-redirection", "manual"]
   - addsTo:
       pack: codeql/csharp-all
       extensible: sinkModel

csharp/ql/lib/ext/System.model.yml

@@ -11,6 +11,11 @@ extensions:
       - ["System", "Environment", False, "get_CommandLine", "()", "", "ReturnValue", "commandargs", "manual"]
       - ["System", "Environment", False, "GetEnvironmentVariable", "", "", "ReturnValue", "environment", "manual"]
       - ["System", "Environment", False, "GetEnvironmentVariables", "", "", "ReturnValue", "environment", "manual"]
+  - addsTo:
+      pack: codeql/csharp-all
+      extensible: barrierGuardModel
+    data:
+      - ["System", "Uri", False, "get_IsAbsoluteUri", "()", "", "Argument[this]", "false", "url-redirection", "manual"]
   - addsTo:
       pack: codeql/csharp-all
       extensible: summaryModel

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

@@ -4,6 +4,7 @@
 
 import csharp
 private import semmle.code.csharp.security.dataflow.flowsources.Remote
+private import semmle.code.csharp.dataflow.internal.ExternalFlow
 private import semmle.code.csharp.frameworks.system.Web
 private import semmle.code.csharp.security.SensitiveActions
 private import semmle.code.csharp.security.dataflow.flowsinks.ExternalLocationSink
@@ -62,3 +63,5 @@ class ProtectSanitizer extends Sanitizer {
  * An external location sink.
  */
 class ExternalSink extends Sink instanceof ExternalLocationSink { }
+
+private class ExternalSanitizer extends Sanitizer instanceof ExternalLocationSanitizer { }

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

@@ -95,7 +95,12 @@ class RoslynCSharpScriptSink extends Sink {
   }
 }
 
-/** Code injection sinks defined through CSV models. */
+/** A code injection sink defined through Models as Data. */
 private class ExternalCodeInjectionExprSink extends Sink {
   ExternalCodeInjectionExprSink() { sinkNode(this, "code-injection") }
 }
+
+/** A sanitizer for code injection defined through Models as Data. */
+private class ExternalCodeInjectionSanitizer extends Sanitizer {
+  ExternalCodeInjectionSanitizer() { barrierNode(this, "code-injection") }
+}

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

@@ -61,11 +61,16 @@ deprecated class RemoteSource extends DataFlow::Node instanceof RemoteFlowSource
 /** A source supported by the current threat model. */
 class ThreatModelSource extends Source instanceof ActiveThreatModelSource { }
 
-/** Command Injection sinks defined through Models as Data. */
+/** A Command Injection sink defined through Models as Data. */
 private class ExternalCommandInjectionExprSink extends Sink {
   ExternalCommandInjectionExprSink() { sinkNode(this, "command-injection") }
 }
 
+/** A sanitizer for command injection defined through Models as Data. */
+private class ExternalCommandInjectionSanitizer extends Sanitizer {
+  ExternalCommandInjectionSanitizer() { barrierNode(this, "command-injection") }
+}
+
 /**
  * A sink in `System.Diagnostic.Process` or its related classes.
  */

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

@@ -46,3 +46,5 @@ private class PrivateDataSource extends Source {
 }
 
 private class ExternalLocation extends Sink instanceof ExternalLocationSink { }
+
+private class ExternalSanitizer extends Sanitizer instanceof ExternalLocationSanitizer { }

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

@@ -64,11 +64,16 @@ deprecated class RemoteSource extends DataFlow::Node instanceof RemoteFlowSource
 /** A source supported by the current threat model. */
 class ThreatModelSource extends Source instanceof ActiveThreatModelSource { }
 
-/** LDAP sinks defined through Models as Data. */
+/** An LDAP sink defined through Models as Data. */
 private class ExternalLdapExprSink extends Sink {
   ExternalLdapExprSink() { sinkNode(this, "ldap-injection") }
 }
 
+/** A sanitizer for LDAP injection defined through Models as Data. */
+private class ExternalLdapInjectionSanitizer extends Sanitizer {
+  ExternalLdapInjectionSanitizer() { barrierNode(this, "ldap-injection") }
+}
+
 /**
  * An argument that sets the `Path` property of a `DirectoryEntry` object that is a sink for LDAP
  * injection.

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

@@ -61,11 +61,16 @@ private class LogForgingLogMessageSink extends Sink, LogMessageSink { }
  */
 private class LogForgingTraceMessageSink extends Sink, TraceMessageSink { }
 
-/** Log Forging sinks defined through Models as Data. */
+/** A Log Forging sink defined through Models as Data. */
 private class ExternalLoggingExprSink extends Sink {
   ExternalLoggingExprSink() { sinkNode(this, "log-injection") }
 }
 
+/** A sanitizer for log forging defined through Models as Data. */
+private class ExternalLogForgingSanitizer extends Sanitizer {
+  ExternalLogForgingSanitizer() { barrierNode(this, "log-injection") }
+}
+
 /**
  * A call to String replace or remove that is considered to sanitize replaced string.
  */

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

@@ -74,11 +74,16 @@ class SqlInjectionExprSink extends Sink {
   SqlInjectionExprSink() { exists(SqlExpr s | this.getExpr() = s.getSql()) }
 }
 
-/** SQL sinks defined through CSV models. */
+/** An SQL sink defined through CSV models. */
 private class ExternalSqlInjectionExprSink extends Sink {
   ExternalSqlInjectionExprSink() { sinkNode(this, "sql-injection") }
 }
 
+/** A sanitizer for SQL injection defined through Models as Data. */
+private class ExternalSqlInjectionSanitizer extends Sanitizer {
+  ExternalSqlInjectionSanitizer() { barrierNode(this, "sql-injection") }
+}
+
 private class SimpleTypeSanitizer extends Sanitizer, SimpleTypeSanitizedExpr { }
 
 private class GuidSanitizer extends Sanitizer, GuidSanitizedExpr { }

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

@@ -56,11 +56,16 @@ deprecated class RemoteSource extends DataFlow::Node instanceof RemoteFlowSource
 /** A source supported by the current threat model. */
 class ThreatModelSource extends Source instanceof ActiveThreatModelSource { }
 
-/** URL Redirection sinks defined through Models as Data. */
+/** A URL Redirection sink defined through Models as Data. */
 private class ExternalUrlRedirectExprSink extends Sink {
   ExternalUrlRedirectExprSink() { sinkNode(this, "url-redirection") }
 }
 
+/** A sanitizer for URL redirection defined through Models as Data. */
+private class ExternalUrlRedirectSanitizer extends Sanitizer {
+  ExternalUrlRedirectSanitizer() { barrierNode(this, "url-redirection") }
+}
+
 /**
  * A URL argument to a call to `HttpResponse.Redirect()` or `Controller.Redirect()`, that is a
  * sink for URL redirects.
@@ -160,27 +165,6 @@ class ContainsUrlSanitizer extends Sanitizer {
   }
 }
 
-/**
- * A check that the URL is relative, and therefore safe for URL redirects.
- */
-private predicate isRelativeUrlSanitizer(Guard guard, Expr e, GuardValue v) {
-  guard =
-    any(PropertyAccess access |
-      access.getProperty().hasFullyQualifiedName("System", "Uri", "IsAbsoluteUri") and
-      e = access.getQualifier() and
-      v.asBooleanValue() = 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"`
@@ -205,16 +189,6 @@ class HostComparisonSanitizer extends Sanitizer {
   }
 }
 
-/**
- * A call to the getter of the RawUrl property, whose value is considered to be safe for URL
- * redirects.
- */
-class RawUrlSanitizer extends Sanitizer {
-  RawUrlSanitizer() {
-    this.getExpr() = any(SystemWebHttpRequestClass r).getRawUrlProperty().getGetter().getACall()
-  }
-}
-
 /**
  * A string concatenation expression, where the left hand side contains the character "?".
  *

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

@@ -7,6 +7,7 @@ import csharp
 private import XSSSinks
 private import semmle.code.csharp.security.Sanitizers
 private import semmle.code.csharp.security.dataflow.flowsources.FlowSources
+private import semmle.code.csharp.dataflow.internal.ExternalFlow
 
 /**
  * Holds if there is tainted flow from `source` to `sink` that may lead to a
@@ -169,6 +170,11 @@ private class SimpleTypeSanitizer extends Sanitizer, SimpleTypeSanitizedExpr { }
 
 private class GuidSanitizer extends Sanitizer, GuidSanitizedExpr { }
 
+/** A sanitizer for XSS defined through Models as Data. */
+private class ExternalXssSanitizer extends Sanitizer {
+  ExternalXssSanitizer() { barrierNode(this, ["html-injection", "js-injection"]) }
+}
+
 /** A call to an HTML encoder. */
 private class HtmlEncodeSanitizer extends Sanitizer {
   HtmlEncodeSanitizer() { this.getExpr() instanceof HtmlSanitizedExpr }

csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ExternalLocationSink.qll

@@ -126,3 +126,11 @@ class LocalFileOutputSink extends ExternalLocationSink {
     )
   }
 }
+
+/**
+ * A sanitizer for writing data to locations that are external to the
+ * application, defined through Models as Data.
+ */
+class ExternalLocationSanitizer extends DataFlow::Node {
+  ExternalLocationSanitizer() { barrierNode(this, "file-content-store") }
+}