Skip to content

Commit 702deb4

Browse files
ropwareJBropwareJB
authored
Merge pull request #352 from microsoft/typenamehandling-queries
Typenamehandling Queries
2 parents 54da53b + 1788a53 commit 702deb4

29 files changed

Lines changed: 1118 additions & 0 deletions

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

Lines changed: 319 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,319 @@
1+
/**
2+
* Provides classes and predicates for detecting unsafe usage of
3+
* Newtonsoft.Json `TypeNameHandling` settings.
4+
*
5+
* Setting `TypeNameHandling` to any value other than `None` during
6+
* deserialization can enable remote code execution if untrusted data
7+
* is deserialized without a custom `SerializationBinder`.
8+
*/
9+
10+
import csharp
11+
import semmle.code.csharp.security.dataflow.flowsources.Remote
12+
import semmle.code.csharp.serialization.Deserializers
13+
14+
// ---------------------------------------------------------------------------
15+
// Source expressions: unsafe TypeNameHandling values
16+
// ---------------------------------------------------------------------------
17+
/**
18+
* An expression that represents an unsafe `TypeNameHandling` value —
19+
* any member of the `TypeNameHandling` enum other than `None`, or an
20+
* integer literal greater than zero (which maps to a non-`None` value).
21+
*/
22+
class BadTypeHandling extends Expr {
23+
BadTypeHandling() {
24+
exists(Enum e, EnumConstant c |
25+
e.hasFullyQualifiedName("Newtonsoft.Json", "TypeNameHandling") and
26+
c = e.getAnEnumConstant() and
27+
this = c.getAnAccess() and
28+
not c.hasName("None")
29+
)
30+
or
31+
this.(IntegerLiteral).getValue().toInt() > 0
32+
}
33+
}
34+
35+
// ---------------------------------------------------------------------------
36+
// TypeNameHandling property modelling
37+
// ---------------------------------------------------------------------------
38+
/**
39+
* The `TypeNameHandling` property on `JsonSerializerSettings` or `JsonSerializer`.
40+
*/
41+
class TypeNameHandlingProperty extends Property {
42+
TypeNameHandlingProperty() {
43+
this.hasFullyQualifiedName("Newtonsoft.Json",
44+
["JsonSerializerSettings", "JsonSerializer"], "TypeNameHandling")
45+
}
46+
}
47+
48+
/** A write to a `TypeNameHandling` property. */
49+
class TypeNameHandlingPropertyWrite extends PropertyWrite {
50+
TypeNameHandlingPropertyWrite() { this.getProperty() instanceof TypeNameHandlingProperty }
51+
52+
/** Gets the right-hand side value assigned to this property. */
53+
Expr getAssignedValue() {
54+
exists(AssignExpr a |
55+
a.getLValue() = this and
56+
result = a.getRValue()
57+
)
58+
}
59+
}
60+
61+
/**
62+
* A write to a `TypeNameHandling` property where the assigned value is
63+
* known to be unsafe (i.e. not `None`).
64+
*/
65+
class BadTypeHandlingPropertyWrite extends TypeNameHandlingPropertyWrite {
66+
BadTypeHandlingPropertyWrite() {
67+
exists(BadTypeHandling b |
68+
DataFlow::localExprFlow(b, this.getAssignedValue())
69+
)
70+
}
71+
}
72+
73+
// ---------------------------------------------------------------------------
74+
// Binder property modelling
75+
// ---------------------------------------------------------------------------
76+
/**
77+
* A write to the `SerializationBinder` or `Binder` property on
78+
* `JsonSerializerSettings` or `JsonSerializer`. Setting a custom binder
79+
* is a mitigation against unsafe `TypeNameHandling`.
80+
*/
81+
class BinderPropertyWrite extends PropertyWrite {
82+
BinderPropertyWrite() {
83+
this.getProperty()
84+
.hasFullyQualifiedName("Newtonsoft.Json",
85+
["JsonSerializerSettings", "JsonSerializer"],
86+
["SerializationBinder", "Binder"])
87+
}
88+
}
89+
90+
// ---------------------------------------------------------------------------
91+
// Deserialize call argument modelling
92+
// ---------------------------------------------------------------------------
93+
/**
94+
* An argument passed to a `Newtonsoft.Json.JsonConvert.DeserializeObject` call.
95+
*/
96+
class DeserializeArg extends Expr {
97+
MethodCall deserializeCall;
98+
99+
DeserializeArg() {
100+
deserializeCall.getTarget() instanceof NewtonsoftJsonConvertClassDeserializeObjectMethod and
101+
deserializeCall.getAnArgument() = this
102+
}
103+
104+
/** Gets the enclosing `DeserializeObject` method call. */
105+
MethodCall getDeserializeCall() { result = deserializeCall }
106+
}
107+
108+
/**
109+
* A `JsonSerializerSettings`-typed argument passed to a `DeserializeObject`
110+
* call. This is the primary sink for unsafe `TypeNameHandling` flow.
111+
*/
112+
class JsonSerializerSettingsArg extends DeserializeArg {
113+
JsonSerializerSettingsArg() { this.getType() instanceof JsonSerializerSettingsClass }
114+
}
115+
116+
// ---------------------------------------------------------------------------
117+
// Taint tracking: remote input → DeserializeObject
118+
// ---------------------------------------------------------------------------
119+
/**
120+
* Tracks tainted data flowing from remote sources into arguments of
121+
* `JsonConvert.DeserializeObject`, including flow through `ToString()` calls.
122+
*/
123+
module UserInputToDeserializeObjectCallConfig implements DataFlow::ConfigSig {
124+
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
125+
126+
predicate isSink(DataFlow::Node sink) {
127+
exists(MethodCall mc |
128+
mc.getTarget().hasFullyQualifiedName("Newtonsoft.Json.JsonConvert", _) and
129+
mc.getTarget().hasUndecoratedName("DeserializeObject") and
130+
sink.asExpr() = mc.getAnArgument()
131+
)
132+
}
133+
134+
predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
135+
exists(MethodCall ma |
136+
ma.getTarget().hasName("ToString") and
137+
ma.getQualifier() = pred.asExpr() and
138+
succ.asExpr() = ma
139+
)
140+
}
141+
}
142+
143+
module UserInputToDeserializeObjectCallFlow =
144+
TaintTracking::Global<UserInputToDeserializeObjectCallConfig>;
145+
146+
// ---------------------------------------------------------------------------
147+
// Data flow: binder set on settings object
148+
// ---------------------------------------------------------------------------
149+
/**
150+
* Tracks whether a `SerializationBinder` is assigned via an object
151+
* initializer and the resulting settings object flows to a
152+
* `DeserializeObject` call argument.
153+
*/
154+
module BinderConfig implements DataFlow::ConfigSig {
155+
predicate isSource(DataFlow::Node source) {
156+
exists(ObjectCreation oc, MemberInitializer mi |
157+
oc.getInitializer().(ObjectInitializer).getAMemberInitializer() = mi and
158+
mi.getInitializedMember().hasName(["Binder", "SerializationBinder"]) and
159+
source.asExpr() = oc
160+
)
161+
}
162+
163+
predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof JsonSerializerSettingsArg }
164+
}
165+
166+
module BinderFlow = DataFlow::Global<BinderConfig>;
167+
168+
// ---------------------------------------------------------------------------
169+
// Binder-set check (mitigation detection)
170+
// ---------------------------------------------------------------------------
171+
/**
172+
* Holds if a custom `SerializationBinder` or `Binder` has been set on the
173+
* settings object referenced by `arg`, either through an object initializer
174+
* (tracked via `BinderFlow`) or through a later property write on the same
175+
* variable.
176+
*/
177+
predicate hasBinderSet(JsonSerializerSettingsArg arg) {
178+
// Binder was set in an object initializer and flowed to `arg`
179+
exists(BinderFlow::PathNode sink |
180+
sink.isSink() and
181+
sink.getNode().asExpr() = arg
182+
)
183+
or
184+
// Binder was set via a property write on the same variable
185+
exists(PropertyWrite pw |
186+
pw.getProperty().hasName(["Binder", "SerializationBinder"]) and
187+
pw.getQualifier().(Access).getTarget() = arg.(Access).getTarget()
188+
)
189+
}
190+
191+
// ---------------------------------------------------------------------------
192+
// Sink node: TypeNameHandling property write value
193+
// ---------------------------------------------------------------------------
194+
/**
195+
* A data-flow node representing the value assigned to a `TypeNameHandling`
196+
* property. Provides a predicate to check whether a mitigating binder is set.
197+
*/
198+
class TypeNameHandlingPropertySink extends DataFlow::Node {
199+
TypeNameHandlingPropertySink() {
200+
exists(TypeNameHandlingPropertyWrite pw |
201+
this.asExpr() = pw.getAssignedValue()
202+
)
203+
}
204+
205+
/** Holds if a custom binder is set on the same settings object. */
206+
predicate hasBinderSet() {
207+
exists(JsonSerializerSettingsArg arg |
208+
this.asExpr() = arg and
209+
hasBinderSet(arg)
210+
)
211+
}
212+
}
213+
214+
// ---------------------------------------------------------------------------
215+
// Main flow: unsafe TypeNameHandling value → DeserializeObject settings arg
216+
// ---------------------------------------------------------------------------
217+
/**
218+
* Tracks unsafe `TypeNameHandling` values flowing into `JsonSerializerSettings`
219+
* arguments of `DeserializeObject` calls. Includes additional flow steps for
220+
* integer-to-enum casts and property writes on settings objects.
221+
*/
222+
module UnsafeTypeNameHandlingFlowConfig implements DataFlow::ConfigSig {
223+
predicate isSource(DataFlow::Node source) { source.asExpr() instanceof BadTypeHandling }
224+
225+
predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof JsonSerializerSettingsArg }
226+
227+
predicate isBarrierIn(DataFlow::Node node) { isSource(node) }
228+
229+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
230+
// Cast from integer literal to TypeNameHandling enum
231+
node1.asExpr() instanceof IntegerLiteral and
232+
node2.asExpr().(CastExpr).getExpr() = node1.asExpr()
233+
or
234+
node1.getType() instanceof TypeNameHandlingEnum and
235+
exists(TypeNameHandlingPropertyWrite pw, Assignment a |
236+
a.getLValue() = pw and
237+
(
238+
// Explicit property write: flow from the assigned value to the
239+
// JsonSerializerSettingsArg that accesses the same settings variable
240+
node1.asExpr() = a.getRValue() and
241+
node2.asExpr().(JsonSerializerSettingsArg).(VariableAccess).getTarget() =
242+
pw.getQualifier().(VariableAccess).getTarget()
243+
or
244+
// Object initializer: flow from the member initializer value to the
245+
// ObjectCreation, which then flows locally to the JsonSerializerSettingsArg
246+
exists(ObjectInitializer oi, ObjectCreation oc |
247+
node1.asExpr() = oi.getAMemberInitializer().getRValue() and
248+
oc.getInitializer() = oi and
249+
DataFlow::localExprFlow(oc, node2.asExpr().(JsonSerializerSettingsArg))
250+
)
251+
)
252+
)
253+
}
254+
}
255+
256+
module UnsafeTypeNameHandlingFlow = DataFlow::Global<UnsafeTypeNameHandlingFlowConfig>;
257+
258+
// ---------------------------------------------------------------------------
259+
// Settings / serializer object creation modelling
260+
// ---------------------------------------------------------------------------
261+
/**
262+
* An `ObjectCreation` of type `Newtonsoft.Json.JsonSerializerSettings` or
263+
* `Newtonsoft.Json.JsonSerializer`.
264+
*/
265+
class JsonSerializerSettingsCreation extends ObjectCreation {
266+
JsonSerializerSettingsCreation() {
267+
this.getTarget()
268+
.hasFullyQualifiedName("Newtonsoft.Json.JsonSerializerSettings",
269+
"JsonSerializerSettings")
270+
or
271+
this.getTarget()
272+
.hasFullyQualifiedName("Newtonsoft.Json.JsonSerializer", "JsonSerializer")
273+
}
274+
275+
/** Gets the type of the binder assigned to this settings object. */
276+
Class getAssignedBinderType() {
277+
exists(AssignExpr ae |
278+
ae.getLValue() = this.getBinderPropertyWrite() and
279+
ae.getRValue().getType() = result
280+
)
281+
}
282+
283+
/** Gets a `BinderPropertyWrite` associated with this settings object. */
284+
BinderPropertyWrite getBinderPropertyWrite() { result = this.getPropertyWrite() }
285+
286+
/** Gets a `TypeNameHandlingPropertyWrite` associated with this settings object. */
287+
TypeNameHandlingPropertyWrite getTypeNameHandlingPropertyWrite() {
288+
result = this.getPropertyWrite()
289+
}
290+
291+
/**
292+
* Gets a `PropertyWrite` associated with this settings object, via an
293+
* initializer, direct local flow, or an assignment to the same property.
294+
*/
295+
PropertyWrite getPropertyWrite() {
296+
result = this.getInitializer().getAChild*()
297+
or
298+
// Direct local flow via some intermediary
299+
DataFlow::localExprFlow(this, result.getQualifier())
300+
or
301+
// Local flow via property writes
302+
this.hasPropertyWrite(result)
303+
}
304+
305+
/**
306+
* Holds if `pw` is a property write on the same target that this object
307+
* creation is assigned to, within the same callable.
308+
*/
309+
bindingset[pw]
310+
pragma[inline_late]
311+
predicate hasPropertyWrite(PropertyWrite pw) {
312+
exists(Assignment a |
313+
a.getRValue() = this and
314+
a.getLValue().(PropertyAccess).getTarget() =
315+
pw.getQualifier().(PropertyAccess).getTarget() and
316+
a.getEnclosingCallable() = pw.getEnclosingCallable()
317+
)
318+
}
319+
}

