Split security flags into more distinct categories

There are now three categories: general security or option flags, those related to TLS version selection, and those related to certificate configuration. The TLS and disabled-certificate-check queries use two categories each.

Author: smowton

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

@@ -36,20 +36,29 @@ predicate becomesPartOf(DataFlow::Node part, DataFlow::Node whole) {
   exists(Write w | w.writesField(whole.(DataFlow::PostUpdateNode).getPreUpdateNode(), _, part))
 }
 
+/**
+ * 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) {
+  isSecurityFlagName(name) or isCertificateFlagName(name)
+}
+
 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
+  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

ql/src/experimental/CWE-327/InsecureTLS.ql

@@ -233,13 +233,14 @@ where
     isInsecureTlsCipherFlow(source, sink, message)
   ) and
   // Exclude sources or sinks guarded by a feature or legacy flag
-  not [getAFeatureFlagCheck(), getALegacyVersionCheck()]
+  not [getASecurityFeatureFlagCheck(), getALegacyTlsVersionCheck()]
       .dominatesNode([source, sink].getNode().asInstruction()) and
   // Exclude sources or sinks that occur lexically within a block related to a feature or legacy flag
-  not astNodeIsFlag([source, sink].getNode().asExpr().getParent*(), [featureFlag(), legacyFlag()]) and
+  not astNodeIsFlag([source, sink].getNode().asExpr().getParent*(),
+    [securityFeatureFlag(), legacyTlsVersionFlag()]) and
   // Exclude results in functions whose name documents insecurity
   not exists(FuncDef fn | fn = sink.getNode().getRoot().getEnclosingFunction*() |
-    isFeatureFlagName(fn.getName()) or
-    isLegacyFlagName(fn.getName())
+    isSecurityFlagName(fn.getName()) or
+    isLegacyTlsFlagName(fn.getName())
   )
 select sink.getNode(), source, sink, message

ql/src/semmle/go/security/InsecureFeatureFlag.qll

@@ -5,31 +5,41 @@
 import go
 
 module InsecureFeatureFlag {
+  /**
+   * Holds if `name` may be the name of a feature flag that controls a security feature.
+   */
+  bindingset[name]
+  predicate isSecurityFlagName(string name) { name.regexpMatch("(?i).*(secure|(en|dis)able).*") }
+
   /**
    * 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).*")
+  predicate isCertificateFlagName(string name) {
+    name.regexpMatch("(?i).*(selfCert|selfSign|validat|verif|trust).*")
   }
 
   /**
-   * Holds if `name` suggests an old or legacy version.
+   * Holds if `name` suggests an old or legacy version of TLS.
    *
    * We accept 'intermediate' because it appears to be common for TLS users
    * to define three profiles: modern, intermediate, legacy/old, perhaps based
    * on https://wiki.mozilla.org/Security/Server_Side_TLS (though note the
    * 'intermediate' used there would now pass muster according to this query)
    */
   bindingset[name]
-  predicate isLegacyFlagName(string name) { name.regexpMatch("(?i).*(old|intermediate|legacy).*") }
+  predicate isLegacyTlsFlagName(string name) {
+    name.regexpMatch("(?i).*(old|intermediate|legacy).*")
+  }
 
   /**
    * A kind of flag that may indicate security expectations regarding the code it guards.
    */
   abstract class FlagKind extends string {
-    FlagKind() { this = "feature" or this = "legacy" }
+    FlagKind() {
+      this = "securityFeature" or this = "legacyTlsVersion" or this = "insecureCertificate"
+    }
 
     /**
      * Returns a flag name of this type.
@@ -40,32 +50,47 @@ module InsecureFeatureFlag {
   /**
    * Flags suggesting an optional feature, perhaps deliberately insecure.
    */
-  class FeatureFlag extends FlagKind {
-    FeatureFlag() { this = "feature" }
+  class SecurityFeatureFlag extends FlagKind {
+    SecurityFeatureFlag() { this = "securityFeature" }
 
     bindingset[result]
-    override string getAFlagName() { isFeatureFlagName(result) }
+    override string getAFlagName() { isSecurityFlagName(result) }
   }
 
   /**
    * Flags suggesting an optional feature, perhaps deliberately insecure.
    */
-  string featureFlag() { result = "feature" }
+  string securityFeatureFlag() { result = "securityFeature" }
 
   /**
-   * Flags suggesting support for an old or legacy feature.
+   * Flags suggesting support for an old or legacy TLS version.
    */
-  class LegacyFlag extends FlagKind {
-    LegacyFlag() { this = "legacy" }
+  class LegacyTlsVersionFlag extends FlagKind {
+    LegacyTlsVersionFlag() { this = "legacyTlsVersion" }
 
     bindingset[result]
-    override string getAFlagName() { isLegacyFlagName(result) }
+    override string getAFlagName() { isLegacyTlsFlagName(result) }
+  }
+
+  /**
+   * Flags suggesting support for an old or legacy TLS version.
+   */
+  string legacyTlsVersionFlag() { result = "legacyTlsVersion" }
+
+  /**
+   * Flags suggesting a deliberately insecure certificate setup.
+   */
+  class InsecureCertificateFlag extends FlagKind {
+    InsecureCertificateFlag() { this = "insecureCertificate" }
+
+    bindingset[result]
+    override string getAFlagName() { isCertificateFlagName(result) }
   }
 
   /**
    * Flags suggesting support for an old or legacy feature.
    */
-  string legacyFlag() { result = "legacy" }
+  string insecureCertificateFlag() { result = "insecureCertificate" }
 
   /** Gets a global value number representing a (likely) security flag. */
   GVN getAFlag(FlagKind flagKind) {
@@ -149,16 +174,23 @@ module InsecureFeatureFlag {
   }
 
   /**
-   * Gets a control-flow node that represents a (likely) feature-flag check for certificate checking.
+   * Gets a control-flow node that represents a (likely) security feature-flag check
+   */
+  ControlFlow::ConditionGuardNode getASecurityFeatureFlagCheck() {
+    result.ensures(getAFlag(securityFeatureFlag()).getANode(), _)
+  }
+
+  /**
+   * Gets a control-flow node that represents a (likely) flag controlling TLS version selection.
    */
-  ControlFlow::ConditionGuardNode getAFeatureFlagCheck() {
-    result.ensures(getAFlag(featureFlag()).getANode(), _)
+  ControlFlow::ConditionGuardNode getALegacyTlsVersionCheck() {
+    result.ensures(getAFlag(legacyTlsVersionFlag()).getANode(), _)
   }
 
   /**
-   * Gets a control-flow node that represents a (likely) feature-flag check for certificate checking.
+   * Gets a control-flow node that represents a (likely) flag controlling an insecure certificate setup.
    */
-  ControlFlow::ConditionGuardNode getALegacyVersionCheck() {
-    result.ensures(getAFlag(legacyFlag()).getANode(), _)
+  ControlFlow::ConditionGuardNode getAnInsecureCertificateCheck() {
+    result.ensures(getAFlag(insecureCertificateFlag()).getANode(), _)
   }
 }