Merge pull request #847 from calumgrant/cs/json.net

C#: Model Json.NET dataflow

change-notes/1.20/analysis-csharp.md

@@ -27,5 +27,6 @@
 ## Changes to QL libraries
 
 * The class `TrivialProperty` now includes library properties determined to be trivial using CIL analysis. This may increase the number of results for all queries that use data flow.
+* Taint-tracking steps have been added for the `Json.NET` package. This will improve results for queries that use taint-tracking.
 
 ## Changes to the autobuilder

csharp/ql/src/semmle/code/csharp/dataflow/TaintTracking.qll

@@ -9,6 +9,7 @@ module TaintTracking {
   private import semmle.code.csharp.dataflow.LibraryTypeDataFlow
   private import semmle.code.csharp.dispatch.Dispatch
   private import semmle.code.csharp.commons.ComparisonTest
+  private import semmle.code.csharp.frameworks.JsonNET
   private import cil
   private import dotnet
 
@@ -18,6 +19,9 @@ module TaintTracking {
    */
   predicate localTaint(DataFlow::Node source, DataFlow::Node sink) { localTaintStep*(source, sink) }
 
+  /** A member (property or field) that is tainted if its containing object is tainted. */
+  abstract class TaintedMember extends AssignableMember { }
+
   /**
    * Holds if taint propagates from `nodeFrom` to `nodeTo` in exactly one local
    * (intra-procedural) step.
@@ -243,6 +247,16 @@ module TaintTracking {
         DataFlow::Internal::flowOutOfDelegateLibraryCall(nodeFrom, nodeTo, false)
         or
         localTaintStepCil(nodeFrom, nodeTo)
+        or
+        // Taint members
+        exists(Access access |
+          access = nodeTo.asExpr() and
+          access.getTarget() instanceof TaintedMember
+        |
+          access.(FieldRead).getQualifier() = nodeFrom.asExpr()
+          or
+          access.(PropertyRead).getQualifier() = nodeFrom.asExpr()
+        )
       }
     }
     import Cached

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

@@ -0,0 +1,231 @@
+/**
+ * Classes for modelling Json.NET.
+ */
+
+import csharp
+import semmle.code.csharp.dataflow.LibraryTypeDataFlow
+
+/** Definitions relating to the `Json.NET` package. */
+module JsonNET {
+  /** The namespace `Newtonsoft.Json`. */
+  class JsonNETNamespace extends Namespace {
+    JsonNETNamespace() { this.hasQualifiedName("Newtonsoft.Json") }
+  }
+
+  /** A class in `Newtonsoft.Json`. */
+  class JsonClass extends Class { JsonClass() { this.getParent() instanceof JsonNETNamespace } }
+
+  /** The class `Newtonsoft.Json.JsonConvert`. */
+  class JsonConvertClass extends JsonClass, LibraryTypeDataFlow {
+    JsonConvertClass() { this.hasName("JsonConvert") }
+
+    /** Gets a `ToString` method. */
+    private Method getAToStringMethod() {
+      result = this.getAMethod("ToString") and
+      result.isStatic()
+    }
+
+    /** Gets a `Deserialize` method. */
+    Method getADeserializeMethod() {
+      result = this.getAMethod() and
+      result.getName().matches("Deserialize%")
+    }
+
+    /** Gets a `Serialize` method. */
+    Method getASerializeMethod() {
+      result = this.getAMethod() and
+      result.getName().matches("Serialize%")
+    }
+
+    private Method getAPopulateMethod() {
+      result = this.getAMethod() and
+      result.getName().matches("Populate%")
+    }
+
+    override predicate callableFlow(
+      CallableFlowSource source, CallableFlowSink sink, SourceDeclarationCallable c,
+      boolean preservesValue
+    ) {
+      // ToString methods
+      c = getAToStringMethod() and
+      preservesValue = true and
+      source = any(CallableFlowSourceArg arg | arg.getArgumentIndex() = 0) and
+      sink instanceof CallableFlowSinkReturn
+      or
+      // Deserialize methods
+      c = getADeserializeMethod() and
+      preservesValue = false and
+      source = any(CallableFlowSourceArg arg | arg.getArgumentIndex() = 0) and
+      sink instanceof CallableFlowSinkReturn
+      or
+      // Serialize methods
+      c = getASerializeMethod() and
+      preservesValue = false and
+      source = any(CallableFlowSourceArg arg | arg.getArgumentIndex() = 0) and
+      sink instanceof CallableFlowSinkReturn
+      or
+      // Populate methods
+      c = getAPopulateMethod() and
+      preservesValue = false and
+      source = any(CallableFlowSourceArg arg | arg.getArgumentIndex() = 0) and
+      sink = any(CallableFlowSinkArg arg | arg.getArgumentIndex() = 1)
+    }
+  }
+
+  /** A type that is serialized. */
+  private class SerializedType extends ValueOrRefType {
+    SerializedType() {
+      // Supplied as a parameter to a serialize or deserialize method
+      exists(TypeParameter tp, JsonConvertClass jc, UnboundGenericMethod serializeMethod |
+        this = tp.getAnUltimatelySuppliedType() and
+        tp = serializeMethod.getATypeParameter()
+      |
+        serializeMethod = jc.getASerializeMethod()
+        or
+        serializeMethod = jc.getADeserializeMethod()
+      )
+      or
+      exists(Class attribute | attribute = this.getAnAttribute().getType() |
+        attribute instanceof JsonObjectAttributeClass
+        or
+        attribute.hasName("JsonConverterAttribute")
+      )
+      or
+      this.getAConstructor().getAnAttribute().getType().hasName("JsonConstructorAttribute")
+    }
+
+    predicate isOptIn() { this.getAnAttribute().(JsonObjectAttribute).isOptIn() }
+  }
+
+  /**
+   * A field/property that can be serialized, either explicitly
+   * or as a member of a serialized type.
+   */
+  private class SerializedMember extends TaintTracking::TaintedMember {
+    SerializedMember() {
+      // This member has a Json attribute
+      exists(Class attribute | attribute = this.(Attributable).getAnAttribute().getType() |
+        attribute.hasName("JsonPropertyAttribute")
+        or
+        attribute.hasName("JsonDictionaryAttribute")
+        or
+        attribute.hasName("JsonRequiredAttribute")
+        or
+        attribute.hasName("JsonArrayAttribute")
+        or
+        attribute.hasName("JsonConverterAttribute")
+        or
+        attribute.hasName("JsonExtensionDataAttribute")
+        or
+        attribute.hasName("SerializableAttribute") // System.SerializableAttribute
+        or
+        attribute.hasName("DataMemberAttribute") // System.DataMemberAttribute
+      )
+      or
+      // This field is a member of an explicitly serialized type
+      this.getDeclaringType() instanceof SerializedType and
+      not this.getDeclaringType().(SerializedType).isOptIn() and
+      not this.(Attributable).getAnAttribute().getType() instanceof NotSerializedAttributeClass
+    }
+  }
+
+  /** The class `NewtonSoft.Json.JsonSerializer`. */
+  class JsonSerializerClass extends JsonClass, LibraryTypeDataFlow {
+    JsonSerializerClass() { this.hasName("JsonSerializer") }
+
+    Method getSerializeMethod() { result = this.getAMethod("Serialize") }
+
+    Method getDeserializeMethod() { result = this.getAMethod("Deserialize") }
+
+    override predicate callableFlow(
+      CallableFlowSource source, CallableFlowSink sink, SourceDeclarationCallable c,
+      boolean preservesValue
+    ) {
+      // Serialize
+      c = this.getSerializeMethod() and
+      preservesValue = false and
+      source = any(CallableFlowSourceArg arg | arg.getArgumentIndex() = 0) and
+      sink = any(CallableFlowSinkArg arg | arg.getArgumentIndex() = 1)
+      or
+      // Deserialize
+      c = this.getDeserializeMethod() and
+      preservesValue = false and
+      source = any(CallableFlowSourceArg arg | arg.getArgumentIndex() = 0) and
+      sink = any(CallableFlowSinkArg arg | arg.getArgumentIndex() = 1)
+    }
+  }
+
+  /** Any attribute class that marks a member to not be serialized. */
+  private class NotSerializedAttributeClass extends JsonClass {
+    NotSerializedAttributeClass() {
+      this.hasName("JsonIgnoreAttribute") or this.hasName("NonSerializedAttribute")
+    }
+  }
+
+  /** The class `Newtonsoft.Json.ObjectAttribute`. */
+  class JsonObjectAttributeClass extends JsonClass {
+    JsonObjectAttributeClass() { this.hasName("JsonObjectAttribute") }
+  }
+
+  /** An attribute of type `Newtonsoft.Json.ObjectAttribute`. */
+  class JsonObjectAttribute extends Attribute {
+    JsonObjectAttribute() { this.getType() instanceof JsonObjectAttributeClass }
+
+    /** Holds if the `OptIn` argument has been supplied to this attribute. */
+    predicate isOptIn() { this.getArgument(_).(FieldAccess).getTarget().hasName("OptIn") }
+  }
+
+  /** The namespace `Newtonsoft.Json.Linq`. */
+  class LinqNamespace extends Namespace {
+    LinqNamespace() {
+      this.getParentNamespace() instanceof JsonNETNamespace and this.hasName("Linq")
+    }
+  }
+
+  /** A class in `Newtonsoft.Json.Linq`. */
+  class LinqClass extends Class {
+    LinqClass() { this.getDeclaringNamespace() instanceof LinqNamespace }
+  }
+
+  /** The `NewtonSoft.Json.Linq.JObject` class. */
+  class JObjectClass extends LinqClass, LibraryTypeDataFlow {
+    JObjectClass() { this.hasName("JObject") }
+
+    override predicate callableFlow(
+      CallableFlowSource source, CallableFlowSink sink, SourceDeclarationCallable c,
+      boolean preservesValue
+    ) {
+      // ToString method
+      c = this.getAMethod("ToString") and
+      source instanceof CallableFlowSourceQualifier and
+      sink instanceof CallableFlowSinkReturn and
+      preservesValue = false
+      or
+      // Parse method
+      c = this.getParseMethod() and
+      source = any(CallableFlowSourceArg arg | arg.getArgumentIndex() = 0) and
+      sink instanceof CallableFlowSinkReturn and
+      preservesValue = false
+      or
+      // operator string
+      c = any(Operator op |
+          op.getDeclaringType() = this.getABaseType*() and op.getReturnType() instanceof StringType
+        ) and
+      source = any(CallableFlowSourceArg arg | arg.getArgumentIndex() = 0) and
+      sink instanceof CallableFlowSinkReturn and
+      preservesValue = false
+      or
+      // SelectToken method
+      c = this.getSelectTokenMethod() and
+      source instanceof CallableFlowSourceQualifier and
+      sink instanceof CallableFlowSinkReturn and
+      preservesValue = false
+    }
+
+    /** Gets the `Parse` method. */
+    Method getParseMethod() { result = this.getAMethod("Parse") }
+
+    /** Gets the `SelectToken` method. */
+    Method getSelectTokenMethod() { result = this.getABaseType*().getAMethod("SelectToken") }
+  }
+}

csharp/ql/test/library-tests/frameworks/JsonNET/Json.cs

@@ -0,0 +1,81 @@
+// semmle-extractor-options: ${testdir}/../../../resources/stubs/JsonNET.cs /r:System.Linq.dll
+
+using Newtonsoft.Json;
+using Newtonsoft.Json.Linq;
+using System.Collections.Generic;
+using System.Linq;
+
+namespace JsonTest
+{
+    class Dataflow
+    {
+        void Sink(object o)
+        {
+        }
+
+        void F()
+        {
+            string t = "tainted";
+            string u = "untainted";
+
+            Sink(JsonConvert.ToString(int.Parse(t)));
+
+            var taintedObject = JsonConvert.DeserializeObject<Object>(t);
+            Sink(taintedObject);
+            Sink(taintedObject.tainted);
+            Sink(taintedObject.untainted);
+            Sink(JsonConvert.SerializeObject(taintedObject));
+            Sink(taintedObject.taintedValues["1"]);
+            Sink(taintedObject.taintedArray[0]);
+
+            var taintedObject2 = JsonConvert.DeserializeObject<Object2>(t);
+            Sink(taintedObject2.tainted);
+            Sink(taintedObject2.untainted);
+
+            Object taintedPopulatedObject = new Object();
+            JsonConvert.PopulateObject(t, taintedPopulatedObject);
+            Sink(taintedPopulatedObject.tainted);  // False negative
+
+            Object untaintedObject = JsonConvert.DeserializeObject<Object>(u);
+            Sink(untaintedObject);
+
+            // JObject tests
+            var jobject = JObject.Parse(t);
+            Sink(jobject);
+            Sink(jobject["1"]);
+            Sink(jobject["1"]["2"]);
+            Sink((string)jobject["1"]["2"]);
+
+            // Linq JToken tests
+            Sink(jobject.First(i => true));
+            Sink(jobject["2"].First(i => true));
+            Sink(jobject["2"]["3"].First(i => true));
+            Sink(jobject.SelectToken("Manufacturers[0].Name"));
+
+            JObject untaintedJObject = JObject.Parse(u);
+            Sink(untaintedJObject);
+            Sink(untaintedJObject.First(i => true));
+        }
+
+        public class Object
+        {
+            public int tainted;
+
+            [JsonIgnore]
+            public int untainted;
+
+            public Dictionary<string,string> taintedValues;
+
+            public string[] taintedArray;
+        }
+
+        [JsonObject(MemberSerialization.OptIn)]
+        public class Object2
+        {
+            public int untainted;
+
+            [JsonRequired]
+            public int tainted;
+        }
+    }
+}

csharp/ql/test/library-tests/frameworks/JsonNET/Json.expected

@@ -0,0 +1,15 @@
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:21:18:21:51 | call to method ToString |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:24:18:24:30 | access to local variable taintedObject |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:25:18:25:38 | (...) ... |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:27:18:27:59 | call to method SerializeObject |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:28:18:28:49 | access to indexer |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:29:18:29:46 | access to array element |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:32:18:32:39 | (...) ... |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:44:18:44:24 | access to local variable jobject |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:45:18:45:29 | access to indexer |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:46:18:46:34 | access to indexer |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:47:18:47:42 | call to operator explicit conversion |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:50:18:50:41 | call to method First |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:51:18:51:46 | call to method First |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:52:18:52:51 | call to method First |
+| Json.cs:18:24:18:32 | "tainted" | Json.cs:53:18:53:61 | call to method SelectToken |

csharp/ql/test/library-tests/frameworks/JsonNET/Json.ql

@@ -0,0 +1,18 @@
+import csharp
+import semmle.code.csharp.dataflow.TaintTracking
+
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "Json.NET test" }
+
+  override predicate isSource(DataFlow::Node src) {
+    src.asExpr().(StringLiteral).getValue() = "tainted"
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodCall c | c.getArgument(0) = sink.asExpr() and c.getTarget().getName() = "Sink")
+  }
+}
+
+from Configuration c, DataFlow::Node source, DataFlow::Node sink
+where c.hasFlow(source, sink)
+select source, sink

