Merge pull request #4921 from erik-krogh/moreShellSan

Approved by esbena

Author: codeql-ci

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

@@ -161,16 +161,19 @@ module UnsafeShellCommandConstruction {
     }
   }
 
+  /**
+   * Gets an unsafe shell character.
+   */
+  private string getAShellChar() {
+    result = ["&", "`", "$", "|", ">", "<", "#", ";", "(", ")", "[", "]", "\n"]
+  }
+
   /**
    * A chain of replace calls that replaces all unsafe chars for shell-commands.
    */
   class ChainSanitizer extends Sanitizer, IncompleteBlacklistSanitizer::StringReplaceCallSequence {
     ChainSanitizer() {
-      forall(string char |
-        char = ["&", "`", "$", "|", ">", "<", "#", ";", "(", ")", "[", "]", "\n"]
-      |
-        this.getAMember().getAReplacedString() = char
-      )
+      forall(string char | char = getAShellChar() | this.getAMember().getAReplacedString() = char)
     }
   }
 
@@ -208,4 +211,50 @@ module UnsafeShellCommandConstruction {
       e = x
     }
   }
+
+  private import semmle.javascript.dataflow.internal.AccessPaths
+  private import semmle.javascript.dataflow.InferredTypes
+
+  /**
+   * Holds if `instance` is an instance of the access-path `ap`, and there exists a guard
+   * that ensures that `instance` is not equal to `char`.
+   *
+   * For example if `ap` is `str[i]` and `char` is `<`:
+   * ```JavaScript
+   * if (str[i] !== "<" && ...) {
+   *   var foo = str[i]; // <- `instance`
+   * }
+   * ```
+   * or
+   * ```JavaScript
+   * if (!(str[i] == "<" || ...)) {
+   *   var foo = str[i]; // <- `instance`
+   * }
+   * ```
+   */
+  private predicate blocksCharInAccess(AccessPath ap, string char, Expr instance) {
+    exists(BasicBlock bb, ConditionGuardNode guard, EqualityTest test |
+      test.getAnOperand().mayHaveStringValue(char) and
+      char = getAShellChar() and
+      guard.getTest() = test and
+      guard.dominates(bb) and
+      test.getAnOperand() = ap.getAnInstance() and
+      instance = ap.getAnInstanceIn(bb) and
+      guard.getOutcome() != test.getPolarity()
+    )
+  }
+
+  /**
+   * A sanitizer for a single character, where the character cannot be an unsafe shell character.
+   */
+  class SanitizedChar extends Sanitizer, DataFlow::ValueNode {
+    override PropAccess astNode;
+
+    SanitizedChar() {
+      exists(AccessPath ap | this.asExpr() = ap.getAnInstance() |
+        forall(string char | char = getAShellChar() | blocksCharInAccess(ap, char, astNode))
+      ) and
+      astNode.getPropertyNameExpr().analyze().getTheType() = TTNumber()
+    }
+  }
 }

javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected

@@ -201,6 +201,10 @@ nodes
 | lib/lib.js:361:20:361:34 | opts.learn_args |
 | lib/lib.js:366:28:366:42 | this.learn_args |
 | lib/lib.js:366:28:366:42 | this.learn_args |
+| lib/lib.js:405:39:405:42 | name |
+| lib/lib.js:405:39:405:42 | name |
+| lib/lib.js:406:22:406:25 | name |
+| lib/lib.js:406:22:406:25 | name |
 edges
 | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
 | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
@@ -436,6 +440,10 @@ edges
 | lib/lib.js:361:20:361:23 | opts | lib/lib.js:361:20:361:34 | opts.learn_args |
 | lib/lib.js:361:20:361:34 | opts.learn_args | lib/lib.js:366:28:366:42 | this.learn_args |
 | lib/lib.js:361:20:361:34 | opts.learn_args | lib/lib.js:366:28:366:42 | this.learn_args |
+| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
+| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
+| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
+| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
 #select
 | lib/lib2.js:4:10:4:25 | "rm -rf " + name | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name | $@ based on library input is later used in $@. | lib/lib2.js:4:10:4:25 | "rm -rf " + name | String concatenation | lib/lib2.js:4:2:4:26 | cp.exec ... + name) | shell command |
 | lib/lib2.js:8:10:8:25 | "rm -rf " + name | lib/lib2.js:7:32:7:35 | name | lib/lib2.js:8:22:8:25 | name | $@ based on library input is later used in $@. | lib/lib2.js:8:10:8:25 | "rm -rf " + name | String concatenation | lib/lib2.js:8:2:8:26 | cp.exec ... + name) | shell command |
@@ -493,3 +501,4 @@ edges
 | lib/lib.js:340:10:340:26 | "rm -rf " + id(n) | lib/lib.js:339:39:339:39 | n | lib/lib.js:340:22:340:26 | id(n) | $@ based on library input is later used in $@. | lib/lib.js:340:10:340:26 | "rm -rf " + id(n) | String concatenation | lib/lib.js:340:2:340:27 | cp.exec ...  id(n)) | shell command |
 | lib/lib.js:351:10:351:27 | "rm -rf " + unsafe | lib/lib.js:349:29:349:34 | unsafe | lib/lib.js:351:22:351:27 | unsafe | $@ based on library input is later used in $@. | lib/lib.js:351:10:351:27 | "rm -rf " + unsafe | String concatenation | lib/lib.js:351:2:351:28 | cp.exec ... unsafe) | shell command |
 | lib/lib.js:366:17:366:56 | "learn  ... + model | lib/lib.js:360:20:360:23 | opts | lib/lib.js:366:28:366:42 | this.learn_args | $@ based on library input is later used in $@. | lib/lib.js:366:17:366:56 | "learn  ... + model | String concatenation | lib/lib.js:367:3:367:18 | cp.exec(command) | shell command |
+| lib/lib.js:406:10:406:25 | "rm -rf " + name | lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name | $@ based on library input is later used in $@. | lib/lib.js:406:10:406:25 | "rm -rf " + name | String concatenation | lib/lib.js:406:2:406:26 | cp.exec ... + name) | shell command |

javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js

@@ -368,3 +368,43 @@ MyTrainer.prototype = {
 	}
 };
 module.exports.MyTrainer = MyTrainer;
+
+
+function yetAnohterSanitizer(str) {
+	const s = str || '';
+	let result = '';
+	for (let i = 0; i <= 2000; i++) {
+		if (!(s[i] === undefined ||
+			s[i] === '>' ||
+			s[i] === '<' ||
+			s[i] === '*' ||
+			s[i] === '?' ||
+			s[i] === '[' ||
+			s[i] === ']' ||
+			s[i] === '|' ||
+			s[i] === '˚' ||
+			s[i] === '$' ||
+			s[i] === ';' ||
+			s[i] === '&' ||
+			s[i] === '(' ||
+			s[i] === ')' ||
+			s[i] === ']' ||
+			s[i] === '#' ||
+			s[i] === '\\' ||
+			s[i] === '\t' ||
+			s[i] === '\n' ||
+			s[i] === '\'' ||
+			s[i] === '`' ||
+			s[i] === '"')) {
+			result = result + s[i];
+		}
+	}
+	return result;
+}
+
+module.exports.sanitizer3 = function (name) {
+	cp.exec("rm -rf " + name); // NOT OK
+
+	var sanitized = yetAnohterSanitizer(name);
+	cp.exec("rm -rf " + sanitized); // OK
+}