From 0ee00d86472292ba2e87764bc080c74d714b2f04 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Thu, 26 Nov 2020 16:33:05 +0000 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Chris Smowton --- ql/src/Security/CWE-020/ExternalAPISinkExample.go | 2 +- .../CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp | 10 +++++++--- .../Security/CWE-020/UntrustedDataToExternalAPI.qhelp | 6 +++--- .../CWE-020/UntrustedDataToUnknownExternalAPI.qhelp | 6 +++--- ql/src/Security/CWE-079/ReflectedXss.go | 2 +- ql/src/semmle/go/security/ExternalAPIs.qll | 5 +++-- 6 files changed, 18 insertions(+), 13 deletions(-) diff --git a/ql/src/Security/CWE-020/ExternalAPISinkExample.go b/ql/src/Security/CWE-020/ExternalAPISinkExample.go index 002732488e1..43e5e022598 100644 --- a/ql/src/Security/CWE-020/ExternalAPISinkExample.go +++ b/ql/src/Security/CWE-020/ExternalAPISinkExample.go @@ -13,7 +13,7 @@ func serve() { // BAD: a request parameter is incorporated without validation into the response fmt.Fprintf(w, "%q is an unknown user", username) } else { - // TODO: do something exciting + // TODO: Handle successful login } }) http.ListenAndServe(":80", nil) diff --git a/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp b/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp index e39347034b2..7f23586d701 100644 --- a/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp +++ b/ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp @@ -9,9 +9,9 @@ unique sources of untrusted data flow to this API. This query is designed primar may be relevant for security analysis of this application.

An external API is defined as a call to a function that is not defined in the source code and is not -modeled as a taint step in the default taint library. Calls made in test files are also excluded. +modeled as a taint step in the default taint library. Calls made in test files are excluded. External APIs may be from the Go standard library, third party dependencies or from internal dependencies. -The query will report the fully qualified method name, along with either [param x], +The query will report the fully-qualified method 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.

@@ -23,7 +23,7 @@ indicating the untrusted data is used as the receiver of the method call.

@@ -33,10 +33,14 @@ class to exclude known safe external APIs from future analysis.

+ +

If the query were to return the API fmt.Fprintf [param 2] 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 fmt.Sprintf [param 1], then this should be reviewed as a possible taint step, because tainted data would flow from the 1st argument to the return value of the call.

diff --git a/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelp b/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelp index 66dd456777e..eb409f5c886 100644 --- a/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelp +++ b/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelp @@ -5,11 +5,11 @@

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 have very little filtering so that you can audit almost all -examples. The query provides data for security reviews of the application and you can also use it to identify +examples. The query provides data for security reviews of the application. It can also be used 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 and is not -modeled as a taint step in the default taint library. Calls made in test files are also excluded. +modeled as a taint step in the default taint library. Calls made in test files are excluded. 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 receiver or as one of the arguments of external APIs.

@@ -23,7 +23,7 @@ The query reports uses of untrusted data in either the receiver or as one of the 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 that transfers taint, add the appropriate modeling, and +
  • If the result represents a call to an external API that transfers taint (for example, from a parameter to its return value), add the appropriate modeling, and re-run the query to determine what new results have appeared due to this additional modeling.
    • diff --git a/ql/src/Security/CWE-020/UntrustedDataToUnknownExternalAPI.qhelp b/ql/src/Security/CWE-020/UntrustedDataToUnknownExternalAPI.qhelp index 3dc5403f255..38d908abd91 100644 --- a/ql/src/Security/CWE-020/UntrustedDataToUnknownExternalAPI.qhelp +++ b/ql/src/Security/CWE-020/UntrustedDataToUnknownExternalAPI.qhelp @@ -5,11 +5,11 @@

    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 have been filtered. 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 +reviews of the application. It can also be used 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 and is not -modeled as a taint step in the default taint library. Calls made in test files are also excluded. +modeled as a taint step in the default taint library. Calls made in test files are excluded. 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 receiver or as one of the arguments of external APIs.

    @@ -24,7 +24,7 @@ to not be a possible source of security vulnerabilities.

    • 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 that transfers taint, add the appropriate modeling, and +
    • If the result represents a call to an external API that transfers taint (for example, from a parameter to its return value), add the appropriate modeling, and re-run the query to determine what new results have appeared due to this additional modeling.
    diff --git a/ql/src/Security/CWE-079/ReflectedXss.go b/ql/src/Security/CWE-079/ReflectedXss.go index 002732488e1..43e5e022598 100644 --- a/ql/src/Security/CWE-079/ReflectedXss.go +++ b/ql/src/Security/CWE-079/ReflectedXss.go @@ -13,7 +13,7 @@ func serve() { // BAD: a request parameter is incorporated without validation into the response fmt.Fprintf(w, "%q is an unknown user", username) } else { - // TODO: do something exciting + // TODO: Handle successful login } }) http.ListenAndServe(":80", nil) diff --git a/ql/src/semmle/go/security/ExternalAPIs.qll b/ql/src/semmle/go/security/ExternalAPIs.qll index 25e43c593f9..31a9eba3925 100644 --- a/ql/src/semmle/go/security/ExternalAPIs.qll +++ b/ql/src/semmle/go/security/ExternalAPIs.qll @@ -74,7 +74,7 @@ class ExternalAPIDataNode extends DataFlow::Node { ) and // Not defined in the code that is being analysed not exists(call.getACallee().getBody()) and - // Not a function pointer, unless it's declared in a package + // Not a function pointer, unless it's declared at package scope not isProbableLocalFunctionPointer(call) and // Not defined in a test file not call.getFile() instanceof TestFile and @@ -113,6 +113,7 @@ class ExternalAPIDataNode extends DataFlow::Node { TaintTracking::FunctionModel getAMethodModelInPackage(Package p) { p = result.getPackage() and result instanceof Method and + // We model any method of the form "String() string" result.getName() != "String" and not exists(TaintTracking::FunctionModel baseMethod | baseMethod != result and result.(Method).implements(baseMethod) @@ -144,7 +145,7 @@ predicate isACommonSink(DataFlow::Node n) { n instanceof CleartextLogging::Sink } -/** A node representing data being passed to an external API. */ +/** A node representing data being passed to an unknown external API. */ class UnknownExternalAPIDataNode extends ExternalAPIDataNode { UnknownExternalAPIDataNode() { // Not a sink for a commonly-used query