Skip to content

Commit 1caca21

Browse files
michaelnebelmichaelnebel
authored
Merge pull request github#12829 from michaelnebel/csharp/refactordataflow4
C#: Re-factor tainttracking and dataflow configurations to use the new API.
2 parents 62f5a5d + e8e25b8 commit 1caca21

File tree

20 files changed

+235
-128
lines changed

20 files changed

+235
-128
lines changed

‎csharp/ql/lib/semmle/code/csharp/frameworks/Sql.qll

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,23 +52,24 @@ class IDbCommandConstructionSqlExpr extends SqlExpr, ObjectCreation {
5252
class DapperCommandDefinitionMethodCallSqlExpr extends SqlExpr, ObjectCreation {
5353
DapperCommandDefinitionMethodCallSqlExpr() {
5454
this.getObjectType() instanceof Dapper::CommandDefinitionStruct and
55-
exists(Conf c | c.hasFlow(DataFlow::exprNode(this), _))
55+
DapperCommandDefinitionMethodCallSql::flow(DataFlow::exprNode(this), _)
5656
}
5757

5858
override Expr getSql() { result = this.getArgumentForName("commandText") }
5959
}
6060

61-
private class Conf extends DataFlow4::Configuration {
62-
Conf() { this = "DapperCommandDefinitionFlowConfig" }
63-
64-
override predicate isSource(DataFlow::Node node) {
61+
private module DapperCommandDefitionMethodCallSqlConfig implements DataFlow::ConfigSig {
62+
predicate isSource(DataFlow::Node node) {
6563
node.asExpr().(ObjectCreation).getObjectType() instanceof Dapper::CommandDefinitionStruct
6664
}
6765

68-
override predicate isSink(DataFlow::Node node) {
66+
predicate isSink(DataFlow::Node node) {
6967
exists(MethodCall mc |
7068
mc.getTarget() = any(Dapper::SqlMapperClass c).getAQueryMethod() and
7169
node.asExpr() = mc.getArgumentForName("command")
7270
)
7371
}
7472
}
73+
74+
private module DapperCommandDefinitionMethodCallSql =
75+
DataFlow::Global<DapperCommandDefitionMethodCallSqlConfig>;

‎csharp/ql/lib/semmle/code/csharp/frameworks/system/Xml.qll

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -162,18 +162,14 @@ class XmlReaderSettingsCreation extends ObjectCreation {
162162
}
163163
}
164164

165-
private class SettingsDataFlowConfig extends DataFlow3::Configuration {
166-
SettingsDataFlowConfig() { this = "SettingsDataFlowConfig" }
165+
private module SettingsDataFlowConfig implements DataFlow::ConfigSig {
166+
predicate isSource(DataFlow::Node source) { source.asExpr() instanceof XmlReaderSettingsCreation }
167167

168-
override predicate isSource(DataFlow::Node source) {
169-
source.asExpr() instanceof XmlReaderSettingsCreation
170-
}
171-
172-
override predicate isSink(DataFlow::Node sink) {
173-
sink.asExpr() instanceof XmlReaderSettingsInstance
174-
}
168+
predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof XmlReaderSettingsInstance }
175169
}
176170

171+
private module SettingsDataFlow = DataFlow::Global<SettingsDataFlowConfig>;
172+
177173
/** A call to `XmlReader.Create`. */
178174
class XmlReaderCreateCall extends MethodCall {
179175
XmlReaderCreateCall() { this.getTarget() = any(SystemXmlXmlReaderClass r).getCreateMethod() }
@@ -190,8 +186,6 @@ class XmlReaderSettingsInstance extends Expr {
190186

191187
/** Gets a possible creation point for this instance of `XmlReaderSettings`. */
192188
XmlReaderSettingsCreation getASettingsCreation() {
193-
exists(SettingsDataFlowConfig settingsFlow |
194-
settingsFlow.hasFlow(DataFlow::exprNode(result), DataFlow::exprNode(this))
195-
)
189+
SettingsDataFlow::flow(DataFlow::exprNode(result), DataFlow::exprNode(this))
196190
}
197191
}

‎csharp/ql/lib/semmle/code/csharp/security/dataflow/ReDoSQuery.qll

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,26 +78,40 @@ predicate isExponentialRegex(StringLiteral s) {
7878
}
7979

8080
/**
81+
* DEPRECATED: Use `ExponentialRegexDataflow` instead.
82+
*
8183
* A data flow configuration for tracking exponential worst case time regular expression string
8284
* literals to the pattern argument of a regex.
8385
*/
84-
class ExponentialRegexDataflow extends DataFlow2::Configuration {
86+
deprecated class ExponentialRegexDataflow extends DataFlow2::Configuration {
8587
ExponentialRegexDataflow() { this = "ExponentialRegex" }
8688

8789
override predicate isSource(DataFlow::Node s) { isExponentialRegex(s.asExpr()) }
8890

8991
override predicate isSink(DataFlow::Node s) { s.asExpr() = any(RegexOperation c).getPattern() }
9092
}
9193

94+
/**
95+
* A data flow configuration for tracking exponential worst case time regular expression string
96+
* literals to the pattern argument of a regex.
97+
*/
98+
private module ExponentialRegexDataFlowConfig implements DataFlow::ConfigSig {
99+
predicate isSource(DataFlow::Node s) { isExponentialRegex(s.asExpr()) }
100+
101+
predicate isSink(DataFlow::Node s) { s.asExpr() = any(RegexOperation c).getPattern() }
102+
}
103+
104+
module ExponentialRegexDataFlow = DataFlow::Global<ExponentialRegexDataFlowConfig>;
105+
92106
/**
93107
* An expression passed as the `input` to a call to a `Regex` method, where the regex appears to
94108
* have exponential behavior.
95109
*/
96110
class ExponentialRegexSink extends DataFlow::ExprNode, Sink {
97111
ExponentialRegexSink() {
98-
exists(ExponentialRegexDataflow regexDataflow, RegexOperation regexOperation |
112+
exists(RegexOperation regexOperation |
99113
// Exponential regex flows to the pattern argument
100-
regexDataflow.hasFlow(_, DataFlow::exprNode(regexOperation.getPattern()))
114+
ExponentialRegexDataFlow::flow(_, DataFlow::exprNode(regexOperation.getPattern()))
101115
|
102116
// This is used as an input for this pattern
103117
this.getExpr() = regexOperation.getInput() and

‎csharp/ql/lib/semmle/code/csharp/security/dataflow/UnsafeDeserializationQuery.qll

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,11 @@ class JsonConvertTrackingConfig extends TaintTracking::Configuration {
7575
}
7676

7777
/**
78+
* DEPRECATED: Use `TypeNameTracking` instead.
79+
*
7880
* Tracks unsafe `TypeNameHandling` setting to `JsonConvert` call
7981
*/
80-
class TypeNameTrackingConfig extends DataFlow::Configuration {
82+
deprecated class TypeNameTrackingConfig extends DataFlow::Configuration {
8183
TypeNameTrackingConfig() { this = "TypeNameTrackingConfig" }
8284

8385
override predicate isSource(DataFlow::Node source) {
@@ -127,6 +129,62 @@ class TypeNameTrackingConfig extends DataFlow::Configuration {
127129
}
128130
}
129131

132+
/**
133+
* Configuration module for tracking unsafe `TypeNameHandling` setting to `JsonConvert` calls.
134+
*/
135+
private module TypeNameTrackingConfig implements DataFlow::ConfigSig {
136+
predicate isSource(DataFlow::Node source) {
137+
(
138+
source.asExpr() instanceof MemberConstantAccess and
139+
source.getType() instanceof TypeNameHandlingEnum
140+
or
141+
source.asExpr() instanceof IntegerLiteral
142+
) and
143+
source.asExpr().hasValue() and
144+
not source.asExpr().getValue() = "0"
145+
}
146+
147+
predicate isSink(DataFlow::Node sink) {
148+
exists(MethodCall mc, Method m, Expr expr |
149+
m = mc.getTarget() and
150+
(
151+
not mc.getArgument(0).hasValue() and
152+
m instanceof NewtonsoftJsonConvertClassDeserializeObjectMethod
153+
) and
154+
expr = mc.getAnArgument() and
155+
sink.asExpr() = expr and
156+
expr.getType() instanceof JsonSerializerSettingsClass
157+
)
158+
}
159+
160+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
161+
node1.asExpr() instanceof IntegerLiteral and
162+
node2.asExpr().(CastExpr).getExpr() = node1.asExpr()
163+
or
164+
node1.getType() instanceof TypeNameHandlingEnum and
165+
exists(PropertyWrite pw, Property p, Assignment a |
166+
a.getLValue() = pw and
167+
pw.getProperty() = p and
168+
p.getDeclaringType() instanceof JsonSerializerSettingsClass and
169+
p.hasName("TypeNameHandling") and
170+
(
171+
node1.asExpr() = a.getRValue() and
172+
node2.asExpr() = pw.getQualifier()
173+
or
174+
exists(ObjectInitializer oi |
175+
node1.asExpr() = oi.getAMemberInitializer().getRValue() and
176+
node2.asExpr() = oi
177+
)
178+
)
179+
)
180+
}
181+
}
182+
183+
/**
184+
* Configuration module for tracking unsafe `TypeNameHandling` setting to `JsonConvert` calls.
185+
*/
186+
module TypeNameTracking = DataFlow::Global<TypeNameTrackingConfig>;
187+
130188
/**
131189
* User input to static method or constructor call deserialization flow tracking.
132190
*/

‎csharp/ql/lib/semmle/code/csharp/security/xml/InsecureXMLQuery.qll

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -172,26 +172,24 @@ module XmlReader {
172172
isNetFrameworkBefore(this.(MethodCall).getTarget().getDeclaringType(), "4.0")
173173
or
174174
// bad settings flow here
175-
exists(SettingsDataFlowConfig flow, ObjectCreation settings |
176-
flow.hasFlow(DataFlow::exprNode(settings), DataFlow::exprNode(this.getSettings())) and
175+
exists(ObjectCreation settings |
176+
SettingsDataFlow::flow(DataFlow::exprNode(settings), DataFlow::exprNode(this.getSettings())) and
177177
XmlSettings::dtdEnabledSettings(settings, evidence, reason)
178178
)
179179
}
180180

181181
private predicate insecureResolver(string reason, Expr evidence) {
182182
// bad settings flow here
183-
exists(SettingsDataFlowConfig flow, ObjectCreation settings |
184-
flow.hasFlow(DataFlow::exprNode(settings), DataFlow::exprNode(this.getSettings())) and
183+
exists(ObjectCreation settings |
184+
SettingsDataFlow::flow(DataFlow::exprNode(settings), DataFlow::exprNode(this.getSettings())) and
185185
XmlSettings::insecureResolverSettings(settings, evidence, reason)
186186
)
187187
// default is secure
188188
}
189189
}
190190

191-
private class SettingsDataFlowConfig extends DataFlow2::Configuration {
192-
SettingsDataFlowConfig() { this = "SettingsDataFlowConfig" }
193-
194-
override predicate isSource(DataFlow::Node source) {
191+
private module SettingsDataFlowConfig implements DataFlow::ConfigSig {
192+
predicate isSource(DataFlow::Node source) {
195193
// flow from places where we construct an XmlReaderSettings
196194
source
197195
.asExpr()
@@ -202,10 +200,12 @@ module XmlReader {
202200
.hasQualifiedName("System.Xml", "XmlReaderSettings")
203201
}
204202

205-
override predicate isSink(DataFlow::Node sink) {
203+
predicate isSink(DataFlow::Node sink) {
206204
sink.asExpr() = any(InsecureXmlReaderCreate create).getSettings()
207205
}
208206
}
207+
208+
private module SettingsDataFlow = DataFlow::Global<SettingsDataFlowConfig>;
209209
}
210210

211211
/** Provides predicates related to `System.Xml.XmlTextReader`. */

‎csharp/ql/src/Language Abuse/ForeachCapture.ql

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,19 +37,19 @@ predicate inForeachStmtBody(ForeachStmt loop, Element e) {
3737
)
3838
}
3939

40-
class LambdaDataFlowConfiguration extends DataFlow::Configuration {
41-
LambdaDataFlowConfiguration() { this = "LambdaDataFlowConfiguration" }
40+
module LambdaDataFlowConfig implements DataFlow::ConfigSig {
41+
predicate isSource(DataFlow::Node source) { lambdaCapturesLoopVariable(source.asExpr(), _, _) }
4242

43-
override predicate isSource(DataFlow::Node source) {
44-
lambdaCapturesLoopVariable(source.asExpr(), _, _)
45-
}
43+
predicate isSink(DataFlow::Node sink) { exists(getAssignmentTarget(sink.asExpr())) }
44+
}
4645

47-
override predicate isSink(DataFlow::Node sink) { exists(getAssignmentTarget(sink.asExpr())) }
46+
module LambdaDataFlow {
47+
private import DataFlow::Global<LambdaDataFlowConfig>
4848

4949
predicate capturesLoopVarAndIsStoredIn(
5050
AnonymousFunctionExpr lambda, Variable loopVar, Element storage
5151
) {
52-
exists(DataFlow::Node sink | this.hasFlow(DataFlow::exprNode(lambda), sink) |
52+
exists(DataFlow::Node sink | flow(DataFlow::exprNode(lambda), sink) |
5353
storage = getAssignmentTarget(sink.asExpr())
5454
) and
5555
exists(ForeachStmt loop | lambdaCapturesLoopVariable(lambda, loop, loopVar) |
@@ -109,7 +109,7 @@ predicate declaredInsideLoop(ForeachStmt loop, LocalVariable v) {
109109
)
110110
}
111111

112-
from LambdaDataFlowConfiguration c, AnonymousFunctionExpr lambda, Variable loopVar, Element storage
113-
where c.capturesLoopVarAndIsStoredIn(lambda, loopVar, storage)
112+
from AnonymousFunctionExpr lambda, Variable loopVar, Element storage
113+
where LambdaDataFlow::capturesLoopVarAndIsStoredIn(lambda, loopVar, storage)
114114
select lambda, "Function which may be stored in $@ captures variable $@.", storage,
115115
storage.toString(), loopVar, loopVar.getName()

‎csharp/ql/src/Security Features/CWE-327/DontInstallRootCert.ql

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,10 @@
1212

1313
import csharp
1414
import semmle.code.csharp.dataflow.DataFlow::DataFlow
15-
import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph
15+
import AddCertToRootStore::PathGraph
1616

17-
class AddCertToRootStoreConfig extends DataFlow::Configuration {
18-
AddCertToRootStoreConfig() { this = "Adding Certificate To Root Store" }
19-
20-
override predicate isSource(DataFlow::Node source) {
17+
module AddCertToRootStoreConfig implements DataFlow::ConfigSig {
18+
predicate isSource(DataFlow::Node source) {
2119
exists(ObjectCreation oc | oc = source.asExpr() |
2220
oc.getType()
2321
.(RefType)
@@ -26,7 +24,7 @@ class AddCertToRootStoreConfig extends DataFlow::Configuration {
2624
)
2725
}
2826

29-
override predicate isSink(DataFlow::Node sink) {
27+
predicate isSink(DataFlow::Node sink) {
3028
exists(MethodCall mc |
3129
(
3230
mc.getTarget()
@@ -40,6 +38,8 @@ class AddCertToRootStoreConfig extends DataFlow::Configuration {
4038
}
4139
}
4240

43-
from DataFlow::PathNode oc, DataFlow::PathNode mc, AddCertToRootStoreConfig config
44-
where config.hasFlowPath(oc, mc)
41+
module AddCertToRootStore = DataFlow::Global<AddCertToRootStoreConfig>;
42+
43+
from AddCertToRootStore::PathNode oc, AddCertToRootStore::PathNode mc
44+
where AddCertToRootStore::flowPath(oc, mc)
4545
select mc.getNode(), oc, mc, "This certificate is added to the root certificate store."

‎csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,23 +11,21 @@
1111
*/
1212

1313
import csharp
14-
import DataFlow::PathGraph
14+
import InsecureSqlConnection::PathGraph
1515

1616
/**
1717
* A data flow configuration for tracking strings passed to `SqlConnection[StringBuilder]` instances.
1818
*/
19-
class TaintTrackingConfiguration extends DataFlow::Configuration {
20-
TaintTrackingConfiguration() { this = "TaintTrackingConfiguration" }
21-
22-
override predicate isSource(DataFlow::Node source) {
19+
module InsecureSqlConnectionConfig implements DataFlow::ConfigSig {
20+
predicate isSource(DataFlow::Node source) {
2321
exists(string s | s = source.asExpr().(StringLiteral).getValue().toLowerCase() |
2422
s.matches("%encrypt=false%")
2523
or
2624
not s.matches("%encrypt=%")
2725
)
2826
}
2927

30-
override predicate isSink(DataFlow::Node sink) {
28+
predicate isSink(DataFlow::Node sink) {
3129
exists(ObjectCreation oc |
3230
oc.getRuntimeArgument(0) = sink.asExpr() and
3331
(
@@ -39,8 +37,13 @@ class TaintTrackingConfiguration extends DataFlow::Configuration {
3937
}
4038
}
4139

42-
from TaintTrackingConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink
43-
where c.hasFlowPath(source, sink)
40+
/**
41+
* A data flow configuration for tracking strings passed to `SqlConnection[StringBuilder]` instances.
42+
*/
43+
module InsecureSqlConnection = DataFlow::Global<InsecureSqlConnectionConfig>;
44+
45+
from InsecureSqlConnection::PathNode source, InsecureSqlConnection::PathNode sink
46+
where InsecureSqlConnection::flowPath(source, sink)
4447
select sink.getNode(), source, sink,
4548
"$@ flows to this SQL connection and does not specify `Encrypt=True`.", source.getNode(),
4649
"Connection string"

‎csharp/ql/src/Security Features/CWE-502/UnsafeDeserializationUntrustedInput.ql

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,9 @@ where
4848
)
4949
or
5050
// JsonConvert static method call, but with additional unsafe typename tracking
51-
exists(
52-
JsonConvertTrackingConfig taintTrackingJsonConvert, TypeNameTrackingConfig typenameTracking,
53-
DataFlow::Node settingsCallArg
54-
|
51+
exists(JsonConvertTrackingConfig taintTrackingJsonConvert, DataFlow::Node settingsCallArg |
5552
taintTrackingJsonConvert.hasFlowPath(userInput, deserializeCallArg) and
56-
typenameTracking.hasFlow(_, settingsCallArg) and
53+
TypeNameTracking::flow(_, settingsCallArg) and
5754
deserializeCallArg.getNode().asExpr().getParent() = settingsCallArg.asExpr().getParent()
5855
)
5956
select deserializeCallArg, userInput, deserializeCallArg, "$@ flows to unsafe deserializer.",

‎csharp/ql/src/experimental/Security Features/CWE-1004/CookieWithoutHttpOnly.ql

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,8 @@ where
3838
// there is no callback `OnAppendCookie` that sets `HttpOnly` to true
3939
not exists(OnAppendCookieHttpOnlyTrackingConfig config | config.hasFlowTo(_)) and
4040
// Passed as third argument to `IResponseCookies.Append`
41-
exists(
42-
CookieOptionsTrackingConfiguration cookieTracking, DataFlow::Node creation,
43-
DataFlow::Node append
44-
|
45-
cookieTracking.hasFlow(creation, append) and
41+
exists(DataFlow::Node creation, DataFlow::Node append |
42+
CookieOptionsTracking::flow(creation, append) and
4643
creation.asExpr() = oc and
4744
append.asExpr() = mc.getArgument(2)
4845
)
@@ -79,8 +76,8 @@ where
7976
oc = c and
8077
oc.getType() instanceof MicrosoftAspNetCoreHttpCookieOptions and
8178
not isPropertySet(oc, "HttpOnly") and
82-
exists(CookieOptionsTrackingConfiguration cookieTracking, DataFlow::Node creation |
83-
cookieTracking.hasFlow(creation, _) and
79+
exists(DataFlow::Node creation |
80+
CookieOptionsTracking::flow(creation, _) and
8481
creation.asExpr() = oc
8582
)
8683
)

0 commit comments

Comments
 (0)