JS: remove flow labels from js/resource-exhaustion

Author: esbena

javascript/ql/src/Security/CWE-770/ResourceExhaustion.qhelp

@@ -33,57 +33,7 @@
 
 		<p>
 
-			The following example allocates a buffer with a user-controlled
-			size.
-
-		</p>
-
-		<sample src="examples/ResourceExhaustion_buffer.js" />
-
-		<p>
-
-			This is problematic since an attacker can choose a size
-			that makes the application run out of memory. Even worse, in older
-			versions of Node.js, this could leak confidential memory.
-
-			To prevent such attacks, limit the buffer size:
-
-		</p>
-
-		<sample src="examples/ResourceExhaustion_buffer_fixed.js" />
-
-	</example>
-
-	<example>
-
-		<p>
-
-			As another example, consider an application that allocates an
-			array with a user-controlled size, and then fills it with values:
-
-		</p>
-
-		<sample src="examples/ResourceExhaustion_array.js" />
-
-		<p>
-			The allocation of the array itself is not problematic since arrays are
-			allocated sparsely, but the subsequent filling of the array will take
-			a long time, causing the application to be unresponsive, or even run
-			out of memory.
-
-			Again, a limit on the size will prevent the attack:
-
-		</p>
-
-		<sample src="examples/ResourceExhaustion_array_fixed.js" />
-
-	</example>
-
-	<example>
-
-		<p>
-
-			Finally, the following example lets a user choose a delay after
+			The following example lets a user choose a delay after
 			which a function is executed:
 
 		</p>
@@ -97,7 +47,7 @@
 			registrations of such delays will therefore use up all of the memory
 			in the application.
 
-			Again, a limit on the delay will prevent the attack:
+			A limit on the delay will prevent the attack:
 
 		</p>
 

javascript/ql/src/Security/CWE-770/examples/ResourceExhaustion_array.js

@@ -19,36 +19,29 @@ module ResourceExhaustion {
   class Configuration extends TaintTracking::Configuration {
     Configuration() { this = "ResourceExhaustion" }
 
-    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
-      source.(Source).getAFlowLabel() = label
-    }
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
 
-    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
-      sink.(Sink).getAFlowLabel() = label
-    }
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-    override predicate isAdditionalFlowStep(
-      DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel,
-      DataFlow::FlowLabel dstlabel
-    ) {
-      dstlabel instanceof Label::Number and
+    override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node dst) {
       isNumericFlowStep(src, dst)
       or
       // reuse most existing taint steps
-      super.isAdditionalFlowStep(src, dst) and
-      not dst.asExpr() instanceof AddExpr and
-      if dst.(DataFlow::MethodCallNode).calls(src, "toString")
-      then dstlabel.isTaint()
-      else srclabel = dstlabel
+      isRestrictedAdditionalTaintStep(src, dst)
     }
 
     override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
       guard instanceof LoopBoundInjection::LengthCheckSanitizerGuard or
-      guard instanceof UpperBoundsCheckSanitizerGuard or
-      guard instanceof TypeTestGuard
+      guard instanceof UpperBoundsCheckSanitizerGuard
     }
   }
 
+  predicate isRestrictedAdditionalTaintStep(DataFlow::Node src, DataFlow::Node dst) {
+    any(TaintTracking::AdditionalTaintStep dts).step(src, dst) and
+    not dst.asExpr() instanceof AddExpr and
+    not dst.(DataFlow::MethodCallNode).calls(src, "toString")
+  }
+
   /**
    * Holds if data may flow from `src` to `dst` as a number.
    */

javascript/ql/src/Security/CWE-770/examples/ResourceExhaustion_array_fixed.js

