Skip to content

Commit 8891ae7

Browse files
aschackmullaschackmull
authored
Merge pull request github#3938 from lcartey/java/untrusted-data-to-external-api
Java: Untrusted data used in external APIs
2 parents 66541f2 + 6f83c55 commit 8891ae7

File tree

7 files changed

+310
-0
lines changed

7 files changed

+310
-0
lines changed

‎java/ql/src/Security/CWE/CWE-020/ExternalAPISinkExample.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
public class XSS extends HttpServlet {
2+
protected void doGet(HttpServletRequest request, HttpServletResponse response)
3+
throws ServletException, IOException {
4+
// BAD: a request parameter is written directly to an error response page
5+
response.sendError(HttpServletResponse.SC_NOT_FOUND,
6+
"The page \"" + request.getParameter("page") + "\" was not found.");
7+
}
8+
}

‎java/ql/src/Security/CWE/CWE-020/ExternalAPITaintStepExample.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
public class SQLInjection extends HttpServlet {
2+
protected void doGet(HttpServletRequest request, HttpServletResponse response)
3+
throws ServletException, IOException {
4+
5+
StringBuilder sqlQueryBuilder = new StringBuilder();
6+
sqlQueryBuilder.append("SELECT * FROM user WHERE user_id='");
7+
sqlQueryBuilder.append(request.getParameter("user_id"));
8+
sqlQueryBuilder.append("'");
9+
10+
// ...
11+
}
12+
}

‎java/ql/src/Security/CWE/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 method call to a method 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+
Java standard library, third party dependencies or from internal dependencies. The query will report the method
14+
signature with a fully qualified name, along with either <code>[param x]</code>, where <code>x</code> indicates the
15+
position of the parameter receiving the untrusted data or <code>[qualifier]</code> indicating the untrusted data is
16+
used as the qualifier to the method call.</p>
17+
18+
</overview>
19+
<recommendation>
20+
21+
<p>For each result:</p>
22+
23+
<ul>
24+
<li>If the result highlights a known sink, no action is required.</li>
25+
<li>If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query.</li>
26+
<li>If the result represents a call to an external API which transfers taint, add the appropriate modeling, and
27+
re-run the query to determine what new results have appeared due to this additional modeling.</li>
28+
</ul>
29+
30+
<p>Otherwise, the result is likely uninteresting. Custom versions of this query can extend the <code>SafeExternalAPIMethod</code>
31+
class to exclude known safe external APIs from future analysis.</p>
32+
33+
</recommendation>
34+
<example>
35+
36+
<p>If the query were to return the API <code>javax.servlet.http.HttpServletResponse.sendError(int, java.lang.String) [param 1]</code>
37+
then we should first 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 XSS query.</p>
39+
40+
<p>If the query were to return the API <code>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>

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
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 java/count-untrusted-data-external-api
7+
* @kind table
8+
* @tags security external/cwe/cwe-20
9+
*/
10+
11+
import java
12+
import semmle.code.java.security.ExternalAPIs
13+
import semmle.code.java.dataflow.DataFlow
14+
15+
from ExternalAPIUsedWithUntrustedData externalAPI
16+
select externalAPI, count(externalAPI.getUntrustedDataNode()) as numberOfUses,
17+
externalAPI.getNumberOfUntrustedSources() as numberOfUntrustedSources order by
18+
numberOfUntrustedSources desc

‎java/ql/src/Security/CWE/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+
Java standard library, third-party dependencies or from internal dependencies. The query reports uses of
12+
untrusted data in either the qualifier or as one of the arguments of external APIs.</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 request parameter is read from <code>HttpServletRequest</code> and then ultimately used in a call to the
35+
<code>HttpServletResponse.sendError</code> external API:</p>
36+
37+
<sample src="ExternalAPISinkExample.java" />
38+
39+
<p>This is an 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 request parameter is read from <code>HttpServletRequest</code>.</p>
44+
45+
<sample src="ExternalAPITaintStepExample.java" />
46+
47+
<p>If the query reported the call to <code>StringBuilder.append</code> on line 7, 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 likely that the result of the
50+
<code>StringBuilder</code> will be executed as an SQL query, potentially leading to an SQL injection vulnerability.</p>
51+
52+
<p>Note that both examples are correctly handled by the standard taint tracking library and XSS query.</p>
53+
</example>
54+
<references>
55+
56+
</references>
57+
</qhelp>

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
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 java/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 java
12+
import semmle.code.java.dataflow.FlowSources
13+
import semmle.code.java.dataflow.TaintTracking
14+
import semmle.code.java.security.ExternalAPIs
15+
import DataFlow::PathGraph
16+
17+
from UntrustedDataToExternalAPIConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
18+
where config.hasFlowPath(source, sink)
19+
select sink, source, sink,
20+
"Call to " + sink.getNode().(ExternalAPIDataNode).getMethodDescription() +
21+
" with untrusted data from $@.", source, source.toString()

