Skip to content

Commit b242a61

Browse files
lcarteylcartey
committed
Java: Untrusted data used in external APIs
This commit adds two queries for identifying external APIs which are used with untrusted data. These queries are intended to facilitate a security review of the application, and will report any external API which is called with untrusted data. The purpose of this is to: - review how untrusted data flows through this application - identify opportunities to improve taint modeling of sinks and taint steps. As a result this is not suitable for integration into a developer workflow, as it will likely have high false positive rate, but it may help identify false negatives for other queries.
1 parent 04a0d47 commit b242a61

File tree

7 files changed

+311
-0
lines changed

7 files changed

+311
-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 which are used with untrusted data, along with how frequently the API is used, and how many
8+
unique sources of untrusted data flow 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 which 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.
32+
33+
</recommendation>
34+
<example>
35+
36+
<p>If the query were to return the API <code>javax.servlet.http.HttpServletResponse.sendError(int sc, java.lang.String msg) [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 str) [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 with 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: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/**
2+
* @name Frequency counts for external APIs which are used with untrusted data
3+
* @description This reports the external APIs which 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+
*/
9+
10+
import java
11+
import semmle.code.java.security.ExternalAPIs::ExternalAPIs
12+
import semmle.code.java.dataflow.DataFlow
13+
14+
from ExternalAPIUsedWithUntrustedData externalAPI
15+
select externalAPI, count(externalAPI.getUntrustedDataNode()) as numberOfUses,
16+
externalAPI.getNumberOfUntrustedSources() as numberOfUntrustedSources order by
17+
numberOfUntrustedSources desc

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

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
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 uses of external APIs with untrusted data for review. This query has a deliberately low true positive rate,
8+
and is designed to help security reviews for the application, as well as helping identify external APIs that
9+
should be modeled as either taint steps, or sinks for specific problems.</p>
10+
11+
<p>An external API is defined as a method call to a method which 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 uses of
14+
untrusted data in either the qualifier or as one of the arguments of external APIs.</p>
15+
16+
</overview>
17+
<recommendation>
18+
19+
<p>For each result:</p>
20+
21+
<ul>
22+
<li>If the result highlights a known sink, confirm that the result is reported by the relevant query, or
23+
that the result is a false positive due to sanitization.</li>
24+
<li>If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query,
25+
and confirm that the result is either found, or is safe due to appropriate sanitization.</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.
32+
33+
</recommendation>
34+
<example>
35+
36+
<p>In this first example, a request parameter is read from <code>HttpServletRequest</code> and then ultimately used in a call to the
37+
<code>HttpServletResponse.sendError</code> external API:
38+
39+
<sample src="ExternalAPISinkExample.java" />
40+
41+
<p>This is an XSS sink. The XSS query should therefore be reviewed to confirm that this sink is appropriately modeled,
42+
and if it is, to confirm that the query reports this particular result, or that the result is false positive due to
43+
some existing sanitization.</p>
44+
45+
<p>In this second example, again a request parameter is read from <code>HttpServletRequest</code>.</p>
46+
47+
<sample src="ExternalAPITaintStepExample.java" />
48+
49+
<p>If the query reported the call to <code>StringBuilder.append</code> on Line 7, this would suggest that this external API is
50+
not currently modeled as a taint step in the taint tracking library. The next step would be to model this as taint step, then
51+
re-run the query to determine what additional results might be found. In this example, it seems likely that the result of the
52+
<code>StringBuilder</code> will be executed as an SQL query, potentially leading to an SQL injection vulnerability.</p>
53+
54+
<p>Note that both examples are correctly handled with the standard taint tracking library and XSS query.</p>
55+
</example>
56+
<references>
57+
58+
</references>
59+
</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.
4+
* @id java/untrusted-data-to-external-api
5+
* @kind path-problem
6+
* @precision very-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::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+
module ExternalAPIs {
11+
/**
12+
* A `Method` which is considered a "safe" external API from a security perspective.
13+
*/
14+
abstract class SafeExternalAPIMethod extends Method { }
15+
16+
/** The default set of "safe" external APIs. */
17+
class DefaultSafeExternalAPIMethod extends SafeExternalAPIMethod {
18+
DefaultSafeExternalAPIMethod() {
19+
this instanceof EqualsMethod
20+
or
21+
getName().regexpMatch("size|length|compareTo|getClass|lastIndexOf")
22+
or
23+
this.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate")
24+
or
25+
getQualifiedName() = "Objects.equals"
26+
or
27+
getDeclaringType().getQualifiedName() = "java.lang.String" and getName() = "equals"
28+
or
29+
getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions")
30+
or
31+
getDeclaringType().getPackage().getName().matches("org.junit%")
32+
or
33+
getDeclaringType().hasQualifiedName("com.google.common.base", "Strings") and
34+
getName() = "isNullOrEmpty"
35+
or
36+
getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "StringUtils") and
37+
getName() = "isNotEmpty"
38+
or
39+
getDeclaringType().hasQualifiedName("java.lang", "Character") and
40+
getName() = "isDigit"
41+
or
42+
getDeclaringType().hasQualifiedName("java.lang", "String") and
43+
getName().regexpMatch("equalsIgnoreCase|regionMatches")
44+
or
45+
getDeclaringType().hasQualifiedName("java.lang", "Boolean") and
46+
getName() = "parseBoolean"
47+
or
48+
getDeclaringType().hasQualifiedName("org.apache.commons.io", "IOUtils") and
49+
getName() = "closeQuietly"
50+
or
51+
getDeclaringType().hasQualifiedName("org.springframework.util", "StringUtils") and
52+
getName().regexpMatch("hasText|isEmpty")
53+
}
54+
}
55+
56+
/** A node representing data being passed to an external API. */
57+
class ExternalAPIDataNode extends DataFlow::Node {
58+
Call call;
59+
int i;
60+
61+
ExternalAPIDataNode() {
62+
(
63+
// Argument to call to a method
64+
this.asExpr() = call.getArgument(i)
65+
or
66+
// Qualifier to call to a method which returns non trivial value
67+
this.asExpr() = call.getQualifier() and
68+
i = -1 and
69+
not call.getCallee().getReturnType() instanceof VoidType and
70+
not call.getCallee().getReturnType() instanceof BooleanType
71+
) and
72+
// Defined outside the source archive
73+
not call.getCallee().fromSource() and
74+
// Not a call to an method which is overridden in source
75+
not exists(Method m |
76+
m.getASourceOverriddenMethod() = call.getCallee().getSourceDeclaration() and
77+
m.fromSource()
78+
) and
79+
// Not already modelled as a taint step
80+
not exists(DataFlow::Node next | TaintTracking::localTaintStep(this, next)) and
81+
// Not a call to a known safe external API
82+
not call.getCallee() instanceof SafeExternalAPIMethod
83+
}
84+
85+
/** Gets the called API `Method`. */
86+
Method getMethod() { result = call.getCallee() }
87+
88+
/** Gets the index which is passed untrusted data (where -1 indicates the qualifier). */
89+
int getIndex() { result = i }
90+
91+
/** Gets the description of the method being called. */
92+
string getMethodDescription() {
93+
result =
94+
getMethod().getDeclaringType().getPackage() + "." + getMethod().getQualifiedName()
95+
}
96+
}
97+
98+
class UntrustedDataToExternalAPIConfig extends TaintTracking::Configuration {
99+
UntrustedDataToExternalAPIConfig() { this = "UntrustedDataToExternalAPIConfig" }
100+
101+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
102+
103+
override predicate isSink(DataFlow::Node sink) { sink instanceof ExternalAPIDataNode }
104+
}
105+
106+
/** A node representing untrusted data being passed to an external API. */
107+
class UntrustedExternalAPIDataNode extends ExternalAPIDataNode {
108+
UntrustedExternalAPIDataNode() { any(UntrustedDataToExternalAPIConfig c).hasFlow(_, this) }
109+
110+
/** Gets a source of untrusted data which is passed to this external API data node. */
111+
DataFlow::Node getAnUntrustedSource() {
112+
any(UntrustedDataToExternalAPIConfig c).hasFlow(result, this)
113+
}
114+
}
115+
116+
private newtype TExternalAPI =
117+
TExternalAPIParameter(Method m, int index) {
118+
exists(UntrustedExternalAPIDataNode n |
119+
m = n.getMethod() and
120+
index = n.getIndex()
121+
)
122+
}
123+
124+
/** An external API which is used with untrusted data. */
125+
class ExternalAPIUsedWithUntrustedData extends TExternalAPI {
126+
/** Gets a possibly untrusted use of this external API. */
127+
UntrustedExternalAPIDataNode getUntrustedDataNode() {
128+
this = TExternalAPIParameter(result.getMethod(), result.getIndex())
129+
}
130+
131+
/** Gets the number of untrusted sources used with this external API. */
132+
int getNumberOfUntrustedSources() {
133+
result = count(getUntrustedDataNode().getAnUntrustedSource())
134+
}
135+
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() + " [" + indexString + "]"
143+
)
144+
}
145+
}
146+
}

0 commit comments

Comments
 (0)