Merge pull request #20762 from joefarebrother/go-insecure-cookie

Go: Promote non-httponly cookie query, and add insecure cookie query

Author: joefarebrother

go/ql/integration-tests/query-suite/go-code-scanning.qls.expected

@@ -12,6 +12,7 @@ ql/go/ql/src/Security/CWE-079/HtmlTemplateEscapingBypassXss.ql
 ql/go/ql/src/Security/CWE-079/ReflectedXss.ql
 ql/go/ql/src/Security/CWE-089/SqlInjection.ql
 ql/go/ql/src/Security/CWE-089/StringBreak.ql
+ql/go/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.ql
 ql/go/ql/src/Security/CWE-190/AllocationSizeOverflow.ql
 ql/go/ql/src/Security/CWE-209/StackTraceExposure.ql
 ql/go/ql/src/Security/CWE-295/DisabledCertificateCheck.ql
@@ -26,6 +27,7 @@ ql/go/ql/src/Security/CWE-347/MissingJwtSignatureCheck.ql
 ql/go/ql/src/Security/CWE-352/ConstantOauth2State.ql
 ql/go/ql/src/Security/CWE-601/BadRedirectCheck.ql
 ql/go/ql/src/Security/CWE-601/OpenUrlRedirect.ql
+ql/go/ql/src/Security/CWE-614/CookieWithoutSecure.ql
 ql/go/ql/src/Security/CWE-640/EmailInjection.ql
 ql/go/ql/src/Security/CWE-643/XPathInjection.ql
 ql/go/ql/src/Security/CWE-681/IncorrectIntegerConversionQuery.ql

go/ql/integration-tests/query-suite/go-security-and-quality.qls.expected

@@ -34,6 +34,7 @@ ql/go/ql/src/Security/CWE-079/HtmlTemplateEscapingBypassXss.ql
 ql/go/ql/src/Security/CWE-079/ReflectedXss.ql
 ql/go/ql/src/Security/CWE-089/SqlInjection.ql
 ql/go/ql/src/Security/CWE-089/StringBreak.ql
+ql/go/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.ql
 ql/go/ql/src/Security/CWE-117/LogInjection.ql
 ql/go/ql/src/Security/CWE-190/AllocationSizeOverflow.ql
 ql/go/ql/src/Security/CWE-209/StackTraceExposure.ql
@@ -49,6 +50,7 @@ ql/go/ql/src/Security/CWE-347/MissingJwtSignatureCheck.ql
 ql/go/ql/src/Security/CWE-352/ConstantOauth2State.ql
 ql/go/ql/src/Security/CWE-601/BadRedirectCheck.ql
 ql/go/ql/src/Security/CWE-601/OpenUrlRedirect.ql
+ql/go/ql/src/Security/CWE-614/CookieWithoutSecure.ql
 ql/go/ql/src/Security/CWE-640/EmailInjection.ql
 ql/go/ql/src/Security/CWE-643/XPathInjection.ql
 ql/go/ql/src/Security/CWE-681/IncorrectIntegerConversionQuery.ql

go/ql/integration-tests/query-suite/go-security-extended.qls.expected

@@ -12,6 +12,7 @@ ql/go/ql/src/Security/CWE-079/HtmlTemplateEscapingBypassXss.ql
 ql/go/ql/src/Security/CWE-079/ReflectedXss.ql
 ql/go/ql/src/Security/CWE-089/SqlInjection.ql
 ql/go/ql/src/Security/CWE-089/StringBreak.ql
+ql/go/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.ql
 ql/go/ql/src/Security/CWE-117/LogInjection.ql
 ql/go/ql/src/Security/CWE-190/AllocationSizeOverflow.ql
 ql/go/ql/src/Security/CWE-209/StackTraceExposure.ql
@@ -27,6 +28,7 @@ ql/go/ql/src/Security/CWE-347/MissingJwtSignatureCheck.ql
 ql/go/ql/src/Security/CWE-352/ConstantOauth2State.ql
 ql/go/ql/src/Security/CWE-601/BadRedirectCheck.ql
 ql/go/ql/src/Security/CWE-601/OpenUrlRedirect.ql
+ql/go/ql/src/Security/CWE-614/CookieWithoutSecure.ql
 ql/go/ql/src/Security/CWE-640/EmailInjection.ql
 ql/go/ql/src/Security/CWE-643/XPathInjection.ql
 ql/go/ql/src/Security/CWE-681/IncorrectIntegerConversionQuery.ql

