diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java b/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java similarity index 92% rename from java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java rename to java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java index ad959cb2dc8..93a860981d1 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java +++ b/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() { diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp similarity index 96% rename from java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp rename to java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp index 61848f111ad..fd62ab2968b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp @@ -19,7 +19,7 @@ bypass a ban-list, for example.

In bad1 method and bad2 method, the client ip the X-Forwarded-For 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 good1 similarly splits an X-Forwarded-For value, but uses the last, more-trustworthy entry.

- + diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql b/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql similarity index 69% rename from java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql rename to java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql index 14ae3444263..78d8bfee5f0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql +++ b/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" diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll similarity index 77% rename from java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll rename to java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll index 04df6aa5dd6..fd9353d3414 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/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])?)$" +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected b/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected new file mode 100644 index 00000000000..5627f9541f1 --- /dev/null +++ b/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 | diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java b/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java similarity index 73% rename from java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java rename to java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java index 689246340da..93a860981d1 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java +++ b/java/ql/test/experimental/query-tests/security/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; @@ -30,15 +30,13 @@ public class UseOfLessTrustedSource { @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 @@ public class UseOfLessTrustedSource { } return xfHeader.split(",")[0]; } -} \ No newline at end of file +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref b/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref new file mode 100644 index 00000000000..476249f5fd9 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected deleted file mode 100644 index ff9cbc24a20..00000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected +++ /dev/null @@ -1,16 +0,0 @@ -edges -| UseOfLessTrustedSource.java:16:21:16:33 | getClientIP(...) : String | UseOfLessTrustedSource.java:17:37:17:38 | ip | -| UseOfLessTrustedSource.java:24:21:24:33 | getClientIP(...) : String | UseOfLessTrustedSource.java:25:33:25:34 | ip | -| UseOfLessTrustedSource.java:45:27:45:62 | getHeader(...) : String | UseOfLessTrustedSource.java:49:16:49:37 | ...[...] : String | -| UseOfLessTrustedSource.java:49:16:49:37 | ...[...] : String | UseOfLessTrustedSource.java:16:21:16:33 | getClientIP(...) : String | -| UseOfLessTrustedSource.java:49:16:49:37 | ...[...] : String | UseOfLessTrustedSource.java:24:21:24:33 | getClientIP(...) : String | -nodes -| UseOfLessTrustedSource.java:16:21:16:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String | -| UseOfLessTrustedSource.java:17:37:17:38 | ip | semmle.label | ip | -| UseOfLessTrustedSource.java:24:21:24:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String | -| UseOfLessTrustedSource.java:25:33:25:34 | ip | semmle.label | ip | -| UseOfLessTrustedSource.java:45:27:45:62 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:49:16:49:37 | ...[...] : String | semmle.label | ...[...] : String | -#select -| UseOfLessTrustedSource.java:17:37:17:38 | ip | UseOfLessTrustedSource.java:45:27:45:62 | getHeader(...) : String | UseOfLessTrustedSource.java:17:37:17:38 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:45:27:45:62 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:25:33:25:34 | ip | UseOfLessTrustedSource.java:45:27:45:62 | getHeader(...) : String | UseOfLessTrustedSource.java:25:33:25:34 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:45:27:45:62 | getHeader(...) | this user input | diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.qlref b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.qlref deleted file mode 100644 index 3c547a2c871..00000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql \ No newline at end of file