Include more scenarios and update qldoc

Author: luchua-bc

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

@@ -4,7 +4,7 @@
 <qhelp>
 
 <overview>
-<p>The JavaScript Engine API has been available since the release of Java 6, which allows
+<p>The Java Scripting 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 
@@ -13,11 +13,11 @@
 </overview>
 
 <recommendation>
-<p>In general, including user input in a JavaScript Engine expression should be avoided.
+<p>In general, including user input in a Java Script 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>
+  context that doesn't allow arbitrary code invocation. Use "Cloudbees Rhino Sandbox" or
+  sandboxing with SecurityManager, which will be deprecated in a future release, or use
+  <a href="https://www.graalvm.org/">GraalVM</a> instead.</p>
 </recommendation>
 
 <example>
@@ -36,14 +36,17 @@
 <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>
+GraalVM: <a href="https://www.graalvm.org/reference-manual/js/NashornMigrationGuide/#secure-by-default">Secure by Default</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>
+  GuardRails: <a href="https://docs.guardrails.io/docs/en/vulnerabilities/java/insecure_use_of_dangerous_function#code-injection">Code Injection</a>
 </li>
 </references>
 </qhelp>

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

@@ -1,7 +1,7 @@
 /**
- * @name Injection in JavaScript Engine
+ * @name Injection in Java Script Engine
  * @description Evaluation of a user-controlled malicious JavaScript or Java expression in
- *              JavaScript Engine may lead to remote code execution.
+ *              Java Script Engine may lead to remote code execution.
  * @kind path-problem
  * @problem.severity error
  * @precision high
@@ -14,31 +14,62 @@ import java
 import semmle.code.java.dataflow.FlowSources
 import DataFlow::PathGraph
 
+/** A method of ScriptEngine that allows code injection. */
 class ScriptEngineMethod extends Method {
   ScriptEngineMethod() {
     this.getDeclaringType().getASupertype*().hasQualifiedName("javax.script", "ScriptEngine") and
     this.hasName("eval")
+    or
+    this.getDeclaringType().getASupertype*().hasQualifiedName("javax.script", "Compilable") and
+    this.hasName("compile")
+    or
+    this.getDeclaringType().getASupertype*().hasQualifiedName("javax.script", "ScriptEngineFactory") and
+    this.hasName(["getProgram", "getMethodCallSyntax"])
   }
 }
 
-/** The context class `org.mozilla.javascript.Context` of Rhino JavaScript Engine. */
+/** The context class `org.mozilla.javascript.Context` of Rhino Java Script Engine. */
 class RhinoContext extends RefType {
   RhinoContext() { this.hasQualifiedName("org.mozilla.javascript", "Context") }
 }
 
-/**
- * A method that evaluates a Rhino expression.
- */
+/** A method that evaluates a Rhino expression with `org.mozilla.javascript.Context`. */
 class RhinoEvaluateExpressionMethod extends Method {
   RhinoEvaluateExpressionMethod() {
     this.getDeclaringType().getAnAncestor*() instanceof RhinoContext and
-    (
-      hasName("evaluateString") or
-      hasName("evaluateReader")
-    )
+    this.hasName([
+        "evaluateString", "evaluateReader", "compileFunction", "compileReader", "compileString"
+      ])
   }
 }
 
