Skip to content

Commit adc7bbf

Browse files
asgerfasgerf
authored
Merge pull request github#4694 from asgerf/js/flow-to-external-api
JS: Add UntrustedDataToExternalAPI query
2 parents 14aa642 + 16429c8 commit adc7bbf

12 files changed

+795
-0
lines changed

‎javascript/ql/src/Security/CWE-020/ExternalAPISinkExample.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
express().get('/news', (req, res) => {
2+
let topic = req.query.topic;
3+
res.send(`<h1>${topic}</h1>`);
4+
});

‎javascript/ql/src/Security/CWE-020/ExternalAPITaintStepExample.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
let path = require('path');
2+
3+
express().get('/data', (req, res) => {
4+
let file = path.join(HOME_DIR, 'public', req.query.file);
5+
res.sendFile(file);
6+
});

‎javascript/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports
7+
all external APIs that are used with untrusted data, along with how frequently the API is used, and how many
8+
unique sources of untrusted data flow to this API. This query is designed primarily to help identify which APIs
9+
may be relevant for security analysis of this application.</p>
10+
11+
<p>An external API is defined as a call to a function that is not defined in the source code, not overridden
12+
in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the
13+
third party dependencies or from internal dependencies. The query will report the external package name, followed
14+
by an access path leading to the function, followed by <code>[param x]</code> where <code>x</code>
15+
indicates the position of the parameter receiving the untrusted data.</p>
16+
17+
</overview>
18+
<recommendation>
19+
20+
<p>For each result:</p>
21+
22+
<ul>
23+
<li>If the result highlights a known sink, no action is required.</li>
24+
<li>If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query.</li>
25+
<li>If the result represents a call to an external API which transfers taint, add the appropriate modeling, and
26+
re-run the query to determine what new results have appeared due to this additional modeling.</li>
27+
</ul>
28+
29+
<p>Otherwise, the result is likely uninteresting. Custom versions of this query can extend the <code>SafeExternalAPIMethod</code>
30+
class to exclude known safe external APIs from future analysis.</p>
31+
32+
</recommendation>
33+
<example>
34+
35+
<p>If the query were to return the API <code>express().get.[callback].[param 'res'].send() [param 0]</code>,
36+
this could correspond to the <code>X</code> in <code>express().get('/foo', (req, res) => res.send(X))</code>.
37+
First we should consider whether this a security relevant sink. In this case, this is writing to a HTTP response, so we should
38+
consider whether this is an XSS sink. If it is, we should confirm that it is handled by the reflected XSS query.</p>
39+
40+
<p>If the query were to return the API <code>url.parse java.lang.StringBuilder.append(java.lang.String) [param 0]</code>, then this should be
41+
reviewed as a possible taint step, because tainted data would flow from the 0th argument to the qualifier of the call.</p>
42+
43+
<p>Note that both examples are correctly handled by the standard taint tracking library and XSS query.</p>
44+
</example>
45+
<references>
46+
47+
</references>
48+
</qhelp>

‎javascript/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.ql

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/**
2+
* @name Frequency counts for external APIs that are used with untrusted data
3+
* @description This reports the external APIs that are used with untrusted data, along with how
4+
* frequently the API is called, and how many unique sources of untrusted data flow
5+
* to it.
6+
* @id js/count-untrusted-data-external-api
7+
* @kind table
8+
* @tags security external/cwe/cwe-20
9+
*/
10+
11+
import javascript
12+
import semmle.javascript.security.dataflow.ExternalAPIUsedWithUntrustedData::ExternalAPIUsedWithUntrustedData
13+
14+
from ExternalAPIUsedWithUntrustedData externalAPI
15+
select externalAPI, count(externalAPI.getUntrustedDataNode()) as numberOfUses,
16+
externalAPI.getNumberOfUntrustedSources() as numberOfUntrustedSources order by
17+
numberOfUntrustedSources desc