go/ql/integration-tests/query-suite/not_included_in_qls.expected

@@ -9,7 +9,6 @@ ql/go/ql/src/Security/CWE-079/StoredXss.ql
 ql/go/ql/src/Security/CWE-798/HardcodedCredentials.ql
 ql/go/ql/src/definitions.ql
 ql/go/ql/src/experimental/CWE-090/LDAPInjection.ql
-ql/go/ql/src/experimental/CWE-1004/CookieWithoutHttpOnly.ql
 ql/go/ql/src/experimental/CWE-203/Timing.ql
 ql/go/ql/src/experimental/CWE-285/PamAuthBypass.ql
 ql/go/ql/src/experimental/CWE-287/ImproperLdapAuth.ql

go/ql/lib/go.qll

@@ -41,6 +41,7 @@ import semmle.go.frameworks.ElazarlGoproxy
 import semmle.go.frameworks.Email
 import semmle.go.frameworks.Encoding
 import semmle.go.frameworks.Fasthttp
+import semmle.go.frameworks.Gin
 import semmle.go.frameworks.GinCors
 import semmle.go.frameworks.Glog
 import semmle.go.frameworks.GoJose

go/ql/lib/semmle/go/concepts/HTTP.qll

@@ -380,4 +380,96 @@ module Http {
     /** Gets a node that is used in a check that is tested before this handler is run. */
     predicate guardedBy(DataFlow::Node check) { super.guardedBy(check) }
   }
+
+  /** Provides a class for modeling new HTTP response cookie write APIs. */
+  module CookieWrite {
+    /**
+     * A write of an HTTP Cookie to an HTTP response.
+     *
+     * Extend this class to model new APIs. If you want to refine existing API models,
+     * extend `HTTP::CookieWrite` instead.
+     */
+    abstract class Range extends DataFlow::Node {
+      /** Gets the name of the cookie written. */
+      abstract DataFlow::Node getName();
+
+      /** Gets the value of the cookie written. */
+      abstract DataFlow::Node getValue();
+
+      /** Gets the `Secure` attribute of the cookie written. */
+      abstract DataFlow::Node getSecure();
+
+      /** Gets the `HttpOnly` attribute of the cookie written. */
+      abstract DataFlow::Node getHttpOnly();
+    }
+  }
+
+  /**
+   * A write of an HTTP Cookie to an HTTP response.
+   *
+   * Extend this class to refine existing API models. If you want to model new APIs,
+   * extend `HTTP::CookieWrite::Range` instead.
+   */
+  class CookieWrite extends DataFlow::Node instanceof CookieWrite::Range {
+    /** Gets the name of the cookie written. */
+    DataFlow::Node getName() { result = super.getName() }
+
+    /** Gets the value of the cookie written. */
+    DataFlow::Node getValue() { result = super.getValue() }
+
+    /** Gets the `Secure` attribute of the cookie written. */
+    DataFlow::Node getSecure() { result = super.getSecure() }
+
+    /** Gets the `HttpOnly` attribute of the cookie written. */
+    DataFlow::Node getHttpOnly() { result = super.getHttpOnly() }
+  }
+
+  /** Provides a class for modeling the new APIs for writes to options of an HTTP cookie. */
+  module CookieOptionWrite {
+    /**
+     * A write to an option of an HTTP cookie object.
+     *
+     * Extend this class to model new APIs. If you want to refine existing API models,
+     * extend `HTTP::CookieOptionWrite` instead.
+     */
+    abstract class Range extends DataFlow::Node {
+      /** Gets the node representing the cookie object for the options being set. */
+      abstract DataFlow::Node getCookieOutput();
+
+      /** Gets the name of the cookie represented, if any. */
+      abstract DataFlow::Node getName();
+
+      /** Gets the value of the cookie represented, if any. */
+      abstract DataFlow::Node getValue();
+
+      /** Gets the `Secure` attribute of the cookie represented, if any. */
+      abstract DataFlow::Node getSecure();
+
+      /** Gets the `HttpOnly` attribute of the cookie represented, if any. */
+      abstract DataFlow::Node getHttpOnly();
+    }
+  }
+
+  /**
+   * A write to an option of an HTTP cookie object.
+   *
+   * Extend this class to refine existing API models. If you want to model new APIs,
+   * extend `HTTP::CookieOptionWrite::Range` instead.
+   */
+  class CookieOptionWrite extends DataFlow::Node instanceof CookieOptionWrite::Range {
+    /** Gets the node representing the cookie object for the options being set. */
+    DataFlow::Node getCookieOutput() { result = super.getCookieOutput() }
+
+    /** Gets the name of the cookie represented, if any. */
+    DataFlow::Node getName() { result = super.getName() }
+
+    /** Gets the value of the cookie represented, if any. */
+    DataFlow::Node getValue() { result = super.getValue() }
+
+    /** Gets the `Secure` attribute of the cookie represented, if any. */
+    DataFlow::Node getSecure() { result = super.getSecure() }
+
+    /** Gets the `HttpOnly` attribute of the cookie represented, if any. */
+    DataFlow::Node getHttpOnly() { result = super.getHttpOnly() }
+  }
 }

