Skip to content

Commit 77c869f

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #2220 from erik-krogh/processEnvTaint
Approved by esbena, max-schaefer
2 parents 9cf8199 + 0a428a8 commit 77c869f

File tree

7 files changed

+209
-13
lines changed

7 files changed

+209
-13
lines changed

‎change-notes/1.24/analysis-javascript.md‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# Improvements to JavaScript analysis
2+
3+
## General improvements
4+
5+
6+
## New queries
7+
8+
| **Query** | **Tags** | **Purpose** |
9+
|---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
10+
11+
12+
## Changes to existing queries
13+
14+
| **Query** | **Expected impact** | **Change** |
15+
|--------------------------------|------------------------------|---------------------------------------------------------------------------|
16+
| Clear-text logging of sensitive information (`js/clear-text-logging`) | More results | More results involving `process.env` and indirect calls to logging methods are recognized. |
17+
18+
## Changes to libraries
19+

‎javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -659,6 +659,20 @@ module TaintTracking {
659659
}
660660
}
661661

662+
/**
663+
* A taint step through the Node.JS function `util.inspect(..)`.
664+
*/
665+
class UtilInspectTaintStep extends AdditionalTaintStep, DataFlow::InvokeNode {
666+
UtilInspectTaintStep() {
667+
this = DataFlow::moduleImport("util").getAMemberCall("inspect")
668+
}
669+
670+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
671+
succ = this and
672+
this.getAnArgument() = pred
673+
}
674+
}
675+
662676
/**
663677
* A conditional checking a tainted string against a regular expression, which is
664678
* considered to be a sanitizer for all configurations.

‎javascript/ql/src/semmle/javascript/frameworks/Logging.qll‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,15 @@ private module Console {
6161
if name = "assert"
6262
then result = getArgument([1 .. getNumArgument()])
6363
else result = getAnArgument()
64+
or
65+
result = getASpreadArgument()
66+
}
67+
68+
/**
69+
* Gets the name of the console logging method, e.g. "log", "error", "assert", etc.
70+
*/
71+
string getName() {
72+
result = name
6473
}
6574
}
6675
}

‎javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll‎

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,35 +13,45 @@ module CleartextLogging {
1313
import CleartextLoggingCustomizations::CleartextLogging
1414

1515
/**
16-
* A dataflow tracking configuration for clear-text logging of sensitive information.
16+
* A taint tracking configuration for clear-text logging of sensitive information.
1717
*
1818
* This configuration identifies flows from `Source`s, which are sources of
1919
* sensitive data, to `Sink`s, which is an abstract class representing all
2020
* the places sensitive data may be stored in clear-text. Additional sources or sinks can be
2121
* added either by extending the relevant class, or by subclassing this configuration itself,
2222
* and amending the sources and sinks.
2323
*/
24-
class Configuration extends DataFlow::Configuration {
24+
class Configuration extends TaintTracking::Configuration {
2525
Configuration() { this = "CleartextLogging" }
2626

27-
override predicate isSource(DataFlow::Node source) { source instanceof Source }
27+
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel lbl) {
28+
source.(Source).getLabel() = lbl
29+
}
2830

29-
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
31+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel lbl) {
32+
sink.(Sink).getLabel() = lbl
33+
}
3034

31-
override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
35+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Barrier }
3236

33-
override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg) {
34-
StringConcatenation::taintStep(src, trg)
35-
or
36-
exists(string name | name = "toString" or name = "valueOf" |
37-
src.(DataFlow::SourceNode).getAMethodCall(name) = trg
38-
)
39-
or
37+
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
38+
succ.(DataFlow::PropRead).getBase() = pred
39+
}
40+
41+
override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
4042
// A taint propagating data flow edge through objects: a tainted write taints the entire object.
4143
exists(DataFlow::PropWrite write |
4244
write.getRhs() = src and
4345
trg.(DataFlow::SourceNode).flowsTo(write.getBase())
4446
)
47+
or
48+
// Taint through the arguments object.
49+
exists(DataFlow::CallNode call, Function f |
50+
src = call.getAnArgument() and
51+
f = call.getACallee() and
52+
not call.isImprecise() and
53+
trg.asExpr() = f.getArgumentsVariable().getAnAccess()
54+
)
4555
}
4656
}
4757
}

