Merge pull request #91 from max-schaefer/disabled-certificate-check

Add new query DisabledCertificateCheck.

change-notes/1.24/analysis-go.md

@@ -15,6 +15,7 @@ The CodeQL library for Go now contains a folder of simple "cookbook" queries tha
 |------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Bad check of redirect URL (`go/bad-redirect-check`)                          | correctness, security, external/cwe/cwe-601                                             | Highlights checks that ensure redirect URLs start with `/` but don't check for `//` or `/\`. Results are shown on LGTM by default.                                                  |
 | Constant length comparison (`go/constant-length-comparison`)                 | correctness                                                                             | Highlights code that checks the length of an array or slice against a constant before indexing it using a variable, suggesting a logic error. Results are shown on LGTM by default. |
+| Disabled TLS certificate check (`go/disabled-certificate-check`)             | security, external/cwe/295                                                              | Highlights code that disables TLS certificate checking. Results are shown on LGTM by default.                                                                                       |
 | Impossible interface nil check (`go/impossible-interface-nil-check`)         | correctness                                                                             | Highlights code that compares an interface value that cannot be `nil` to `nil`, suggesting a logic error. Results are shown on LGTM by default.                                     |
 | Incomplete URL scheme check (`go/incomplete-url-scheme-check`)               | correctness, security, external/cwe/cwe-020                                             | Highlights checks for `javascript` URLs that do not take `data` or `vbscript` URLs into account. Results are shown on LGTM by default.                                              |
 | Potentially unsafe quoting (`go/unsafe-quoting`)                             | correctness, security, external/cwe/cwe-078, external/cwe/cwe-089, external/cwe/cwe-094 | Highlights code that constructs a quoted string literal containing data that may itself contain quotes. Results are shown on LGTM by default.                                       |

ql/src/Security/CWE-295/DisabledCertificateCheck.go

@@ -0,0 +1,15 @@
+package main
+
+import (
+	"crypto/tls"
+	"net/http"
+)
+
+func doAuthReq(authReq *http.Request) *http.Response {
+	tr := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+	}
+	client := &http.Client{Transport: tr}
+	res, _ := client.Do(authReq)
+	return res
+}

ql/src/Security/CWE-295/DisabledCertificateCheck.qhelp

@@ -0,0 +1,39 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+The field <code>InsecureSkipVerify</code> controls whether a TLS client verifies the server's
+certificate chain and host name. If set to <code>true</code>, the client will accept any certificate
+and any host name in that certificate, making it susceptible to man-in-the-middle attacks.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Do not set <code>InsecureSkipVerify</code> to <code>true</code> except in tests.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following code snippet shows a function that performs an HTTP request over TLS with
+certificate verification disabled:
+</p>
+<sample src="DisabledCertificateCheck.go"/>
+<p>
+While this is acceptable in a test, it should not be used in production code. Instead, certificates
+should be configured such that verification can be performed.
+</p>
+</example>
+<references>
+<li>
+Package tls: <a href="https://golang.org/pkg/crypto/tls/#Config">Config</a>.
+</li>
+<li>
+SSL.com: <a href="https://www.ssl.com/article/browsers-and-certificate-validation/">Browsers and Certificate Validation</a>.
+</li>
+</references>
+</qhelp>

ql/src/Security/CWE-295/DisabledCertificateCheck.ql

