PR feedback integration

Author: torque59

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

@@ -7,20 +7,20 @@
 	<p>
 
 		A server can send the
-		<code>"Access-Control-Allow-Credentials"</code> CORS header to control
+		<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 to
+		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
 		dynamically compute the value of the
-		<code>"Access-Control-Allow-Origin"</code> header. Computing this
+		<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.
@@ -35,19 +35,20 @@
 	<p>
 
 		When the <code>Access-Control-Allow-Credentials</code> header
-		value is <code>"true"</code>, a dynamic computation of the
+		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
+		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>.
+		<code>true</code>.This can be done using a sandboxed iframe. A more detailed
+		explanation is available in the portswigger blogpost.
 
 	</p>
 </recommendation>

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

@@ -15,16 +15,17 @@ import semmle.code.java.frameworks.Servlets
 import semmle.code.java.dataflow.TaintTracking
 import DataFlow::PathGraph
 
-// Check for Access-Control-Allow-Credentials as well, this ensures fair chances of exploitability.
-predicate satisfyAllowCredentials(MethodAccess header, MethodAccess check) {
+/**
+ *  Holds if `header` sets `Access-Control-Allow-Credentials` to `true`. This ensures fair chances of exploitability.
+ */
+private predicate setsAllowCredentials(MethodAccess header) {
   header.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() =
     "access-control-allow-credentials" and
-  header.getArgument(1).(CompileTimeConstantExpr).getStringValue() = "true" and
-  header.getEnclosingCallable() = check.getEnclosingCallable()
+  header.getArgument(1).(CompileTimeConstantExpr).getStringValue() = "true"
 }
 
-predicate checkAccessControlAllowOriginHeader(Expr expr) {
-  expr.(CompileTimeConstantExpr).getStringValue().toLowerCase() = "access-control-allow-origin"
+private Expr getAccessControlAllowOriginHeaderName() {
+  result.(CompileTimeConstantExpr).getStringValue().toLowerCase() = "access-control-allow-origin"
 }
 
 class CorsOriginConfig extends TaintTracking::Configuration {
@@ -33,18 +34,19 @@ class CorsOriginConfig extends TaintTracking::Configuration {
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(ResponseSetHeaderMethod h, MethodAccess m |
-      m = h.getAReference() and
-      checkAccessControlAllowOriginHeader(m.getArgument(0)) and
-      satisfyAllowCredentials(h.getAReference(), m) and
-      sink.asExpr() = m.getArgument(1)
-    )
-    or
-    exists(ResponseAddHeaderMethod a, MethodAccess m |
-      m = a.getAReference() and
-      checkAccessControlAllowOriginHeader(m.getArgument(0)) and
-      satisfyAllowCredentials(a.getAReference(), m) and
-      sink.asExpr() = m.getArgument(1)
+    exists(MethodAccess corsheader, MethodAccess allowcredentialsheader |
+      (
+        corsheader.getMethod() instanceof ResponseSetHeaderMethod or
+        corsheader.getMethod() instanceof ResponseAddHeaderMethod
+      ) and
+      (
+        allowcredentialsheader.getMethod() instanceof ResponseSetHeaderMethod or
+        allowcredentialsheader.getMethod() instanceof ResponseAddHeaderMethod
+      ) and
+      getAccessControlAllowOriginHeaderName() = corsheader.getArgument(0) and
+      setsAllowCredentials(allowcredentialsheader) and
+      corsheader.getEnclosingCallable() = allowcredentialsheader.getEnclosingCallable() and
+      sink.asExpr() = corsheader.getArgument(1)
     )
   }
 }