Skip to content

Commit f1c538a

Browse files
author
Max Schaefer
committed
JavaScript: Restrict RemotePropertyInjection query to avoid double-reporting.
This query now only flags user-controlled property and header writes, method calls are handled by the new unsafe/unvalidated method call queries.
1 parent 2889e07 commit f1c538a

File tree

5 files changed

+62
-93
lines changed

5 files changed

+62
-93
lines changed

javascript/ql/src/Security/CWE-400/RemotePropertyInjection.qhelp

Lines changed: 34 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -6,89 +6,80 @@
66
<overview>
77
<p>
88
Dynamically computing object property names from untrusted input
9-
may have multiple undesired consequences. For example,
10-
if the property access is used as part of a write, an
11-
attacker may overwrite vital properties of objects, such as
12-
<code>__proto__</code>. This attack is known as <i>prototype
13-
pollution attack</i> and may serve as a vehicle for denial-of-service
14-
attacks. A similar attack vector, is to replace the
15-
<code>toString</code> property of an object with a primitive.
16-
Whenever <code>toString</code> is then called on that object, either
17-
explicitly or implicitly as part of a type coercion, an exception
9+
may have multiple undesired consequences. For example,
10+
if the property access is used as part of a write, an
11+
attacker may overwrite vital properties of objects, such as
12+
<code>__proto__</code>. This attack is known as <i>prototype
13+
pollution attack</i> and may serve as a vehicle for denial-of-service
14+
attacks. A similar attack vector, is to replace the
15+
<code>toString</code> property of an object with a primitive.
16+
Whenever <code>toString</code> is then called on that object, either
17+
explicitly or implicitly as part of a type coercion, an exception
1818
will be raised.
1919
</p>
2020

2121
<p>
22-
Moreover, if the dynamically computed property is
23-
used as part of a method call, the attacker may trigger
24-
the execution of unwanted functions such as the
25-
<code>Function</code> constructor or the
26-
<code>eval</code> method, which can be used
27-
for code injection.
28-
</p>
29-
30-
<p>
31-
Additionally, if the name of an HTTP header is user-controlled,
32-
an attacker may exploit this to overwrite security-critical headers
33-
such as <code>Access-Control-Allow-Origin</code> or
22+
Moreover, if the name of an HTTP header is user-controlled,
23+
an attacker may exploit this to overwrite security-critical headers
24+
such as <code>Access-Control-Allow-Origin</code> or
3425
<code>Content-Security-Policy</code>.
3526
</p>
3627
</overview>
3728

3829
<recommendation>
3930
<p>
4031
The most common case in which prototype pollution vulnerabilities arise
41-
is when JavaScript objects are used for implementing map data
42-
structures. This case should be avoided whenever possible by using the
43-
ECMAScript 2015 <code>Map</code> instead. When this is not possible, an
44-
alternative fix is to prepend untrusted input with a marker character
45-
such as <code>$</code>, before using it in properties accesses. In this way,
46-
the attacker does not have access to built-in properties which do not
47-
start with the chosen character.
32+
is when JavaScript objects are used for implementing map data
33+
structures. This case should be avoided whenever possible by using the
34+
ECMAScript 2015 <code>Map</code> instead. When this is not possible, an
35+
alternative fix is to prepend untrusted input with a marker character
36+
such as <code>$</code>, before using it in properties accesses. In this way,
37+
the attacker does not have access to built-in properties which do not
38+
start with the chosen character.
4839
</p>
4940
<p>
50-
When using user input as part of header or method names, a sanitization
51-
step should be performed on the input to ensure that the name does not
52-
clash with existing property and header names such as
53-
<code>__proto__</code> or <code>Content-Security-Policy</code>.
41+
When using user input as part of a header name, a sanitization
42+
step should be performed on the input to ensure that the name does not
43+
clash with existing header names such as
44+
<code>Content-Security-Policy</code>.
5445
</p>
5546
</recommendation>
5647

5748
<example>
5849
<p>
59-
In the example below, the dynamically computed property
60-
<code>prop</code> is accessed on <code>myObj</code> using a
50+
In the example below, the dynamically computed property
51+
<code>prop</code> is accessed on <code>myObj</code> using a
6152
user-controlled value.
6253
</p>
6354

6455
<sample src="examples/RemotePropertyInjection.js"/>
6556

6657
<p>
67-
This is not secure since an attacker may exploit this code to
58+
This is not secure since an attacker may exploit this code to
6859
overwrite the property <code>__proto__</code> with an empty function.
69-
If this happens, the concatenation in the <code>console.log</code>
70-
argument will fail with a confusing message such as
60+
If this happens, the concatenation in the <code>console.log</code>
61+
argument will fail with a confusing message such as
7162
"Function.prototype.toString is not generic". If the application does
7263
not properly handle this error, this scenario may result in a serious
73-
denial-of-service attack. The fix is to prepend the user-controlled
74-
string with a marker character such as <code>$</code> which will
75-
prevent arbitrary property names from being overwritten.
64+
denial-of-service attack. The fix is to prepend the user-controlled
65+
string with a marker character such as <code>$</code> which will
66+
prevent arbitrary property names from being overwritten.
7667
</p>
7768

