From b242a6170121096e8c06b008e35a32a4e417182c Mon Sep 17 00:00:00 2001 From: "lcartey@github.com" Date: Fri, 3 Jul 2020 17:32:08 +0100 Subject: [PATCH 01/10] 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. --- .../CWE/CWE-020/ExternalAPISinkExample.java | 8 + .../CWE-020/ExternalAPITaintStepExample.java | 12 ++ .../ExternalAPIsUsedWithUntrustedData.qhelp | 48 ++++++ .../ExternalAPIsUsedWithUntrustedData.ql | 17 ++ .../CWE-020/UntrustedDataToExternalAPI.qhelp | 59 +++++++ .../CWE/CWE-020/UntrustedDataToExternalAPI.ql | 21 +++ .../code/java/security/ExternalAPIs.qll | 146 ++++++++++++++++++ 7 files changed, 311 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-020/ExternalAPISinkExample.java create mode 100644 java/ql/src/Security/CWE/CWE-020/ExternalAPITaintStepExample.java create mode 100644 java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql create mode 100644 java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql create mode 100644 java/ql/src/semmle/code/java/security/ExternalAPIs.qll diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPISinkExample.java b/java/ql/src/Security/CWE/CWE-020/ExternalAPISinkExample.java new file mode 100644 index 00000000000..273da84901c --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPISinkExample.java @@ -0,0 +1,8 @@ +public class XSS extends HttpServlet { + protected void doGet(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + // BAD: a request parameter is written directly to an error response page + response.sendError(HttpServletResponse.SC_NOT_FOUND, + "The page \"" + request.getParameter("page") + "\" was not found."); + } +} diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPITaintStepExample.java b/java/ql/src/Security/CWE/CWE-020/ExternalAPITaintStepExample.java new file mode 100644 index 00000000000..65735194a69 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPITaintStepExample.java @@ -0,0 +1,12 @@ +public class SQLInjection extends HttpServlet { + protected void doGet(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + + StringBuilder sqlQueryBuilder = new StringBuilder(); + sqlQueryBuilder.append("SELECT * FROM user WHERE user_id='"); + sqlQueryBuilder.append(request.getParameter("user_id")); + sqlQueryBuilder.append("'"); + + // ... + } +} diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp new file mode 100644 index 00000000000..ff8febbd397 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp @@ -0,0 +1,48 @@ + + + +

Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports +all external APIs which are used with untrusted data, along with how frequently the API is used, and how many +unique sources of untrusted data flow this API. This query is designed primarily to help identify which APIs +may be relevant for security analysis of this application

+ +

An external API is defined as a method call to a method which is not defined in the source code, not overridden +in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the +Java standard library, third party dependencies or from internal dependencies. The query will report the method +signature with a fully qualified name, along with either [param x], where x indicates the +position of the parameter receiving the untrusted data or [qualifier] indicating the untrusted data is +used as the qualifier to the method call.

+ + + + +

For each result:

+ +
    +
  • If the result highlights a known sink, no action is required.
    • +
  • If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query.
    • +
  • If the result represents a call to an external API which transfers taint, add the appropriate modeling, and + re-run the query to determine what new results have appeared due to this additional modeling.
    • +
+ +

Otherwise, the result is likely uninteresting. Custom versions of this query can extend the SafeExternalAPIMethod +class to exclude known safe external APIs from future analysis. + + + + +

If the query were to return the API javax.servlet.http.HttpServletResponse.sendError(int sc, java.lang.String msg) [param 1] +then we should first consider whether this a security relevant sink. In this case, this is writing to a HTTP response, so we should +consider whether this is an XSS sink. If it is, we should confirm that it is handled by the XSS query.

+ +

If the query were to return the API java.lang.StringBuilder.append(java.lang.String str) [param 0], then this should be +reviewed as a possible taint step, because tainted data would flow from the 0th argument to the qualifier of the call.

+ +

Note that both examples are correctly handled with the standard taint tracking library and XSS query.

+ + + + + diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql new file mode 100644 index 00000000000..bbe627e0490 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql @@ -0,0 +1,17 @@ +/** + * @name Frequency counts for external APIs which are used with untrusted data + * @description This reports the external APIs which are used with untrusted data, along with how + * frequently the API is called, and how many unique sources of untrusted data flow + * to it. + * @id java/count-untrusted-data-external-api + * @kind table + */ + +import java +import semmle.code.java.security.ExternalAPIs::ExternalAPIs +import semmle.code.java.dataflow.DataFlow + +from ExternalAPIUsedWithUntrustedData externalAPI +select externalAPI, count(externalAPI.getUntrustedDataNode()) as numberOfUses, + externalAPI.getNumberOfUntrustedSources() as numberOfUntrustedSources order by + numberOfUntrustedSources desc diff --git a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp new file mode 100644 index 00000000000..855d0ca30e4 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp @@ -0,0 +1,59 @@ + + + +

Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports +all uses of external APIs with untrusted data for review. This query has a deliberately low true positive rate, +and is designed to help security reviews for the application, as well as helping identify external APIs that +should be modeled as either taint steps, or sinks for specific problems.

+ +

An external API is defined as a method call to a method which is not defined in the source code, not overridden +in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the +Java standard library, third party dependencies or from internal dependencies. The query will report uses of +untrusted data in either the qualifier or as one of the arguments of external APIs.

+ + + + +

For each result:

+ +
    +
  • If the result highlights a known sink, confirm that the result is reported by the relevant query, or + that the result is a false positive due to sanitization.
    • +
  • If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query, + and confirm that the result is either found, or is safe due to appropriate sanitization.
    • +
  • If the result represents a call to an external API which transfers taint, add the appropriate modeling, and + re-run the query to determine what new results have appeared due to this additional modeling.
    • +
+ +

Otherwise, the result is likely uninteresting. Custom versions of this query can extend the SafeExternalAPIMethod +class to exclude known safe external APIs from future analysis. + + + + +

In this first example, a request parameter is read from HttpServletRequest and then ultimately used in a call to the +HttpServletResponse.sendError external API: + + + +

This is an XSS sink. The XSS query should therefore be reviewed to confirm that this sink is appropriately modeled, +and if it is, to confirm that the query reports this particular result, or that the result is false positive due to +some existing sanitization.

+ +

In this second example, again a request parameter is read from HttpServletRequest.

+ + + +

If the query reported the call to StringBuilder.append on Line 7, this would suggest that this external API is +not currently modeled as a taint step in the taint tracking library. The next step would be to model this as taint step, then +re-run the query to determine what additional results might be found. In this example, it seems likely that the result of the +StringBuilder will be executed as an SQL query, potentially leading to an SQL injection vulnerability.

+ +

Note that both examples are correctly handled with the standard taint tracking library and XSS query.

+ + + + + diff --git a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql new file mode 100644 index 00000000000..837604601db --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql @@ -0,0 +1,21 @@ +/** + * @name Untrusted data passed to external API + * @description Data provided remotely is used in this external API. + * @id java/untrusted-data-to-external-api + * @kind path-problem + * @precision very-low + * @problem.severity error + * @tags security external/cwe/cwe-20 + */ + +import java +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.ExternalAPIs::ExternalAPIs +import DataFlow::PathGraph + +from UntrustedDataToExternalAPIConfig config, DataFlow::PathNode source, DataFlow::PathNode sink +where config.hasFlowPath(source, sink) +select sink, source, sink, + "Call to " + sink.getNode().(ExternalAPIDataNode).getMethodDescription() + + " with untrusted data from $@.", source, source.toString() diff --git a/java/ql/src/semmle/code/java/security/ExternalAPIs.qll b/java/ql/src/semmle/code/java/security/ExternalAPIs.qll new file mode 100644 index 00000000000..2039a3f3f14 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/ExternalAPIs.qll @@ -0,0 +1,146 @@ +/** + * Definitions for reasoning about untrusted data used in APIs defined outside the + * database. + */ + +import java +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking + +module ExternalAPIs { + /** + * A `Method` which is considered a "safe" external API from a security perspective. + */ + abstract class SafeExternalAPIMethod extends Method { } + + /** The default set of "safe" external APIs. */ + class DefaultSafeExternalAPIMethod extends SafeExternalAPIMethod { + DefaultSafeExternalAPIMethod() { + this instanceof EqualsMethod + or + getName().regexpMatch("size|length|compareTo|getClass|lastIndexOf") + or + this.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate") + or + getQualifiedName() = "Objects.equals" + or + getDeclaringType().getQualifiedName() = "java.lang.String" and getName() = "equals" + or + getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions") + or + getDeclaringType().getPackage().getName().matches("org.junit%") + or + getDeclaringType().hasQualifiedName("com.google.common.base", "Strings") and + getName() = "isNullOrEmpty" + or + getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "StringUtils") and + getName() = "isNotEmpty" + or + getDeclaringType().hasQualifiedName("java.lang", "Character") and + getName() = "isDigit" + or + getDeclaringType().hasQualifiedName("java.lang", "String") and + getName().regexpMatch("equalsIgnoreCase|regionMatches") + or + getDeclaringType().hasQualifiedName("java.lang", "Boolean") and + getName() = "parseBoolean" + or + getDeclaringType().hasQualifiedName("org.apache.commons.io", "IOUtils") and + getName() = "closeQuietly" + or + getDeclaringType().hasQualifiedName("org.springframework.util", "StringUtils") and + getName().regexpMatch("hasText|isEmpty") + } + } + + /** A node representing data being passed to an external API. */ + class ExternalAPIDataNode extends DataFlow::Node { + Call call; + int i; + + ExternalAPIDataNode() { + ( + // Argument to call to a method + this.asExpr() = call.getArgument(i) + or + // Qualifier to call to a method which returns non trivial value + this.asExpr() = call.getQualifier() and + i = -1 and + not call.getCallee().getReturnType() instanceof VoidType and + not call.getCallee().getReturnType() instanceof BooleanType + ) and + // Defined outside the source archive + not call.getCallee().fromSource() and + // Not a call to an method which is overridden in source + not exists(Method m | + m.getASourceOverriddenMethod() = call.getCallee().getSourceDeclaration() and + m.fromSource() + ) and + // Not already modelled as a taint step + not exists(DataFlow::Node next | TaintTracking::localTaintStep(this, next)) and + // Not a call to a known safe external API + not call.getCallee() instanceof SafeExternalAPIMethod + } + + /** Gets the called API `Method`. */ + Method getMethod() { result = call.getCallee() } + + /** Gets the index which is passed untrusted data (where -1 indicates the qualifier). */ + int getIndex() { result = i } + + /** Gets the description of the method being called. */ + string getMethodDescription() { + result = + getMethod().getDeclaringType().getPackage() + "." + getMethod().getQualifiedName() + } + } + + class UntrustedDataToExternalAPIConfig extends TaintTracking::Configuration { + UntrustedDataToExternalAPIConfig() { this = "UntrustedDataToExternalAPIConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof ExternalAPIDataNode } + } + + /** A node representing untrusted data being passed to an external API. */ + class UntrustedExternalAPIDataNode extends ExternalAPIDataNode { + UntrustedExternalAPIDataNode() { any(UntrustedDataToExternalAPIConfig c).hasFlow(_, this) } + + /** Gets a source of untrusted data which is passed to this external API data node. */ + DataFlow::Node getAnUntrustedSource() { + any(UntrustedDataToExternalAPIConfig c).hasFlow(result, this) + } + } + + private newtype TExternalAPI = + TExternalAPIParameter(Method m, int index) { + exists(UntrustedExternalAPIDataNode n | + m = n.getMethod() and + index = n.getIndex() + ) + } + + /** An external API which is used with untrusted data. */ + class ExternalAPIUsedWithUntrustedData extends TExternalAPI { + /** Gets a possibly untrusted use of this external API. */ + UntrustedExternalAPIDataNode getUntrustedDataNode() { + this = TExternalAPIParameter(result.getMethod(), result.getIndex()) + } + + /** Gets the number of untrusted sources used with this external API. */ + int getNumberOfUntrustedSources() { + result = count(getUntrustedDataNode().getAnUntrustedSource()) + } + + string toString() { + exists(Method m, int index, string indexString | + if index = -1 then indexString = "qualifier" else indexString = "param " + index + | + this = TExternalAPIParameter(m, index) and + result = + m.getDeclaringType().(RefType).getQualifiedName() + "." + m.getSignature() + " [" + indexString + "]" + ) + } + } +} From e0c081a2afedcbf1822380f8a8c42280daf5c7ab Mon Sep 17 00:00:00 2001 From: Luke Cartey <5377966+lcartey@users.noreply.github.com> Date: Tue, 4 Aug 2020 09:40:28 +0100 Subject: [PATCH 02/10] Add missing `

` tag Co-authored-by: Felicity Chapman --- .../CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp index ff8febbd397..4113c547fa0 100644 --- a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp @@ -28,7 +28,7 @@ used as the qualifier to the method call.

Otherwise, the result is likely uninteresting. Custom versions of this query can extend the SafeExternalAPIMethod -class to exclude known safe external APIs from future analysis. +class to exclude known safe external APIs from future analysis.

From 7928a024247bbd26c6ad888ab7856a9d49966444 Mon Sep 17 00:00:00 2001 From: Luke Cartey <5377966+lcartey@users.noreply.github.com> Date: Tue, 4 Aug 2020 09:40:51 +0100 Subject: [PATCH 03/10] Add missing full stop. Co-authored-by: Marcono1234 --- .../CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp index 4113c547fa0..a59de163b2a 100644 --- a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp @@ -6,7 +6,7 @@

Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports all external APIs which are used with untrusted data, along with how frequently the API is used, and how many unique sources of untrusted data flow this API. This query is designed primarily to help identify which APIs -may be relevant for security analysis of this application

+may be relevant for security analysis of this application.

An external API is defined as a method call to a method which is not defined in the source code, not overridden in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the From 368572f1f066b6237201210d17de95b9903a520e Mon Sep 17 00:00:00 2001 From: Luke Cartey <5377966+lcartey@users.noreply.github.com> Date: Tue, 4 Aug 2020 09:40:59 +0100 Subject: [PATCH 04/10] Update java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp Co-authored-by: Marcono1234 --- .../src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp index 855d0ca30e4..02f754cfd56 100644 --- a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp +++ b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp @@ -46,7 +46,7 @@ some existing sanitization.

-

If the query reported the call to StringBuilder.append on Line 7, this would suggest that this external API is +

If the query reported the call to StringBuilder.append on line 7, this would suggest that this external API is not currently modeled as a taint step in the taint tracking library. The next step would be to model this as taint step, then re-run the query to determine what additional results might be found. In this example, it seems likely that the result of the StringBuilder will be executed as an SQL query, potentially leading to an SQL injection vulnerability.

From 5a96ee1a7bf92f305055b4f3918dc6fb9d2bd5c4 Mon Sep 17 00:00:00 2001 From: Luke Cartey <5377966+lcartey@users.noreply.github.com> Date: Tue, 4 Aug 2020 09:41:40 +0100 Subject: [PATCH 05/10] Remove parameter names from signatures Co-authored-by: Marcono1234 --- .../CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp index a59de163b2a..4f04b90fa49 100644 --- a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp @@ -33,11 +33,11 @@ class to exclude known safe external APIs from future analysis.

-

If the query were to return the API javax.servlet.http.HttpServletResponse.sendError(int sc, java.lang.String msg) [param 1] +

If the query were to return the API javax.servlet.http.HttpServletResponse.sendError(int, java.lang.String) [param 1] then we should first consider whether this a security relevant sink. In this case, this is writing to a HTTP response, so we should consider whether this is an XSS sink. If it is, we should confirm that it is handled by the XSS query.

-

If the query were to return the API java.lang.StringBuilder.append(java.lang.String str) [param 0], then this should be +

If the query were to return the API java.lang.StringBuilder.append(java.lang.String) [param 0], then this should be reviewed as a possible taint step, because tainted data would flow from the 0th argument to the qualifier of the call.

Note that both examples are correctly handled with the standard taint tracking library and XSS query.

From 8a65dd2cd6551a8b396825960518faa752eb9d47 Mon Sep 17 00:00:00 2001 From: "lcartey@github.com" Date: Tue, 11 Aug 2020 15:28:06 +0100 Subject: [PATCH 06/10] Java: Address review comments --- .../CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp | 4 ++-- .../CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql | 4 ++-- .../Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp | 4 ++-- java/ql/src/semmle/code/java/security/ExternalAPIs.qll | 6 +++--- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp index 4f04b90fa49..ed0abf51cb5 100644 --- a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp @@ -4,11 +4,11 @@

Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports -all external APIs which are used with untrusted data, along with how frequently the API is used, and how many +all external APIs that are used with untrusted data, along with how frequently the API is used, and how many unique sources of untrusted data flow this API. This query is designed primarily to help identify which APIs may be relevant for security analysis of this application.

-

An external API is defined as a method call to a method which is not defined in the source code, not overridden +

An external API is defined as a method call to a method that is not defined in the source code, not overridden in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the Java standard library, third party dependencies or from internal dependencies. The query will report the method signature with a fully qualified name, along with either [param x], where x indicates the diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql index bbe627e0490..6bb7c6c6c51 100644 --- a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql @@ -1,6 +1,6 @@ /** - * @name Frequency counts for external APIs which are used with untrusted data - * @description This reports the external APIs which are used with untrusted data, along with how + * @name Frequency counts for external APIs that are used with untrusted data + * @description This reports the external APIs that are used with untrusted data, along with how * frequently the API is called, and how many unique sources of untrusted data flow * to it. * @id java/count-untrusted-data-external-api diff --git a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp index 02f754cfd56..3a89f3a7af0 100644 --- a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp +++ b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp @@ -8,7 +8,7 @@ all uses of external APIs with untrusted data for review. This query has a delib and is designed to help security reviews for the application, as well as helping identify external APIs that should be modeled as either taint steps, or sinks for specific problems.

-

An external API is defined as a method call to a method which is not defined in the source code, not overridden +

An external API is defined as a method call to a method that is not defined in the source code, not overridden in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the Java standard library, third party dependencies or from internal dependencies. The query will report uses of untrusted data in either the qualifier or as one of the arguments of external APIs.

@@ -28,7 +28,7 @@ untrusted data in either the qualifier or as one of the arguments of external AP

Otherwise, the result is likely uninteresting. Custom versions of this query can extend the SafeExternalAPIMethod -class to exclude known safe external APIs from future analysis. +class to exclude known safe external APIs from future analysis.

diff --git a/java/ql/src/semmle/code/java/security/ExternalAPIs.qll b/java/ql/src/semmle/code/java/security/ExternalAPIs.qll index 2039a3f3f14..ca81a4b0b8b 100644 --- a/java/ql/src/semmle/code/java/security/ExternalAPIs.qll +++ b/java/ql/src/semmle/code/java/security/ExternalAPIs.qll @@ -9,7 +9,7 @@ import semmle.code.java.dataflow.TaintTracking module ExternalAPIs { /** - * A `Method` which is considered a "safe" external API from a security perspective. + * A `Method` that is considered a "safe" external API from a security perspective. */ abstract class SafeExternalAPIMethod extends Method { } @@ -24,7 +24,7 @@ module ExternalAPIs { or getQualifiedName() = "Objects.equals" or - getDeclaringType().getQualifiedName() = "java.lang.String" and getName() = "equals" + getDeclaringType() instanceof TypeString and getName() = "equals" or getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions") or @@ -76,7 +76,7 @@ module ExternalAPIs { m.getASourceOverriddenMethod() = call.getCallee().getSourceDeclaration() and m.fromSource() ) and - // Not already modelled as a taint step + // Not already modeled as a taint step not exists(DataFlow::Node next | TaintTracking::localTaintStep(this, next)) and // Not a call to a known safe external API not call.getCallee() instanceof SafeExternalAPIMethod From e1d4b989239295de2850f8df9e70a12e2afee645 Mon Sep 17 00:00:00 2001 From: "lcartey@github.com" Date: Tue, 11 Aug 2020 15:28:55 +0100 Subject: [PATCH 07/10] Java: Add further missing

to qhelp --- .../src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp index 3a89f3a7af0..c606d2c8369 100644 --- a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp +++ b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp @@ -34,7 +34,7 @@ class to exclude known safe external APIs from future analysis.

In this first example, a request parameter is read from HttpServletRequest and then ultimately used in a call to the -HttpServletResponse.sendError external API: +HttpServletResponse.sendError external API:

From 6b6172fa5bd12d74f258c37b5fd7429d387fbf96 Mon Sep 17 00:00:00 2001 From: "lcartey@github.com" Date: Wed, 12 Aug 2020 09:21:14 +0100 Subject: [PATCH 08/10] Java: ExternalAPIs: Further review comments - Extra qldoc - Remove unnecessary module --- .../ExternalAPIsUsedWithUntrustedData.ql | 2 +- .../CWE/CWE-020/UntrustedDataToExternalAPI.ql | 2 +- .../code/java/security/ExternalAPIs.qll | 268 +++++++++--------- 3 files changed, 136 insertions(+), 136 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql index 6bb7c6c6c51..745e3ad9aa4 100644 --- a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql @@ -8,7 +8,7 @@ */ import java -import semmle.code.java.security.ExternalAPIs::ExternalAPIs +import semmle.code.java.security.ExternalAPIs import semmle.code.java.dataflow.DataFlow from ExternalAPIUsedWithUntrustedData externalAPI diff --git a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql index 837604601db..fd3382eca7c 100644 --- a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql +++ b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql @@ -11,7 +11,7 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.security.ExternalAPIs::ExternalAPIs +import semmle.code.java.security.ExternalAPIs import DataFlow::PathGraph from UntrustedDataToExternalAPIConfig config, DataFlow::PathNode source, DataFlow::PathNode sink diff --git a/java/ql/src/semmle/code/java/security/ExternalAPIs.qll b/java/ql/src/semmle/code/java/security/ExternalAPIs.qll index ca81a4b0b8b..57d085eba75 100644 --- a/java/ql/src/semmle/code/java/security/ExternalAPIs.qll +++ b/java/ql/src/semmle/code/java/security/ExternalAPIs.qll @@ -7,140 +7,140 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking -module ExternalAPIs { - /** - * A `Method` that is considered a "safe" external API from a security perspective. - */ - abstract class SafeExternalAPIMethod extends Method { } +/** + * A `Method` that is considered a "safe" external API from a security perspective. + */ +abstract class SafeExternalAPIMethod extends Method { } - /** The default set of "safe" external APIs. */ - class DefaultSafeExternalAPIMethod extends SafeExternalAPIMethod { - DefaultSafeExternalAPIMethod() { - this instanceof EqualsMethod - or - getName().regexpMatch("size|length|compareTo|getClass|lastIndexOf") - or - this.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate") - or - getQualifiedName() = "Objects.equals" - or - getDeclaringType() instanceof TypeString and getName() = "equals" - or - getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions") - or - getDeclaringType().getPackage().getName().matches("org.junit%") - or - getDeclaringType().hasQualifiedName("com.google.common.base", "Strings") and - getName() = "isNullOrEmpty" - or - getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "StringUtils") and - getName() = "isNotEmpty" - or - getDeclaringType().hasQualifiedName("java.lang", "Character") and - getName() = "isDigit" - or - getDeclaringType().hasQualifiedName("java.lang", "String") and - getName().regexpMatch("equalsIgnoreCase|regionMatches") - or - getDeclaringType().hasQualifiedName("java.lang", "Boolean") and - getName() = "parseBoolean" - or - getDeclaringType().hasQualifiedName("org.apache.commons.io", "IOUtils") and - getName() = "closeQuietly" - or - getDeclaringType().hasQualifiedName("org.springframework.util", "StringUtils") and - getName().regexpMatch("hasText|isEmpty") - } - } - - /** A node representing data being passed to an external API. */ - class ExternalAPIDataNode extends DataFlow::Node { - Call call; - int i; - - ExternalAPIDataNode() { - ( - // Argument to call to a method - this.asExpr() = call.getArgument(i) - or - // Qualifier to call to a method which returns non trivial value - this.asExpr() = call.getQualifier() and - i = -1 and - not call.getCallee().getReturnType() instanceof VoidType and - not call.getCallee().getReturnType() instanceof BooleanType - ) and - // Defined outside the source archive - not call.getCallee().fromSource() and - // Not a call to an method which is overridden in source - not exists(Method m | - m.getASourceOverriddenMethod() = call.getCallee().getSourceDeclaration() and - m.fromSource() - ) and - // Not already modeled as a taint step - not exists(DataFlow::Node next | TaintTracking::localTaintStep(this, next)) and - // Not a call to a known safe external API - not call.getCallee() instanceof SafeExternalAPIMethod - } - - /** Gets the called API `Method`. */ - Method getMethod() { result = call.getCallee() } - - /** Gets the index which is passed untrusted data (where -1 indicates the qualifier). */ - int getIndex() { result = i } - - /** Gets the description of the method being called. */ - string getMethodDescription() { - result = - getMethod().getDeclaringType().getPackage() + "." + getMethod().getQualifiedName() - } - } - - class UntrustedDataToExternalAPIConfig extends TaintTracking::Configuration { - UntrustedDataToExternalAPIConfig() { this = "UntrustedDataToExternalAPIConfig" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof ExternalAPIDataNode } - } - - /** A node representing untrusted data being passed to an external API. */ - class UntrustedExternalAPIDataNode extends ExternalAPIDataNode { - UntrustedExternalAPIDataNode() { any(UntrustedDataToExternalAPIConfig c).hasFlow(_, this) } - - /** Gets a source of untrusted data which is passed to this external API data node. */ - DataFlow::Node getAnUntrustedSource() { - any(UntrustedDataToExternalAPIConfig c).hasFlow(result, this) - } - } - - private newtype TExternalAPI = - TExternalAPIParameter(Method m, int index) { - exists(UntrustedExternalAPIDataNode n | - m = n.getMethod() and - index = n.getIndex() - ) - } - - /** An external API which is used with untrusted data. */ - class ExternalAPIUsedWithUntrustedData extends TExternalAPI { - /** Gets a possibly untrusted use of this external API. */ - UntrustedExternalAPIDataNode getUntrustedDataNode() { - this = TExternalAPIParameter(result.getMethod(), result.getIndex()) - } - - /** Gets the number of untrusted sources used with this external API. */ - int getNumberOfUntrustedSources() { - result = count(getUntrustedDataNode().getAnUntrustedSource()) - } - - string toString() { - exists(Method m, int index, string indexString | - if index = -1 then indexString = "qualifier" else indexString = "param " + index - | - this = TExternalAPIParameter(m, index) and - result = - m.getDeclaringType().(RefType).getQualifiedName() + "." + m.getSignature() + " [" + indexString + "]" - ) - } +/** The default set of "safe" external APIs. */ +private class DefaultSafeExternalAPIMethod extends SafeExternalAPIMethod { + DefaultSafeExternalAPIMethod() { + this instanceof EqualsMethod + or + getName().regexpMatch("size|length|compareTo|getClass|lastIndexOf") + or + this.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate") + or + getQualifiedName() = "Objects.equals" + or + getDeclaringType() instanceof TypeString and getName() = "equals" + or + getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions") + or + getDeclaringType().getPackage().getName().matches("org.junit%") + or + getDeclaringType().hasQualifiedName("com.google.common.base", "Strings") and + getName() = "isNullOrEmpty" + or + getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "StringUtils") and + getName() = "isNotEmpty" + or + getDeclaringType().hasQualifiedName("java.lang", "Character") and + getName() = "isDigit" + or + getDeclaringType().hasQualifiedName("java.lang", "String") and + getName().regexpMatch("equalsIgnoreCase|regionMatches") + or + getDeclaringType().hasQualifiedName("java.lang", "Boolean") and + getName() = "parseBoolean" + or + getDeclaringType().hasQualifiedName("org.apache.commons.io", "IOUtils") and + getName() = "closeQuietly" + or + getDeclaringType().hasQualifiedName("org.springframework.util", "StringUtils") and + getName().regexpMatch("hasText|isEmpty") + } +} + +/** A node representing data being passed to an external API. */ +class ExternalAPIDataNode extends DataFlow::Node { + Call call; + int i; + + ExternalAPIDataNode() { + ( + // Argument to call to a method + this.asExpr() = call.getArgument(i) + or + // Qualifier to call to a method which returns non trivial value + this.asExpr() = call.getQualifier() and + i = -1 and + not call.getCallee().getReturnType() instanceof VoidType and + not call.getCallee().getReturnType() instanceof BooleanType + ) and + // Defined outside the source archive + not call.getCallee().fromSource() and + // Not a call to an method which is overridden in source + not exists(Method m | + m.getASourceOverriddenMethod() = call.getCallee().getSourceDeclaration() and + m.fromSource() + ) and + // Not already modeled as a taint step + not exists(DataFlow::Node next | TaintTracking::localTaintStep(this, next)) and + // Not a call to a known safe external API + not call.getCallee() instanceof SafeExternalAPIMethod + } + + /** Gets the called API `Method`. */ + Method getMethod() { result = call.getCallee() } + + /** Gets the index which is passed untrusted data (where -1 indicates the qualifier). */ + int getIndex() { result = i } + + /** Gets the description of the method being called. */ + string getMethodDescription() { + result = getMethod().getDeclaringType().getPackage() + "." + getMethod().getQualifiedName() + } +} + +/** A configuration for tracking flow from `RemoteFlowSource`s to `ExternalAPIDataNode`s. */ +class UntrustedDataToExternalAPIConfig extends TaintTracking::Configuration { + UntrustedDataToExternalAPIConfig() { this = "UntrustedDataToExternalAPIConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof ExternalAPIDataNode } +} + +/** A node representing untrusted data being passed to an external API. */ +class UntrustedExternalAPIDataNode extends ExternalAPIDataNode { + UntrustedExternalAPIDataNode() { any(UntrustedDataToExternalAPIConfig c).hasFlow(_, this) } + + /** Gets a source of untrusted data which is passed to this external API data node. */ + DataFlow::Node getAnUntrustedSource() { + any(UntrustedDataToExternalAPIConfig c).hasFlow(result, this) + } +} + +private newtype TExternalAPI = + TExternalAPIParameter(Method m, int index) { + exists(UntrustedExternalAPIDataNode n | + m = n.getMethod() and + index = n.getIndex() + ) + } + +/** An external API which is used with untrusted data. */ +class ExternalAPIUsedWithUntrustedData extends TExternalAPI { + /** Gets a possibly untrusted use of this external API. */ + UntrustedExternalAPIDataNode getUntrustedDataNode() { + this = TExternalAPIParameter(result.getMethod(), result.getIndex()) + } + + /** Gets the number of untrusted sources used with this external API. */ + int getNumberOfUntrustedSources() { + result = count(getUntrustedDataNode().getAnUntrustedSource()) + } + + /** Gets a textual representation of this element. */ + string toString() { + exists(Method m, int index, string indexString | + if index = -1 then indexString = "qualifier" else indexString = "param " + index + | + this = TExternalAPIParameter(m, index) and + result = + m.getDeclaringType().(RefType).getQualifiedName() + "." + m.getSignature() + " [" + + indexString + "]" + ) } } From 56ff8cf0844ad235aacccd66f092db2b88f05434 Mon Sep 17 00:00:00 2001 From: Luke Cartey <5377966+lcartey@users.noreply.github.com> Date: Wed, 12 Aug 2020 13:12:06 +0100 Subject: [PATCH 09/10] Apply suggestions from code review Co-authored-by: Felicity Chapman --- .../ExternalAPIsUsedWithUntrustedData.qhelp | 4 ++-- .../CWE-020/ExternalAPIsUsedWithUntrustedData.ql | 1 + .../CWE/CWE-020/UntrustedDataToExternalAPI.qhelp | 16 +++++++--------- .../CWE/CWE-020/UntrustedDataToExternalAPI.ql | 2 +- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp index ed0abf51cb5..8b6babc5c62 100644 --- a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp @@ -5,7 +5,7 @@

Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports all external APIs that are used with untrusted data, along with how frequently the API is used, and how many -unique sources of untrusted data flow this API. This query is designed primarily to help identify which APIs +unique sources of untrusted data flow to this API. This query is designed primarily to help identify which APIs may be relevant for security analysis of this application.

An external API is defined as a method call to a method that is not defined in the source code, not overridden @@ -40,7 +40,7 @@ consider whether this is an XSS sink. If it is, we should confirm that it is han

If the query were to return the API java.lang.StringBuilder.append(java.lang.String) [param 0], then this should be reviewed as a possible taint step, because tainted data would flow from the 0th argument to the qualifier of the call.

-

Note that both examples are correctly handled with the standard taint tracking library and XSS query.

+

Note that both examples are correctly handled by the standard taint tracking library and XSS query.

 diff --git a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql index 745e3ad9aa4..f9643429d72 100644 --- a/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql +++ b/java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql @@ -5,6 +5,7 @@ * to it. * @id java/count-untrusted-data-external-api * @kind table + * @tags security external/cwe/cwe-20 */ import java diff --git a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp index c606d2c8369..c053494d332 100644 --- a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp +++ b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp @@ -4,13 +4,11 @@

Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports -all uses of external APIs with untrusted data for review. This query has a deliberately low true positive rate, -and is designed to help security reviews for the application, as well as helping identify external APIs that -should be modeled as either taint steps, or sinks for specific problems.

+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.

An external API is defined as a method call to a method that is not defined in the source code, not overridden in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the -Java standard library, third party dependencies or from internal dependencies. The query will report uses of +Java standard library, third-party dependencies or from internal dependencies. The query reports uses of untrusted data in either the qualifier or as one of the arguments of external APIs.

 @@ -20,10 +18,10 @@ untrusted data in either the qualifier or as one of the arguments of external AP
  • If the result highlights a known sink, confirm that the result is reported by the relevant query, or - that the result is a false positive due to sanitization.
    • + that the result is a false positive because this data is sanitized.
  • If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query, and confirm that the result is either found, or is safe due to appropriate sanitization.
    • -
  • If the result represents a call to an external API which transfers taint, add the appropriate modeling, and +
  • If the result represents a call to an external API that transfers taint, add the appropriate modeling, and re-run the query to determine what new results have appeared due to this additional modeling.
@@ -39,7 +37,7 @@ class to exclude known safe external APIs from future analysis.

This is an XSS sink. The XSS query should therefore be reviewed to confirm that this sink is appropriately modeled, -and if it is, to confirm that the query reports this particular result, or that the result is false positive due to +and if it is, to confirm that the query reports this particular result, or that the result is a false positive due to some existing sanitization.

In this second example, again a request parameter is read from HttpServletRequest.

@@ -47,11 +45,11 @@ some existing sanitization.

If the query reported the call to StringBuilder.append on line 7, this would suggest that this external API is -not currently modeled as a taint step in the taint tracking library. The next step would be to model this as taint step, then +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 re-run the query to determine what additional results might be found. In this example, it seems likely that the result of the StringBuilder will be executed as an SQL query, potentially leading to an SQL injection vulnerability.

-

Note that both examples are correctly handled with the standard taint tracking library and XSS query.

+

Note that both examples are correctly handled by the standard taint tracking library and XSS query.

 diff --git a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql index fd3382eca7c..41663f1ba46 100644 --- a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql +++ b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql @@ -1,6 +1,6 @@ /** * @name Untrusted data passed to external API - * @description Data provided remotely is used in this external API. + * @description Data provided remotely is used in this external API without sanitization, which could be a security risk. * @id java/untrusted-data-to-external-api * @kind path-problem * @precision very-low From 6f83c55ebde327f2444921a65915378ffd5290d5 Mon Sep 17 00:00:00 2001 From: "lcartey@github.com" Date: Wed, 12 Aug 2020 13:48:59 +0100 Subject: [PATCH 10/10] Java: Switch to `low` as a precision Code Scanning doesn't support "very-low" --- java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql index 41663f1ba46..35c6a69c022 100644 --- a/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql +++ b/java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql @@ -3,7 +3,7 @@ * @description Data provided remotely is used in this external API without sanitization, which could be a security risk. * @id java/untrusted-data-to-external-api * @kind path-problem - * @precision very-low + * @precision low * @problem.severity error * @tags security external/cwe/cwe-20 */