-
Notifications
You must be signed in to change notification settings - Fork 1.9k
C#: Add csharp cors query #18120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
C#: Add csharp cors query #18120
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
a4fb0e0
Added tests
Kwstubbs 3037c79
Add changelog
Kwstubbs 44a1c3c
Change names
Kwstubbs ed2c778
Fixed tests
Kwstubbs ad62989
id stuff
Kwstubbs 7e2b831
First round feedback
Kwstubbs 2a4ed45
Formatting
Kwstubbs 374eec5
Second round feedback
Kwstubbs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * C#: Added `cs/web/cors-misconfiguration` queries which looks for CORS misconfigurations with and without credentials. |
85 changes: 85 additions & 0 deletions
85
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| <!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 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 | ||
| 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>. | ||
|
|
||
| </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="examples/CorsBad.cs"/> | ||
|
|
||
| <p> | ||
|
|
||
| This is not secure, since an attacker can choose the value of | ||
| the <code>origin</code> 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: | ||
|
|
||
| </p> | ||
|
|
||
| <sample src="examples/CorsGood.cs"/> | ||
| </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> |
93 changes: 93 additions & 0 deletions
93
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| /** | ||
| * @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 | ||
| * @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) { | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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) { | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| forex(ReturnStmt rs | rs.getEnclosingCallable() = c | | ||
| rs.getExpr().(BoolLiteral).getBoolValue() = true | ||
| ) | ||
| or | ||
| c.getBody().(BoolLiteral).getBoolValue() = true | ||
| } | ||
|
|
||
| /** | ||
| * 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 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", "*"] | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** | ||
| * Holds if UseCors is called with the revlevant cors policy | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| */ | ||
| 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 | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| uc.getArgument(1).(VariableAccess).getTarget() = | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| add_policy.getArgument(0).(VariableAccess).getTarget() or | ||
| localFlow(DataFlow::exprNode(add_policy.getArgument(0)), DataFlow::exprNode(uc.getArgument(1))) | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ) | ||
| ) | ||
| } | ||
|
|
||
| from MethodCall add_policy, MethodCall m, MethodCall allowsCredentials | ||
| where | ||
| ( | ||
| add_policy | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| .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*() | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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 | ||
| (hasDangerousOrigins(m) or allowAnyOrigin(m) or functionAlwaysReturnsTrue(m)) | ||
| select add_policy, | ||
| "The following CORS policy may allow credentialed requests from 3rd party websites" | ||
84 changes: 84 additions & 0 deletions
84
csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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))) | ||
| ) | ||
| ) | ||
| } | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| 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*() | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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*() | ||
Kwstubbs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ) and | ||
| (allowsCredentials(allowsCredentials) and functionAlwaysReturnsTrue(m)) | ||
| select add_policy, | ||
| "The following CORS policy may allow credentialed requests from 3rd party websites" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<MySqlConnection>(_ => 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(); | ||
| }); | ||
|
|
||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.