7869
<sample src="examples/RemotePropertyInjection_fixed.js"/>
7970
</example>
8071

8172
<references>
82-
<li>Prototype pollution attacks:
73+
<li>Prototype pollution attacks:
8374
<a href="https://github.com/electron/electron/pull/9287">electron</a>,
8475
<a href="https://hackerone.com/reports/310443">lodash</a>,
8576
<a href="https://nodesecurity.io/advisories/566">hoek</a>.
8677
</li>
87-
<li> Penetration testing report:
78+
<li> Penetration testing report:
8879
<a href="http://seclists.org/pen-test/2009/Mar/67">
8980
header name injection attack</a>
9081
</li>
91-
<li> npm blog post:
82+
<li> npm blog post:
9283
<a href="https://blog.liftsecurity.io/2015/01/14/the-dangers-of-square-bracket-notation#lift-security">
9384
dangers of square bracket notation</a>
9485
</li>

javascript/ql/src/Security/CWE-400/RemotePropertyInjection.ql

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
/**
22
* @name Remote property injection
3-
* @description Allowing writes to arbitrary properties or calls to arbitrary
4-
* methods of an object may lead to denial-of-service attacks.
5-
*
3+
* @description Allowing writes to arbitrary properties of an object may lead to
4+
* denial-of-service attacks.
65
* @kind path-problem
76
* @problem.severity warning
87
* @precision medium

javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll

Lines changed: 20 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
2-
* Provides a taint tracking configuration for reasoning about injections in
3-
* property names, used either for writing into a property, into a header or
2+
* Provides a taint tracking configuration for reasoning about injections in
3+
* property names, used either for writing into a property, into a header or
44
* for calling an object's method.
55
*/
66