@@ -10,18 +10,12 @@ module ResourceExhaustion {
   /**
    * A data flow source for resource exhaustion vulnerabilities.
    */
-  abstract class Source extends DataFlow::Node {
-    /** Gets a flow label denoting the type of value for which this is a source. */
-    DataFlow::FlowLabel getAFlowLabel() { result.isTaint() }
-  }
+  abstract class Source extends DataFlow::Node { }
 
   /**
    * A data flow sink for resource exhaustion vulnerabilities.
    */
   abstract class Sink extends DataFlow::Node {
-    /** Gets a flow label denoting the type of value for which this is a sink. */
-    DataFlow::FlowLabel getAFlowLabel() { result instanceof Label::Number }
-
     /**
      * Gets a description of why this is a problematic sink.
      */
@@ -33,59 +27,19 @@ module ResourceExhaustion {
    */
   abstract class Sanitizer extends DataFlow::Node { }
 
-  /**
-   * Provides data flow labels for resource exhaustion vulnerabilities.
-   */
-  module Label {
-    /**
-     * A number data flow label.
-     */
-    class Number extends DataFlow::FlowLabel {
-      Number() { this = "number" }
-    }
-  }
-
   /**
    * A sanitizer that blocks taint flow if the size of a number is limited.
    */
-  class UpperBoundsCheckSanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode,
+  class UpperBoundsCheckSanitizerGuard extends TaintTracking::SanitizerGuardNode,
     DataFlow::ValueNode {
     override RelationalComparison astNode;
 
-    override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
-      label instanceof Label::Number and
-      (
-        true = outcome and
-        e = astNode.getLesserOperand()
-        or
-        false = outcome and
-        e = astNode.getGreaterOperand()
-      )
-    }
-  }
-
-  /**
-   * A test of form `typeof x === "something"`, preventing `x` from being a number in some cases.
-   */
-  class TypeTestGuard extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode {
-    override EqualityTest astNode;
-    Expr x;
-    boolean polarity;
-
-    TypeTestGuard() {
-      // typeof x === "number" sanitizes `x` when it evaluates to false
-      TaintTracking::isTypeofGuard(astNode, x, "number") and
-      polarity = astNode.getPolarity().booleanNot()
+    override predicate sanitizes(boolean outcome, Expr e) {
+      true = outcome and
+      e = astNode.getLesserOperand()
       or
-      // typeof x === "string" sanitizes `x` when it evaluates to true
-      TaintTracking::isTypeofGuard(astNode, x, any(string s | s != "number")) and
-      polarity = astNode.getPolarity()
-    }
-
-    override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
-      polarity = outcome and
-      e = x and
-      label instanceof Label::Number
+      false = outcome and
+      e = astNode.getGreaterOperand()
     }
   }
 
@@ -94,60 +48,6 @@ module ResourceExhaustion {
     RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
   }
 
-  /**
-   * A node that determines the size of a buffer, considered as a data flow sink for resource exhaustion vulnerabilities.
-   */
-  class BufferSizeSink extends Sink {
-    BufferSizeSink() {
-      exists(DataFlow::SourceNode clazz, DataFlow::InvokeNode invk, int index |
-        clazz = DataFlow::globalVarRef("Buffer") and this = invk.getArgument(index)
-      |
-        exists(string name |
-          invk = clazz.getAMemberCall(name) and
-          (
-            name = "from" and index = 2
-            or
-            name = ["alloc", "allocUnsafe", "allocUnsafeSlow"] and index = 0
-          )
-        )
-        or
-        invk = clazz.getAnInvocation() and
-        (
-          invk.getNumArgument() = 1 and
-          index = 0
-          or
-          invk.getNumArgument() = 3 and index = 2
-        )
-      )
-      or
-      this = DataFlow::globalVarRef("SlowBuffer").getAnInstantiation().getArgument(0)
-    }
-
-    override string getProblemDescription() {
-      result = "This creates a buffer with a user-controlled size"
-    }
-  }
-
-  /**
-   * A node that determines the size of an array, considered as a data flow sink for resource exhaustion vulnerabilities.
-   */
-  class DenseArraySizeSink extends Sink {
-    DenseArraySizeSink() {
-      // Arrays are sparse by default, so we must also look at how the array is used
-      exists(DataFlow::ArrayConstructorInvokeNode instance |
-        this = instance.getArgument(0) and
-        instance.getNumArgument() = 1
-      |
-        exists(instance.getAMethodCall(["map", "fill", "join", "toString"])) or
-        instance.flowsToExpr(any(AddExpr p).getAnOperand())
-      )
-    }
-
-    override string getProblemDescription() {
-      result = "This creates an array with a user-controlled length"
-    }
-  }
-
   /**
    * A node that determines the repetitions of a string, considered as a data flow sink for resource exhaustion vulnerabilities.
    */
@@ -159,8 +59,6 @@ module ResourceExhaustion {
       )
     }
 
-    override DataFlow::FlowLabel getAFlowLabel() { any() }
-
     override string getProblemDescription() {
       result = "This creates a string with a user-controlled length"
     }
@@ -175,8 +73,6 @@ module ResourceExhaustion {
       this = LodashUnderscore::member(["delay", "throttle", "debounce"]).getACall().getArgument(1)
     }
 
-    override DataFlow::FlowLabel getAFlowLabel() { any() }
-
     override string getProblemDescription() {
       result = "This creates a timer with a user-controlled duration"
     }