JavaScript: Fix a few false positives in PasswordInConfigurationFile.

Author: Max Schaefer

change-notes/1.21/analysis-javascript.md

@@ -33,7 +33,7 @@
 | Expression has no effect       | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. |
 | Incomplete regular expression for hostnames | More results | This rule now tracks regular expressions for host names further. |
 | Incomplete string escaping or encoding | More results | This rule now considers the flow of regular expressions literals, and it no longer flags the removal of trailing newlines. |
-| Password in configuration file | Fewer false positive results | This query now excludes passwords that are inserted into the configuration file using a templating mechanism. |
+| Password in configuration file | Fewer false positive results | This query now excludes passwords that are inserted into the configuration file using a templating mechanism or read from environment variables. |
 | Replacement of a substring with itself | More results | This rule now considers the flow of regular expressions literals. |
 | Server-side URL redirect       | Fewer false-positive results | This rule now treats URLs as safe in more cases where the hostname cannot be tampered with. |
 | Type confusion through parameter tampering | Fewer false-positive results | This rule now recognizes additional emptiness checks. |

javascript/ql/src/Security/CWE-313/PasswordInConfigurationFile.ql

@@ -35,10 +35,14 @@ predicate config(string key, string val, Locatable valElement) {
 
 /**
  * Holds if file `f` should be excluded because it looks like it may be
- * a dictionary file, or a test or example.
+ * an API specification, a dictionary file, or a test or example.
  */
 predicate exclude(File f) {
-  f.getRelativePath().regexpMatch(".*(^|/)(lang(uage)?s?|locales?|tests?|examples?)/.*")
+  f.getRelativePath().regexpMatch("(?i).*(^|/)(lang(uage)?s?|locales?|tests?|examples?|i18n)/.*")
+  or
+  f.getStem().regexpMatch("(?i)translations?")
+  or
+  f.getExtension().toLowerCase() = "raml"
 }
 
 from string key, string val, Locatable valElement
@@ -48,11 +52,14 @@ where
   // exclude possible templates
   not val.regexpMatch(Templating::getDelimiterMatchingRegexp()) and
   (
-    key.toLowerCase() = "password"
+    key.toLowerCase() = "password" and
+    // exclude interpolations of environment variables
+    not val.regexpMatch("\\$\\w+|\\$[{(].+[)}]|%.*%")
     or
     key.toLowerCase() != "readme" and
-    // look for `password=...`, but exclude `password=;` and `password="$(...)"`
-    val.regexpMatch("(?is).*password\\s*=(?!\\s*;)(?!\"?[$`]).*")
+    // look for `password=...`, but exclude `password=;`, `password="$(...)"`,
+    // `password=%s` and `password==`
+    val.regexpMatch("(?is).*password\\s*=(?!\\s*;)(?!\"?[$`])(?!%s)(?!=).*")
   ) and
   not exclude(valElement.getFile())
 select valElement, "Avoid plaintext passwords in configuration files."

javascript/ql/test/query-tests/Security/CWE-313/PasswordInConfigurationFile.expected

@@ -1 +1,2 @@
 | mysql-config.json:4:16:4:23 | "secret" | Avoid plaintext passwords in configuration files. |
+| tst4.json:2:10:2:38 | "script ... ecret'" | Avoid plaintext passwords in configuration files. |

javascript/ql/test/query-tests/Security/CWE-313/i18n/de.json

@@ -0,0 +1,3 @@
+{
+  "password": "Passwort"
+}

javascript/ql/test/query-tests/Security/CWE-313/locales/de.json

@@ -0,0 +1,3 @@
+{
+  "password": "Passwort"
+}

javascript/ql/test/query-tests/Security/CWE-313/tst.raml

@@ -0,0 +1 @@
+password: string

javascript/ql/test/query-tests/Security/CWE-313/tst1.json

@@ -0,0 +1,3 @@
+{
+  "password": "$pwd"
+}

javascript/ql/test/query-tests/Security/CWE-313/tst2.json

@@ -0,0 +1,3 @@
+{
+  "password": "%pwd%"
+}

javascript/ql/test/query-tests/Security/CWE-313/tst3.json

@@ -0,0 +1,3 @@
+{
+  "password": "${pwd:foo}"
+}

javascript/ql/test/query-tests/Security/CWE-313/tst4.json

@@ -0,0 +1,3 @@
+{
+  "cmd": "script.sh password='secret'"
+}