Skip to content

Commit 7e2b831

Browse files
KwstubbsKwstubbs
committed
First round feedback
1 parent ad62989 commit 7e2b831

File tree

5 files changed

+142
-124
lines changed

5 files changed

+142
-124
lines changed

‎csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql‎

Lines changed: 15 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -14,27 +14,7 @@
1414
import csharp
1515
private import DataFlow
1616
import semmle.code.csharp.frameworks.system.Web
17-
18-
/**
19-
* Holds if SetIsOriginAllowed always returns true. This sets the Access-Control-Allow-Origin to the requester
20-
*/
21-
private predicate functionAlwaysReturnsTrue(MethodCall mc) {
22-
mc.getTarget()
23-
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder",
24-
"SetIsOriginAllowed") and
25-
alwaysReturnsTrue(mc.getArgument(0))
26-
}
27-
28-
/**
29-
* Holds if `c` always returns `true`.
30-
*/
31-
private predicate alwaysReturnsTrue(Callable c) {
32-
forex(ReturnStmt rs | rs.getEnclosingCallable() = c |
33-
rs.getExpr().(BoolLiteral).getBoolValue() = true
34-
)
35-
or
36-
c.getBody().(BoolLiteral).getBoolValue() = true
37-
}
17+
import CorsMisconfigurationLib
3818

3919
/**
4020
* Holds if the application allows an origin using "*" origin.
@@ -52,42 +32,29 @@ private predicate hasDangerousOrigins(MethodCall m) {
5232
m.getTarget()
5333
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder",
5434
"WithOrigins") and
55-
m.getAnArgument().getValue() = ["null", "*"]
56-
}
57-
58-
/**
59-
* Holds if UseCors is called with the revlevant cors policy
60-
*/
61-
private predicate configIsUsed(MethodCall add_policy) {
62-
exists(MethodCall uc |
63-
uc.getTarget()
64-
.hasFullyQualifiedName("Microsoft.AspNetCore.Builder.CorsMiddlewareExtensions", "UseCors") and
65-
(
66-
uc.getArgument(1).getValue() = add_policy.getArgument(0).getValue() or
67-
uc.getArgument(1).(VariableAccess).getTarget() =
68-
add_policy.getArgument(0).(VariableAccess).getTarget() or
69-
localFlow(DataFlow::exprNode(add_policy.getArgument(0)), DataFlow::exprNode(uc.getArgument(1)))
35+
(
36+
m.getAnArgument().getValue() = ["null", "*"]
37+
or
38+
exists(StringLiteral idStr |
39+
idStr.getValue().toLowerCase().matches(["null", "*"]) and
40+
DataFlow::localExprFlow(idStr, m.getAnArgument())
7041
)
7142
)
7243
}
7344

74-
from MethodCall add_policy, MethodCall m, MethodCall allowsCredentials
45+
from MethodCall add_policy, MethodCall child
7546
where
7647
(
77-
add_policy
78-
.getTarget()
79-
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddPolicy") and
80-
add_policy.getArgument(1) = m.getParent*() and
81-
configIsUsed(add_policy) and
82-
add_policy.getArgument(1) = allowsCredentials.getParent*()
48+
usedPolicy(add_policy) and
49+
// Misconfigured origin affects used policy
50+
add_policy.getArgument(1) = child.getParent*()
8351
or
8452
add_policy
8553
.getTarget()
8654
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions",
8755
"AddDefaultPolicy") and
88-
add_policy.getArgument(0) = m.getParent*() and
89-
add_policy.getArgument(0) = allowsCredentials.getParent*()
56+
// Misconfigured origin affects added default policy
57+
add_policy.getArgument(0) = child.getParent*()
9058
) and
91-
(hasDangerousOrigins(m) or allowAnyOrigin(m) or functionAlwaysReturnsTrue(m))
92-
select add_policy,
93-
"The following CORS policy may allow credentialed requests from 3rd party websites"
59+
(hasDangerousOrigins(child) or allowAnyOrigin(child))
60+
select add_policy, "The following CORS policy may allow requests from 3rd party websites"

‎csharp/ql/src/experimental/CWE-942/CorsMisconfigurationCredentials.ql‎

Lines changed: 12 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -14,71 +14,33 @@
1414
import csharp
1515
private import DataFlow
1616
import semmle.code.csharp.frameworks.system.Web
17+
import CorsMisconfigurationLib
1718

1819
/**
1920
* Holds if credentials are allowed
2021
*/
21-
private predicate allowsCredentials(MethodCall credentials) {
22-
credentials
23-
.getTarget()
24-
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder",
25-
"AllowCredentials")
22+
class AllowsCredentials extends MethodCall {
23+
AllowsCredentials() {
24+
this.getTarget()
25+
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder",
26+
"AllowCredentials")
27+
}
2628
}
2729

