Merge pull request #5908 from erik-krogh/protoLib

JS: Add library input as source to js/prototype-polluting-assignment

Author: erik-krogh

javascript/change-notes/2020-05-17-prototype-assignment.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* The `js/prototype-polluting-assignment` query now flags assignments that may modify 
+  the built-in Object prototype where the property name originates from library input.

javascript/ql/lib/semmle/javascript/PackageExports.qll

@@ -11,11 +11,36 @@ private import semmle.javascript.internal.CachedStages
  * Gets a parameter that is a library input to a top-level package.
  */
 cached
-DataFlow::ParameterNode getALibraryInputParameter() {
+DataFlow::SourceNode getALibraryInputParameter() {
   Stages::Taint::ref() and
   exists(int bound, DataFlow::FunctionNode func |
-    func = getAValueExportedByPackage().getABoundFunctionValue(bound) and
+    func = getAValueExportedByPackage().getABoundFunctionValue(bound)
+  |
     result = func.getParameter(any(int arg | arg >= bound))
+    or
+    result = getAnArgumentsRead(func.getFunction())
+  )
+}
+
+private DataFlow::SourceNode getAnArgumentsRead(Function func) {
+  exists(DataFlow::PropRead read |
+    not read.getPropertyName() = "length" and
+    result = read
+  |
+    read.getBase() = func.getArgumentsVariable().getAnAccess().flow()
+    or
+    exists(DataFlow::MethodCallNode call |
+      call =
+        DataFlow::globalVarRef("Array")
+            .getAPropertyRead("prototype")
+            .getAPropertyRead("slice")
+            .getAMethodCall("call")
+      or
+      call = DataFlow::globalVarRef("Array").getAMethodCall("from")
+    |
+      call.getArgument(0) = func.getArgumentsVariable().getAnAccess().flow() and
+      call.flowsTo(read.getBase())
+    )
   )
 }
 

javascript/ql/lib/semmle/javascript/filters/ClassifyFiles.qll

@@ -45,6 +45,49 @@ private predicate looksLikeExterns(TopLevel tl) {
   )
 }
 
+/**
+ * Holds if `f` contains generated or minified code.
+ */
+predicate isGeneratedCodeFile(File f) { isGenerated(f.getATopLevel()) }
+
+/**
+ * Holds if `f` contains test code.
+ */
+predicate isTestFile(File f) {
+  exists(Test t | t.getFile() = f)
+  or
+  exists(string stemExt | stemExt = "test" or stemExt = "spec" |
+    f = getTestFile(any(File orig), stemExt)
+  )
+  or
+  f.getAbsolutePath().regexpMatch(".*/__(mocks|tests)__/.*")
+}
+
+/**
+ * Holds if `f` contains externs declarations.
+ */
+predicate isExternsFile(File f) {
+  (f.getATopLevel().isExterns() or looksLikeExterns(f.getATopLevel()))
+}
+
+/**
+ * Holds if `f` contains library code.
+ */
+predicate isLibaryFile(File f) { f.getATopLevel() instanceof FrameworkLibraryInstance }
+
+/**
+ * Holds if `f` contains template code.
+ */
+predicate isTemplateFile(File f) {
+  exists(JSParseError err | maybeCausedByTemplate(err) | f = err.getFile())
+  or
+  // Polymer templates
+  exists(HTML::Element elt | elt.getName() = "template" |
+    f = elt.getFile() and
+    not f.getExtension() = "vue"
+  )
+}
+
 /**
  * Holds if `f` is classified as belonging to `category`.
  *
@@ -55,33 +98,15 @@ private predicate looksLikeExterns(TopLevel tl) {
  *   - `"library"`: `f` contains library code;
  *   - `"template"`: `f` contains template code.
  */