@@ -0,0 +1,119 @@
+/**
+ * @name Disabled TLS certificate check
+ * @description If an application disables TLS certificate checking, it may be vulnerable to
+ *              man-in-the-middle attacks.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id go/disabled-certificate-check
+ * @tags security
+ *       external/cwe/cwe-295
+ *
+ * The approach taken by this query is to look for assignments that set `InsecureSkipVerify`
+ * (from struct `Config` of package `crypto/tls`) to `true`. We exclude assignments that are
+ * guarded by a feature-flag selecting whether verification should be skipped or not, since
+ * such assignments are not by themselves dangerous. Similarly, we exclude cases where the
+ * name of the enclosing function or the name of a variable/field into which the configuration
+ * flows suggests that it is deliberately insecure.
+ *
+ * We also exclude assignments in test code, where it makes sense to disable certificate checking.
+ */
+
+import go
+
+/**
+ * Holds if `name` may be the name of a feature flag that controls whether certificate checking is
+ * enabled.
+ */
+bindingset[name]
+predicate isFeatureFlagName(string name) {
+  name.regexpMatch("(?i).*(secure|selfCert|selfSign|validat|verif|trust|(en|dis)able).*")
+}
+
+/** Gets a global value number representing a (likely) feature flag for certificate checking. */
+GVN getAFeatureFlag() {
+  // a call like `cfg.disableVerification()`
+  exists(DataFlow::CallNode c | isFeatureFlagName(c.getTarget().getName()) |
+    result = globalValueNumber(c)
+  )
+  or
+  // a variable or field like `insecure`
+  exists(ValueEntity flag | isFeatureFlagName(flag.getName()) |
+    result = globalValueNumber(flag.getARead())
+  )
+  or
+  // a string constant such as `"insecure"` or `"skipVerification"`
+  exists(DataFlow::Node const | isFeatureFlagName(const.getStringValue()) |
+    result = globalValueNumber(const)
+  )
+  or
+  // track feature flags through various operations
+  exists(DataFlow::Node flag | flag = getAFeatureFlag().getANode() |
+    // tuple destructurings
+    result = globalValueNumber(DataFlow::extractTupleElement(flag, _))
+    or
+    // type casts
+    exists(DataFlow::TypeCastNode tc |
+      tc.getOperand() = flag and
+      result = globalValueNumber(tc)
+    )
+    or
+    // pointer dereferences
+    exists(DataFlow::PointerDereferenceNode deref |
+      deref.getOperand() = flag and
+      result = globalValueNumber(deref)
+    )
+    or
+    // calls like `os.Getenv("DISABLE_TLS_VERIFICATION")`
+    exists(DataFlow::CallNode call |
+      call.getAnArgument() = flag and
+      result = globalValueNumber(call)
+    )
+    or
+    // comparisons like `insecure == true`
+    exists(DataFlow::EqualityTestNode eq |
+      eq.getAnOperand() = flag and
+      result = globalValueNumber(eq)
+    )
+  )
+}
+
+/**
+ * Gets a control-flow node that represents a (likely) feature-flag check for certificate checking.
+ */
+ControlFlow::ConditionGuardNode getAFeatureFlagCheck() {
+  result.ensures(getAFeatureFlag().getANode(), _)
+}
+
+/**
+ * Holds if `part` becomes a part of `whole`, either by (local) data flow or by being incorporated
+ * into `whole` through having its address taken or being written to a field of `whole`.
+ */
+predicate becomesPartOf(DataFlow::Node part, DataFlow::Node whole) {
+  DataFlow::localFlow(part, whole)
+  or
+  whole.(DataFlow::AddressOperationNode).getOperand() = part
+  or
+  exists(Write w | w.writesField(whole.(DataFlow::PostUpdateNode).getPreUpdateNode(), _, part))
+}
+
+from Write w, DataFlow::Node base, Field f, DataFlow::Node rhs
+where
+  w.writesField(base, f, rhs) and
+  f.hasQualifiedName("crypto/tls", "Config", "InsecureSkipVerify") and
+  rhs.getBoolValue() = true and
+  // exclude writes guarded by a feature flag
+  not getAFeatureFlagCheck().dominatesNode(w) and
+  // exclude results in functions whose name documents the insecurity
+  not exists(FuncDef fn | fn = w.getRoot() |
+    isFeatureFlagName(fn.getEnclosingFunction*().getName())
+  ) and
+  // exclude results that flow into a field/variable whose name documents the insecurity
+  not exists(ValueEntity e, DataFlow::Node init |
+    isFeatureFlagName(e.getName()) and
+    any(Write w2).writes(e, init) and
+    becomesPartOf*(base, init)
+  ) and
+  // exclude results in test code
+  exists(File fl | fl = w.getFile() | not fl = any(TestCase tc).getFile())
+select w, "InsecureSkipVerify should not be used in production code."

ql/src/filters/ClassifyFiles.ql

