Merge pull request github#5508 from edvraa/deserializers

deserialization sinks

Author: hvitved

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

@@ -15,7 +15,46 @@ import csharp
 import semmle.code.csharp.security.dataflow.UnsafeDeserializationQuery
 import DataFlow::PathGraph
 
-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(),
-  "User-provided data"
+from DataFlow::PathNode userInput, DataFlow::PathNode deserializeCallArg
+where
+  exists(TaintToObjectMethodTrackingConfig taintTracking |
+    // all flows from user input to deserialization with weak and strong type serializers
+    taintTracking.hasFlowPath(userInput, deserializeCallArg)
+  ) and
+  // intersect with strong types, but user controlled or weak types deserialization usages
+  (
+    exists(
+      DataFlow::Node weakTypeCreation, DataFlow::Node weakTypeUsage,
+      WeakTypeCreationToUsageTrackingConfig weakTypeDeserializerTracking, MethodCall mc
+    |
+      weakTypeDeserializerTracking.hasFlow(weakTypeCreation, weakTypeUsage) and
+      mc.getQualifier() = weakTypeUsage.asExpr() and
+      mc.getAnArgument() = deserializeCallArg.getNode().asExpr()
+    )
+    or
+    exists(
+      TaintToObjectTypeTrackingConfig userControlledTypeTracking, DataFlow::Node taintedTypeUsage,
+      DataFlow::Node userInput2, MethodCall mc
+    |
+      userControlledTypeTracking.hasFlow(userInput2, taintedTypeUsage) and
+      mc.getQualifier() = taintedTypeUsage.asExpr() and
+      mc.getAnArgument() = deserializeCallArg.getNode().asExpr()
+    )
+  )
+  or
+  // no type check needed - straightforward taint -> sink
+  exists(TaintToConstructorOrStaticMethodTrackingConfig taintTracking2 |
+    taintTracking2.hasFlowPath(userInput, deserializeCallArg)
+  )
+  or
+  // JsonConvert static method call, but with additional unsafe typename tracking
+  exists(
+    JsonConvertTrackingConfig taintTrackingJsonConvert, TypeNameTrackingConfig typenameTracking,
+    DataFlow::Node settingsCallArg
+  |
+    taintTrackingJsonConvert.hasFlowPath(userInput, deserializeCallArg) and
+    typenameTracking.hasFlow(_, settingsCallArg) and
+    deserializeCallArg.getNode().asExpr().getParent() = settingsCallArg.asExpr().getParent()
+  )
+select deserializeCallArg, userInput, deserializeCallArg, "$@ flows to unsafe deserializer.",
+  userInput, "User-provided data"

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

@@ -6,7 +6,7 @@ class Good
     public static object Deserialize(TextBox textBox)
     {
         JavaScriptSerializer sr = new JavaScriptSerializer();
-        // GOOD
+        // GOOD: no unsafe type resolver
         return sr.DeserializeObject(textBox.Text);
     }
 }

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);
+    }
+}

csharp/ql/src/semmle/code/csharp/frameworks/JsonNET.qll

@@ -17,6 +17,19 @@ module JsonNET {
     JsonClass() { this.getParent() instanceof JsonNETNamespace }
   }
 
+  /** Newtonsoft.Json.TypeNameHandling enum */
+  class TypeNameHandlingEnum extends Enum {
+    TypeNameHandlingEnum() {
+      this.getParent() instanceof JsonNETNamespace and
+      this.hasName("TypeNameHandling")
+    }
+  }
+
+  /** Newtonsoft.Json.JsonSerializerSettings class */
+  class JsonSerializerSettingsClass extends JsonClass {
+    JsonSerializerSettingsClass() { this.hasName("JsonSerializerSettings") }
+  }
+
   /** The class `Newtonsoft.Json.JsonConvert`. */
   class JsonConvertClass extends JsonClass, LibraryTypeDataFlow {
     JsonConvertClass() { this.hasName("JsonConvert") }