Skip to content

Commit cedc9c0

Browse files
erik-krogherik-krogh
authored
Merge pull request #11582 from erik-krogh/heuristics
JS: Add experimental variants of common security queries with more sources
2 parents c513867 + 66be8cd commit cedc9c0

File tree

71 files changed

+1705
-1061
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

71 files changed

+1705
-1061
lines changed

‎javascript/ql/lib/semmle/javascript/security/dataflow/ConditionalBypassQuery.qll

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,3 +30,96 @@ class Configuration extends TaintTracking::Configuration {
3030
dst.asExpr().(Comparison).hasOperands(src.asExpr(), any(ConstantExpr c))
3131
}
3232
}
33+
34+
/**
35+
* Holds if the value of `nd` flows into `guard`.
36+
*/
37+
predicate flowsToGuardExpr(DataFlow::Node nd, SensitiveActionGuardConditional guard) {
38+
nd = guard or
39+
flowsToGuardExpr(nd.getASuccessor(), guard)
40+
}
41+
42+
/**
43+
* A comparison that guards a sensitive action, e.g. the comparison in:
44+
* `var ok = x == y; if (ok) login()`.
45+
*/
46+
class SensitiveActionGuardComparison extends Comparison {
47+
SensitiveActionGuardConditional guard;
48+
49+
SensitiveActionGuardComparison() { flowsToGuardExpr(DataFlow::valueNode(this), guard) }
50+
51+
/**
52+
* Gets the guard that uses this comparison.
53+
*/
54+
SensitiveActionGuardConditional getGuard() { result = guard }
55+
}
56+
57+
/**
58+
* An intermediary sink to enable reuse of the taint configuration.
59+
* This sink should not be presented to the client of this query.
60+
*/
61+
class SensitiveActionGuardComparisonOperand extends Sink {
62+
SensitiveActionGuardComparison comparison;
63+
64+
SensitiveActionGuardComparisonOperand() { asExpr() = comparison.getAnOperand() }
65+
66+
override SensitiveAction getAction() { result = comparison.getGuard().getAction() }
67+
}
68+
69+
/**
70+
* Holds if `sink` guards `action`, and `source` taints `sink`.
71+
*
72+
* If flow from `source` taints `sink`, then an attacker can
73+
* control if `action` should be executed or not.
74+
*/
75+
predicate isTaintedGuardForSensitiveAction(
76+
DataFlow::PathNode sink, DataFlow::PathNode source, SensitiveAction action
77+
) {
78+
action = sink.getNode().(Sink).getAction() and
79+
// exclude the intermediary sink
80+
not sink.getNode() instanceof SensitiveActionGuardComparisonOperand and
81+
exists(Configuration cfg |
82+
// ordinary taint tracking to a guard
83+
cfg.hasFlowPath(source, sink)
84+
or
85+
// taint tracking to both operands of a guard comparison
86+
exists(
87+
SensitiveActionGuardComparison cmp, DataFlow::PathNode lSource, DataFlow::PathNode rSource,
88+
DataFlow::PathNode lSink, DataFlow::PathNode rSink
89+
|
90+
sink.getNode() = cmp.getGuard() and
91+
cfg.hasFlowPath(lSource, lSink) and
92+
lSink.getNode() = DataFlow::valueNode(cmp.getLeftOperand()) and
93+
cfg.hasFlowPath(rSource, rSink) and
94+
rSink.getNode() = DataFlow::valueNode(cmp.getRightOperand())
95+
|
96+
source = lSource or
97+
source = rSource
98+
)
99+
)
100+
}
101+
102+
/**
103+
* Holds if `e` effectively guards access to `action` by returning or throwing early.
104+
*
105+
* Example: `if (e) return; action(x)`.
106+
*/
107+
predicate isEarlyAbortGuard(DataFlow::PathNode e, SensitiveAction action) {
108+
exists(IfStmt guard |
109+
// `e` is in the condition of an if-statement ...
110+
e.getNode().(Sink).asExpr().getParentExpr*() = guard.getCondition() and
111+
// ... where the then-branch always throws or returns
112+
exists(Stmt abort |
113+
abort instanceof ThrowStmt or
114+
abort instanceof ReturnStmt
115+
|
116+
abort.nestedIn(guard) and
117+
abort.getBasicBlock().(ReachableBasicBlock).postDominates(guard.getThen().getBasicBlock())
118+
) and
119+
// ... and the else-branch does not exist
120+
not exists(guard.getElse())
121+
|
122+
// ... and `action` is outside the if-statement
123+
not action.asExpr().getEnclosingStmt().nestedIn(guard)
124+
)
125+
}

‎javascript/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.inc.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.qhelp

Lines changed: 4 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,57 +1,6 @@
11
<!DOCTYPE qhelp PUBLIC
2-
"-//Semmle//qhelp//EN"
3-
"qhelp.dtd">
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
44
<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>
5+
<include src="UntrustedDataToExternalAPI.inc.qhelp" />
6+
</qhelp>

