UseOfLessTrustedSource -> ClientSuppliedIpUsedInSecurityCheck"

Author: haby0

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java renamed to java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java

@@ -6,7 +6,7 @@
 import org.springframework.web.bind.annotation.ResponseBody;
 
 @Controller
-public class UseOfLessTrustedSource {
+public class ClientSuppliedIpUsedInSecurityCheck {
 
     @Autowired
     private HttpServletRequest request;
@@ -31,12 +31,12 @@ public void bad2(HttpServletRequest request) {
     @ResponseBody
     public String good1(HttpServletRequest request) {
         String ip = request.getHeader("X-FORWARDED-FOR");
-        String[] parts = ip.split(",");
         // Good: if this application runs behind a reverse proxy it may append the real remote IP to the end of any client-supplied X-Forwarded-For header.
-        ip = parts[parts.length - 1];
+        ip = ip.split(",")[ip.split(",").length - 1];
         if (!StringUtils.startsWith(ip, "192.168.")) {
             new Exception("ip illegal");
         }
+        return ip;
     }
 
     protected String getClientIP() {

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp renamed to java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp

@@ -19,7 +19,7 @@ bypass a ban-list, for example.</p>
 In <code>bad1</code> method and <code>bad2</code> method, the client ip the <code>X-Forwarded-For</code> is split into comma-separated values, but the less-trustworthy first one is used. Both of these examples could be deceived by providing a forged HTTP header. The method
 <code>good1</code> similarly splits an <code>X-Forwarded-For</code> value, but uses the last, more-trustworthy entry.</p>
 
-<sample src="UseOfLessTrustedSource.java" />
+<sample src="ClientSuppliedIpUsedInSecurityCheck.java" />
 
 </example>
 <references>

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql renamed to java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql

@@ -11,19 +11,23 @@
  */
 
 import java
-import UseOfLessTrustedSourceLib
+import ClientSuppliedIpUsedInSecurityCheckLib
 import semmle.code.java.dataflow.FlowSources
 import DataFlow::PathGraph
 
 /**
  * Taint-tracking configuration tracing flow from obtaining a client ip from an HTTP header to a sensitive use.
  */
-class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration {
-  UseOfLessTrustedSourceConfig() { this = "UseOfLessTrustedSourceConfig" }
+class ClientSuppliedIpUsedInSecurityCheckConfig extends TaintTracking::Configuration {
+  ClientSuppliedIpUsedInSecurityCheckConfig() { this = "ClientSuppliedIpUsedInSecurityCheckConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof UseOfLessTrustedSource }
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof ClientSuppliedIpUsedInSecurityCheck
+  }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof UseOfLessTrustedSink }
+  override predicate isSink(DataFlow::Node sink) {
+    sink instanceof ClientSuppliedIpUsedInSecurityCheckSink
+  }
 
   /**
    * Splitting a header value by `,` and taking an entry other than the first is sanitizing, because
@@ -42,7 +46,8 @@ class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration {
   }
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, UseOfLessTrustedSourceConfig conf
+from
+  DataFlow::PathNode source, DataFlow::PathNode sink, ClientSuppliedIpUsedInSecurityCheckConfig conf
 where conf.hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "IP address spoofing might include code from $@.",
   source.getNode(), "this user input"

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll renamed to java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll

@@ -10,8 +10,8 @@ import experimental.semmle.code.java.Logging
  *
  * For example: `ServletRequest.getHeader("X-Forwarded-For")`.
  */
-class UseOfLessTrustedSource extends DataFlow::Node {
-  UseOfLessTrustedSource() {
+class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::Node {
+  ClientSuppliedIpUsedInSecurityCheck() {
     exists(MethodAccess ma |
       ma.getMethod().hasName("getHeader") and
       ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [
@@ -25,15 +25,15 @@ class UseOfLessTrustedSource extends DataFlow::Node {
 }
 
 /** A data flow sink for ip address forgery vulnerabilities. */
-abstract class UseOfLessTrustedSink extends DataFlow::Node { }
+abstract class ClientSuppliedIpUsedInSecurityCheckSink extends DataFlow::Node { }
 
 /**
  * A data flow sink for remote client ip comparison.
  *
  * For example: `if (!StringUtils.startsWith(ipAddr, "192.168.")){...` determine whether the client ip starts
  * with `192.168.`, and the program can be deceived by forging the ip address.
  */
-private class CompareSink extends UseOfLessTrustedSink {
+private class CompareSink extends ClientSuppliedIpUsedInSecurityCheckSink {
   CompareSink() {
     exists(MethodAccess ma |
       ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and
@@ -55,10 +55,7 @@ private class CompareSink extends UseOfLessTrustedSink {
       ma.getMethod().getDeclaringType() instanceof TypeString and
       ma.getMethod().getNumberOfParameters() = 1 and
       ma.getQualifier() = this.asExpr() and
-      ma.getAnArgument()
-          .(CompileTimeConstantExpr)
-          .getStringValue()
-          .regexpMatch("^((10\\.((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?)|(192\\.168\\.)|172\\.(1[6789]|2[0-9]|3[01])\\.)((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)$") // Matches IP-address-like strings
+      ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().regexpMatch(getIpAddressRegex()) // Matches IP-address-like strings
     )
     or
     exists(MethodAccess ma |
@@ -68,10 +65,7 @@ private class CompareSink extends UseOfLessTrustedSink {
           .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], "StringUtils") and
       ma.getMethod().getNumberOfParameters() = 2 and
       ma.getAnArgument() = this.asExpr() and
-      ma.getAnArgument()
-          .(CompileTimeConstantExpr)
-          .getStringValue()
-          .regexpMatch("^((10\\.((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?)|(192\\.168\\.)|172\\.(1[6789]|2[0-9]|3[01])\\.)((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)$")
+      ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().regexpMatch(getIpAddressRegex())
     )
     or
     exists(MethodAccess ma |
@@ -88,7 +82,7 @@ private class CompareSink extends UseOfLessTrustedSink {
 }
 
 /** A data flow sink for sql operation. */
-private class SqlOperationSink extends UseOfLessTrustedSink {
+private class SqlOperationSink extends ClientSuppliedIpUsedInSecurityCheckSink {
   SqlOperationSink() { this instanceof QueryInjectionSink }
 }
 
@@ -99,3 +93,8 @@ class SplitMethod extends Method {
     this.hasQualifiedName("java.lang", "String", "split")
   }
 }
+
+string getIpAddressRegex() {
+  result =
+    "^((10\\.((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?)|(192\\.168\\.)|172\\.(1[6789]|2[0-9]|3[01])\\.)((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)$"
+}

java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected

@@ -0,0 +1,16 @@
+edges
+| ClientSuppliedIpUsedInSecurityCheck.java:16:21:16:33 | getClientIP(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:17:37:17:38 | ip |
+| ClientSuppliedIpUsedInSecurityCheck.java:24:21:24:33 | getClientIP(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:25:33:25:34 | ip |
+| ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String |
+| ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String | ClientSuppliedIpUsedInSecurityCheck.java:16:21:16:33 | getClientIP(...) : String |
+| ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String | ClientSuppliedIpUsedInSecurityCheck.java:24:21:24:33 | getClientIP(...) : String |
+nodes
+| ClientSuppliedIpUsedInSecurityCheck.java:16:21:16:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String |
+| ClientSuppliedIpUsedInSecurityCheck.java:17:37:17:38 | ip | semmle.label | ip |
+| ClientSuppliedIpUsedInSecurityCheck.java:24:21:24:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String |
+| ClientSuppliedIpUsedInSecurityCheck.java:25:33:25:34 | ip | semmle.label | ip |
+| ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) : String | semmle.label | getHeader(...) : String |
+| ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String | semmle.label | ...[...] : String |
+#select
+| ClientSuppliedIpUsedInSecurityCheck.java:17:37:17:38 | ip | ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:17:37:17:38 | ip | IP address spoofing might include code from $@. | ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) | this user input |
+| ClientSuppliedIpUsedInSecurityCheck.java:25:33:25:34 | ip | ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:25:33:25:34 | ip | IP address spoofing might include code from $@. | ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) | this user input |

java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java renamed to java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java

@@ -6,7 +6,7 @@
 import org.springframework.web.bind.annotation.ResponseBody;
 
 @Controller
-public class UseOfLessTrustedSource {
+public class ClientSuppliedIpUsedInSecurityCheck {
 
     @Autowired
     private HttpServletRequest request;
@@ -30,15 +30,13 @@ public void bad2(HttpServletRequest request) {
     @GetMapping(value = "good1")
     @ResponseBody
     public String good1(HttpServletRequest request) {
-        String remoteAddr = "";
-        if (request != null) {
-            remoteAddr = request.getHeader("X-FORWARDED-FOR");
-            remoteAddr = remoteAddr.split(",")[remoteAddr.split(",").length - 1]; // good
-            if (remoteAddr == null || "".equals(remoteAddr)) {
-                remoteAddr = request.getRemoteAddr();
-            }
+        String ip = request.getHeader("X-FORWARDED-FOR");
+        // Good: if this application runs behind a reverse proxy it may append the real remote IP to the end of any client-supplied X-Forwarded-For header.
+        ip = ip.split(",")[ip.split(",").length - 1];
+        if (!StringUtils.startsWith(ip, "192.168.")) {
+            new Exception("ip illegal");
         }
-        return remoteAddr;
+        return ip;
     }
 
     protected String getClientIP() {
@@ -48,4 +46,4 @@ protected String getClientIP() {
         }
         return xfHeader.split(",")[0];
     }
-}
+}

java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql