From 3f0a3266aa61617fb374c0d49e006dfabe429fc0 Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 8 Apr 2021 17:14:03 +0800 Subject: [PATCH 01/28] [Java] CWE-348: Use of less trusted source --- .../CWE/CWE-348/UseOfLessTrustedSource.java | 74 +++++++ .../CWE/CWE-348/UseOfLessTrustedSource.qhelp | 38 ++++ .../CWE/CWE-348/UseOfLessTrustedSource.ql | 44 ++++ .../CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 88 ++++++++ .../CWE-348/UseOfLessTrustedSource.expected | 14 ++ .../CWE-348/UseOfLessTrustedSource.java | 74 +++++++ .../CWE-348/UseOfLessTrustedSource.qlref | 1 + .../query-tests/security/CWE-348/options | 2 + .../slf4j-api-1.6.4/org/slf4j/Logger.java | 127 +++++++++++ .../org/slf4j/LoggerFactory.java | 21 ++ .../slf4j-api-1.6.4/org/slf4j/Marker.java | 30 +++ .../core/annotation/AliasFor.java | 21 ++ .../stereotype/Controller.java | 15 +- .../org/springframework/util/ObjectUtils.java | 202 ++++++++++++++++++ .../org/springframework/util/StringUtils.java | 30 +++ .../web/bind/annotation/GetMapping.java | 51 +++++ .../web/bind/annotation/Mapping.java | 4 + .../web/bind/annotation/RequestMapping.java | 23 +- .../web/bind/annotation/RequestParam.java | 23 +- .../web/bind/annotation/ResponseBody.java | 13 ++ 20 files changed, 885 insertions(+), 10 deletions(-) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll create mode 100644 java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.qlref create mode 100644 java/ql/test/experimental/query-tests/security/CWE-348/options create mode 100644 java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Logger.java create mode 100644 java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/LoggerFactory.java create mode 100644 java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Marker.java create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/core/annotation/AliasFor.java create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/util/ObjectUtils.java create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/util/StringUtils.java create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/GetMapping.java create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/Mapping.java create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/ResponseBody.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java new file mode 100644 index 000000000000..d338134ed3ed --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java @@ -0,0 +1,74 @@ +import javax.servlet.http.HttpServletRequest; +import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.ResponseBody; + +@Controller +public class UseOfLessTrustedSource { + + private static final Logger log = LoggerFactory.getLogger(UseOfLessTrustedSource.class); + + @GetMapping(value = "bad1") + @ResponseBody + public String bad1(HttpServletRequest request) { + String remoteAddr = ""; + if (request != null) { + remoteAddr = request.getHeader("X-FORWARDED-FOR"); + remoteAddr = remoteAddr.split(",")[0]; + if (remoteAddr == null || "".equals(remoteAddr)) { + remoteAddr = request.getRemoteAddr(); + } + } + return remoteAddr; + } + + @GetMapping(value = "bad2") + public void bad2(HttpServletRequest request) { + String ip = request.getHeader("X-Forwarded-For"); + + log.debug("getClientIP header X-Forwarded-For:{}", ip); + + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getHeader("Proxy-Client-IP"); + log.debug("getClientIP header Proxy-Client-IP:{}", ip); + } + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getHeader("WL-Proxy-Client-IP"); + log.debug("getClientIP header WL-Proxy-Client-IP:{}", ip); + } + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getHeader("HTTP_CLIENT_IP"); + log.debug("getClientIP header HTTP_CLIENT_IP:{}", ip); + } + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getHeader("HTTP_X_FORWARDED_FOR"); + log.debug("getClientIP header HTTP_X_FORWARDED_FOR:{}", ip); + } + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getHeader("X-Real-IP"); + log.debug("getClientIP header X-Real-IP:{}", ip); + } + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getRemoteAddr(); + log.debug("getRemoteAddr IP:{}", ip); + } + System.out.println("client ip is: " + ip); + } + + @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(); + } + } + return remoteAddr; + } +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp new file mode 100644 index 000000000000..1c3a4485cbd8 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -0,0 +1,38 @@ + + + +

The software obtains the original client IP address through the http header X-Forwarded-For, which is used to ensure +security or track it in the log for statistical or other reasons. Attackers can use X-Forwarded-For Spoofing software.

+ + + + +

When the software is not using a proxy server, get the last ip.

+ + + + +

The following examples show the bad case and the good case respectively. Bad case, such as bad1 to bad2. +In the bad1 method, the value of X-Forwarded-For in header is split, and the first value of +the split array is obtained. Good case, such as good1, split the value of X-Forwarded-For in header +and get the last value of the split array.