+/**
+ * A method that compiles a Rhino expression with
+ * `org.mozilla.javascript.optimizer.ClassCompiler`.
+ */
+class RhinoCompileClassMethod extends Method {
+  RhinoCompileClassMethod() {
+    this.getDeclaringType()
+        .getASupertype*()
+        .hasQualifiedName("org.mozilla.javascript.optimizer", "ClassCompiler") and
+    this.hasName("compileToClassFiles")
+  }
+}
+
+/**
+ * A method that defines a Java class from a Rhino expression with
+ * `org.mozilla.javascript.GeneratedClassLoader`.
+ */
+class RhinoDefineClassMethod extends Method {
+  RhinoDefineClassMethod() {
+    this.getDeclaringType()
+        .getASupertype*()
+        .hasQualifiedName("org.mozilla.javascript", "GeneratedClassLoader") and
+    this.hasName("defineClass")
+  }
+}
+
+/** Holds if `ma` is a method access of `ScriptEngineMethod`. */
 predicate scriptEngine(MethodAccess ma, Expr sink) {
   exists(Method m | m = ma.getMethod() |
     m instanceof ScriptEngineMethod and
@@ -47,11 +78,17 @@ predicate scriptEngine(MethodAccess ma, Expr sink) {
 }
 
 /**
- * Holds if `ma` has Rhino code injection vulnerabilities.
+ * Holds if a Rhino expression evaluation method has the code injection vulnerability.
  */
 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
+    (
+      sink = ma.getArgument(1) and // The second argument is the JavaScript or Java input
+      not ma.getMethod().getName() = "compileReader"
+      or
+      sink = ma.getArgument(0) and // The first argument is the input reader
+      ma.getMethod().getName() = "compileReader"
+    ) and
     not exists(MethodAccess ca |
       (
         ca.getMethod().hasName("initSafeStandardObjects") // safe mode
@@ -63,15 +100,34 @@ predicate evaluateRhinoExpression(MethodAccess ma, Expr sink) {
   )
 }
 
+/**
+ * Holds if a Rhino expression compilation method has the code injection vulnerability.
+ */
+predicate compileScript(MethodAccess ma, Expr sink) {
+  exists(RhinoCompileClassMethod m | m = ma.getMethod() | sink = ma.getArgument(0))
+}
+
+/**
+ * Holds if a Rhino class loading method has the code injection vulnerability.
+ */
+predicate defineClass(MethodAccess ma, Expr sink) {
+  exists(RhinoDefineClassMethod m | m = ma.getMethod() | sink = ma.getArgument(1))
+}
+
+/** A sink of script injection. */
 class ScriptInjectionSink extends DataFlow::ExprNode {
   ScriptInjectionSink() {
     scriptEngine(_, this.getExpr()) or
-    evaluateRhinoExpression(_, this.getExpr())
+    evaluateRhinoExpression(_, this.getExpr()) or
+    compileScript(_, this.getExpr()) or
+    defineClass(_, this.getExpr())
   }
 
   MethodAccess getMethodAccess() {
     scriptEngine(result, this.getExpr()) or
-    evaluateRhinoExpression(result, this.getExpr())
+    evaluateRhinoExpression(result, this.getExpr()) or
+    compileScript(result, this.getExpr()) or
+    defineClass(result, this.getExpr())
   }
 }
 
@@ -90,4 +146,4 @@ class ScriptInjectionConfiguration extends TaintTracking::Configuration {
 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"
+  "Java Script Engine evaluate $@.", source.getNode(), "user input"

java/ql/test/experimental/query-tests/security/CWE-094/RhinoServlet.java

@@ -5,9 +5,12 @@
 import javax.servlet.http.HttpServletResponse;
 
 import org.mozilla.javascript.ClassShutter;
+import org.mozilla.javascript.CompilerEnvirons;
 import org.mozilla.javascript.Context;
+import org.mozilla.javascript.DefiningClassLoader;
 import org.mozilla.javascript.Scriptable;
 import org.mozilla.javascript.RhinoException;
+import org.mozilla.javascript.optimizer.ClassCompiler;
 
 /**
  * Servlet implementation class RhinoServlet
@@ -75,4 +78,17 @@ public boolean visibleToScripts(String className) {
             Context.exit();
         }
     }    
+
+    // BAD: allow arbitrary code to be compiled for subsequent execution
+    protected void doGet2(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
+        String code = request.getParameter("code");
+        ClassCompiler compiler = new ClassCompiler(new CompilerEnvirons());
+        Object[] objs = compiler.compileToClassFiles(code, "/sourceLocation", 1, "mainClassName");
+    }
+
+    // BAD: allow arbitrary code to be loaded for subsequent execution
+    protected void doPost2(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
+        String code = request.getParameter("code");
+        Class clazz = new DefiningClassLoader().defineClass("Powerfunc", code.getBytes());
+    }
 }

java/ql/test/experimental/query-tests/security/CWE-094/ScriptEngineTest.java

@@ -33,26 +33,53 @@ public void testCustomScriptEngineReference(String input) throws ScriptException
 		MyCustomScriptEngine engine = (MyCustomScriptEngine) factory.getScriptEngine(new String[] { "-scripting" });
 		Object result = engine.eval(input);
 	}
+
+	public void testScriptEngineCompilable(String input) throws ScriptException {
+		NashornScriptEngineFactory factory = new NashornScriptEngineFactory();
+		Compilable engine = (Compilable) factory.getScriptEngine(new String[] { "-scripting" });
+		CompiledScript script = engine.compile(input);
+		Object result = script.eval();
+	}
+
+	public void testScriptEngineGetProgram(String input) throws ScriptException {
+		ScriptEngineManager scriptEngineManager = new ScriptEngineManager();
+		ScriptEngine engine = scriptEngineManager.getEngineByName("nashorn");
+		String program = engine.getFactory().getProgram(input);
+		Object result = engine.eval(program);
+	}
 
 	public static void main(String[] args) throws ScriptException {
 		new ScriptEngineTest().testWithScriptEngineReference(args[0]);
 		new ScriptEngineTest().testNashornWithScriptEngineReference(args[0]);
 		new ScriptEngineTest().testNashornWithNashornScriptEngineReference(args[0]);
 		new ScriptEngineTest().testCustomScriptEngineReference(args[0]);
+		new ScriptEngineTest().testScriptEngineCompilable(args[0]);
+		new ScriptEngineTest().testScriptEngineGetProgram(args[0]);
 	}
 
 	private static class MyCustomScriptEngine extends AbstractScriptEngine {
-		public Object eval(String var1) throws ScriptException {
-			return null;
-		}
+		public Object eval(String var1) throws ScriptException { return null; }
+
+		@Override
+		public ScriptEngineFactory getFactory() { return null; }
 	}
 
 	private static class MyCustomFactory implements ScriptEngineFactory {
 		public MyCustomFactory() {
-    		}
-    		
-    		public ScriptEngine getScriptEngine() { return null; }
+		}
+
+		@Override
+		public ScriptEngine getScriptEngine() { return null; }
 
 		public ScriptEngine getScriptEngine(String... args) { return null; }
+
+		@Override
+		public String getEngineName() { return null; }
+
+		@Override
+		public String getMethodCallSyntax(final String obj, final String method, final String... args) { return null; }
+
+		@Override
+		public String getProgram(final String... statements) { return null; }
 	}
 }