28-
/**
29-
* Holds if SetIsOriginAllowed always returns true. This sets the Access-Control-Allow-Origin to the requester
30-
*/
31-
private predicate functionAlwaysReturnsTrue(MethodCall mc) {
32-
mc.getTarget()
33-
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder",
34-
"SetIsOriginAllowed") and
35-
alwaysReturnsTrue(mc.getArgument(0))
36-
}
37-
38-
/**
39-
* Holds if `c` always returns `true`.
40-
*/
41-
private predicate alwaysReturnsTrue(Callable c) {
42-
forex(ReturnStmt rs | rs.getEnclosingCallable() = c |
43-
rs.getExpr().(BoolLiteral).getBoolValue() = true
44-
)
45-
or
46-
c.getBody().(BoolLiteral).getBoolValue() = true
47-
}
48-
49-
/**
50-
* Holds if UseCors is called with the revlevant cors policy
51-
*/
52-
private predicate configIsUsed(MethodCall add_policy) {
53-
exists(MethodCall uc |
54-
uc.getTarget()
55-
.hasFullyQualifiedName("Microsoft.AspNetCore.Builder.CorsMiddlewareExtensions", "UseCors") and
56-
(
57-
uc.getArgument(1).getValue() = add_policy.getArgument(0).getValue() or
58-
uc.getArgument(1).(VariableAccess).getTarget() =
59-
add_policy.getArgument(0).(VariableAccess).getTarget() or
60-
localFlow(DataFlow::exprNode(add_policy.getArgument(0)), DataFlow::exprNode(uc.getArgument(1)))
61-
)
62-
)
63-
}
64-
65-
from MethodCall add_policy, MethodCall m, MethodCall allowsCredentials
30+
from MethodCall add_policy, MethodCall setIsOriginAllowed, AllowsCredentials allowsCredentials
6631
where
6732
(
68-
add_policy
69-
.getTarget()
70-
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddPolicy") and
71-
add_policy.getArgument(1) = m.getParent*() and
72-
configIsUsed(add_policy) and
33+
add_policy.getArgument(1) = setIsOriginAllowed.getParent*() and
34+
usedPolicy(add_policy) and
7335
add_policy.getArgument(1) = allowsCredentials.getParent*()
7436
or
7537
add_policy
7638
.getTarget()
7739
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions",
7840
"AddDefaultPolicy") and
79-
add_policy.getArgument(0) = m.getParent*() and
41+
add_policy.getArgument(0) = setIsOriginAllowed.getParent*() and
8042
add_policy.getArgument(0) = allowsCredentials.getParent*()
8143
) and
82-
(allowsCredentials(allowsCredentials) and functionAlwaysReturnsTrue(m))
44+
setIsOriginAllowedReturnsTrue(setIsOriginAllowed)
8345
select add_policy,
8446
"The following CORS policy may allow credentialed requests from 3rd party websites"