@@ -18,11 +18,11 @@ module RemotePropertyInjection {
1818
* A data flow sink for remote property injection.
1919
*/
2020
abstract class Sink extends DataFlow::Node {
21-
21+
2222
/**
2323
* Gets a string to identify the different types of sinks.
2424
*/
25-
abstract string getMessage();
25+
abstract string getMessage();
2626
}
2727

2828
/**
@@ -52,58 +52,40 @@ module RemotePropertyInjection {
5252
}
5353

5454
/**
55-
* A source of remote user input, considered as a flow source for remote property
56-
* injection.
55+
* A source of remote user input, considered as a flow source for remote property
56+
* injection.
5757
*/
5858
class RemoteFlowSourceAsSource extends Source {
5959
RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
6060
}
6161

6262
/**
63-
* A sink for property writes with dynamically computed property name.
63+
* A sink for property writes with dynamically computed property name.
6464
*/
6565
class PropertyWriteSink extends Sink, DataFlow::ValueNode {
6666
PropertyWriteSink() {
67-
exists (DataFlow::PropWrite pw | astNode = pw.getPropertyNameExpr()) or
68-
exists (DeleteExpr expr | expr.getOperand().(PropAccess).getPropertyNameExpr() = astNode)
67+
exists (DataFlow::PropWrite pw | astNode = pw.getPropertyNameExpr()) or
68+
exists (DeleteExpr expr | expr.getOperand().(PropAccess).getPropertyNameExpr() = astNode)
6969
}
7070

7171
override string getMessage() {
7272
result = " a property name to write to."
73-
}
74-
}
75-
76-
/**
77-
* A sink for method calls using dynamically computed method names.
78-
*/
79-
class MethodCallSink extends Sink, DataFlow::ValueNode {
80-
MethodCallSink() {
81-
exists (DataFlow::PropRead pr | astNode = pr.getPropertyNameExpr() |
82-
exists (pr.getAnInvocation()) and
83-
84-
// Omit sinks covered by the UnsafeDynamicMethodAccess query
85-
not PropertyInjection::hasUnsafeMethods(pr.getBase().getALocalSource())
86-
)
87-
}
88-
89-
override string getMessage() {
90-
result = " a method name to be called."
9173
}
9274
}
93-
94-
/**
95-
* A sink for HTTP header writes with dynamically computed header name.
96-
* This sink avoids double-flagging by ignoring `SetMultipleHeaders` since
97-
* the multiple headers use case consists of an objects containing different
98-
* header names as properties. This case is already handled by
99-
* `PropertyWriteSink`.
75+
76+
/**
77+
* A sink for HTTP header writes with dynamically computed header name.
78+
* This sink avoids double-flagging by ignoring `SetMultipleHeaders` since
79+
* the multiple headers use case consists of an objects containing different
80+
* header names as properties. This case is already handled by
81+
* `PropertyWriteSink`.
10082
*/
10183
class HeaderNameSink extends Sink, DataFlow::ValueNode {
10284
HeaderNameSink() {
103-
exists (HTTP::ExplicitHeaderDefinition hd |
104-
not hd instanceof Express::SetMultipleHeaders and
105-
astNode = hd.getNameExpr()
106-
)
85+
exists (HTTP::ExplicitHeaderDefinition hd |
86+
not hd instanceof Express::SetMultipleHeaders and
87+
astNode = hd.getNameExpr()
88+
)
10789
}
10890

10991
override string getMessage() {

javascript/ql/test/query-tests/Security/CWE-400/RemotePropertyInjection.expected

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ nodes
33
| tst.js:8:13:8:52 | myCoolL ... rolled) |
44
| tst.js:8:28:8:51 | req.que ... trolled |
55
| tst.js:9:8:9:11 | prop |
6-
| tst.js:11:16:11:19 | prop |
76
| tst.js:13:15:13:18 | prop |
87
| tst.js:14:31:14:34 | prop |
98
| tst.js:16:10:16:13 | prop |
@@ -12,7 +11,6 @@ nodes
1211
| tstNonExpr.js:8:17:8:23 | userVal |
1312
edges
1413
| tst.js:8:6:8:52 | prop | tst.js:9:8:9:11 | prop |
15-
| tst.js:8:6:8:52 | prop | tst.js:11:16:11:19 | prop |
1614
| tst.js:8:6:8:52 | prop | tst.js:13:15:13:18 | prop |
1715
| tst.js:8:6:8:52 | prop | tst.js:14:31:14:34 | prop |
1816
| tst.js:8:6:8:52 | prop | tst.js:16:10:16:13 | prop |
@@ -22,7 +20,6 @@ edges
2220
| tstNonExpr.js:5:17:5:23 | req.url | tstNonExpr.js:5:7:5:23 | userVal |
2321
#select
2422
| tst.js:9:8:9:11 | prop | tst.js:8:28:8:51 | req.que ... trolled | tst.js:9:8:9:11 | prop | A $@ is used as a property name to write to. | tst.js:8:28:8:51 | req.que ... trolled | user-provided value |
25-
| tst.js:11:16:11:19 | prop | tst.js:8:28:8:51 | req.que ... trolled | tst.js:11:16:11:19 | prop | A $@ is used as a method name to be called. | tst.js:8:28:8:51 | req.que ... trolled | user-provided value |
2623
| tst.js:13:15:13:18 | prop | tst.js:8:28:8:51 | req.que ... trolled | tst.js:13:15:13:18 | prop | A $@ is used as a property name to write to. | tst.js:8:28:8:51 | req.que ... trolled | user-provided value |
2724
| tst.js:14:31:14:34 | prop | tst.js:8:28:8:51 | req.que ... trolled | tst.js:14:31:14:34 | prop | A $@ is used as a property name to write to. | tst.js:8:28:8:51 | req.que ... trolled | user-provided value |
2825
| tst.js:16:10:16:13 | prop | tst.js:8:28:8:51 | req.que ... trolled | tst.js:16:10:16:13 | prop | A $@ is used as a property name to write to. | tst.js:8:28:8:51 | req.que ... trolled | user-provided value |

javascript/ql/test/query-tests/Security/CWE-400/tst.js

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@ var myObj = {}
55

66
app.get('/user/:id', function(req, res) {
77
myCoolLocalFct(req.query.userControlled);
8-
var prop = myCoolLocalFct(req.query.userControlled);
8+
var prop = myCoolLocalFct(req.query.userControlled);
99
myObj[prop] = 23; // NOT OK
1010
myObj.prop = 23; // OK
11-
var x = myObj[prop]; // NOT OK
12-
x(23);
13-
delete myObj[prop]; // NOT OK
11+
var x = myObj[prop]; // NOT OK, but flagged by different query
12+
x(23);
13+
delete myObj[prop]; // NOT OK
1414
Object.defineProperty(myObj, prop, {value: 24}); // NOT OK
15-
var headers = {};
15+
var headers = {};
1616
headers[prop] = 42; // NOT OK
1717
res.set(headers);
1818
myCoolLocalFct[req.query.x](); // OK - flagged by method name injection
@@ -21,5 +21,5 @@ app.get('/user/:id', function(req, res) {
2121
function myCoolLocalFct(x) {
2222
var result = x;
2323
return result.substring(0, result.length);
24-
24+
2525
}

0 commit comments

Comments
 (0)