C#: Address review comments.

Author: calumgrant

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,10 +11,8 @@ 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.Primitives
 private import semmle.code.csharp.frameworks.microsoft.AspNetCore
 
-
 /** A data flow source of remote user input. */
 abstract class RemoteFlowSource extends DataFlow::Node {
   /** Gets a string that describes the type of this remote flow source. */
@@ -31,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
@@ -64,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). */
@@ -116,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). */
@@ -126,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" }
 }
 
 /**
@@ -139,15 +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`). */
 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
@@ -168,66 +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. */
 class ActionMethodParameter extends RemoteFlowSource, DataFlow::ParameterNode {
   ActionMethodParameter() {
     exists(Parameter p |
       p = this.getParameter() and
-      p.fromSource() |
-      p = any(Controller c).getAnActionMethod().getAParameter()
-      or
+      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 MicrosoftAspNetCoreHttpHttpRequest or
+      t instanceof MicrosoftAspNetCoreHttpQueryCollection or
       t instanceof MicrosoftAspNetCoreHttpQueryString
-      |
-      this.getExpr().(Call).getTarget().getDeclaringType() = t
-      or 
+    |
+      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()
+          .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" }
+  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.fromSource()
+    |
       p = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod().getAParameter()
     )
   }
 
-  override
-  string getSourceType() { result = "ASP.NET Core MVC action method parameter" }
+  override string getSourceType() { result = "ASP.NET Core MVC action method parameter" }
 }