From a4fb0e06b311b3228dfa590c57ecc42565475858 Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Tue, 26 Nov 2024 15:16:32 -0800 Subject: [PATCH 1/8] Added tests --- .../CWE-942/CORSMisconfiguration.txt | 32 +++++++++++++++++++ .../CWE-942/CorsMiconfiguration.cs | 25 +++++++++++++++ .../CWE-942/CorsMisconfiguration.qlref | 1 + csharp/ql/test/experimental/CWE-942/options | 5 +++ 4 files changed, 63 insertions(+) create mode 100644 csharp/ql/test/experimental/CWE-942/CORSMisconfiguration.txt create mode 100644 csharp/ql/test/experimental/CWE-942/CorsMiconfiguration.cs create mode 100644 csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref create mode 100644 csharp/ql/test/experimental/CWE-942/options diff --git a/csharp/ql/test/experimental/CWE-942/CORSMisconfiguration.txt b/csharp/ql/test/experimental/CWE-942/CORSMisconfiguration.txt new file mode 100644 index 000000000000..4d66e855941b --- /dev/null +++ b/csharp/ql/test/experimental/CWE-942/CORSMisconfiguration.txt @@ -0,0 +1,32 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Mvc; +using System; + + +public class Startup +{ + public void ConfigureServices(IServiceCollection services) + { +var builder = WebApplication.CreateBuilder(args); +var MyAllowSpecificOrigins = "_myAllowSpecificOrigins"; + + +builder.Services.AddCors(options => +{ + options.AddPolicy(MyAllowSpecificOrigins, + policy => + { + policy.SetIsOriginAllowed(test => true).AllowCredentials().AllowAnyHeader().AllowAnyMethod(); + }); +}); + +var app = builder.Build(); + + + +app.MapGet("/", () => "Hello World!"); +app.UseCors(MyAllowSpecificOrigins); + +app.Run(); + } +} \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-942/CorsMiconfiguration.cs b/csharp/ql/test/experimental/CWE-942/CorsMiconfiguration.cs new file mode 100644 index 000000000000..c5712603d7fd --- /dev/null +++ b/csharp/ql/test/experimental/CWE-942/CorsMiconfiguration.cs @@ -0,0 +1,25 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Mvc; +using System; + +var builder = WebApplication.CreateBuilder(args); +var MyAllowSpecificOrigins = "_myAllowSpecificOrigins"; + + +builder.Services.AddCors(options => +{ + options.AddPolicy(MyAllowSpecificOrigins, + policy => + { + policy.SetIsOriginAllowed(test => true).AllowCredentials().AllowAnyHeader().AllowAnyMethod(); + }); +}); + +var app = builder.Build(); + + + +app.MapGet("/", () => "Hello World!"); +app.UseCors(MyAllowSpecificOrigins); + +app.Run(); \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref new file mode 100644 index 000000000000..cadcb0509195 --- /dev/null +++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref @@ -0,0 +1 @@ +experimental/CWE-942/CorsMisconfiguration.ql \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-942/options b/csharp/ql/test/experimental/CWE-942/options new file mode 100644 index 000000000000..67d8fa711a4a --- /dev/null +++ b/csharp/ql/test/experimental/CWE-942/options @@ -0,0 +1,5 @@ +semmle-extractor-options: /nostdlib /noconfig +semmle-extractor-options: --load-sources-from-project:${testdir}/../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj +semmle-extractor-options: --load-sources-from-project:../../resources/stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.App.csproj +semmle-extractor-options: --load-sources-from-project:../../resources/stubs/Microsoft.Extensions.DependencyInjection.Abstractions/6.0.0/Microsoft.Extensions.DependencyInjection.Abstractions.csproj + From 3037c790d6905365cb84425adf7499039b25a44e Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Tue, 26 Nov 2024 15:25:59 -0800 Subject: [PATCH 2/8] Add changelog Fix issues Change this --- .../src/change-notes/2024-11-26-cors-query.md | 4 + .../CWE-942/CorsMisconfiguration.qhelp | 85 ++++++++++++++++++ .../CWE-942/CorsMisconfiguration.ql | 88 +++++++++++++++++++ .../CorsMisconfigurationCredentials.ql | 84 ++++++++++++++++++ .../experimental/CWE-942/examples/CORSBad.cs | 64 ++++++++++++++ .../experimental/CWE-942/examples/CORSGood.cs | 64 ++++++++++++++ .../CWE-942/CORSMisconfiguration.txt | 32 ------- ...n.cs => CorsMiconfigurationCredentials.cs} | 0 .../CWE-942/CorsMisconfiguration.expected | 0 .../CWE-942/CorsMisconfiguration.qlref | 1 - .../CorsMisconfigurationCredentials.qlref | 1 + 11 files changed, 390 insertions(+), 33 deletions(-) create mode 100644 csharp/ql/src/change-notes/2024-11-26-cors-query.md create mode 100644 csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp create mode 100644 csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql create mode 100644 csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql create mode 100644 csharp/ql/src/experimental/CWE-942/examples/CORSBad.cs create mode 100644 csharp/ql/src/experimental/CWE-942/examples/CORSGood.cs delete mode 100644 csharp/ql/test/experimental/CWE-942/CORSMisconfiguration.txt rename csharp/ql/test/experimental/CWE-942/{CorsMiconfiguration.cs => CorsMiconfigurationCredentials.cs} (100%) create mode 100644 csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected delete mode 100644 csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref create mode 100644 csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.qlref diff --git a/csharp/ql/src/change-notes/2024-11-26-cors-query.md b/csharp/ql/src/change-notes/2024-11-26-cors-query.md new file mode 100644 index 000000000000..77df5ba3a398 --- /dev/null +++ b/csharp/ql/src/change-notes/2024-11-26-cors-query.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* C#: Added `ccs/web/cors-misconfiguration` queries which looks for CORS misconfigurations with and without credentials. \ No newline at end of file diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp new file mode 100644 index 000000000000..3ab8d7fbba93 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp @@ -0,0 +1,85 @@ + + + + +

