Merge pull request #2658 from calumgrant/cs/serialization-check-bypass-type

C#: Fix cs/serialization-check-bypass

Author: hvitved

csharp/ql/src/Security Features/CWE-020/RuntimeChecksBypass.ql

@@ -11,6 +11,7 @@
 
 import semmle.code.csharp.serialization.Serialization
 import semmle.code.csharp.controlflow.Guards
+import semmle.code.csharp.dataflow.DataFlow
 
 /**
  * The result is a write to the field `f`, assigning it the value
@@ -29,7 +30,11 @@ GuardedExpr checkedWrite(Field f, Variable v, IfStmt check) {
 Expr uncheckedWrite(Callable callable, Field f) {
   result = f.getAnAssignedValue() and
   result.getEnclosingCallable() = callable and
-  not callable.calls*(checkedWrite(f, _, _).getEnclosingCallable())
+  not callable.calls*(checkedWrite(f, _, _).getEnclosingCallable()) and
+  // Exclude object creations because they were not deserialized
+  not exists(Expr src | DataFlow::localExprFlow(src, result) |
+    src instanceof ObjectCreation or src.hasValue()
+  )
 }
 
 from BinarySerializableType t, Field f, IfStmt check, Expr write, Expr unsafeWrite

csharp/ql/test/query-tests/Security Features/CWE-020/RuntimeChecksBypass.cs

@@ -10,14 +10,14 @@ public Test1(string v)
     {
         if (v == "valid")
         {
-            f = v /* safe write */;
+            f = v;  // GOOD
         }
     }
 
     [OnDeserializing]
     public void Deserialize()
     {
-        f = "invalid" /* unsafe write */;
+        f = $"invalid";  // BAD
     }
 }
 
@@ -30,19 +30,19 @@ public Test2(string v)
     {
         if (v == "valid")
         {
-            f = v /* safe write */;
+            f = v;  // GOOD
         }
     }
 
     [OnDeserializing]
     public void Deserialize()
     {
-        var v = "invalid";
-        f = v /* unsafe write -- false negative */;
+        var v = $"invalid";
+        f = v;  // BAD: False negative
 
         if (v == "valid")
         {
-            f = v;  /* safe write */
+            f = v;  // GOOD
         }
     }
 }
@@ -56,25 +56,25 @@ public Test3(string v)
     {
         if (v == "valid")
         {
-            f = v /* safe write */;
+            f = v;  // GOOD
         }
     }
 
     [OnDeserializing]
     public void Deserialize()
     {
-        var v = "invalid";
-        f = v /* unsafe write -- false negative */;
+        var v = $"invalid";
+        f = v;  // GOOD: False negative
         Assign(v);
     }
 
     private void Assign(string v)
     {
-        f = v /* unsafe write -- false negative */;
+        f = v;  // GOOD: False negative
 
         if (v == "valid")
         {
-            f = v /* safe write */;
+            f = v;  // GOOD
         }
     }
 }
@@ -88,21 +88,21 @@ public Test4(string v)
     {
         if (v == "valid")
         {
-            f = v /* safe write */;
+            f = v;  // GOOD
         }
     }
 
     [OnDeserializing]
     public void Deserialize()
     {
-        var v = "invalid";
+        var v = $"invalid";
         if (v == "valid")
             Assign(v);
     }
 
     private void Assign(string v)
     {
-        f = v /* safe write */;
+        f = v;  // GOOD
     }
 }
 
@@ -115,13 +115,13 @@ public Test5(int age)
     {
         if (age < 0)
             throw new ArgumentException(nameof(age));
-        Age = age /* safe write */;
+        Age = age;  // GOOD
     }
 
     [OnDeserializing]
     void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
     {
-        Age = info.GetInt32("age"); /* unsafe write */;
+        Age = info.GetInt32("age");  // BAD
     }
 }
 
@@ -134,7 +134,7 @@ public Test6(int age)
     {
         if (age < 0)
             throw new ArgumentException(nameof(age));
-        Age = age /* safe write */;
+        Age = age;  // GOOD
     }
 
     [OnDeserializing]
@@ -143,7 +143,7 @@ void ISerializable.GetObjectData(SerializationInfo info, StreamingContext contex
         int age = info.GetInt32("age");
         if (age < 0)
             throw new SerializationException("age");
-        Age = age; /* safe write */;
+        Age = age;  // GOOD
     }
 }
 
@@ -156,7 +156,7 @@ public Test7(int age)
     {
         if (age < 0)
             throw new ArgumentException(nameof(age));
-        Age = age /* safe write */;
+        Age = age;  // GOOD
     }
 
     [OnDeserializing]
@@ -165,6 +165,27 @@ void ISerializable.GetObjectData(SerializationInfo info, StreamingContext contex
         int age = info.GetInt32("age");
         if (false)
             throw new SerializationException("age");
-        Age = age; /* unsafe write */;
+        Age = age;  // BAD
+    }
+}
+
+[Serializable]
+public class Test8 : ISerializable
+{
+    string Options;
+
+    public int Age;
+
+    public Test8(string options)
+    {
+        if (options == null)
+            throw new ArgumentNullException(nameof(options));
+        Options = options;  // GOOD
+    }
+
+    [OnDeserializing]
+    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
+    {
+        Options = new string("");  // GOOD: A created object
     }
 }

csharp/ql/test/query-tests/Security Features/CWE-020/RuntimeChecksbypass.expected

@@ -1,4 +1,4 @@
-| RuntimeChecksBypass.cs:20:13:20:21 | "invalid" | This write to $@ may be circumventing a $@. | RuntimeChecksBypass.cs:7:19:7:19 | f | f | RuntimeChecksBypass.cs:11:9:14:9 | if (...) ... | check |
+| RuntimeChecksBypass.cs:20:13:20:22 | $"..." | This write to $@ may be circumventing a $@. | RuntimeChecksBypass.cs:7:19:7:19 | f | f | RuntimeChecksBypass.cs:11:9:14:9 | if (...) ... | check |
 | RuntimeChecksBypass.cs:124:15:124:34 | call to method GetInt32 | This write to $@ may be circumventing a $@. | RuntimeChecksBypass.cs:112:16:112:18 | Age | Age | RuntimeChecksBypass.cs:116:9:117:53 | if (...) ... | check |
 | RuntimeChecksBypass.cs:168:15:168:17 | access to local variable age | This write to $@ may be circumventing a $@. | RuntimeChecksBypass.cs:153:16:153:18 | Age | Age | RuntimeChecksBypass.cs:157:9:158:53 | if (...) ... | check |
 | RuntimeChecksBypassBad.cs:19:15:19:34 | call to method GetInt32 | This write to $@ may be circumventing a $@. | RuntimeChecksBypassBad.cs:7:16:7:18 | Age | Age | RuntimeChecksBypassBad.cs:11:9:12:53 | if (...) ... | check |