From 374eec5f9a3fe6731463389b454c3281b3763bcc Mon Sep 17 00:00:00 2001
From: Kevin Stubbings <kwstubbs@github.com>
Date: Tue, 17 Dec 2024 15:13:55 -0800
Subject: [PATCH] Second round feedback

---
 .../experimental/CWE-942/CorsMisconfiguration.ql   | 14 +++++---------
 .../CWE-942/CorsMisconfigurationCredentials.ql     |  8 ++++----
 .../CWE-942/CorsMisconfigurationLib.qll            | 12 ++++++++++++
 .../experimental/CWE-942/CorsMisconfiguration.cs   |  7 ++++++-
 .../CWE-942/CorsMisconfiguration.expected          |  1 +
 .../CorsMisconfigurationCredentials.expected       |  2 +-
 6 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql
index 492e3e97b8d8..6e13f9fc03cc 100644
--- a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql
+++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql
@@ -30,13 +30,9 @@ private predicate hasDangerousOrigins(MethodCall m) {
   m.getTarget()
       .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder",
         "WithOrigins") and
-  (
-    m.getAnArgument().getValue() = ["null", "*"]
-    or
-    exists(StringLiteral idStr |
-      idStr.getValue().toLowerCase().matches(["null", "*"]) and
-      DataFlow::localExprFlow(idStr, m.getAnArgument())
-    )
+  exists(StringLiteral idStr |
+    idStr.getValue().toLowerCase().matches(["null", "*"]) and
+    TaintTracking::localExprTaint(idStr, m.getAnArgument())
   )
 }
 
@@ -45,14 +41,14 @@ where
   (
     usedPolicy(add_policy) and
     // Misconfigured origin affects used policy
-    add_policy.getArgument(1) = child.getParent*()
+    getCallableFromExpr(add_policy.getArgument(1)).calls*(child.getTarget())
     or
     add_policy
         .getTarget()
         .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions",
           "AddDefaultPolicy") and
     // Misconfigured origin affects added default policy
-    add_policy.getArgument(0) = child.getParent*()
+    getCallableFromExpr(add_policy.getArgument(0)).calls*(child.getTarget())
   ) and
   (hasDangerousOrigins(child) or allowAnyOrigin(child))
 select add_policy, "The following CORS policy may allow requests from 3rd party websites"
diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql
index 29b7dc3aa107..0252830a6f01 100644
--- a/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql
+++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql
@@ -26,16 +26,16 @@ class AllowsCredentials extends MethodCall {
 from MethodCall add_policy, MethodCall setIsOriginAllowed, AllowsCredentials allowsCredentials
 where
   (
-    add_policy.getArgument(1) = setIsOriginAllowed.getParent*() and
+    getCallableFromExpr(add_policy.getArgument(1)).calls*(setIsOriginAllowed.getTarget()) and
     usedPolicy(add_policy) and
-    add_policy.getArgument(1) = allowsCredentials.getParent*()
+    getCallableFromExpr(add_policy.getArgument(1)).calls*(allowsCredentials.getTarget())
     or
     add_policy
         .getTarget()
         .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions",
           "AddDefaultPolicy") and
-    add_policy.getArgument(0) = setIsOriginAllowed.getParent*() and
-    add_policy.getArgument(0) = allowsCredentials.getParent*()
+    getCallableFromExpr(add_policy.getArgument(0)).calls*(setIsOriginAllowed.getTarget()) and
+    getCallableFromExpr(add_policy.getArgument(0)).calls*(allowsCredentials.getTarget())
   ) and
   setIsOriginAllowedReturnsTrue(setIsOriginAllowed)
 select add_policy,
diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationLib.qll b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationLib.qll
index 301dd354a39c..be3310476a88 100644
--- a/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationLib.qll
+++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationLib.qll
@@ -1,6 +1,18 @@
 import csharp
 import DataFlow
 
+/**
+ * Gets the actual callable corresponding to the expression `e`.
+ */
+Callable getCallableFromExpr(Expr e) {
+  exists(Expr dcArg | dcArg = e.(DelegateCreation).getArgument() |
+    result = dcArg.(CallableAccess).getTarget() or
+    result = dcArg.(AnonymousFunctionExpr)
+  )
+  or
+  result = e
+}
+
 /**
  * Holds if the `Callable` c throws any exception other than `ThrowsArgumentNullException`
  */
diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs
index 8cf87d73220a..bc1456b55ea1 100644
--- a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs
+++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs
@@ -11,8 +11,13 @@ public void ConfigureServices(string[] args) {
     builder.Services.AddCors(options => {
       options.AddPolicy(MyAllowSpecificOrigins,
         policy => {
-          policy.AllowAnyOrigin().AllowCredentials().AllowAnyHeader().AllowAnyMethod();
+          policy.AllowAnyOrigin().AllowAnyHeader().AllowAnyMethod();
         });
+      options.AddDefaultPolicy(
+        builder => builder
+        .WithOrigins(["*"])
+        .AllowAnyMethod()
+        .AllowAnyHeader());
     });
 
     var app = builder.Build();
diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected
index 9ea84114ecca..b874b10022c9 100644
--- a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected
+++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected
@@ -1 +1,2 @@
 | CorsMisconfiguration.cs:12:7:15:10 | call to method AddPolicy | The following CORS policy may allow requests from 3rd party websites |
+| CorsMisconfiguration.cs:16:7:20:26 | call to method AddDefaultPolicy | The following CORS policy may allow requests from 3rd party websites |
diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.expected b/csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.expected
index 390c1e5b2eec..161cf001dd1b 100644
--- a/csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.expected
+++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.expected
@@ -1 +1 @@
-| CorsMiconfigurationCredentials.cs:18:5:22:24 | call to method AddPolicy | The following CORS policy may allow credentialed requests from 3rd party websites |
+| CorsMiconfigurationCredentials.cs:12:7:15:10 | call to method AddPolicy | The following CORS policy may allow credentialed requests from 3rd party websites |