Apply suggestions from code review

Added suggestion from atorralba.

Co-authored-by: Tony Torralba <[email protected]>

Author: masterofnow

java/ql/src/experimental/Security/CWE/CWE-470/LoadClassNoSignatureCheck.ql

@@ -6,6 +6,7 @@
  *              potentially leading to arbitrary code execution.
  * @problem.severity error
  * @precision high
+ * @kind problem
  * @id java/unsafe-reflection
  * @tags security
  *       experimental
@@ -16,55 +17,39 @@ import java
 import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.TaintTracking
 
-ControlFlowNode getControlFlowNodeSuccessor(ControlFlowNode node)
-{
-    result = node.getASuccessor()
-}
 
 MethodAccess getClassLoaderReachableMethodAccess(DataFlow::Node node)
 {
-    exists(
-        MethodAccess maGetClassLoader, ControlFlowNode cfnGetClassLoader, ControlFlowNode cfnSuccessor |
+    exists(MethodCall maGetClassLoader |
         maGetClassLoader.getCallee().getName() = "getClassLoader" and
         maGetClassLoader.getQualifier() = node.asExpr() and
-        maGetClassLoader.getControlFlowNode() = cfnGetClassLoader and
-        //cfnGetClassLoader.getASuccessor+() = cfnSuccessor and
-        getControlFlowNodeSuccessor+(cfnGetClassLoader) = cfnSuccessor and
-        cfnSuccessor instanceof MethodAccess and
-        result = cfnSuccessor.(MethodAccess)
+        result = maGetClassLoader.getControlFlowNode().getASuccessor+()
     )
 }
 
 MethodAccess getDangerousReachableMethodAccess(MethodAccess ma)
 {
-    (ma.getCallee().hasName("getMethod") or
-     ma.getCallee().hasName("getDeclaredMethod")) and
-    (( 
-        exists(MethodAccess maInvoke | 
-            //ma.getControlFlowNode().getASuccessor*() = maInvoke and
-            getControlFlowNodeSuccessor+(ma.getControlFlowNode()) = maInvoke and
-            maInvoke.getCallee().hasName("invoke") and
-            result = maInvoke
-            )
-    ) or
-    (
-        exists(AssignExpr ae, VarAccess va1, VarAccess va2, MethodAccess maInvoke |
-            ae.getSource() = ma and
-            ae.getDest() = va1 and
-            maInvoke.getQualifier() = va2 and
-            va1.getVariable() = va2.getVariable() and
-            result = maInvoke
-            )
-    ))
+  ma.getCallee().hasName(["getMethod", "getDeclaredMethod"]) and
+  (
+    result = ma.getControlFlowNode().getASuccessor*() and
+    result.getCallee().hasName("invoke")
+    or
+    exists(AssignExpr ae |
+      ae.getSource() = ma and
+      ae.getDest().(VarAccess).getVariable() =
+        result.getQualifier().(VarAccess).getVariable()
+    )
+  )
 }
 
 module SignaturePackageConfig implements DataFlow::ConfigSig {
     predicate isSource(DataFlow::Node source) {
-        exists(MethodAccess maCheckSignatures |
-            maCheckSignatures.getCallee().getDeclaringType().getQualifiedName() = "android.content.pm.PackageManager" and
-            maCheckSignatures.getCallee().getName() = "checkSignatures" and
-            source.asExpr() = maCheckSignatures.getArgument(0)
-            )
+exists(MethodCall maCheckSignatures |
+      maCheckSignatures
+          .getMethod()
+          .hasQualifiedName("android.content.pm", "PackageManager", "checkSignatures") and
+      source.asExpr() = maCheckSignatures.getArgument(0)
+    )
     }
 
     predicate isSink(DataFlow::Node sink) {
@@ -81,31 +66,20 @@ module SigPkgCfg = TaintTracking::Global<SignaturePackageConfig>;
 
 predicate isSignaturesChecked(MethodAccess maCreatePackageContext)
 {
-    exists(DataFlow::Node source, DataFlow::Node sink |
-        SigPkgCfg::flow(source, sink) and
-        sink.asExpr() = maCreatePackageContext.getArgument(0)
-        )
+  SigPkgCfg::flowToExpr(maCreatePackageContext.getArgument(0))
 }
 
-from 
-    MethodAccess maCreatePackageContext, 
-    LocalVariableDeclExpr lvdePackageContext,
-    DataFlow::Node sinkPackageContext,
-    MethodAccess maGetMethod,
-    MethodAccess maInvoke 
+from
+  MethodCall maCreatePackageContext, LocalVariableDeclExpr lvdePackageContext,
+  Expr sinkPackageContext, MethodCall maGetMethod, MethodCall maInvoke
 where
-    (maCreatePackageContext.getCallee().getDeclaringType().getQualifiedName() = "android.content.ContextWrapper" or
-     maCreatePackageContext.getCallee().getDeclaringType().getQualifiedName() = "android.content.Context") and 
-    maCreatePackageContext.getCallee().getName() = "createPackageContext" and
-    not isSignaturesChecked(maCreatePackageContext) and
-    lvdePackageContext.getEnclosingStmt() = maCreatePackageContext.getEnclosingStmt() and
-    TaintTracking::localTaint(DataFlow::exprNode(lvdePackageContext.getAnAccess()), sinkPackageContext) and
-    getClassLoaderReachableMethodAccess(sinkPackageContext) = maGetMethod and
-    getDangerousReachableMethodAccess(maGetMethod) = maInvoke
-select
-    lvdePackageContext, 
-    sinkPackageContext, 
-    maGetMethod,
-    maInvoke,
-    "Potential arbitary code execution due to class loading without package signature checking."
+  maCreatePackageContext
+      .getMethod()
+      .hasQualifiedName("android.content", ["ContextWrapper", "Context"], "createPackageContext") and
+  not isSignaturesChecked(maCreatePackageContext) and
+  lvdePackageContext.getEnclosingStmt() = maCreatePackageContext.getEnclosingStmt() and
+  TaintTracking::localExprTaint(lvdePackageContext.getAnAccess(), sinkPackageContext) and
+  getClassLoaderReachableMethodCall(sinkPackageContext) = maGetMethod and
+  getGetMethodMethodCall(maGetMethod) = maInvoke
+select maInvoke, "Potential arbitary code execution due to $@ without $@ signature checking.", sinkPackageContext, "class loading", sinkPackageContext, "package"