From b780f828694f0836b898c0d6c1c8a21fa7072497 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Thu, 22 Nov 2018 13:38:43 +0100 Subject: [PATCH] JS: sharpen `js/clear-text-logging` (ODASA-7485) --- change-notes/1.19/analysis-javascript.md | 1 + .../security/dataflow/CleartextLogging.qll | 2 +- .../CWE-312/CleartextLogging.expected | 25 +++++++++++++------ .../query-tests/Security/CWE-312/passwords.js | 15 +++++++++++ 4 files changed, 35 insertions(+), 8 deletions(-) diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index c52b5c82a70a..7118faff58a8 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -36,6 +36,7 @@ | **Query** | **Expected impact** | **Change** | |--------------------------------|----------------------------|----------------------------------------------| | Ambiguous HTML id attribute | Lower severity | The severity of this rule has been revised to "warning". | +| Clear-text logging of sensitive information | Fewer results | This rule now tracks flow more precisely. | | Client side cross-site scripting | More results | This rule now also flags HTML injection in the body of an email. | | Client-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. | | Conflicting HTML element attributes | Lower severity | The severity of this rule has been revised to "warning". | diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll index a0987cc21efb..7ab3b0689c25 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll @@ -52,7 +52,7 @@ module CleartextLogging { } override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg) { - any (TaintTracking::StringConcatenationTaintStep s).step(src, trg) + StringConcatenation::taintStep(src, trg) or exists (string name | name = "toString" or name = "valueOf" | src.(DataFlow::SourceNode).getAMethodCall(name) = trg diff --git a/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected b/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected index fb3e8e11bba8..e562c22f9042 100644 --- a/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected +++ b/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected @@ -22,10 +22,7 @@ nodes | passwords.js:28:16:28:17 | {} | | passwords.js:29:17:29:20 | obj3 | | passwords.js:30:14:30:21 | password | -| passwords.js:77:9:77:55 | temp | -| passwords.js:77:16:77:55 | { encry ... sword } | | passwords.js:77:37:77:53 | req.body.password | -| passwords.js:78:17:78:20 | temp | | passwords.js:78:17:78:38 | temp.en ... assword | | passwords.js:80:9:80:25 | secret | | passwords.js:80:18:80:25 | password | @@ -49,6 +46,13 @@ nodes | passwords.js:123:17:123:48 | name + ... lueOf() | | passwords.js:123:31:123:38 | password | | passwords.js:123:31:123:48 | password.valueOf() | +| passwords.js:127:9:132:5 | config | +| passwords.js:127:18:132:5 | {\\n ... )\\n } | +| passwords.js:130:12:130:19 | password | +| passwords.js:131:12:131:24 | getPassword() | +| passwords.js:135:17:135:22 | config | +| passwords.js:136:17:136:24 | config.x | +| passwords.js:137:17:137:24 | config.y | | passwords_in_browser1.js:2:13:2:20 | password | | passwords_in_browser2.js:2:13:2:20 | password | | passwords_in_server_1.js:6:13:6:20 | password | @@ -71,11 +75,7 @@ edges | passwords.js:28:9:28:17 | obj3 | passwords.js:29:17:29:20 | obj3 | | passwords.js:28:16:28:17 | {} | passwords.js:28:9:28:17 | obj3 | | passwords.js:30:14:30:21 | password | passwords.js:28:16:28:17 | {} | -| passwords.js:77:9:77:55 | temp | passwords.js:78:17:78:20 | temp | -| passwords.js:77:16:77:55 | { encry ... sword } | passwords.js:77:9:77:55 | temp | -| passwords.js:77:37:77:53 | req.body.password | passwords.js:77:16:77:55 | { encry ... sword } | | passwords.js:77:37:77:53 | req.body.password | passwords.js:78:17:78:38 | temp.en ... assword | -| passwords.js:78:17:78:20 | temp | passwords.js:78:17:78:38 | temp.en ... assword | | passwords.js:80:9:80:25 | secret | passwords.js:81:24:81:29 | secret | | passwords.js:80:18:80:25 | password | passwords.js:80:9:80:25 | secret | | passwords.js:81:24:81:29 | secret | passwords.js:81:17:81:31 | `pw: ${secret}` | @@ -89,6 +89,12 @@ edges | passwords.js:122:31:122:49 | password.toString() | passwords.js:122:17:122:49 | name + ... tring() | | passwords.js:123:31:123:38 | password | passwords.js:123:31:123:48 | password.valueOf() | | passwords.js:123:31:123:48 | password.valueOf() | passwords.js:123:17:123:48 | name + ... lueOf() | +| passwords.js:127:9:132:5 | config | passwords.js:135:17:135:22 | config | +| passwords.js:127:18:132:5 | {\\n ... )\\n } | passwords.js:127:9:132:5 | config | +| passwords.js:130:12:130:19 | password | passwords.js:127:18:132:5 | {\\n ... )\\n } | +| passwords.js:130:12:130:19 | password | passwords.js:136:17:136:24 | config.x | +| passwords.js:131:12:131:24 | getPassword() | passwords.js:127:18:132:5 | {\\n ... )\\n } | +| passwords.js:131:12:131:24 | getPassword() | passwords.js:137:17:137:24 | config.y | | passwords_in_server_5.js:4:7:4:24 | req.query.password | passwords_in_server_5.js:7:12:7:12 | x | | passwords_in_server_5.js:7:12:7:12 | x | passwords_in_server_5.js:8:17:8:17 | x | #select @@ -113,6 +119,11 @@ edges | passwords.js:119:21:119:46 | "Passwo ... assword | passwords.js:119:39:119:46 | password | passwords.js:119:21:119:46 | "Passwo ... assword | Sensitive data returned by $@ is logged here. | passwords.js:119:39:119:46 | password | an access to password | | passwords.js:122:17:122:49 | name + ... tring() | passwords.js:122:31:122:38 | password | passwords.js:122:17:122:49 | name + ... tring() | Sensitive data returned by $@ is logged here. | passwords.js:122:31:122:38 | password | an access to password | | passwords.js:123:17:123:48 | name + ... lueOf() | passwords.js:123:31:123:38 | password | passwords.js:123:17:123:48 | name + ... lueOf() | Sensitive data returned by $@ is logged here. | passwords.js:123:31:123:38 | password | an access to password | +| passwords.js:135:17:135:22 | config | passwords.js:127:18:132:5 | {\\n ... )\\n } | passwords.js:135:17:135:22 | config | Sensitive data returned by $@ is logged here. | passwords.js:127:18:132:5 | {\\n ... )\\n } | an access to password | +| passwords.js:135:17:135:22 | config | passwords.js:130:12:130:19 | password | passwords.js:135:17:135:22 | config | Sensitive data returned by $@ is logged here. | passwords.js:130:12:130:19 | password | an access to password | +| passwords.js:135:17:135:22 | config | passwords.js:131:12:131:24 | getPassword() | passwords.js:135:17:135:22 | config | Sensitive data returned by $@ is logged here. | passwords.js:131:12:131:24 | getPassword() | a call to getPassword | +| passwords.js:136:17:136:24 | config.x | passwords.js:130:12:130:19 | password | passwords.js:136:17:136:24 | config.x | Sensitive data returned by $@ is logged here. | passwords.js:130:12:130:19 | password | an access to password | +| passwords.js:137:17:137:24 | config.y | passwords.js:131:12:131:24 | getPassword() | passwords.js:137:17:137:24 | config.y | Sensitive data returned by $@ is logged here. | passwords.js:131:12:131:24 | getPassword() | a call to getPassword | | passwords_in_server_1.js:6:13:6:20 | password | passwords_in_server_1.js:6:13:6:20 | password | passwords_in_server_1.js:6:13:6:20 | password | Sensitive data returned by $@ is logged here. | passwords_in_server_1.js:6:13:6:20 | password | an access to password | | passwords_in_server_2.js:3:13:3:20 | password | passwords_in_server_2.js:3:13:3:20 | password | passwords_in_server_2.js:3:13:3:20 | password | Sensitive data returned by $@ is logged here. | passwords_in_server_2.js:3:13:3:20 | password | an access to password | | passwords_in_server_3.js:2:13:2:20 | password | passwords_in_server_3.js:2:13:2:20 | password | passwords_in_server_3.js:2:13:2:20 | password | Sensitive data returned by $@ is logged here. | passwords_in_server_3.js:2:13:2:20 | password | an access to password | diff --git a/javascript/ql/test/query-tests/Security/CWE-312/passwords.js b/javascript/ql/test/query-tests/Security/CWE-312/passwords.js index 24be2573e60f..d752ba7493f6 100644 --- a/javascript/ql/test/query-tests/Security/CWE-312/passwords.js +++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords.js @@ -122,3 +122,18 @@ console.log(name + ", " + password.toString()); // NOT OK console.log(name + ", " + password.valueOf()); // NOT OK }); + +(function() { + var config = { + password: x, + hostname: "tarski", + x: password, + y: getPassword() + }; + var cfg = x? config: config; + console.log(config.hostname); // OK + console.log(config); // NOT OK + console.log(config.x); // NOT OK + console.log(config.y); // NOT OK + console.log(config[x]); // OK (probably) +});