‎java/ql/src/semmle/code/java/security/ExternalAPIs.qll

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
/**
2+
* Definitions for reasoning about untrusted data used in APIs defined outside the
3+
* database.
4+
*/
5+
6+
import java
7+
import semmle.code.java.dataflow.FlowSources
8+
import semmle.code.java.dataflow.TaintTracking
9+
10+
/**
11+
* A `Method` that is considered a "safe" external API from a security perspective.
12+
*/
13+
abstract class SafeExternalAPIMethod extends Method { }
14+
15+
/** The default set of "safe" external APIs. */
16+
private class DefaultSafeExternalAPIMethod extends SafeExternalAPIMethod {
17+
DefaultSafeExternalAPIMethod() {
18+
this instanceof EqualsMethod
19+
or
20+
getName().regexpMatch("size|length|compareTo|getClass|lastIndexOf")
21+
or
22+
this.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate")
23+
or
24+
getQualifiedName() = "Objects.equals"
25+
or
26+
getDeclaringType() instanceof TypeString and getName() = "equals"
27+
or
28+
getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions")
29+
or
30+
getDeclaringType().getPackage().getName().matches("org.junit%")
31+
or
32+
getDeclaringType().hasQualifiedName("com.google.common.base", "Strings") and
33+
getName() = "isNullOrEmpty"
34+
or
35+
getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "StringUtils") and
36+
getName() = "isNotEmpty"
37+
or
38+
getDeclaringType().hasQualifiedName("java.lang", "Character") and
39+
getName() = "isDigit"
40+
or
41+
getDeclaringType().hasQualifiedName("java.lang", "String") and
42+
getName().regexpMatch("equalsIgnoreCase|regionMatches")
43+
or
44+
getDeclaringType().hasQualifiedName("java.lang", "Boolean") and
45+
getName() = "parseBoolean"
46+
or
47+
getDeclaringType().hasQualifiedName("org.apache.commons.io", "IOUtils") and
48+
getName() = "closeQuietly"
49+
or
50+
getDeclaringType().hasQualifiedName("org.springframework.util", "StringUtils") and
51+
getName().regexpMatch("hasText|isEmpty")
52+
}
53+
}
54+
55+
/** A node representing data being passed to an external API. */
56+
class ExternalAPIDataNode extends DataFlow::Node {
57+
Call call;
58+
int i;
59+
60+
ExternalAPIDataNode() {
61+
(
62+
// Argument to call to a method
63+
this.asExpr() = call.getArgument(i)
64+
or
65+
// Qualifier to call to a method which returns non trivial value
66+
this.asExpr() = call.getQualifier() and
67+
i = -1 and
68+
not call.getCallee().getReturnType() instanceof VoidType and
69+
not call.getCallee().getReturnType() instanceof BooleanType
70+
) and
71+
// Defined outside the source archive
72+
not call.getCallee().fromSource() and
73+
// Not a call to an method which is overridden in source
74+
not exists(Method m |
75+
m.getASourceOverriddenMethod() = call.getCallee().getSourceDeclaration() and
76+
m.fromSource()
77+
) and
78+
// Not already modeled as a taint step
79+
not exists(DataFlow::Node next | TaintTracking::localTaintStep(this, next)) and
80+
// Not a call to a known safe external API
81+
not call.getCallee() instanceof SafeExternalAPIMethod
82+
}
83+
84+
/** Gets the called API `Method`. */
85+
Method getMethod() { result = call.getCallee() }
86+
87+
/** Gets the index which is passed untrusted data (where -1 indicates the qualifier). */
88+
int getIndex() { result = i }
89+
90+
/** Gets the description of the method being called. */
91+
string getMethodDescription() {
92+
result = getMethod().getDeclaringType().getPackage() + "." + getMethod().getQualifiedName()
93+
}
94+
}
95+
96+
/** A configuration for tracking flow from `RemoteFlowSource`s to `ExternalAPIDataNode`s. */
97+
class UntrustedDataToExternalAPIConfig extends TaintTracking::Configuration {
98+
UntrustedDataToExternalAPIConfig() { this = "UntrustedDataToExternalAPIConfig" }
99+
100+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
101+
102+
override predicate isSink(DataFlow::Node sink) { sink instanceof ExternalAPIDataNode }
103+
}
104+
105+
/** A node representing untrusted data being passed to an external API. */
106+
class UntrustedExternalAPIDataNode extends ExternalAPIDataNode {
107+
UntrustedExternalAPIDataNode() { any(UntrustedDataToExternalAPIConfig c).hasFlow(_, this) }
108+
109+
/** Gets a source of untrusted data which is passed to this external API data node. */
110+
DataFlow::Node getAnUntrustedSource() {
111+
any(UntrustedDataToExternalAPIConfig c).hasFlow(result, this)
112+
}
113+
}
114+
115+
private newtype TExternalAPI =
116+
TExternalAPIParameter(Method m, int index) {
117+
exists(UntrustedExternalAPIDataNode n |
118+
m = n.getMethod() and
119+
index = n.getIndex()
120+
)
121+
}
122+
123+
/** An external API which is used with untrusted data. */
124+
class ExternalAPIUsedWithUntrustedData extends TExternalAPI {
125+
/** Gets a possibly untrusted use of this external API. */
126+
UntrustedExternalAPIDataNode getUntrustedDataNode() {
127+
this = TExternalAPIParameter(result.getMethod(), result.getIndex())
128+
}
129+
130+
/** Gets the number of untrusted sources used with this external API. */
131+
int getNumberOfUntrustedSources() {
132+
result = count(getUntrustedDataNode().getAnUntrustedSource())
133+
}
134+
135+
/** Gets a textual representation of this element. */
136+
string toString() {
137+
exists(Method m, int index, string indexString |
138+
if index = -1 then indexString = "qualifier" else indexString = "param " + index
139+
|
140+
this = TExternalAPIParameter(m, index) and
141+
result =
142+
m.getDeclaringType().(RefType).getQualifiedName() + "." + m.getSignature() + " [" +
143+
indexString + "]"
144+
)
145+
}
146+
}

0 commit comments

Comments
 (0)