From f31ed52943efec3a27ec6c71d402855e2bf7ef3b Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 29 Jul 2020 15:15:50 +0100 Subject: [PATCH] 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. --- .../CWE-295/DisabledCertificateCheck.ql | 11 ++- ql/src/Security/CWE-327/InsecureTLS.ql | 15 ++-- .../go/security/InsecureFeatureFlag.qll | 69 +++++-------------- 3 files changed, 37 insertions(+), 58 deletions(-) diff --git a/ql/src/Security/CWE-295/DisabledCertificateCheck.ql b/ql/src/Security/CWE-295/DisabledCertificateCheck.ql index 3375f6c9a46..7e340824f05 100644 --- a/ql/src/Security/CWE-295/DisabledCertificateCheck.ql +++ b/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 diff --git a/ql/src/Security/CWE-327/InsecureTLS.ql b/ql/src/Security/CWE-327/InsecureTLS.ql index f02a3cc61c4..456e3cef027 100644 --- a/ql/src/Security/CWE-327/InsecureTLS.ql +++ b/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 diff --git a/ql/src/semmle/go/security/InsecureFeatureFlag.qll b/ql/src/semmle/go/security/InsecureFeatureFlag.qll index 84f368c5e4d..8707a6b9e3d 100644 --- a/ql/src/semmle/go/security/InsecureFeatureFlag.qll +++ b/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,29 +25,24 @@ 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. */ @@ -84,14 +50,11 @@ module InsecureFeatureFlag { 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(), _) } }