+ + + + + + +
  • Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring: + +Prevent IP address spoofing with... +
    • + +
  • Security Rule Zero: A Warning about X-Forwarded-For: + +Security Rule Zero: A Warning about X-Forwarded-For +
    • + +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql new file mode 100644 index 000000000000..15d665d37abd --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql @@ -0,0 +1,44 @@ +/** + * @name X-Forwarded-For spoofing + * @description The software obtains the client ip through `X-Forwarded-For`, + * and the attacker can modify the value of `X-Forwarded-For` to forge the ip. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/use-of-less-trusted-source + * @tags security + * external/cwe/cwe-348 + */ + +import java +import UseOfLessTrustedSourceLib +import semmle.code.java.dataflow.FlowSources +import DataFlow::PathGraph + +class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration { + UseOfLessTrustedSourceConfig() { this = "UseOfLessTrustedSourceConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof UseOfLessTrustedSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof UseOfLessTrustedSink } + + /** + * When using `,` split request data and not taking the first value of + * the array, it is considered as `good`. + */ + override predicate isSanitizer(DataFlow::Node node) { + exists(ArrayAccess aa, MethodAccess ma | aa.getArray() = ma | + ma.getQualifier() = node.asExpr() and + ma.getMethod() instanceof SplitMethod and + not aa.getIndexExpr().toString() = "0" + ) + } +} + +from DataFlow::PathNode source, DataFlow::PathNode sink, UseOfLessTrustedSourceConfig conf +where + conf.hasFlowPath(source, sink) and + source.getNode().getEnclosingCallable() = sink.getNode().getEnclosingCallable() and + xffIsFirstGet(source.getNode()) +select sink.getNode(), source, sink, "X-Forwarded-For 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/UseOfLessTrustedSourceLib.qll new file mode 100644 index 000000000000..0598f8438404 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -0,0 +1,88 @@ +import java +import DataFlow +import semmle.code.java.dataflow.DataFlow +import experimental.semmle.code.java.Logging + +/** A data flow source of the value of the `x-forwarded-for` field in the `header`. */ +class UseOfLessTrustedSource extends DataFlow::Node { + UseOfLessTrustedSource() { + exists(MethodAccess ma | + ma.getMethod().hasName("getHeader") and + ma.getArgument(0).toString().toLowerCase() = "\"x-forwarded-for\"" and + ma = this.asExpr() + ) + } +} + +/** A data flow sink of method return or log output or local print. */ +class UseOfLessTrustedSink extends DataFlow::Node { + UseOfLessTrustedSink() { + exists(ReturnStmt rs | rs.getResult() = this.asExpr()) + or + exists(MethodAccess ma | + ma.getMethod().getName() in ["print", "println"] and + ( + ma.getMethod().getDeclaringType().hasQualifiedName("java.io", "PrintWriter") or + ma.getMethod().getDeclaringType().hasQualifiedName("java.io", "PrintStream") + ) and + ma.getAnArgument() = this.asExpr() + ) + or + exists(LoggingCall lc | lc.getAnArgument() = this.asExpr()) + } +} + +/** A method that split string. */ +class SplitMethod extends Method { + SplitMethod() { + this.getNumberOfParameters() = 1 and + this.hasQualifiedName("java.lang", "String", "split") + } +} + +/** + * A call to the ServletRequest.getHeader method and the argument are + * `wl-proxy-client-ip`/`proxy-client-ip`/`http_client_ip`/`http_x_forwarded_for`/`x-real-ip`. + */ +class HeaderIpCall extends MethodAccess { + HeaderIpCall() { + this.getMethod().hasName("getHeader") and + this.getMethod() + .getDeclaringType() + .getASupertype*() + .hasQualifiedName("javax.servlet", "ServletRequest") and + this.getArgument(0).toString().toLowerCase() in [ + "\"wl-proxy-client-ip\"", "\"proxy-client-ip\"", "\"http_client_ip\"", + "\"http_x_forwarded_for\"", "\"x-real-ip\"" + ] + } +} + +/** A call to `ServletRequest.getRemoteAddr` method. */ +class RemoteAddrCall extends MethodAccess { + RemoteAddrCall() { + this.getMethod().hasName("getRemoteAddr") and + this.getMethod() + .getDeclaringType() + .getASupertype*() + .hasQualifiedName("javax.servlet", "ServletRequest") + } +} + +/** The first one in the method to get the ip value through `x-forwarded-for`. */ +predicate xffIsFirstGet(Node node) { + exists(HeaderIpCall hic | + node.getEnclosingCallable() = hic.getEnclosingCallable() and + node.getLocation().getEndLine() < hic.getLocation().getEndLine() + ) + or + exists(RemoteAddrCall rac | + node.getEnclosingCallable() = rac.getEnclosingCallable() and + node.getLocation().getEndLine() < rac.getLocation().getEndLine() + ) + or + not exists(HeaderIpCall hic, RemoteAddrCall rac | + node.getEnclosingCallable() = hic.getEnclosingCallable() and + node.getEnclosingCallable() = rac.getEnclosingCallable() + ) +} 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 new file mode 100644 index 000000000000..a71ad9629ceb --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected @@ -0,0 +1,14 @@ +edges +| UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) : String | UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | +| UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:32:60:32:61 | ip | +| UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | +nodes +| UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | semmle.label | remoteAddr | +| UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| UseOfLessTrustedSource.java:32:60:32:61 | ip | semmle.label | ip | +| UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | semmle.label | ... + ... | +#select +| UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) : String | UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | X-Forwarded-For spoofing might include code from $@. | UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:32:60:32:61 | ip | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:32:60:32:61 | ip | X-Forwarded-For spoofing might include code from $@. | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | X-Forwarded-For spoofing might include code from $@. | UseOfLessTrustedSource.java:30:21:30:56 | 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/UseOfLessTrustedSource.java new file mode 100644 index 000000000000..d338134ed3ed --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java @@ -0,0 +1,74 @@ +import javax.servlet.http.HttpServletRequest; +import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.ResponseBody; + +@Controller +public class UseOfLessTrustedSource { + + private static final Logger log = LoggerFactory.getLogger(UseOfLessTrustedSource.class); + + @GetMapping(value = "bad1") + @ResponseBody + public String bad1(HttpServletRequest request) { + String remoteAddr = ""; + if (request != null) { + remoteAddr = request.getHeader("X-FORWARDED-FOR"); + remoteAddr = remoteAddr.split(",")[0]; + if (remoteAddr == null || "".equals(remoteAddr)) { + remoteAddr = request.getRemoteAddr(); + } + } + return remoteAddr; + } + + @GetMapping(value = "bad2") + public void bad2(HttpServletRequest request) { + String ip = request.getHeader("X-Forwarded-For"); + + log.debug("getClientIP header X-Forwarded-For:{}", ip); + + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getHeader("Proxy-Client-IP"); + log.debug("getClientIP header Proxy-Client-IP:{}", ip); + } + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getHeader("WL-Proxy-Client-IP"); + log.debug("getClientIP header WL-Proxy-Client-IP:{}", ip); + } + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getHeader("HTTP_CLIENT_IP"); + log.debug("getClientIP header HTTP_CLIENT_IP:{}", ip); + } + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getHeader("HTTP_X_FORWARDED_FOR"); + log.debug("getClientIP header HTTP_X_FORWARDED_FOR:{}", ip); + } + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getHeader("X-Real-IP"); + log.debug("getClientIP header X-Real-IP:{}", ip); + } + if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { + ip = request.getRemoteAddr(); + log.debug("getRemoteAddr IP:{}", ip); + } + System.out.println("client ip is: " + ip); + } + + @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(); + } + } + return remoteAddr; + } +} \ No newline at end of file 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 new file mode 100644 index 000000000000..3c547a2c8714 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/options b/java/ql/test/experimental/query-tests/security/CWE-348/options new file mode 100644 index 000000000000..40bcee8c133d --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-348/options @@ -0,0 +1,2 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/:${tes +tdir}/../../../../stubs/slf4j-api-1.6.4/:${testdir}/../../../../stubs/apache-commons-lang3-3.7/ \ No newline at end of file diff --git a/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Logger.java b/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Logger.java new file mode 100644 index 000000000000..1d8b7ea23189 --- /dev/null +++ b/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Logger.java @@ -0,0 +1,127 @@ +package org.slf4j; + +public interface Logger { + String ROOT_LOGGER_NAME = "ROOT"; + + String getName(); + + boolean isTraceEnabled(); + + void trace(String var1); + + void trace(String var1, Object var2); + + void trace(String var1, Object var2, Object var3); + + void trace(String var1, Object[] var2); + + void trace(String var1, Throwable var2); + + boolean isTraceEnabled(Marker var1); + + void trace(Marker var1, String var2); + + void trace(Marker var1, String var2, Object var3); + + void trace(Marker var1, String var2, Object var3, Object var4); + + void trace(Marker var1, String var2, Object[] var3); + + void trace(Marker var1, String var2, Throwable var3); + + boolean isDebugEnabled(); + + void debug(String var1); + + void debug(String var1, Object var2); + + void debug(String var1, Object var2, Object var3); + + void debug(String var1, Object[] var2); + + void debug(String var1, Throwable var2); + + boolean isDebugEnabled(Marker var1); + + void debug(Marker var1, String var2); + + void debug(Marker var1, String var2, Object var3); + + void debug(Marker var1, String var2, Object var3, Object var4); + + void debug(Marker var1, String var2, Object[] var3); + + void debug(Marker var1, String var2, Throwable var3); + + boolean isInfoEnabled(); + + void info(String var1); + + void info(String var1, Object var2); + + void info(String var1, Object var2, Object var3); + + void info(String var1, Object[] var2); + + void info(String var1, Throwable var2); + + boolean isInfoEnabled(Marker var1); + + void info(Marker var1, String var2); + + void info(Marker var1, String var2, Object var3); + + void info(Marker var1, String var2, Object var3, Object var4); + + void info(Marker var1, String var2, Object[] var3); + + void info(Marker var1, String var2, Throwable var3); + + boolean isWarnEnabled(); + + void warn(String var1); + + void warn(String var1, Object var2); + + void warn(String var1, Object[] var2); + + void warn(String var1, Object var2, Object var3); + + void warn(String var1, Throwable var2); + + boolean isWarnEnabled(Marker var1); + + void warn(Marker var1, String var2); + + void warn(Marker var1, String var2, Object var3); + + void warn(Marker var1, String var2, Object var3, Object var4); + + void warn(Marker var1, String var2, Object[] var3); + + void warn(Marker var1, String var2, Throwable var3); + + boolean isErrorEnabled(); + + void error(String var1); + + void error(String var1, Object var2); + + void error(String var1, Object var2, Object var3); + + void error(String var1, Object[] var2); + + void error(String var1, Throwable var2); + + boolean isErrorEnabled(Marker var1); + + void error(Marker var1, String var2); + + void error(Marker var1, String var2, Object var3); + + void error(Marker var1, String var2, Object var3, Object var4); + + void error(Marker var1, String var2, Object[] var3); + + void error(Marker var1, String var2, Throwable var3); +} \ No newline at end of file diff --git a/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/LoggerFactory.java b/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/LoggerFactory.java new file mode 100644 index 000000000000..21ef158130a8 --- /dev/null +++ b/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/LoggerFactory.java @@ -0,0 +1,21 @@ +package org.slf4j; + +import java.io.IOException; +import java.net.URL; +import java.util.Arrays; +import java.util.Enumeration; +import java.util.Iterator; +import java.util.LinkedHashSet; +import java.util.List; + +public final class LoggerFactory { + + public static Logger getLogger(String name) { + return null; + } + + public static Logger getLogger(Class clazz) { + return null; + } + +} \ No newline at end of file diff --git a/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Marker.java b/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Marker.java new file mode 100644 index 000000000000..e6ac4846a791 --- /dev/null +++ b/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Marker.java @@ -0,0 +1,30 @@ +package org.slf4j; + +import java.io.Serializable; +import java.util.Iterator; + +public interface Marker extends Serializable { + String ANY_MARKER = "*"; + String ANY_NON_NULL_MARKER = "+"; + + String getName(); + + void add(Marker var1); + + boolean remove(Marker var1); + + /** @deprecated */ + boolean hasChildren(); + + boolean hasReferences(); + + Iterator iterator(); + + boolean contains(Marker var1); + + boolean contains(String var1); + + boolean equals(Object var1); + + int hashCode(); +} diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/core/annotation/AliasFor.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/core/annotation/AliasFor.java new file mode 100644 index 000000000000..edfe917400b7 --- /dev/null +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/core/annotation/AliasFor.java @@ -0,0 +1,21 @@ +package org.springframework.core.annotation; + +import java.lang.annotation.Annotation; +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.METHOD}) +@Documented +public @interface AliasFor { + @AliasFor("attribute") + String value() default ""; + + @AliasFor("value") + String attribute() default ""; + + Class annotation() default Annotation.class; +} diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/stereotype/Controller.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/stereotype/Controller.java index 0bfb67d99c18..9b1751fa2ae3 100644 --- a/java/ql/test/stubs/springframework-5.2.3/org/springframework/stereotype/Controller.java +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/stereotype/Controller.java @@ -1,9 +1,14 @@ package org.springframework.stereotype; -import java.lang.annotation.*; +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; -@Target(value=ElementType.TYPE) -@Retention(value=RetentionPolicy.RUNTIME) +@Target({ElementType.TYPE}) +@Retention(RetentionPolicy.RUNTIME) @Documented -@Component -public @interface Controller { } +public @interface Controller { + String value() default ""; +} diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/ObjectUtils.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/ObjectUtils.java new file mode 100644 index 000000000000..a5a51786d209 --- /dev/null +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/ObjectUtils.java @@ -0,0 +1,202 @@ +package org.springframework.util; + +import java.lang.reflect.Array; +import java.util.Arrays; +import java.util.Collection; +import java.util.Map; +import java.util.Optional; +import java.util.StringJoiner; +import org.springframework.lang.Nullable; + +public abstract class ObjectUtils { + + private static final int INITIAL_HASH = 7; + private static final int MULTIPLIER = 31; + private static final String EMPTY_STRING = ""; + private static final String NULL_STRING = "null"; + private static final String ARRAY_START = "{"; + private static final String ARRAY_END = "}"; + private static final String EMPTY_ARRAY = "{}"; + private static final String ARRAY_ELEMENT_SEPARATOR = ", "; + private static final Object[] EMPTY_OBJECT_ARRAY = new Object[0]; + + public ObjectUtils() { + } + + public static boolean isCheckedException(Throwable ex) { + return false; + } + + public static boolean isCompatibleWithThrowsClause(Throwable ex, @Nullable Class... declaredExceptions) { + return false; + } + + public static boolean isArray(@Nullable Object obj) { + return false; + } + + public static boolean isEmpty(@Nullable Object[] array) { + return false; + } + + public static boolean isEmpty(@Nullable Object obj) { + return false; + } + + @Nullable + public static Object unwrapOptional(@Nullable Object obj) { + return null; + } + + public static boolean containsElement(@Nullable Object[] array, Object element) { + return true; + } + + public static boolean containsConstant(Enum[] enumValues, String constant) { + return true; + } + + public static boolean containsConstant(Enum[] enumValues, String constant, boolean caseSensitive) { + return true; + } + + public static > E caseInsensitiveValueOf(E[] enumValues, String constant) { + return null; + } + + public static A[] addObjectToArray(@Nullable A[] array, @Nullable O obj) { + return null; + } + + public static Object[] toObjectArray(@Nullable Object source) { + return null; + } + + public static boolean nullSafeEquals(@Nullable Object o1, @Nullable Object o2) { + return false; + } + + private static boolean arrayEquals(Object o1, Object o2) { + return false; + } + + public static int nullSafeHashCode(@Nullable Object obj) { + return 1; + } + + public static int nullSafeHashCode(@Nullable Object[] array) { + return 1; + } + + public static int nullSafeHashCode(@Nullable boolean[] array) { + return 1; + } + + public static int nullSafeHashCode(@Nullable byte[] array) { + return 1; + } + + public static int nullSafeHashCode(@Nullable char[] array) { + return 1; + } + + public static int nullSafeHashCode(@Nullable double[] array) { + return 1; + } + + public static int nullSafeHashCode(@Nullable float[] array) { + return 1; + } + + public static int nullSafeHashCode(@Nullable int[] array) { + return 1; + } + + public static int nullSafeHashCode(@Nullable long[] array) { + return 1; + } + + public static int nullSafeHashCode(@Nullable short[] array) { + return 1; + } + + /** @deprecated */ + @Deprecated + public static int hashCode(boolean bool) { + return 1; + } + + /** @deprecated */ + @Deprecated + public static int hashCode(double dbl) { + return 1; + } + + /** @deprecated */ + @Deprecated + public static int hashCode(float flt) { + return 1; + } + + /** @deprecated */ + @Deprecated + public static int hashCode(long lng) { + return 1; + } + + public static String identityToString(@Nullable Object obj) { + return ""; + } + + public static String getIdentityHexString(Object obj) { + return ""; + } + + public static String getDisplayString(@Nullable Object obj) { + return ""; + } + + public static String nullSafeClassName(@Nullable Object obj) { + return ""; + } + + public static String nullSafeToString(@Nullable Object obj) { + return ""; + } + + public static String nullSafeToString(@Nullable Object[] array) { + return ""; + } + + public static String nullSafeToString(@Nullable boolean[] array) { + return ""; + } + + public static String nullSafeToString(@Nullable byte[] array) { + return ""; + } + + public static String nullSafeToString(@Nullable char[] array) { + return ""; + } + + public static String nullSafeToString(@Nullable double[] array) { + return ""; + } + + public static String nullSafeToString(@Nullable float[] array) { + return ""; + } + + public static String nullSafeToString(@Nullable int[] array) { + return ""; + } + + public static String nullSafeToString(@Nullable long[] array) { + return ""; + } + + public static String nullSafeToString(@Nullable short[] array) { + return ""; + } +} diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/StringUtils.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/StringUtils.java new file mode 100644 index 000000000000..a78d47e44c6a --- /dev/null +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/StringUtils.java @@ -0,0 +1,30 @@ +package org.springframework.util; + +import java.io.ByteArrayOutputStream; +import java.nio.charset.Charset; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Deque; +import java.util.Enumeration; +import java.util.Iterator; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Locale; +import java.util.Properties; +import java.util.Set; +import java.util.StringJoiner; +import java.util.StringTokenizer; +import java.util.TimeZone; +import org.springframework.lang.Nullable; + +public abstract class StringUtils { + + @Deprecated + public static boolean isEmpty(@Nullable Object str) { + return true; + } + +} diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/GetMapping.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/GetMapping.java new file mode 100644 index 000000000000..541c7cd4e217 --- /dev/null +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/GetMapping.java @@ -0,0 +1,51 @@ +package org.springframework.web.bind.annotation; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import org.springframework.core.annotation.AliasFor; + +@Target({ElementType.METHOD}) +@Retention(RetentionPolicy.RUNTIME) +@Documented +@RequestMapping( + method = {RequestMethod.GET} +) +public @interface GetMapping { + @AliasFor( + annotation = RequestMapping.class + ) + String name() default ""; + + @AliasFor( + annotation = RequestMapping.class + ) + String[] value() default {}; + + @AliasFor( + annotation = RequestMapping.class + ) + String[] path() default {}; + + @AliasFor( + annotation = RequestMapping.class + ) + String[] params() default {}; + + @AliasFor( + annotation = RequestMapping.class + ) + String[] headers() default {}; + + @AliasFor( + annotation = RequestMapping.class + ) + String[] consumes() default {}; + + @AliasFor( + annotation = RequestMapping.class + ) + String[] produces() default {}; +} diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/Mapping.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/Mapping.java new file mode 100644 index 000000000000..5f269bbcbb87 --- /dev/null +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/Mapping.java @@ -0,0 +1,4 @@ +package org.springframework.web.bind.annotation; + +public @interface Mapping { +} diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/RequestMapping.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/RequestMapping.java index 73ff8d1b03af..093065ca5f16 100644 --- a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/RequestMapping.java +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/RequestMapping.java @@ -1,11 +1,32 @@ package org.springframework.web.bind.annotation; -import java.lang.annotation.*; +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import org.springframework.core.annotation.AliasFor; @Target(value={ElementType.METHOD,ElementType.TYPE}) @Retention(value=RetentionPolicy.RUNTIME) @Documented +@Mapping public @interface RequestMapping { + String name() default ""; + + @AliasFor("path") + String[] value() default {}; + + @AliasFor("value") + String[] path() default {}; RequestMethod[] method() default {}; + + String[] params() default {}; + + String[] headers() default {}; + + String[] consumes() default {}; + + String[] produces() default {}; } diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/RequestParam.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/RequestParam.java index 5ae52ad123fa..56094811c376 100644 --- a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/RequestParam.java +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/RequestParam.java @@ -1,8 +1,23 @@ package org.springframework.web.bind.annotation; -import java.lang.annotation.*; +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import org.springframework.core.annotation.AliasFor; -@Target(value=ElementType.PARAMETER) -@Retention(value=RetentionPolicy.RUNTIME) +@Target({ElementType.PARAMETER}) +@Retention(RetentionPolicy.RUNTIME) @Documented -public @interface RequestParam { } +public @interface RequestParam { + @AliasFor("name") + String value() default ""; + + @AliasFor("value") + String name() default ""; + + boolean required() default true; + + String defaultValue() default "\n\t\t\n\t\t\n\ue000\ue001\ue002\n\t\t\t\t\n"; +} diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/ResponseBody.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/ResponseBody.java new file mode 100644 index 000000000000..4f21b6daaaf0 --- /dev/null +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/bind/annotation/ResponseBody.java @@ -0,0 +1,13 @@ +package org.springframework.web.bind.annotation; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +@Target({ElementType.TYPE, ElementType.METHOD}) +@Retention(RetentionPolicy.RUNTIME) +@Documented +public @interface ResponseBody { +} From 86ef2588f1599cbf0c1ed578676b0e51728823d6 Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 8 Apr 2021 17:55:29 +0800 Subject: [PATCH 02/28] Restore @Component annotation --- .../org/springframework/stereotype/Controller.java | 1 + 1 file changed, 1 insertion(+) diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/stereotype/Controller.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/stereotype/Controller.java index 9b1751fa2ae3..a4a49fffb90a 100644 --- a/java/ql/test/stubs/springframework-5.2.3/org/springframework/stereotype/Controller.java +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/stereotype/Controller.java @@ -9,6 +9,7 @@ @Target({ElementType.TYPE}) @Retention(RetentionPolicy.RUNTIME) @Documented +@Component public @interface Controller { String value() default ""; } From 21004006d652b82213ff502bc3609b57afa626a9 Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 8 Apr 2021 19:17:04 +0800 Subject: [PATCH 03/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index 0598f8438404..a3204dc109a4 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -41,7 +41,7 @@ class SplitMethod extends Method { } /** - * A call to the ServletRequest.getHeader method and the argument are + * A call to the ServletRequest.getHeader method with an argument * `wl-proxy-client-ip`/`proxy-client-ip`/`http_client_ip`/`http_x_forwarded_for`/`x-real-ip`. */ class HeaderIpCall extends MethodAccess { From bfbfe7af13617476188fa08f69d5f670727c721f Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 8 Apr 2021 19:21:58 +0800 Subject: [PATCH 04/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index 1c3a4485cbd8..031aa0d11fdb 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -24,9 +24,8 @@ and get the last value of the split array.

    -
  • Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring: - -Prevent IP address spoofing with... +
  • Dennis Schneider: +Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring
  • Security Rule Zero: A Warning about X-Forwarded-For: From 1da48ed4d10b1ed7ed9585c647db3094f1d813e3 Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 8 Apr 2021 19:22:14 +0800 Subject: [PATCH 05/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index 031aa0d11fdb..6b2eb2e7eed1 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -28,9 +28,7 @@ and get the last value of the split array.

    Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring
    • -
  • Security Rule Zero: A Warning about X-Forwarded-For: - -Security Rule Zero: A Warning about X-Forwarded-For +
  • Security Rule Zero: A Warning about X-Forwarded-For
    • From 8296abcea84e89166608fa603025101c4c8eca47 Mon Sep 17 00:00:00 2001 From: haby0 Date: Mon, 19 Apr 2021 20:59:47 +0800 Subject: [PATCH 06/28] Fix Modify the ql query (the qhelp part is not modified). --- .../CWE/CWE-348/UseOfLessTrustedSource.java | 38 +++-- .../CWE/CWE-348/UseOfLessTrustedSource.qhelp | 9 +- .../CWE/CWE-348/UseOfLessTrustedSource.ql | 27 ++-- .../CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 146 +++++++++++------- .../CWE-348/UseOfLessTrustedSource.expected | 56 +++++-- .../CWE-348/UseOfLessTrustedSource.java | 38 +++-- .../query-tests/security/CWE-348/options | 3 +- .../beans/factory/annotation/Autowired.java | 14 ++ 8 files changed, 213 insertions(+), 118 deletions(-) create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/beans/factory/annotation/Autowired.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java index d338134ed3ed..22d7100223ce 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java @@ -2,6 +2,7 @@ import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.ResponseBody; @@ -11,24 +12,13 @@ public class UseOfLessTrustedSource { private static final Logger log = LoggerFactory.getLogger(UseOfLessTrustedSource.class); - @GetMapping(value = "bad1") - @ResponseBody - public String bad1(HttpServletRequest request) { - String remoteAddr = ""; - if (request != null) { - remoteAddr = request.getHeader("X-FORWARDED-FOR"); - remoteAddr = remoteAddr.split(",")[0]; - if (remoteAddr == null || "".equals(remoteAddr)) { - remoteAddr = request.getRemoteAddr(); - } - } - return remoteAddr; - } + @Autowired + private HttpServletRequest request; - @GetMapping(value = "bad2") - public void bad2(HttpServletRequest request) { + @GetMapping(value = "bad1") + public void bad1(HttpServletRequest request) { String ip = request.getHeader("X-Forwarded-For"); - + log.debug("getClientIP header X-Forwarded-For:{}", ip); if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { @@ -58,6 +48,14 @@ public void bad2(HttpServletRequest request) { System.out.println("client ip is: " + ip); } + @GetMapping(value = "bad2") + public void bad2(HttpServletRequest request) { + String ip = getClientIP(); + if (!StringUtils.startsWith(ip, "192.168.")) { + new Exception("ip illegal"); + } + } + @GetMapping(value = "good1") @ResponseBody public String good1(HttpServletRequest request) { @@ -71,4 +69,12 @@ public String good1(HttpServletRequest request) { } return remoteAddr; } + + protected String getClientIP() { + String xfHeader = request.getHeader("X-Forwarded-For"); + if (xfHeader == null) { + return request.getRemoteAddr(); + } + return xfHeader.split(",")[0]; + } } \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index 1c3a4485cbd8..6b2eb2e7eed1 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -24,14 +24,11 @@ and get the last value of the split array.

    -
  • Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring: - -Prevent IP address spoofing with... +
  • Dennis Schneider: +Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring
    • -
  • Security Rule Zero: A Warning about X-Forwarded-For: - -Security Rule Zero: A Warning about X-Forwarded-For +
  • Security Rule Zero: A Warning about X-Forwarded-For
    • diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql index 15d665d37abd..3c3feeb8f031 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql @@ -1,20 +1,23 @@ /** - * @name X-Forwarded-For spoofing + * @name IP address spoofing * @description The software obtains the client ip through `X-Forwarded-For`, * and the attacker can modify the value of `X-Forwarded-For` to forge the ip. * @kind path-problem * @problem.severity error * @precision high - * @id java/use-of-less-trusted-source + * @id java/ip-address-spoofing * @tags security * external/cwe/cwe-348 */ import java import UseOfLessTrustedSourceLib +import semmle.code.java.dataflow.DataFlow2 +import semmle.code.java.dataflow.TaintTracking2 import semmle.code.java.dataflow.FlowSources import DataFlow::PathGraph +/** Taint-tracking configuration tracing flow from get method request sources to output jsonp data. */ class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration { UseOfLessTrustedSourceConfig() { this = "UseOfLessTrustedSourceConfig" } @@ -23,22 +26,26 @@ class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof UseOfLessTrustedSink } /** - * When using `,` split request data and not taking the first value of - * the array, it is considered as `good`. + * When using `,` split request data and not taking the first value of the array, it is considered as `good`. */ override predicate isSanitizer(DataFlow::Node node) { exists(ArrayAccess aa, MethodAccess ma | aa.getArray() = ma | ma.getQualifier() = node.asExpr() and ma.getMethod() instanceof SplitMethod and - not aa.getIndexExpr().toString() = "0" + not aa.getIndexExpr().(CompileTimeConstantExpr).getIntValue() = 0 + ) + } + + override predicate isAdditionalTaintStep(DataFlow::Node prod, DataFlow::Node succ) { + exists(MethodAccess ma | + ma.getAnArgument() = prod.asExpr() and + ma = succ.asExpr() and + ma.getMethod().getReturnType() instanceof BooleanType ) } } from DataFlow::PathNode source, DataFlow::PathNode sink, UseOfLessTrustedSourceConfig conf -where - conf.hasFlowPath(source, sink) and - source.getNode().getEnclosingCallable() = sink.getNode().getEnclosingCallable() and - xffIsFirstGet(source.getNode()) -select sink.getNode(), source, sink, "X-Forwarded-For spoofing might include code from $@.", +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/UseOfLessTrustedSourceLib.qll index 0598f8438404..ac72be64f5d5 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -1,24 +1,105 @@ import java import DataFlow -import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.TaintTracking2 +import semmle.code.java.security.QueryInjection import experimental.semmle.code.java.Logging -/** A data flow source of the value of the `x-forwarded-for` field in the `header`. */ +/** + * A data flow source of the client ip obtained according to the remote endpoint identifier specified + * in the header (`X-Forwarded-For`, `X-Real-IP`, `Proxy-Client-IP`, etc.). + * + * For example: `ServletRequest.getHeader("X-Forwarded-For")`. + */ class UseOfLessTrustedSource extends DataFlow::Node { UseOfLessTrustedSource() { exists(MethodAccess ma | ma.getMethod().hasName("getHeader") and - ma.getArgument(0).toString().toLowerCase() = "\"x-forwarded-for\"" and + ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ + "x-forwarded-for", "x-real-ip", "proxy-client-ip", "wl-proxy-client-ip", + "http_x_forwarded_for", "http_x_forwarded", "http_x_cluster_client_ip", "http_client_ip", + "http_forwarded_for", "http_forwarded", "http_via", "remote_addr" + ] and ma = this.asExpr() ) } } -/** A data flow sink of method return or log output or local print. */ -class UseOfLessTrustedSink extends DataFlow::Node { - UseOfLessTrustedSink() { - exists(ReturnStmt rs | rs.getResult() = this.asExpr()) - or +/** A data flow sink for ip address forgery vulnerabilities. */ +abstract class UseOfLessTrustedSink extends DataFlow::Node { } + +/** + * A data flow sink for the if condition, which does not include the null judgment of the remote client ip address. + * + * 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. + * `if (remoteAddr == null || "".equals(remoteAddr)) {...` judging whether the client ip is a null value, + * it needs to be excluded + */ +private class IfConditionSink extends UseOfLessTrustedSink { + IfConditionSink() { + exists(IfStmt is | + is.getCondition() = this.asExpr() and + not exists(EQExpr eqe | + eqe.getAnOperand() instanceof NullLiteral and + is.getCondition() = eqe.getParent*() + ) and + not exists(NEExpr nee | + nee.getAnOperand() instanceof NullLiteral and + is.getCondition() = nee.getParent*() + ) and + not exists(MethodAccess ma | + ma.getMethod().hasName("equals") and + ma.getMethod().getNumberOfParameters() = 1 and + ( + ma.getQualifier().(CompileTimeConstantExpr).getStringValue() = "" or + ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "" + ) and + is.getCondition() = ma.getParent*() + ) and + not exists(MethodAccess ma | + ma.getMethod().hasName("equalsIgnoreCase") and + ma.getMethod().getNumberOfParameters() = 1 and + ( + ma.getQualifier().(CompileTimeConstantExpr).getStringValue() = "unknown" or + ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "unknown" + ) and + is.getCondition() = ma.getParent*() + ) and + not exists(MethodAccess ma | + ma.getMethod().getName() in ["isEmpty", "isNotEmpty"] and + ma.getMethod().getNumberOfParameters() = 1 and + is.getCondition() = ma.getParent*() + ) and + not exists(MethodAccess ma | + ( + ma.getMethod().hasQualifiedName("org.apache.commons.lang3", "StringUtils", "isBlank") or + ma.getMethod().hasQualifiedName("org.apache.commons.lang3", "StringUtils", "isNotBlank") + ) and + is.getCondition() = ma.getParent*() + ) and + not exists(MethodAccess ma | + ma.getMethod() + .hasQualifiedName("org.apache.commons.lang3", "StringUtils", "equalsIgnoreCase") and + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "unknown" and + is.getCondition() = ma.getParent*() + ) + ) + } +} + +/** A data flow sink for sql operation. */ +private class SqlOperationSink extends UseOfLessTrustedSink { + SqlOperationSink() { this instanceof QueryInjectionSink } +} + +/** A data flow sink for log operation. */ +private class LogOperationSink extends UseOfLessTrustedSink { + LogOperationSink() { exists(LoggingCall lc | lc.getAnArgument() = this.asExpr()) } +} + +/** A data flow sink for local output. */ +private class PrintSink extends UseOfLessTrustedSink { + PrintSink() { exists(MethodAccess ma | ma.getMethod().getName() in ["print", "println"] and ( @@ -27,8 +108,6 @@ class UseOfLessTrustedSink extends DataFlow::Node { ) and ma.getAnArgument() = this.asExpr() ) - or - exists(LoggingCall lc | lc.getAnArgument() = this.asExpr()) } } @@ -39,50 +118,3 @@ class SplitMethod extends Method { this.hasQualifiedName("java.lang", "String", "split") } } - -/** - * A call to the ServletRequest.getHeader method and the argument are - * `wl-proxy-client-ip`/`proxy-client-ip`/`http_client_ip`/`http_x_forwarded_for`/`x-real-ip`. - */ -class HeaderIpCall extends MethodAccess { - HeaderIpCall() { - this.getMethod().hasName("getHeader") and - this.getMethod() - .getDeclaringType() - .getASupertype*() - .hasQualifiedName("javax.servlet", "ServletRequest") and - this.getArgument(0).toString().toLowerCase() in [ - "\"wl-proxy-client-ip\"", "\"proxy-client-ip\"", "\"http_client_ip\"", - "\"http_x_forwarded_for\"", "\"x-real-ip\"" - ] - } -} - -/** A call to `ServletRequest.getRemoteAddr` method. */ -class RemoteAddrCall extends MethodAccess { - RemoteAddrCall() { - this.getMethod().hasName("getRemoteAddr") and - this.getMethod() - .getDeclaringType() - .getASupertype*() - .hasQualifiedName("javax.servlet", "ServletRequest") - } -} - -/** The first one in the method to get the ip value through `x-forwarded-for`. */ -predicate xffIsFirstGet(Node node) { - exists(HeaderIpCall hic | - node.getEnclosingCallable() = hic.getEnclosingCallable() and - node.getLocation().getEndLine() < hic.getLocation().getEndLine() - ) - or - exists(RemoteAddrCall rac | - node.getEnclosingCallable() = rac.getEnclosingCallable() and - node.getLocation().getEndLine() < rac.getLocation().getEndLine() - ) - or - not exists(HeaderIpCall hic, RemoteAddrCall rac | - node.getEnclosingCallable() = hic.getEnclosingCallable() and - node.getEnclosingCallable() = rac.getEnclosingCallable() - ) -} 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 index a71ad9629ceb..65bbc8d218b5 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected @@ -1,14 +1,48 @@ edges -| UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) : String | UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | -| UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:32:60:32:61 | ip | -| UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | +| UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) : String | UseOfLessTrustedSource.java:22:60:22:61 | ip | +| UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | +| UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) : String | UseOfLessTrustedSource.java:26:64:26:65 | ip | +| UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | +| UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) : String | UseOfLessTrustedSource.java:30:67:30:68 | ip | +| UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | +| UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | UseOfLessTrustedSource.java:34:63:34:64 | ip | +| UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | +| UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | UseOfLessTrustedSource.java:38:69:38:70 | ip | +| UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | +| UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:42:58:42:59 | ip | +| UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | +| UseOfLessTrustedSource.java:53:21:53:33 | getClientIP(...) : String | UseOfLessTrustedSource.java:54:13:54:51 | !... | +| UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) : String | UseOfLessTrustedSource.java:78:16:78:37 | ...[...] : String | +| UseOfLessTrustedSource.java:78:16:78:37 | ...[...] : String | UseOfLessTrustedSource.java:53:21:53:33 | getClientIP(...) : String | nodes -| UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | semmle.label | remoteAddr | -| UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:32:60:32:61 | ip | semmle.label | ip | -| UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | semmle.label | ... + ... | +| UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| UseOfLessTrustedSource.java:22:60:22:61 | ip | semmle.label | ip | +| UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| UseOfLessTrustedSource.java:26:64:26:65 | ip | semmle.label | ip | +| UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| UseOfLessTrustedSource.java:30:67:30:68 | ip | semmle.label | ip | +| UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| UseOfLessTrustedSource.java:34:63:34:64 | ip | semmle.label | ip | +| UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| UseOfLessTrustedSource.java:38:69:38:70 | ip | semmle.label | ip | +| UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| UseOfLessTrustedSource.java:42:58:42:59 | ip | semmle.label | ip | +| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | semmle.label | ... + ... | +| UseOfLessTrustedSource.java:53:21:53:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String | +| UseOfLessTrustedSource.java:54:13:54:51 | !... | semmle.label | !... | +| UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| UseOfLessTrustedSource.java:78:16:78:37 | ...[...] : String | semmle.label | ...[...] : String | #select -| UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) : String | UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | X-Forwarded-For spoofing might include code from $@. | UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:32:60:32:61 | ip | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:32:60:32:61 | ip | X-Forwarded-For spoofing might include code from $@. | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | X-Forwarded-For spoofing might include code from $@. | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:22:60:22:61 | ip | UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) : String | UseOfLessTrustedSource.java:22:60:22:61 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:26:64:26:65 | ip | UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) : String | UseOfLessTrustedSource.java:26:64:26:65 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:30:67:30:68 | ip | UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) : String | UseOfLessTrustedSource.java:30:67:30:68 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:34:63:34:64 | ip | UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | UseOfLessTrustedSource.java:34:63:34:64 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:38:69:38:70 | ip | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | UseOfLessTrustedSource.java:38:69:38:70 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:42:58:42:59 | ip | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:42:58:42:59 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:54:13:54:51 | !... | UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) : String | UseOfLessTrustedSource.java:54:13:54:51 | !... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:74:27:74: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/UseOfLessTrustedSource.java index d338134ed3ed..22d7100223ce 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java +++ b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java @@ -2,6 +2,7 @@ import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.ResponseBody; @@ -11,24 +12,13 @@ public class UseOfLessTrustedSource { private static final Logger log = LoggerFactory.getLogger(UseOfLessTrustedSource.class); - @GetMapping(value = "bad1") - @ResponseBody - public String bad1(HttpServletRequest request) { - String remoteAddr = ""; - if (request != null) { - remoteAddr = request.getHeader("X-FORWARDED-FOR"); - remoteAddr = remoteAddr.split(",")[0]; - if (remoteAddr == null || "".equals(remoteAddr)) { - remoteAddr = request.getRemoteAddr(); - } - } - return remoteAddr; - } + @Autowired + private HttpServletRequest request; - @GetMapping(value = "bad2") - public void bad2(HttpServletRequest request) { + @GetMapping(value = "bad1") + public void bad1(HttpServletRequest request) { String ip = request.getHeader("X-Forwarded-For"); - + log.debug("getClientIP header X-Forwarded-For:{}", ip); if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { @@ -58,6 +48,14 @@ public void bad2(HttpServletRequest request) { System.out.println("client ip is: " + ip); } + @GetMapping(value = "bad2") + public void bad2(HttpServletRequest request) { + String ip = getClientIP(); + if (!StringUtils.startsWith(ip, "192.168.")) { + new Exception("ip illegal"); + } + } + @GetMapping(value = "good1") @ResponseBody public String good1(HttpServletRequest request) { @@ -71,4 +69,12 @@ public String good1(HttpServletRequest request) { } return remoteAddr; } + + protected String getClientIP() { + String xfHeader = request.getHeader("X-Forwarded-For"); + if (xfHeader == null) { + return request.getRemoteAddr(); + } + return xfHeader.split(",")[0]; + } } \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/options b/java/ql/test/experimental/query-tests/security/CWE-348/options index 40bcee8c133d..7edf075ba8e8 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-348/options +++ b/java/ql/test/experimental/query-tests/security/CWE-348/options @@ -1,2 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/:${tes -tdir}/../../../../stubs/slf4j-api-1.6.4/:${testdir}/../../../../stubs/apache-commons-lang3-3.7/ \ No newline at end of file +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/:${testdir}/../../../../stubs/slf4j-api-1.6.4/:${testdir}/../../../../stubs/apache-commons-lang3-3.7/ \ No newline at end of file diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/beans/factory/annotation/Autowired.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/beans/factory/annotation/Autowired.java new file mode 100644 index 000000000000..8fcef9793603 --- /dev/null +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/beans/factory/annotation/Autowired.java @@ -0,0 +1,14 @@ +package org.springframework.beans.factory.annotation; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +@Target({ElementType.CONSTRUCTOR, ElementType.METHOD, ElementType.PARAMETER, ElementType.FIELD, ElementType.ANNOTATION_TYPE}) +@Retention(RetentionPolicy.RUNTIME) +@Documented +public @interface Autowired { + boolean required() default true; +} \ No newline at end of file From 00531588840481bfa6f67692e555d1203f09b359 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 20 Apr 2021 10:58:54 +0800 Subject: [PATCH 07/28] update qhelp file and ql comments --- .../CWE/CWE-348/UseOfLessTrustedSource.qhelp | 12 +++++++----- .../Security/CWE/CWE-348/UseOfLessTrustedSource.ql | 11 ++++++----- .../CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 3 +-- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index 6b2eb2e7eed1..48923b85f025 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -3,8 +3,9 @@ "qhelp.dtd"> -

    The software obtains the original client IP address through the http header X-Forwarded-For, which is used to ensure -security or track it in the log for statistical or other reasons. Attackers can use X-Forwarded-For Spoofing software.

    +

    The software obtains the original client IP address through the http header (X-Forwarded-For or X-Real-IP or Proxy-Client-IP +etc.), which is used to ensure security or track it in the log for statistical or other reasons. Attackers can forge the value of these identifiers to attack the +software.

     @@ -15,9 +16,10 @@ security or track it in the log for statistical or other reasons. Attackers can

    The following examples show the bad case and the good case respectively. Bad case, such as bad1 to bad2. -In the bad1 method, the value of X-Forwarded-For in header is split, and the first value of -the split array is obtained. Good case, such as good1, split the value of X-Forwarded-For in header -and get the last value of the split array.

    +In the bad1 method, obtain the client ip according to the specified identifier from the header for local +output and logging. In the bad2 method, the client ip is obtained and judged according to the specified identifier +from the header. When used for permission verification, it can be bypassed by forging the ip. Good case, such as +good1, split the value of X-Forwarded-For in header and get the last value of the split array.

    diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql index 3c3feeb8f031..df019a7c765d 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql @@ -1,7 +1,8 @@ /** * @name IP address spoofing - * @description The software obtains the client ip through `X-Forwarded-For`, - * and the attacker can modify the value of `X-Forwarded-For` to forge the ip. + * @description The software obtains the client ip from the remote endpoint identifier specified (`X-Forwarded-For`, + * `X-Real-IP`, `Proxy-Client-IP`, etc.) in the header and uses it. Attackers can modify these The value + * of the identifier to forge the client ip. * @kind path-problem * @problem.severity error * @precision high @@ -12,12 +13,12 @@ import java import UseOfLessTrustedSourceLib -import semmle.code.java.dataflow.DataFlow2 -import semmle.code.java.dataflow.TaintTracking2 import semmle.code.java.dataflow.FlowSources import DataFlow::PathGraph -/** Taint-tracking configuration tracing flow from get method request sources to output jsonp data. */ +/** + * Taint-tracking configuration tracing flow from obtain client ip to use the client ip. + */ class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration { UseOfLessTrustedSourceConfig() { this = "UseOfLessTrustedSourceConfig" } diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index ac72be64f5d5..64bf84380922 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -1,12 +1,11 @@ import java import DataFlow -import semmle.code.java.dataflow.TaintTracking2 import semmle.code.java.security.QueryInjection import experimental.semmle.code.java.Logging /** * A data flow source of the client ip obtained according to the remote endpoint identifier specified - * in the header (`X-Forwarded-For`, `X-Real-IP`, `Proxy-Client-IP`, etc.). + * (`X-Forwarded-For`, `X-Real-IP`, `Proxy-Client-IP`, etc.) in the header. * * For example: `ServletRequest.getHeader("X-Forwarded-For")`. */ From b60bffaf83fb8cbd87ea87e76751d783a8a6275a Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 20 Apr 2021 19:31:59 +0800 Subject: [PATCH 08/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index 64bf84380922..98c754c77273 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -102,8 +102,7 @@ private class PrintSink extends UseOfLessTrustedSink { exists(MethodAccess ma | ma.getMethod().getName() in ["print", "println"] and ( - ma.getMethod().getDeclaringType().hasQualifiedName("java.io", "PrintWriter") or - ma.getMethod().getDeclaringType().hasQualifiedName("java.io", "PrintStream") + ma.getMethod().getDeclaringType().hasQualifiedName("java.io", ["PrintWriter", "PrintStream"]) ) and ma.getAnArgument() = this.asExpr() ) From 0b1637a4099416de032b701bd20d812bfaa7560e Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 20 Apr 2021 19:32:39 +0800 Subject: [PATCH 09/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index 48923b85f025..3b56a9650bd0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -3,7 +3,7 @@ "qhelp.dtd"> -

    The software obtains the original client IP address through the http header (X-Forwarded-For or X-Real-IP or Proxy-Client-IP +

    An original client IP address is retrieved from an http header (X-Forwarded-For or X-Real-IP or Proxy-Client-IP etc.), which is used to ensure security or track it in the log for statistical or other reasons. Attackers can forge the value of these identifiers to attack the software.

    From d82878ac3b833bfbc006836f01895fa34bd00542 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 20 Apr 2021 19:33:06 +0800 Subject: [PATCH 10/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index 3b56a9650bd0..e195cda1cdd3 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -4,8 +4,8 @@

    An original client IP address is retrieved from an http header (X-Forwarded-For or X-Real-IP or Proxy-Client-IP -etc.), which is used to ensure security or track it in the log for statistical or other reasons. Attackers can forge the value of these identifiers to attack the -software.

    +etc.), which is used to ensure security or track it in the log for statistical or other reasons. Attackers can forge the value of these identifiers to +bypass a ban-list, for example.

     From 9ece4dac0ff03b68d84e639517686b0721dcba09 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 20 Apr 2021 19:33:47 +0800 Subject: [PATCH 11/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index e195cda1cdd3..c060eaf3e511 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -10,7 +10,7 @@ bypass a ban-list, for example.

     -

    When the software is not using a proxy server, get the last ip.

    +

    Do not trust the values of HTTP headers allegedly identifying the originating IP. If you are aware your application will run behind some reverse proxies then the last entry of a X-Forwarded-For header value may be more trustworthy than the rest of it because some reverse proxies append the IP address they observed to the end of any remote-supplied header.

     From 408dd31d3cf2b35bf524648e621553f6028c0a30 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 20 Apr 2021 19:34:37 +0800 Subject: [PATCH 12/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index c060eaf3e511..15508f049770 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -15,11 +15,10 @@ bypass a ban-list, for example.

     -

    The following examples show the bad case and the good case respectively. Bad case, such as bad1 to bad2. -In the bad1 method, obtain the client ip according to the specified identifier from the header for local -output and logging. In the bad2 method, the client ip is obtained and judged according to the specified identifier -from the header. When used for permission verification, it can be bypassed by forging the ip. Good case, such as -good1, split the value of X-Forwarded-For in header and get the last value of the split array.

    +

    The following examples show the bad case and the good case respectively. +In the bad1 method, the client ip is obtained from an HTTP header for local +output and logging. In the 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.

    From 9e87f4ec4e1e97baafa7b2677aeb87e8dd7d82e7 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 20 Apr 2021 19:35:34 +0800 Subject: [PATCH 13/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSource.ql | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql index df019a7c765d..89c0dbd615a0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql @@ -27,7 +27,8 @@ class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof UseOfLessTrustedSink } /** - * When using `,` split request data and not taking the first value of the array, it is considered as `good`. + * Splitting a header value by `,` and taking an entry other than the first is sanitizing, because + * later entries may originate from more-trustworthy intermediate proxies, not the original client. */ override predicate isSanitizer(DataFlow::Node node) { exists(ArrayAccess aa, MethodAccess ma | aa.getArray() = ma | From b1ee864ad94a24a36245e1796a0c03e13adedfe3 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 20 Apr 2021 19:35:52 +0800 Subject: [PATCH 14/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSource.ql | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql index 89c0dbd615a0..80b5deec5782 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql @@ -1,7 +1,6 @@ /** * @name IP address spoofing - * @description The software obtains the client ip from the remote endpoint identifier specified (`X-Forwarded-For`, - * `X-Real-IP`, `Proxy-Client-IP`, etc.) in the header and uses it. Attackers can modify these The value + * @description A remote endpoint identifier is read from an HTTP header. Attackers can modify the value * of the identifier to forge the client ip. * @kind path-problem * @problem.severity error From 3e376f95c49bdbc9d313d33f268c121f821a1abb Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 20 Apr 2021 19:36:16 +0800 Subject: [PATCH 15/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql Co-authored-by: Chris Smowton --- .../experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql index 80b5deec5782..e1d2a28c9397 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql @@ -16,7 +16,7 @@ import semmle.code.java.dataflow.FlowSources import DataFlow::PathGraph /** - * Taint-tracking configuration tracing flow from obtain client ip to use the client ip. + * 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" } From 84f00c21df471cc39f38f4c5b467895f4b2f7280 Mon Sep 17 00:00:00 2001 From: haby0 Date: Wed, 21 Apr 2021 15:38:41 +0800 Subject: [PATCH 16/28] update IfConditionSink. --- .../CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 94 ++++++++++--------- 1 file changed, 48 insertions(+), 46 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index 98c754c77273..fa9ec4da9bb6 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -38,49 +38,53 @@ private class IfConditionSink extends UseOfLessTrustedSink { IfConditionSink() { exists(IfStmt is | is.getCondition() = this.asExpr() and - not exists(EQExpr eqe | - eqe.getAnOperand() instanceof NullLiteral and - is.getCondition() = eqe.getParent*() - ) and - not exists(NEExpr nee | - nee.getAnOperand() instanceof NullLiteral and - is.getCondition() = nee.getParent*() - ) and - not exists(MethodAccess ma | - ma.getMethod().hasName("equals") and - ma.getMethod().getNumberOfParameters() = 1 and - ( - ma.getQualifier().(CompileTimeConstantExpr).getStringValue() = "" or - ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "" - ) and - is.getCondition() = ma.getParent*() - ) and - not exists(MethodAccess ma | - ma.getMethod().hasName("equalsIgnoreCase") and - ma.getMethod().getNumberOfParameters() = 1 and - ( - ma.getQualifier().(CompileTimeConstantExpr).getStringValue() = "unknown" or - ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "unknown" - ) and - is.getCondition() = ma.getParent*() - ) and - not exists(MethodAccess ma | - ma.getMethod().getName() in ["isEmpty", "isNotEmpty"] and - ma.getMethod().getNumberOfParameters() = 1 and - is.getCondition() = ma.getParent*() - ) and - not exists(MethodAccess ma | - ( - ma.getMethod().hasQualifiedName("org.apache.commons.lang3", "StringUtils", "isBlank") or - ma.getMethod().hasQualifiedName("org.apache.commons.lang3", "StringUtils", "isNotBlank") - ) and - is.getCondition() = ma.getParent*() - ) and - not exists(MethodAccess ma | - ma.getMethod() - .hasQualifiedName("org.apache.commons.lang3", "StringUtils", "equalsIgnoreCase") and - ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "unknown" and - is.getCondition() = ma.getParent*() + ( + exists(MethodAccess ma | + ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().getNumberOfParameters() = 1 and + not ma.getQualifier().(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ + "", "unknown", ":" + ] and + not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ + "", "unknown", ":" + ] and + is.getCondition() = ma.getParent*() + ) + or + exists(MethodAccess ma | + ma.getMethod().hasName("contains") and + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().getNumberOfParameters() = 1 and + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ + "", "unknown" + ] and + is.getCondition() = ma.getParent*() + ) + or + exists(MethodAccess ma | + ma.getMethod().hasName("startsWith") and + ma.getMethod() + .getDeclaringType() + .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], + "StringUtils") and + ma.getMethod().getNumberOfParameters() = 2 and + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() != "" and + is.getCondition() = ma.getParent*() + ) + or + exists(MethodAccess ma | + ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and + ma.getMethod() + .getDeclaringType() + .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], + "StringUtils") and + ma.getMethod().getNumberOfParameters() = 2 and + not ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ + "", "unknown", ":" + ] and + is.getCondition() = ma.getParent*() + ) ) ) } @@ -101,9 +105,7 @@ private class PrintSink extends UseOfLessTrustedSink { PrintSink() { exists(MethodAccess ma | ma.getMethod().getName() in ["print", "println"] and - ( - ma.getMethod().getDeclaringType().hasQualifiedName("java.io", ["PrintWriter", "PrintStream"]) - ) and + ma.getMethod().getDeclaringType().hasQualifiedName("java.io", ["PrintWriter", "PrintStream"]) and ma.getAnArgument() = this.asExpr() ) } From 454324781d6115cbff7eb1f90a2d1bb2d99a9fd4 Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 22 Apr 2021 11:59:33 +0800 Subject: [PATCH 17/28] delete IfStmt --- .../CWE/CWE-348/UseOfLessTrustedSource.ql | 8 -- .../CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 97 +++++++++---------- .../CWE-348/UseOfLessTrustedSource.expected | 6 +- 3 files changed, 48 insertions(+), 63 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql index e1d2a28c9397..3c1d2a33ec12 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql @@ -36,14 +36,6 @@ class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration { not aa.getIndexExpr().(CompileTimeConstantExpr).getIntValue() = 0 ) } - - override predicate isAdditionalTaintStep(DataFlow::Node prod, DataFlow::Node succ) { - exists(MethodAccess ma | - ma.getAnArgument() = prod.asExpr() and - ma = succ.asExpr() and - ma.getMethod().getReturnType() instanceof BooleanType - ) - } } from DataFlow::PathNode source, DataFlow::PathNode sink, UseOfLessTrustedSourceConfig conf diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index fa9ec4da9bb6..29baa90875c0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -27,66 +27,59 @@ class UseOfLessTrustedSource extends DataFlow::Node { abstract class UseOfLessTrustedSink extends DataFlow::Node { } /** - * A data flow sink for the if condition, which does not include the null judgment of the remote client ip address. + * 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. - * `if (remoteAddr == null || "".equals(remoteAddr)) {...` judging whether the client ip is a null value, - * it needs to be excluded */ -private class IfConditionSink extends UseOfLessTrustedSink { - IfConditionSink() { - exists(IfStmt is | - is.getCondition() = this.asExpr() and +private class CompareSink extends UseOfLessTrustedSink { + CompareSink() { + exists(MethodAccess ma | + ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().getNumberOfParameters() = 1 and ( - exists(MethodAccess ma | - ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().getNumberOfParameters() = 1 and - not ma.getQualifier().(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ - "", "unknown", ":" - ] and - not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ - "", "unknown", ":" - ] and - is.getCondition() = ma.getParent*() - ) - or - exists(MethodAccess ma | - ma.getMethod().hasName("contains") and - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().getNumberOfParameters() = 1 and - ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ - "", "unknown" - ] and - is.getCondition() = ma.getParent*() - ) + ma.getArgument(0) = this.asExpr() and + not ma.getQualifier().(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ + "", "unknown", ":" + ] or - exists(MethodAccess ma | - ma.getMethod().hasName("startsWith") and - ma.getMethod() - .getDeclaringType() - .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], - "StringUtils") and - ma.getMethod().getNumberOfParameters() = 2 and - ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() != "" and - is.getCondition() = ma.getParent*() - ) - or - exists(MethodAccess ma | - ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and - ma.getMethod() - .getDeclaringType() - .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], - "StringUtils") and - ma.getMethod().getNumberOfParameters() = 2 and - not ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ - "", "unknown", ":" - ] and - is.getCondition() = ma.getParent*() - ) + ma.getQualifier() = this.asExpr() and + not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ + "", "unknown", ":" + ] ) ) + or + exists(MethodAccess ma | + ma.getMethod().hasName("contains") and + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().getNumberOfParameters() = 1 and + ma.getQualifier() = this.asExpr() and + ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in ["", "unknown"] + ) + or + exists(MethodAccess ma, int i | + ma.getMethod().hasName("startsWith") and + ma.getMethod() + .getDeclaringType() + .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], "StringUtils") and + ma.getMethod().getNumberOfParameters() = 2 and + ma.getArgument(i) = this.asExpr() and + ma.getArgument(1 - i).(CompileTimeConstantExpr).getStringValue() != "" + ) + or + exists(MethodAccess ma, int i | + ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and + ma.getMethod() + .getDeclaringType() + .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], "StringUtils") and + ma.getMethod().getNumberOfParameters() = 2 and + ma.getArgument(i) = this.asExpr() and + not ma.getArgument(1 - i).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ + "", "unknown", ":" + ] + ) } } 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 index 65bbc8d218b5..1ece0b21ee01 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected @@ -11,7 +11,7 @@ edges | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:42:58:42:59 | ip | | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | -| UseOfLessTrustedSource.java:53:21:53:33 | getClientIP(...) : String | UseOfLessTrustedSource.java:54:13:54:51 | !... | +| UseOfLessTrustedSource.java:53:21:53:33 | getClientIP(...) : String | UseOfLessTrustedSource.java:54:37:54:38 | ip | | UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) : String | UseOfLessTrustedSource.java:78:16:78:37 | ...[...] : String | | UseOfLessTrustedSource.java:78:16:78:37 | ...[...] : String | UseOfLessTrustedSource.java:53:21:53:33 | getClientIP(...) : String | nodes @@ -29,7 +29,7 @@ nodes | UseOfLessTrustedSource.java:42:58:42:59 | ip | semmle.label | ip | | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | semmle.label | ... + ... | | UseOfLessTrustedSource.java:53:21:53:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String | -| UseOfLessTrustedSource.java:54:13:54:51 | !... | semmle.label | !... | +| UseOfLessTrustedSource.java:54:37:54:38 | ip | semmle.label | ip | | UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) : String | semmle.label | getHeader(...) : String | | UseOfLessTrustedSource.java:78:16:78:37 | ...[...] : String | semmle.label | ...[...] : String | #select @@ -45,4 +45,4 @@ nodes | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) | this user input | | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) | this user input | | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:54:13:54:51 | !... | UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) : String | UseOfLessTrustedSource.java:54:13:54:51 | !... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:54:37:54:38 | ip | UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) : String | UseOfLessTrustedSource.java:54:37:54:38 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) | this user input | From aaef4ef22b90b60bed45b45c4cd6b5d49562e56c Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 22 Apr 2021 18:52:55 +0800 Subject: [PATCH 18/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index 29baa90875c0..e7f6f17e9aef 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -56,7 +56,7 @@ private class CompareSink extends UseOfLessTrustedSink { ma.getMethod().getDeclaringType() instanceof TypeString and ma.getMethod().getNumberOfParameters() = 1 and ma.getQualifier() = this.asExpr() and - ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in ["", "unknown"] + not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in ["", "unknown"] ) or exists(MethodAccess ma, int i | From 9b4442be8bd23859e8e55bb145250ae1fe87d6ee Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 22 Apr 2021 19:01:55 +0800 Subject: [PATCH 19/28] Fix some errors --- .../CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index 29baa90875c0..796fa9d3fc39 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -56,27 +56,27 @@ private class CompareSink extends UseOfLessTrustedSink { ma.getMethod().getDeclaringType() instanceof TypeString and ma.getMethod().getNumberOfParameters() = 1 and ma.getQualifier() = this.asExpr() and - ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in ["", "unknown"] + not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in ["", "unknown"] ) or - exists(MethodAccess ma, int i | + exists(MethodAccess ma | ma.getMethod().hasName("startsWith") and ma.getMethod() .getDeclaringType() .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], "StringUtils") and ma.getMethod().getNumberOfParameters() = 2 and - ma.getArgument(i) = this.asExpr() and - ma.getArgument(1 - i).(CompileTimeConstantExpr).getStringValue() != "" + ma.getAnArgument() = this.asExpr() and + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() != "" ) or - exists(MethodAccess ma, int i | + exists(MethodAccess ma | ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and ma.getMethod() .getDeclaringType() .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], "StringUtils") and ma.getMethod().getNumberOfParameters() = 2 and - ma.getArgument(i) = this.asExpr() and - not ma.getArgument(1 - i).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ + ma.getAnArgument() = this.asExpr() and + not ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ "", "unknown", ":" ] ) From 407dcea75144aeba01a682df9bef3e705af0b84d Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 22 Apr 2021 19:20:54 +0800 Subject: [PATCH 20/28] add String type startsWith --- .../Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index 796fa9d3fc39..be2aef5e9c7e 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -52,11 +52,13 @@ private class CompareSink extends UseOfLessTrustedSink { ) or exists(MethodAccess ma | - ma.getMethod().hasName("contains") and + ma.getMethod().getName() in ["contains", "startsWith"] and ma.getMethod().getDeclaringType() instanceof TypeString and ma.getMethod().getNumberOfParameters() = 1 and ma.getQualifier() = this.asExpr() and - not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in ["", "unknown"] + not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ + "", "unknown" + ] ) or exists(MethodAccess ma | From 5be9fbbc5a299fa83f37556bdace2e3f87058fe1 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 27 Apr 2021 14:12:33 +0800 Subject: [PATCH 21/28] Remove LogOperationSink and PrintSink --- .../CWE/CWE-348/UseOfLessTrustedSource.java | 37 ---- .../CWE/CWE-348/UseOfLessTrustedSource.qhelp | 3 +- .../CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 16 -- .../CWE-348/UseOfLessTrustedSource.expected | 53 +---- .../CWE-348/UseOfLessTrustedSource.java | 37 ---- .../query-tests/security/CWE-348/options | 2 +- .../slf4j-api-1.6.4/org/slf4j/Logger.java | 127 ----------- .../org/slf4j/LoggerFactory.java | 21 -- .../slf4j-api-1.6.4/org/slf4j/Marker.java | 30 --- .../org/springframework/util/ObjectUtils.java | 202 ------------------ .../org/springframework/util/StringUtils.java | 30 --- 11 files changed, 10 insertions(+), 548 deletions(-) delete mode 100644 java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Logger.java delete mode 100644 java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/LoggerFactory.java delete mode 100644 java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Marker.java delete mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/util/ObjectUtils.java delete mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/util/StringUtils.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java index 22d7100223ce..35a66dc2731d 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java @@ -1,7 +1,5 @@ import javax.servlet.http.HttpServletRequest; import org.apache.commons.lang3.StringUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.GetMapping; @@ -10,46 +8,11 @@ @Controller public class UseOfLessTrustedSource { - private static final Logger log = LoggerFactory.getLogger(UseOfLessTrustedSource.class); - @Autowired private HttpServletRequest request; @GetMapping(value = "bad1") public void bad1(HttpServletRequest request) { - String ip = request.getHeader("X-Forwarded-For"); - - log.debug("getClientIP header X-Forwarded-For:{}", ip); - - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getHeader("Proxy-Client-IP"); - log.debug("getClientIP header Proxy-Client-IP:{}", ip); - } - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getHeader("WL-Proxy-Client-IP"); - log.debug("getClientIP header WL-Proxy-Client-IP:{}", ip); - } - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getHeader("HTTP_CLIENT_IP"); - log.debug("getClientIP header HTTP_CLIENT_IP:{}", ip); - } - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getHeader("HTTP_X_FORWARDED_FOR"); - log.debug("getClientIP header HTTP_X_FORWARDED_FOR:{}", ip); - } - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getHeader("X-Real-IP"); - log.debug("getClientIP header X-Real-IP:{}", ip); - } - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getRemoteAddr(); - log.debug("getRemoteAddr IP:{}", ip); - } - System.out.println("client ip is: " + ip); - } - - @GetMapping(value = "bad2") - public void bad2(HttpServletRequest request) { String ip = getClientIP(); if (!StringUtils.startsWith(ip, "192.168.")) { new Exception("ip illegal"); diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index 15508f049770..6d5404a26836 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -16,8 +16,7 @@ bypass a ban-list, for example.

    The following examples show the bad case and the good case respectively. -In the bad1 method, the client ip is obtained from an HTTP header for local -output and logging. In the 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 +In the bad1 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/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index be2aef5e9c7e..1639d1c82d12 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -90,22 +90,6 @@ private class SqlOperationSink extends UseOfLessTrustedSink { SqlOperationSink() { this instanceof QueryInjectionSink } } -/** A data flow sink for log operation. */ -private class LogOperationSink extends UseOfLessTrustedSink { - LogOperationSink() { exists(LoggingCall lc | lc.getAnArgument() = this.asExpr()) } -} - -/** A data flow sink for local output. */ -private class PrintSink extends UseOfLessTrustedSink { - PrintSink() { - exists(MethodAccess ma | - ma.getMethod().getName() in ["print", "println"] and - ma.getMethod().getDeclaringType().hasQualifiedName("java.io", ["PrintWriter", "PrintStream"]) and - ma.getAnArgument() = this.asExpr() - ) - } -} - /** A method that split string. */ class SplitMethod extends Method { SplitMethod() { 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 index 1ece0b21ee01..df238e4c0a32 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected @@ -1,48 +1,11 @@ edges -| UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) : String | UseOfLessTrustedSource.java:22:60:22:61 | ip | -| UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | -| UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) : String | UseOfLessTrustedSource.java:26:64:26:65 | ip | -| UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | -| UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) : String | UseOfLessTrustedSource.java:30:67:30:68 | ip | -| UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | -| UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | UseOfLessTrustedSource.java:34:63:34:64 | ip | -| UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | -| UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | UseOfLessTrustedSource.java:38:69:38:70 | ip | -| UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | -| UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:42:58:42:59 | ip | -| UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | -| UseOfLessTrustedSource.java:53:21:53:33 | getClientIP(...) : String | UseOfLessTrustedSource.java:54:37:54:38 | ip | -| UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) : String | UseOfLessTrustedSource.java:78:16:78:37 | ...[...] : String | -| UseOfLessTrustedSource.java:78:16:78:37 | ...[...] : String | UseOfLessTrustedSource.java:53:21:53:33 | getClientIP(...) : String | +| UseOfLessTrustedSource.java:16:21:16:33 | getClientIP(...) : String | UseOfLessTrustedSource.java:17:37:17:38 | ip | +| UseOfLessTrustedSource.java:37:27:37:62 | getHeader(...) : String | UseOfLessTrustedSource.java:41:16:41:37 | ...[...] : String | +| UseOfLessTrustedSource.java:41:16:41:37 | ...[...] : String | UseOfLessTrustedSource.java:16:21:16:33 | getClientIP(...) : String | nodes -| UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:22:60:22:61 | ip | semmle.label | ip | -| UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:26:64:26:65 | ip | semmle.label | ip | -| UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:30:67:30:68 | ip | semmle.label | ip | -| UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:34:63:34:64 | ip | semmle.label | ip | -| UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:38:69:38:70 | ip | semmle.label | ip | -| UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:42:58:42:59 | ip | semmle.label | ip | -| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | semmle.label | ... + ... | -| UseOfLessTrustedSource.java:53:21:53:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String | -| UseOfLessTrustedSource.java:54:37:54:38 | ip | semmle.label | ip | -| UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:78:16:78:37 | ...[...] : String | semmle.label | ...[...] : String | +| UseOfLessTrustedSource.java:16:21:16:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String | +| UseOfLessTrustedSource.java:17:37:17:38 | ip | semmle.label | ip | +| UseOfLessTrustedSource.java:37:27:37:62 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| UseOfLessTrustedSource.java:41:16:41:37 | ...[...] : String | semmle.label | ...[...] : String | #select -| UseOfLessTrustedSource.java:22:60:22:61 | ip | UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) : String | UseOfLessTrustedSource.java:22:60:22:61 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:26:64:26:65 | ip | UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) : String | UseOfLessTrustedSource.java:26:64:26:65 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:30:67:30:68 | ip | UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) : String | UseOfLessTrustedSource.java:30:67:30:68 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:34:63:34:64 | ip | UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | UseOfLessTrustedSource.java:34:63:34:64 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:38:69:38:70 | ip | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | UseOfLessTrustedSource.java:38:69:38:70 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:42:58:42:59 | ip | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:42:58:42:59 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:20:21:20:56 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:25:18:25:53 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:29:18:29:56 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:33:18:33:52 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:37:18:37:58 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) : String | UseOfLessTrustedSource.java:48:28:48:48 | ... + ... | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:41:18:41:47 | getHeader(...) | this user input | -| UseOfLessTrustedSource.java:54:37:54:38 | ip | UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) : String | UseOfLessTrustedSource.java:54:37:54:38 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:74:27:74:62 | getHeader(...) | this user input | +| UseOfLessTrustedSource.java:17:37:17:38 | ip | UseOfLessTrustedSource.java:37:27:37:62 | getHeader(...) : String | UseOfLessTrustedSource.java:17:37:17:38 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:37:27:37: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/UseOfLessTrustedSource.java index 22d7100223ce..35a66dc2731d 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java +++ b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java @@ -1,7 +1,5 @@ import javax.servlet.http.HttpServletRequest; import org.apache.commons.lang3.StringUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.GetMapping; @@ -10,46 +8,11 @@ @Controller public class UseOfLessTrustedSource { - private static final Logger log = LoggerFactory.getLogger(UseOfLessTrustedSource.class); - @Autowired private HttpServletRequest request; @GetMapping(value = "bad1") public void bad1(HttpServletRequest request) { - String ip = request.getHeader("X-Forwarded-For"); - - log.debug("getClientIP header X-Forwarded-For:{}", ip); - - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getHeader("Proxy-Client-IP"); - log.debug("getClientIP header Proxy-Client-IP:{}", ip); - } - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getHeader("WL-Proxy-Client-IP"); - log.debug("getClientIP header WL-Proxy-Client-IP:{}", ip); - } - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getHeader("HTTP_CLIENT_IP"); - log.debug("getClientIP header HTTP_CLIENT_IP:{}", ip); - } - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getHeader("HTTP_X_FORWARDED_FOR"); - log.debug("getClientIP header HTTP_X_FORWARDED_FOR:{}", ip); - } - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getHeader("X-Real-IP"); - log.debug("getClientIP header X-Real-IP:{}", ip); - } - if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { - ip = request.getRemoteAddr(); - log.debug("getRemoteAddr IP:{}", ip); - } - System.out.println("client ip is: " + ip); - } - - @GetMapping(value = "bad2") - public void bad2(HttpServletRequest request) { String ip = getClientIP(); if (!StringUtils.startsWith(ip, "192.168.")) { new Exception("ip illegal"); diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/options b/java/ql/test/experimental/query-tests/security/CWE-348/options index 7edf075ba8e8..05f93394f49a 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-348/options +++ b/java/ql/test/experimental/query-tests/security/CWE-348/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/:${testdir}/../../../../stubs/slf4j-api-1.6.4/:${testdir}/../../../../stubs/apache-commons-lang3-3.7/ \ No newline at end of file +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/:${testdir}/../../../../stubs/apache-commons-lang3-3.7/ \ No newline at end of file diff --git a/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Logger.java b/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Logger.java deleted file mode 100644 index 1d8b7ea23189..000000000000 --- a/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Logger.java +++ /dev/null @@ -1,127 +0,0 @@ -package org.slf4j; - -public interface Logger { - String ROOT_LOGGER_NAME = "ROOT"; - - String getName(); - - boolean isTraceEnabled(); - - void trace(String var1); - - void trace(String var1, Object var2); - - void trace(String var1, Object var2, Object var3); - - void trace(String var1, Object[] var2); - - void trace(String var1, Throwable var2); - - boolean isTraceEnabled(Marker var1); - - void trace(Marker var1, String var2); - - void trace(Marker var1, String var2, Object var3); - - void trace(Marker var1, String var2, Object var3, Object var4); - - void trace(Marker var1, String var2, Object[] var3); - - void trace(Marker var1, String var2, Throwable var3); - - boolean isDebugEnabled(); - - void debug(String var1); - - void debug(String var1, Object var2); - - void debug(String var1, Object var2, Object var3); - - void debug(String var1, Object[] var2); - - void debug(String var1, Throwable var2); - - boolean isDebugEnabled(Marker var1); - - void debug(Marker var1, String var2); - - void debug(Marker var1, String var2, Object var3); - - void debug(Marker var1, String var2, Object var3, Object var4); - - void debug(Marker var1, String var2, Object[] var3); - - void debug(Marker var1, String var2, Throwable var3); - - boolean isInfoEnabled(); - - void info(String var1); - - void info(String var1, Object var2); - - void info(String var1, Object var2, Object var3); - - void info(String var1, Object[] var2); - - void info(String var1, Throwable var2); - - boolean isInfoEnabled(Marker var1); - - void info(Marker var1, String var2); - - void info(Marker var1, String var2, Object var3); - - void info(Marker var1, String var2, Object var3, Object var4); - - void info(Marker var1, String var2, Object[] var3); - - void info(Marker var1, String var2, Throwable var3); - - boolean isWarnEnabled(); - - void warn(String var1); - - void warn(String var1, Object var2); - - void warn(String var1, Object[] var2); - - void warn(String var1, Object var2, Object var3); - - void warn(String var1, Throwable var2); - - boolean isWarnEnabled(Marker var1); - - void warn(Marker var1, String var2); - - void warn(Marker var1, String var2, Object var3); - - void warn(Marker var1, String var2, Object var3, Object var4); - - void warn(Marker var1, String var2, Object[] var3); - - void warn(Marker var1, String var2, Throwable var3); - - boolean isErrorEnabled(); - - void error(String var1); - - void error(String var1, Object var2); - - void error(String var1, Object var2, Object var3); - - void error(String var1, Object[] var2); - - void error(String var1, Throwable var2); - - boolean isErrorEnabled(Marker var1); - - void error(Marker var1, String var2); - - void error(Marker var1, String var2, Object var3); - - void error(Marker var1, String var2, Object var3, Object var4); - - void error(Marker var1, String var2, Object[] var3); - - void error(Marker var1, String var2, Throwable var3); -} \ No newline at end of file diff --git a/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/LoggerFactory.java b/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/LoggerFactory.java deleted file mode 100644 index 21ef158130a8..000000000000 --- a/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/LoggerFactory.java +++ /dev/null @@ -1,21 +0,0 @@ -package org.slf4j; - -import java.io.IOException; -import java.net.URL; -import java.util.Arrays; -import java.util.Enumeration; -import java.util.Iterator; -import java.util.LinkedHashSet; -import java.util.List; - -public final class LoggerFactory { - - public static Logger getLogger(String name) { - return null; - } - - public static Logger getLogger(Class clazz) { - return null; - } - -} \ No newline at end of file diff --git a/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Marker.java b/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Marker.java deleted file mode 100644 index e6ac4846a791..000000000000 --- a/java/ql/test/stubs/slf4j-api-1.6.4/org/slf4j/Marker.java +++ /dev/null @@ -1,30 +0,0 @@ -package org.slf4j; - -import java.io.Serializable; -import java.util.Iterator; - -public interface Marker extends Serializable { - String ANY_MARKER = "*"; - String ANY_NON_NULL_MARKER = "+"; - - String getName(); - - void add(Marker var1); - - boolean remove(Marker var1); - - /** @deprecated */ - boolean hasChildren(); - - boolean hasReferences(); - - Iterator iterator(); - - boolean contains(Marker var1); - - boolean contains(String var1); - - boolean equals(Object var1); - - int hashCode(); -} diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/ObjectUtils.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/ObjectUtils.java deleted file mode 100644 index a5a51786d209..000000000000 --- a/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/ObjectUtils.java +++ /dev/null @@ -1,202 +0,0 @@ -package org.springframework.util; - -import java.lang.reflect.Array; -import java.util.Arrays; -import java.util.Collection; -import java.util.Map; -import java.util.Optional; -import java.util.StringJoiner; -import org.springframework.lang.Nullable; - -public abstract class ObjectUtils { - - private static final int INITIAL_HASH = 7; - private static final int MULTIPLIER = 31; - private static final String EMPTY_STRING = ""; - private static final String NULL_STRING = "null"; - private static final String ARRAY_START = "{"; - private static final String ARRAY_END = "}"; - private static final String EMPTY_ARRAY = "{}"; - private static final String ARRAY_ELEMENT_SEPARATOR = ", "; - private static final Object[] EMPTY_OBJECT_ARRAY = new Object[0]; - - public ObjectUtils() { - } - - public static boolean isCheckedException(Throwable ex) { - return false; - } - - public static boolean isCompatibleWithThrowsClause(Throwable ex, @Nullable Class... declaredExceptions) { - return false; - } - - public static boolean isArray(@Nullable Object obj) { - return false; - } - - public static boolean isEmpty(@Nullable Object[] array) { - return false; - } - - public static boolean isEmpty(@Nullable Object obj) { - return false; - } - - @Nullable - public static Object unwrapOptional(@Nullable Object obj) { - return null; - } - - public static boolean containsElement(@Nullable Object[] array, Object element) { - return true; - } - - public static boolean containsConstant(Enum[] enumValues, String constant) { - return true; - } - - public static boolean containsConstant(Enum[] enumValues, String constant, boolean caseSensitive) { - return true; - } - - public static > E caseInsensitiveValueOf(E[] enumValues, String constant) { - return null; - } - - public static A[] addObjectToArray(@Nullable A[] array, @Nullable O obj) { - return null; - } - - public static Object[] toObjectArray(@Nullable Object source) { - return null; - } - - public static boolean nullSafeEquals(@Nullable Object o1, @Nullable Object o2) { - return false; - } - - private static boolean arrayEquals(Object o1, Object o2) { - return false; - } - - public static int nullSafeHashCode(@Nullable Object obj) { - return 1; - } - - public static int nullSafeHashCode(@Nullable Object[] array) { - return 1; - } - - public static int nullSafeHashCode(@Nullable boolean[] array) { - return 1; - } - - public static int nullSafeHashCode(@Nullable byte[] array) { - return 1; - } - - public static int nullSafeHashCode(@Nullable char[] array) { - return 1; - } - - public static int nullSafeHashCode(@Nullable double[] array) { - return 1; - } - - public static int nullSafeHashCode(@Nullable float[] array) { - return 1; - } - - public static int nullSafeHashCode(@Nullable int[] array) { - return 1; - } - - public static int nullSafeHashCode(@Nullable long[] array) { - return 1; - } - - public static int nullSafeHashCode(@Nullable short[] array) { - return 1; - } - - /** @deprecated */ - @Deprecated - public static int hashCode(boolean bool) { - return 1; - } - - /** @deprecated */ - @Deprecated - public static int hashCode(double dbl) { - return 1; - } - - /** @deprecated */ - @Deprecated - public static int hashCode(float flt) { - return 1; - } - - /** @deprecated */ - @Deprecated - public static int hashCode(long lng) { - return 1; - } - - public static String identityToString(@Nullable Object obj) { - return ""; - } - - public static String getIdentityHexString(Object obj) { - return ""; - } - - public static String getDisplayString(@Nullable Object obj) { - return ""; - } - - public static String nullSafeClassName(@Nullable Object obj) { - return ""; - } - - public static String nullSafeToString(@Nullable Object obj) { - return ""; - } - - public static String nullSafeToString(@Nullable Object[] array) { - return ""; - } - - public static String nullSafeToString(@Nullable boolean[] array) { - return ""; - } - - public static String nullSafeToString(@Nullable byte[] array) { - return ""; - } - - public static String nullSafeToString(@Nullable char[] array) { - return ""; - } - - public static String nullSafeToString(@Nullable double[] array) { - return ""; - } - - public static String nullSafeToString(@Nullable float[] array) { - return ""; - } - - public static String nullSafeToString(@Nullable int[] array) { - return ""; - } - - public static String nullSafeToString(@Nullable long[] array) { - return ""; - } - - public static String nullSafeToString(@Nullable short[] array) { - return ""; - } -} diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/StringUtils.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/StringUtils.java deleted file mode 100644 index a78d47e44c6a..000000000000 --- a/java/ql/test/stubs/springframework-5.2.3/org/springframework/util/StringUtils.java +++ /dev/null @@ -1,30 +0,0 @@ -package org.springframework.util; - -import java.io.ByteArrayOutputStream; -import java.nio.charset.Charset; -import java.util.ArrayDeque; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.Deque; -import java.util.Enumeration; -import java.util.Iterator; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Locale; -import java.util.Properties; -import java.util.Set; -import java.util.StringJoiner; -import java.util.StringTokenizer; -import java.util.TimeZone; -import org.springframework.lang.Nullable; - -public abstract class StringUtils { - - @Deprecated - public static boolean isEmpty(@Nullable Object str) { - return true; - } - -} From b0f745365d8924351b814d0c0f2194621ca90c57 Mon Sep 17 00:00:00 2001 From: haby0 Date: Wed, 28 Apr 2021 14:32:25 +0800 Subject: [PATCH 22/28] Node type restriction --- .../Security/CWE/CWE-348/UseOfLessTrustedSource.ql | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql index 3c1d2a33ec12..3e25dba427ce 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql @@ -35,6 +35,12 @@ class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration { ma.getMethod() instanceof SplitMethod and not aa.getIndexExpr().(CompileTimeConstantExpr).getIntValue() = 0 ) + or + node.getType().hasName("Object") + or + node.getType() instanceof PrimitiveType + or + node.getType() instanceof BoxedType } } From e813257431654f2b39eed2d9c972b000d1dd48f3 Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 29 Apr 2021 21:23:52 +0800 Subject: [PATCH 23/28] use hardCode --- .../CWE/CWE-348/UseOfLessTrustedSource.java | 8 ++++++ .../CWE/CWE-348/UseOfLessTrustedSource.qhelp | 2 +- .../CWE/CWE-348/UseOfLessTrustedSource.ql | 2 -- .../CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 25 +++++++++---------- .../CWE-348/UseOfLessTrustedSource.expected | 15 +++++++---- .../CWE-348/UseOfLessTrustedSource.java | 8 ++++++ 6 files changed, 39 insertions(+), 21 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java index 35a66dc2731d..689246340da1 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java @@ -19,6 +19,14 @@ public void bad1(HttpServletRequest request) { } } + @GetMapping(value = "bad2") + public void bad2(HttpServletRequest request) { + String ip = getClientIP(); + if (!"127.0.0.1".equals(ip)) { + new Exception("ip illegal"); + } + } + @GetMapping(value = "good1") @ResponseBody public String good1(HttpServletRequest request) { diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index 6d5404a26836..5e43b9b1cfed 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -16,7 +16,7 @@ bypass a ban-list, for example.

    The following examples show the bad case and the good case respectively. -In the bad1 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 +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/UseOfLessTrustedSource.ql index 3e25dba427ce..14ae34442636 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql @@ -36,8 +36,6 @@ class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration { not aa.getIndexExpr().(CompileTimeConstantExpr).getIntValue() = 0 ) or - node.getType().hasName("Object") - or node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index 1639d1c82d12..708460cdaa1c 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -1,5 +1,6 @@ import java import DataFlow +import semmle.code.java.frameworks.Networking import semmle.code.java.security.QueryInjection import experimental.semmle.code.java.Logging @@ -40,14 +41,10 @@ private class CompareSink extends UseOfLessTrustedSink { ma.getMethod().getNumberOfParameters() = 1 and ( ma.getArgument(0) = this.asExpr() and - not ma.getQualifier().(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ - "", "unknown", ":" - ] + ma.getQualifier().(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName or ma.getQualifier() = this.asExpr() and - not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ - "", "unknown", ":" - ] + ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName ) ) or @@ -56,9 +53,10 @@ private class CompareSink extends UseOfLessTrustedSink { ma.getMethod().getDeclaringType() instanceof TypeString and ma.getMethod().getNumberOfParameters() = 1 and ma.getQualifier() = this.asExpr() and - not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ - "", "unknown" - ] + 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])?)$") ) or exists(MethodAccess ma | @@ -68,7 +66,10 @@ 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() != "" + 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])?)$") ) or exists(MethodAccess ma | @@ -78,9 +79,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 - not ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().toLowerCase() in [ - "", "unknown", ":" - ] + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName ) } } 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 index df238e4c0a32..ff9cbc24a20b 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected @@ -1,11 +1,16 @@ edges | UseOfLessTrustedSource.java:16:21:16:33 | getClientIP(...) : String | UseOfLessTrustedSource.java:17:37:17:38 | ip | -| UseOfLessTrustedSource.java:37:27:37:62 | getHeader(...) : String | UseOfLessTrustedSource.java:41:16:41:37 | ...[...] : String | -| UseOfLessTrustedSource.java:41:16:41:37 | ...[...] : String | UseOfLessTrustedSource.java:16:21:16:33 | getClientIP(...) : String | +| 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:37:27:37:62 | getHeader(...) : String | semmle.label | getHeader(...) : String | -| UseOfLessTrustedSource.java:41:16:41:37 | ...[...] : String | semmle.label | ...[...] : String | +| 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:37:27:37:62 | getHeader(...) : String | UseOfLessTrustedSource.java:17:37:17:38 | ip | IP address spoofing might include code from $@. | UseOfLessTrustedSource.java:37:27:37:62 | getHeader(...) | this user input | +| 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.java b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java index 35a66dc2731d..689246340da1 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java +++ b/java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.java @@ -19,6 +19,14 @@ public void bad1(HttpServletRequest request) { } } + @GetMapping(value = "bad2") + public void bad2(HttpServletRequest request) { + String ip = getClientIP(); + if (!"127.0.0.1".equals(ip)) { + new Exception("ip illegal"); + } + } + @GetMapping(value = "good1") @ResponseBody public String good1(HttpServletRequest request) { From 711a74c9c9a4fc2a0e925f09617f3c0fad9c3699 Mon Sep 17 00:00:00 2001 From: haby0 Date: Fri, 30 Apr 2021 10:31:40 +0800 Subject: [PATCH 24/28] Eliminate false positives\ --- .../Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index 708460cdaa1c..807ebd013d77 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -41,10 +41,12 @@ private class CompareSink extends UseOfLessTrustedSink { ma.getMethod().getNumberOfParameters() = 1 and ( ma.getArgument(0) = this.asExpr() and - ma.getQualifier().(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName + ma.getQualifier().(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName and + not ma.getQualifier().(CompileTimeConstantExpr).getStringValue() = "0:0:0:0:0:0:0:1" or ma.getQualifier() = this.asExpr() and - ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName + ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName and + not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "0:0:0:0:0:0:0:1" ) ) or @@ -79,7 +81,8 @@ 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() instanceof PrivateHostName + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName and + not ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "0:0:0:0:0:0:0:1" ) } } From 8142810455cf4266e6b4266053daa8908ded3f79 Mon Sep 17 00:00:00 2001 From: haby0 Date: Fri, 30 Apr 2021 16:54:28 +0800 Subject: [PATCH 25/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp index 5e43b9b1cfed..61848f111ad0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp @@ -4,7 +4,7 @@

    An original client IP address is retrieved from an http header (X-Forwarded-For or X-Real-IP or Proxy-Client-IP -etc.), which is used to ensure security or track it in the log for statistical or other reasons. Attackers can forge the value of these identifiers to +etc.), which is used to ensure security. Attackers can forge the value of these identifiers to bypass a ban-list, for example.

     From 0691cac5ab2429644cfc84d5acced219f7ed6a99 Mon Sep 17 00:00:00 2001 From: haby0 Date: Fri, 30 Apr 2021 16:54:41 +0800 Subject: [PATCH 26/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll index 807ebd013d77..04df6aa5dd60 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll @@ -58,7 +58,7 @@ private class CompareSink extends UseOfLessTrustedSink { 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])?)$") + .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 ) or exists(MethodAccess ma | From f41301f8f58f48ebb2663deae3108a12ed738936 Mon Sep 17 00:00:00 2001 From: haby0 Date: Fri, 30 Apr 2021 16:55:17 +0800 Subject: [PATCH 27/28] Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java Co-authored-by: Chris Smowton --- .../CWE/CWE-348/UseOfLessTrustedSource.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java index 689246340da1..ad959cb2dc8b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java +++ b/java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java @@ -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"); + 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]; + if (!StringUtils.startsWith(ip, "192.168.")) { + new Exception("ip illegal"); } - return remoteAddr; } protected String getClientIP() { @@ -48,4 +46,4 @@ protected String getClientIP() { } return xfHeader.split(",")[0]; } -} \ No newline at end of file +} From fdcc517b9fcb8917775feb5977cb050d4bfc9f94 Mon Sep 17 00:00:00 2001 From: haby0 Date: Fri, 30 Apr 2021 17:43:34 +0800 Subject: [PATCH 28/28] UseOfLessTrustedSource -> ClientSuppliedIpUsedInSecurityCheck" --- ... ClientSuppliedIpUsedInSecurityCheck.java} | 6 ++--- ...ClientSuppliedIpUsedInSecurityCheck.qhelp} | 2 +- ...=> ClientSuppliedIpUsedInSecurityCheck.ql} | 17 ++++++++----- ...lientSuppliedIpUsedInSecurityCheckLib.qll} | 25 +++++++++---------- ...ientSuppliedIpUsedInSecurityCheck.expected | 16 ++++++++++++ ... ClientSuppliedIpUsedInSecurityCheck.java} | 18 ++++++------- .../ClientSuppliedIpUsedInSecurityCheck.qlref | 1 + .../CWE-348/UseOfLessTrustedSource.expected | 16 ------------ .../CWE-348/UseOfLessTrustedSource.qlref | 1 - 9 files changed, 52 insertions(+), 50 deletions(-) rename java/ql/src/experimental/Security/CWE/CWE-348/{UseOfLessTrustedSource.java => ClientSuppliedIpUsedInSecurityCheck.java} (92%) rename java/ql/src/experimental/Security/CWE/CWE-348/{UseOfLessTrustedSource.qhelp => ClientSuppliedIpUsedInSecurityCheck.qhelp} (96%) rename java/ql/src/experimental/Security/CWE/CWE-348/{UseOfLessTrustedSource.ql => ClientSuppliedIpUsedInSecurityCheck.ql} (69%) rename java/ql/src/experimental/Security/CWE/CWE-348/{UseOfLessTrustedSourceLib.qll => ClientSuppliedIpUsedInSecurityCheckLib.qll} (77%) create mode 100644 java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected rename java/ql/test/experimental/query-tests/security/CWE-348/{UseOfLessTrustedSource.java => ClientSuppliedIpUsedInSecurityCheck.java} (73%) create mode 100644 java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.expected delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-348/UseOfLessTrustedSource.qlref 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 ad959cb2dc8b..93a860981d1d 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.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() { 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 61848f111ad0..fd62ab2968b2 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 14ae34442636..78d8bfee5f06 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 04df6aa5dd60..fd9353d3414b 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 000000000000..5627f9541f18 --- /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 689246340da1..93a860981d1d 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.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]; } -} \ 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 000000000000..476249f5fd9a --- /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 ff9cbc24a20b..000000000000 --- 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 3c547a2c8714..000000000000 --- 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