‎csharp/ql/src/experimental/CWE-942/CorsMisconfigurationLib.qll‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
import csharp
2+
import DataFlow
3+
4+
/**
5+
* Holds if the `Callable` c throws any exception other than `ThrowsArgumentNullException`
6+
*/
7+
predicate callableMayThrowException(Callable c) {
8+
exists(ThrowStmt thre | c = thre.getEnclosingCallable()) and
9+
not callableOnlyThrowsArgumentNullException(c)
10+
}
11+
12+
/**
13+
* Holds if any exception being thrown by the callable is of type `System.ArgumentNullException`
14+
* It will also hold if no exceptions are thrown by the callable
15+
*/
16+
predicate callableOnlyThrowsArgumentNullException(Callable c) {
17+
forall(ThrowElement thre | c = thre.getEnclosingCallable() |
18+
thre.getThrownExceptionType().hasFullyQualifiedName("System", "ArgumentNullException")
19+
)
20+
}
21+
22+
/**
23+
* Hold if the `Expr` e is a `BoolLiteral` with value true,
24+
* the expression has a predictable value == `true`,
25+
* or if it is a `ConditionalExpr` where the `then` and `else` expressions meet `isExpressionAlwaysTrue` criteria
26+
*/
27+
predicate isExpressionAlwaysTrue(Expr e) {
28+
e.(BoolLiteral).getBoolValue() = true
29+
or
30+
e.getValue() = "true"
31+
or
32+
e instanceof ConditionalExpr and
33+
isExpressionAlwaysTrue(e.(ConditionalExpr).getThen()) and
34+
isExpressionAlwaysTrue(e.(ConditionalExpr).getElse())
35+
or
36+
exists(Callable callable |
37+
callableHasAReturnStmtAndAlwaysReturnsTrue(callable) and
38+
callable.getACall() = e
39+
)
40+
}
41+
42+
/**
43+
* Holds if the lambda expression `le` always returns true
44+
*/
45+
predicate lambdaExprReturnsOnlyLiteralTrue(AnonymousFunctionExpr le) {
46+
isExpressionAlwaysTrue(le.getExpressionBody())
47+
}
48+
49+
/**
50+
* Holds if the callable has a return statement and it always returns true for all such statements
51+
*/
52+
predicate callableHasAReturnStmtAndAlwaysReturnsTrue(Callable c) {
53+
c.getReturnType() instanceof BoolType and
54+
not callableMayThrowException(c) and
55+
forex(ReturnStmt rs | rs.getEnclosingCallable() = c |
56+
rs.getNumberOfChildren() = 1 and
57+
isExpressionAlwaysTrue(rs.getChildExpr(0))
58+
)
59+
}
60+
61+
/**
62+
* Holds if `c` always returns `true`.
63+
*/
64+
private predicate alwaysReturnsTrue(Callable c) {
65+
callableHasAReturnStmtAndAlwaysReturnsTrue(c)
66+
or
67+
lambdaExprReturnsOnlyLiteralTrue(c)
68+
}
69+
70+
/**
71+
* Holds if SetIsOriginAllowed always returns true. This sets the Access-Control-Allow-Origin to the requester
72+
*/
73+
predicate setIsOriginAllowedReturnsTrue(MethodCall mc) {
74+
mc.getTarget()
75+
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder",
76+
"SetIsOriginAllowed") and
77+
alwaysReturnsTrue(mc.getArgument(0))
78+
}
79+
80+
/**
81+
* Holds if UseCors is called with the relevant cors policy
82+
*/
83+
predicate usedPolicy(MethodCall add_policy) {
84+
exists(MethodCall uc |
85+
uc.getTarget()
86+
.hasFullyQualifiedName("Microsoft.AspNetCore.Builder.CorsMiddlewareExtensions", "UseCors") and
87+
(
88+
// Same hardcoded name
89+
uc.getArgument(1).getValue() = add_policy.getArgument(0).getValue() or
90+
// Same variable access
91+
uc.getArgument(1).(VariableAccess).getTarget() =
92+
add_policy.getArgument(0).(VariableAccess).getTarget() or
93+
DataFlow::localExprFlow(add_policy.getArgument(0), uc.getArgument(1))
94+
)
95+
) and
96+
add_policy
97+
.getTarget()
98+
.hasFullyQualifiedName("Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions", "AddPolicy")
99+
}

‎csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.cs‎

Lines changed: 15 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,32 +3,23 @@
33
using System;
44
using Microsoft.Extensions.DependencyInjection;
55

6+
public class Test {
7+
public void ConfigureServices(string[] args) {
8+
var builder = WebApplication.CreateBuilder(args);
9+
var MyAllowSpecificOrigins = "_myAllowSpecificOrigins";
610

11+
builder.Services.AddCors(options => {
12+
options.AddPolicy(MyAllowSpecificOrigins,
13+
policy => {
14+
policy.AllowAnyOrigin().AllowCredentials().AllowAnyHeader().AllowAnyMethod();
15+
});
16+
});
717

8-
public class Test
9-
{
10-
    public void ConfigureServices(string[] args)
11-
    {
12-
var builder = WebApplication.CreateBuilder(args);
13-
var MyAllowSpecificOrigins = "_myAllowSpecificOrigins";
18+
var app = builder.Build();
1419

20+
app.MapGet("/", () => "Hello World!");
21+
app.UseCors(MyAllowSpecificOrigins);
1522

16-
builder.Services.AddCors(options =>
17-
{
18-
    options.AddPolicy(MyAllowSpecificOrigins,
19-
                      policy =>
20-
                      {
21-
                          policy.AllowAnyOrigin().AllowCredentials().AllowAnyHeader().AllowAnyMethod();
22-
                      });
23-
});
24-
25-
var app = builder.Build();
26-
27-
28-
29-
app.MapGet("/", () => "Hello World!");
30-
app.UseCors(MyAllowSpecificOrigins);
31-
32-
app.Run();
33-
    }
23+
app.Run();
24+
}
3425
}

‎csharp/ql/test/experimental/CWE-942/CorsMisconfiguration.expected‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1 @@
1-
| CorsMiconfigurationCredentials.cs:18:5:22:24 | call to method AddPolicy | The following CORS policy may allow credentialed requests from 3rd party websites |
2-
| CorsMisconfiguration.cs:18:5:22:24 | call to method AddPolicy | The following CORS policy may allow credentialed requests from 3rd party websites |
1+
| CorsMisconfiguration.cs:12:7:15:10 | call to method AddPolicy | The following CORS policy may allow requests from 3rd party websites |

0 commit comments

Comments
 (0)