‎javascript/ql/src/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll‎

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,17 +15,39 @@ module CleartextLogging {
1515
abstract class Source extends DataFlow::Node {
1616
/** Gets a string that describes the type of this data flow source. */
1717
abstract string describe();
18+
19+
abstract DataFlow::FlowLabel getLabel();
1820
}
1921

2022
/**
2123
* A data flow sink for clear-text logging of sensitive information.
2224
*/
23-
abstract class Sink extends DataFlow::Node { }
25+
abstract class Sink extends DataFlow::Node {
26+
DataFlow::FlowLabel getLabel() {
27+
result.isDataOrTaint()
28+
}
29+
}
2430

2531
/**
2632
* A barrier for clear-text logging of sensitive information.
2733
*/
2834
abstract class Barrier extends DataFlow::Node { }
35+
36+
/**
37+
* A call to `.replace()` that seems to mask sensitive information.
38+
*/
39+
class MaskingReplacer extends Barrier, DataFlow::MethodCallNode {
40+
MaskingReplacer() {
41+
this.getCalleeName() = "replace" and
42+
exists(RegExpLiteral reg |
43+
reg = this.getArgument(0).getALocalSource().asExpr() and
44+
reg.isGlobal() and
45+
any(RegExpDot term).getLiteral() = reg
46+
)
47+
and
48+
exists(this.getArgument(1).getStringValue())
49+
}
50+
}
2951

3052
/**
3153
* An argument to a logging mechanism.
@@ -107,6 +129,10 @@ module CleartextLogging {
107129
}
108130

109131
override string describe() { result = "an access to " + name }
132+
133+
override DataFlow::FlowLabel getLabel() {
134+
result.isData()
135+
}
110136
}
111137

112138
/** An access to a variable or property that might contain a password. */
@@ -131,6 +157,10 @@ module CleartextLogging {
131157
}
132158

133159
override string describe() { result = "an access to " + name }
160+
161+
override DataFlow::FlowLabel getLabel() {
162+
result.isData()
163+
}
134164
}
135165

136166
/** A call that might return a password. */
@@ -143,5 +173,33 @@ module CleartextLogging {
143173
}
144174

145175
override string describe() { result = "a call to " + name }
176+
177+
override DataFlow::FlowLabel getLabel() {
178+
result.isData()
179+
}
180+
}
181+
182+
/** An access to the sensitive object `process.env`. */
183+
class ProcessEnvSource extends Source {
184+
ProcessEnvSource() {
185+
this = NodeJSLib::process().getAPropertyRead("env")
186+
}
187+
188+
override string describe() { result = "process environment" }
189+
190+
override DataFlow::FlowLabel getLabel() {
191+
result.isData() or
192+
result instanceof PartiallySensitiveMap
193+
}
194+
}
195+
196+
/**
197+
* A flow label describing a map that might contain sensitive information in some properties.
198+
* Property reads on such maps where the property name is fixed is unlikely to leak sensitive information.
199+
*/
200+
class PartiallySensitiveMap extends DataFlow::FlowLabel {
201+
PartiallySensitiveMap() {
202+
this = "PartiallySensitiveMap"
203+
}
146204
}
147205
}

