From 57886e1713a4711854bc1fba11ccd91768729654 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Thu, 2 Mar 2023 13:19:37 -0500 Subject: [PATCH 01/23] Moved files from experimental to src/ --- .../Security/CWE/CWE-522/InsecureLdapAuth.java | 0 .../Security/CWE/CWE-522/InsecureLdapAuth.qhelp | 0 .../{experimental => }/Security/CWE/CWE-522/InsecureLdapAuth.ql | 0 .../query-tests/security/CWE-522/InsecureLdapAuth.qlref | 1 - .../query-tests/security/CWE-522/InsecureLdapAuth.expected | 0 .../query-tests/security/CWE-522/InsecureLdapAuth.java | 0 java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.qlref | 1 + 7 files changed, 1 insertion(+), 1 deletion(-) rename java/ql/src/{experimental => }/Security/CWE/CWE-522/InsecureLdapAuth.java (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-522/InsecureLdapAuth.qhelp (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-522/InsecureLdapAuth.ql (100%) delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.qlref rename java/ql/test/{experimental => }/query-tests/security/CWE-522/InsecureLdapAuth.expected (100%) rename java/ql/test/{experimental => }/query-tests/security/CWE-522/InsecureLdapAuth.java (100%) create mode 100644 java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.qlref diff --git a/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.java b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.java rename to java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.qhelp b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.qhelp rename to java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp diff --git a/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql rename to java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.qlref b/java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.qlref deleted file mode 100644 index c2baa9841779..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.expected b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.expected similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.expected rename to java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.expected diff --git a/java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.java b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.java similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.java rename to java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.java diff --git a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.qlref b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.qlref new file mode 100644 index 000000000000..952f1d5663ce --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-522/InsecureLdapAuth.ql From 5ff4fcbc76caf25bb66275354408266661636a8e Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Thu, 2 Mar 2023 13:20:36 -0500 Subject: [PATCH 02/23] Replace `exists` with `any` --- java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index 5f2bda49f7cd..d0da8198420d 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -205,7 +205,7 @@ class SslFlowConfig extends DataFlow::Configuration { from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureUrlFlowConfig config where config.hasFlowPath(source, sink) and - exists(BasicAuthFlowConfig bc | bc.hasFlowTo(sink.getNode())) and - not exists(SslFlowConfig sc | sc.hasFlowTo(sink.getNode())) + any(BasicAuthFlowConfig bc).hasFlowTo(sink.getNode()) and + not any(SslFlowConfig sc).hasFlowTo(sink.getNode()) select sink.getNode(), source, sink, "Insecure LDAP authentication from $@.", source.getNode(), "LDAP connection string" From 938d953789f99d8546781404dc7d2dd9296ce3e4 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 3 Mar 2023 15:01:13 -0500 Subject: [PATCH 03/23] Refactor getLeftmostOperand method --- java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index d0da8198420d..3f2ee65b0fad 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -53,8 +53,13 @@ predicate concatInsecureLdapString(Expr protocol, Expr host) { /** Gets the leftmost operand in a concatenated string */ Expr getLeftmostConcatOperand(Expr expr) { + // if expr instanceof AddExpr + // then result = getLeftmostConcatOperand(expr.(AddExpr).getLeftOperand()) + // else result = expr if expr instanceof AddExpr - then result = getLeftmostConcatOperand(expr.(AddExpr).getLeftOperand()) + then + result = expr.(AddExpr).getLeftOperand*() and + not result instanceof AddExpr else result = expr } From 9275b54e975a5aede33c00548c5717f200d13efe Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 7 Mar 2023 17:42:14 -0500 Subject: [PATCH 04/23] Refactoring the InsecureLdapUrl constructor --- .../Security/CWE/CWE-522/InsecureLdapAuth.ql | 47 ++++++++++--------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index 3f2ee65b0fad..c235511fd445 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -36,33 +36,30 @@ class TypeHashtable extends Class { TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") } } +string getHostname(Expr expr) { + result = expr.(CompileTimeConstantExpr).getStringValue() or + result = + expr.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getStringValue() +} + /** * Holds if a non-private LDAP string is concatenated from both protocol and host. */ -predicate concatInsecureLdapString(Expr protocol, Expr host) { - protocol.(CompileTimeConstantExpr).getStringValue() = "ldap://" and - not exists(string hostString | - hostString = host.(CompileTimeConstantExpr).getStringValue() or - hostString = - host.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getStringValue() - | +predicate concatInsecureLdapString(CompileTimeConstantExpr protocol, Expr host) { + protocol.getStringValue() = "ldap://" and + not exists(string hostString | hostString = getHostname(host) | hostString.length() = 0 or // Empty host is loopback address hostString instanceof PrivateHostName ) } -/** Gets the leftmost operand in a concatenated string */ -Expr getLeftmostConcatOperand(Expr expr) { - // if expr instanceof AddExpr - // then result = getLeftmostConcatOperand(expr.(AddExpr).getLeftOperand()) - // else result = expr - if expr instanceof AddExpr - then - result = expr.(AddExpr).getLeftOperand*() and - not result instanceof AddExpr - else result = expr -} - +// Expr getLeftmostConcatOperand(Expr expr) { +// if expr instanceof AddExpr +// then +// result = expr.(AddExpr).getLeftOperand() and +// not result instanceof AddExpr +// else result = expr +// } /** * String concatenated with `InsecureLdapUrlLiteral`. */ @@ -70,8 +67,16 @@ class InsecureLdapUrl extends Expr { InsecureLdapUrl() { this instanceof InsecureLdapUrlLiteral or - concatInsecureLdapString(this.(AddExpr).getLeftOperand(), - getLeftmostConcatOperand(this.(AddExpr).getRightOperand())) + // protocol + host + ... + exists(AddExpr e, CompileTimeConstantExpr protocol, Expr rest, Expr host | + e = this and + protocol = e.getLeftOperand() and + rest = e.getRightOperand() and + if rest instanceof AddExpr then host = rest.(AddExpr).getLeftOperand() else host = rest + | + protocol.getStringValue() = "ldap://" and + concatInsecureLdapString(protocol, host) + ) } } From 3936aea69062beb53afc4a74d77d6b515ef4ad63 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 8 Mar 2023 20:52:08 -0500 Subject: [PATCH 05/23] Split Ldap query file into libraries --- .../code/java/security/InsecureLdapAuth.qll | 128 +++++++++++ .../java/security/InsecureLdapAuthQuery.qll | 78 +++++++ .../Security/CWE/CWE-522/InsecureLdapAuth.ql | 199 +----------------- 3 files changed, 207 insertions(+), 198 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll create mode 100644 java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll new file mode 100644 index 000000000000..d5948f2ff048 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll @@ -0,0 +1,128 @@ +/** Provides classes to reason about insecure LDAP authentication. */ + +import java +import semmle.code.java.frameworks.Networking +import semmle.code.java.frameworks.Jndi + +/** + * Insecure (non-SSL, non-private) LDAP URL string literal. + */ +class InsecureLdapUrlLiteral extends StringLiteral { + InsecureLdapUrlLiteral() { + // Match connection strings with the LDAP protocol and without private IP addresses to reduce false positives. + exists(string s | this.getValue() = s | + s.regexpMatch("(?i)ldap://[\\[a-zA-Z0-9].*") and + not s.substring(7, s.length()) instanceof PrivateHostName + ) + } +} + +/** The class `java.util.Hashtable`. */ +class TypeHashtable extends Class { + TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") } +} + +string getHostname(Expr expr) { + result = expr.(CompileTimeConstantExpr).getStringValue() or + result = + expr.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getStringValue() +} + +/** + * Holds if a non-private LDAP string is concatenated from both protocol and host. + */ +predicate concatInsecureLdapString(CompileTimeConstantExpr protocol, Expr host) { + protocol.getStringValue() = "ldap://" and + not exists(string hostString | hostString = getHostname(host) | + hostString.length() = 0 or // Empty host is loopback address + hostString instanceof PrivateHostName + ) +} + +// Expr getLeftmostConcatOperand(Expr expr) { +// if expr instanceof AddExpr +// then +// result = expr.(AddExpr).getLeftOperand() and +// not result instanceof AddExpr +// else result = expr +// } +/** + * String concatenated with `InsecureLdapUrlLiteral`. + */ +class InsecureLdapUrl extends Expr { + InsecureLdapUrl() { + this instanceof InsecureLdapUrlLiteral + or + // protocol + host + ... + exists(AddExpr e, CompileTimeConstantExpr protocol, Expr rest, Expr host | + e = this and + protocol = e.getLeftOperand() and + rest = e.getRightOperand() and + if rest instanceof AddExpr then host = rest.(AddExpr).getLeftOperand() else host = rest + | + protocol.getStringValue() = "ldap://" and + concatInsecureLdapString(protocol, host) + ) + } +} + +/** + * Holds if `ma` writes the `java.naming.provider.url` (also known as `Context.PROVIDER_URL`) key of a `Hashtable`. + */ +predicate isProviderUrlSetter(MethodAccess ma) { + ma.getMethod().getDeclaringType().getAnAncestor() instanceof TypeHashtable and + ma.getMethod().hasName(["put", "setProperty"]) and + ( + ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "java.naming.provider.url" + or + exists(Field f | + ma.getArgument(0) = f.getAnAccess() and + f.hasName("PROVIDER_URL") and + f.getDeclaringType() instanceof TypeNamingContext + ) + ) +} + +/** + * Holds if `ma` sets `fieldValue` to `envValue` in some `Hashtable`. + */ +bindingset[fieldValue, envValue] +predicate hasFieldValueEnv(MethodAccess ma, string fieldValue, string envValue) { + // environment.put("java.naming.security.authentication", "simple") + ma.getMethod().getDeclaringType().getAnAncestor() instanceof TypeHashtable and + ma.getMethod().hasName(["put", "setProperty"]) and + ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = fieldValue and + ma.getArgument(1).(CompileTimeConstantExpr).getStringValue() = envValue +} + +/** + * Holds if `ma` sets attribute name `fieldName` to `envValue` in some `Hashtable`. + */ +bindingset[fieldName, envValue] +predicate hasFieldNameEnv(MethodAccess ma, string fieldName, string envValue) { + // environment.put(Context.SECURITY_AUTHENTICATION, "simple") + ma.getMethod().getDeclaringType().getAnAncestor() instanceof TypeHashtable and + ma.getMethod().hasName(["put", "setProperty"]) and + exists(Field f | + ma.getArgument(0) = f.getAnAccess() and + f.hasName(fieldName) and + f.getDeclaringType() instanceof TypeNamingContext + ) and + ma.getArgument(1).(CompileTimeConstantExpr).getStringValue() = envValue +} + +/** + * Holds if `ma` sets `java.naming.security.authentication` (also known as `Context.SECURITY_AUTHENTICATION`) to `simple` in some `Hashtable`. + */ +predicate isBasicAuthEnv(MethodAccess ma) { + hasFieldValueEnv(ma, "java.naming.security.authentication", "simple") or + hasFieldNameEnv(ma, "SECURITY_AUTHENTICATION", "simple") +} + +/** + * Holds if `ma` sets `java.naming.security.protocol` (also known as `Context.SECURITY_PROTOCOL`) to `ssl` in some `Hashtable`. + */ +predicate isSslEnv(MethodAccess ma) { + hasFieldValueEnv(ma, "java.naming.security.protocol", "ssl") or + hasFieldNameEnv(ma, "SECURITY_PROTOCOL", "ssl") +} diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll new file mode 100644 index 000000000000..7d9b81dc3d39 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll @@ -0,0 +1,78 @@ +/** Provides dataflow configurations to reason about insecure LDAP authentication. */ + +import java +import DataFlow +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.InsecureLdapAuth + +/** + * A taint-tracking configuration for `ldap://` URL in LDAP authentication. + */ +class InsecureUrlFlowConfig extends TaintTracking::Configuration { + InsecureUrlFlowConfig() { this = "InsecureLdapAuth:InsecureUrlFlowConfig" } + + /** Source of `ldap://` connection string. */ + override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl } + + /** Sink of directory context creation. */ + override predicate isSink(DataFlow::Node sink) { + exists(ConstructorCall cc | + cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and + sink.asExpr() = cc.getArgument(0) + ) + } + + /** Method call of `env.put()`. */ + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(MethodAccess ma | + pred.asExpr() = ma.getArgument(1) and + isProviderUrlSetter(ma) and + succ.asExpr() = ma.getQualifier() + ) + } +} + +/** + * A taint-tracking configuration for `simple` basic-authentication in LDAP configuration. + */ +class BasicAuthFlowConfig extends DataFlow::Configuration { + BasicAuthFlowConfig() { this = "InsecureLdapAuth:BasicAuthFlowConfig" } + + /** Source of `simple` configuration. */ + override predicate isSource(DataFlow::Node src) { + exists(MethodAccess ma | + isBasicAuthEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() + ) + } + + /** Sink of directory context creation. */ + override predicate isSink(DataFlow::Node sink) { + exists(ConstructorCall cc | + cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and + sink.asExpr() = cc.getArgument(0) + ) + } +} + +/** + * A taint-tracking configuration for `ssl` configuration in LDAP authentication. + */ +class SslFlowConfig extends DataFlow::Configuration { + SslFlowConfig() { this = "InsecureLdapAuth:SSLFlowConfig" } + + /** Source of `ssl` configuration. */ + override predicate isSource(DataFlow::Node src) { + exists(MethodAccess ma | + isSslEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() + ) + } + + /** Sink of directory context creation. */ + override predicate isSink(DataFlow::Node sink) { + exists(ConstructorCall cc | + cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and + sink.asExpr() = cc.getArgument(0) + ) + } +} diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index c235511fd445..be129f9521ca 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -12,206 +12,9 @@ */ import java -import DataFlow -import semmle.code.java.frameworks.Jndi -import semmle.code.java.frameworks.Networking -import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.InsecureLdapAuthQuery import DataFlow::PathGraph -/** - * Insecure (non-SSL, non-private) LDAP URL string literal. - */ -class InsecureLdapUrlLiteral extends StringLiteral { - InsecureLdapUrlLiteral() { - // Match connection strings with the LDAP protocol and without private IP addresses to reduce false positives. - exists(string s | this.getValue() = s | - s.regexpMatch("(?i)ldap://[\\[a-zA-Z0-9].*") and - not s.substring(7, s.length()) instanceof PrivateHostName - ) - } -} - -/** The class `java.util.Hashtable`. */ -class TypeHashtable extends Class { - TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") } -} - -string getHostname(Expr expr) { - result = expr.(CompileTimeConstantExpr).getStringValue() or - result = - expr.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getStringValue() -} - -/** - * Holds if a non-private LDAP string is concatenated from both protocol and host. - */ -predicate concatInsecureLdapString(CompileTimeConstantExpr protocol, Expr host) { - protocol.getStringValue() = "ldap://" and - not exists(string hostString | hostString = getHostname(host) | - hostString.length() = 0 or // Empty host is loopback address - hostString instanceof PrivateHostName - ) -} - -// Expr getLeftmostConcatOperand(Expr expr) { -// if expr instanceof AddExpr -// then -// result = expr.(AddExpr).getLeftOperand() and -// not result instanceof AddExpr -// else result = expr -// } -/** - * String concatenated with `InsecureLdapUrlLiteral`. - */ -class InsecureLdapUrl extends Expr { - InsecureLdapUrl() { - this instanceof InsecureLdapUrlLiteral - or - // protocol + host + ... - exists(AddExpr e, CompileTimeConstantExpr protocol, Expr rest, Expr host | - e = this and - protocol = e.getLeftOperand() and - rest = e.getRightOperand() and - if rest instanceof AddExpr then host = rest.(AddExpr).getLeftOperand() else host = rest - | - protocol.getStringValue() = "ldap://" and - concatInsecureLdapString(protocol, host) - ) - } -} - -/** - * Holds if `ma` writes the `java.naming.provider.url` (also known as `Context.PROVIDER_URL`) key of a `Hashtable`. - */ -predicate isProviderUrlSetter(MethodAccess ma) { - ma.getMethod().getDeclaringType().getAnAncestor() instanceof TypeHashtable and - ma.getMethod().hasName(["put", "setProperty"]) and - ( - ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "java.naming.provider.url" - or - exists(Field f | - ma.getArgument(0) = f.getAnAccess() and - f.hasName("PROVIDER_URL") and - f.getDeclaringType() instanceof TypeNamingContext - ) - ) -} - -/** - * Holds if `ma` sets `fieldValue` to `envValue` in some `Hashtable`. - */ -bindingset[fieldValue, envValue] -predicate hasFieldValueEnv(MethodAccess ma, string fieldValue, string envValue) { - // environment.put("java.naming.security.authentication", "simple") - ma.getMethod().getDeclaringType().getAnAncestor() instanceof TypeHashtable and - ma.getMethod().hasName(["put", "setProperty"]) and - ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = fieldValue and - ma.getArgument(1).(CompileTimeConstantExpr).getStringValue() = envValue -} - -/** - * Holds if `ma` sets attribute name `fieldName` to `envValue` in some `Hashtable`. - */ -bindingset[fieldName, envValue] -predicate hasFieldNameEnv(MethodAccess ma, string fieldName, string envValue) { - // environment.put(Context.SECURITY_AUTHENTICATION, "simple") - ma.getMethod().getDeclaringType().getAnAncestor() instanceof TypeHashtable and - ma.getMethod().hasName(["put", "setProperty"]) and - exists(Field f | - ma.getArgument(0) = f.getAnAccess() and - f.hasName(fieldName) and - f.getDeclaringType() instanceof TypeNamingContext - ) and - ma.getArgument(1).(CompileTimeConstantExpr).getStringValue() = envValue -} - -/** - * Holds if `ma` sets `java.naming.security.authentication` (also known as `Context.SECURITY_AUTHENTICATION`) to `simple` in some `Hashtable`. - */ -predicate isBasicAuthEnv(MethodAccess ma) { - hasFieldValueEnv(ma, "java.naming.security.authentication", "simple") or - hasFieldNameEnv(ma, "SECURITY_AUTHENTICATION", "simple") -} - -/** - * Holds if `ma` sets `java.naming.security.protocol` (also known as `Context.SECURITY_PROTOCOL`) to `ssl` in some `Hashtable`. - */ -predicate isSslEnv(MethodAccess ma) { - hasFieldValueEnv(ma, "java.naming.security.protocol", "ssl") or - hasFieldNameEnv(ma, "SECURITY_PROTOCOL", "ssl") -} - -/** - * A taint-tracking configuration for `ldap://` URL in LDAP authentication. - */ -class InsecureUrlFlowConfig extends TaintTracking::Configuration { - InsecureUrlFlowConfig() { this = "InsecureLdapAuth:InsecureUrlFlowConfig" } - - /** Source of `ldap://` connection string. */ - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl } - - /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { - exists(ConstructorCall cc | - cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and - sink.asExpr() = cc.getArgument(0) - ) - } - - /** Method call of `env.put()`. */ - override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { - exists(MethodAccess ma | - pred.asExpr() = ma.getArgument(1) and - isProviderUrlSetter(ma) and - succ.asExpr() = ma.getQualifier() - ) - } -} - -/** - * A taint-tracking configuration for `simple` basic-authentication in LDAP configuration. - */ -class BasicAuthFlowConfig extends DataFlow::Configuration { - BasicAuthFlowConfig() { this = "InsecureLdapAuth:BasicAuthFlowConfig" } - - /** Source of `simple` configuration. */ - override predicate isSource(DataFlow::Node src) { - exists(MethodAccess ma | - isBasicAuthEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() - ) - } - - /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { - exists(ConstructorCall cc | - cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and - sink.asExpr() = cc.getArgument(0) - ) - } -} - -/** - * A taint-tracking configuration for `ssl` configuration in LDAP authentication. - */ -class SslFlowConfig extends DataFlow::Configuration { - SslFlowConfig() { this = "InsecureLdapAuth:SSLFlowConfig" } - - /** Source of `ssl` configuration. */ - override predicate isSource(DataFlow::Node src) { - exists(MethodAccess ma | - isSslEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() - ) - } - - /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { - exists(ConstructorCall cc | - cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and - sink.asExpr() = cc.getArgument(0) - ) - } -} - from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureUrlFlowConfig config where config.hasFlowPath(source, sink) and From 98b445c6b756eda24f373ab08714ed47295c8a30 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 8 Mar 2023 20:53:59 -0500 Subject: [PATCH 06/23] Convert test to InlineExpectationsTest --- .../CWE-522/InsecureLdapAuth.expected | 111 ------------------ .../security/CWE-522/InsecureLdapAuth.java | 32 ++--- .../security/CWE-522/InsecureLdapAuth.qlref | 1 - .../CWE-522/InsecureLdapAuthTest.expected | 0 .../security/CWE-522/InsecureLdapAuthTest.ql | 20 ++++ 5 files changed, 36 insertions(+), 128 deletions(-) delete mode 100644 java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.expected delete mode 100644 java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.qlref create mode 100644 java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.expected create mode 100644 java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql diff --git a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.expected b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.expected deleted file mode 100644 index 4ae27bd27af9..000000000000 --- a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.expected +++ /dev/null @@ -1,111 +0,0 @@ -edges -| InsecureLdapAuth.java:11:20:11:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:15:41:15:47 | ldapUrl : String | -| InsecureLdapAuth.java:11:20:11:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:20:49:20:59 | environment | -| InsecureLdapAuth.java:15:3:15:13 | environment [post update] [] : String | InsecureLdapAuth.java:20:49:20:59 | environment | -| InsecureLdapAuth.java:15:41:15:47 | ldapUrl : String | InsecureLdapAuth.java:15:3:15:13 | environment [post update] [] : String | -| InsecureLdapAuth.java:17:3:17:13 | environment [post update] : Hashtable | InsecureLdapAuth.java:20:49:20:59 | environment | -| InsecureLdapAuth.java:25:20:25:39 | ... + ... : String | InsecureLdapAuth.java:29:41:29:47 | ldapUrl : String | -| InsecureLdapAuth.java:25:20:25:39 | ... + ... : String | InsecureLdapAuth.java:34:49:34:59 | environment | -| InsecureLdapAuth.java:29:3:29:13 | environment [post update] [] : String | InsecureLdapAuth.java:34:49:34:59 | environment | -| InsecureLdapAuth.java:29:41:29:47 | ldapUrl : String | InsecureLdapAuth.java:29:3:29:13 | environment [post update] [] : String | -| InsecureLdapAuth.java:31:3:31:13 | environment [post update] : Hashtable | InsecureLdapAuth.java:34:49:34:59 | environment | -| InsecureLdapAuth.java:45:3:45:13 | environment [post update] : Hashtable | InsecureLdapAuth.java:48:49:48:59 | environment | -| InsecureLdapAuth.java:53:20:53:50 | "ldap://ad.your-server.com:636" : String | InsecureLdapAuth.java:57:41:57:47 | ldapUrl : String | -| InsecureLdapAuth.java:53:20:53:50 | "ldap://ad.your-server.com:636" : String | InsecureLdapAuth.java:63:49:63:59 | environment | -| InsecureLdapAuth.java:57:3:57:13 | environment [post update] [] : String | InsecureLdapAuth.java:63:49:63:59 | environment | -| InsecureLdapAuth.java:57:41:57:47 | ldapUrl : String | InsecureLdapAuth.java:57:3:57:13 | environment [post update] [] : String | -| InsecureLdapAuth.java:59:3:59:13 | environment [post update] : Hashtable | InsecureLdapAuth.java:63:49:63:59 | environment | -| InsecureLdapAuth.java:62:3:62:13 | environment [post update] : Hashtable | InsecureLdapAuth.java:63:49:63:59 | environment | -| InsecureLdapAuth.java:68:20:68:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:72:41:72:47 | ldapUrl : String | -| InsecureLdapAuth.java:68:20:68:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:77:49:77:59 | environment | -| InsecureLdapAuth.java:72:3:72:13 | environment [post update] [] : String | InsecureLdapAuth.java:77:49:77:59 | environment | -| InsecureLdapAuth.java:72:41:72:47 | ldapUrl : String | InsecureLdapAuth.java:72:3:72:13 | environment [post update] [] : String | -| InsecureLdapAuth.java:88:3:88:13 | environment [post update] : Hashtable | InsecureLdapAuth.java:91:49:91:59 | environment | -| InsecureLdapAuth.java:96:20:96:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:100:41:100:47 | ldapUrl : String | -| InsecureLdapAuth.java:96:20:96:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:105:59:105:69 | environment | -| InsecureLdapAuth.java:100:3:100:13 | environment [post update] [] : String | InsecureLdapAuth.java:105:59:105:69 | environment | -| InsecureLdapAuth.java:100:41:100:47 | ldapUrl : String | InsecureLdapAuth.java:100:3:100:13 | environment [post update] [] : String | -| InsecureLdapAuth.java:102:3:102:13 | environment [post update] : Hashtable | InsecureLdapAuth.java:105:59:105:69 | environment | -| InsecureLdapAuth.java:111:20:111:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:115:47:115:53 | ldapUrl : String | -| InsecureLdapAuth.java:111:20:111:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:120:49:120:59 | environment | -| InsecureLdapAuth.java:115:3:115:13 | environment [post update] [] : String | InsecureLdapAuth.java:120:49:120:59 | environment | -| InsecureLdapAuth.java:115:47:115:53 | ldapUrl : String | InsecureLdapAuth.java:115:3:115:13 | environment [post update] [] : String | -| InsecureLdapAuth.java:117:3:117:13 | environment [post update] : Hashtable | InsecureLdapAuth.java:120:49:120:59 | environment | -| InsecureLdapAuth.java:124:3:124:5 | env [post update] : Hashtable | InsecureLdapAuth.java:137:10:137:20 | environment [post update] : Hashtable | -| InsecureLdapAuth.java:128:3:128:5 | env [post update] : Hashtable | InsecureLdapAuth.java:141:16:141:26 | environment [post update] : Hashtable | -| InsecureLdapAuth.java:128:3:128:5 | env [post update] : Hashtable | InsecureLdapAuth.java:152:16:152:26 | environment [post update] : Hashtable | -| InsecureLdapAuth.java:135:20:135:39 | ... + ... : String | InsecureLdapAuth.java:140:41:140:47 | ldapUrl : String | -| InsecureLdapAuth.java:135:20:135:39 | ... + ... : String | InsecureLdapAuth.java:142:50:142:60 | environment | -| InsecureLdapAuth.java:137:10:137:20 | environment [post update] : Hashtable | InsecureLdapAuth.java:142:50:142:60 | environment | -| InsecureLdapAuth.java:140:3:140:13 | environment [post update] [] : String | InsecureLdapAuth.java:142:50:142:60 | environment | -| InsecureLdapAuth.java:140:41:140:47 | ldapUrl : String | InsecureLdapAuth.java:140:3:140:13 | environment [post update] [] : String | -| InsecureLdapAuth.java:141:16:141:26 | environment [post update] : Hashtable | InsecureLdapAuth.java:142:50:142:60 | environment | -| InsecureLdapAuth.java:147:20:147:39 | ... + ... : String | InsecureLdapAuth.java:151:41:151:47 | ldapUrl : String | -| InsecureLdapAuth.java:147:20:147:39 | ... + ... : String | InsecureLdapAuth.java:153:50:153:60 | environment | -| InsecureLdapAuth.java:151:3:151:13 | environment [post update] [] : String | InsecureLdapAuth.java:153:50:153:60 | environment | -| InsecureLdapAuth.java:151:41:151:47 | ldapUrl : String | InsecureLdapAuth.java:151:3:151:13 | environment [post update] [] : String | -| InsecureLdapAuth.java:152:16:152:26 | environment [post update] : Hashtable | InsecureLdapAuth.java:153:50:153:60 | environment | -nodes -| InsecureLdapAuth.java:11:20:11:50 | "ldap://ad.your-server.com:389" : String | semmle.label | "ldap://ad.your-server.com:389" : String | -| InsecureLdapAuth.java:15:3:15:13 | environment [post update] [] : String | semmle.label | environment [post update] [] : String | -| InsecureLdapAuth.java:15:41:15:47 | ldapUrl : String | semmle.label | ldapUrl : String | -| InsecureLdapAuth.java:17:3:17:13 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:20:49:20:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:20:49:20:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:25:20:25:39 | ... + ... : String | semmle.label | ... + ... : String | -| InsecureLdapAuth.java:29:3:29:13 | environment [post update] [] : String | semmle.label | environment [post update] [] : String | -| InsecureLdapAuth.java:29:41:29:47 | ldapUrl : String | semmle.label | ldapUrl : String | -| InsecureLdapAuth.java:31:3:31:13 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:34:49:34:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:34:49:34:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:45:3:45:13 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:48:49:48:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:53:20:53:50 | "ldap://ad.your-server.com:636" : String | semmle.label | "ldap://ad.your-server.com:636" : String | -| InsecureLdapAuth.java:57:3:57:13 | environment [post update] [] : String | semmle.label | environment [post update] [] : String | -| InsecureLdapAuth.java:57:41:57:47 | ldapUrl : String | semmle.label | ldapUrl : String | -| InsecureLdapAuth.java:59:3:59:13 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:62:3:62:13 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:63:49:63:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:63:49:63:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:63:49:63:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:68:20:68:50 | "ldap://ad.your-server.com:389" : String | semmle.label | "ldap://ad.your-server.com:389" : String | -| InsecureLdapAuth.java:72:3:72:13 | environment [post update] [] : String | semmle.label | environment [post update] [] : String | -| InsecureLdapAuth.java:72:41:72:47 | ldapUrl : String | semmle.label | ldapUrl : String | -| InsecureLdapAuth.java:77:49:77:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:88:3:88:13 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:91:49:91:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:96:20:96:50 | "ldap://ad.your-server.com:389" : String | semmle.label | "ldap://ad.your-server.com:389" : String | -| InsecureLdapAuth.java:100:3:100:13 | environment [post update] [] : String | semmle.label | environment [post update] [] : String | -| InsecureLdapAuth.java:100:41:100:47 | ldapUrl : String | semmle.label | ldapUrl : String | -| InsecureLdapAuth.java:102:3:102:13 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:105:59:105:69 | environment | semmle.label | environment | -| InsecureLdapAuth.java:105:59:105:69 | environment | semmle.label | environment | -| InsecureLdapAuth.java:111:20:111:50 | "ldap://ad.your-server.com:389" : String | semmle.label | "ldap://ad.your-server.com:389" : String | -| InsecureLdapAuth.java:115:3:115:13 | environment [post update] [] : String | semmle.label | environment [post update] [] : String | -| InsecureLdapAuth.java:115:47:115:53 | ldapUrl : String | semmle.label | ldapUrl : String | -| InsecureLdapAuth.java:117:3:117:13 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:120:49:120:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:120:49:120:59 | environment | semmle.label | environment | -| InsecureLdapAuth.java:124:3:124:5 | env [post update] : Hashtable | semmle.label | env [post update] : Hashtable | -| InsecureLdapAuth.java:128:3:128:5 | env [post update] : Hashtable | semmle.label | env [post update] : Hashtable | -| InsecureLdapAuth.java:135:20:135:39 | ... + ... : String | semmle.label | ... + ... : String | -| InsecureLdapAuth.java:137:10:137:20 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:140:3:140:13 | environment [post update] [] : String | semmle.label | environment [post update] [] : String | -| InsecureLdapAuth.java:140:41:140:47 | ldapUrl : String | semmle.label | ldapUrl : String | -| InsecureLdapAuth.java:141:16:141:26 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:142:50:142:60 | environment | semmle.label | environment | -| InsecureLdapAuth.java:142:50:142:60 | environment | semmle.label | environment | -| InsecureLdapAuth.java:142:50:142:60 | environment | semmle.label | environment | -| InsecureLdapAuth.java:147:20:147:39 | ... + ... : String | semmle.label | ... + ... : String | -| InsecureLdapAuth.java:151:3:151:13 | environment [post update] [] : String | semmle.label | environment [post update] [] : String | -| InsecureLdapAuth.java:151:41:151:47 | ldapUrl : String | semmle.label | ldapUrl : String | -| InsecureLdapAuth.java:152:16:152:26 | environment [post update] : Hashtable | semmle.label | environment [post update] : Hashtable | -| InsecureLdapAuth.java:153:50:153:60 | environment | semmle.label | environment | -| InsecureLdapAuth.java:153:50:153:60 | environment | semmle.label | environment | -subpaths -#select -| InsecureLdapAuth.java:20:49:20:59 | environment | InsecureLdapAuth.java:11:20:11:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:20:49:20:59 | environment | Insecure LDAP authentication from $@. | InsecureLdapAuth.java:11:20:11:50 | "ldap://ad.your-server.com:389" | LDAP connection string | -| InsecureLdapAuth.java:34:49:34:59 | environment | InsecureLdapAuth.java:25:20:25:39 | ... + ... : String | InsecureLdapAuth.java:34:49:34:59 | environment | Insecure LDAP authentication from $@. | InsecureLdapAuth.java:25:20:25:39 | ... + ... | LDAP connection string | -| InsecureLdapAuth.java:105:59:105:69 | environment | InsecureLdapAuth.java:96:20:96:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:105:59:105:69 | environment | Insecure LDAP authentication from $@. | InsecureLdapAuth.java:96:20:96:50 | "ldap://ad.your-server.com:389" | LDAP connection string | -| InsecureLdapAuth.java:120:49:120:59 | environment | InsecureLdapAuth.java:111:20:111:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:120:49:120:59 | environment | Insecure LDAP authentication from $@. | InsecureLdapAuth.java:111:20:111:50 | "ldap://ad.your-server.com:389" | LDAP connection string | -| InsecureLdapAuth.java:153:50:153:60 | environment | InsecureLdapAuth.java:147:20:147:39 | ... + ... : String | InsecureLdapAuth.java:153:50:153:60 | environment | Insecure LDAP authentication from $@. | InsecureLdapAuth.java:147:20:147:39 | ... + ... | LDAP connection string | diff --git a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.java b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.java index 978a4df671bc..d769258a6119 100644 --- a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.java +++ b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.java @@ -11,13 +11,13 @@ public void testCleartextLdapAuth(String ldapUserName, String password) throws E String ldapUrl = "ldap://ad.your-server.com:389"; Hashtable environment = new Hashtable(); environment.put(Context.INITIAL_CONTEXT_FACTORY, - "com.sun.jndi.ldap.LdapCtxFactory"); + "com.sun.jndi.ldap.LdapCtxFactory"); environment.put(Context.PROVIDER_URL, ldapUrl); environment.put(Context.REFERRAL, "follow"); environment.put(Context.SECURITY_AUTHENTICATION, "simple"); environment.put(Context.SECURITY_PRINCIPAL, ldapUserName); environment.put(Context.SECURITY_CREDENTIALS, password); - DirContext dirContext = new InitialDirContext(environment); + DirContext dirContext = new InitialDirContext(environment); // $ hasInsecureLdapAuth } // BAD - Test LDAP authentication in cleartext using `DirContext`. @@ -25,13 +25,13 @@ public void testCleartextLdapAuth(String ldapUserName, String password, String s String ldapUrl = "ldap://"+serverName+":389"; Hashtable environment = new Hashtable(); environment.put(Context.INITIAL_CONTEXT_FACTORY, - "com.sun.jndi.ldap.LdapCtxFactory"); + "com.sun.jndi.ldap.LdapCtxFactory"); environment.put(Context.PROVIDER_URL, ldapUrl); environment.put(Context.REFERRAL, "follow"); environment.put(Context.SECURITY_AUTHENTICATION, "simple"); environment.put(Context.SECURITY_PRINCIPAL, ldapUserName); environment.put(Context.SECURITY_CREDENTIALS, password); - DirContext dirContext = new InitialDirContext(environment); + DirContext dirContext = new InitialDirContext(environment); // $ hasInsecureLdapAuth } // GOOD - Test LDAP authentication over SSL. @@ -39,7 +39,7 @@ public void testSslLdapAuth(String ldapUserName, String password) throws Excepti String ldapUrl = "ldaps://ad.your-server.com:636"; Hashtable environment = new Hashtable(); environment.put(Context.INITIAL_CONTEXT_FACTORY, - "com.sun.jndi.ldap.LdapCtxFactory"); + "com.sun.jndi.ldap.LdapCtxFactory"); environment.put(Context.PROVIDER_URL, ldapUrl); environment.put(Context.REFERRAL, "follow"); environment.put(Context.SECURITY_AUTHENTICATION, "simple"); @@ -53,7 +53,7 @@ public void testSslLdapAuth2(String ldapUserName, String password) throws Except String ldapUrl = "ldap://ad.your-server.com:636"; Hashtable environment = new Hashtable(); environment.put(Context.INITIAL_CONTEXT_FACTORY, - "com.sun.jndi.ldap.LdapCtxFactory"); + "com.sun.jndi.ldap.LdapCtxFactory"); environment.put(Context.PROVIDER_URL, ldapUrl); environment.put(Context.REFERRAL, "follow"); environment.put(Context.SECURITY_AUTHENTICATION, "simple"); @@ -68,7 +68,7 @@ public void testSaslLdapAuth(String ldapUserName, String password) throws Except String ldapUrl = "ldap://ad.your-server.com:389"; Hashtable environment = new Hashtable(); environment.put(Context.INITIAL_CONTEXT_FACTORY, - "com.sun.jndi.ldap.LdapCtxFactory"); + "com.sun.jndi.ldap.LdapCtxFactory"); environment.put(Context.PROVIDER_URL, ldapUrl); environment.put(Context.REFERRAL, "follow"); environment.put(Context.SECURITY_AUTHENTICATION, "DIGEST-MD5 GSSAPI"); @@ -82,7 +82,7 @@ public void testCleartextLdapAuth2(String ldapUserName, String password) throws String ldapUrl = "ldap://localhost:389"; Hashtable environment = new Hashtable(); environment.put(Context.INITIAL_CONTEXT_FACTORY, - "com.sun.jndi.ldap.LdapCtxFactory"); + "com.sun.jndi.ldap.LdapCtxFactory"); environment.put(Context.PROVIDER_URL, ldapUrl); environment.put(Context.REFERRAL, "follow"); environment.put(Context.SECURITY_AUTHENTICATION, "simple"); @@ -96,14 +96,14 @@ public void testCleartextLdapAuth3(String ldapUserName, String password) throws String ldapUrl = "ldap://ad.your-server.com:389"; Hashtable environment = new Hashtable(); environment.put(Context.INITIAL_CONTEXT_FACTORY, - "com.sun.jndi.ldap.LdapCtxFactory"); + "com.sun.jndi.ldap.LdapCtxFactory"); environment.put(Context.PROVIDER_URL, ldapUrl); environment.put(Context.REFERRAL, "follow"); environment.put(Context.SECURITY_AUTHENTICATION, "simple"); environment.put(Context.SECURITY_PRINCIPAL, ldapUserName); environment.put(Context.SECURITY_CREDENTIALS, password); - InitialLdapContext ldapContext = new InitialLdapContext(environment, null); - } + InitialLdapContext ldapContext = new InitialLdapContext(environment, null); // $ hasInsecureLdapAuth + } // BAD - Test LDAP authentication in cleartext using `DirContext` and string literals. @@ -111,13 +111,13 @@ public void testCleartextLdapAuth4(String ldapUserName, String password) throws String ldapUrl = "ldap://ad.your-server.com:389"; Hashtable environment = new Hashtable(); environment.put("java.naming.factory.initial", - "com.sun.jndi.ldap.LdapCtxFactory"); + "com.sun.jndi.ldap.LdapCtxFactory"); environment.put("java.naming.provider.url", ldapUrl); environment.put("java.naming.referral", "follow"); environment.put("java.naming.security.authentication", "simple"); environment.put("java.naming.security.principal", ldapUserName); environment.put("java.naming.security.credentials", password); - DirContext dirContext = new InitialDirContext(environment); + DirContext dirContext = new InitialDirContext(environment); // $ hasInsecureLdapAuth } private void setSSL(Hashtable env) { @@ -136,7 +136,7 @@ public void testCleartextLdapAuth5(String ldapUserName, String password, String Hashtable environment = new Hashtable(); setSSL(environment); environment.put(Context.INITIAL_CONTEXT_FACTORY, - "com.sun.jndi.ldap.LdapCtxFactory"); + "com.sun.jndi.ldap.LdapCtxFactory"); environment.put(Context.PROVIDER_URL, ldapUrl); setBasicAuth(environment, ldapUserName, password); DirContext dirContext = new InitialLdapContext(environment, null); @@ -147,9 +147,9 @@ public void testCleartextLdapAuth6(String ldapUserName, String password, String String ldapUrl = "ldap://"+serverName+":389"; Hashtable environment = new Hashtable(); environment.put(Context.INITIAL_CONTEXT_FACTORY, - "com.sun.jndi.ldap.LdapCtxFactory"); + "com.sun.jndi.ldap.LdapCtxFactory"); environment.put(Context.PROVIDER_URL, ldapUrl); setBasicAuth(environment, ldapUserName, password); - DirContext dirContext = new InitialLdapContext(environment, null); + DirContext dirContext = new InitialLdapContext(environment, null); // $ hasInsecureLdapAuth } } diff --git a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.qlref b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.qlref deleted file mode 100644 index 952f1d5663ce..000000000000 --- a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuth.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE/CWE-522/InsecureLdapAuth.ql diff --git a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.expected b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql new file mode 100644 index 000000000000..c0779029591c --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql @@ -0,0 +1,20 @@ +import java +import semmle.code.java.security.InsecureLdapAuthQuery +import TestUtilities.InlineExpectationsTest + +class InsecureLdapAuthenticationTest extends InlineExpectationsTest { + InsecureLdapAuthenticationTest() { this = "InsecureLdapAuthentication" } + + override string getARelevantTag() { result = "hasInsecureLdapAuth" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasInsecureLdapAuth" and + exists(DataFlow::Node sink, InsecureUrlFlowConfig conf | conf.hasFlowTo(sink) | + any(BasicAuthFlowConfig bc).hasFlowTo(sink) and + not any(SslFlowConfig sc).hasFlowTo(sink) and + sink.getLocation() = location and + element = sink.toString() and + value = "" + ) + } +} From 05da1dc4a37ce91cc2df60d050cfe2c8a76889a1 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 8 Mar 2023 21:08:02 -0500 Subject: [PATCH 07/23] Merge concatInsecureLdapString into InsecureLdapUrl constructor --- .../code/java/security/InsecureLdapAuth.qll | 27 +++++-------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll index d5948f2ff048..68cafa845d2d 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll @@ -22,30 +22,13 @@ class TypeHashtable extends Class { TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") } } -string getHostname(Expr expr) { +/** Get the string value of an expression representing a hostname. */ +private string getHostname(Expr expr) { result = expr.(CompileTimeConstantExpr).getStringValue() or result = expr.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getStringValue() } -/** - * Holds if a non-private LDAP string is concatenated from both protocol and host. - */ -predicate concatInsecureLdapString(CompileTimeConstantExpr protocol, Expr host) { - protocol.getStringValue() = "ldap://" and - not exists(string hostString | hostString = getHostname(host) | - hostString.length() = 0 or // Empty host is loopback address - hostString instanceof PrivateHostName - ) -} - -// Expr getLeftmostConcatOperand(Expr expr) { -// if expr instanceof AddExpr -// then -// result = expr.(AddExpr).getLeftOperand() and -// not result instanceof AddExpr -// else result = expr -// } /** * String concatenated with `InsecureLdapUrlLiteral`. */ @@ -53,6 +36,7 @@ class InsecureLdapUrl extends Expr { InsecureLdapUrl() { this instanceof InsecureLdapUrlLiteral or + // Concatentation of insecure protcol and non-private host: // protocol + host + ... exists(AddExpr e, CompileTimeConstantExpr protocol, Expr rest, Expr host | e = this and @@ -61,7 +45,10 @@ class InsecureLdapUrl extends Expr { if rest instanceof AddExpr then host = rest.(AddExpr).getLeftOperand() else host = rest | protocol.getStringValue() = "ldap://" and - concatInsecureLdapString(protocol, host) + not exists(string hostString | hostString = getHostname(host) | + hostString.length() = 0 or // Empty host is loopback address + hostString instanceof PrivateHostName + ) ) } } From 6a0167fa7fadedfd6530d56ff908e6e187a47bc4 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 8 Mar 2023 21:50:09 -0500 Subject: [PATCH 08/23] Convert to using the new DataFlow modules --- .../java/security/InsecureLdapAuthQuery.qll | 32 +++++++++---------- .../Security/CWE/CWE-522/InsecureLdapAuth.ql | 8 ++--- .../security/CWE-522/InsecureLdapAuthTest.ql | 6 ++-- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll index 7d9b81dc3d39..caa451605eab 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll @@ -9,14 +9,12 @@ import semmle.code.java.security.InsecureLdapAuth /** * A taint-tracking configuration for `ldap://` URL in LDAP authentication. */ -class InsecureUrlFlowConfig extends TaintTracking::Configuration { - InsecureUrlFlowConfig() { this = "InsecureLdapAuth:InsecureUrlFlowConfig" } - +private module InsecureUrlFlowConfig implements DataFlow::ConfigSig { /** Source of `ldap://` connection string. */ - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl } + predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl } /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { exists(ConstructorCall cc | cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and sink.asExpr() = cc.getArgument(0) @@ -24,7 +22,7 @@ class InsecureUrlFlowConfig extends TaintTracking::Configuration { } /** Method call of `env.put()`. */ - override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) { exists(MethodAccess ma | pred.asExpr() = ma.getArgument(1) and isProviderUrlSetter(ma) and @@ -33,21 +31,21 @@ class InsecureUrlFlowConfig extends TaintTracking::Configuration { } } +module InsecureUrlFlowConfiguration = TaintTracking::Make; + /** * A taint-tracking configuration for `simple` basic-authentication in LDAP configuration. */ -class BasicAuthFlowConfig extends DataFlow::Configuration { - BasicAuthFlowConfig() { this = "InsecureLdapAuth:BasicAuthFlowConfig" } - +private module BasicAuthFlowConfig implements DataFlow::ConfigSig { /** Source of `simple` configuration. */ - override predicate isSource(DataFlow::Node src) { + predicate isSource(DataFlow::Node src) { exists(MethodAccess ma | isBasicAuthEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() ) } /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { exists(ConstructorCall cc | cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and sink.asExpr() = cc.getArgument(0) @@ -55,24 +53,26 @@ class BasicAuthFlowConfig extends DataFlow::Configuration { } } +module BasicAuthFlowConfiguration = DataFlow::Make; + /** * A taint-tracking configuration for `ssl` configuration in LDAP authentication. */ -class SslFlowConfig extends DataFlow::Configuration { - SslFlowConfig() { this = "InsecureLdapAuth:SSLFlowConfig" } - +private module SslFlowConfig implements DataFlow::ConfigSig { /** Source of `ssl` configuration. */ - override predicate isSource(DataFlow::Node src) { + predicate isSource(DataFlow::Node src) { exists(MethodAccess ma | isSslEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() ) } /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { exists(ConstructorCall cc | cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and sink.asExpr() = cc.getArgument(0) ) } } + +module SslFlowConfiguration = DataFlow::Make; diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index be129f9521ca..129262567546 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -15,10 +15,10 @@ import java import semmle.code.java.security.InsecureLdapAuthQuery import DataFlow::PathGraph -from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureUrlFlowConfig config +from InsecureUrlFlowConfiguration::PathNode source, InsecureUrlFlowConfiguration::PathNode sink where - config.hasFlowPath(source, sink) and - any(BasicAuthFlowConfig bc).hasFlowTo(sink.getNode()) and - not any(SslFlowConfig sc).hasFlowTo(sink.getNode()) + InsecureUrlFlowConfiguration::hasFlowPath(source, sink) and + BasicAuthFlowConfiguration::hasFlowTo(sink.getNode()) and + not SslFlowConfiguration::hasFlowTo(sink.getNode()) select sink.getNode(), source, sink, "Insecure LDAP authentication from $@.", source.getNode(), "LDAP connection string" diff --git a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql index c0779029591c..67660872c39e 100644 --- a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql +++ b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql @@ -9,9 +9,9 @@ class InsecureLdapAuthenticationTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasInsecureLdapAuth" and - exists(DataFlow::Node sink, InsecureUrlFlowConfig conf | conf.hasFlowTo(sink) | - any(BasicAuthFlowConfig bc).hasFlowTo(sink) and - not any(SslFlowConfig sc).hasFlowTo(sink) and + exists(DataFlow::Node sink | InsecureUrlFlowConfiguration::hasFlowTo(sink) | + BasicAuthFlowConfiguration::hasFlowTo(sink) and + not SslFlowConfiguration::hasFlowTo(sink) and sink.getLocation() = location and element = sink.toString() and value = "" From db60c08de74ca0b0a3939248e5507dcd84fe7848 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 8 Mar 2023 22:04:02 -0500 Subject: [PATCH 09/23] Add security severity --- java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index 129262567546..2bf227e0feab 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -3,6 +3,7 @@ * @description LDAP authentication with credentials sent in cleartext. * @kind path-problem * @problem.severity warning + * @security-severity 8.8 * @precision medium * @id java/insecure-ldap-auth * @tags security From 0f4709e76944e1557f70d924ac68346e95b1da7f Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 8 Mar 2023 22:03:34 -0500 Subject: [PATCH 10/23] Add change note --- .../change-notes/2023-03-08-insecure-ldap-authentication.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/src/change-notes/2023-03-08-insecure-ldap-authentication.md diff --git a/java/ql/src/change-notes/2023-03-08-insecure-ldap-authentication.md b/java/ql/src/change-notes/2023-03-08-insecure-ldap-authentication.md new file mode 100644 index 000000000000..160d5639b22d --- /dev/null +++ b/java/ql/src/change-notes/2023-03-08-insecure-ldap-authentication.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* The query `java/insecure-ldap-auth` has been promoted from experimental to the main query pack. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/4854) \ No newline at end of file From 59ce0d76821103f93cf8b87808f9e9fc9d9f013d Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 8 Mar 2023 22:16:44 -0500 Subject: [PATCH 11/23] Documentation changes --- java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll | 4 ++-- .../lib/semmle/code/java/security/InsecureLdapAuthQuery.qll | 6 ------ 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll index 68cafa845d2d..1c9cab6f24d7 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll @@ -5,7 +5,7 @@ import semmle.code.java.frameworks.Networking import semmle.code.java.frameworks.Jndi /** - * Insecure (non-SSL, non-private) LDAP URL string literal. + * An insecure (non-SSL, non-private) LDAP URL string literal. */ class InsecureLdapUrlLiteral extends StringLiteral { InsecureLdapUrlLiteral() { @@ -30,7 +30,7 @@ private string getHostname(Expr expr) { } /** - * String concatenated with `InsecureLdapUrlLiteral`. + * An expression that represents an insecure (non-SSL, non-private) LDAP URL. */ class InsecureLdapUrl extends Expr { InsecureLdapUrl() { diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll index caa451605eab..04d452be4235 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll @@ -10,10 +10,8 @@ import semmle.code.java.security.InsecureLdapAuth * A taint-tracking configuration for `ldap://` URL in LDAP authentication. */ private module InsecureUrlFlowConfig implements DataFlow::ConfigSig { - /** Source of `ldap://` connection string. */ predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl } - /** Sink of directory context creation. */ predicate isSink(DataFlow::Node sink) { exists(ConstructorCall cc | cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and @@ -37,14 +35,12 @@ module InsecureUrlFlowConfiguration = TaintTracking::Make * A taint-tracking configuration for `simple` basic-authentication in LDAP configuration. */ private module BasicAuthFlowConfig implements DataFlow::ConfigSig { - /** Source of `simple` configuration. */ predicate isSource(DataFlow::Node src) { exists(MethodAccess ma | isBasicAuthEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() ) } - /** Sink of directory context creation. */ predicate isSink(DataFlow::Node sink) { exists(ConstructorCall cc | cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and @@ -59,14 +55,12 @@ module BasicAuthFlowConfiguration = DataFlow::Make; * A taint-tracking configuration for `ssl` configuration in LDAP authentication. */ private module SslFlowConfig implements DataFlow::ConfigSig { - /** Source of `ssl` configuration. */ predicate isSource(DataFlow::Node src) { exists(MethodAccess ma | isSslEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() ) } - /** Sink of directory context creation. */ predicate isSink(DataFlow::Node sink) { exists(ConstructorCall cc | cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and From efdfc2d0c303edd1e2b350a2ec5ebb1d562e2c06 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Thu, 9 Mar 2023 11:00:54 -0500 Subject: [PATCH 12/23] Change version of PathNode used to appropriate module --- java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index 2bf227e0feab..98c4332bcfe1 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -14,7 +14,7 @@ import java import semmle.code.java.security.InsecureLdapAuthQuery -import DataFlow::PathGraph +import InsecureLdapAuthQuery::PathGraph from InsecureUrlFlowConfiguration::PathNode source, InsecureUrlFlowConfiguration::PathNode sink where From 752620a34d1f3a2e7e889ba70158981e32f125fd Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Thu, 9 Mar 2023 17:15:21 -0500 Subject: [PATCH 13/23] Rename SSL configuration and fix PathGraph --- .../lib/semmle/code/java/security/InsecureLdapAuthQuery.qll | 4 ++-- java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll index 04d452be4235..b8acf539bedb 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll @@ -54,7 +54,7 @@ module BasicAuthFlowConfiguration = DataFlow::Make; /** * A taint-tracking configuration for `ssl` configuration in LDAP authentication. */ -private module SslFlowConfig implements DataFlow::ConfigSig { +private module RequiresSslConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { exists(MethodAccess ma | isSslEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() @@ -69,4 +69,4 @@ private module SslFlowConfig implements DataFlow::ConfigSig { } } -module SslFlowConfiguration = DataFlow::Make; +module RequiresSslConfiguration = DataFlow::Make; diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index 98c4332bcfe1..1abfdd69fec8 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -14,12 +14,12 @@ import java import semmle.code.java.security.InsecureLdapAuthQuery -import InsecureLdapAuthQuery::PathGraph +import InsecureUrlFlowConfiguration::PathGraph from InsecureUrlFlowConfiguration::PathNode source, InsecureUrlFlowConfiguration::PathNode sink where InsecureUrlFlowConfiguration::hasFlowPath(source, sink) and BasicAuthFlowConfiguration::hasFlowTo(sink.getNode()) and - not SslFlowConfiguration::hasFlowTo(sink.getNode()) + not RequiresSslConfiguration::hasFlowTo(sink.getNode()) select sink.getNode(), source, sink, "Insecure LDAP authentication from $@.", source.getNode(), "LDAP connection string" From cb58936c083d1a5943fa2fd68fd3a1d8ef6c6b77 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 10 Mar 2023 14:48:41 -0500 Subject: [PATCH 14/23] Documentation changes --- .../CWE/CWE-522/InsecureLdapAuth.java | 24 -------------- .../CWE/CWE-522/InsecureLdapAuth.qhelp | 32 ++++++++++++++++--- .../Security/CWE/CWE-522/LdapAuthUseLdap.java | 9 ++++++ .../CWE/CWE-522/LdapAuthUseLdaps.java | 9 ++++++ .../Security/CWE/CWE-522/LdapEnableSasl.java | 9 ++++++ 5 files changed, 55 insertions(+), 28 deletions(-) delete mode 100644 java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.java create mode 100644 java/ql/src/Security/CWE/CWE-522/LdapAuthUseLdap.java create mode 100644 java/ql/src/Security/CWE/CWE-522/LdapAuthUseLdaps.java create mode 100644 java/ql/src/Security/CWE/CWE-522/LdapEnableSasl.java diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.java b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.java deleted file mode 100644 index 3c5f65551004..000000000000 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.java +++ /dev/null @@ -1,24 +0,0 @@ -public class InsecureLdapAuth { - /** LDAP authentication */ - public DirContext ldapAuth(String ldapUserName, String password) { - { - // BAD: LDAP authentication in cleartext - String ldapUrl = "ldap://ad.your-server.com:389"; - } - - { - // GOOD: LDAPS authentication over SSL - String ldapUrl = "ldaps://ad.your-server.com:636"; - } - - Hashtable environment = new Hashtable(); - environment.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory"); - environment.put(Context.PROVIDER_URL, ldapUrl); - environment.put(Context.REFERRAL, "follow"); - environment.put(Context.SECURITY_AUTHENTICATION, "simple"); - environment.put(Context.SECURITY_PRINCIPAL, ldapUserName); - environment.put(Context.SECURITY_CREDENTIALS, password); - DirContext dirContext = new InitialDirContext(environment); - return dirContext; - } -} diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp index c729759a06e5..9483e8d37059 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp @@ -2,16 +2,40 @@ -

When using the Java LDAP API to perform LDAPv3-style extended operations and controls, a context with connection properties including user credentials is started. Transmission of LDAP credentials in cleartext allows remote attackers to obtain sensitive information by sniffing the network.

+

+ When using the Java LDAP API to perform LDAPv3-style extended operations + and controls, a context with connection properties including user + credentials is started. Transmission of LDAP credentials in cleartext + allows remote attackers to obtain sensitive information by sniffing the + network. +

-

Use LDAPS to send credentials through SSL or use SASL authentication.

+

+ Use the ldaps:// protocol to send credentials through SSL or + use SASL authentication. +

-

The following example shows two ways of using LDAP authentication. In the 'BAD' case, the credentials are transmitted in cleartext. In the 'GOOD' case, the credentials are transmitted over SSL.

- +

+ In the following (bad) example, a ldap:// URL is used and + credentials will be sent in plaintext. +

+ + +

+ In the following (good) example, a ldaps:// URL is used so + credentials will be encrypted with SSL. +

+ + +

+ In the following (good) example, a ldap:// URL is used, but + SASL authentication is enabled. +

+ diff --git a/java/ql/src/Security/CWE/CWE-522/LdapAuthUseLdap.java b/java/ql/src/Security/CWE/CWE-522/LdapAuthUseLdap.java new file mode 100644 index 000000000000..36826bf27b0d --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-522/LdapAuthUseLdap.java @@ -0,0 +1,9 @@ +String ldapUrl = "ldap://ad.your-server.com:389"; +Hashtable environment = new Hashtable(); +environment.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory"); +environment.put(Context.PROVIDER_URL, ldapUrl); +environment.put(Context.REFERRAL, "follow"); +environment.put(Context.SECURITY_AUTHENTICATION, "simple"); +environment.put(Context.SECURITY_PRINCIPAL, ldapUserName); +environment.put(Context.SECURITY_CREDENTIALS, password); +DirContext dirContext = new InitialDirContext(environment); diff --git a/java/ql/src/Security/CWE/CWE-522/LdapAuthUseLdaps.java b/java/ql/src/Security/CWE/CWE-522/LdapAuthUseLdaps.java new file mode 100644 index 000000000000..e7d8bdefc600 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-522/LdapAuthUseLdaps.java @@ -0,0 +1,9 @@ +String ldapUrl = "ldaps://ad.your-server.com:636"; +Hashtable environment = new Hashtable(); +environment.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory"); +environment.put(Context.PROVIDER_URL, ldapUrl); +environment.put(Context.REFERRAL, "follow"); +environment.put(Context.SECURITY_AUTHENTICATION, "simple"); +environment.put(Context.SECURITY_PRINCIPAL, ldapUserName); +environment.put(Context.SECURITY_CREDENTIALS, password); +DirContext dirContext = new InitialDirContext(environment); diff --git a/java/ql/src/Security/CWE/CWE-522/LdapEnableSasl.java b/java/ql/src/Security/CWE/CWE-522/LdapEnableSasl.java new file mode 100644 index 000000000000..2e191c62918c --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-522/LdapEnableSasl.java @@ -0,0 +1,9 @@ +String ldapUrl = "ldap://ad.your-server.com:389"; +Hashtable environment = new Hashtable(); +environment.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory"); +environment.put(Context.PROVIDER_URL, ldapUrl); +environment.put(Context.REFERRAL, "follow"); +environment.put(Context.SECURITY_AUTHENTICATION, "DIGEST-MD5 GSSAPI"); +environment.put(Context.SECURITY_PRINCIPAL, ldapUserName); +environment.put(Context.SECURITY_CREDENTIALS, password); +DirContext dirContext = new InitialDirContext(environment); From 658c54a18f2edf892b9fbcb549d9bdf517c9c4b8 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 10 Mar 2023 15:05:07 -0500 Subject: [PATCH 15/23] Change names of configuration to fit new naming convention --- .../code/java/security/InsecureLdapAuthQuery.qll | 10 +++++----- java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql | 10 +++++----- .../security/CWE-522/InsecureLdapAuthTest.ql | 6 +++--- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll index b8acf539bedb..548f3fafd0ac 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll @@ -9,7 +9,7 @@ import semmle.code.java.security.InsecureLdapAuth /** * A taint-tracking configuration for `ldap://` URL in LDAP authentication. */ -private module InsecureUrlFlowConfig implements DataFlow::ConfigSig { +private module InsecureLdapUrlConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl } predicate isSink(DataFlow::Node sink) { @@ -29,12 +29,12 @@ private module InsecureUrlFlowConfig implements DataFlow::ConfigSig { } } -module InsecureUrlFlowConfiguration = TaintTracking::Make; +module InsecureLdapUrlFlow = TaintTracking::Make; /** * A taint-tracking configuration for `simple` basic-authentication in LDAP configuration. */ -private module BasicAuthFlowConfig implements DataFlow::ConfigSig { +private module BasicAuthConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { exists(MethodAccess ma | isBasicAuthEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() @@ -49,7 +49,7 @@ private module BasicAuthFlowConfig implements DataFlow::ConfigSig { } } -module BasicAuthFlowConfiguration = DataFlow::Make; +module BasicAuthFlow = DataFlow::Make; /** * A taint-tracking configuration for `ssl` configuration in LDAP authentication. @@ -69,4 +69,4 @@ private module RequiresSslConfig implements DataFlow::ConfigSig { } } -module RequiresSslConfiguration = DataFlow::Make; +module RequiresSslFlow = DataFlow::Make; diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index 1abfdd69fec8..635026222845 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -14,12 +14,12 @@ import java import semmle.code.java.security.InsecureLdapAuthQuery -import InsecureUrlFlowConfiguration::PathGraph +import InsecureLdapUrlFlow::PathGraph -from InsecureUrlFlowConfiguration::PathNode source, InsecureUrlFlowConfiguration::PathNode sink +from InsecureLdapUrlFlow::PathNode source, InsecureLdapUrlFlow::PathNode sink where - InsecureUrlFlowConfiguration::hasFlowPath(source, sink) and - BasicAuthFlowConfiguration::hasFlowTo(sink.getNode()) and - not RequiresSslConfiguration::hasFlowTo(sink.getNode()) + InsecureLdapUrlFlow::hasFlowPath(source, sink) and + BasicAuthFlow::hasFlowTo(sink.getNode()) and + not RequiresSslFlow::hasFlowTo(sink.getNode()) select sink.getNode(), source, sink, "Insecure LDAP authentication from $@.", source.getNode(), "LDAP connection string" diff --git a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql index 67660872c39e..1d8bfbea58f9 100644 --- a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql +++ b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql @@ -9,9 +9,9 @@ class InsecureLdapAuthenticationTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasInsecureLdapAuth" and - exists(DataFlow::Node sink | InsecureUrlFlowConfiguration::hasFlowTo(sink) | - BasicAuthFlowConfiguration::hasFlowTo(sink) and - not SslFlowConfiguration::hasFlowTo(sink) and + exists(DataFlow::Node sink | InsecureLdapUrlFlow::hasFlowTo(sink) | + BasicAuthFlow::hasFlowTo(sink) and + not RequiresSslFlow::hasFlowTo(sink) and sink.getLocation() = location and element = sink.toString() and value = "" From 151357d02d2d2451803ff41424907c03bf73b8f1 Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Fri, 17 Mar 2023 10:25:46 -0400 Subject: [PATCH 16/23] Make classes/predicates not used outside of query private Co-authored-by: Tony Torralba --- .../lib/semmle/code/java/security/InsecureLdapAuth.qll | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll index 1c9cab6f24d7..e90bfd4cf4fb 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll @@ -1,13 +1,13 @@ /** Provides classes to reason about insecure LDAP authentication. */ import java -import semmle.code.java.frameworks.Networking -import semmle.code.java.frameworks.Jndi +private import semmle.code.java.frameworks.Networking +private import semmle.code.java.frameworks.Jndi /** * An insecure (non-SSL, non-private) LDAP URL string literal. */ -class InsecureLdapUrlLiteral extends StringLiteral { +private class InsecureLdapUrlLiteral extends StringLiteral { InsecureLdapUrlLiteral() { // Match connection strings with the LDAP protocol and without private IP addresses to reduce false positives. exists(string s | this.getValue() = s | @@ -18,7 +18,7 @@ class InsecureLdapUrlLiteral extends StringLiteral { } /** The class `java.util.Hashtable`. */ -class TypeHashtable extends Class { +private class TypeHashtable extends Class { TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") } } @@ -86,7 +86,7 @@ predicate hasFieldValueEnv(MethodAccess ma, string fieldValue, string envValue) * Holds if `ma` sets attribute name `fieldName` to `envValue` in some `Hashtable`. */ bindingset[fieldName, envValue] -predicate hasFieldNameEnv(MethodAccess ma, string fieldName, string envValue) { +private predicate hasFieldNameEnv(MethodAccess ma, string fieldName, string envValue) { // environment.put(Context.SECURITY_AUTHENTICATION, "simple") ma.getMethod().getDeclaringType().getAnAncestor() instanceof TypeHashtable and ma.getMethod().hasName(["put", "setProperty"]) and From 24d48591491ba845a6a3792c1392febd220a0b78 Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Fri, 17 Mar 2023 10:26:32 -0400 Subject: [PATCH 17/23] Import changes Co-authored-by: Tony Torralba --- .../lib/semmle/code/java/security/InsecureLdapAuthQuery.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll index 548f3fafd0ac..dcc5529759d1 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll @@ -1,9 +1,9 @@ /** Provides dataflow configurations to reason about insecure LDAP authentication. */ import java -import DataFlow import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.frameworks.Jndi import semmle.code.java.security.InsecureLdapAuth /** @@ -37,7 +37,7 @@ module InsecureLdapUrlFlow = TaintTracking::Make; private module BasicAuthConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { exists(MethodAccess ma | - isBasicAuthEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() + isBasicAuthEnv(ma) and ma.getQualifier() = src.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() ) } @@ -57,7 +57,7 @@ module BasicAuthFlow = DataFlow::Make; private module RequiresSslConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { exists(MethodAccess ma | - isSslEnv(ma) and ma.getQualifier() = src.(PostUpdateNode).getPreUpdateNode().asExpr() + isSslEnv(ma) and ma.getQualifier() = src.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() ) } From f28f1af5a4e0c856707220aa3d58a154cb322a74 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 17 Mar 2023 11:36:12 -0400 Subject: [PATCH 18/23] Add `InsecureLdapUrlSink` --- .../code/java/security/InsecureLdapAuth.qll | 10 +++++++ .../java/security/InsecureLdapAuthQuery.qll | 27 +++++-------------- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll index e90bfd4cf4fb..42b56a11b67b 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll @@ -1,6 +1,7 @@ /** Provides classes to reason about insecure LDAP authentication. */ import java +private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.frameworks.Networking private import semmle.code.java.frameworks.Jndi @@ -113,3 +114,12 @@ predicate isSslEnv(MethodAccess ma) { hasFieldValueEnv(ma, "java.naming.security.protocol", "ssl") or hasFieldNameEnv(ma, "SECURITY_PROTOCOL", "ssl") } + +class InsecureLdapUrlSink extends DataFlow::Node { + InsecureLdapUrlSink() { + exists(ConstructorCall cc | + cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and + this.asExpr() = cc.getArgument(0) + ) + } +} diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll index dcc5529759d1..150d9c724580 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll @@ -12,12 +12,7 @@ import semmle.code.java.security.InsecureLdapAuth private module InsecureLdapUrlConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl } - predicate isSink(DataFlow::Node sink) { - exists(ConstructorCall cc | - cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and - sink.asExpr() = cc.getArgument(0) - ) - } + predicate isSink(DataFlow::Node sink) { sink instanceof InsecureLdapUrlSink } /** Method call of `env.put()`. */ predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) { @@ -37,16 +32,12 @@ module InsecureLdapUrlFlow = TaintTracking::Make; private module BasicAuthConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { exists(MethodAccess ma | - isBasicAuthEnv(ma) and ma.getQualifier() = src.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() + isBasicAuthEnv(ma) and + ma.getQualifier() = src.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() ) } - predicate isSink(DataFlow::Node sink) { - exists(ConstructorCall cc | - cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and - sink.asExpr() = cc.getArgument(0) - ) - } + predicate isSink(DataFlow::Node sink) { sink instanceof InsecureLdapUrlSink } } module BasicAuthFlow = DataFlow::Make; @@ -57,16 +48,12 @@ module BasicAuthFlow = DataFlow::Make; private module RequiresSslConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { exists(MethodAccess ma | - isSslEnv(ma) and ma.getQualifier() = src.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() + isSslEnv(ma) and + ma.getQualifier() = src.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() ) } - predicate isSink(DataFlow::Node sink) { - exists(ConstructorCall cc | - cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and - sink.asExpr() = cc.getArgument(0) - ) - } + predicate isSink(DataFlow::Node sink) { sink instanceof InsecureLdapUrlSink } } module RequiresSslFlow = DataFlow::Make; From 0eaf222b54dd3c184ad19207df52ac861578560d Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 17 Mar 2023 11:58:17 -0400 Subject: [PATCH 19/23] Move public classes/predicates to top of library file --- .../code/java/security/InsecureLdapAuth.qll | 105 +++++++++--------- 1 file changed, 54 insertions(+), 51 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll index 42b56a11b67b..80007650c633 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuth.qll @@ -5,31 +5,6 @@ private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.frameworks.Networking private import semmle.code.java.frameworks.Jndi -/** - * An insecure (non-SSL, non-private) LDAP URL string literal. - */ -private class InsecureLdapUrlLiteral extends StringLiteral { - InsecureLdapUrlLiteral() { - // Match connection strings with the LDAP protocol and without private IP addresses to reduce false positives. - exists(string s | this.getValue() = s | - s.regexpMatch("(?i)ldap://[\\[a-zA-Z0-9].*") and - not s.substring(7, s.length()) instanceof PrivateHostName - ) - } -} - -/** The class `java.util.Hashtable`. */ -private class TypeHashtable extends Class { - TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") } -} - -/** Get the string value of an expression representing a hostname. */ -private string getHostname(Expr expr) { - result = expr.(CompileTimeConstantExpr).getStringValue() or - result = - expr.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getStringValue() -} - /** * An expression that represents an insecure (non-SSL, non-private) LDAP URL. */ @@ -54,6 +29,34 @@ class InsecureLdapUrl extends Expr { } } +/** + * A sink representing the construction of a `DirContextEnvironment`. + */ +class InsecureLdapUrlSink extends DataFlow::Node { + InsecureLdapUrlSink() { + exists(ConstructorCall cc | + cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and + this.asExpr() = cc.getArgument(0) + ) + } +} + +/** + * Holds if `ma` sets `java.naming.security.authentication` (also known as `Context.SECURITY_AUTHENTICATION`) to `simple` in some `Hashtable`. + */ +predicate isBasicAuthEnv(MethodAccess ma) { + hasFieldValueEnv(ma, "java.naming.security.authentication", "simple") or + hasFieldNameEnv(ma, "SECURITY_AUTHENTICATION", "simple") +} + +/** + * Holds if `ma` sets `java.naming.security.protocol` (also known as `Context.SECURITY_PROTOCOL`) to `ssl` in some `Hashtable`. + */ +predicate isSslEnv(MethodAccess ma) { + hasFieldValueEnv(ma, "java.naming.security.protocol", "ssl") or + hasFieldNameEnv(ma, "SECURITY_PROTOCOL", "ssl") +} + /** * Holds if `ma` writes the `java.naming.provider.url` (also known as `Context.PROVIDER_URL`) key of a `Hashtable`. */ @@ -71,11 +74,36 @@ predicate isProviderUrlSetter(MethodAccess ma) { ) } +/** + * An insecure (non-SSL, non-private) LDAP URL string literal. + */ +private class InsecureLdapUrlLiteral extends StringLiteral { + InsecureLdapUrlLiteral() { + // Match connection strings with the LDAP protocol and without private IP addresses to reduce false positives. + exists(string s | this.getValue() = s | + s.regexpMatch("(?i)ldap://[\\[a-zA-Z0-9].*") and + not s.substring(7, s.length()) instanceof PrivateHostName + ) + } +} + +/** The class `java.util.Hashtable`. */ +private class TypeHashtable extends Class { + TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") } +} + +/** Get the string value of an expression representing a hostname. */ +private string getHostname(Expr expr) { + result = expr.(CompileTimeConstantExpr).getStringValue() or + result = + expr.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getStringValue() +} + /** * Holds if `ma` sets `fieldValue` to `envValue` in some `Hashtable`. */ bindingset[fieldValue, envValue] -predicate hasFieldValueEnv(MethodAccess ma, string fieldValue, string envValue) { +private predicate hasFieldValueEnv(MethodAccess ma, string fieldValue, string envValue) { // environment.put("java.naming.security.authentication", "simple") ma.getMethod().getDeclaringType().getAnAncestor() instanceof TypeHashtable and ma.getMethod().hasName(["put", "setProperty"]) and @@ -98,28 +126,3 @@ private predicate hasFieldNameEnv(MethodAccess ma, string fieldName, string envV ) and ma.getArgument(1).(CompileTimeConstantExpr).getStringValue() = envValue } - -/** - * Holds if `ma` sets `java.naming.security.authentication` (also known as `Context.SECURITY_AUTHENTICATION`) to `simple` in some `Hashtable`. - */ -predicate isBasicAuthEnv(MethodAccess ma) { - hasFieldValueEnv(ma, "java.naming.security.authentication", "simple") or - hasFieldNameEnv(ma, "SECURITY_AUTHENTICATION", "simple") -} - -/** - * Holds if `ma` sets `java.naming.security.protocol` (also known as `Context.SECURITY_PROTOCOL`) to `ssl` in some `Hashtable`. - */ -predicate isSslEnv(MethodAccess ma) { - hasFieldValueEnv(ma, "java.naming.security.protocol", "ssl") or - hasFieldNameEnv(ma, "SECURITY_PROTOCOL", "ssl") -} - -class InsecureLdapUrlSink extends DataFlow::Node { - InsecureLdapUrlSink() { - exists(ConstructorCall cc | - cc.getConstructedType().getAnAncestor() instanceof TypeDirContext and - this.asExpr() = cc.getArgument(0) - ) - } -} From 43d79dc5b80b44d438a382b98bd1954d35a819ff Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Fri, 24 Mar 2023 11:36:52 -0400 Subject: [PATCH 20/23] Apply docs review suggestions Co-authored-by: Sarita Iyer <66540150+saritai@users.noreply.github.com> --- java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp | 2 +- .../src/change-notes/2023-03-08-insecure-ldap-authentication.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp index 9483e8d37059..096fa99c809a 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.qhelp @@ -33,7 +33,7 @@

In the following (good) example, a ldap:// URL is used, but - SASL authentication is enabled. + SASL authentication is enabled so that the credentials will be encrypted.

diff --git a/java/ql/src/change-notes/2023-03-08-insecure-ldap-authentication.md b/java/ql/src/change-notes/2023-03-08-insecure-ldap-authentication.md index 160d5639b22d..8331274891cc 100644 --- a/java/ql/src/change-notes/2023-03-08-insecure-ldap-authentication.md +++ b/java/ql/src/change-notes/2023-03-08-insecure-ldap-authentication.md @@ -1,4 +1,4 @@ --- category: newQuery --- -* The query `java/insecure-ldap-auth` has been promoted from experimental to the main query pack. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/4854) \ No newline at end of file +* The query `java/insecure-ldap-auth` has been promoted from experimental to the main query pack. This query detects transmission of cleartext credentials in LDAP authentication. Insecure LDAP authentication causes sensitive information to be vulnerable to remote attackers. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/4854) \ No newline at end of file From 106e5e714506be5d1c85c6272ac9bae542fb596d Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Fri, 24 Mar 2023 12:40:10 -0400 Subject: [PATCH 21/23] Docs review suggestion Co-authored-by: Sarita Iyer <66540150+saritai@users.noreply.github.com> --- java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index 635026222845..01c16ded3ddd 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -1,6 +1,6 @@ /** * @name Insecure LDAP authentication - * @description LDAP authentication with credentials sent in cleartext. + * @description LDAP authentication with credentials sent in cleartext makes sensitive information vulnerable to remote attackers * @kind path-problem * @problem.severity warning * @security-severity 8.8 From 9bfb13b942a8d51b44b9df3a81219e7cb44e4f45 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Mon, 27 Mar 2023 12:26:18 -0400 Subject: [PATCH 22/23] Update to the `Global`/`flow*` api --- .../lib/semmle/code/java/security/InsecureLdapAuthQuery.qll | 6 +++--- java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql | 6 +++--- .../query-tests/security/CWE-522/InsecureLdapAuthTest.ql | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll index 150d9c724580..4d4546c7f8a0 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll @@ -24,7 +24,7 @@ private module InsecureLdapUrlConfig implements DataFlow::ConfigSig { } } -module InsecureLdapUrlFlow = TaintTracking::Make; +module InsecureLdapUrlFlow = TaintTracking::Global; /** * A taint-tracking configuration for `simple` basic-authentication in LDAP configuration. @@ -40,7 +40,7 @@ private module BasicAuthConfig implements DataFlow::ConfigSig { predicate isSink(DataFlow::Node sink) { sink instanceof InsecureLdapUrlSink } } -module BasicAuthFlow = DataFlow::Make; +module BasicAuthFlow = DataFlow::Global; /** * A taint-tracking configuration for `ssl` configuration in LDAP authentication. @@ -56,4 +56,4 @@ private module RequiresSslConfig implements DataFlow::ConfigSig { predicate isSink(DataFlow::Node sink) { sink instanceof InsecureLdapUrlSink } } -module RequiresSslFlow = DataFlow::Make; +module RequiresSslFlow = DataFlow::Global; diff --git a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql index 01c16ded3ddd..dabd0d2c6450 100644 --- a/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -18,8 +18,8 @@ import InsecureLdapUrlFlow::PathGraph from InsecureLdapUrlFlow::PathNode source, InsecureLdapUrlFlow::PathNode sink where - InsecureLdapUrlFlow::hasFlowPath(source, sink) and - BasicAuthFlow::hasFlowTo(sink.getNode()) and - not RequiresSslFlow::hasFlowTo(sink.getNode()) + InsecureLdapUrlFlow::flowPath(source, sink) and + BasicAuthFlow::flowTo(sink.getNode()) and + not RequiresSslFlow::flowTo(sink.getNode()) select sink.getNode(), source, sink, "Insecure LDAP authentication from $@.", source.getNode(), "LDAP connection string" diff --git a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql index 1d8bfbea58f9..4ae89d213135 100644 --- a/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql +++ b/java/ql/test/query-tests/security/CWE-522/InsecureLdapAuthTest.ql @@ -9,9 +9,9 @@ class InsecureLdapAuthenticationTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasInsecureLdapAuth" and - exists(DataFlow::Node sink | InsecureLdapUrlFlow::hasFlowTo(sink) | - BasicAuthFlow::hasFlowTo(sink) and - not RequiresSslFlow::hasFlowTo(sink) and + exists(DataFlow::Node sink | InsecureLdapUrlFlow::flowTo(sink) | + BasicAuthFlow::flowTo(sink) and + not RequiresSslFlow::flowTo(sink) and sink.getLocation() = location and element = sink.toString() and value = "" From 97ec808a6f5dde9a662eff1dfdb918905807e82c Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Tue, 28 Mar 2023 10:28:15 -0400 Subject: [PATCH 23/23] Make configuration public Co-authored-by: Tony Torralba --- java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll index 4d4546c7f8a0..6fb53b769e49 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureLdapAuthQuery.qll @@ -9,7 +9,7 @@ import semmle.code.java.security.InsecureLdapAuth /** * A taint-tracking configuration for `ldap://` URL in LDAP authentication. */ -private module InsecureLdapUrlConfig implements DataFlow::ConfigSig { +module InsecureLdapUrlConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl } predicate isSink(DataFlow::Node sink) { sink instanceof InsecureLdapUrlSink }