Factor private host regex into the networking library and enhance the query

luchua-bc · luchua-bc · commit c069a5b4c654 · 2021-01-04T14:51:32.000Z
diff --git a/java/ql/src/experimental/Security/CWE/CWE-522/InsecureBasicAuth.ql b/java/ql/src/experimental/Security/CWE/CWE-522/InsecureBasicAuth.ql
@@ -14,14 +14,6 @@ import semmle.code.java.frameworks.ApacheHttp
 import semmle.code.java.dataflow.TaintTracking
 import DataFlow::PathGraph
 
-/**
- * Gets a regular expression for matching private hosts, which only matches the host portion therefore checking for port is not necessary.
- */
-private string getPrivateHostRegex() {
-  result =
-    "(?i)localhost(?:[:/?#].*)?|127\\.0\\.0\\.1(?:[:/?#].*)?|10(?:\\.[0-9]+){3}(?:[:/?#].*)?|172\\.16(?:\\.[0-9]+){2}(?:[:/?#].*)?|192.168(?:\\.[0-9]+){2}(?:[:/?#].*)?|\\[?0:0:0:0:0:0:0:1\\]?(?:[:/?#].*)?|\\[?::1\\]?(?:[:/?#].*)?"
-}
-
 /**
  * Class of Java URL constructor.
  */
@@ -76,7 +68,7 @@ class HttpStringLiteral extends StringLiteral {
     // Match URLs with the HTTP protocol and without private IP addresses to reduce false positives.
     exists(string s | this.getRepresentedString() = s |
       s.regexpMatch("(?i)http://[\\[a-zA-Z0-9].*") and
-      not s.substring(7, s.length()).regexpMatch(getPrivateHostRegex())
+      not s.substring(7, s.length()) instanceof PrivateHostName
     )
   }
 }
@@ -101,7 +93,7 @@ predicate concatHttpString(Expr protocol, Expr host) {
       host.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getStringValue()
   |
     hostString.length() = 0 or // Empty host is loopback address
-    hostString.regexpMatch(getPrivateHostRegex())
+    hostString instanceof PrivateHostName
   )
 }
 
diff --git a/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.qhelp b/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.qhelp
@@ -11,7 +11,7 @@
 
   <example>
     <p>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.</p>
-    <sample src="InsecureLDAPAuth.java" />
+    <sample src="InsecureLdapAuth.java" />
   </example>
 
   <references>
diff --git a/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql
@@ -10,26 +10,19 @@
 
 import java
 import semmle.code.java.frameworks.Jndi
+import semmle.code.java.frameworks.Networking
 import semmle.code.java.dataflow.TaintTracking
 import DataFlow::PathGraph
 
 /**
- * Gets a regular expression for matching private hosts, which only matches the host portion therefore checking for port is not necessary.
+ * Insecure (non-SSL, non-private) LDAP URL string literal.
  */
-private string getPrivateHostRegex() {
-  result =
-    "(?i)localhost(?:[:/?#].*)?|127\\.0\\.0\\.1(?:[:/?#].*)?|10(?:\\.[0-9]+){3}(?:[:/?#].*)?|172\\.16(?:\\.[0-9]+){2}(?:[:/?#].*)?|192.168(?:\\.[0-9]+){2}(?:[:/?#].*)?|\\[?0:0:0:0:0:0:0:1\\]?(?:[:/?#].*)?|\\[?::1\\]?(?:[:/?#].*)?"
-}
-
-/**
- * String of LDAP connections not in private domains.
- */
-class LdapStringLiteral extends StringLiteral {
-  LdapStringLiteral() {
+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.getRepresentedString() = s |
       s.regexpMatch("(?i)ldap://[\\[a-zA-Z0-9].*") and
-      not s.substring(7, s.length()).regexpMatch(getPrivateHostRegex())
+      not s.substring(7, s.length()) instanceof PrivateHostName
     )
   }
 }
