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

C#: Improvements to cs/serialization-check-bypass

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

@@ -5,10 +5,40 @@
 
 <overview>
 <p>
- A write that looks like it may be bypassing runtime checks.
+Fields that are deserialized should be validated, otherwise the deserialized object
+could contain invalid data.
+</p>
+
+<p>
+This query finds cases where a field is validated in a constructor, but not in a deserialization method.
+This is an indication that the deserialization method is missing a validation step.
 </p>
 
 </overview>
+
+<recommendation>
+<p>
+If a field needs to be validated, then ensure that validation is also performed during deserialization.
+</p>
+</recommendation>
+
+<example>
+
+<p>
+The following example has the validation of the <code>Age</code> field in the constructor
+but not in the deserialization method:
+</p>
+
+<sample src="RuntimeChecksBypassBad.cs" />
+
+<p>
+The problem is fixed by adding validation to the deserialization method as follows:
+</p>
+
+<sample src="RuntimeChecksBypassGood.cs" />
+
+</example>
+
 <references>
 
 <li>

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

@@ -13,23 +13,31 @@
  */
 
 import semmle.code.csharp.serialization.Serialization
+import semmle.code.csharp.controlflow.Guards
 
 /**
  * The result is a write to the field `f`, assigning it the value
  * of variable `v` which was checked by the condition `check`.
  */