go/ql/lib/semmle/go/frameworks/Gin.qll

@@ -0,0 +1,24 @@
+/**
+ * Provides classes for modeling the `github.com/gin-gonic/gin` package.
+ */
+
+import go
+import semmle.go.concepts.HTTP
+
+/** Provides models for the `gin-gonic/gin` package. */
+module Gin {
+  /** Gets the package name `github.com/gin-gonic/gin`. */
+  string packagePath() { result = package("github.com/gin-gonic/gin", "") }
+
+  private class GinCookieWrite extends Http::CookieWrite::Range, DataFlow::MethodCallNode {
+    GinCookieWrite() { this.getTarget().hasQualifiedName(packagePath(), "Context", "SetCookie") }
+
+    override DataFlow::Node getName() { result = this.getArgument(0) }
+
+    override DataFlow::Node getValue() { result = this.getArgument(1) }
+
+    override DataFlow::Node getSecure() { result = this.getArgument(5) }
+
+    override DataFlow::Node getHttpOnly() { result = this.getArgument(6) }
+  }
+}

go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll

@@ -293,4 +293,38 @@ module NetHttp {
 
     override DataFlow::Node getAPathArgument() { result = this.getArgument(2) }
   }
+
+  private class CookieWrite extends Http::CookieWrite::Range, DataFlow::CallNode {
+    CookieWrite() { this.getTarget().hasQualifiedName(package("net/http", ""), "SetCookie") }
+
+    override DataFlow::Node getName() { result = this.getArgument(1) }
+
+    override DataFlow::Node getValue() { result = this.getArgument(1) }
+
+    override DataFlow::Node getSecure() { result = this.getArgument(1) }
+
+    override DataFlow::Node getHttpOnly() { result = this.getArgument(1) }
+  }
+
+  private class CookieFieldWrite extends Http::CookieOptionWrite::Range {
+    DataFlow::Node written;
+    string fieldName;
+
+    CookieFieldWrite() {
+      exists(Write w, Field f |
+        f.hasQualifiedName(package("net/http", ""), "Cookie", fieldName) and
+        w.writesField(this, f, written)
+      )
+    }
+
+    override DataFlow::Node getCookieOutput() { result = this }
+
+    override DataFlow::Node getName() { fieldName = "Name" and result = written }
+
+    override DataFlow::Node getValue() { fieldName = "Value" and result = written }
+
+    override DataFlow::Node getSecure() { fieldName = "Secure" and result = written }
+
+    override DataFlow::Node getHttpOnly() { fieldName = "HttpOnly" and result = written }
+  }
 }

go/ql/lib/semmle/go/security/CookieWithoutHttpOnly.qll

