Merge pull request github#6203 from smowton/smowton/admin/avoid-config-imports-from-qlls

smowton · web-flow · commit 6823855e9cac · 2021-07-02T11:27:27.000+01:00
Java: Reduce DataFlow Configuration pollution from Random.qll and JexlInjection.qll
diff --git a/java/change-notes/2021-07-02-split-queries.md b/java/change-notes/2021-07-02-split-queries.md
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Library `semmle.code.java.security.Random` is split into `RandomQuery`, for use by randomness-related queries, and `RandomValueSource`, for use by libraries wishing to augment the built-in set of random value sources. Any code importing `Random` will need changing to import one or other of these.
diff --git a/java/ql/src/Likely Bugs/Arithmetic/BadAbsOfRandom.ql b/java/ql/src/Likely Bugs/Arithmetic/BadAbsOfRandom.ql
@@ -11,7 +11,7 @@
  */
 
 import java
-import semmle.code.java.security.Random
+import semmle.code.java.security.RandomQuery
 
 from MethodAccess ma, Method abs, Method nextIntOrLong, RandomDataSource nma
 where
diff --git a/java/ql/src/Likely Bugs/Arithmetic/RandomUsedOnce.ql b/java/ql/src/Likely Bugs/Arithmetic/RandomUsedOnce.ql
@@ -13,7 +13,7 @@
  */
 
 import java
-import semmle.code.java.security.Random
+import semmle.code.java.security.RandomQuery
 
 from RandomDataSource ma
 where ma.getQualifier() instanceof ClassInstanceExpr
diff --git a/java/ql/src/Security/CWE/CWE-094/JexlInjection.ql b/java/ql/src/Security/CWE/CWE-094/JexlInjection.ql
@@ -12,27 +12,9 @@
  */
 
 import java
-import semmle.code.java.dataflow.FlowSources
-import semmle.code.java.security.JexlInjection
+import semmle.code.java.security.JexlInjectionQuery
 import DataFlow::PathGraph
 
-/**
- * A taint-tracking configuration for unsafe user input
- * that is used to construct and evaluate a JEXL expression.
- * It supports both JEXL 2 and 3.
- */
-class JexlInjectionConfig extends TaintTracking::Configuration {
-  JexlInjectionConfig() { this = "JexlInjectionConfig" }
-
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node sink) { sink instanceof JexlEvaluationSink }
-
-  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
-    any(JexlInjectionAdditionalTaintStep c).step(node1, node2)
-  }
-}
-
 from DataFlow::PathNode source, DataFlow::PathNode sink, JexlInjectionConfig conf
 where conf.hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "JEXL injection from $@.", source.getNode(), "this user input"
diff --git a/java/ql/src/Security/CWE/CWE-129/ArraySizing.qll b/java/ql/src/Security/CWE/CWE-129/ArraySizing.qll
@@ -1,7 +1,7 @@
 import java
 import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.DefUse
-import semmle.code.java.security.Random
+import semmle.code.java.security.RandomDataSource
 private import BoundingChecks
 
 /**
diff --git a/java/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql b/java/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql
@@ -14,7 +14,7 @@
 
 import java
 import semmle.code.java.dataflow.TaintTracking
-import semmle.code.java.security.Random
+import semmle.code.java.security.RandomQuery
 import semmle.code.java.security.SecurityTests
 import ArithmeticCommon
 import DataFlow::PathGraph
diff --git a/java/ql/src/Security/CWE/CWE-335/PredictableSeed.ql b/java/ql/src/Security/CWE/CWE-335/PredictableSeed.ql
@@ -11,7 +11,7 @@
  */
 
 import java
-import semmle.code.java.security.Random
+import semmle.code.java.security.RandomQuery
 
 from GetRandomData da, RValue use, PredictableSeedExpr source
 where
diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll
@@ -77,6 +77,7 @@ private import FlowSummary
  */
 private module Frameworks {
   private import internal.ContainerFlow
+  private import semmle.code.java.frameworks.android.XssSinks
   private import semmle.code.java.frameworks.ApacheHttp
   private import semmle.code.java.frameworks.apache.Collections
   private import semmle.code.java.frameworks.apache.Lang
@@ -91,10 +92,9 @@ private module Frameworks {
   private import semmle.code.java.frameworks.spring.SpringBeans
   private import semmle.code.java.security.ResponseSplitting
   private import semmle.code.java.security.InformationLeak
-  private import semmle.code.java.security.XSS
+  private import semmle.code.java.security.JexlInjectionSinkModels
   private import semmle.code.java.security.LdapInjection
   private import semmle.code.java.security.XPath
-  private import semmle.code.java.security.JexlInjection
   private import semmle.code.java.frameworks.android.SQLite
   private import semmle.code.java.frameworks.Jdbc
   private import semmle.code.java.frameworks.SpringJdbc
diff --git a/java/ql/src/semmle/code/java/dataflow/RangeAnalysis.qll b/java/ql/src/semmle/code/java/dataflow/RangeAnalysis.qll
@@ -68,7 +68,7 @@ private import SSA
 private import RangeUtils
 private import semmle.code.java.dataflow.internal.rangeanalysis.SsaReadPositionCommon
 private import semmle.code.java.controlflow.internal.GuardsLogic
-private import semmle.code.java.security.Random
+private import semmle.code.java.security.RandomDataSource
 private import SignAnalysis
 private import ModulusAnalysis
 private import semmle.code.java.Reflection
diff --git a/java/ql/src/semmle/code/java/frameworks/android/XssSinks.qll b/java/ql/src/semmle/code/java/frameworks/android/XssSinks.qll
@@ -0,0 +1,16 @@
+/** Provides XSS sink models relating to the `android.webkit.WebView` class. */
+
+import java
+private import semmle.code.java.dataflow.ExternalFlow
+
+/** CSV sink models representing methods susceptible to XSS attacks. */
+private class DefaultXssSinkModel extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        "android.webkit;WebView;false;loadData;;;Argument[0];xss",
+        "android.webkit;WebView;false;loadUrl;;;Argument[0];xss",
+        "android.webkit;WebView;false;loadDataWithBaseURL;;;Argument[1];xss"
+      ]
+  }
+}
diff --git a/java/ql/src/semmle/code/java/security/JexlInjectionQuery.qll b/java/ql/src/semmle/code/java/security/JexlInjectionQuery.qll
@@ -2,6 +2,7 @@
 
 import java
 import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.dataflow.ExternalFlow
 
 /**
@@ -15,46 +16,6 @@ private class DefaultJexlEvaluationSink extends JexlEvaluationSink {
   DefaultJexlEvaluationSink() { sinkNode(this, "jexl") }
 }
 
-private class DefaultJexlInjectionSinkModel extends SinkModelCsv {
-  override predicate row(string row) {
-    row =
-      [
-        // JEXL2
-        "org.apache.commons.jexl2;JexlEngine;false;getProperty;(JexlContext,Object,String);;Argument[2];jexl",
-        "org.apache.commons.jexl2;JexlEngine;false;getProperty;(Object,String);;Argument[1];jexl",
-        "org.apache.commons.jexl2;JexlEngine;false;setProperty;(JexlContext,Object,String,Object);;Argument[2];jexl",
-        "org.apache.commons.jexl2;JexlEngine;false;setProperty;(Object,String,Object);;Argument[1];jexl",
-        "org.apache.commons.jexl2;Expression;false;evaluate;;;Argument[-1];jexl",
-        "org.apache.commons.jexl2;Expression;false;callable;;;Argument[-1];jexl",
-        "org.apache.commons.jexl2;JexlExpression;false;evaluate;;;Argument[-1];jexl",
-        "org.apache.commons.jexl2;JexlExpression;false;callable;;;Argument[-1];jexl",
-        "org.apache.commons.jexl2;Script;false;execute;;;Argument[-1];jexl",
-        "org.apache.commons.jexl2;Script;false;callable;;;Argument[-1];jexl",
-        "org.apache.commons.jexl2;JexlScript;false;execute;;;Argument[-1];jexl",
-        "org.apache.commons.jexl2;JexlScript;false;callable;;;Argument[-1];jexl",
-        "org.apache.commons.jexl2;UnifiedJEXL$Expression;false;evaluate;;;Argument[-1];jexl",
-        "org.apache.commons.jexl2;UnifiedJEXL$Expression;false;prepare;;;Argument[-1];jexl",
-        "org.apache.commons.jexl2;UnifiedJEXL$Template;false;evaluate;;;Argument[-1];jexl",
-        // JEXL3
-        "org.apache.commons.jexl3;JexlEngine;false;getProperty;(JexlContext,Object,String);;Argument[2];jexl",
-        "org.apache.commons.jexl3;JexlEngine;false;getProperty;(Object,String);;Argument[1];jexl",
-        "org.apache.commons.jexl3;JexlEngine;false;setProperty;(JexlContext,Object,String);;Argument[2];jexl",
-        "org.apache.commons.jexl3;JexlEngine;false;setProperty;(Object,String,Object);;Argument[1];jexl",
-        "org.apache.commons.jexl3;Expression;false;evaluate;;;Argument[-1];jexl",
-        "org.apache.commons.jexl3;Expression;false;callable;;;Argument[-1];jexl",
-        "org.apache.commons.jexl3;JexlExpression;false;evaluate;;;Argument[-1];jexl",
-        "org.apache.commons.jexl3;JexlExpression;false;callable;;;Argument[-1];jexl",
-        "org.apache.commons.jexl3;Script;false;execute;;;Argument[-1];jexl",
-        "org.apache.commons.jexl3;Script;false;callable;;;Argument[-1];jexl",
-        "org.apache.commons.jexl3;JexlScript;false;execute;;;Argument[-1];jexl",
-        "org.apache.commons.jexl3;JexlScript;false;callable;;;Argument[-1];jexl",
-        "org.apache.commons.jexl3;JxltEngine$Expression;false;evaluate;;;Argument[-1];jexl",
-        "org.apache.commons.jexl3;JxltEngine$Expression;false;prepare;;;Argument[-1];jexl",
-        "org.apache.commons.jexl3;JxltEngine$Template;false;evaluate;;;Argument[-1];jexl"
-      ]
-  }
-}
-
 /**
  * A unit class for adding additional taint steps.
  *
@@ -77,6 +38,23 @@ private class DefaultJexlInjectionAdditionalTaintStep extends JexlInjectionAddit
   }
 }
 
+/**
+ * A taint-tracking configuration for unsafe user input
+ * that is used to construct and evaluate a JEXL expression.
+ * It supports both JEXL 2 and 3.
+ */
+class JexlInjectionConfig extends TaintTracking::Configuration {
+  JexlInjectionConfig() { this = "JexlInjectionConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof JexlEvaluationSink }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    any(JexlInjectionAdditionalTaintStep c).step(node1, node2)
+  }
+}
+
 /**
  * Holds if `n1` to `n2` is a dataflow step that creates a JEXL script using an unsafe engine
  * by calling `tainted.createScript(jexlExpr)`.
diff --git a/java/ql/src/semmle/code/java/security/JexlInjectionSinkModels.qll b/java/ql/src/semmle/code/java/security/JexlInjectionSinkModels.qll
@@ -0,0 +1,43 @@
+/** Provides sink models relating to Expression Langauge (JEXL) injection vulnerabilities. */
+
+private import semmle.code.java.dataflow.ExternalFlow
+
+private class DefaultJexlInjectionSinkModel extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        // JEXL2
+        "org.apache.commons.jexl2;JexlEngine;false;getProperty;(JexlContext,Object,String);;Argument[2];jexl",
+        "org.apache.commons.jexl2;JexlEngine;false;getProperty;(Object,String);;Argument[1];jexl",
+        "org.apache.commons.jexl2;JexlEngine;false;setProperty;(JexlContext,Object,String,Object);;Argument[2];jexl",
+        "org.apache.commons.jexl2;JexlEngine;false;setProperty;(Object,String,Object);;Argument[1];jexl",
+        "org.apache.commons.jexl2;Expression;false;evaluate;;;Argument[-1];jexl",
+        "org.apache.commons.jexl2;Expression;false;callable;;;Argument[-1];jexl",
+        "org.apache.commons.jexl2;JexlExpression;false;evaluate;;;Argument[-1];jexl",
+        "org.apache.commons.jexl2;JexlExpression;false;callable;;;Argument[-1];jexl",
+        "org.apache.commons.jexl2;Script;false;execute;;;Argument[-1];jexl",
+        "org.apache.commons.jexl2;Script;false;callable;;;Argument[-1];jexl",
+        "org.apache.commons.jexl2;JexlScript;false;execute;;;Argument[-1];jexl",
+        "org.apache.commons.jexl2;JexlScript;false;callable;;;Argument[-1];jexl",
+        "org.apache.commons.jexl2;UnifiedJEXL$Expression;false;evaluate;;;Argument[-1];jexl",
+        "org.apache.commons.jexl2;UnifiedJEXL$Expression;false;prepare;;;Argument[-1];jexl",
+        "org.apache.commons.jexl2;UnifiedJEXL$Template;false;evaluate;;;Argument[-1];jexl",
+        // JEXL3
+        "org.apache.commons.jexl3;JexlEngine;false;getProperty;(JexlContext,Object,String);;Argument[2];jexl",
+        "org.apache.commons.jexl3;JexlEngine;false;getProperty;(Object,String);;Argument[1];jexl",
+        "org.apache.commons.jexl3;JexlEngine;false;setProperty;(JexlContext,Object,String);;Argument[2];jexl",
+        "org.apache.commons.jexl3;JexlEngine;false;setProperty;(Object,String,Object);;Argument[1];jexl",
+        "org.apache.commons.jexl3;Expression;false;evaluate;;;Argument[-1];jexl",
+        "org.apache.commons.jexl3;Expression;false;callable;;;Argument[-1];jexl",
+        "org.apache.commons.jexl3;JexlExpression;false;evaluate;;;Argument[-1];jexl",
+        "org.apache.commons.jexl3;JexlExpression;false;callable;;;Argument[-1];jexl",
+        "org.apache.commons.jexl3;Script;false;execute;;;Argument[-1];jexl",
+        "org.apache.commons.jexl3;Script;false;callable;;;Argument[-1];jexl",
+        "org.apache.commons.jexl3;JexlScript;false;execute;;;Argument[-1];jexl",
+        "org.apache.commons.jexl3;JexlScript;false;callable;;;Argument[-1];jexl",
+        "org.apache.commons.jexl3;JxltEngine$Expression;false;evaluate;;;Argument[-1];jexl",
+        "org.apache.commons.jexl3;JxltEngine$Expression;false;prepare;;;Argument[-1];jexl",
+        "org.apache.commons.jexl3;JxltEngine$Template;false;evaluate;;;Argument[-1];jexl"
+      ]
+  }
+}
diff --git a/java/ql/src/semmle/code/java/security/RandomDataSource.qll b/java/ql/src/semmle/code/java/security/RandomDataSource.qll
diff --git a/java/ql/src/semmle/code/java/security/RandomQuery.qll b/java/ql/src/semmle/code/java/security/RandomQuery.qll
diff --git a/java/ql/src/semmle/code/java/security/XSS.qll b/java/ql/src/semmle/code/java/security/XSS.qll
diff --git a/java/ql/test/query-tests/security/CWE-094/JexlInjectionTest.ql b/java/ql/test/query-tests/security/CWE-094/JexlInjectionTest.ql

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+lgtm,codescanning`
	`2`	+* Library `semmle.code.java.security.Random` is split into `RandomQuery`, for use by randomness-related queries, and `RandomValueSource`, for use by libraries wishing to augment the built-in set of random value sources. Any code importing `Random` will need changing to import one or other of these.