Skip to content

Commit 5a7b653

Browse files
raulgarciamsftraulgarciamsft
committed
Updated to handle lambda statements (previously false negatives) + a couple of bug fixes.
1 parent 9b79668 commit 5a7b653

File tree

4 files changed

+91
-15
lines changed

4 files changed

+91
-15
lines changed

‎csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll

Lines changed: 62 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ class TokenValidationParametersPropertyWriteToBypassSensitiveValidation extends
1818
p.getAnAccess() = this and
1919
c.getAProperty() = p and
2020
p.getName() in [
21-
"ValidateIssuer", "ValidateAudience", "ValidateLifetime", "RequireExpirationTime", "RequireAudience"
21+
"ValidateIssuer", "ValidateAudience", "ValidateLifetime", "RequireExpirationTime",
22+
"RequireAudience"
2223
]
2324
)
2425
}
@@ -38,9 +39,9 @@ class FalseValueFlowsToTokenValidationParametersPropertyWriteToBypassValidation
3839
}
3940

4041
override predicate isSink(DataFlow::Node sink) {
41-
exists(Assignment a |
42-
sink.asExpr() = a.getRValue()
43-
and a.getLValue() instanceof TokenValidationParametersPropertyWrite
42+
exists(Assignment a |
43+
sink.asExpr() = a.getRValue() and
44+
a.getLValue() instanceof TokenValidationParametersPropertyWrite
4445
)
4546
}
4647
}
@@ -139,18 +140,28 @@ class TokenValidationParametersPropertyWriteToValidationDelegated extends Proper
139140
* Holds if the callable has a return statement and it always returns true for all such statements
140141
*/
141142
predicate callableHasAReturnStmtAndAlwaysReturnsTrue(Callable c) {
142-
c.getReturnType().toString() = "Boolean" and
143+
c.getReturnType() instanceof BoolType and
144+
not callableMayThrowException(c) and
143145
forall(ReturnStmt rs | rs.getEnclosingCallable() = c |
144-
rs.getChildExpr(0).(BoolLiteral).getBoolValue() = true
146+
rs.getNumberOfChildren() = 1 and
147+
isExpressionAlwaysTrue(rs.getChildExpr(0))
145148
) and
146149
exists(ReturnStmt rs | rs.getEnclosingCallable() = c)
147150
}
148151

149152
/**
150153
* Holds if the lambda expression `le` always returns true
151154
*/
152-
predicate lambdaExprReturnsOnlyLiteralTrue(LambdaExpr le) {
155+
predicate lambdaExprReturnsOnlyLiteralTrue(AnonymousFunctionExpr le) {
153156
le.getExpressionBody().(BoolLiteral).getBoolValue() = true
157+
or
158+
// special scenarios where the expression is not a `BoolLiteral`, but it will evaluatue to `true`
159+
exists(Expr e | le.getExpressionBody() = e |
160+
not e instanceof Call and
161+
not e instanceof Literal and
162+
e.getType() instanceof BoolType and
163+
e.getValue() = "true"
164+
)
154165
}
155166

156167
class CallableAlwaysReturnsTrue extends Callable {
@@ -159,9 +170,12 @@ class CallableAlwaysReturnsTrue extends Callable {
159170
or
160171
lambdaExprReturnsOnlyLiteralTrue(this)
161172
or
162-
exists(LambdaExpr le, Call call, CallableAlwaysReturnsTrue cat | this = le |
173+
exists(AnonymousFunctionExpr le, Call call, CallableAlwaysReturnsTrue cat, Callable callable |
174+
this = le
175+
|
176+
callable.getACall() = call and
163177
call = le.getExpressionBody() and
164-
cat.getACall() = call
178+
callableHasAReturnStmtAndAlwaysReturnsTrue(callable)
165179
)
166180
}
167181
}
@@ -188,10 +202,16 @@ class CallableAlwaysReturnsTrueHigherPrecision extends CallableAlwaysReturnsTrue
188202
callable instanceof CallableAlwaysReturnsTrueHigherPrecision
189203
)
190204
or
191-
exists(LambdaExpr le, Call call, CallableAlwaysReturnsTrueHigherPrecision cat | this = le |
205+
exists(AnonymousFunctionExpr le, Call call, CallableAlwaysReturnsTrueHigherPrecision cat |
206+
this = le
207+
|
192208
le.canReturn(call) and
193209
cat.getACall() = call
194210
)
211+
or
212+
exists(LambdaExpr le | le = this |
213+
le.getBody() instanceof CallableAlwaysReturnsTrueHigherPrecision
214+
)
195215
)
196216
}
197217
}
@@ -231,7 +251,7 @@ class CallableAlwaysReturnsParameter0 extends CallableReturnsStringAndArg0IsStri
231251
) and
232252
exists(ReturnStmt rs | rs.getEnclosingCallable() = this)
233253
or
234-
exists(LambdaExpr le, Call call, CallableAlwaysReturnsParameter0 cat | this = le |
254+
exists(AnonymousFunctionExpr le, Call call, CallableAlwaysReturnsParameter0 cat | this = le |
235255
call = le.getExpressionBody() and
236256
cat.getACall() = call
237257
)
@@ -251,7 +271,9 @@ class CallableAlwaysReturnsParameter0MayThrowExceptions extends CallableReturnsS
251271
) and
252272
exists(ReturnStmt rs | rs.getEnclosingCallable() = this)
253273
or
254-
exists(LambdaExpr le, Call call, CallableAlwaysReturnsParameter0MayThrowExceptions cat |
274+
exists(
275+
AnonymousFunctionExpr le, Call call, CallableAlwaysReturnsParameter0MayThrowExceptions cat
276+
|
255277
this = le
256278
|
257279
call = le.getExpressionBody() and
@@ -263,3 +285,31 @@ class CallableAlwaysReturnsParameter0MayThrowExceptions extends CallableReturnsS
263285
this.getBody() = this.getParameter(0).getAnAccess()
264286
}
265287
}
288+
289+
/**
290+
* Hold if the `Expr` e is a `BoolLiteral` with value true,
291+
* the expression has a predictable value == `true`,
292+
* or if it is a `ConditionalExpr` where the `then` and `else` expressions meet `isExpressionAlwaysTrue` criteria
293+
*/
294+
predicate isExpressionAlwaysTrue(Expr e) {
295+
e.(BoolLiteral).getBoolValue() = true
296+
or
297+
e.(Expr).getValue() = "true"
298+
or
299+
e instanceof ConditionalExpr and
300+
isExpressionAlwaysTrue(e.(ConditionalExpr).getThen()) and
301+
isExpressionAlwaysTrue(e.(ConditionalExpr).getElse())
302+
or
303+
exists(Callable callable |
304+
callableHasAReturnStmtAndAlwaysReturnsTrue(callable) and
305+
callable.getACall() = e
306+
)
307+
}
308+
309+
/**
310+
* Holds if the `Callable` c throws any exception other than `ThrowsArgumentNullException`
311+
*/
312+
predicate callableMayThrowException(Callable c) {
313+
exists(ThrowStmt thre | c = thre.getEnclosingCallable()) and
314+
not callableOnlyThrowsArgumentNullException(c)
315+
}

