Refactor the ScriptEngine query and the Rhino code injection query into one

Author: luchua-bc

java/ql/src/experimental/Security/CWE/CWE-094/RhinoInjection.qhelp

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>The JavaScript Engine API has been available since the release of Java 6, which allows
+  applications to interact with scripts written in languages such as JavaScript. It serves
+  as an embedded scripting engine inside Java applications which allows Java-to-JavaScript
+  interoperability and provides a seamless integration between the two languages. If an 
+  expression is built using attacker-controlled data, and then evaluated in a powerful 
+  context, it may allow the attacker to run arbitrary code.</p>
+</overview>
+
+<recommendation>
+<p>In general, including user input in a JavaScript Engine expression should be avoided.
+  If user input must be included in the expression, it should be then evaluated in a safe
+  context that doesn't allow arbitrary code invocation. Use "Cloudbees Rhino Sandbox" or 
+  sandboxing with SecurityManager or use <a href="https://www.graalvm.org/">graalvm</a> 
+  instead.</p>
+</recommendation>
+
+<example>
+<p>The following code could execute random JavaScript code in <code>ScriptEngine</code></p>
+<sample src="ScriptEngine.java" />
+<sample src="NashornScriptEngine.java" />
+
+<p>The following example shows two ways of using Rhino expression. In the 'BAD' case,
+  an unsafe context is initialized with <code>initStandardObjects</code> that allows arbitrary
+  Java code to be executed. In the 'GOOD' case, a safe context is initialized with 
+  <code>initSafeStandardObjects</code> or <code>setClassShutter</code>.</p>
+  <sample src="RhinoInjection.java" />
+</example>
+
+<references>
+<li>
+CERT coding standard: <a href="https://wiki.sei.cmu.edu/confluence/display/java/IDS52-J.+Prevent+code+injection">ScriptEngine code injection</a>
+</li>
+<li>
+  Mozilla Rhino: <a href="https://github.com/mozilla/rhino">Rhino: JavaScript in Java</a>
+</li>
+<li>
+  Rhino Sandbox: <a href="https://github.com/javadelight/delight-rhino-sandbox">A sandbox to execute JavaScript code with Rhino in Java</a>
+</li>
+<li>
+  GuardRails: <a href="https://docs.guardrails.io/docs/en/vulnerabilities/java/insecure_use_of_dangerous_function">Code Injection</a>
+</li>
+</references>
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-094/RhinoInjection.ql

@@ -0,0 +1,93 @@
+/**
+ * @name Injection in JavaScript Engine
+ * @description Evaluation of a user-controlled malicious JavaScript or Java expression in
+ *              JavaScript Engine may lead to remote code execution.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/unsafe-eval
+ * @tags security
+ *       external/cwe/cwe-094
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import DataFlow::PathGraph
+
+class ScriptEngineMethod extends Method {
+  ScriptEngineMethod() {
+    this.getDeclaringType().getASupertype*().hasQualifiedName("javax.script", "ScriptEngine") and
+    this.hasName("eval")
+  }
+}
+
+/** The context class `org.mozilla.javascript.Context` of Rhino JavaScript Engine. */
+class RhinoContext extends RefType {
+  RhinoContext() { this.hasQualifiedName("org.mozilla.javascript", "Context") }
+}
+
+/**
+ * A method that evaluates a Rhino expression.
+ */
+class RhinoEvaluateExpressionMethod extends Method {
+  RhinoEvaluateExpressionMethod() {
+    this.getDeclaringType().getAnAncestor*() instanceof RhinoContext and
+    (
+      hasName("evaluateString") or
+      hasName("evaluateReader")
+    )
+  }
+}
+
+predicate scriptEngine(MethodAccess ma, Expr sink) {
+  exists(Method m | m = ma.getMethod() |
+    m instanceof ScriptEngineMethod and
+    sink = ma.getArgument(0)
+  )
+}
+
+/**
+ * Holds if `ma` has Rhino code injection vulnerabilities.
+ */
+predicate evaluateRhinoExpression(MethodAccess ma, Expr sink) {
+  exists(RhinoEvaluateExpressionMethod m | m = ma.getMethod() |
+    sink = ma.getArgument(1) and // The second argument is the JavaScript or Java input
+    not exists(MethodAccess ca |
+      (
+        ca.getMethod().hasName("initSafeStandardObjects") // safe mode
+        or
+        ca.getMethod().hasName("setClassShutter") // `ClassShutter` constraint is enforced
+      ) and
+      ma.getQualifier() = ca.getQualifier().(VarAccess).getVariable().getAnAccess()
+    )
+  )
+}
+
+class ScriptInjectionSink extends DataFlow::ExprNode {
+  ScriptInjectionSink() {
+    scriptEngine(_, this.getExpr()) or
+    evaluateRhinoExpression(_, this.getExpr())
+  }
+
+  MethodAccess getMethodAccess() {
+    scriptEngine(result, this.getExpr()) or
+    evaluateRhinoExpression(result, this.getExpr())
+  }
+}
+
+class ScriptInjectionConfiguration extends TaintTracking::Configuration {
+  ScriptInjectionConfiguration() { this = "ScriptInjectionConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource
+    or
+    source instanceof LocalUserInput
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof ScriptInjectionSink }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, ScriptInjectionConfiguration conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode().(ScriptInjectionSink).getMethodAccess(), source, sink,
+  "JavaScript Engine evaluate $@.", source.getNode(), "user input"