Merge pull request #3619 from erik-krogh/CWE022-Correctness

Approved by asgerf

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

@@ -369,6 +369,20 @@ module TaintedPath {
     }
   }
 
+  /**
+   * A check of the form `whitelist.includes(x)` or equivalent, which sanitizes `x` in its "then" branch.
+   */
+  class MembershipTestBarrierGuard extends BarrierGuardNode {
+    MembershipCandidate candidate;
+
+    MembershipTestBarrierGuard() { this = candidate.getTest() }
+
+    override predicate blocks(boolean outcome, Expr e) {
+      candidate = e.flow() and
+      candidate.getTestPolarity() = outcome
+    }
+  }
+
   /**
    * A check of form `x.startsWith(dir)` that sanitizes normalized absolute paths, since it is then
    * known to be in a subdirectory of `dir`.

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

@@ -1,4 +0,0 @@
-| normalizedPaths.js:208:38:208:63 | // OK - ...  anyway | Spurious alert |
-| tainted-string-steps.js:25:43:25:74 | // NOT  ... flagged | Missing alert |
-| tainted-string-steps.js:26:49:26:74 | // OK - ... flagged | Spurious alert |
-| tainted-string-steps.js:28:39:28:70 | // NOT  ... flagged | Missing alert |

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

@@ -1,32 +1,3 @@
 import javascript
 import semmle.javascript.security.dataflow.TaintedPath::TaintedPath
-
-class Assertion extends LineComment {
-  boolean shouldHaveAlert;
-
-  Assertion() {
-    if getText().matches("%NOT OK%")
-    then shouldHaveAlert = true
-    else (
-      getText().matches("%OK%") and shouldHaveAlert = false
-    )
-  }
-
-  predicate shouldHaveAlert() { shouldHaveAlert = true }
-
-  predicate hasAlert() {
-    exists(Configuration cfg, DataFlow::Node src, DataFlow::Node sink, Location loc |
-      cfg.hasFlow(src, sink) and
-      loc = sink.getAstNode().getLocation() and
-      loc.getFile() = getFile() and
-      loc.getEndLine() = getLocation().getEndLine()
-    )
-  }
-}
-
-from Assertion assertion, string message
-where
-  assertion.shouldHaveAlert() and not assertion.hasAlert() and message = "Missing alert"
-  or
-  not assertion.shouldHaveAlert() and assertion.hasAlert() and message = "Spurious alert"
-select assertion, message
+import testUtilities.ConsistencyChecking

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

@@ -14,40 +14,33 @@ var server = http.createServer(function(req, res) {
   // BAD: This could still read any file on the file system
   res.write(fs.readFileSync("/home/user/" + path));
 
-  // BAD: Insufficient sanitisation
   if (path.startsWith("/home/user/"))
-      res.write(fs.readFileSync(path));
+      res.write(fs.readFileSync(path)); // BAD: Insufficient sanitisation
 
-  // BAD: Insufficient sanitisation
   if (path.indexOf("secret") == -1)
-      res.write(fs.readFileSync(path));
+      res.write(fs.readFileSync(path)); // BAD: Insufficient sanitisation
 
-  // BAD: Insufficient sanitisation
   if (fs.existsSync(path))
-      res.write(fs.readFileSync(path));
+      res.write(fs.readFileSync(path)); // BAD: Insufficient sanitisation
 
-  // GOOD: Path is compared to white-list
   if (path === 'foo.txt')
-    res.write(fs.readFileSync(path));
+    res.write(fs.readFileSync(path)); // GOOD: Path is compared to white-list
 
-  // GOOD: Path is compared to white-list
   if (path === 'foo.txt' || path === 'bar.txt')
-    res.write(fs.readFileSync(path));
+    res.write(fs.readFileSync(path)); // GOOD: Path is compared to white-list
 
-  // BAD: Path is incompletely compared to white-list
   if (path === 'foo.txt' || path === 'bar.txt' || someOpaqueCondition())
-    res.write(fs.readFileSync(path));
+    res.write(fs.readFileSync(path)); // BAD: Path is incompletely compared to white-list
 
-  // GOOD: Path is sanitized
   path = sanitize(path);
-  res.write(fs.readFileSync(path));
+  res.write(fs.readFileSync(path)); // GOOD: Path is sanitized
 
   path = url.parse(req.url, true).query.path;
-  // BAD: taint is preserved
+  // GOOD: basename is safe
   res.write(fs.readFileSync(pathModule.basename(path)));
   // BAD: taint is preserved
   res.write(fs.readFileSync(pathModule.dirname(path)));
-  // BAD: taint is preserved
+  // GOOD: extname is safe
   res.write(fs.readFileSync(pathModule.extname(path)));
   // BAD: taint is preserved
   res.write(fs.readFileSync(pathModule.join(path)));

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

@@ -205,7 +205,7 @@ app.get('/join-regression', (req, res) => {
     fs.readFileSync(normalizedPath); // NOT OK
 
   if (normalizedPath.startsWith('/home/user/www') || normalizedPath.startsWith('/home/user/public'))
-    fs.readFileSync(normalizedPath); // OK - but flagged anyway
+    fs.readFileSync(normalizedPath); // OK - but flagged anyway [INCONSISTENCY]
   else
     fs.readFileSync(normalizedPath); // NOT OK
 });

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-array-steps.js

@@ -7,12 +7,11 @@ var fs = require('fs'),
 
 var server = http.createServer(function(req, res) {
   let path = url.parse(req.url, true).query.path;
-  // BAD: taint is preserved
-  res.write(fs.readFileSync(['public', path].join('/')));
-  // BAD: taint is preserved
+  res.write(fs.readFileSync(['public', path].join('/'))); // BAD - but not flagged because we have no array-steps [INCONSISTENCY]
+
   let parts = ['public', path];
   parts = parts.map(x => x.toLowerCase());
-  res.write(fs.readFileSync(parts.join('/')));
+  res.write(fs.readFileSync(parts.join('/'))); // BAD - but not flagged because we have no array-steps [INCONSISTENCY]
 });
 
 server.listen();

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-string-steps.js

@@ -22,10 +22,10 @@ var server = http.createServer(function(req, res) {
 	fs.readFileSync(path.split('/')[i]); // NOT OK
 	fs.readFileSync(path.split(/\//)[i]); // NOT OK
 	fs.readFileSync(path.split("?")[0]); // NOT OK
-	fs.readFileSync(path.split(unknown)[i]); // NOT OK -- but not yet flagged
-	fs.readFileSync(path.split(unknown).whatever); // OK -- but still flagged
+	fs.readFileSync(path.split(unknown)[i]); // NOT OK -- but not yet flagged [INCONSISTENCY]
+	fs.readFileSync(path.split(unknown).whatever); // OK -- but still flagged [INCONSISTENCY]
 	fs.readFileSync(path.split(unknown)); // NOT OK
-	fs.readFileSync(path.split("?")[i]); // NOT OK -- but not yet flagged
+	fs.readFileSync(path.split("?")[i]); // NOT OK -- but not yet flagged [INCONSISTENCY]
 });
 
 server.listen();

javascript/ql/test/testUtilities/ConsistencyChecking.qll

@@ -0,0 +1,158 @@
+import javascript
+
+/**
+ * A configuration for consistency checking.
+ * Used to specify where the alerts are (the positives)
+ * And which files should be included in the consistency-check.
+ *
+ * If no configuration is specified, then the default is that the all sinks from a `DataFlow::Configuration` are alerts, and all files are consistency-checked.
+ */
+abstract class ConsistencyConfiguration extends string {
+  bindingset[this]
+  ConsistencyConfiguration() { any() }
+
+  /**
+   * Gets an alert that should be checked for consistency.
+   * The alert must match with a `NOT OK` comment.
+   *
+   * And likewise a `OK` comment must not have a corresponding alert on the same line.
+   */
+  DataFlow::Node getAnAlert() { result = getASink() }
+
+  /**
+   * Gets a file to include in the consistency checking.
+   */
+  File getAFile() { none() }
+}
+
+/**
+ * A line-comment that asserts whether a result exists at that line or not.
+ * Can optionally include `[INCONSISTENCY]` to indicate that a consistency issue is expected at the location
+ */
+private class AssertionComment extends LineComment {
+  boolean shouldHaveAlert;
+
+  AssertionComment() {
+    if getText().regexpMatch("\\s*(NOT OK|BAD).*")
+    then shouldHaveAlert = true
+    else (
+      getText().regexpMatch("\\s*(OK|GOOD).*") and shouldHaveAlert = false
+    )
+  }
+
+  /**
+   * Holds if there should be an alert at this location
+   */
+  predicate shouldHaveAlert() { shouldHaveAlert = true }
+
+  /**
+   * Holds if a consistency issue is expected at this location.
+   */
+  predicate expectConsistencyError() { getText().matches(["%[INCONSISTENCY]%"]) }
+}
+
+private DataFlow::Node getASink() { exists(DataFlow::Configuration cfg | cfg.hasFlow(_, result)) }
+
+/**
+ * Gets all the alerts for consistency consistency checking.
+ */
+private DataFlow::Node alerts() {
+  result = any(ConsistencyConfiguration res).getAnAlert()
+  or
+  not exists(ConsistencyConfiguration r) and
+  result = getASink()
+}
+
+/**
+ * Gets an alert in `file` at `line`.
+ * The `line` can be either the first or the last line of the alert.
+ * And if no expression exists at `line`, then an alert on the next line is used.
+ */
+private DataFlow::Node getAlert(File file, int line) {
+  result = alerts() and
+  result.getFile() = file and
+  (result.hasLocationInfo(_, _, _, line, _) or result.hasLocationInfo(_, line, _, _, _))
+  or
+  // The comment can be right above the result, so an alert also counts for the line above.
+  not exists(Expr e |
+    e.getFile() = file and [e.getLocation().getStartLine(), e.getLocation().getEndLine()] = line
+  ) and
+  result = alerts() and
+  result.getFile() = file and
+  result.hasLocationInfo(_, line + 1, _, _, _)
+}
+
+/**
+ * Gets a comment that asserts either the existence or the absence of an alert in `file` at `line`.
+ */
+private AssertionComment getComment(File file, int line) {
+  result.getLocation().getEndLine() = line and
+  result.getFile() = file
+}
+
+/**
+ * Holds if there is a false positive in `file` at `line`
+ */
+private predicate falsePositive(File file, int line, AssertionComment comment) {
+  exists(getAlert(file, line)) and
+  comment = getComment(file, line) and
+  not comment.shouldHaveAlert()
+}
+
+/**
+ * Holds if there is a false negative in `file` at `line`
+ */
+private predicate falseNegative(File file, int line, AssertionComment comment) {
+  not exists(getAlert(file, line)) and
+  comment = getComment(file, line) and
+  comment.shouldHaveAlert()
+}
+
+/**
+ * Gets a file that should be included for consistency checking.
+ */
+private File getATestFile() {
+  not exists(any(ConsistencyConfiguration res).getAFile()) and
+  result = any(LineComment comment).getFile()
+  or
+  result = any(ConsistencyConfiguration res).getAFile()
+}
+
+/**
+ * Gets a description of the configuration that has a sink in `file` at `line`.
+ * Or the empty string
+ */
+bindingset[file, line]
+private string getSinkDescription(File file, int line) {
+  not exists(DataFlow::Configuration c | c.hasFlow(_, getAlert(file, line))) and result = ""
+  or
+  exists(DataFlow::Configuration c | c.hasFlow(_, getAlert(file, line)) | result = " for " + c)
+}
+
+/**
+ * Holds if there is a consistency-issue at `location` with description `msg`.
+ * The consistency issue an unexpected false positive/negative.
+ * Or that false positive/negative was expected, and none were found.
+ */
+query predicate consistencyIssue(string location, string msg, string commentText) {
+  exists(File file, int line |
+    file = getATestFile() and location = file.getRelativePath() + ":" + line
+  |
+    exists(AssertionComment comment |
+      comment.getText().trim() = commentText and comment = getComment(file, line)
+    |
+      falsePositive(file, line, comment) and
+      not comment.expectConsistencyError() and
+      msg = "did not expected an alert, but found an alert" + getSinkDescription(file, line)
+      or
+      falseNegative(file, line, comment) and
+      not comment.expectConsistencyError() and
+      msg = "expected an alert, but found none"
+      or
+      not falsePositive(file, line, comment) and
+      not falseNegative(file, line, comment) and
+      comment.expectConsistencyError() and
+      msg = "expected consistency issue, but found no such issue (" + comment.getText().trim() + ")"
+    )
+  )
+}