csharp/ql/test/resources/stubs/JsonNET.cs

@@ -0,0 +1,56 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;
+
+namespace Newtonsoft.Json
+{
+    public static class JsonConvert
+    {
+        public static string ToString(int x) => null;
+        public static T DeserializeObject<T>(string s) => default(T);
+        public static string SerializeObject(object obj) => null;
+        public static void PopulateObject(string s, object obj) { }
+    }
+
+    public class JsonIgnoreAttribute : Attribute
+    {
+    }
+
+    public class JsonRequiredAttribute : Attribute
+    {
+    }
+
+    public class JsonLoadSettings { }
+
+    public enum MemberSerialization { OptOut, OptIn, Fields }
+
+    public class JsonObjectAttribute : Attribute
+    {
+      public JsonObjectAttribute() { }
+      public JsonObjectAttribute(MemberSerialization ms) { }
+    }
+}
+
+namespace Newtonsoft.Json.Linq
+{
+    public class JToken : IEnumerable<JToken>, IEnumerable
+    {
+        public virtual JToken this[object key] => null;
+        public virtual JToken this[string key] => null;
+
+        public IEnumerator<JToken> GetEnumerator() => null;
+        IEnumerator IEnumerable.GetEnumerator() => null;
+
+        public static explicit operator string(JToken t) => null;
+
+        public IEnumerable<JToken> SelectToken(string s) => null; 
+    }
+
+    public class JObject : JToken
+    {
+        public static JObject Parse(string str) => null;
+        public static JObject Parse(string str, JsonLoadSettings settings) => null;
+        public JToken this[object key] => null;
+        public JToken this[string key] => null;
+    }
+}