Merge pull request #256 from smowton/smowton/admin/cwe-327-cleanup

Polish CWE-327 (weak TLS config) query

Author: max-schaefer

change-notes/2020-07-15-insecure-tls.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Query "Insecure TLS configuration" (`go/insecure-tls`) is promoted from experimental status. This checks for use of insecure SSL/TLS versions and cipher suites.

ql/src/InconsistentCode/MissingErrorCheck.ql

@@ -90,7 +90,7 @@ predicate returnUncheckedAtNode(
     err.getAPredecessor() = call.getResult(1) and
     call.asInstruction() = node and
     isDereferenceableType(ptr.getType()) and
-    err.getType().implements(Builtin::error().getType().getUnderlyingType()) and
+    err.getType() instanceof ErrorType and
     calleeMayReturnNilWithError(call)
     or
     // Recursive case: check that some predecessor is missing a check, and `node` does not itself

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

@@ -22,81 +22,55 @@
  */
 
 import go
+import semmle.go.security.InsecureFeatureFlag::InsecureFeatureFlag
 
 /**
- * Holds if `name` may be the name of a feature flag that controls whether certificate checking is
- * enabled.
+ * 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`.
  */
-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())
-  )
+predicate becomesPartOf(DataFlow::Node part, DataFlow::Node whole) {
+  DataFlow::localFlow(part, whole)
   or
-  // a string constant such as `"insecure"` or `"skipVerification"`
-  exists(DataFlow::Node const | isFeatureFlagName(const.getStringValue()) |
-    result = globalValueNumber(const)
-  )
+  whole.(DataFlow::AddressOperationNode).getOperand() = part
   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)
-    )
-  )
+  exists(Write w | w.writesField(whole.(DataFlow::PostUpdateNode).getPreUpdateNode(), _, part))
 }
 
 /**
- * Gets a control-flow node that represents a (likely) feature-flag check for certificate checking.
+ * Flags suggesting a deliberately insecure certificate setup.
  */
-ControlFlow::ConditionGuardNode getAFeatureFlagCheck() {
-  result.ensures(getAFeatureFlag().getANode(), _)
+class InsecureCertificateFlag extends FlagKind {
+  InsecureCertificateFlag() { this = "insecureCertificate" }
+
+  bindingset[result]
+  override string getAFlagName() {
+    result.regexpMatch("(?i).*(selfCert|selfSign|validat|verif|trust).*")
+  }
 }
 
 /**
- * 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`.
+ * Gets a control-flow node that represents a (likely) flag controlling an insecure certificate setup.
  */
-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))
+ControlFlow::ConditionGuardNode getAnInsecureCertificateCheck() {
+  result.ensures(any(InsecureCertificateFlag f).getAFlag().getANode(), _)
+}
+
+/**
+ * Returns flag kinds relevant to this query: a generic security feature flag, or one
+ * specifically controlling insecure certificate configuration.
+ */
+FlagKind securityOrTlsVersionFlag() {
+  result = any(SecurityFeatureFlag f) or
+  result = any(InsecureCertificateFlag f)
+}
+
+/**
+ * Holds if `name` is (likely to be) a general security flag or one specifically controlling
+ * an insecure certificate setup.
+ */
+bindingset[name]
+predicate isSecurityOrCertificateConfigFlag(string name) {
+  name = securityOrTlsVersionFlag().getAFlagName()
 }
 
 from Write w, DataFlow::Node base, Field f, DataFlow::Node rhs
@@ -105,14 +79,14 @@ where
   f.hasQualifiedName("crypto/tls", "Config", "InsecureSkipVerify") and
   rhs.getBoolValue() = true and
   // exclude writes guarded by a feature flag
-  not getAFeatureFlagCheck().dominatesNode(w) and
+  not [getASecurityFeatureFlagCheck(), getAnInsecureCertificateCheck()].dominatesNode(w) and
   // exclude results in functions whose name documents the insecurity
   not exists(FuncDef fn | fn = w.getRoot() |
-    isFeatureFlagName(fn.getEnclosingFunction*().getName())
+    isSecurityOrCertificateConfigFlag(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
+    isSecurityOrCertificateConfigFlag(e.getName()) and
     any(Write w2).writes(e, init) and
     becomesPartOf*(base, init)
   ) and

…/src/experimental/CWE-327/InsecureTLS.go ql/src/Security/CWE-327/InsecureTLS.goql/src/experimental/CWE-327/InsecureTLS.go renamed to ql/src/Security/CWE-327/InsecureTLS.go ql/src/experimental/CWE-327/InsecureTLS.go renamed to ql/src/Security/CWE-327/InsecureTLS.go

@@ -39,6 +39,10 @@
 	Wikipedia:
 	<a href="https://en.wikipedia.org/wiki/Transport_Layer_Security">Transport Layer Security</a>
 </li>
+<li>
+	Mozilla:
+	<a href="https://wiki.mozilla.org/Security/Server_Side_TLS">Security/Server Side TLS</a>
+</li>
 <li>
 	OWASP:
 	<a href="https://cheatsheetseries.owasp.org/cheatsheets/Transport_Layer_Protection_Cheat_Sheet.html">Transport Layer Protection Cheat Sheet</a>