diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index f71ef2a1613..6355bf3bd63 100644 --- a/change-notes/1.24/analysis-go.md +++ b/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. | diff --git a/ql/src/Security/CWE-295/DisabledCertificateCheck.go b/ql/src/Security/CWE-295/DisabledCertificateCheck.go new file mode 100644 index 00000000000..ecc091f7afd --- /dev/null +++ b/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 +} diff --git a/ql/src/Security/CWE-295/DisabledCertificateCheck.qhelp b/ql/src/Security/CWE-295/DisabledCertificateCheck.qhelp new file mode 100644 index 00000000000..604a52fa2a3 --- /dev/null +++ b/ql/src/Security/CWE-295/DisabledCertificateCheck.qhelp @@ -0,0 +1,39 @@ + + + + +

+The field InsecureSkipVerify controls whether a TLS client verifies the server's +certificate chain and host name. If set to true, the client will accept any certificate +and any host name in that certificate, making it susceptible to man-in-the-middle attacks. +

+
+ + +

+Do not set InsecureSkipVerify to true except in tests. +

+
+ + +

+The following code snippet shows a function that performs an HTTP request over TLS with +certificate verification disabled: +

+ +

+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. +

+
+ +
  • +Package tls: Config. +
  • +
  • +SSL.com: Browsers and Certificate Validation. +
  • +
    +
    diff --git a/ql/src/Security/CWE-295/DisabledCertificateCheck.ql b/ql/src/Security/CWE-295/DisabledCertificateCheck.ql new file mode 100644 index 00000000000..1adeb5be67c --- /dev/null +++ b/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." diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index d1396f86f81..faa47e44df3 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -711,7 +711,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; @@ -731,7 +731,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; @@ -743,6 +743,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. diff --git a/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.expected b/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.expected new file mode 100644 index 00000000000..6e7de24be8e --- /dev/null +++ b/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. | diff --git a/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.go b/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.go new file mode 100644 index 00000000000..b0490ad6f4f --- /dev/null +++ b/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 +} diff --git a/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.qlref b/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.qlref new file mode 100644 index 00000000000..cca259717b5 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.qlref @@ -0,0 +1 @@ +Security/CWE-295/DisabledCertificateCheck.ql diff --git a/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/main.go b/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/main.go new file mode 100644 index 00000000000..027a88b9bb6 --- /dev/null +++ b/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 +} diff --git a/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/tst.go b/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/tst.go new file mode 100644 index 00000000000..b3bec373fd9 --- /dev/null +++ b/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) {}