From 05da1dc4a37ce91cc2df60d050cfe2c8a76889a1 Mon Sep 17 00:00:00 2001
From: Ed Minnix <egregius313@github.com>
Date: Wed, 8 Mar 2023 21:08:02 -0500
Subject: [PATCH] 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 d5948f2ff04..68cafa845d2 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
+      )
     )
   }
 }