diff --git a/ql/src/Security/CWE-020/ExternalAPISinkExample.go b/ql/src/Security/CWE-020/ExternalAPISinkExample.go new file mode 100644 index 00000000000..273da84901c --- /dev/null +++ b/ql/src/Security/CWE-020/ExternalAPISinkExample.go @@ -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/ql/src/Security/CWE-020/ExternalAPITaintStepExample.go b/ql/src/Security/CWE-020/ExternalAPITaintStepExample.go new file mode 100644 index 00000000000..65735194a69 --- /dev/null +++ b/ql/src/Security/CWE-020/ExternalAPITaintStepExample.go @@ -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/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp b/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp new file mode 100644 index 00000000000..73bedfef1c4 --- /dev/null +++ b/ql/src/Security/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 that are used with untrusted data, along with how frequently the API is used, and how many +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 +in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the +Go 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 [receiver] indicating the untrusted data is +used as the receiver of the method call.

+ + + + +

For each result:

+ + + +

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, 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) [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 by the standard taint tracking library and XSS query.

+ + + + + diff --git a/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.ql b/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.ql new file mode 100644 index 00000000000..cd8592e8fc2 --- /dev/null +++ b/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.ql @@ -0,0 +1,17 @@ +/** + * @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 go/count-untrusted-data-external-api + * @kind table + * @tags security external/cwe/cwe-20 + */ + +import go +import semmle.go.security.ExternalAPIs + +from ExternalAPIUsedWithUntrustedData externalAPI +select externalAPI, count(externalAPI.getUntrustedDataNode()) as numberOfUses, + externalAPI.getNumberOfUntrustedSources() as numberOfUntrustedSources order by + numberOfUntrustedSources desc diff --git a/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelp b/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelp new file mode 100644 index 00000000000..b73f6952946 --- /dev/null +++ b/ql/src/Security/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 +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 call to a function 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 +Go 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.

+ + + + +

For each result:

+ + + +

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 a 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 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 by the standard taint tracking library and XSS query.

+ + + + + diff --git a/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.ql b/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.ql new file mode 100644 index 00000000000..c0341893fcb --- /dev/null +++ b/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.ql @@ -0,0 +1,19 @@ +/** + * @name Untrusted data passed to external API + * @description Data provided remotely is used in this external API without sanitization, which could be a security risk. + * @id go/untrusted-data-to-external-api + * @kind path-problem + * @precision low + * @problem.severity error + * @tags security external/cwe/cwe-20 + */ + +import go +import semmle.go.security.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).getFunctionDescription() + + " with untrusted data from $@.", source, source.toString() diff --git a/ql/src/semmle/go/security/ExternalAPIs.qll b/ql/src/semmle/go/security/ExternalAPIs.qll new file mode 100644 index 00000000000..b5fc42fe505 --- /dev/null +++ b/ql/src/semmle/go/security/ExternalAPIs.qll @@ -0,0 +1,109 @@ +/** + * Definitions for reasoning about untrusted data used in APIs defined outside the + * database. + */ + +import go + +/** + * A `Function` that is considered a "safe" external API from a security perspective. + */ +abstract class SafeExternalAPIFunction extends Function { } + +/** The default set of "safe" external APIs. */ +private class DefaultSafeExternalAPIFunction extends SafeExternalAPIFunction { + DefaultSafeExternalAPIFunction() { + this instanceof BuiltinFunction + // TODO: Add more external API functions which we know are safe here + } +} + +/** A node representing data being passed to an external API. */ +class ExternalAPIDataNode extends DataFlow::Node { + DataFlow::CallNode call; + int i; + + ExternalAPIDataNode() { + ( + // Argument to call to a function + this = call.getArgument(i) + or + // Receiver to a call to a method which returns non trivial value + this = call.getReceiver() and + i = -1 and + ( + call.getTarget().getNumResult() >= 2 + or + call.getTarget().getNumResult() = 1 and + not call.getTarget().getResultType(0) instanceof BoolType + ) + ) and + // Not defined in the code that is being analysed + not exists(call.getACallee().getBody()) 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 = any(SafeExternalAPIFunction f).getACall() + } + + /** Gets the called API `Function`. */ + Function getFunction() { result.getACall() = call } + + /** Gets the index which is passed untrusted data (where -1 indicates the receiver). */ + int getIndex() { result = i } + + /** Gets the description of the function being called. */ + string getFunctionDescription() { result = call.getCalleeName() } +} + +/** 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 UntrustedFlowSource } + + 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(Function m, int index) { + exists(UntrustedExternalAPIDataNode n | + m = n.getFunction() 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.getFunction(), 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(Function f, int index, string indexString | + if index = -1 then indexString = "receiver" else indexString = "param " + index + | + this = TExternalAPIParameter(f, index) and + if exists(f.getQualifiedName()) + then result = f.getQualifiedName() + " [" + indexString + "]" + else result = f.getName() + " [" + indexString + "]" + ) + } +}