diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 24892805242..d8bb02a1f42 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -659,6 +659,20 @@ module TaintTracking { } } + /** + * A taint step through the NodeJS function `util.inspect(..)`. + */ + class UtilInspectTaintStep extends AdditionalTaintStep, DataFlow::InvokeNode { + UtilInspectTaintStep() { + this = DataFlow::moduleImport("util").getAMethodCall("inspect") + } + + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + succ = this and + this.(DataFlow::CallNode).getAnArgument() = pred + } + } + /** * A conditional checking a tainted string against a regular expression, which is * considered to be a sanitizer for all configurations. diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll index 5522d301d05..d88990951dc 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll @@ -13,7 +13,7 @@ module CleartextLogging { import CleartextLoggingCustomizations::CleartextLogging /** - * A dataflow tracking configuration for clear-text logging of sensitive information. + * A taint tracking configuration for clear-text logging of sensitive information. * * This configuration identifies flows from `Source`s, which are sources of * sensitive data, to `Sink`s, which is an abstract class representing all @@ -21,36 +21,40 @@ module CleartextLogging { * added either by extending the relevant class, or by subclassing this configuration itself, * and amending the sources and sinks. */ - class Configuration extends DataFlow::Configuration { + class Configuration extends TaintTracking::Configuration { Configuration() { this = "CleartextLogging" } - override predicate isSource(DataFlow::Node source) { source instanceof Source } + override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel lbl) { + source.(Source).getLabel() = lbl + } - override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel lbl) { + sink.(Sink).getLabel() = lbl + } - override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier } + override predicate isSanitizer(DataFlow::Node node) { node instanceof Barrier } - override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg) { - StringConcatenation::taintStep(src, trg) - or - exists(string name | name = "toString" or name = "valueOf" | - src.(DataFlow::SourceNode).getAMethodCall(name) = trg - ) - or + override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel lbl) { + // Property reads do not propagate taint. + not lbl instanceof ProcessEnvLabel and + succ.(DataFlow::PropRead).getBase() = pred + } + + override predicate isAdditionalFlowStep( + DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl + ) { + trg.(DataFlow::PropRead).getBase() = src and + inlbl instanceof ProcessEnvLabel and + outlbl.isData() + } + + override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) { // A taint propagating data flow edge through objects: a tainted write taints the entire object. exists(DataFlow::PropWrite write | write.getRhs() = src and trg.(DataFlow::SourceNode).flowsTo(write.getBase()) ) or - // Taint step through `util.inspect(..)` from Node.js - trg = DataFlow::moduleImport("util").getAMethodCall("inspect") and - trg.(DataFlow::CallNode).getAnArgument() = src - or - // 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 diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll index 9dc11d720e7..68e0357e0cf 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll @@ -15,12 +15,18 @@ module CleartextLogging { abstract class Source extends DataFlow::Node { /** Gets a string that describes the type of this data flow source. */ abstract string describe(); + + abstract DataFlow::FlowLabel getLabel(); } /** * A data flow sink for clear-text logging of sensitive information. */ - abstract class Sink extends DataFlow::Node { } + abstract class Sink extends DataFlow::Node { + DataFlow::FlowLabel getLabel() { + result.isDataOrTaint() + } + } /** * A barrier for clear-text logging of sensitive information. @@ -107,6 +113,10 @@ module CleartextLogging { } override string describe() { result = "an access to " + name } + + override DataFlow::FlowLabel getLabel() { + result.isData() + } } /** An access to a variable or property that might contain a password. */ @@ -131,6 +141,10 @@ module CleartextLogging { } override string describe() { result = "an access to " + name } + + override DataFlow::FlowLabel getLabel() { + result.isData() + } } /** A call that might return a password. */ @@ -143,14 +157,28 @@ module CleartextLogging { } override string describe() { result = "a call to " + name } + + override DataFlow::FlowLabel getLabel() { + result.isData() + } } - private class ProcessEnvSource extends Source { + class ProcessEnvSource extends Source { ProcessEnvSource() { this = NodeJSLib::process().getAPropertyRead("env") } override string describe() { result = "process environment" } + + override DataFlow::FlowLabel getLabel() { + result.isData() or + result instanceof ProcessEnvLabel + } + } + class ProcessEnvLabel extends DataFlow::FlowLabel{ + ProcessEnvLabel() { + this = "processEnv" + } } } 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 fb000e2cee2..74c1329983c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected +++ b/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected @@ -89,6 +89,8 @@ nodes | 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:9:132:5 | config | +| passwords.js:127:18:132:5 | {\\n ... )\\n } | | passwords.js:127:18:132:5 | {\\n ... )\\n } | | passwords.js:127:18:132:5 | {\\n ... )\\n } | | passwords.js:130:12:130:19 | password | @@ -97,6 +99,7 @@ nodes | passwords.js:131:12:131:24 | getPassword() | | passwords.js:135:17:135:22 | config | | passwords.js:135:17:135:22 | config | +| passwords.js:135:17:135:22 | config | | passwords.js:136:17:136:24 | config.x | | passwords.js:136:17:136:24 | config.x | | passwords.js:137:17:137:24 | config.y | @@ -111,6 +114,8 @@ nodes | passwords.js:152:33:152:43 | process.env | | passwords.js:154:21:154:28 | procdesc | | passwords.js:156:17:156:27 | process.env | +| passwords.js:157:17:157:27 | process.env | +| passwords.js:157:17:157:32 | process.env.PATH | | 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 | @@ -209,6 +214,8 @@ edges | 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:9:132:5 | config | passwords.js:135:17:135:22 | config | +| 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:127:18:132:5 | {\\n ... )\\n } | passwords.js:127:9:132:5 | 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 } | @@ -228,6 +235,7 @@ edges | 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.js:157:17:157:27 | process.env | passwords.js:157:17:157:32 | process.env.PATH | | 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 | @@ -262,6 +270,7 @@ edges | 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.js:157:17:157:32 | process.env.PATH | passwords.js:157:17:157:27 | process.env | passwords.js:157:17:157:32 | process.env.PATH | Sensitive data returned by $@ is logged here. | passwords.js:157:17:157: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 8441438a8f4..d248af3db21 100644 --- a/javascript/ql/test/query-tests/Security/CWE-312/passwords.js +++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords.js @@ -154,5 +154,5 @@ var Util = require('util'); 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. + console.log(process.env.PATH); // NOT OK. }); \ No newline at end of file