Switch from sanitizer to tainttracking, formatting and qldoc changes

Author: torque59

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

@@ -29,6 +29,15 @@ public void doFilter(ServletRequest req, ServletResponse res,
             }
         }
 
+        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);
     }
 

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

@@ -16,9 +16,9 @@
 
 		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 to
-		make browsers accept the header. Therefore, to allow multiple origins
-		for Cross-Origin requests with credentials, the server must
+		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
@@ -47,8 +47,8 @@
 		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>.This can be done using a sandboxed iframe. A more detailed
-		explanation is available in the portswigger blogpost referenced below.
+		<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>
@@ -57,7 +57,7 @@
 	<p>
 
 		In the example below, the server allows the browser to send
-		user credentials in a Cross-Origin request. The request header
+		user credentials in a cross-origin request. The request header
 		<code>origins</code> controls the allowed origins for such a
 		Cross-Origin request.
 

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

@@ -1,6 +1,6 @@
 /**
- * @name Cors header being set from remote source
- * @description Cors header is being set from remote source, allowing to control the origin.
+ * @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
@@ -13,6 +13,7 @@ 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
 
 /**
@@ -25,10 +26,10 @@ private predicate setsAllowCredentials(MethodAccess header) {
   ) and
   header.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() =
     "access-control-allow-credentials" and
-  header.getArgument(1).(CompileTimeConstantExpr).getStringValue() = "true"
+  header.getArgument(1).(CompileTimeConstantExpr).getStringValue().toLowerCase() = "true"
 }
 
-class CorsProbableCheckAccess extends MethodAccess {
+private class CorsProbableCheckAccess extends MethodAccess {
   CorsProbableCheckAccess() {
     getMethod().hasName("contains") and
     getMethod().getDeclaringType().getASourceSupertype*() instanceof CollectionType
@@ -45,46 +46,41 @@ 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()
+  }
+}
+
 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 |
+    exists(MethodAccess corsHeader, MethodAccess allowCredentialsHeader |
       (
-        corsheader.getMethod() instanceof ResponseSetHeaderMethod or
-        corsheader.getMethod() instanceof ResponseAddHeaderMethod
+        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)
-    )
-  }
-
-  /**
-   * this sanitizer is oversimplistic:
-   * - it only considers local dataflows
-   * - it will consider any method calling `Collection.contains` or `String.equals` as a sanitizer
-   *   no matter if that check is taken into account and its result reaches the
-   *   return statement of the wrapper.
-   */
-  override predicate isSanitizer(DataFlow::Node node) {
-    exists(CorsProbableCheckAccess check |
-      TaintTracking::localTaint(node, DataFlow::exprNode(check.getAnArgument()))
-      or
-      exists(MethodAccess wrapperAccess, Method wrapper, int i |
-        TaintTracking::localTaint(node, DataFlow::exprNode(wrapperAccess.getArgument(i))) and
-        wrapperAccess.getMethod() = wrapper and
-        TaintTracking::localTaint(DataFlow::parameterNode(wrapper.getParameter(i)),
-          DataFlow::exprNode(check.getAnArgument()))
-      )
+      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
-where conf.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "Cors header is being set using user controlled value $@.",
+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/query-tests/security/CWE-346/UnvalidatedCors.expected

@@ -4,4 +4,4 @@ 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 |
+| 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 |