-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you looking into the comments. I have added a second round of comments, which could improve the precision of the query.
csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql
Outdated
Show resolved
Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql
Outdated
Show resolved
Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql
Outdated
Show resolved
Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql
Outdated
Show resolved
Hide resolved
/** | ||
* Holds if `c` always returns `true`. | ||
*/ | ||
private predicate alwaysReturnsTrue(Callable c) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
Thank you :-)
If you are interested, you can try and use DCA for testing the new query (this can also be done when it is included in the CodeQL community pack).
Also, once again, I apologise if I went a bit overboard with review suggestions for improvements!
Merged in Community Pack |
Added CORS queries for C# MVC