From 88cb435843ecc75b3c34520d37409c253b5feceb Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 28 Jul 2020 13:26:55 +0100 Subject: [PATCH] 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. --- .../CWE-295/DisabledCertificateCheck.ql | 15 +++- ql/src/experimental/CWE-327/InsecureTLS.ql | 9 +-- .../go/security/InsecureFeatureFlag.qll | 72 +++++++++++++------ 3 files changed, 69 insertions(+), 27 deletions(-) diff --git a/ql/src/Security/CWE-295/DisabledCertificateCheck.ql b/ql/src/Security/CWE-295/DisabledCertificateCheck.ql index 68ca03a4aa0..3375f6c9a46 100644 --- a/ql/src/Security/CWE-295/DisabledCertificateCheck.ql +++ b/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 diff --git a/ql/src/experimental/CWE-327/InsecureTLS.ql b/ql/src/experimental/CWE-327/InsecureTLS.ql index 35a704ab5bc..f02a3cc61c4 100644 --- a/ql/src/experimental/CWE-327/InsecureTLS.ql +++ b/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 diff --git a/ql/src/semmle/go/security/InsecureFeatureFlag.qll b/ql/src/semmle/go/security/InsecureFeatureFlag.qll index d6f22aa2d51..84f368c5e4d 100644 --- a/ql/src/semmle/go/security/InsecureFeatureFlag.qll +++ b/ql/src/semmle/go/security/InsecureFeatureFlag.qll @@ -5,17 +5,23 @@ 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 @@ -23,13 +29,17 @@ module InsecureFeatureFlag { * '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 getAFeatureFlagCheck() { - result.ensures(getAFlag(featureFlag()).getANode(), _) + ControlFlow::ConditionGuardNode getASecurityFeatureFlagCheck() { + result.ensures(getAFlag(securityFeatureFlag()).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 TLS version selection. */ - ControlFlow::ConditionGuardNode getALegacyVersionCheck() { - result.ensures(getAFlag(legacyFlag()).getANode(), _) + ControlFlow::ConditionGuardNode getALegacyTlsVersionCheck() { + result.ensures(getAFlag(legacyTlsVersionFlag()).getANode(), _) + } + + /** + * Gets a control-flow node that represents a (likely) flag controlling an insecure certificate setup. + */ + ControlFlow::ConditionGuardNode getAnInsecureCertificateCheck() { + result.ensures(getAFlag(insecureCertificateFlag()).getANode(), _) } }