Merge pull request github#2778 from erik-krogh/FalsySanitizer

Approved by asgerf

Author: semmle-qlci

javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql

@@ -188,12 +188,7 @@ class PropNameTracking extends DataFlow::Configuration {
   override predicate isBarrier(DataFlow::Node node) {
     super.isBarrier(node)
     or
-    exists(ConditionGuardNode guard, SsaRefinementNode refinement |
-      node = DataFlow::ssaDefinitionNode(refinement) and
-      refinement.getGuard() = guard and
-      guard.getTest() instanceof VarAccess and
-      guard.getOutcome() = false
-    )
+    node instanceof DataFlow::VarAccessBarrier
   }
 
   override predicate isBarrierGuard(DataFlow::BarrierGuardNode node) {

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

@@ -1480,3 +1480,18 @@ private class AdditionalBarrierGuardCall extends AdditionalBarrierGuardNode, Dat
 
   override predicate appliesTo(Configuration cfg) { f.appliesTo(cfg) }
 }
+
+/**
+  * A guard node for a variable in a negative condition, such as `x` in `if(!x)`.
+  * Can be added to a `isBarrier` in a data-flow configuration to block flow through such checks.
+  */
+class VarAccessBarrier extends DataFlow::Node {
+  VarAccessBarrier() {
+    exists(ConditionGuardNode guard, SsaRefinementNode refinement |
+      this = DataFlow::ssaDefinitionNode(refinement) and
+      refinement.getGuard() = guard and
+      guard.getTest() instanceof VarAccess and
+      guard.getOutcome() = false
+    )
+  }
+}

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -89,7 +89,8 @@ module TaintTracking {
 
     final override predicate isBarrier(DataFlow::Node node) {
       super.isBarrier(node) or
-      isSanitizer(node)
+      isSanitizer(node) or
+      node instanceof DataFlow::VarAccessBarrier
     }
 
     final override predicate isBarrierEdge(DataFlow::Node source, DataFlow::Node sink) {

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

@@ -355,6 +355,11 @@ module TaintedPath {
     }
   }
 
+  /**
+   * A guard node for a variable in a negative condition, such as `x` in `if(!x)`.
+   */
+  private class VarAccessBarrier extends Sanitizer, DataFlow::VarAccessBarrier { }
+
   /**
    * A source of remote user input, considered as a flow source for
    * tainted-path vulnerabilities.

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -79,6 +79,8 @@ typeInferenceMismatch
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:45:8:45:8 | x |
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x |
 | sanitizer-guards.js:68:11:68:18 | source() | sanitizer-guards.js:75:8:75:8 | x |
+| sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:81:8:81:8 | x |
+| sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:84:10:84:10 | x |
 | spread.js:2:15:2:22 | source() | spread.js:4:8:4:19 | { ...taint } |
 | spread.js:2:15:2:22 | source() | spread.js:5:8:5:43 | { f: 'h ... orld' } |
 | spread.js:2:15:2:22 | source() | spread.js:7:8:7:19 | [ ...taint ] |

javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected

@@ -54,6 +54,9 @@
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x |
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:52:10:52:10 | x |
 | sanitizer-guards.js:68:11:68:18 | source() | sanitizer-guards.js:75:8:75:8 | x |
+| sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:81:8:81:8 | x |
+| sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:84:10:84:10 | x |
+| sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:86:7:86:7 | x |
 | thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field |
 | thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 |
 | tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |

javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js

@@ -74,3 +74,15 @@ function phi2() {
   }
   sink(x); // NOT OK
 }
+
+function falsy() {
+  let x = source();
+  
+  sink(x); // NOT OK
+  
+  if (x) {
+    sink(x); // OK (for taint-tracking)
+  } else {
+	sink(x); // NOT OK
+  }
+}

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected

@@ -115,9 +115,32 @@ var server = http.createServer(function(req, res) {
 
 });
 
+var server = http.createServer(function(req, res) {
+  let path = url.parse(req.url, true).query.path;
+  
+  if (path) { // sanitization
+    path = path.replace(/[\]\[*,;'"`<>\\?\/]/g, ''); // remove all invalid characters from states plus slashes
+    path = path.replace(/\.\./g, ''); // remove all ".."
+  }
+  
+  res.write(fs.readFileSync(path));  // OK. Is sanitized above.
+});
+
+var server = http.createServer(function(req, res) {
+  let path = url.parse(req.url, true).query.path;
+  
+  if (!path) {
+    
+  } else { // sanitization
+	  path = path.replace(/[\]\[*,;'"`<>\\?\/]/g, ''); // remove all invalid characters from states plus slashes
+    path = path.replace(/\.\./g, ''); // remove all ".."
+  }
+  
+  res.write(fs.readFileSync(path));  // OK. Is sanitized above.
+});
+
 var server = http.createServer(function(req, res) {
 	let path = url.parse(req.url, true).query.path;
 
 	require('send')(req, path); // NOT OK
-
 });