-Expr checkedWrite(Field f, Variable v, IfStmt check) {
+GuardedExpr checkedWrite(Field f, Variable v, IfStmt check) {
   result = v.getAnAccess() and
   result = f.getAnAssignedValue() and
-  check.getCondition() = v.getAnAccess().getParent*() and
-  result.getAControlFlowNode() = check.getAControlFlowNode().getASuccessor*()
+  check.getCondition().getAChildExpr*() = result.getAGuard(_, _)
+}
+
+/**
+ * The result is an unsafe write to the field `f`, where
+ * there is no check performed within the (calling) scope of the method.
+ */
+Expr uncheckedWrite(Callable callable, Field f) {
+  result = f.getAnAssignedValue() and
+  result.getEnclosingCallable() = callable and
+  not callable.calls*(checkedWrite(f, _, _).getEnclosingCallable())
 }
 
 from BinarySerializableType t, Field f, IfStmt check, Expr write, Expr unsafeWrite
 where
   f = t.getASerializedField() and
   write = checkedWrite(f, t.getAConstructor().getAParameter(), check) and
-  unsafeWrite = f.getAnAssignedValue() and
-  t.getADeserializationCallback() = unsafeWrite.getEnclosingCallable() and
-  not t.getADeserializationCallback().calls*(checkedWrite(f, _, _).getEnclosingCallable())
+  unsafeWrite = uncheckedWrite(t.getADeserializationCallback(), f)
 select unsafeWrite, "This write to $@ may be circumventing a $@.", f, f.toString(), check, "check"

csharp/ql/src/Security Features/CWE-020/RuntimeChecksBypassBad.cs

@@ -0,0 +1,21 @@
+using System;
+using System.Runtime.Serialization;
+
+[Serializable]
+public class PersonBad : ISerializable
+{
+    public int Age;
+
+    public PersonBad(int age)
+    {
+        if (age < 0)
+            throw new ArgumentException(nameof(age));
+        Age = age;
+    }
+
+    [OnDeserializing]
+    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
+    {
+        Age = info.GetInt32("age");  // BAD - write is unsafe
+    }
+}

csharp/ql/src/Security Features/CWE-020/RuntimeChecksBypassGood.cs

@@ -0,0 +1,24 @@
+using System;
+using System.Runtime.Serialization;
+
+[Serializable]
+public class PersonGood : ISerializable
+{
+    public int Age;
+
+    public PersonGood(int age)
+    {
+        if (age < 0)
+            throw new ArgumentException(nameof(age));
+        Age = age;
+    }
+
+    [OnDeserializing]
+    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
+    {
+        int age = info.GetInt32("age");
+        if (age < 0)
+            throw new SerializationException(nameof(Age));
+        Age = age;  // GOOD - write is safe
+    }
+}

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

@@ -0,0 +1,170 @@
+using System;
+using System.Runtime.Serialization;
+
+[Serializable]
+public class Test1
+{
+    public string f;
+
+    public Test1(string v)
+    {
+        if (v == "valid")
+        {
+            f = v /* safe write */;
+        }
+    }
+
+    [OnDeserializing]
+    public void Deserialize()
+    {
+        f = "invalid" /* unsafe write */;
+    }
+}
+
+[Serializable]
+public class Test2
+{
+    public string f;
+
+    public Test2(string v)
+    {
+        if (v == "valid")
+        {
+            f = v /* safe write */;
+        }
+    }
+
+    [OnDeserializing]
+    public void Deserialize()
+    {
+        var v = "invalid";
+        f = v /* unsafe write -- false negative */;
+
+        if (v == "valid")
+        {
+            f = v;  /* safe write */
+        }
+    }
+}
+
+[Serializable]
+public class Test3
+{
+    public string f;
+
+    public Test3(string v)
+    {
+        if (v == "valid")
+        {
+            f = v /* safe write */;
+        }
+    }
+
+    [OnDeserializing]
+    public void Deserialize()
+    {
+        var v = "invalid";
+        f = v /* unsafe write -- false negative */;
+        Assign(v);
+    }
+
+    private void Assign(string v)
+    {
+        f = v /* unsafe write -- false negative */;
+
+        if (v == "valid")
+        {
+            f = v /* safe write */;
+        }
+    }
+}
+
+[Serializable]
+public class Test4
+{
+    public string f;
+
+    public Test4(string v)
+    {
+        if (v == "valid")
+        {
+            f = v /* safe write */;
+        }
+    }
+
+    [OnDeserializing]
+    public void Deserialize()
+    {
+        var v = "invalid";
+        if (v == "valid")
+            Assign(v);
+    }
+
+    private void Assign(string v)
+    {
+        f = v /* safe write */;
+    }
+}
+
+[Serializable]
+public class Test5 : ISerializable
+{
+    public int Age;
+
+    public Test5(int age)
+    {
+        if (age < 0)
+            throw new ArgumentException(nameof(age));
+        Age = age /* safe write */;
+    }
+
+    [OnDeserializing]
+    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
+    {
+        Age = info.GetInt32("age"); /* unsafe write */;
+    }
+}
+
+[Serializable]
+public class Test6 : ISerializable
+{
+    public int Age;
+
+    public Test6(int age)
+    {
+        if (age < 0)
+            throw new ArgumentException(nameof(age));
+        Age = age /* safe write */;
+    }
+
+    [OnDeserializing]
+    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
+    {
+        int age = info.GetInt32("age");
+        if (age < 0)
+            throw new SerializationException("age");
+        Age = age; /* safe write */;
+    }
+}
+
+[Serializable]
+public class Test7 : ISerializable
+{
+    public int Age;
+
+    public Test7(int age)
+    {
+        if (age < 0)
+            throw new ArgumentException(nameof(age));
+        Age = age /* safe write */;
+    }
+
+    [OnDeserializing]
+    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
+    {
+        int age = info.GetInt32("age");
+        if (false)
+            throw new SerializationException("age");
+        Age = age; /* unsafe write */;
+    }
+}

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

@@ -0,0 +1,21 @@
+using System;
+using System.Runtime.Serialization;
+
+[Serializable]
+public class PersonBad : ISerializable
+{
+    public int Age;
+
+    public PersonBad(int age)
+    {
+        if (age < 0)
+            throw new ArgumentException(nameof(age));
+        Age = age;
+    }
+
+    [OnDeserializing]
+    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
+    {
+        Age = info.GetInt32("age");  // BAD - write is unsafe
+    }
+}

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

@@ -0,0 +1,24 @@
+using System;
+using System.Runtime.Serialization;
+
+[Serializable]
+public class PersonGood : ISerializable
+{
+    public int Age;
+
+    public PersonGood(int age)
+    {
+        if (age < 0)
+            throw new ArgumentException(nameof(age));
+        Age = age;
+    }
+
+    [OnDeserializing]
+    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
+    {
+        int age = info.GetInt32("age");
+        if (age < 0)
+            throw new SerializationException(nameof(Age));
+        Age = age;  // GOOD - write is safe
+    }
+}

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

@@ -0,0 +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: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 |

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

@@ -0,0 +1 @@
+Security Features/CWE-020/RuntimeChecksBypass.ql