Skip to content
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

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions csharp/ql/src/change-notes/2024-11-26-cors-query.md
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 csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp
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>
54 changes: 54 additions & 0 deletions csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/**
* @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
import CorsMisconfigurationLib

/**
* 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
exists(StringLiteral idStr |
idStr.getValue().toLowerCase().matches(["null", "*"]) and
TaintTracking::localExprTaint(idStr, m.getAnArgument())
)
}

from MethodCall add_policy, MethodCall child
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
where
(
usedPolicy(add_policy) and
// Misconfigured origin affects used policy
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
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"
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* @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
import CorsMisconfigurationLib

/** A call to `CorsPolicyBuilder.AllowCredentials`. */
class AllowsCredentials extends MethodCall {
AllowsCredentials() {
this.getTarget()
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder",
"AllowCredentials")
}
}

from MethodCall add_policy, MethodCall setIsOriginAllowed, AllowsCredentials allowsCredentials
where
(
getCallableFromExpr(add_policy.getArgument(1)).calls*(setIsOriginAllowed.getTarget()) and
usedPolicy(add_policy) and
getCallableFromExpr(add_policy.getArgument(1)).calls*(allowsCredentials.getTarget())
or
add_policy
.getTarget()
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions",
"AddDefaultPolicy") and
getCallableFromExpr(add_policy.getArgument(0)).calls*(setIsOriginAllowed.getTarget()) and
getCallableFromExpr(add_policy.getArgument(0)).calls*(allowsCredentials.getTarget())
) and
setIsOriginAllowedReturnsTrue(setIsOriginAllowed)
select add_policy,
"The following CORS policy may allow credentialed requests from 3rd party websites"
111 changes: 111 additions & 0 deletions csharp/ql/src/experimental/CWE-942/CorsMisconfigurationLib.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
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`
*/
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) {
Copy link
Contributor

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.

Copy link
Contributor Author

@Kwstubbs Kwstubbs Dec 16, 2024

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.

Copy link
Contributor

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.

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")
}
64 changes: 64 additions & 0 deletions csharp/ql/src/experimental/CWE-942/examples/CorsBad.cs
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();
});

}
}
}
Loading
Loading