@@ -0,0 +1,77 @@
+/** Provides classes and predicates for identifying HTTP cookies without the `HttpOnly` attribute. */
+
+import go
+import semmle.go.concepts.HTTP
+import semmle.go.dataflow.DataFlow
+
+private module SensitiveCookieNameConfig implements DataFlow::ConfigSig {
+  /**
+   * Holds if `source` is an expression with a name or literal value `val` indicating a sensitive cookie.
+   */
+  additional predicate isSource(DataFlow::Node source, string val) {
+    (
+      val = source.asExpr().getStringValue() or
+      val = source.asExpr().(Name).getTarget().getName()
+    ) and
+    val.regexpMatch("(?i).*(session|login|token|user|auth|credential).*") and
+    not val.regexpMatch("(?i).*(xsrf|csrf|forgery).*")
+  }
+
+  predicate isSource(DataFlow::Node source) { isSource(source, _) }
+
+  additional predicate isSink(DataFlow::Node sink, Http::CookieWrite cw) { sink = cw.getName() }
+
+  predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(Http::CookieOptionWrite co | co.getName() = pred and co.getCookieOutput() = succ)
+  }
+}
+
+/** Tracks flow from sensitive names to HTTP cookie writes. */
+module SensitiveCookieNameFlow = TaintTracking::Global<SensitiveCookieNameConfig>;
+
+private module BooleanCookieHttpOnlyConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source.getType().getUnderlyingType() instanceof BoolType
+  }
+
+  predicate isSink(DataFlow::Node sink) { exists(Http::CookieWrite cw | sink = cw.getHttpOnly()) }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(Http::CookieOptionWrite co | co.getHttpOnly() = pred and co.getCookieOutput() = succ)
+  }
+}
+
+/** Tracks flow from boolean expressions to the `HttpOnly` attribute of HTTP cookie writes. */
+module BooleanCookieHttpOnlyFlow = TaintTracking::Global<BooleanCookieHttpOnlyConfig>;
+
+/** Holds if `cw` has the `HttpOnly` attribute left at its default value of `false`. */
+predicate isNonHttpOnlyDefault(Http::CookieWrite cw) {
+  not BooleanCookieHttpOnlyFlow::flowTo(cw.getHttpOnly())
+}
+
+/** Holds if `cw` has the `HttpOnly` attribute explicitly set to `false`, from the expression `boolFalse`. */
+predicate isNonHttpOnlyDirect(Http::CookieWrite cw, Expr boolFalse) {
+  BooleanCookieHttpOnlyFlow::flow(DataFlow::exprNode(boolFalse), cw.getHttpOnly()) and
+  boolFalse.getBoolValue() = false
+}
+
+/** Holds if `cw` has the `HttpOnly` attribute set to `false`, either explicitly or by default. */
+predicate isNonHttpOnlyCookie(Http::CookieWrite cw) {
+  isNonHttpOnlyDefault(cw) or
+  isNonHttpOnlyDirect(cw, _)
+}
+
+/**
+ * Holds if `cw` has the sensitive name `name`, from the expression `nameExpr`.
+ * `source` and `sink` represent the data flow path from the sensitive name expression to the cookie write.
+ */
+predicate isSensitiveCookie(
+  Http::CookieWrite cw, string name, SensitiveCookieNameFlow::PathNode source,
+  SensitiveCookieNameFlow::PathNode sink
+) {
+  SensitiveCookieNameFlow::flowPath(source, sink) and
+  SensitiveCookieNameConfig::isSource(source.getNode(), name) and
+  SensitiveCookieNameConfig::isSink(sink.getNode(), cw)
+}

go/ql/lib/semmle/go/security/CookieWithoutSecure.qll

@@ -0,0 +1,37 @@
+/** Provides classes and predicates for identifying HTTP cookies without the `Secure` attribute. */
+
+import go
+import semmle.go.concepts.HTTP
+import semmle.go.dataflow.DataFlow
+
+private module BooleanCookieSecureConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source.getType().getUnderlyingType() instanceof BoolType
+  }
+
+  predicate isSink(DataFlow::Node sink) { exists(Http::CookieWrite cw | sink = cw.getSecure()) }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(Http::CookieOptionWrite co | co.getSecure() = pred and co.getCookieOutput() = succ)
+  }
+}
+
+/** Tracks flow from boolean expressions to the `Secure` attribute of HTTP cookie writes. */
+module BooleanCookieSecureFlow = TaintTracking::Global<BooleanCookieSecureConfig>;
+
+/** Holds if `cw` has the `Secure` attribute left at its default value of `false`. */
+predicate isInsecureDefault(Http::CookieWrite cw) {
+  not BooleanCookieSecureFlow::flowTo(cw.getSecure())
+}
+
+/** Holds if `cw` has the `Secure` attribute explicitly set to `false`, from the expression `boolFalse`. */
+predicate isInsecureDirect(Http::CookieWrite cw, Expr boolFalse) {
+  BooleanCookieSecureFlow::flow(DataFlow::exprNode(boolFalse), cw.getSecure()) and
+  boolFalse.getBoolValue() = false
+}
+
+/** Holds if `cw` has the `Secure` attribute set to `false`, either explicitly or by default. */
+predicate isInsecureCookie(Http::CookieWrite cw) {
+  isInsecureDefault(cw) or
+  isInsecureDirect(cw, _)
+}