From c16e9a77f3c487b914d5148a5e51a0a42b656caa Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 7 May 2019 17:41:02 +0100 Subject: [PATCH] JavaScript: Fix a few false positives in `PasswordInConfigurationFile`. --- change-notes/1.21/analysis-javascript.md | 2 +- .../CWE-313/PasswordInConfigurationFile.ql | 17 ++++++++++++----- .../PasswordInConfigurationFile.expected | 1 + .../query-tests/Security/CWE-313/i18n/de.json | 3 +++ .../Security/CWE-313/locales/de.json | 3 +++ .../test/query-tests/Security/CWE-313/tst.raml | 1 + .../test/query-tests/Security/CWE-313/tst1.json | 3 +++ .../test/query-tests/Security/CWE-313/tst2.json | 3 +++ .../test/query-tests/Security/CWE-313/tst3.json | 3 +++ .../test/query-tests/Security/CWE-313/tst4.json | 3 +++ .../test/query-tests/Security/CWE-313/tst5.json | 3 +++ .../test/query-tests/Security/CWE-313/tst6.json | 3 +++ 12 files changed, 39 insertions(+), 6 deletions(-) create mode 100644 javascript/ql/test/query-tests/Security/CWE-313/i18n/de.json create mode 100644 javascript/ql/test/query-tests/Security/CWE-313/locales/de.json create mode 100644 javascript/ql/test/query-tests/Security/CWE-313/tst.raml create mode 100644 javascript/ql/test/query-tests/Security/CWE-313/tst1.json create mode 100644 javascript/ql/test/query-tests/Security/CWE-313/tst2.json create mode 100644 javascript/ql/test/query-tests/Security/CWE-313/tst3.json create mode 100644 javascript/ql/test/query-tests/Security/CWE-313/tst4.json create mode 100644 javascript/ql/test/query-tests/Security/CWE-313/tst5.json create mode 100644 javascript/ql/test/query-tests/Security/CWE-313/tst6.json diff --git a/change-notes/1.21/analysis-javascript.md b/change-notes/1.21/analysis-javascript.md index a21ccd24d14..42a86ad0dd1 100644 --- a/change-notes/1.21/analysis-javascript.md +++ b/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. | diff --git a/javascript/ql/src/Security/CWE-313/PasswordInConfigurationFile.ql b/javascript/ql/src/Security/CWE-313/PasswordInConfigurationFile.ql index 7384f778f9d..2217c58c7e9 100644 --- a/javascript/ql/src/Security/CWE-313/PasswordInConfigurationFile.ql +++ b/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." diff --git a/javascript/ql/test/query-tests/Security/CWE-313/PasswordInConfigurationFile.expected b/javascript/ql/test/query-tests/Security/CWE-313/PasswordInConfigurationFile.expected index 5f86edfc29a..c2164960179 100644 --- a/javascript/ql/test/query-tests/Security/CWE-313/PasswordInConfigurationFile.expected +++ b/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. | diff --git a/javascript/ql/test/query-tests/Security/CWE-313/i18n/de.json b/javascript/ql/test/query-tests/Security/CWE-313/i18n/de.json new file mode 100644 index 00000000000..3967c1a5217 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-313/i18n/de.json @@ -0,0 +1,3 @@ +{ + "password": "Passwort" +} \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-313/locales/de.json b/javascript/ql/test/query-tests/Security/CWE-313/locales/de.json new file mode 100644 index 00000000000..3967c1a5217 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-313/locales/de.json @@ -0,0 +1,3 @@ +{ + "password": "Passwort" +} \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-313/tst.raml b/javascript/ql/test/query-tests/Security/CWE-313/tst.raml new file mode 100644 index 00000000000..826bc49a2d6 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-313/tst.raml @@ -0,0 +1 @@ +password: string diff --git a/javascript/ql/test/query-tests/Security/CWE-313/tst1.json b/javascript/ql/test/query-tests/Security/CWE-313/tst1.json new file mode 100644 index 00000000000..793b531fa22 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-313/tst1.json @@ -0,0 +1,3 @@ +{ + "password": "$pwd" +} diff --git a/javascript/ql/test/query-tests/Security/CWE-313/tst2.json b/javascript/ql/test/query-tests/Security/CWE-313/tst2.json new file mode 100644 index 00000000000..5553e429d77 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-313/tst2.json @@ -0,0 +1,3 @@ +{ + "password": "%pwd%" +} diff --git a/javascript/ql/test/query-tests/Security/CWE-313/tst3.json b/javascript/ql/test/query-tests/Security/CWE-313/tst3.json new file mode 100644 index 00000000000..78464498bdd --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-313/tst3.json @@ -0,0 +1,3 @@ +{ + "password": "${pwd:foo}" +} diff --git a/javascript/ql/test/query-tests/Security/CWE-313/tst4.json b/javascript/ql/test/query-tests/Security/CWE-313/tst4.json new file mode 100644 index 00000000000..87e9454dfd7 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-313/tst4.json @@ -0,0 +1,3 @@ +{ + "cmd": "script.sh password='secret'" +} diff --git a/javascript/ql/test/query-tests/Security/CWE-313/tst5.json b/javascript/ql/test/query-tests/Security/CWE-313/tst5.json new file mode 100644 index 00000000000..447d613eeba --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-313/tst5.json @@ -0,0 +1,3 @@ +{ + "cmd": "script.sh password=%s" +} diff --git a/javascript/ql/test/query-tests/Security/CWE-313/tst6.json b/javascript/ql/test/query-tests/Security/CWE-313/tst6.json new file mode 100644 index 00000000000..472e5680713 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-313/tst6.json @@ -0,0 +1,3 @@ +{ + "foo": "password==bar" +}