Merge pull request #359 from calumgrant/cs/with-stubs

C#: Sources and sinks for ASP.NET Core

Author: hvitved

change-notes/1.19/analysis-csharp.md

@@ -3,9 +3,9 @@
 ## General improvements
 
 * Control flow graph improvements:
-  * The control flow graph construction now takes simple Boolean conditions on local scope variables into account. For example, in `if (b) x = 0; if (b) x = 1;`, the control flow graph will reflect that taking the `true` (resp. `false`) branch in the first condition implies taking the same branch in the second condition. In effect, the first assignment to `x` will now be identified as being dead.
+* The control flow graph construction now takes simple Boolean conditions on local scope variables into account. For example, in `if (b) x = 0; if (b) x = 1;`, the control flow graph will reflect that taking the `true` (resp. `false`) branch in the first condition implies taking the same branch in the second condition. In effect, the first assignment to `x` will now be identified as being dead.
   * Code that is only reachable from a constant failing assertion, such as `Debug.Assert(false)`, is considered to be unreachable.
-  
+
 ## New queries
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
@@ -16,6 +16,7 @@
 
 | Inconsistent lock sequence (`cs/inconsistent-lock-sequence`) | More results | This query now finds inconsistent lock sequences globally across calls. |
 | Local scope variable shadows member (`cs/local-shadows-member`) | Fewer results | Results have been removed where a constructor parameter shadows a member, because the parameter is probably used to initialize the member. |
+| Cross-site scripting (`cs/web/xss`) | More results | This query now finds cross-site scripting vulnerabilities in ASP.NET Core applications. |
 | *@name of query (Query ID)*| *Impact on results*    | *How/why the query has changed*                                  |
 
 

csharp/ql/src/semmle/code/csharp/dataflow/LibraryTypeDataFlow.qll

@@ -1746,3 +1746,28 @@ class SystemNetWebUtilityFlow extends LibraryTypeDataFlow, SystemNetWebUtility {
     preservesValue = false
   }
 }
+
+/**
+ * The `StringValues` class used in many .NET Core libraries. Requires special `LibraryTypeDataFlow` flow.
+ */
+class StringValues extends Struct {
+  StringValues() { this.hasQualifiedName("Microsoft.Extensions.Primitives", "StringValues") }
+}
+
+/**
+ * Custom flow through StringValues.StringValues library class
+ */
+class StringValuesFlow extends LibraryTypeDataFlow, StringValues {
+  override predicate callableFlow(
+    CallableFlowSource source, CallableFlowSink sink, SourceDeclarationCallable c,
+    boolean preservesValue
+  ) {
+    c = any(Callable ca | this = ca.getDeclaringType()) and
+    (
+      source = any(CallableFlowSourceArg a) or
+      source = any(CallableFlowSourceQualifier q)
+    ) and
+    sink = any(CallableFlowSinkReturn r) and
+    preservesValue = false
+  }
+}

csharp/ql/src/semmle/code/csharp/dataflow/flowsources/Remote.qll

@@ -11,6 +11,7 @@ private import semmle.code.csharp.frameworks.system.web.Services
 private import semmle.code.csharp.frameworks.system.web.ui.WebControls
 private import semmle.code.csharp.frameworks.WCF
 private import semmle.code.csharp.frameworks.microsoft.Owin
+private import semmle.code.csharp.frameworks.microsoft.AspNetCore
 
 /** A data flow source of remote user input. */
 abstract class RemoteFlowSource extends DataFlow::Node {
@@ -28,9 +29,8 @@ class AspNetQueryStringMember extends Member {
       t instanceof SystemWebHttpRequestClass or
       t instanceof SystemNetHttpListenerRequestClass or
       t instanceof SystemWebHttpRequestBaseClass
-      |
-      this = t.getProperty(getHttpRequestFlowPropertyNames())
-      or
+    |
+      this = t.getProperty(getHttpRequestFlowPropertyNames()) or
       this.(Field).getType() = t or
       this.(Property).getType() = t or
       this.(Callable).getReturnType() = t
@@ -61,47 +61,43 @@ class AspNetQueryStringRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow
       t instanceof SystemWebHttpRequestClass or
       t instanceof SystemNetHttpListenerRequestClass or
       t instanceof SystemWebHttpRequestBaseClass
-      |
+    |
       // A request object can be indexed, so taint the object as well
       this.getExpr().getType() = t
     )
     or
     this.getExpr() = any(AspNetQueryStringMember m).getAnAccess()
   }
 
-  override
-  string getSourceType() { result = "ASP.NET query string" }
+  override string getSourceType() { result = "ASP.NET query string" }
 }
 
 /** A data flow source of remote user input (ASP.NET unvalidated request data). */
-class AspNetUnvalidatedQueryStringRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::ExprNode {
+class AspNetUnvalidatedQueryStringRemoteFlowSource extends AspNetRemoteFlowSource,
+  DataFlow::ExprNode {
   AspNetUnvalidatedQueryStringRemoteFlowSource() {
     this.getExpr() = any(SystemWebUnvalidatedRequestValues c).getAProperty().getGetter().getACall() or
-    this.getExpr() = any(SystemWebUnvalidatedRequestValuesBase c).getAProperty().getGetter().getACall()
+    this.getExpr() = any(SystemWebUnvalidatedRequestValuesBase c)
+          .getAProperty()
+          .getGetter()
+          .getACall()
   }
 
-  override
-  string getSourceType() { result = "ASP.NET unvalidated request data" }
+  override string getSourceType() { result = "ASP.NET unvalidated request data" }
 }
 
 /** A data flow source of remote user input (ASP.NET user input). */
 class AspNetUserInputRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::ExprNode {
-  AspNetUserInputRemoteFlowSource() {
-    getType() instanceof SystemWebUIWebControlsTextBoxClass
-  }
+  AspNetUserInputRemoteFlowSource() { getType() instanceof SystemWebUIWebControlsTextBoxClass }
 
-  override
-  string getSourceType() { result = "ASP.NET user input" }
+  override string getSourceType() { result = "ASP.NET user input" }
 }
 
 /** A data flow source of remote user input (WCF based web service). */
 class WcfRemoteFlowSource extends RemoteFlowSource, DataFlow::ParameterNode {
-  WcfRemoteFlowSource() {
-    exists(OperationMethod om | om.getAParameter() = this.getParameter())
-  }
+  WcfRemoteFlowSource() { exists(OperationMethod om | om.getAParameter() = this.getParameter()) }
 
-  override
-  string getSourceType() { result = "web service input" }
+  override string getSourceType() { result = "web service input" }
 }
 
 /** A data flow source of remote user input (ASP.NET web service). */
@@ -113,8 +109,7 @@ class AspNetServiceRemoteFlowSource extends RemoteFlowSource, DataFlow::Paramete
     )
   }
 
-  override
-  string getSourceType() { result = "ASP.NET web service input" }
+  override string getSourceType() { result = "ASP.NET web service input" }
 }
 
 /** A data flow source of remote user input (ASP.NET request message). */
@@ -123,8 +118,7 @@ class SystemNetHttpRequestMessageRemoteFlowSource extends RemoteFlowSource, Data
     getType() instanceof SystemWebHttpRequestMessageClass
   }
 
-  override
-  string getSourceType() { result = "ASP.NET request message" }
+  override string getSourceType() { result = "ASP.NET request message" }
 }
 
 /**
@@ -136,17 +130,15 @@ class MicrosoftOwinStringFlowSource extends RemoteFlowSource, DataFlow::ExprNode
     this.getExpr() = any(MicrosoftOwinString owinString).getValueProperty().getGetter().getACall()
   }
 
-  override
-  string getSourceType() { result = "Microsoft Owin request or query string" }
+  override string getSourceType() { result = "Microsoft Owin request or query string" }
 }
 
-/**
- * A data flow source of remote user input (`Microsoft Owin IOwinRequest`).
- */
+/** A data flow source of remote user input (`Microsoft Owin IOwinRequest`). */
 class MicrosoftOwinRequestRemoteFlowSource extends RemoteFlowSource, DataFlow::ExprNode {
   MicrosoftOwinRequestRemoteFlowSource() {
     exists(Property p, MicrosoftOwinIOwinRequestClass owinRequest |
-      this.getExpr() = p.getGetter().getACall() |
+      this.getExpr() = p.getGetter().getACall()
+    |
       p = owinRequest.getAcceptProperty() or
       p = owinRequest.getBodyProperty() or
       p = owinRequest.getCacheControlProperty() or
@@ -167,23 +159,62 @@ class MicrosoftOwinRequestRemoteFlowSource extends RemoteFlowSource, DataFlow::E
     )
   }
 
-  override
-  string getSourceType() { result = "Microsoft Owin request" }
+  override string getSourceType() { result = "Microsoft Owin request" }
 }
 
-/**
- * A parameter to an Mvc controller action method, viewed as a source of remote user input.
- */
+/** A parameter to an Mvc controller action method, viewed as a source of remote user input. */
 class ActionMethodParameter extends RemoteFlowSource, DataFlow::ParameterNode {
   ActionMethodParameter() {
     exists(Parameter p |
       p = this.getParameter() and
-      p.fromSource() |
+      p.fromSource()
+    |
       p = any(Controller c).getAnActionMethod().getAParameter() or
       p = any(ApiController c).getAnActionMethod().getAParameter()
     )
   }
 
-  override
-  string getSourceType() { result = "ASP.NET MVC action method parameter" }
+  override string getSourceType() { result = "ASP.NET MVC action method parameter" }
+}
+
+/** A data flow source of remote user input (ASP.NET Core). */
+abstract class AspNetCoreRemoteFlowSource extends RemoteFlowSource { }
+
+/** A data flow source of remote user input (ASP.NET query collection). */
+class AspNetCoreQueryRemoteFlowSource extends AspNetCoreRemoteFlowSource, DataFlow::ExprNode {
+  AspNetCoreQueryRemoteFlowSource() {
+    exists(ValueOrRefType t |
+      t instanceof MicrosoftAspNetCoreHttpHttpRequest or
+      t instanceof MicrosoftAspNetCoreHttpQueryCollection or
+      t instanceof MicrosoftAspNetCoreHttpQueryString
+    |
+      this.getExpr().(Call).getTarget().getDeclaringType() = t or
+      this.asExpr().(Access).getTarget().getDeclaringType() = t
+    )
+    or
+    exists(Call c |
+      c
+          .getTarget()
+          .getDeclaringType()
+          .hasQualifiedName("Microsoft.AspNetCore.Http", "IQueryCollection") and
+      c.getTarget().getName() = "TryGetValue" and
+      this.asExpr() = c.getArgumentForName("value")
+    )
+  }
+
+  override string getSourceType() { result = "ASP.NET Core query string" }
+}
+
+/** A parameter to a `Mvc` controller action method, viewed as a source of remote user input. */
+class AspNetCoreActionMethodParameter extends RemoteFlowSource, DataFlow::ParameterNode {
+  AspNetCoreActionMethodParameter() {
+    exists(Parameter p |
+      p = this.getParameter() and
+      p.fromSource()
+    |
+      p = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod().getAParameter()
+    )
+  }
+
+  override string getSourceType() { result = "ASP.NET Core MVC action method parameter" }
 }