JS: share detection of objects with unsafe methods

javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll

@@ -69,15 +69,8 @@ module MethodNameInjection {
     /**
      * Holds if a property of the given object is an unsafe function.
      */
-    predicate isUnsafeBaseObject(DataFlow::SourceNode node) {
-      // eval and friends can be accessed from the global object.
-      node = DataFlow::globalObjectRef()
-      or
-      // 'constructor' property leads to the Function constructor.
-      node.analyze().getAValue() instanceof AbstractCallable
-      or
-      // Assume that a value that is invoked can refer to a function.
-      exists (node.getAnInvocation()) 
+    predicate hasUnsafeMethods(DataFlow::SourceNode node) {
+      PropertyInjection::hasUnsafeMethods(node) // Redefined here so custom queries can override it
     }
 
     /**
@@ -91,7 +84,7 @@ module MethodNameInjection {
     override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel, DataFlow::FlowLabel dstlabel) {
       // Reading a property of the global object or of a function
       exists (DataFlow::PropRead read |
-        isUnsafeBaseObject(read.getBase().getALocalSource()) and
+        hasUnsafeMethods(read.getBase().getALocalSource()) and
         src = read.getPropertyNameExpr().flow() and
         dst = read and
         srclabel = taint() and

javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll

@@ -15,4 +15,18 @@ module PropertyInjection {
       StringConcatenation::getAnOperand(this).asExpr() instanceof ConstantString
     }
   }
+
+  /**
+   * Holds if the methods of the given value are unsafe, such as `eval`.
+   */
+  predicate hasUnsafeMethods(DataFlow::SourceNode node) {
+    // eval and friends can be accessed from the global object.
+    node = DataFlow::globalObjectRef()
+    or
+    // 'constructor' property leads to the Function constructor.
+    node.analyze().getAValue() instanceof AbstractCallable
+    or
+    // Assume that a value that is invoked can refer to a function.
+    exists (node.getAnInvocation())
+  }
 }

javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll

@@ -72,15 +72,6 @@ module RemotePropertyInjection {
       result = " a property name to write to."
     }     
   }
-
-  /**
-   * Holds if the method name injection on the given base object is handled by another query.
-   */
-  private predicate isCoveredByMethodNameInjection(DataFlow::SourceNode node) {
-    node = DataFlow::globalObjectRef()
-    or
-    node.analyze().getAValue() instanceof AbstractCallable
-  }
   
   /**
    * A sink for method calls using dynamically computed method names.   
@@ -89,7 +80,9 @@ module RemotePropertyInjection {
     MethodCallSink() {
       exists (DataFlow::PropRead pr | astNode = pr.getPropertyNameExpr() |
         exists (pr.getAnInvocation()) and
-        not isCoveredByMethodNameInjection(pr.getBase().getALocalSource())
+
+        // Omit sinks covered by the MethodNameInjection query
+        not PropertyInjection::hasUnsafeMethods(pr.getBase().getALocalSource())
       )
     }