+ + A server can send the + "Access-Control-Allow-Credentials" CORS header to control + when a browser may send user credentials in Cross-Origin HTTP + requests. + +

+

+ + When the Access-Control-Allow-Credentials header + is "true", the Access-Control-Allow-Origin + header must have a value different from "*" 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 + "Access-Control-Allow-Origin" 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. + +

+ + + + + + +

+ + When the Access-Control-Allow-Credentials header + value is "true", a dynamic computation of the + Access-Control-Allow-Origin header must involve + sanitization if it relies on user-controlled input. + + +

+

+ + Since the "null" origin is easy to obtain for an + attacker, it is never safe to use "null" as the value of + the Access-Control-Allow-Origin header when the + Access-Control-Allow-Credentials header value is + "true". + +

+ + + +

+ + In the example below, the server allows the browser to send + user credentials in a Cross-Origin request. The request header + origins controls the allowed origins for such a + Cross-Origin request. + +

+ + + +

+ + This is not secure, since an attacker can choose the value of + the origin request header to make the browser send + credentials to their own server. The use of a allowlist containing + allowed origins for the Cross-Origin request fixes the issue: + +

+ + + + + +
  • Mozilla Developer Network: CORS, Access-Control-Allow-Origin.
    • +
  • Mozilla Developer Network: CORS, Access-Control-Allow-Credentials.
    • +
  • PortSwigger: Exploiting CORS Misconfigurations for Bitcoins and Bounties
    • +
  • W3C: CORS for developers, Advice for Resource Owners
    • +     +     diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql new file mode 100644 index 000000000000..8f75a4e7787a --- /dev/null +++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -0,0 +1,88 @@ +/** + * @name CORS misconfiguration + * @description Keeping an open CORS policy may result in security issues as third party website may be able to + * access other websites. + * @kind problem + * @problem.severity error + * @security-severity 7.5 + * @precision high + * @id cs/web/cors-misconfiguration + * @tags security + * external/cwe/cwe-942 + */ + +import csharp +private import DataFlow +import semmle.code.csharp.frameworks.system.Web + +/** + * Holds if SetIsOriginAllowed always returns true. This sets the Access-Control-Allow-Origin to the requester + */ +private predicate functionAlwaysReturnsTrue(MethodCall mc) { + mc.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", + "SetIsOriginAllowed") and + alwaysReturnsTrue(mc.getArgument(0)) +} + +/** + * Holds if `c` always returns `true`. + */ +private predicate alwaysReturnsTrue(Callable c) { + forex(ReturnStmt rs | rs.getEnclosingCallable() = c | + rs.getExpr().(BoolLiteral).getBoolValue() = true + ) + or + c.getBody().(BoolLiteral).getBoolValue() = true +} + +/** + * Holds if the application uses a vulnerable CORS policy. + */ +private predicate hasDangerousOrigins(MethodCall m) { + m.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", + "WithOrigins") and + m.getAnArgument().getValue() = ["null", "*"] +} + +/** + * Holds if the application allows an origin using "*" origin. + */ +private predicate allowAnyOrigin(MethodCall m) { + m.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", + "AllowAnyOrigin") +} + +/** + * Holds if UseCors is called with the revlevant cors policy + */ +private predicate configIsUsed(MethodCall add_policy) { + exists(MethodCall uc | + uc.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Builder.CorsMiddlewareExtensions", "UseCors") and + ( + uc.getArgument(1).getValue() = add_policy.getArgument(0).getValue() or + localFlow(DataFlow::exprNode(add_policy.getArgument(0)), DataFlow::exprNode(uc.getArgument(1))) + ) + ) +} + +from MethodCall add_policy, MethodCall m +where + ( + add_policy + .getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddPolicy") and + add_policy.getArgument(1) = m.getParent*() and + configIsUsed(add_policy) + or + add_policy + .getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", + "AddDefaultPolicy") and + add_policy.getArgument(0) = m.getParent*() + ) and + (hasDangerousOrigins(m) or allowAnyOrigin(m) or functionAlwaysReturnsTrue(m)) +select add_policy, "The following CORS policy may be vulnerable to 3rd party websites" diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql new file mode 100644 index 000000000000..e1288fd0c890 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql @@ -0,0 +1,84 @@ +/** + * @name Credentialed CORS Misconfiguration + * @description Allowing any origin while allowing credentials may result in security issues as third party website may be able to + * access private resources. + * @kind problem + * @problem.severity error + * @security-severity 7.5 + * @precision high + * @id cs/web/cors-misconfiguration-credentials + * @tags security + * external/cwe/cwe-942 + */ + +import csharp +private import DataFlow +import semmle.code.csharp.frameworks.system.Web + +/** + * Holds if credentials are allowed + */ +private predicate allowsCredentials(MethodCall credentials) { + credentials + .getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", + "AllowCredentials") +} + +/** + * Holds if SetIsOriginAllowed always returns true. This sets the Access-Control-Allow-Origin to the requester + */ +private predicate functionAlwaysReturnsTrue(MethodCall mc) { + mc.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", + "SetIsOriginAllowed") and + alwaysReturnsTrue(mc.getArgument(0)) +} + +/** + * Holds if `c` always returns `true`. + */ +private predicate alwaysReturnsTrue(Callable c) { + forex(ReturnStmt rs | rs.getEnclosingCallable() = c | + rs.getExpr().(BoolLiteral).getBoolValue() = true + ) + or + c.getBody().(BoolLiteral).getBoolValue() = true +} + +/** + * Holds if UseCors is called with the revlevant cors policy + */ +private predicate configIsUsed(MethodCall add_policy) { + exists(MethodCall uc | + uc.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Builder.CorsMiddlewareExtensions", "UseCors") and + ( + uc.getArgument(1).getValue() = add_policy.getArgument(0).getValue() or + uc.getArgument(1).(VariableAccess).getTarget() = + add_policy.getArgument(0).(VariableAccess).getTarget() or + localFlow(DataFlow::exprNode(add_policy.getArgument(0)), DataFlow::exprNode(uc.getArgument(1))) + ) + ) +} + +from MethodCall add_policy, MethodCall m, MethodCall allowsCredentials +where + ( + add_policy + .getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddPolicy") and + add_policy.getArgument(1) = m.getParent*() and + configIsUsed(add_policy) and + add_policy.getArgument(1) = allowsCredentials.getParent*() + or + add_policy + .getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", + "AddDefaultPolicy") and + add_policy.getArgument(0) = m.getParent*() and + add_policy.getArgument(0) = allowsCredentials.getParent*() + ) and + (allowsCredentials(allowsCredentials) and functionAlwaysReturnsTrue(m)) +select add_policy, + "The following CORS policy may allow credentialed requests from 3rd party websites" diff --git a/csharp/ql/src/experimental/CWE-942/examples/CORSBad.cs b/csharp/ql/src/experimental/CWE-942/examples/CORSBad.cs new file mode 100644 index 000000000000..214dbcd5263e --- /dev/null +++ b/csharp/ql/src/experimental/CWE-942/examples/CORSBad.cs @@ -0,0 +1,64 @@ +using Leaf.Middlewares; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; + +namespace Leaf +{ + public class Startup + { + public Startup(IConfiguration configuration) + { + Configuration = configuration; + } + + public IConfiguration Configuration { get; } + + // This method gets called by the runtime. Use this method to add services to the container. + public void ConfigureServices(IServiceCollection services) + { + services.AddControllers(); + //services.AddTransient(_ => new MySqlConnection(Configuration["ConnectionStrings:Default"])); + services.AddControllersWithViews() + .AddNewtonsoftJson(options => + options.SerializerSettings.ReferenceLoopHandling = Newtonsoft.Json.ReferenceLoopHandling.Ignore + ); + + services.AddCors(options => { + options.AddPolicy("AllowPolicy", builder => builder + .WithOrigins("null") + .AllowCredentials() + .AllowAnyMethod() + .AllowAnyHeader()); + }); + + } + + // This method gets called by the runtime. Use this method to configure the HTTP request pipeline. + public void Configure(IApplicationBuilder app, IWebHostEnvironment env) + { + app.UseRouting(); + + app.UseCors("AllowPolicy"); + + app.UseRequestResponseLogging(); + + if (env.IsDevelopment()) + { + app.UseDeveloperExceptionPage(); + } + + app.UseHttpsRedirection(); + + app.UseAuthorization(); + + app.UseEndpoints(endpoints => + { + endpoints.MapControllers(); + }); + + } + } +} diff --git a/csharp/ql/src/experimental/CWE-942/examples/CORSGood.cs b/csharp/ql/src/experimental/CWE-942/examples/CORSGood.cs new file mode 100644 index 000000000000..5bd5013d0e68 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-942/examples/CORSGood.cs @@ -0,0 +1,64 @@ +using Leaf.Middlewares; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; + +namespace Leaf +{ + public class Startup + { + public Startup(IConfiguration configuration) + { + Configuration = configuration; + } + + public IConfiguration Configuration { get; } + + // This method gets called by the runtime. Use this method to add services to the container. + public void ConfigureServices(IServiceCollection services) + { + services.AddControllers(); + //services.AddTransient(_ => new MySqlConnection(Configuration["ConnectionStrings:Default"])); + services.AddControllersWithViews() + .AddNewtonsoftJson(options => + options.SerializerSettings.ReferenceLoopHandling = Newtonsoft.Json.ReferenceLoopHandling.Ignore + ); + + services.AddCors(options => { + options.AddPolicy("AllowPolicy", builder => builder + .WithOrigins("http://example.com") + .AllowCredentials() + .AllowAnyMethod() + .AllowAnyHeader()); + }); + + } + + // This method gets called by the runtime. Use this method to configure the HTTP request pipeline. + public void Configure(IApplicationBuilder app, IWebHostEnvironment env) + { + app.UseRouting(); + + app.UseCors("AllowPolicy"); + + app.UseRequestResponseLogging(); + + if (env.IsDevelopment()) + { + app.UseDeveloperExceptionPage(); + } + + app.UseHttpsRedirection(); + + app.UseAuthorization(); + + app.UseEndpoints(endpoints => + { + endpoints.MapControllers(); + }); + + } + } +} diff --git a/csharp/ql/test/experimental/CWE-942/CORSMisconfiguration.txt b/csharp/ql/test/experimental/CWE-942/CORSMisconfiguration.txt deleted file mode 100644 index 4d66e855941b..000000000000 --- a/csharp/ql/test/experimental/CWE-942/CORSMisconfiguration.txt +++ /dev/null @@ -1,32 +0,0 @@ -using Microsoft.AspNetCore.Builder; -using Microsoft.AspNetCore.Mvc; -using System; - - -public class Startup -{ - public void ConfigureServices(IServiceCollection services) - { -var builder = WebApplication.CreateBuilder(args); -var MyAllowSpecificOrigins = "_myAllowSpecificOrigins"; - - -builder.Services.AddCors(options => -{ - options.AddPolicy(MyAllowSpecificOrigins, - policy => - { - policy.SetIsOriginAllowed(test => true).AllowCredentials().AllowAnyHeader().AllowAnyMethod(); - }); -}); - -var app = builder.Build(); - - - -app.MapGet("/", () => "Hello World!"); -app.UseCors(MyAllowSpecificOrigins); - -app.Run(); - } -} \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-942/CorsMiconfiguration.cs b/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs similarity index 100% rename from csharp/ql/test/experimental/CWE-942/CorsMiconfiguration.cs rename to csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref deleted file mode 100644 index cadcb0509195..000000000000 --- a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/CWE-942/CorsMisconfiguration.ql \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.qlref b/csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.qlref new file mode 100644 index 000000000000..5b17285c64b0 --- /dev/null +++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.qlref @@ -0,0 +1 @@ +experimental/CWE-942/CorsMisconfigurationCredentials.ql \ No newline at end of file From 44a1c3c28bfb07d4ad75996bf313a830af3fe4b4 Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Tue, 26 Nov 2024 16:20:20 -0800 Subject: [PATCH 3/8] Change names --- .../src/experimental/CWE-942/examples/{CORSBad.cs => CorsBad.cs} | 0 .../experimental/CWE-942/examples/{CORSGood.cs => CorsGood.cs} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename csharp/ql/src/experimental/CWE-942/examples/{CORSBad.cs => CorsBad.cs} (100%) rename csharp/ql/src/experimental/CWE-942/examples/{CORSGood.cs => CorsGood.cs} (100%) diff --git a/csharp/ql/src/experimental/CWE-942/examples/CORSBad.cs b/csharp/ql/src/experimental/CWE-942/examples/CorsBad.cs similarity index 100% rename from csharp/ql/src/experimental/CWE-942/examples/CORSBad.cs rename to csharp/ql/src/experimental/CWE-942/examples/CorsBad.cs diff --git a/csharp/ql/src/experimental/CWE-942/examples/CORSGood.cs b/csharp/ql/src/experimental/CWE-942/examples/CorsGood.cs similarity index 100% rename from csharp/ql/src/experimental/CWE-942/examples/CORSGood.cs rename to csharp/ql/src/experimental/CWE-942/examples/CorsGood.cs From ed2c7781d9a18e25ab2f37c25d8ae4b137a1637e Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Mon, 9 Dec 2024 17:52:09 -0800 Subject: [PATCH 4/8] Fixed tests --- .../CWE-942/CorsMisconfiguration.ql | 35 +++++++++++-------- .../CWE-942/CorsMiconfigurationCredentials.cs | 21 +++++++---- .../CWE-942/CorsMisconfiguration.cs | 34 ++++++++++++++++++ .../CWE-942/CorsMisconfiguration.expected | 2 ++ .../CWE-942/CorsMisconfiguration.qlref | 1 + .../CorsMisconfigurationCredentials.expected | 1 + csharp/ql/test/experimental/CWE-942/options | 1 - 7 files changed, 73 insertions(+), 22 deletions(-) create mode 100644 csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs create mode 100644 csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref create mode 100644 csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.expected diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index 8f75a4e7787a..7923c974402d 100644 --- a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -1,12 +1,12 @@ /** - * @name CORS misconfiguration - * @description Keeping an open CORS policy may result in security issues as third party website may be able to - * access other websites. + * @name Credentialed CORS Misconfiguration + * @description Allowing any origin while allowing credentials may result in security issues as third party website may be able to + * access private resources. * @kind problem * @problem.severity error * @security-severity 7.5 * @precision high - * @id cs/web/cors-misconfiguration + * @id cs/web/cors-misconfiguration-credentials * @tags security * external/cwe/cwe-942 */ @@ -37,22 +37,22 @@ private predicate alwaysReturnsTrue(Callable c) { } /** - * Holds if the application uses a vulnerable CORS policy. + * Holds if the application allows an origin using "*" origin. */ -private predicate hasDangerousOrigins(MethodCall m) { +private predicate allowAnyOrigin(MethodCall m) { m.getTarget() .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", - "WithOrigins") and - m.getAnArgument().getValue() = ["null", "*"] + "AllowAnyOrigin") } /** - * Holds if the application allows an origin using "*" origin. + * Holds if the application uses a vulnerable CORS policy. */ -private predicate allowAnyOrigin(MethodCall m) { +private predicate hasDangerousOrigins(MethodCall m) { m.getTarget() .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", - "AllowAnyOrigin") + "WithOrigins") and + m.getAnArgument().getValue() = ["null", "*"] } /** @@ -64,25 +64,30 @@ private predicate configIsUsed(MethodCall add_policy) { .hasFullyQualifiedName("Microsoft.AspNetCore.Builder.CorsMiddlewareExtensions", "UseCors") and ( uc.getArgument(1).getValue() = add_policy.getArgument(0).getValue() or + uc.getArgument(1).(VariableAccess).getTarget() = + add_policy.getArgument(0).(VariableAccess).getTarget() or localFlow(DataFlow::exprNode(add_policy.getArgument(0)), DataFlow::exprNode(uc.getArgument(1))) ) ) } -from MethodCall add_policy, MethodCall m +from MethodCall add_policy, MethodCall m, MethodCall allowsCredentials where ( add_policy .getTarget() .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddPolicy") and add_policy.getArgument(1) = m.getParent*() and - configIsUsed(add_policy) + configIsUsed(add_policy) and + add_policy.getArgument(1) = allowsCredentials.getParent*() or add_policy .getTarget() .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddDefaultPolicy") and - add_policy.getArgument(0) = m.getParent*() + add_policy.getArgument(0) = m.getParent*() and + add_policy.getArgument(0) = allowsCredentials.getParent*() ) and (hasDangerousOrigins(m) or allowAnyOrigin(m) or functionAlwaysReturnsTrue(m)) -select add_policy, "The following CORS policy may be vulnerable to 3rd party websites" +select add_policy, + "The following CORS policy may allow credentialed requests from 3rd party websites" diff --git a/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs b/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs index c5712603d7fd..c24f2eb2fedd 100644 --- a/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs +++ b/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs @@ -1,18 +1,25 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Mvc; using System; +using Microsoft.Extensions.DependencyInjection; + + +public class Startup +{ +    public void ConfigureServices(string[] args) +    { var builder = WebApplication.CreateBuilder(args); var MyAllowSpecificOrigins = "_myAllowSpecificOrigins"; builder.Services.AddCors(options => { - options.AddPolicy(MyAllowSpecificOrigins, - policy => - { - policy.SetIsOriginAllowed(test => true).AllowCredentials().AllowAnyHeader().AllowAnyMethod(); - }); +    options.AddPolicy(MyAllowSpecificOrigins, +                      policy => +                      { +                          policy.SetIsOriginAllowed(test => true).AllowCredentials().AllowAnyHeader().AllowAnyMethod(); +                      }); }); var app = builder.Build(); @@ -22,4 +29,6 @@ app.MapGet("/", () => "Hello World!"); app.UseCors(MyAllowSpecificOrigins); -app.Run(); \ No newline at end of file +app.Run(); +    } +} \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs new file mode 100644 index 000000000000..554a4ae5705c --- /dev/null +++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs @@ -0,0 +1,34 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Mvc; +using System; +using Microsoft.Extensions.DependencyInjection; + + + +public class Test +{ +    public void ConfigureServices(string[] args) +    { +var builder = WebApplication.CreateBuilder(args); +var MyAllowSpecificOrigins = "_myAllowSpecificOrigins"; + + +builder.Services.AddCors(options => +{ +    options.AddPolicy(MyAllowSpecificOrigins, +                      policy => +                      { +                          policy.AllowAnyOrigin().AllowCredentials().AllowAnyHeader().AllowAnyMethod(); +                      }); +}); + +var app = builder.Build(); + + + +app.MapGet("/", () => "Hello World!"); +app.UseCors(MyAllowSpecificOrigins); + +app.Run(); +    } +} \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected index e69de29bb2d1..b268c1a686a5 100644 --- a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected +++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected @@ -0,0 +1,2 @@ +| CorsMiconfigurationCredentials.cs:18:5:22:24 | call to method AddPolicy | The following CORS policy may allow credentialed requests from 3rd party websites | +| CorsMisconfiguration.cs:18:5:22:24 | call to method AddPolicy | The following CORS policy may allow credentialed requests from 3rd party websites | diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref new file mode 100644 index 000000000000..cadcb0509195 --- /dev/null +++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref @@ -0,0 +1 @@ +experimental/CWE-942/CorsMisconfiguration.ql \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.expected b/csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.expected new file mode 100644 index 000000000000..390c1e5b2eec --- /dev/null +++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfigurationCredentials.expected @@ -0,0 +1 @@ +| CorsMiconfigurationCredentials.cs:18:5:22:24 | call to method AddPolicy | The following CORS policy may allow credentialed requests from 3rd party websites | diff --git a/csharp/ql/test/experimental/CWE-942/options b/csharp/ql/test/experimental/CWE-942/options index 67d8fa711a4a..f6d138f82b77 100644 --- a/csharp/ql/test/experimental/CWE-942/options +++ b/csharp/ql/test/experimental/CWE-942/options @@ -1,5 +1,4 @@ semmle-extractor-options: /nostdlib /noconfig semmle-extractor-options: --load-sources-from-project:${testdir}/../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj semmle-extractor-options: --load-sources-from-project:../../resources/stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.App.csproj -semmle-extractor-options: --load-sources-from-project:../../resources/stubs/Microsoft.Extensions.DependencyInjection.Abstractions/6.0.0/Microsoft.Extensions.DependencyInjection.Abstractions.csproj From ad62989327cdebd1a308e7f92faf53d37c11befd Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Mon, 9 Dec 2024 19:29:21 -0800 Subject: [PATCH 5/8] id stuff --- csharp/ql/src/change-notes/2024-11-26-cors-query.md | 2 +- csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/csharp/ql/src/change-notes/2024-11-26-cors-query.md b/csharp/ql/src/change-notes/2024-11-26-cors-query.md index 77df5ba3a398..281bb312e64a 100644 --- a/csharp/ql/src/change-notes/2024-11-26-cors-query.md +++ b/csharp/ql/src/change-notes/2024-11-26-cors-query.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* C#: Added `ccs/web/cors-misconfiguration` queries which looks for CORS misconfigurations with and without credentials. \ No newline at end of file +* C#: Added `cs/web/cors-misconfiguration` queries which looks for CORS misconfigurations with and without credentials. \ No newline at end of file diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index 7923c974402d..e6758e09e833 100644 --- a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -6,7 +6,7 @@ * @problem.severity error * @security-severity 7.5 * @precision high - * @id cs/web/cors-misconfiguration-credentials + * @id cs/web/cors-misconfiguration * @tags security * external/cwe/cwe-942 */ From 7e2b8314ca4de0237292517bf6d59e36f0da923c Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Fri, 13 Dec 2024 16:31:39 -0800 Subject: [PATCH 6/8] First round feedback --- .../CWE-942/CorsMisconfiguration.ql | 63 +++--------- .../CorsMisconfigurationCredentials.ql | 62 +++--------- .../CWE-942/CorsMisconfigurationLib.qll | 99 +++++++++++++++++++ .../CWE-942/CorsMisconfiguration.cs | 39 +++----- .../CWE-942/CorsMisconfiguration.expected | 3 +- 5 files changed, 142 insertions(+), 124 deletions(-) create mode 100644 csharp/ql/src/experimental/CWE-942/CorsMisconfigurationLib.qll diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index e6758e09e833..a4e96d9015ce 100644 --- a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -14,27 +14,7 @@ import csharp private import DataFlow import semmle.code.csharp.frameworks.system.Web - -/** - * Holds if SetIsOriginAllowed always returns true. This sets the Access-Control-Allow-Origin to the requester - */ -private predicate functionAlwaysReturnsTrue(MethodCall mc) { - mc.getTarget() - .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", - "SetIsOriginAllowed") and - alwaysReturnsTrue(mc.getArgument(0)) -} - -/** - * Holds if `c` always returns `true`. - */ -private predicate alwaysReturnsTrue(Callable c) { - forex(ReturnStmt rs | rs.getEnclosingCallable() = c | - rs.getExpr().(BoolLiteral).getBoolValue() = true - ) - or - c.getBody().(BoolLiteral).getBoolValue() = true -} +import CorsMisconfigurationLib /** * Holds if the application allows an origin using "*" origin. @@ -52,42 +32,29 @@ private predicate hasDangerousOrigins(MethodCall m) { m.getTarget() .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", "WithOrigins") and - m.getAnArgument().getValue() = ["null", "*"] -} - -/** - * Holds if UseCors is called with the revlevant cors policy - */ -private predicate configIsUsed(MethodCall add_policy) { - exists(MethodCall uc | - uc.getTarget() - .hasFullyQualifiedName("Microsoft.AspNetCore.Builder.CorsMiddlewareExtensions", "UseCors") and - ( - uc.getArgument(1).getValue() = add_policy.getArgument(0).getValue() or - uc.getArgument(1).(VariableAccess).getTarget() = - add_policy.getArgument(0).(VariableAccess).getTarget() or - localFlow(DataFlow::exprNode(add_policy.getArgument(0)), DataFlow::exprNode(uc.getArgument(1))) + ( + m.getAnArgument().getValue() = ["null", "*"] + or + exists(StringLiteral idStr | + idStr.getValue().toLowerCase().matches(["null", "*"]) and + DataFlow::localExprFlow(idStr, m.getAnArgument()) ) ) } -from MethodCall add_policy, MethodCall m, MethodCall allowsCredentials +from MethodCall add_policy, MethodCall child where ( - add_policy - .getTarget() - .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddPolicy") and - add_policy.getArgument(1) = m.getParent*() and - configIsUsed(add_policy) and - add_policy.getArgument(1) = allowsCredentials.getParent*() + usedPolicy(add_policy) and + // Misconfigured origin affects used policy + add_policy.getArgument(1) = child.getParent*() or add_policy .getTarget() .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddDefaultPolicy") and - add_policy.getArgument(0) = m.getParent*() and - add_policy.getArgument(0) = allowsCredentials.getParent*() + // Misconfigured origin affects added default policy + add_policy.getArgument(0) = child.getParent*() ) and - (hasDangerousOrigins(m) or allowAnyOrigin(m) or functionAlwaysReturnsTrue(m)) -select add_policy, - "The following CORS policy may allow credentialed requests from 3rd party websites" + (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 e1288fd0c890..b152f95dd2c9 100644 --- a/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql +++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql @@ -14,71 +14,33 @@ import csharp private import DataFlow import semmle.code.csharp.frameworks.system.Web +import CorsMisconfigurationLib /** * Holds if credentials are allowed */ -private predicate allowsCredentials(MethodCall credentials) { - credentials - .getTarget() - .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", - "AllowCredentials") +class AllowsCredentials extends MethodCall { + AllowsCredentials() { + this.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", + "AllowCredentials") + } } -/** - * Holds if SetIsOriginAllowed always returns true. This sets the Access-Control-Allow-Origin to the requester - */ -private predicate functionAlwaysReturnsTrue(MethodCall mc) { - mc.getTarget() - .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", - "SetIsOriginAllowed") and - alwaysReturnsTrue(mc.getArgument(0)) -} - -/** - * Holds if `c` always returns `true`. - */ -private predicate alwaysReturnsTrue(Callable c) { - forex(ReturnStmt rs | rs.getEnclosingCallable() = c | - rs.getExpr().(BoolLiteral).getBoolValue() = true - ) - or - c.getBody().(BoolLiteral).getBoolValue() = true -} - -/** - * Holds if UseCors is called with the revlevant cors policy - */ -private predicate configIsUsed(MethodCall add_policy) { - exists(MethodCall uc | - uc.getTarget() - .hasFullyQualifiedName("Microsoft.AspNetCore.Builder.CorsMiddlewareExtensions", "UseCors") and - ( - uc.getArgument(1).getValue() = add_policy.getArgument(0).getValue() or - uc.getArgument(1).(VariableAccess).getTarget() = - add_policy.getArgument(0).(VariableAccess).getTarget() or - localFlow(DataFlow::exprNode(add_policy.getArgument(0)), DataFlow::exprNode(uc.getArgument(1))) - ) - ) -} - -from MethodCall add_policy, MethodCall m, MethodCall allowsCredentials +from MethodCall add_policy, MethodCall setIsOriginAllowed, AllowsCredentials allowsCredentials where ( - add_policy - .getTarget() - .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddPolicy") and - add_policy.getArgument(1) = m.getParent*() and - configIsUsed(add_policy) and + add_policy.getArgument(1) = setIsOriginAllowed.getParent*() and + usedPolicy(add_policy) and add_policy.getArgument(1) = allowsCredentials.getParent*() or add_policy .getTarget() .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddDefaultPolicy") and - add_policy.getArgument(0) = m.getParent*() and + add_policy.getArgument(0) = setIsOriginAllowed.getParent*() and add_policy.getArgument(0) = allowsCredentials.getParent*() ) and - (allowsCredentials(allowsCredentials) and functionAlwaysReturnsTrue(m)) + setIsOriginAllowedReturnsTrue(setIsOriginAllowed) select add_policy, "The following CORS policy may allow credentialed requests from 3rd party websites" diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationLib.qll b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationLib.qll new file mode 100644 index 000000000000..301dd354a39c --- /dev/null +++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationLib.qll @@ -0,0 +1,99 @@ +import csharp +import DataFlow + +/** + * Holds if the `Callable` c throws any exception other than `ThrowsArgumentNullException` + */ +predicate callableMayThrowException(Callable c) { + exists(ThrowStmt thre | c = thre.getEnclosingCallable()) and + not callableOnlyThrowsArgumentNullException(c) +} + +/** + * Holds if any exception being thrown by the callable is of type `System.ArgumentNullException` + * It will also hold if no exceptions are thrown by the callable + */ +predicate callableOnlyThrowsArgumentNullException(Callable c) { + forall(ThrowElement thre | c = thre.getEnclosingCallable() | + thre.getThrownExceptionType().hasFullyQualifiedName("System", "ArgumentNullException") + ) +} + +/** + * Hold if the `Expr` e is a `BoolLiteral` with value true, + * the expression has a predictable value == `true`, + * or if it is a `ConditionalExpr` where the `then` and `else` expressions meet `isExpressionAlwaysTrue` criteria + */ +predicate isExpressionAlwaysTrue(Expr e) { + e.(BoolLiteral).getBoolValue() = true + or + e.getValue() = "true" + or + e instanceof ConditionalExpr and + isExpressionAlwaysTrue(e.(ConditionalExpr).getThen()) and + isExpressionAlwaysTrue(e.(ConditionalExpr).getElse()) + or + exists(Callable callable | + callableHasAReturnStmtAndAlwaysReturnsTrue(callable) and + callable.getACall() = e + ) +} + +/** + * Holds if the lambda expression `le` always returns true + */ +predicate lambdaExprReturnsOnlyLiteralTrue(AnonymousFunctionExpr le) { + isExpressionAlwaysTrue(le.getExpressionBody()) +} + +/** + * Holds if the callable has a return statement and it always returns true for all such statements + */ +predicate callableHasAReturnStmtAndAlwaysReturnsTrue(Callable c) { + c.getReturnType() instanceof BoolType and + not callableMayThrowException(c) and + forex(ReturnStmt rs | rs.getEnclosingCallable() = c | + rs.getNumberOfChildren() = 1 and + isExpressionAlwaysTrue(rs.getChildExpr(0)) + ) +} + +/** + * Holds if `c` always returns `true`. + */ +private predicate alwaysReturnsTrue(Callable c) { + callableHasAReturnStmtAndAlwaysReturnsTrue(c) + or + lambdaExprReturnsOnlyLiteralTrue(c) +} + +/** + * Holds if SetIsOriginAllowed always returns true. This sets the Access-Control-Allow-Origin to the requester + */ +predicate setIsOriginAllowedReturnsTrue(MethodCall mc) { + mc.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", + "SetIsOriginAllowed") and + alwaysReturnsTrue(mc.getArgument(0)) +} + +/** + * Holds if UseCors is called with the relevant cors policy + */ +predicate usedPolicy(MethodCall add_policy) { + exists(MethodCall uc | + uc.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Builder.CorsMiddlewareExtensions", "UseCors") and + ( + // Same hardcoded name + uc.getArgument(1).getValue() = add_policy.getArgument(0).getValue() or + // Same variable access + uc.getArgument(1).(VariableAccess).getTarget() = + add_policy.getArgument(0).(VariableAccess).getTarget() or + DataFlow::localExprFlow(add_policy.getArgument(0), uc.getArgument(1)) + ) + ) and + add_policy + .getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddPolicy") +} diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs index 554a4ae5705c..8cf87d73220a 100644 --- a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs +++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs @@ -3,32 +3,23 @@ using System; using Microsoft.Extensions.DependencyInjection; +public class Test { + public void ConfigureServices(string[] args) { + var builder = WebApplication.CreateBuilder(args); + var MyAllowSpecificOrigins = "_myAllowSpecificOrigins"; + builder.Services.AddCors(options => { + options.AddPolicy(MyAllowSpecificOrigins, + policy => { + policy.AllowAnyOrigin().AllowCredentials().AllowAnyHeader().AllowAnyMethod(); + }); + }); -public class Test -{ -    public void ConfigureServices(string[] args) -    { -var builder = WebApplication.CreateBuilder(args); -var MyAllowSpecificOrigins = "_myAllowSpecificOrigins"; + var app = builder.Build(); + app.MapGet("/", () => "Hello World!"); + app.UseCors(MyAllowSpecificOrigins); -builder.Services.AddCors(options => -{ -    options.AddPolicy(MyAllowSpecificOrigins, -                      policy => -                      { -                          policy.AllowAnyOrigin().AllowCredentials().AllowAnyHeader().AllowAnyMethod(); -                      }); -}); - -var app = builder.Build(); - - - -app.MapGet("/", () => "Hello World!"); -app.UseCors(MyAllowSpecificOrigins); - -app.Run(); -    } + app.Run(); + } } \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected index b268c1a686a5..9ea84114ecca 100644 --- a/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected +++ b/csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected @@ -1,2 +1 @@ -| CorsMiconfigurationCredentials.cs:18:5:22:24 | call to method AddPolicy | The following CORS policy may allow credentialed requests from 3rd party websites | -| CorsMisconfiguration.cs:18:5:22:24 | call to method AddPolicy | The following CORS policy may allow credentialed requests from 3rd party websites | +| CorsMisconfiguration.cs:12:7:15:10 | call to method AddPolicy | The following CORS policy may allow requests from 3rd party websites | From 2a4ed45edc7509c6427184b62ad7cc692fbfa9d8 Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Fri, 13 Dec 2024 16:43:47 -0800 Subject: [PATCH 7/8] Formatting --- .../CWE-942/CorsMisconfiguration.ql | 2 - .../CorsMisconfigurationCredentials.ql | 6 +-- .../CWE-942/CorsMiconfigurationCredentials.cs | 39 +++++++------------ 3 files changed, 16 insertions(+), 31 deletions(-) diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index a4e96d9015ce..492e3e97b8d8 100644 --- a/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -12,8 +12,6 @@ */ import csharp -private import DataFlow -import semmle.code.csharp.frameworks.system.Web import CorsMisconfigurationLib /** diff --git a/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql index b152f95dd2c9..29b7dc3aa107 100644 --- a/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql +++ b/csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql @@ -12,13 +12,9 @@ */ import csharp -private import DataFlow -import semmle.code.csharp.frameworks.system.Web import CorsMisconfigurationLib -/** - * Holds if credentials are allowed - */ +/** A call to `CorsPolicyBuilder.AllowCredentials`. */ class AllowsCredentials extends MethodCall { AllowsCredentials() { this.getTarget() diff --git a/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs b/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs index c24f2eb2fedd..8565421e00f2 100644 --- a/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs +++ b/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs @@ -3,32 +3,23 @@ using System; using Microsoft.Extensions.DependencyInjection; +public class Startup { + public void ConfigureServices(string[] args) { + var builder = WebApplication.CreateBuilder(args); + var MyAllowSpecificOrigins = "_myAllowSpecificOrigins"; + builder.Services.AddCors(options => { + options.AddPolicy(MyAllowSpecificOrigins, + policy => { + policy.SetIsOriginAllowed(test => true).AllowCredentials().AllowAnyHeader().AllowAnyMethod(); + }); + }); -public class Startup -{ -    public void ConfigureServices(string[] args) -    { -var builder = WebApplication.CreateBuilder(args); -var MyAllowSpecificOrigins = "_myAllowSpecificOrigins"; + var app = builder.Build(); + app.MapGet("/", () => "Hello World!"); + app.UseCors(MyAllowSpecificOrigins); -builder.Services.AddCors(options => -{ -    options.AddPolicy(MyAllowSpecificOrigins, -                      policy => -                      { -                          policy.SetIsOriginAllowed(test => true).AllowCredentials().AllowAnyHeader().AllowAnyMethod(); -                      }); -}); - -var app = builder.Build(); - - - -app.MapGet("/", () => "Hello World!"); -app.UseCors(MyAllowSpecificOrigins); - -app.Run(); -    } + app.Run(); + } } \ No newline at end of file From 374eec5f9a3fe6731463389b454c3281b3763bcc Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Tue, 17 Dec 2024 15:13:55 -0800 Subject: [PATCH 8/8] 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 |