deserialization sinks

Author: edvraa

csharp/ql/src/Security Features/CWE-502/DeserializedDelegate.ql

@@ -16,7 +16,7 @@ import semmle.code.csharp.serialization.Deserializers
 
 from Call deserialization, Cast cast
 where
-  deserialization.getTarget() instanceof UnsafeDeserializer and
+  deserialization.getTarget() instanceof UnsafeDeserializerCallable and
   cast.getExpr() = deserialization and
   cast.getTargetType() instanceof SystemLinqExpressions::DelegateExtType
 select deserialization, "Deserialization of delegate type."

csharp/ql/src/Security Features/CWE-502/UnsafeDeserialization.ql

@@ -13,7 +13,17 @@
 import csharp
 import semmle.code.csharp.security.dataflow.UnsafeDeserialization::UnsafeDeserialization
 
-from Call deserializeCall, Sink sink
-where deserializeCall.getAnArgument() = sink.asExpr()
+from Call deserializeCall, ObjectMethodSink sink
+where
+  deserializeCall.getAnArgument() = sink.asExpr() and
+  not exists(
+    DataFlow::PathNode constructor, DataFlow::PathNode usage,
+    SafeConstructorTrackingConfig constructorTracking
+  |
+    constructorTracking.hasFlowPath(constructor, usage) and
+    usage.getNode().asExpr().getParent() = sink.asExpr().getParent()
+  )
+  or
+  exists(ConstructorOrStaticMethodSink sink2 | deserializeCall.getAnArgument() = sink2.asExpr())
 select deserializeCall,
   "Unsafe deserializer is used. Make sure the value being deserialized comes from a trusted source."

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

@@ -27,6 +27,17 @@ it may be necessary to use a different deserialization framework.</p>
 
 <sample src="UnsafeDeserializationUntrustedInputGood.cs" />
 
+<p>In the following example potentially untrusted stream and type is deserialized using a
+<code>DataContractJsonSerializer</code> which is known to be vulnerable with user supplied types.</p>
+
+<sample src="UnsafeDeserializationUntrustedInputTypeBad.cs" />
+
+<p>To fix this specific vulnerability, we are using hardcoded
+Plain Old CLR Object (<a href="https://en.wikipedia.org/wiki/Plain_old_CLR_object">POCO</a>) type. In other cases,
+it may be necessary to use a different deserialization framework.</p>
+
+<sample src="UnsafeDeserializationUntrustedInputTypeGood.cs" />
+
 </example>
 <references>
 

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

@@ -13,8 +13,54 @@
 import csharp
 import semmle.code.csharp.security.dataflow.UnsafeDeserialization::UnsafeDeserialization
 import DataFlow::PathGraph
+import semmle.code.csharp.security.dataflow.flowsources.Remote
+import semmle.code.csharp.security.dataflow.flowsources.Local
 
-from TaintTrackingConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
-where config.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "$@ flows to unsafe deserializer.", source.getNode(),
+class RemoteSource extends Source {
+  RemoteSource() { this instanceof RemoteFlowSource }
+}
+
+class LocalSource extends Source {
+  LocalSource() { this instanceof LocalFlowSource }
+}
+
+from
+  TaintToObjectMethodTrackingConfig taintTracking, DataFlow::PathNode userInput,
+  DataFlow::PathNode deserializeCall
+where
+  // all flows from user input to deserialization with weak and strong type serializers
+  taintTracking.hasFlowPath(userInput, deserializeCall) and
+  // intersect with strong types, but user controlled or weak types deserialization usages
+  (
+    exists(
+      DataFlow::PathNode weakTypeCreation, DataFlow::PathNode weakTypeUsage,
+      WeakTypeCreationToUsageTrackingConfig weakTypeDeserializerTracking
+    |
+      weakTypeDeserializerTracking.hasFlowPath(weakTypeCreation, weakTypeUsage) and
+      weakTypeUsage.getNode().asExpr().getParent() = deserializeCall.getNode().asExpr().getParent()
+    )
+    or
+    exists(
+      TaintToObjectTypeTrackingConfig userControlledTypeTracking,
+      DataFlow::PathNode taintedTypeUsage, DataFlow::PathNode userInput2
+    |
+      userControlledTypeTracking.hasFlowPath(userInput2, taintedTypeUsage) and
+      taintedTypeUsage.getNode().asExpr().getParent() =
+        deserializeCall.getNode().asExpr().getParent()
+    )
+  ) and
+  // exclude deserialization flows with safe instances (i.e. JavaScriptSerializer without resolver)
+  not exists(
+    SafeConstructorTrackingConfig safeConstructorTracking, DataFlow::PathNode safeCreation,
+    DataFlow::PathNode safeTypeUsage
+  |
+    safeConstructorTracking.hasFlowPath(safeCreation, safeTypeUsage) and
+    safeTypeUsage.getNode().asExpr().getParent() = deserializeCall.getNode().asExpr().getParent()
+  )
+  or
+  // no type check needed - straightforward taint -> sink
+  exists(TaintToConstructorOrStaticMethodTrackingConfig taintTracking2 |
+    taintTracking2.hasFlowPath(userInput, deserializeCall)
+  )
+select deserializeCall, userInput, deserializeCall, "$@ flows to unsafe deserializer.", userInput,
   "User-provided data"

csharp/ql/src/Security Features/CWE-502/UnsafeDeserializationUntrustedInputTypeBad.cs

@@ -0,0 +1,13 @@
+using System.Runtime.Serialization.Json;
+using System.IO;
+using System;
+
+class BadDataContractJsonSerializer
+{
+    public static object Deserialize(string type, Stream s)
+    {
+        // BAD: stream and type are potentially untrusted
+        var ds = new DataContractJsonSerializer(Type.GetType(type));
+        return ds.ReadObject(s);
+    }
+}

csharp/ql/src/Security Features/CWE-502/UnsafeDeserializationUntrustedInputTypeGood.cs

@@ -0,0 +1,20 @@
+using System.Runtime.Serialization.Json;
+using System.IO;
+using System;
+
+class Poco
+{
+    public int Count;
+
+    public string Comment;
+}
+
+class GoodDataContractJsonSerializer
+{
+    public static Poco Deserialize(Stream s)
+    {
+        // GOOD: while stream is potentially untrusted, the instantiated type is hardcoded
+        var ds = new DataContractJsonSerializer(typeof(Poco));
+        return (Poco)ds.ReadObject(s);
+    }
+}