From 14e4decffa98d4f1d9777fee74f6bdaef88b9c97 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 29 Oct 2019 14:22:53 +0100 Subject: [PATCH] changes based on review feedback. No flow-labels yet --- .../semmle/javascript/frameworks/Logging.qll | 28 ++----------------- .../security/dataflow/CleartextLogging.qll | 10 +++++++ .../CWE-312/CleartextLogging.expected | 12 ++++++-- .../query-tests/Security/CWE-312/passwords.js | 3 ++ 4 files changed, 24 insertions(+), 29 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/Logging.qll b/javascript/ql/src/semmle/javascript/frameworks/Logging.qll index b8de5f09cbd..d8646575c4a 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Logging.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Logging.qll @@ -61,6 +61,8 @@ private module Console { if name = "assert" then result = getArgument([1 .. getNumArgument()]) else result = getAnArgument() + or + result = getASpreadArgument() } /** @@ -70,32 +72,6 @@ private module Console { result = name } } - - /** - * A call to a function that forwards all arguments to some console logging mechanism. - * The called function contain a single statement similar to: `console.log.apply(this, arguments)`. - */ - class IndirectConsoleLoggerCall extends LoggerCall { - ConsoleLoggerCall consoleCall; - DataFlow::MethodCallNode applyCall; - - IndirectConsoleLoggerCall() { - forex(Function f | f = this.getACallee() | - f.getNumBodyStmt() = 1 and - consoleCall.getContainer() = f and - applyCall.getContainer() = f and - applyCall.getMethodName() = "apply" and - applyCall.getReceiver() = consoleCall.getCalleeNode() and - applyCall.getArgument(1).asExpr().(VarAccess).getName() = "arguments" - ) - } - - override DataFlow::Node getAMessageComponent() { - if consoleCall.getName() = "assert" - then result = getArgument([1 .. getNumArgument()]) - else result = getAnArgument() - } - } } /** diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll index 734723aefb8..5522d301d05 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll @@ -50,6 +50,16 @@ module CleartextLogging { // Taint step through a `str.replace(..)` call. trg.(DataFlow::MethodCallNode).getCalleeName() = "replace" and trg.(DataFlow::MethodCallNode).getReceiver() = src + or + // Taint through the arguments object. + exists(DataFlow::CallNode call, Function f, VarAccess var | + src = call.getAnArgument() and + f = call.getACallee() and + not call.isImprecise() and + var.getName() = "arguments" and + var.getContainer() = f and + trg.asExpr() = var + ) } } } 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 7ca43e503ac..fb000e2cee2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected +++ b/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected @@ -101,6 +101,7 @@ nodes | passwords.js:136:17:136:24 | config.x | | passwords.js:137:17:137:24 | config.y | | passwords.js:137:17:137:24 | config.y | +| passwords.js:142:26:142:34 | arguments | | passwords.js:147:12:147:19 | password | | passwords.js:149:21:149:28 | config.x | | passwords.js:150:21:150:31 | process.env | @@ -109,6 +110,7 @@ nodes | passwords.js:152:20:152:63 | Util.in ... /g, '') | | passwords.js:152:33:152:43 | process.env | | passwords.js:154:21:154:28 | procdesc | +| passwords.js:156:17:156:27 | process.env | | passwords_in_browser1.js:2:13:2:20 | password | | passwords_in_browser1.js:2:13:2:20 | password | | passwords_in_browser1.js:2:13:2:20 | password | @@ -219,10 +221,13 @@ edges | 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.js:147:12:147:19 | password | passwords.js:149:21:149:28 | config.x | +| passwords.js:149:21:149:28 | config.x | passwords.js:142:26:142:34 | arguments | +| passwords.js:150:21:150:31 | process.env | passwords.js:142:26:142:34 | arguments | | passwords.js:152:9:152:63 | procdesc | passwords.js:154:21:154:28 | procdesc | | passwords.js:152:20:152:44 | Util.in ... ss.env) | passwords.js:152:20:152:63 | Util.in ... /g, '') | | passwords.js:152:20:152:63 | Util.in ... /g, '') | passwords.js:152:9:152:63 | procdesc | | passwords.js:152:33:152:43 | process.env | passwords.js:152:20:152:44 | Util.in ... ss.env) | +| passwords.js:154:21:154:28 | procdesc | passwords.js:142:26:142:34 | arguments | | 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 | | passwords_in_server_5.js:7:12:7:12 | x | passwords_in_server_5.js:8:17:8:17 | x | @@ -253,9 +258,10 @@ edges | 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.js:149:21:149:28 | config.x | passwords.js:147:12:147:19 | password | passwords.js:149:21:149:28 | config.x | Sensitive data returned by $@ is logged here. | passwords.js:147:12:147:19 | password | an access to password | -| passwords.js:150:21:150:31 | process.env | passwords.js:150:21:150:31 | process.env | passwords.js:150:21:150:31 | process.env | Sensitive data returned by $@ is logged here. | passwords.js:150:21:150:31 | process.env | process environment | -| passwords.js:154:21:154:28 | procdesc | passwords.js:152:33:152:43 | process.env | passwords.js:154:21:154:28 | procdesc | Sensitive data returned by $@ is logged here. | passwords.js:152:33:152:43 | process.env | process environment | +| passwords.js:142:26:142:34 | arguments | passwords.js:147:12:147:19 | password | passwords.js:142:26:142:34 | arguments | Sensitive data returned by $@ is logged here. | passwords.js:147:12:147:19 | password | an access to password | +| passwords.js:142:26:142:34 | arguments | passwords.js:150:21:150:31 | process.env | passwords.js:142:26:142:34 | arguments | Sensitive data returned by $@ is logged here. | passwords.js:150:21:150:31 | process.env | process environment | +| passwords.js:142:26:142:34 | arguments | passwords.js:152:33:152:43 | process.env | passwords.js:142:26:142:34 | arguments | Sensitive data returned by $@ is logged here. | passwords.js:152:33:152:43 | process.env | process environment | +| passwords.js:156:17:156:27 | process.env | passwords.js:156:17:156:27 | process.env | passwords.js:156:17:156:27 | process.env | Sensitive data returned by $@ is logged here. | passwords.js:156:17:156:27 | process.env | process environment | | 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 7ad199732b8..8441438a8f4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-312/passwords.js +++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords.js @@ -152,4 +152,7 @@ var Util = require('util'); var procdesc = Util.inspect(process.env).replace(/\n/g, '') indirectLogCall(procdesc); // NOT OK + + console.log(process.env); // NOT OK + console.log(process.env.PATH); // <- Should be marked, but isn't. Hopefully fixed when introducing flow-labels. }); \ No newline at end of file