‎javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,8 @@ nodes
8989
| passwords.js:123:31:123:38 | password |
9090
| passwords.js:123:31:123:48 | password.valueOf() |
9191
| passwords.js:127:9:132:5 | config |
92+
| passwords.js:127:9:132:5 | config |
93+
| passwords.js:127:18:132:5 | {\\n ... )\\n } |
9294
| passwords.js:127:18:132:5 | {\\n ... )\\n } |
9395
| passwords.js:127:18:132:5 | {\\n ... )\\n } |
9496
| passwords.js:130:12:130:19 | password |
@@ -97,10 +99,35 @@ nodes
9799
| passwords.js:131:12:131:24 | getPassword() |
98100
| passwords.js:135:17:135:22 | config |
99101
| passwords.js:135:17:135:22 | config |
102+
| passwords.js:135:17:135:22 | config |
100103
| passwords.js:136:17:136:24 | config.x |
101104
| passwords.js:136:17:136:24 | config.x |
102105
| passwords.js:137:17:137:24 | config.y |
103106
| passwords.js:137:17:137:24 | config.y |
107+
| passwords.js:142:26:142:34 | arguments |
108+
| passwords.js:142:26:142:34 | arguments |
109+
| passwords.js:147:12:147:19 | password |
110+
| passwords.js:147:12:147:19 | password |
111+
| passwords.js:149:21:149:28 | config.x |
112+
| passwords.js:150:21:150:31 | process.env |
113+
| passwords.js:150:21:150:31 | process.env |
114+
| passwords.js:152:9:152:63 | procdesc |
115+
| passwords.js:152:20:152:44 | Util.in ... ss.env) |
116+
| passwords.js:152:20:152:63 | Util.in ... /g, '') |
117+
| passwords.js:152:33:152:43 | process.env |
118+
| passwords.js:152:33:152:43 | process.env |
119+
| passwords.js:154:21:154:28 | procdesc |
120+
| passwords.js:156:17:156:27 | process.env |
121+
| passwords.js:156:17:156:27 | process.env |
122+
| passwords.js:156:17:156:27 | process.env |
123+
| passwords.js:163:14:163:21 | password |
124+
| passwords.js:163:14:163:21 | password |
125+
| passwords.js:163:14:163:41 | passwor ... g, "*") |
126+
| passwords.js:163:14:163:41 | passwor ... g, "*") |
127+
| passwords.js:164:14:164:21 | password |
128+
| passwords.js:164:14:164:21 | password |
129+
| passwords.js:164:14:164:42 | passwor ... g, "*") |
130+
| passwords.js:164:14:164:42 | passwor ... g, "*") |
104131
| passwords_in_browser1.js:2:13:2:20 | password |
105132
| passwords_in_browser1.js:2:13:2:20 | password |
106133
| passwords_in_browser1.js:2:13:2:20 | password |
@@ -199,6 +226,9 @@ edges
199226
| passwords.js:123:31:123:48 | password.valueOf() | passwords.js:123:17:123:48 | name + ... lueOf() |
200227
| passwords.js:127:9:132:5 | config | passwords.js:135:17:135:22 | config |
201228
| passwords.js:127:9:132:5 | config | passwords.js:135:17:135:22 | config |
229+
| passwords.js:127:9:132:5 | config | passwords.js:135:17:135:22 | config |
230+
| passwords.js:127:9:132:5 | config | passwords.js:135:17:135:22 | config |
231+
| passwords.js:127:18:132:5 | {\\n ... )\\n } | passwords.js:127:9:132:5 | config |
202232
| passwords.js:127:18:132:5 | {\\n ... )\\n } | passwords.js:127:9:132:5 | config |
203233
| passwords.js:127:18:132:5 | {\\n ... )\\n } | passwords.js:127:9:132:5 | config |
204234
| passwords.js:130:12:130:19 | password | passwords.js:127:18:132:5 | {\\n ... )\\n } |
@@ -213,6 +243,30 @@ edges
213243
| passwords.js:131:12:131:24 | getPassword() | passwords.js:137:17:137:24 | config.y |
214244
| passwords.js:131:12:131:24 | getPassword() | passwords.js:137:17:137:24 | config.y |
215245
| passwords.js:131:12:131:24 | getPassword() | passwords.js:137:17:137:24 | config.y |
246+
| passwords.js:147:12:147:19 | password | passwords.js:149:21:149:28 | config.x |
247+
| passwords.js:147:12:147:19 | password | passwords.js:149:21:149:28 | config.x |
248+
| passwords.js:149:21:149:28 | config.x | passwords.js:142:26:142:34 | arguments |
249+
| passwords.js:149:21:149:28 | config.x | passwords.js:142:26:142:34 | arguments |
250+
| passwords.js:150:21:150:31 | process.env | passwords.js:142:26:142:34 | arguments |
251+
| passwords.js:150:21:150:31 | process.env | passwords.js:142:26:142:34 | arguments |
252+
| passwords.js:150:21:150:31 | process.env | passwords.js:142:26:142:34 | arguments |
253+
| passwords.js:150:21:150:31 | process.env | passwords.js:142:26:142:34 | arguments |
254+
| passwords.js:152:9:152:63 | procdesc | passwords.js:154:21:154:28 | procdesc |
255+
| passwords.js:152:20:152:44 | Util.in ... ss.env) | passwords.js:152:20:152:63 | Util.in ... /g, '') |
256+
| passwords.js:152:20:152:63 | Util.in ... /g, '') | passwords.js:152:9:152:63 | procdesc |
257+
| passwords.js:152:33:152:43 | process.env | passwords.js:152:20:152:44 | Util.in ... ss.env) |
258+
| passwords.js:152:33:152:43 | process.env | passwords.js:152:20:152:44 | Util.in ... ss.env) |
259+
| passwords.js:154:21:154:28 | procdesc | passwords.js:142:26:142:34 | arguments |
260+
| passwords.js:154:21:154:28 | procdesc | passwords.js:142:26:142:34 | arguments |
261+
| passwords.js:156:17:156:27 | process.env | passwords.js:156:17:156:27 | process.env |
262+
| passwords.js:163:14:163:21 | password | passwords.js:163:14:163:41 | passwor ... g, "*") |
263+
| passwords.js:163:14:163:21 | password | passwords.js:163:14:163:41 | passwor ... g, "*") |
264+
| passwords.js:163:14:163:21 | password | passwords.js:163:14:163:41 | passwor ... g, "*") |
265+
| passwords.js:163:14:163:21 | password | passwords.js:163:14:163:41 | passwor ... g, "*") |
266+
| passwords.js:164:14:164:21 | password | passwords.js:164:14:164:42 | passwor ... g, "*") |
267+
| passwords.js:164:14:164:21 | password | passwords.js:164:14:164:42 | passwor ... g, "*") |
268+
| passwords.js:164:14:164:21 | password | passwords.js:164:14:164:42 | passwor ... g, "*") |
269+
| passwords.js:164:14:164:21 | password | passwords.js:164:14:164:42 | passwor ... g, "*") |
216270
| passwords_in_browser1.js:2:13:2:20 | password | passwords_in_browser1.js:2:13:2:20 | password |
217271
| passwords_in_browser2.js:2:13:2:20 | password | passwords_in_browser2.js:2:13:2:20 | password |
218272
| passwords_in_server_1.js:6:13:6:20 | password | passwords_in_server_1.js:6:13:6:20 | password |
@@ -250,6 +304,12 @@ edges
250304
| 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 |
251305
| 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 |
252306
| 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 |
307+
| 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 |
308+
| 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 |
309+
| 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 |
310+
| 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 |
311+
| passwords.js:163:14:163:41 | passwor ... g, "*") | passwords.js:163:14:163:21 | password | passwords.js:163:14:163:41 | passwor ... g, "*") | Sensitive data returned by $@ is logged here. | passwords.js:163:14:163:21 | password | an access to password |
312+
| passwords.js:164:14:164:42 | passwor ... g, "*") | passwords.js:164:14:164:21 | password | passwords.js:164:14:164:42 | passwor ... g, "*") | Sensitive data returned by $@ is logged here. | passwords.js:164:14:164:21 | password | an access to password |
253313
| 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 |
254314
| 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 |
255315
| 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 |

‎javascript/ql/test/query-tests/Security/CWE-312/passwords.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,3 +137,29 @@
137137
console.log(config.y); // NOT OK
138138
console.log(config[x]); // OK (probably)
139139
});
140+
141+
function indirectLogCall() {
142+
console.log.apply(this, arguments);
143+
}
144+
var Util = require('util');
145+
(function() {
146+
var config = {
147+
x: password
148+
};
149+
indirectLogCall(config.x); // NOT OK
150+
indirectLogCall(process.env); // NOT OK
151+
152+
var procdesc = Util.inspect(process.env).replace(/\n/g, '')
153+
154+
indirectLogCall(procdesc); // NOT OK
155+
156+
console.log(process.env); // NOT OK
157+
console.log(process.env.PATH); // OK.
158+
console.log(process.env["foo" + "bar"]); // OK.
159+
});
160+
161+
(function () {
162+
console.log(password.replace(/./g, "*")); // OK!
163+
console.log(password.replace(/\./g, "*")); // NOT OK!
164+
console.log(password.replace(/foo/g, "*")); // NOT OK!
165+
})();

0 commit comments

Comments
 (0)