‎javascript/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelp

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports
7+
external APIs that use untrusted data. The results are not filtered so that you can audit all examples. The query provides data for security reviews of the application and you can also use it to identify external APIs that should be modeled as either taint steps, or sinks for specific problems.</p>
8+
9+
<p>An external API is defined as a method call to a method that is not defined in the source code, not overridden
10+
in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the
11+
third-party dependencies or from internal dependencies. The query reports uses of
12+
untrusted data one of the arguments of external API call or in the return value from a callback passed to an external API.</p>
13+
14+
</overview>
15+
<recommendation>
16+
17+
<p>For each result:</p>
18+
19+
<ul>
20+
<li>If the result highlights a known sink, confirm that the result is reported by the relevant query, or
21+
that the result is a false positive because this data is sanitized.</li>
22+
<li>If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query,
23+
and confirm that the result is either found, or is safe due to appropriate sanitization.</li>
24+
<li>If the result represents a call to an external API that transfers taint, add the appropriate modeling, and
25+
re-run the query to determine what new results have appeared due to this additional modeling.</li>
26+
</ul>
27+
28+
<p>Otherwise, the result is likely uninteresting. Custom versions of this query can extend the <code>SafeExternalAPIMethod</code>
29+
class to exclude known safe external APIs from future analysis.</p>
30+
31+
</recommendation>
32+
<example>
33+
34+
<p>In this first example, a query parameter is read from the <code>req</code> parameter and then ultimately used in a call to the
35+
<code>res.send</code> external API:</p>
36+
37+
<sample src="ExternalAPISinkExample.js" />
38+
39+
<p>This is a reflected XSS sink. The XSS query should therefore be reviewed to confirm that this sink is appropriately modeled,
40+
and if it is, to confirm that the query reports this particular result, or that the result is a false positive due to
41+
some existing sanitization.</p>
42+
43+
<p>In this second example, again a query parameter is read from <code>req</code>.</p>
44+
45+
<sample src="ExternalAPITaintStepExample.js" />
46+
47+
<p>If the query reported the call to <code>path.join</code> on line 4, this would suggest that this external API is
48+
not currently modeled as a taint step in the taint tracking library. The next step would be to model this as a taint step, then
49+
re-run the query to determine what additional results might be found. In this example, it seems the result of the
50+
<code>path.join</code> will be used as a file path, leading to a path traversal vulnerability.</p>
51+
52+
<p>Note that both examples are correctly handled by the standard taint tracking library and security queries.</p>
53+
</example>
54+
<references>
55+
56+
</references>
57+
</qhelp>

