Clean up InsecureFeatureFlag

Move the flag regexes inline, use `any` instead of a constructor function to select a particular flag kind, and remove explicit limitation on the common superclass FlagKind.

Author: smowton

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

@@ -36,13 +36,22 @@ predicate becomesPartOf(DataFlow::Node part, DataFlow::Node whole) {
   exists(Write w | w.writesField(whole.(DataFlow::PostUpdateNode).getPreUpdateNode(), _, part))
 }
 
+/**
+ * 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) {
-  isSecurityFlagName(name) or isCertificateFlagName(name)
+  name = securityOrTlsVersionFlag().getAFlagName()
 }
 
 from Write w, DataFlow::Node base, Field f, DataFlow::Node rhs

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

@@ -226,6 +226,15 @@ predicate isInsecureTlsCipherFlow(DataFlow::PathNode source, DataFlow::PathNode
   )
 }
 
+/**
+ * Returns flag kinds relevant to this query: a generic security feature flag, or one
+ * specifically controlling TLS version selection.
+ */
+FlagKind securityOrTlsVersionFlag() {
+  result = any(SecurityFeatureFlag f) or
+  result = any(LegacyTlsVersionFlag f)
+}
+
 from DataFlow::PathNode source, DataFlow::PathNode sink, string message
 where
   (
@@ -236,11 +245,9 @@ where
   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*(),
-    [securityFeatureFlag(), legacyTlsVersionFlag()]) and
+  not astNodeIsFlag([source, sink].getNode().asExpr().getParent*(), securityOrTlsVersionFlag()) and
   // Exclude results in functions whose name documents insecurity
   not exists(FuncDef fn | fn = sink.getNode().getRoot().getEnclosingFunction*() |
-    isSecurityFlagName(fn.getName()) or
-    isLegacyTlsFlagName(fn.getName())
+    fn.getName() = securityOrTlsVersionFlag().getAFlagName()
   )
 select sink.getNode(), source, sink, message

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

@@ -5,41 +5,12 @@
 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 isCertificateFlagName(string name) {
-    name.regexpMatch("(?i).*(selfCert|selfSign|validat|verif|trust).*")
-  }
-
-  /**
-   * 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 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 = "securityFeature" or this = "legacyTlsVersion" or this = "insecureCertificate"
-    }
+    bindingset[this]
+    FlagKind() { any() }
 
     /**
      * Returns a flag name of this type.
@@ -54,44 +25,36 @@ module InsecureFeatureFlag {
     SecurityFeatureFlag() { this = "securityFeature" }
 
     bindingset[result]
-    override string getAFlagName() { isSecurityFlagName(result) }
+    override string getAFlagName() { result.regexpMatch("(?i).*(secure|(en|dis)able).*") }
   }
 
-  /**
-   * Flags suggesting an optional feature, perhaps deliberately insecure.
-   */
-  string securityFeatureFlag() { result = "securityFeature" }
-
   /**
    * Flags suggesting support for an old or legacy TLS version.
+   *
+   * 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)
    */
   class LegacyTlsVersionFlag extends FlagKind {
     LegacyTlsVersionFlag() { this = "legacyTlsVersion" }
 
     bindingset[result]
-    override string getAFlagName() { isLegacyTlsFlagName(result) }
+    override string getAFlagName() { result.regexpMatch("(?i).*(old|intermediate|legacy).*") }
   }
 
-  /**
-   * 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) }
+    override string getAFlagName() {
+      result.regexpMatch("(?i).*(selfCert|selfSign|validat|verif|trust).*")
+    }
   }
 
-  /**
-   * Flags suggesting support for an old or legacy feature.
-   */
-  string insecureCertificateFlag() { result = "insecureCertificate" }
-
   /** Gets a global value number representing a (likely) security flag. */
   GVN getAFlag(FlagKind flagKind) {
     // a call like `cfg.disableVerification()`
@@ -151,7 +114,7 @@ module InsecureFeatureFlag {
   }
 
   /**
-   * Holds if `node` suggests an old TLS version according to `flagKind`.
+   * Holds if `node` involves a string of kind `flagKind`.
    */
   predicate astNodeIsFlag(AstNode node, FlagKind flagKind) {
     // Map literal flag: value or "flag": value
@@ -177,20 +140,20 @@ module InsecureFeatureFlag {
    * Gets a control-flow node that represents a (likely) security feature-flag check
    */
   ControlFlow::ConditionGuardNode getASecurityFeatureFlagCheck() {
-    result.ensures(getAFlag(securityFeatureFlag()).getANode(), _)
+    result.ensures(getAFlag(any(SecurityFeatureFlag f)).getANode(), _)
   }
 
   /**
    * Gets a control-flow node that represents a (likely) flag controlling TLS version selection.
    */
   ControlFlow::ConditionGuardNode getALegacyTlsVersionCheck() {
-    result.ensures(getAFlag(legacyTlsVersionFlag()).getANode(), _)
+    result.ensures(getAFlag(any(LegacyTlsVersionFlag f)).getANode(), _)
   }
 
   /**
    * Gets a control-flow node that represents a (likely) flag controlling an insecure certificate setup.
    */
   ControlFlow::ConditionGuardNode getAnInsecureCertificateCheck() {
-    result.ensures(getAFlag(insecureCertificateFlag()).getANode(), _)
+    result.ensures(getAFlag(any(InsecureCertificateFlag f)).getANode(), _)
   }
 }