+pragma[inline]
 predicate classify(File f, string category) {
-  isGenerated(f.getATopLevel()) and category = "generated"
-  or
-  (
-    exists(Test t | t.getFile() = f)
-    or
-    exists(string stemExt | stemExt = "test" or stemExt = "spec" |
-      f = getTestFile(any(File orig), stemExt)
-    )
-    or
-    f.getAbsolutePath().regexpMatch(".*/__(mocks|tests)__/.*")
-  ) and
-  category = "test"
+  isGeneratedCodeFile(f) and category = "generated"
   or
-  (f.getATopLevel().isExterns() or looksLikeExterns(f.getATopLevel())) and
-  category = "externs"
+  isTestFile(f) and category = "test"
   or
-  f.getATopLevel() instanceof FrameworkLibraryInstance and category = "library"
+  isExternsFile(f) and category = "externs"
   or
-  exists(JSParseError err | maybeCausedByTemplate(err) |
-    f = err.getFile() and category = "template"
-  )
+  isLibaryFile(f) and category = "library"
   or
-  // Polymer templates
-  exists(HTML::Element elt | elt.getName() = "template" |
-    f = elt.getFile() and
-    category = "template" and
-    not f.getExtension() = "vue"
-  )
+  isTemplateFile(f) and category = "template"
 }

javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentCustomizations.qll

@@ -13,7 +13,12 @@ module PrototypePollutingAssignment {
   /**
    * A data flow source for untrusted data from which the special `__proto__` property name may be arise.
    */
-  abstract class Source extends DataFlow::Node { }
+  abstract class Source extends DataFlow::Node {
+    /**
+     * Gets a string that describes the type of source.
+     */
+    abstract string describe();
+  }
 
   /**
    * A data flow sink for prototype-polluting assignments or untrusted property names.
@@ -44,6 +49,8 @@ module PrototypePollutingAssignment {
       this = any(DataFlow::PropWrite write).getBase()
       or
       this = any(ExtendCall c).getDestinationOperand()
+      or
+      this = any(DeleteExpr del).getOperand().flow().(DataFlow::PropRef).getBase()
     }
 
     override DataFlow::FlowLabel getAFlowLabel() { result instanceof ObjectPrototype }
@@ -52,5 +59,18 @@ module PrototypePollutingAssignment {
   /** A remote flow source or location.{hash,search} as a taint source. */
   private class DefaultSource extends Source {
     DefaultSource() { this instanceof RemoteFlowSource }
+
+    override string describe() { result = "user controlled input" }
+  }
+
+  import semmle.javascript.PackageExports as Exports
+
+  /**
+   * A parameter of an exported function, seen as a source prototype-polluting assignment.
+   */
+  class ExternalInputSource extends Source, DataFlow::SourceNode {
+    ExternalInputSource() { this = Exports::getALibraryInputParameter() }
+
+    override string describe() { result = "library input" }
   }
 }

javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll

@@ -10,7 +10,8 @@
 private import javascript
 private import semmle.javascript.DynamicPropertyAccess
 private import semmle.javascript.dataflow.InferredTypes
-private import PrototypePollutingAssignmentCustomizations::PrototypePollutingAssignment
+import PrototypePollutingAssignmentCustomizations::PrototypePollutingAssignment
+private import semmle.javascript.filters.ClassifyFiles as ClassifyFiles
 
 // Materialize flow labels
 private class ConcreteObjectPrototype extends ObjectPrototype {
@@ -31,7 +32,10 @@ class Configuration extends TaintTracking::Configuration {
     node instanceof Sanitizer
     or
     // Concatenating with a string will in practice prevent the string `__proto__` from arising.
-    node instanceof StringOps::ConcatenationRoot
+    exists(StringOps::ConcatenationRoot root | node = root |
+      // Exclude the string coercion `"" + node` from this filter.
+      not node.(StringOps::ConcatenationNode).isCoercion()
+    )
     or
     node instanceof DataFlow::ThisNode
     or
@@ -79,6 +83,29 @@ class Configuration extends TaintTracking::Configuration {
       inlbl.isTaint() and
       outlbl instanceof ObjectPrototype
     )
+    or
+    DataFlow::localFieldStep(pred, succ) and inlbl = outlbl
+  }
+
+  override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
+    super.hasFlowPath(source, sink) and
+    // require that there is a path without unmatched return steps
+    DataFlow::hasPathWithoutUnmatchedReturn(source, sink) and
+    // filter away paths that start with library inputs and end with a write to a fixed property.
+    not exists(ExternalInputSource src, Sink snk, DataFlow::PropWrite write |
+      source.getNode() = src and sink.getNode() = snk
+    |
+      snk = write.getBase() and
+      (
+        // fixed property name
+        exists(write.getPropertyName())
+        or
+        // non-string property name (likely number)
+        exists(Expr prop | prop = write.getPropertyNameExpr() |
+          not prop.analyze().getAType() = TTString()
+        )
+      )
+    )
   }
 
   override predicate isLabeledBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {
@@ -95,7 +122,8 @@ class Configuration extends TaintTracking::Configuration {
     guard instanceof InstanceofCheck or
     guard instanceof IsArrayCheck or
     guard instanceof TypeofCheck or
-    guard instanceof EqualityCheck
+    guard instanceof EqualityCheck or
+    guard instanceof IncludesCheck
   }
 }
 