‎javascript/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.ql

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
/**
2+
* @name Untrusted data passed to external API
3+
* @description Data provided remotely is used in this external API without sanitization, which could be a security risk.
4+
* @id js/untrusted-data-to-external-api
5+
* @kind path-problem
6+
* @precision low
7+
* @problem.severity error
8+
* @tags security external/cwe/cwe-20
9+
*/
10+
11+
import javascript
12+
import semmle.javascript.security.dataflow.ExternalAPIUsedWithUntrustedData::ExternalAPIUsedWithUntrustedData
13+
import DataFlow::PathGraph
14+
15+
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
16+
where config.hasFlowPath(source, sink)
17+
select sink, source, sink,
18+
"Call to " + sink.getNode().(Sink).getApiName() + " with untrusted data from $@.", source,
19+
source.toString()

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -519,6 +519,11 @@ class FunctionNode extends DataFlow::ValueNode, DataFlow::SourceNode {
519519
*/
520520
class ObjectLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode {
521521
override ObjectExpr astNode;
522+
523+
/** Gets the value of a spread property of this object literal, such as `x` in `{...x}` */
524+
DataFlow::Node getASpreadProperty() {
525+
result = astNode.getAProperty().(SpreadProperty).getInit().(SpreadElement).getOperand().flow()
526+
}
522527
}
523528

524529
/**

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

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about untrusted
3+
* data flowing to an external API call.
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `ExternalAPIUsedWithUntrustedData::Configuration` is needed, otherwise
7+
* `ExternalAPIUsedWithUntrustedDataCustomizations` should be imported instead.
8+
*/
9+
10+
import javascript
11+
12+
/**
13+
* Provides a taint tracking configuration for reasoning about untrusted
14+
* data flowing to an external API call.
15+
*/
16+
module ExternalAPIUsedWithUntrustedData {
17+
import ExternalAPIUsedWithUntrustedDataCustomizations::ExternalAPIUsedWithUntrustedData
18+
19+
/** Flow label for objects from which a tainted value is reachable. */
20+
private class ObjectWrapperFlowLabel extends DataFlow::FlowLabel {
21+
ObjectWrapperFlowLabel() { this = "object-wrapper" }
22+
}
23+
24+
/**
25+
* A taint tracking configuration for untrusted data flowing to an external API.
26+
*/
27+
class Configuration extends TaintTracking::Configuration {
28+
Configuration() { this = "ExternalAPIUsedWithUntrustedData" }
29+
30+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
31+
32+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel lbl) {
33+
sink instanceof Sink and
34+
(lbl.isTaint() or lbl instanceof ObjectWrapperFlowLabel)
35+
}
36+
37+
override predicate isSanitizer(DataFlow::Node node) {
38+
super.isSanitizer(node) or
39+
node instanceof Sanitizer
40+
}
41+
42+
override predicate isAdditionalFlowStep(
43+
DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predLbl,
44+
DataFlow::FlowLabel succLbl
45+
) {
46+
// Step into an object and switch to the 'object-wrapper' label.
47+
exists(DataFlow::PropWrite write |
48+
pred = write.getRhs() and
49+
succ = write.getBase().getALocalSource() and
50+
(predLbl.isTaint() or predLbl instanceof ObjectWrapperFlowLabel) and
51+
succLbl instanceof ObjectWrapperFlowLabel
52+
)
53+
}
54+
55+
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
56+
// Block flow from the ___location to its properties, as the relevant properties (hash and search) are taint sources of their own.
57+
// The ___location source is only used for propagating through API calls like `new URL(___location)` and into external APIs where
58+
// the whole ___location object escapes.
59+
exists(DataFlow::PropRead read |
60+
read = DOM::locationRef().getAPropertyRead() and
61+
pred = read.getBase() and
62+
succ = read
63+
)
64+
}
65+
}
66+
67+
/** A node representing data being passed to an external API. */
68+
class ExternalAPIDataNode extends DataFlow::Node {
69+
ExternalAPIDataNode() { this instanceof Sink }
70+
}
71+
72+
/** A node representing untrusted data being passed to an external API. */
73+
class UntrustedExternalAPIDataNode extends ExternalAPIDataNode {
74+
UntrustedExternalAPIDataNode() { any(Configuration c).hasFlow(_, this) }
75+
76+
/** Gets a source of untrusted data which is passed to this external API data node. */
77+
DataFlow::Node getAnUntrustedSource() { any(Configuration c).hasFlow(result, this) }
78+
}
79+
80+
/**
81+
* Name of an external API sink, boxed in a newtype for consistency with other languages.
82+
*/
83+
private newtype TExternalApi =
84+
MkExternalApiNode(string name) {
85+
exists(Sink sink |
86+
any(Configuration c).hasFlow(_, sink) and
87+
name = sink.getApiName()
88+
)
89+
}
90+
91+
/** An external API which is used with untrusted data. */
92+
class ExternalAPIUsedWithUntrustedData extends TExternalApi {
93+
/** Gets a possibly untrusted use of this external API. */
94+
UntrustedExternalAPIDataNode getUntrustedDataNode() {
95+
this = MkExternalApiNode(result.(Sink).getApiName())
96+
}
97+
98+
/** Gets the number of untrusted sources used with this external API. */
99+
int getNumberOfUntrustedSources() {
100+
result = count(getUntrustedDataNode().getAnUntrustedSource())
101+
}
102+
103+
/** Gets a textual representation of this element. */
104+
string toString() { this = MkExternalApiNode(result) }
105+
}
106+
}

0 commit comments

Comments
 (0)