Merge pull request #5008 from torque59/cwe-346

Java: Queries to detect remote source flow origins to CORS header.

java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.java

@@ -0,0 +1,45 @@
+import java.io.IOException;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.apache.commons.lang3.StringUtils;
+
+public class CorsFilter implements Filter {
+    public void init(FilterConfig filterConfig) throws ServletException {}
+
+    public void doFilter(ServletRequest req, ServletResponse res,
+            FilterChain chain) throws IOException, ServletException {
+        HttpServletRequest request = (HttpServletRequest) req;
+        HttpServletResponse response = (HttpServletResponse) res;
+        String url = request.getHeader("Origin");
+
+        if (!StringUtils.isEmpty(url)) {
+            String val = response.getHeader("Access-Control-Allow-Origin");
+
+            if (StringUtils.isEmpty(val)) {
+                response.addHeader("Access-Control-Allow-Origin", url); // BAD -> User controlled CORS header being set here.
+                response.addHeader("Access-Control-Allow-Credentials", "true");
+            }
+        }
+
+        if (!StringUtils.isEmpty(url)) {
+            List<String> checkorigins = Arrays.asList("www.example.com", "www.sub.example.com");
+
+            if (checkorigins.contains(url)) { // GOOD -> Origin is validated here.
+                response.addHeader("Access-Control-Allow-Origin", url);
+                response.addHeader("Access-Control-Allow-Credentials", "true");
+            }
+        }
+
+        chain.doFilter(req, res);
+    }
+
+    public void destroy() {}
+}

java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.qhelp

@@ -0,0 +1,76 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+<overview>
+	<p>
+
+		A server can send the
+		<code>Access-Control-Allow-Credentials</code> CORS header to control
+		when a browser may send user credentials in Cross-Origin HTTP
+		requests.
+
+	</p>
+	<p>
+
+		When the <code>Access-Control-Allow-Credentials</code> header
+		is <code>true</code>, the <code>Access-Control-Allow-Origin</code>
+		header must have a value different from <code>*</code> in order
+		for browsers to accept the header. Therefore, to allow multiple origins
+		for cross-origin requests with credentials, the server must
+		dynamically compute the value of the
+		<code>Access-Control-Allow-Origin</code> header. Computing this
+		header value from information in the request to the server can
+		therefore potentially allow an attacker to control the origins that
+		the browser sends credentials to.
+
+	</p>
+
+
+
+</overview>
+
+<recommendation>
+	<p>
+
+		When the <code>Access-Control-Allow-Credentials</code> header
+		value is <code>true</code>, a dynamic computation of the
+		<code>Access-Control-Allow-Origin</code> header must involve
+		sanitization if it relies on user-controlled input.
+
+
+	</p>
+	<p>
+
+		Since the <code>null</code> origin is easy to obtain for an
+		attacker, it is never safe to use <code>null</code> as the value of
+		the <code>Access-Control-Allow-Origin</code> header when the
+		<code>Access-Control-Allow-Credentials</code> header value is
+		<code>true</code>.A null origin can be set by an attacker using a sandboxed iframe.
+		A more detailed explanation is available in the portswigger blogpost referenced below.
+
+	</p>
+</recommendation>
+
+<example>
+	<p>
+
+		In the example below, the server allows the browser to send
+		user credentials in a cross-origin request. The request header
+		<code>origins</code> controls the allowed origins for such a
+		Cross-Origin request.
+
+	</p>
+
+	<sample src="UnvalidatedCors.java"/>
+
+</example>
+
+<references>
+	<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin">CORS, Access-Control-Allow-Origin</a>.</li>
+	<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials">CORS, Access-Control-Allow-Credentials</a>.</li>
+	<li>PortSwigger: <a href="http://blog.portswigger.net/2016/10/exploiting-cors-misconfigurations-for.html">Exploiting CORS Misconfigurations for Bitcoins and Bounties</a></li>
+	<li>W3C: <a href="https://w3c.github.io/webappsec-cors-for-developers/#resources">CORS for developers, Advice for Resource Owners</a></li>
+</references>
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.ql

@@ -0,0 +1,86 @@
+/**
+ * @name CORS is derived from untrusted input
+ * @description CORS header is derived from untrusted input, allowing a remote user to control which origins are trusted.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/unvalidated-cors-origin-set
+ * @tags security
+ *       external/cwe/cwe-346
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.frameworks.Servlets
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.TaintTracking2
+import DataFlow::PathGraph
+
+/**
+ *  Holds if `header` sets `Access-Control-Allow-Credentials` to `true`. This ensures fair chances of exploitability.
+ */
+private predicate setsAllowCredentials(MethodAccess header) {
+  (
+    header.getMethod() instanceof ResponseSetHeaderMethod or
+    header.getMethod() instanceof ResponseAddHeaderMethod
+  ) and
+  header.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() =
+    "access-control-allow-credentials" and
+  header.getArgument(1).(CompileTimeConstantExpr).getStringValue().toLowerCase() = "true"
+}
+
+private class CorsProbableCheckAccess extends MethodAccess {
+  CorsProbableCheckAccess() {
+    getMethod().hasName("contains") and
+    getMethod().getDeclaringType().getASourceSupertype*() instanceof CollectionType
+    or
+    getMethod().hasName("containsKey") and
+    getMethod().getDeclaringType().getASourceSupertype*() instanceof MapType
+    or
+    getMethod().hasName("equals") and
+    getQualifier().getType() instanceof TypeString
+  }
+}
+
+private Expr getAccessControlAllowOriginHeaderName() {
+  result.(CompileTimeConstantExpr).getStringValue().toLowerCase() = "access-control-allow-origin"
+}
+
+/**
+ * This taintflow2 configuration checks if there is a flow from source node towards CorsProbableCheckAccess methods.
+ */
+class CorsSourceReachesCheckConfig extends TaintTracking2::Configuration {
+  CorsSourceReachesCheckConfig() { this = "CorsOriginConfig" }
+
+  override predicate isSource(DataFlow::Node source) { any(CorsOriginConfig c).hasFlow(source, _) }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink.asExpr() = any(CorsProbableCheckAccess check).getAnArgument()
+  }
+}
+
+private class CorsOriginConfig extends TaintTracking::Configuration {
+  CorsOriginConfig() { this = "CorsOriginConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess corsHeader, MethodAccess allowCredentialsHeader |
+      (
+        corsHeader.getMethod() instanceof ResponseSetHeaderMethod or
+        corsHeader.getMethod() instanceof ResponseAddHeaderMethod
+      ) and
+      getAccessControlAllowOriginHeaderName() = corsHeader.getArgument(0) and
+      setsAllowCredentials(allowCredentialsHeader) and
+      corsHeader.getEnclosingCallable() = allowCredentialsHeader.getEnclosingCallable() and
+      sink.asExpr() = corsHeader.getArgument(1)
+    )
+  }
+}
+
+from
+  DataFlow::PathNode source, DataFlow::PathNode sink, CorsOriginConfig conf,
+  CorsSourceReachesCheckConfig sanconf
+where conf.hasFlowPath(source, sink) and not sanconf.hasFlow(source.getNode(), _)
+select sink.getNode(), source, sink, "CORS header is being set using user controlled value $@.",
+  source.getNode(), "user-provided value"

