C#: Add csharp cors query #18120
C#: Add csharp cors query #18120
Conversation
Fix issues Change this
|
QHelp previews: csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelpCredentialed CORS MisconfigurationA server can send the When the RecommendationWhen the Since the ExampleIn the example below, the server allows the browser to send user credentials in a Cross-Origin request. The request header 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();
});
}
}
}This is not secure, since an attacker can choose the value of the 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("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();
});
}
}
}References
|
|
@github/codeql-csharp Hey team, I am having problems with building the database for the test in this PR. Any recommendations would be great. I get the following error. I have wrapped the test in the following class to get rid of the first error: public class Startup
{
public void ConfigureServices(IServiceCollection services)
{New Errors: |
Thank you for doing this! |
There was a problem hiding this comment.
Thank you for doing this!
I have added an initial set of comments and clarifying questions.
We are also in the process of moving experimental queries to https://github.com/GitHubSecurityLab/CodeQL-Community-Packs/
Do you want to add the query there instead?
|
@michaelnebel Lets do the review process here to make sure all the checks are good and then I will open a PR in the Community Pack 😄 |
| .hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder", | ||
| "WithOrigins") and | ||
| ( | ||
| m.getAnArgument().getValue() = ["null", "*"] |
There was a problem hiding this comment.
Testing for .WithOrigins(["*"]) doesn't work with the new DataFlow::localExprFlow(idStr, m.getAnArgument()) addition or with what I have deleted. I'll add support for this soon.
| ) | ||
| } | ||
|
|
||
| from MethodCall add_policy, MethodCall child |
There was a problem hiding this comment.
Another approach for writing the query would be to use inter-procedural (global) dataflow.
To handle the case with the builder we would need to define the set of sources to be the unsafe builders and the sinks would be the call to AddPolicy. This could increase the precision of the query.
There was a problem hiding this comment.
To be honest, I don't think it will be worth my time to write anything more involved that what I currently have. This query was initially a query I used for security research and I have am done using it now. Most C# software saves authentication in places other than cookies so the number of projects affected by CORS issues that are actually exploitable are pretty small. I think its worth alerting on to let the developer know of the unsafe setting and I'm happy to improve it to the point of being serviceable to code scanning.
There was a problem hiding this comment.
That makes a lot of sense! I apologise if all this stuff adds to much scope creep - that was not my intention - I just got carried away thinking about the query when review'ing it :-) The query is definitely valuable it its current state without converting into using global dataflow and I understand that there is limit to the level of time and details that can be addressed!
Once again, thank you making the query in the first place!
| /** | ||
| * Holds if `c` always returns `true`. | ||
| */ | ||
| private predicate alwaysReturnsTrue(Callable c) { |
There was a problem hiding this comment.
When the PR to the community lab is opened, it would be great, if this code is not copied, but re-used with the implementation where it was copied from.
There was a problem hiding this comment.
Sure, how do I import a library with in Security Features folder (theres a space in the import?). It just seemed like a "library" of another query so I didn't want my query to be dependent on it if there are changes.
There was a problem hiding this comment.
Spaces in paths are turned into _, so in this case Security_Features.
For code that is shared between queries, I would recommend moving it to the library pack in the CodeQL community pack.
Added CORS queries for C# MVC