Fixes, add secure query

Author: joefarebrother

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

@@ -27,10 +27,12 @@ private module SensitiveCookieNameConfig implements DataFlow::ConfigSig {
 }
 
 /** Tracks flow from sensitive names to HTTP cookie writes. */
-module SensitiveCookieNameFlow = DataFlow::Global<SensitiveCookieNameConfig>;
+module SensitiveCookieNameFlow = TaintTracking::Global<SensitiveCookieNameConfig>;
 
 private module BooleanCookieSecureConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { exists(source.asExpr().getBoolValue()) }
+  predicate isSource(DataFlow::Node source) {
+    source.getType().getUnderlyingType() instanceof BoolType
+  }
 
   predicate isSink(DataFlow::Node sink) { exists(Http::CookieWrite cw | sink = cw.getSecure()) }
 
@@ -39,11 +41,13 @@ private module BooleanCookieSecureConfig implements DataFlow::ConfigSig {
   }
 }
 
-/** Tracks flow from boolean expressions to the `Secure` attribute HTTP cookie writes. */
-module BooleanCookieSecureFlow = DataFlow::Global<BooleanCookieSecureConfig>;
+/** Tracks flow from boolean expressions to the `Secure` attribute of HTTP cookie writes. */
+module BooleanCookieSecureFlow = TaintTracking::Global<BooleanCookieSecureConfig>;
 
 private module BooleanCookieHttpOnlyConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { exists(source.asExpr().getBoolValue()) }
+  predicate isSource(DataFlow::Node source) {
+    source.getType().getUnderlyingType() instanceof BoolType
+  }
 
   predicate isSink(DataFlow::Node sink) { exists(Http::CookieWrite cw | sink = cw.getHttpOnly()) }
 
@@ -52,28 +56,53 @@ private module BooleanCookieHttpOnlyConfig implements DataFlow::ConfigSig {
   }
 }
 
-/** Tracks flow from boolean expressions to the `HttpOnly` attribute HTTP cookie writes. */
-module BooleanCookieHttpOnlyFlow = DataFlow::Global<BooleanCookieHttpOnlyConfig>;
+/** Tracks flow from boolean expressions to the `HttpOnly` attribute of HTTP cookie writes. */
+module BooleanCookieHttpOnlyFlow = TaintTracking::Global<BooleanCookieHttpOnlyConfig>;
 
+/** Holds if `cw` has the `Secure` attribute left at its default value of `false`. */
 predicate isInsecureDefault(Http::CookieWrite cw) {
   not BooleanCookieSecureFlow::flow(_, cw.getSecure())
 }
 
-predicate isNonHttpOnlyDefault(Http::CookieWrite cw) {
-  not BooleanCookieHttpOnlyFlow::flow(_, cw.getHttpOnly())
-}
-
+/** 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, _)
+}
+
+/** Holds if `cw` has the `HttpOnly` attribute left at its default value of `false`. */
+predicate isNonHttpOnlyDefault(Http::CookieWrite cw) {
+  not BooleanCookieHttpOnlyFlow::flow(_, 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
 }
 
-predicate isSensitiveCookie(Http::CookieWrite cw, Expr nameExpr, string name) {
-  SensitiveCookieNameFlow::flow(DataFlow::exprNode(nameExpr), cw.getName()) and
+/** 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, Expr nameExpr, string name, SensitiveCookieNameFlow::PathNode source,
+  SensitiveCookieNameFlow::PathNode sink
+) {
+  SensitiveCookieNameFlow::flowPath(source, sink) and
+  source.getNode().asExpr() = nameExpr and
+  sink.getNode() = cw.getName() and
   isSensitiveExpr(nameExpr, name)
 }

go/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.ql

@@ -4,25 +4,24 @@
  *              malicious JavaScript to steal it in case of XSS vulnerability. Always set
  *              'HttpOnly' to 'true' to authentication related cookie to make it
  *              not accessible by JavaScript.
- * @kind problem
+ * @kind path-problem
  * @problem.severity warning
  * @precision high
  * @id go/cookie-httponly-not-set
  * @tags security
- *       experimental
  *       external/cwe/cwe-1004
  */
 
 import go
 import semmle.go.security.SecureCookies
 import semmle.go.concepts.HTTP
+import SensitiveCookieNameFlow::PathGraph
 
-from Http::CookieWrite cw, Expr sensitiveNameExpr, string name
+from
+  Http::CookieWrite cw, Expr sensitiveNameExpr, string name,
+  SensitiveCookieNameFlow::PathNode source, SensitiveCookieNameFlow::PathNode sink
 where
-  isSensitiveCookie(cw, sensitiveNameExpr, name) and
-  (
-    isNonHttpOnlyDefault(cw)
-    or
-    isNonHttpOnlyDirect(cw, _)
-  )
-select cw, "Sensitive cookie $@ does not set HttpOnly to true", sensitiveNameExpr, name
+  isSensitiveCookie(cw, sensitiveNameExpr, name, source, sink) and
+  isNonHttpOnlyCookie(cw)
+select cw, source, sink, "Sensitive cookie $@ does not set HttpOnly attribute to true.",
+  sensitiveNameExpr, name

go/ql/src/Security/CWE-614/CookieWithoutSecure.ql

@@ -0,0 +1,18 @@
+/**
+ * @name 'Secure' attribute is not set to true
+ * @description todo
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id go/cookie-secure-not-set
+ * @tags security
+ *       external/cwe/cwe-1004
+ */
+
+import go
+import semmle.go.security.SecureCookies
+import semmle.go.concepts.HTTP
+
+from Http::CookieWrite cw
+where isInsecureCookie(cw)
+select cw, "Cookie does not set Secure attribute to true"