‎csharp/ql/src/Security Features/CWE-502/UnsafeTypeNameHandling.qhelp‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>This query checks for flow from a user input source to a Newtonsoft.Json DeserializeObject call that uses an unsafe TypeNameHandling setting and has a SerializationBinder set.</p>
8+
9+
<p> The <a href="https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_TypeNameHandling.htm"> Newtonsoft.Json.TypeNameHandling </a> enumeration value of <code>Auto</code> or <code>All</code> is vulnerable when deserializing untrusted data.
10+
An attacker could modify the serialized data to include unexpected types to inject objects with malicious side effects.
11+
An attack against an insecure deserializer could, for example, execute commands on the underlying operating system, communicate over the network, or delete files.</p>
12+
</overview>
13+
14+
<recommendation>
15+
<p>Use the <a href="https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_TypeNameHandling.htm">TypeNameHandling</a> value of <code>None</code>, or use a custom ISerializationBinder.</p>
16+
</recommendation>
17+
18+
<example>
19+
20+
<p>Violation:</p>
21+
22+
<sample src="examples/UnsafeTypeNameHandlingBad.cs" />
23+
24+
<p>Solution using TypeNameHandling.None:</p>
25+
26+
<sample src="examples/UnsafeTypeNameHandlingGood.cs" />
27+
28+
<p>Solution using custom ISerializationBinder: </p>
29+
30+
<sample src="examples/UnsafeTypeNameHandlingWithBinderGood.cs" />
31+
32+
</example>
33+
34+
<references>
35+
<li>Microsoft: <a href="https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2326">Do Not Use TypeNameHandling Values Other Than None</a>.</li>
36+
</references>
37+
38+
</qhelp>

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/**
2+
* @name Unsafe TypeNameHandling value
3+
* @description Checks for when JsonSerializer that also uses an unsafe TypeNameHandling setting
4+
* @id cs/unsafe-type-name-handling
5+
* @kind problem
6+
* @problem.severity warning
7+
* @precision high
8+
* @tags security
9+
* external/cwe/cwe-502
10+
*/
11+
12+
import csharp
13+
import semmle.code.csharp.security.dataflow.TypeNameHandlingQuery
14+
15+
from
16+
JsonSerializerSettingsCreation oc, TypeNameHandlingPropertyWrite pw, DataFlow::Node source,
17+
DataFlow::Node sink
18+
where
19+
pw = oc.getTypeNameHandlingPropertyWrite() and
20+
sink = DataFlow::exprNode(pw.getAssignedValue()) and
21+
UnsafeTypeNameHandlingFlow::flow(source, sink)
22+
select oc.getAssignedBinderType().getAMethod("BindToType"),
23+
"This is the BindToType() implementation used in $@ with $@. Ensure that it checks the TypeName against an allow or deny list, and returns null or throws an exception when passed an unexpected type", oc, "JsonSerializerSettings",
24+
pw.getAssignedValue(), "an unsafe TypeNameHandling value"

0 commit comments

Comments
 (0)