@@ -47,24 +40,15 @@ class TypeHashtable extends Class {
 /**
  * Holds if a non-private LDAP string is concatenated from both protocol and host.
  */
-predicate concatLdapString(Expr protocol, Expr host) {
-  (
-    protocol.(CompileTimeConstantExpr).getStringValue().regexpMatch("(?i)ldap(://)?") or
-    protocol
-        .(VarAccess)
-        .getVariable()
-        .getAnAssignedValue()
-        .(CompileTimeConstantExpr)
-        .getStringValue()
-        .regexpMatch("(?i)ldap(://)?")
-  ) and
+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()
   |
     hostString.length() = 0 or // Empty host is loopback address
-    hostString.regexpMatch(getPrivateHostRegex())
+    hostString instanceof PrivateHostName
   )
 }
 
@@ -76,22 +60,21 @@ Expr getLeftmostConcatOperand(Expr expr) {
 }
 
 /**
- * String concatenated with `LdapStringLiteral`.
+ * String concatenated with `InsecureLdapUrlLiteral`.
  */
-class LdapString extends Expr {
-  LdapString() {
-    this instanceof LdapStringLiteral
+class InsecureLdapUrl extends Expr {
+  InsecureLdapUrl() {
+    this instanceof InsecureLdapUrlLiteral
     or
-    concatLdapString(this.(AddExpr).getLeftOperand(),
+    concatInsecureLdapString(this.(AddExpr).getLeftOperand(),
       getLeftmostConcatOperand(this.(AddExpr).getRightOperand()))
   }
 }
 
 /**
- * Tainted value passed to env `Hashtable` as the provider URL, i.e.
- * `env.put(Context.PROVIDER_URL, tainted)` or `env.setProperty(Context.PROVIDER_URL, tainted)`.
+ * Holds if `ma` writes the `java.naming.provider.url` (also known as `Context.PROVIDER_URL`) key of a `Hashtable`.
  */
-predicate isProviderUrlEnv(MethodAccess ma) {
+predicate isProviderUrlSetter(MethodAccess ma) {
   ma.getMethod().getDeclaringType().getAnAncestor() instanceof TypeHashtable and
   (ma.getMethod().hasName("put") or ma.getMethod().hasName("setProperty")) and
   (
@@ -106,8 +89,7 @@ predicate isProviderUrlEnv(MethodAccess ma) {
 }
 
 /**
- * Holds if the value "simple" is passed to env `Hashtable` as the authentication mechanism, i.e.
- * `env.put(Context.SECURITY_AUTHENTICATION, "simple")` or `env.setProperty(Context.SECURITY_AUTHENTICATION, "simple")`.
+ * Holds if `ma` sets `java.naming.security.authentication` (also known as `Context.SECURITY_AUTHENTICATION`) to `simple` in some `Hashtable`.
  */
 predicate isSimpleAuthEnv(MethodAccess ma) {
   ma.getMethod().getDeclaringType().getAnAncestor() instanceof TypeHashtable and
@@ -132,13 +114,13 @@ class LdapAuthFlowConfig extends TaintTracking::Configuration {
   LdapAuthFlowConfig() { this = "InsecureLdapAuth:LdapAuthFlowConfig" }
 
   /** Source of non-private LDAP connection string */
-  override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof LdapString }
+  override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl }
 
   /** Sink of provider URL with simple authentication */
   override predicate isSink(DataFlow::Node sink) {
     exists(MethodAccess pma |
       sink.asExpr() = pma.getArgument(1) and
-      isProviderUrlEnv(pma) and
+      isProviderUrlSetter(pma) and
       exists(MethodAccess sma |
         sma.getQualifier() = pma.getQualifier().(VarAccess).getVariable().getAnAccess() and
         isSimpleAuthEnv(sma)
diff --git a/java/ql/src/semmle/code/java/frameworks/Networking.qll b/java/ql/src/semmle/code/java/frameworks/Networking.qll
@@ -129,3 +129,13 @@ class UrlOpenConnectionMethod extends Method {
     this.getName() = "openConnection"
   }
 }
+
+/**
+ * A string matching private host names of IPv4 and IPv6, which only matches the host portion therefore checking for port is not necessary.
+ */
+class PrivateHostName extends string {
+  bindingset[this]
+  PrivateHostName() {
+    this.regexpMatch("(?i)localhost(?:[:/?#].*)?|127\\.0\\.0\\.1(?:[:/?#].*)?|10(?:\\.[0-9]+){3}(?:[:/?#].*)?|172\\.16(?:\\.[0-9]+){2}(?:[:/?#].*)?|192.168(?:\\.[0-9]+){2}(?:[:/?#].*)?|\\[?0:0:0:0:0:0:0:1\\]?(?:[:/?#].*)?|\\[?::1\\]?(?:[:/?#].*)?")
+  }
+}
diff --git a/java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.expected b/java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.expected
@@ -2,14 +2,18 @@ edges
 | InsecureLdapAuth.java:11:20:11:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:15:41:15:47 | ldapUrl |
 | InsecureLdapAuth.java:25:20:25:39 | ... + ... : String | InsecureLdapAuth.java:29:41:29:47 | ldapUrl |
 | InsecureLdapAuth.java:81:20:81:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:85:41:85:47 | ldapUrl |
+| InsecureLdapAuth.java:96:20:96:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:100:47:100:53 | ldapUrl |
 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:41:15:47 | ldapUrl | semmle.label | ldapUrl |
 | InsecureLdapAuth.java:25:20:25:39 | ... + ... : String | semmle.label | ... + ... : String |
 | InsecureLdapAuth.java:29:41:29:47 | ldapUrl | semmle.label | ldapUrl |
 | InsecureLdapAuth.java:81:20:81:50 | "ldap://ad.your-server.com:389" : String | semmle.label | "ldap://ad.your-server.com:389" : String |
 | InsecureLdapAuth.java:85:41:85:47 | ldapUrl | semmle.label | ldapUrl |
+| 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:47:100:53 | ldapUrl | semmle.label | ldapUrl |
 #select
 | InsecureLdapAuth.java:15:41:15:47 | ldapUrl | InsecureLdapAuth.java:11:20:11:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:15:41:15:47 | ldapUrl | Insecure LDAP authentication from $@. | InsecureLdapAuth.java:11:20:11:50 | "ldap://ad.your-server.com:389" | LDAP connection string |
 | InsecureLdapAuth.java:29:41:29:47 | ldapUrl | InsecureLdapAuth.java:25:20:25:39 | ... + ... : String | InsecureLdapAuth.java:29:41:29:47 | ldapUrl | Insecure LDAP authentication from $@. | InsecureLdapAuth.java:25:20:25:39 | ... + ... | LDAP connection string |
 | InsecureLdapAuth.java:85:41:85:47 | ldapUrl | InsecureLdapAuth.java:81:20:81:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:85:41:85:47 | ldapUrl | Insecure LDAP authentication from $@. | InsecureLdapAuth.java:81:20:81:50 | "ldap://ad.your-server.com:389" | LDAP connection string |
+| InsecureLdapAuth.java:100:47:100:53 | ldapUrl | InsecureLdapAuth.java:96:20:96:50 | "ldap://ad.your-server.com:389" : String | InsecureLdapAuth.java:100:47:100:53 | ldapUrl | Insecure LDAP authentication from $@. | InsecureLdapAuth.java:96:20:96:50 | "ldap://ad.your-server.com:389" | LDAP connection string |
diff --git a/java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.java b/java/ql/test/experimental/query-tests/security/CWE-522/InsecureLdapAuth.java
@@ -89,4 +89,19 @@ public void testCleartextLdapAuth3(String ldapUserName, String password) {
 		environment.put(Context.SECURITY_CREDENTIALS, password);
 		InitialLdapContext ldapContext = new InitialLdapContext(environment, null);
 	}	
+
+
+	// BAD - Test LDAP authentication in cleartext using `DirContext` and string literals.
+	public void testCleartextLdapAuth4(String ldapUserName, String password) {
+		String ldapUrl = "ldap://ad.your-server.com:389";
+		Hashtable<String, String> environment = new Hashtable<String, String>();
+		environment.put("java.naming.factory.initial",
+			"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);
+	}
 }

Original file line number	Diff line number	Diff line change
`@@ -14,14 +14,6 @@ import semmle.code.java.frameworks.ApacheHttp`
`14`	`14`	`import semmle.code.java.dataflow.TaintTracking`
`15`	`15`	`import DataFlow::PathGraph`
`16`	`16`
`17`		`-/**`
`18`		`- * Gets a regular expression for matching private hosts, which only matches the host portion therefore checking for port is not necessary.`
`19`		`- */`
`20`		`-private string getPrivateHostRegex() {`
`21`		`- result =`
`22`		`- "(?i)localhost(?:[:/?#].)?\|127\\.0\\.0\\.1(?:[:/?#].)?\|10(?:\\.[0-9]+){3}(?:[:/?#].)?\|172\\.16(?:\\.[0-9]+){2}(?:[:/?#].)?\|192.168(?:\\.[0-9]+){2}(?:[:/?#].)?\|\\[?0:0:0:0:0:0:0:1\\]?(?:[:/?#].)?\|\\[?::1\\]?(?:[:/?#].*)?"`
`23`		`-}`
`24`		`-`
`25`	`17`	`/**`
`26`	`18`	`* Class of Java URL constructor.`
`27`	`19`	`*/`
`@@ -76,7 +68,7 @@ class HttpStringLiteral extends StringLiteral {`
`76`	`68`	`// Match URLs with the HTTP protocol and without private IP addresses to reduce false positives.`
`77`	`69`	`exists(string s \| this.getRepresentedString() = s \|`
`78`	`70`	`s.regexpMatch("(?i)http://[\\[a-zA-Z0-9].*") and`
`79`		`- not s.substring(7, s.length()).regexpMatch(getPrivateHostRegex())`
	`71`	`+ not s.substring(7, s.length()) instanceof PrivateHostName`
`80`	`72`	`)`
`81`	`73`	`}`
`82`	`74`	`}`
`@@ -101,7 +93,7 @@ predicate concatHttpString(Expr protocol, Expr host) {`
`101`	`93`	`host.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getStringValue()`
`102`	`94`	`\|`
`103`	`95`	`hostString.length() = 0 or // Empty host is loopback address`
`104`		`- hostString.regexpMatch(getPrivateHostRegex())`
	`96`	`+ hostString instanceof PrivateHostName`
`105`	`97`	`)`
`106`	`98`	`}`
`107`	`99`
Original file line number	Diff line number	Diff line change
`@@ -129,3 +129,13 @@ class UrlOpenConnectionMethod extends Method {`
`129`	`129`	`this.getName() = "openConnection"`
`130`	`130`	`}`
`131`	`131`	`}`
	`132`	`+`
	`133`	`+/**`
	`134`	`+ * A string matching private host names of IPv4 and IPv6, which only matches the host portion therefore checking for port is not necessary.`
	`135`	`+ */`
	`136`	`+class PrivateHostName extends string {`
	`137`	`+ bindingset[this]`
	`138`	`+ PrivateHostName() {`
	`139`	`+ this.regexpMatch("(?i)localhost(?:[:/?#].)?\|127\\.0\\.0\\.1(?:[:/?#].)?\|10(?:\\.[0-9]+){3}(?:[:/?#].)?\|172\\.16(?:\\.[0-9]+){2}(?:[:/?#].)?\|192.168(?:\\.[0-9]+){2}(?:[:/?#].)?\|\\[?0:0:0:0:0:0:0:1\\]?(?:[:/?#].)?\|\\[?::1\\]?(?:[:/?#].*)?")`
	`140`	`+ }`
	`141`	`+}`