UseOfLessTrustedSource -> ClientSuppliedIpUsedInSecurityCheck"

java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java

@@ -6,7 +6,7 @@ import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.ResponseBody;
 
 @Controller
-public class UseOfLessTrustedSource {
+public class ClientSuppliedIpUsedInSecurityCheck {
 
     @Autowired
     private HttpServletRequest request;
@@ -31,12 +31,12 @@ public class UseOfLessTrustedSource {
     @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/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/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/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,7 +25,7 @@ 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.
@@ -33,7 +33,7 @@ abstract class UseOfLessTrustedSink extends DataFlow::Node { }
  * 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])?)$"
+}