‎javascript/ql/src/Security/CWE-078/CommandInjection.inc.qhelp

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Code that passes user input directly to
7+
<code>require('child_process').exec</code>, or some other library
8+
routine that executes a command, allows the user to execute malicious
9+
code.</p>
10+
11+
</overview>
12+
<recommendation>
13+
14+
<p>If possible, use hard-coded string literals to specify the command to run
15+
or library to load. Instead of passing the user input directly to the
16+
process or library function, examine the user input and then choose
17+
among hard-coded string literals.</p>
18+
19+
<p>If the applicable libraries or commands cannot be determined at
20+
compile time, then add code to verify that the user input string is
21+
safe before using it.</p>
22+
23+
</recommendation>
24+
<example>
25+
26+
<p>The following example shows code that takes a shell script that can be changed
27+
maliciously by a user, and passes it straight to <code>child_process.exec</code>
28+
without examining it first.</p>
29+
30+
<sample src="examples/command-injection.js" />
31+
32+
</example>
33+
<references>
34+
35+
<li>
36+
OWASP:
37+
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
38+
</li>
39+
40+
<!-- LocalWords: CWE untrusted unsanitized Runtime
41+
-->
42+
43+
</references>
44+
</qhelp>

‎javascript/ql/src/Security/CWE-078/CommandInjection.qhelp

Lines changed: 4 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,6 @@
11
<!DOCTYPE qhelp PUBLIC
2-
"-//Semmle//qhelp//EN"
3-
"qhelp.dtd">
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
44
<qhelp>
5-
<overview>
6-
<p>Code that passes user input directly to
7-
<code>require('child_process').exec</code>, or some other library
8-
routine that executes a command, allows the user to execute malicious
9-
code.</p>
10-
11-
</overview>
12-
<recommendation>
13-
14-
<p>If possible, use hard-coded string literals to specify the command to run
15-
or library to load. Instead of passing the user input directly to the
16-
process or library function, examine the user input and then choose
17-
among hard-coded string literals.</p>
18-
19-
<p>If the applicable libraries or commands cannot be determined at
20-
compile time, then add code to verify that the user input string is
21-
safe before using it.</p>
22-
23-
</recommendation>
24-
<example>
25-
26-
<p>The following example shows code that takes a shell script that can be changed
27-
maliciously by a user, and passes it straight to <code>child_process.exec</code>
28-
without examining it first.</p>
29-
30-
<sample src="examples/command-injection.js" />
31-
32-
</example>
33-
<references>
34-
35-
<li>
36-
OWASP:
37-
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
38-
</li>
39-
40-
<!-- LocalWords: CWE untrusted unsanitized Runtime
41-
-->
42-
43-
</references>
44-
</qhelp>
5+
<include src="CommandInjection.inc.qhelp" />
6+
</qhelp>

‎javascript/ql/src/Security/CWE-079/Xss.inc.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+
6+
<overview>
7+
<p>
8+
Directly writing user input (for example, a URL query parameter) to a webpage
9+
without properly sanitizing the input first, allows for a cross-site scripting vulnerability.
10+
</p>
11+
<p>
12+
This kind of vulnerability is also called <i>DOM-based</i> cross-site scripting, to distinguish
13+
it from other types of cross-site scripting.
14+
</p>
15+
</overview>
16+
17+
<recommendation>
18+
<p>
19+
To guard against cross-site scripting, consider using contextual output encoding/escaping before
20+
writing user input to the page, or one of the other solutions that are mentioned in the
21+
references.
22+
</p>
23+
</recommendation>
24+
25+
<example>
26+
<p>
27+
The following example shows part of the page URL being written directly to the document,
28+
leaving the website vulnerable to cross-site scripting.
29+
</p>
30+
<sample src="examples/Xss.js" />
31+
</example>
32+
33+
<references>
34+
<li>
35+
OWASP:
36+
<a href="https://cheatsheetseries.owasp.org/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.html">DOM based
37+
XSS Prevention Cheat Sheet</a>.
38+
</li>
39+
<li>
40+
OWASP:
41+
<a href="https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html">XSS
42+
(Cross Site Scripting) Prevention Cheat Sheet</a>.
43+
</li>
44+
<li>
45+
OWASP
46+
<a href="https://www.owasp.org/index.php/DOM_Based_XSS">DOM Based XSS</a>.
47+
</li>
48+
<li>
49+
OWASP
50+
<a href="https://www.owasp.org/index.php/Types_of_Cross-Site_Scripting">Types of Cross-Site
51+
Scripting</a>.
52+
</li>
53+
<li>
54+
Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
55+
</li>
56+
</references>
57+
</qhelp>

0 commit comments

Comments
 (0)