@@ -12,19 +12,16 @@ string generatorCommentRegex() {
   result = "Generated By\\b.*\\bDo not edit" or
   result = "This (file|class|interface|art[ei]fact) (was|is|(has been)) (?:auto[ -]?)?gener(e?)ated" or
   result = "Any modifications to this file will be lost" or
-  result = "This (file|class|interface|art[ei]fact) (was|is) (?:mechanically|automatically) generated" or
+  result =
+    "This (file|class|interface|art[ei]fact) (was|is) (?:mechanically|automatically) generated" or
   result = "The following code was (?:auto[ -]?)?generated (?:by|from)" or
   result = "Autogenerated by Thrift" or
   result = "(Code g|G)enerated from .* by ANTLR"
 }
 
 predicate classify(File f, string category) {
-  // `go test`-style test
-  f.getBaseName().regexpMatch(".*_test.go") and
-  exists(FuncDecl fn |
-    fn.getName().regexpMatch("(Test|Benchmark|Example)[^a-z].*") and
-    fn.getFile() = f
-  ) and
+  // tests
+  f = any(TestCase tc).getFile() and
   category = "test"
   or
   // vendored code

ql/src/go.qll

@@ -28,5 +28,6 @@ import semmle.go.frameworks.SystemCommandExecutors
 import semmle.go.frameworks.SQL
 import semmle.go.frameworks.XPath
 import semmle.go.frameworks.Stdlib
+import semmle.go.frameworks.Testing
 import semmle.go.security.FlowSources
 import semmle.go.Util

ql/src/semmle/go/Decls.qll

@@ -129,6 +129,13 @@ class FuncDef extends @funcdef, StmtParent, ExprParent {
     result.getFunction() = this
   }
 
+  /**
+   * Gets the number of parameters of this function.
+   */
+  int getNumParameter() {
+    result = count(getAParameter())
+  }
+
   /**
    * Gets a call to this function.
    */

ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll

@@ -736,7 +736,7 @@ class MethodReadNode extends ReadNode {
 }
 
 /**
- * An IR instruction performing a relational comparison using `<`, `<=`, `>` or `>=`.
+ * A data-flow node performing a relational comparison using `<`, `<=`, `>` or `>=`.
  */
 class RelationalComparisonNode extends BinaryOperationNode, ExprNode {
   override RelationalComparisonExpr expr;
@@ -756,7 +756,7 @@ class RelationalComparisonNode extends BinaryOperationNode, ExprNode {
 }
 
 /**
- * An IR instruction performing an equality test using `==` or `!=`.
+ * A data-flow node performing an equality test using `==` or `!=`.
  */
 class EqualityTestNode extends BinaryOperationNode, ExprNode {
   override EqualityTestExpr expr;
@@ -768,6 +768,25 @@ class EqualityTestNode extends BinaryOperationNode, ExprNode {
   }
 }
 
+/**
+ * A data-flow node performing a type cast using either a type conversion
+ * or an assertion.
+ */
+class TypeCastNode extends ExprNode {
+  TypeCastNode() {
+    expr instanceof TypeAssertExpr
+    or
+    expr instanceof ConversionExpr
+  }
+
+  /** Gets the operand of the type cast. */
+  DataFlow::Node getOperand() {
+    result.asExpr() = expr.(TypeAssertExpr).getExpr()
+    or
+    result.asExpr() = expr.(ConversionExpr).getOperand()
+  }
+}
+
 /**
  * A model of a function specifying that the function copies input values from
  * a parameter or qualifier to a result.

ql/src/semmle/go/frameworks/Testing.qll

@@ -0,0 +1,42 @@
+/** Provides classes for working with tests. */
+
+import go
+
+/**
+ * A program element that represents a test case.
+ *
+ * Extend this class to refine existing models of testing frameworks. If you want to model new
+ * frameworks, extend `TestCase::Range` instead.
+ */
+class TestCase extends AstNode {
+  TestCase::Range self;
+
+  TestCase() { this = self }
+}
+
+/** Provides classes for working with test cases. */
+module TestCase {
+  /**
+   * A program element that represents a test case.
+   *
+   * Extend this class to model new testing frameworks. If you want to refine existing models,
+   * extend `TestCase` instead.
+   */
+  abstract class Range extends AstNode { }
+
+  /** A `go test` style test (including benchmarks and examples). */
+  private class GoTestFunction extends Range, FuncDef {
+    GoTestFunction() {
+      getName().regexpMatch("Test[^a-z].*") and
+      getNumParameter() = 1 and
+      getParameter(0).getType().(PointerType).getBaseType().hasQualifiedName("testing", "T")
+      or
+      getName().regexpMatch("Benchmark[^a-z].*") and
+      getNumParameter() = 1 and
+      getParameter(0).getType().(PointerType).getBaseType().hasQualifiedName("testing", "B")
+      or
+      getName().regexpMatch("Example[^a-z].*") and
+      getNumParameter() = 0
+    }
+  }
+}

ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.expected

@@ -0,0 +1,4 @@
+| DisabledCertificateCheck.go:10:32:10:55 | init of key-value pair | InsecureSkipVerify should not be used in production code. |
+| main.go:9:2:9:23 | assignment to field InsecureSkipVerify | InsecureSkipVerify should not be used in production code. |
+| main.go:57:21:57:44 | init of key-value pair | InsecureSkipVerify should not be used in production code. |
+| main.go:62:32:62:55 | init of key-value pair | InsecureSkipVerify should not be used in production code. |

ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.go

@@ -0,0 +1,15 @@
+package main
+
+import (
+	"crypto/tls"
+	"net/http"
+)
+
+func doAuthReq(authReq *http.Request) *http.Response {
+	tr := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, // NOT OK
+	}
+	client := &http.Client{Transport: tr}
+	res, _ := client.Do(authReq)
+	return res
+}

ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.qlref

@@ -0,0 +1 @@
+Security/CWE-295/DisabledCertificateCheck.ql

ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/main.go

@@ -0,0 +1,72 @@
+package main
+
+import (
+	"crypto/tls"
+	"net/http"
+)
+
+func bad1(cfg *tls.Config) {
+	cfg.InsecureSkipVerify = true // NOT OK
+}
+
+func good1(cfg *tls.Config) {
+	cfg.InsecureSkipVerify = false // OK
+}
+
+type opts struct {
+	mode string
+}
+
+func (opts) validateCertificates() bool { return true }
+
+func (opts) get(option string) (bool, error) { return true, nil }
+
+func good2(cfg *tls.Config, secure, selfCert, selfSign bool, options *opts, trustCertificates *bool,
+	enableVerification interface{}, disableVerification bool) {
+	if !secure {
+		cfg.InsecureSkipVerify = true // OK
+	}
+	if selfCert {
+		cfg.InsecureSkipVerify = true // OK
+	}
+	if selfSign {
+		cfg.InsecureSkipVerify = true // OK
+	}
+	if !options.validateCertificates() {
+		cfg.InsecureSkipVerify = true // OK
+	}
+	if v, _ := options.get("verifyCertificates"); !v {
+		cfg.InsecureSkipVerify = true // OK
+	}
+	if *trustCertificates {
+		cfg.InsecureSkipVerify = true // OK
+	}
+	if !enableVerification.(bool) {
+		cfg.InsecureSkipVerify = true // OK
+	}
+	if options.mode == "disableVerification" {
+		cfg.InsecureSkipVerify = true // OK
+	}
+}
+
+func makeInsecureConfig() *tls.Config {
+	return &tls.Config{InsecureSkipVerify: true} // OK
+}
+
+func makeConfig() *tls.Config {
+	return &tls.Config{InsecureSkipVerify: true} // NOT OK
+}
+
+func bad3() *http.Transport {
+	transport := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, // NOT OK
+	}
+	return transport
+}
+
+func good3() *http.Transport {
+	insecureTransport := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, // OK
+	}
+	return insecureTransport
+}

ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/tst.go

@@ -0,0 +1,13 @@
+package main
+
+import "testing"
+
+
+func TestSomethingExciting(t *testing.T) {
+	transport := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, // OK
+	}
+	doStuffTo(transport)
+}
+
+func doStuffTo(t *http.Transport) {}