Merge pull request #1305 from aschackmull/java/abstract-flowsources

Max Schaefer · web-flow · commit 79e01a2de56d · 2019-05-10T11:42:15.000+01:00
Java: Introduce an abstract class RemoteFlowSource to ease customization.
diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql b/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql
@@ -20,7 +20,7 @@ import DataFlow::PathGraph
 class TaintedPathConfig extends TaintTracking::Configuration {
   TaintedPathConfig() { this = "TaintedPathConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
     exists(Expr e | e = sink.asExpr() | e = any(PathCreation p).getInput() and not guarded(e))
diff --git a/java/ql/src/Security/CWE/CWE-078/ExecCommon.qll b/java/ql/src/Security/CWE/CWE-078/ExecCommon.qll
@@ -6,7 +6,7 @@ private class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::C
     this = "ExecCommon::RemoteUserInputToArgumentToExecFlowConfig"
   }
 
-  override predicate isSource(DataFlow::Node src) { src instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec }
 
diff --git a/java/ql/src/Security/CWE/CWE-079/XSS.ql b/java/ql/src/Security/CWE/CWE-079/XSS.ql
@@ -18,7 +18,7 @@ import DataFlow2::PathGraph
 class XSSConfig extends TaintTracking::Configuration2 {
   XSSConfig() { this = "XSSConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof XssSink }
 
diff --git a/java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll b/java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll
@@ -49,7 +49,7 @@ class PersistenceQueryInjectionSink extends QueryInjectionSink {
 private class QueryInjectionFlowConfig extends TaintTracking::Configuration {
   QueryInjectionFlowConfig() { this = "SqlInjectionLib::QueryInjectionFlowConfig" }
 
-  override predicate isSource(DataFlow::Node src) { src instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof QueryInjectionSink }
 
diff --git a/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql b/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql
@@ -18,7 +18,7 @@ class ResponseSplittingConfig extends TaintTracking::Configuration {
   ResponseSplittingConfig() { this = "ResponseSplittingConfig" }
 
   override predicate isSource(DataFlow::Node source) {
-    source instanceof RemoteUserInput and
+    source instanceof RemoteFlowSource and
     not source instanceof WhitelistedSource
   }
 
diff --git a/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.qll b/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.qll
@@ -30,7 +30,7 @@ class HeaderSplittingSink extends DataFlow::ExprNode {
   }
 }
 
-class WhitelistedSource extends RemoteUserInput {
+class WhitelistedSource extends DataFlow::ExprNode {
   WhitelistedSource() {
     this.asExpr().(MethodAccess).getMethod() instanceof HttpServletRequestGetHeaderMethod or
     this.asExpr().(MethodAccess).getMethod() instanceof CookieGetNameMethod
diff --git a/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayConstruction.ql b/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayConstruction.ql
@@ -17,7 +17,7 @@ import DataFlow::PathGraph
 class Conf extends TaintTracking::Configuration {
   Conf() { this = "RemoteUserInputTocanThrowOutOfBoundsDueToEmptyArrayConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
     any(CheckableArrayAccess caa).canThrowOutOfBoundsDueToEmptyArray(sink.asExpr(), _)
diff --git a/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayIndex.ql b/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayIndex.ql
@@ -17,7 +17,7 @@ import DataFlow::PathGraph
 class Conf extends TaintTracking::Configuration {
   Conf() { this = "RemoteUserInputTocanThrowOutOfBoundsDueToEmptyArrayConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
     any(CheckableArrayAccess caa).canThrowOutOfBounds(sink.asExpr())
diff --git a/java/ql/src/Security/CWE/CWE-134/ExternallyControlledFormatString.ql b/java/ql/src/Security/CWE/CWE-134/ExternallyControlledFormatString.ql
@@ -17,7 +17,7 @@ import DataFlow::PathGraph
 class ExternallyControlledFormatStringConfig extends TaintTracking::Configuration {
   ExternallyControlledFormatStringConfig() { this = "ExternallyControlledFormatStringConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
     sink.asExpr() = any(StringFormat formatCall).getFormatArgument()
diff --git a/java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql b/java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql
@@ -32,7 +32,7 @@ predicate sink(ArithExpr exp, VarAccess tainted, string effect) {
 class RemoteUserInputConfig extends TaintTracking::Configuration {
   RemoteUserInputConfig() { this = "ArithmeticTainted.ql:RemoteUserInputConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) { sink(_, sink.asExpr(), _) }
 
diff --git a/java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.ql b/java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.ql
@@ -18,7 +18,7 @@ import DataFlow::PathGraph
 class UnsafeDeserializationConfig extends TaintTracking::Configuration {
   UnsafeDeserializationConfig() { this = "UnsafeDeserializationConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeDeserializationSink }
 }
diff --git a/java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql b/java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql
@@ -18,7 +18,7 @@ import DataFlow::PathGraph
 class UrlRedirectConfig extends TaintTracking::Configuration {
   UrlRedirectConfig() { this = "UrlRedirectConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof UrlRedirectSink }
 }
diff --git a/java/ql/src/Security/CWE/CWE-611/XXE.ql b/java/ql/src/Security/CWE/CWE-611/XXE.ql
@@ -40,7 +40,7 @@ class UnsafeXxeSink extends DataFlow::ExprNode {
 class XxeConfig extends TaintTracking::Configuration {
   XxeConfig() { this = "XXE.ql::XxeConfig" }
 
-  override predicate isSource(DataFlow::Node src) { src instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeXxeSink }
 }
diff --git a/java/ql/src/Security/CWE/CWE-681/NumericCastTainted.ql b/java/ql/src/Security/CWE/CWE-681/NumericCastTainted.ql
@@ -19,7 +19,7 @@ import DataFlow::PathGraph
 private class NumericCastFlowConfig extends TaintTracking::Configuration {
   NumericCastFlowConfig() { this = "NumericCastTainted::RemoteUserInputToNumericNarrowingCastExpr" }
 
-  override predicate isSource(DataFlow::Node src) { src instanceof RemoteUserInput }
+  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
     sink.asExpr() = any(NumericNarrowingCastExpr cast).getExpr()
diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll
@@ -21,68 +21,136 @@ import semmle.code.java.frameworks.Guice
 import semmle.code.java.frameworks.struts.StrutsActions
 import semmle.code.java.frameworks.Thrift
 
-/** Class for `tainted` user input. */
-abstract class UserInput extends DataFlow::Node { }
-
-private predicate variableStep(Expr tracked, VarAccess sink) {
-  exists(VariableAssign def |
-    def.getSource() = tracked and
-    defUsePair(def, sink)
-  )
+/** 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. */
+  abstract string getSourceType();
 }
 
-/** Input that may be controlled by a remote user. */
-class RemoteUserInput extends UserInput {
-  RemoteUserInput() {
+private class RemoteTaintedMethodAccessSource extends RemoteFlowSource {
+  RemoteTaintedMethodAccessSource() {
     this.asExpr().(MethodAccess).getMethod() instanceof RemoteTaintedMethod
-    or
-    // Parameters to RMI methods.
+  }
+
+  override string getSourceType() { result = "network data source" }
+}
+
+private class RmiMethodParameterSource extends RemoteFlowSource {
+  RmiMethodParameterSource() {
     exists(RemoteCallableMethod method |
       method.getAParameter() = this.asParameter() and
       (
         getType() instanceof PrimitiveType or
         getType() instanceof TypeString
       )
     )
-    or
-    // Parameters to Jax WS methods.
+  }
+
+  override string getSourceType() { result = "RMI method parameter" }
+}
+
+private class JaxWsMethodParameterSource extends RemoteFlowSource {
+  JaxWsMethodParameterSource() {
     exists(JaxWsEndpoint endpoint |
       endpoint.getARemoteMethod().getAParameter() = this.asParameter()
     )
-    or
-    // Parameters to Jax Rs methods.
+  }
+
+  override string getSourceType() { result = "Jax WS method parameter" }
+}
+
+private class JaxRsMethodParameterSource extends RemoteFlowSource {
+  JaxRsMethodParameterSource() {
     exists(JaxRsResourceClass service |
       service.getAnInjectableCallable().getAParameter() = this.asParameter() or
       service.getAnInjectableField().getAnAccess() = this.asExpr()
     )
-    or
-    // Reverse DNS. Try not to trigger on `localhost`.
+  }
+
+  override string getSourceType() { result = "Jax Rs method parameter" }
+}
+
+private predicate variableStep(Expr tracked, VarAccess sink) {
+  exists(VariableAssign def |
+    def.getSource() = tracked and
+    defUsePair(def, sink)
+  )
+}
+
+private class ReverseDnsSource extends RemoteFlowSource {
+  ReverseDnsSource() {
+    // Try not to trigger on `localhost`.
     exists(MethodAccess m | m = this.asExpr() |
       m.getMethod() instanceof ReverseDNSMethod and
       not exists(MethodAccess l |
         (variableStep(l, m.getQualifier()) or l = m.getQualifier()) and
         l.getMethod().getName() = "getLocalHost"
       )
     )
-    or
-    //MessageBodyReader
+  }
+
+  override string getSourceType() { result = "reverse DNS lookup" }
+}
+
+private class MessageBodyReaderParameterSource extends RemoteFlowSource {
+  MessageBodyReaderParameterSource() {
     exists(MessageBodyReaderRead m |
       m.getParameter(4) = this.asParameter() or
       m.getParameter(5) = this.asParameter()
     )
-    or
+  }
+
+  override string getSourceType() { result = "MessageBodyReader parameter" }
+}
+
+private class SpringServletInputParameterSource extends RemoteFlowSource {
+  SpringServletInputParameterSource() {
     this.asParameter().getAnAnnotation() instanceof SpringServletInputAnnotation
-    or
+  }
+
+  override string getSourceType() { result = "Spring servlet input parameter" }
+}
+
+private class GuiceRequestParameterSource extends RemoteFlowSource {
+  GuiceRequestParameterSource() {
     exists(GuiceRequestParametersAnnotation a |
       a = this.asParameter().getAnAnnotation() or
       a = this.asExpr().(FieldRead).getField().getAnAnnotation()
     )
-    or
-    exists(Struts2ActionSupportClass c | c.getASetterMethod().getField() = this.asExpr().(FieldRead).getField())
-    or
+  }
+
+  override string getSourceType() { result = "Guice request parameter" }
+}
+
+private class Struts2ActionSupportClassFieldReadSource extends RemoteFlowSource {
+  Struts2ActionSupportClassFieldReadSource() {
+    exists(Struts2ActionSupportClass c |
+      c.getASetterMethod().getField() = this.asExpr().(FieldRead).getField()
+    )
+  }
+
+  override string getSourceType() { result = "Struts2 ActionSupport field" }
+}
+
+private class ThriftIfaceParameterSource extends RemoteFlowSource {
+  ThriftIfaceParameterSource() {
     exists(ThriftIface i | i.getAnImplementingMethod().getAParameter() = this.asParameter())
   }
 
+  override string getSourceType() { result = "Thrift Iface parameter" }
+}
+
+/** Class for `tainted` user input. */
+abstract class UserInput extends DataFlow::Node { }
+
+/**
+ * DEPRECATED: Use `RemoteFlowSource` instead.
+ *
+ * Input that may be controlled by a remote user.
+ */
+class RemoteUserInput extends UserInput {
+  RemoteUserInput() { this instanceof RemoteFlowSource }
+
   /**
    * DEPRECATED: Use a configuration with a defined sink instead.
    *
diff --git a/java/ql/test/library-tests/frameworks/guice/flow.ql b/java/ql/test/library-tests/frameworks/guice/flow.ql
@@ -6,7 +6,7 @@ class Conf extends TaintTracking::Configuration {
   Conf() { this = "conf" }
 
   override predicate isSource(DataFlow::Node src) {
-    src instanceof RemoteUserInput
+    src instanceof RemoteFlowSource
   }
 
   override predicate isSink(DataFlow::Node sink) {

Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,7 @@ private class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::C`
`6`	`6`	`this = "ExecCommon::RemoteUserInputToArgumentToExecFlowConfig"`
`7`	`7`	`}`
`8`	`8`
`9`		`- override predicate isSource(DataFlow::Node src) { src instanceof RemoteUserInput }`
	`9`	`+ override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }`
`10`	`10`
`11`	`11`	`override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec }`
`12`	`12`
Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,7 @@ class ResponseSplittingConfig extends TaintTracking::Configuration {`
`18`	`18`	`ResponseSplittingConfig() { this = "ResponseSplittingConfig" }`
`19`	`19`
`20`	`20`	`override predicate isSource(DataFlow::Node source) {`
`21`		`- source instanceof RemoteUserInput and`
	`21`	`+ source instanceof RemoteFlowSource and`
`22`	`22`	`not source instanceof WhitelistedSource`
`23`	`23`	`}`
`24`	`24`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ class HeaderSplittingSink extends DataFlow::ExprNode {`
`30`	`30`	`}`
`31`	`31`	`}`
`32`	`32`
`33`		`-class WhitelistedSource extends RemoteUserInput {`
	`33`	`+class WhitelistedSource extends DataFlow::ExprNode {`
`34`	`34`	`WhitelistedSource() {`
`35`	`35`	`this.asExpr().(MethodAccess).getMethod() instanceof HttpServletRequestGetHeaderMethod or`
`36`	`36`	`this.asExpr().(MethodAccess).getMethod() instanceof CookieGetNameMethod`