@@ -108,7 +136,8 @@ private DataFlow::SourceNode prototypeLessObject(DataFlow::TypeTracker t) {
   t.start() and
   // We assume the argument to Object.create is not Object.prototype, since most
   // users wouldn't bother to call Object.create in that case.
-  result = DataFlow::globalVarRef("Object").getAMemberCall("create")
+  result = DataFlow::globalVarRef("Object").getAMemberCall("create") and
+  not result.getFile() instanceof TestFile
   or
   // Allow use of SharedFlowSteps to track a bit further
   exists(DataFlow::Node mid |
@@ -119,6 +148,14 @@ private DataFlow::SourceNode prototypeLessObject(DataFlow::TypeTracker t) {
   exists(DataFlow::TypeTracker t2 | result = prototypeLessObject(t2).track(t2, t))
 }
 
+/**
+ * A test file.
+ * Objects created in such files are ignored in the `prototypeLessObject` predicate.
+ */
+private class TestFile extends File {
+  TestFile() { ClassifyFiles::isTestFile(this) }
+}
+
 /** Holds if `Object.prototype` has a member named `prop`. */
 private predicate isPropertyPresentOnObjectPrototype(string prop) {
   exists(ExternalInstanceMemberDecl decl |
@@ -215,3 +252,15 @@ private class EqualityCheck extends TaintTracking::SanitizerGuardNode, DataFlow:
     outcome = astNode.getPolarity().booleanNot()
   }
 }
+
+/**
+ * Sanitizer guard of the form `x.includes("__proto__")`.
+ */
+private class IncludesCheck extends TaintTracking::LabeledSanitizerGuardNode, InclusionTest {
+  IncludesCheck() { this.getContainedNode().mayHaveStringValue("__proto__") }
+
+  override predicate sanitizes(boolean outcome, Expr e) {
+    e = getContainerNode().asExpr() and
+    outcome = getPolarity().booleanNot()
+  }
+}

javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll

@@ -50,14 +50,14 @@ module UnsafeShellCommandConstruction {
   /**
    * A parameter of an exported function, seen as a source for shell command constructed from library input.
    */
-  class ExternalInputSource extends Source, DataFlow::ParameterNode {
+  class ExternalInputSource extends Source, DataFlow::SourceNode {
     ExternalInputSource() {
       this = Exports::getALibraryInputParameter() and
       not (
         // looks to be on purpose.
-        this.getName() = ["cmd", "command"]
+        this.(DataFlow::ParameterNode).getName() = ["cmd", "command"]
         or
-        this.getName().regexpMatch(".*(Cmd|Command)$") // ends with "Cmd" or "Command"
+        this.(DataFlow::ParameterNode).getName().regexpMatch(".*(Cmd|Command)$") // ends with "Cmd" or "Command"
       )
     }
   }

javascript/ql/lib/semmle/javascript/security/performance/PolynomialReDoSCustomizations.qll

@@ -124,7 +124,7 @@ module PolynomialReDoS {
   /**
    * A parameter of an exported function, seen as a source for polynomial-redos.
    */
-  class ExternalInputSource extends Source, DataFlow::ParameterNode {
+  class ExternalInputSource extends Source, DataFlow::SourceNode {
     ExternalInputSource() { this = Exports::getALibraryInputParameter() }
 
     override string getKind() { result = "library" }

javascript/ql/src/Security/CWE-915/PrototypePollutingAssignment.ql

@@ -24,4 +24,4 @@ from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
 where cfg.hasFlowPath(source, sink)
 select sink, source, sink,
   "This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@.",
-  source.getNode(), "here"
+  source.getNode(), source.getNode().(Source).describe()