java/ql/test/experimental/query-tests/security/CWE-346/UnvalidatedCors.expected

@@ -0,0 +1,7 @@
+edges
+| UnvalidatedCors.java:21:22:21:48 | getHeader(...) : String | UnvalidatedCors.java:27:67:27:69 | url |
+nodes
+| UnvalidatedCors.java:21:22:21:48 | getHeader(...) : String | semmle.label | getHeader(...) : String |
+| UnvalidatedCors.java:27:67:27:69 | url | semmle.label | url |
+#select
+| UnvalidatedCors.java:27:67:27:69 | url | UnvalidatedCors.java:21:22:21:48 | getHeader(...) : String | UnvalidatedCors.java:27:67:27:69 | url | CORS header is being set using user controlled value $@. | UnvalidatedCors.java:21:22:21:48 | getHeader(...) | user-provided value |

java/ql/test/experimental/query-tests/security/CWE-346/UnvalidatedCors.java

@@ -0,0 +1,37 @@
+import java.io.IOException;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.apache.commons.lang3.StringUtils;
+
+public class UnvalidatedCors implements Filter {
+    public void init(FilterConfig filterConfig) throws ServletException {}
+
+    public void doFilter(ServletRequest req, ServletResponse res,
+            FilterChain chain) throws IOException, ServletException {
+        HttpServletRequest request = (HttpServletRequest) req;
+        HttpServletResponse response = (HttpServletResponse) res;
+        String url = request.getHeader("Origin");
+
+        if (!StringUtils.isEmpty(url)) {
+            String val = response.getHeader("Access-Control-Allow-Origin");
+
+            if (StringUtils.isEmpty(val)) {
+                response.addHeader("Access-Control-Allow-Origin", url);
+                response.addHeader("Access-Control-Allow-Credentials", "true");
+            }
+        }
+
+        chain.doFilter(req, res);
+    }
+
+    public void destroy() {}
+}
+

java/ql/test/experimental/query-tests/security/CWE-346/UnvalidatedCors.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-346/UnvalidatedCors.ql

java/ql/test/experimental/query-tests/security/CWE-346/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/apache-commons-lang3-3.7

java/ql/test/stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/StringUtils.java

@@ -960,4 +960,4 @@ public class StringUtils {
     public StringUtils() {
     }
 
-}
+}

java/ql/test/stubs/servlet-api-2.4/javax/servlet/Filter.java

@@ -0,0 +1,11 @@
+package javax.servlet;
+
+import java.io.IOException;
+
+public interface Filter {
+    default public void init(FilterConfig filterConfig) throws ServletException {}
+    public void doFilter(ServletRequest request, ServletResponse response,
+                         FilterChain chain)
+            throws IOException, ServletException;
+    default public void destroy() {}
+}

java/ql/test/stubs/servlet-api-2.4/javax/servlet/FilterChain.java

@@ -0,0 +1,7 @@
+package javax.servlet;
+
+import java.io.IOException;
+
+public interface FilterChain {
+   public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException;
+}

java/ql/test/stubs/servlet-api-2.4/javax/servlet/FilterConfig.java

@@ -0,0 +1,10 @@
+package javax.servlet;
+
+import java.util.Enumeration;
+
+public interface FilterConfig {
+    public String getFilterName();
+    public ServletContext getServletContext();
+    public String getInitParameter(String name);
+    public Enumeration<String> getInitParameterNames();
+}

java/ql/test/stubs/servlet-api-2.4/javax/servlet/http/HttpServletResponse.java

@@ -24,6 +24,7 @@
 package javax.servlet.http;
 
 import java.io.IOException;
+import java.util.Collection;
 import javax.servlet.ServletResponse;
 
 public interface HttpServletResponse extends ServletResponse {
@@ -44,6 +45,11 @@ public interface HttpServletResponse extends ServletResponse {
     public void addIntHeader(String name, int value);
     public void setStatus(int sc);
     public void setStatus(int sc, String sm);
+    public int getStatus();
+    public String getHeader(String name);
+    public Collection<String> getHeaders(String name);
+    public Collection<String> getHeaderNames();
+
 
     public static final int SC_CONTINUE = 100;
     public static final int SC_SWITCHING_PROTOCOLS = 101;