‎csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/delegated-security-validations-always-return-true.ql

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,5 @@ from
1919
TokenValidationParametersPropertyWriteToValidationDelegated tv, Assignment a,
2020
CallableAlwaysReturnsTrueHigherPrecision e
2121
where a.getLValue() = tv and a.getRValue().getAChild*() = e
22-
select tv,
23-
"JsonWebTokenHandler security-sensitive property $@ is being delegated to $@.",
24-
tv, tv.getTarget().toString(), e, "a callable that always returns \"true\""
22+
select tv, "JsonWebTokenHandler security-sensitive property $@ is being delegated to $@.", tv,
23+
tv.getTarget().toString(), e, "a callable that always returns \"true\""

‎csharp/ql/test/experimental/Security Features/JsonWebTokenHandler/delegated-security-validations-always-return-true.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,7 @@
11
| delegation-test.cs:101:13:101:59 | access to property LifetimeValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:101:13:101:59 | access to property LifetimeValidator | LifetimeValidator | delegation-test.cs:101:63:101:186 | (...) => ... | a callable that always returns "true" |
22
| delegation-test.cs:102:13:102:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:102:13:102:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:102:63:102:178 | (...) => ... | a callable that always returns "true" |
3+
| delegation-test.cs:115:13:115:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:115:13:115:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:115:63:115:190 | (...) => ... | a callable that always returns "true" |
4+
| delegation-test.cs:116:13:116:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:116:13:116:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:116:63:116:180 | (...) => ... | a callable that always returns "true" |
5+
| delegation-test.cs:117:13:117:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:117:13:117:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:117:63:117:217 | (...) => ... | a callable that always returns "true" |
6+
| delegation-test.cs:118:13:118:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:118:13:118:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:118:63:118:248 | (...) => ... | a callable that always returns "true" |
7+
| delegation-test.cs:119:13:119:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:119:13:119:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:119:63:119:177 | (...) => ... | a callable that always returns "true" |

‎csharp/ql/test/experimental/Security Features/JsonWebTokenHandler/delegation-test.cs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,29 @@ public void TestCase01()
109109
return true;
110110
};
111111

112+
tokenValidationParamsBaseline.LifetimeValidator = (notBefore, expires, securityToken, validationParameters) => ValidateLifetime02(securityToken, validationParameters); // GOOD
113+
tokenValidationParamsBaseline.AudienceValidator = (IEnumerable<string> audiences, SecurityToken securityToken, TokenValidationParameters validationParameters) => {return securityToken is null?false:true; }; // GOOD
114+
115+
tokenValidationParamsBaseline.AudienceValidator = (IEnumerable<string> audiences, SecurityToken securityToken, TokenValidationParameters validationParameters) => { return true; }; // BUG
116+
tokenValidationParamsBaseline.AudienceValidator = (IEnumerable<string> audiences, SecurityToken securityToken, TokenValidationParameters validationParameters) => !false ; // BUG
117+
tokenValidationParamsBaseline.AudienceValidator = (IEnumerable<string> audiences, SecurityToken securityToken, TokenValidationParameters validationParameters) => { return securityToken is null?true:true; }; // BUG
118+
tokenValidationParamsBaseline.AudienceValidator = (IEnumerable<string> audiences, SecurityToken securityToken, TokenValidationParameters validationParameters) => { return ValidateLifetimeAlwaysTrue(securityToken, validationParameters);}; //BUG
119+
tokenValidationParamsBaseline.AudienceValidator = (audiences, securityToken, validationParameters) => ValidateLifetimeAlwaysTrue(securityToken, validationParameters); //BUG
120+
121+
}
122+
123+
internal static bool ValidateLifetime02(
124+
SecurityToken token,
125+
TokenValidationParameters validationParameters)
126+
{
127+
return token is null?false:true;
112128
}
113129

130+
internal static bool ValidateLifetimeAlwaysTrue02(
131+
SecurityToken token,
132+
TokenValidationParameters validationParameters)
133+
{
134+
return !false;
135+
}
114136
}
115137
}

0 commit comments

Comments
 (0)