Added DataSetSurrogate to guard against the DataSet attacks.

Improved the API: new BinaryFormatter().Safe().
zyanfx · Apr 25, 2018 · 7292308 · 7292308
1 parent fa02939
commit 7292308
Show file tree

Hide file tree

Showing 10 changed files with 148 additions and 100 deletions.
diff --git a/README.md b/README.md
@@ -14,8 +14,7 @@ var fmt = new BinaryFormatter();
 var object = fmt.Deserialize(stream);
 
 // better: deserialization is checked against known vulnerabilities
-var fmt = new BinaryFormatter();
-fmt.Binder = new SafeSerializationBinder();
+var fmt = new BinaryFormatter().Safe();
 var object = fmt.Deserialize(stream);
 ```
 

diff --git a/SafeDeserializationHelpers.Tests/SafeSerializationBinderTests.cs b/SafeDeserializationHelpers.Tests/SafeSerializationBinderTests.cs
@@ -53,23 +53,34 @@ public void SafeSerializationBinderDoesntBreakNormalClasses()
             row["Name"] = "432";
             dt.Rows.Add(row);
 
+            var rdt = new DataTable("AnotherTestTable");
+            rdt.Columns.Add("Value", typeof(string));
+            row = rdt.NewRow();
+            row["Value"] = "Hello";
+            rdt.Rows.Add(row);
+
             // data sets
-            var ds = new DataSet("TestData");
+            var ds = new DataSet("XmlTestData");
             ds.Tables.Add(dt);
+            ds.RemotingFormat = SerializationFormat.Xml;
+
+            var rds = new DataSet("BinaryTestData");
+            rds.RemotingFormat = SerializationFormat.Binary;
+            rds.Tables.Add(rdt);
 
             // scalar values, collections and dictionaries
             var samples = new object[]
             {
-                1, "Test", func, action, dt, ds,
+                1, "Test", func, action, dt, ds, rds,
                 new List<string> { "abc", "def" },
                 new Dictionary<int, char> { { 1, 'a' }, { 2, 'b'} },
                 new Hashtable { { "Hello", "World" } },
                 new List<Delegate> { func, action, action, func },
                 new PublicSerializable { X = 123 },
                 new PrivateSerializable { Y = "Hello" },
-                new DataSet[] { ds, null, null, ds, ds },
-                new List<DataSet> { null, ds, null },
-                new Dictionary<string, DataSet> { { ds.DataSetName, ds } }
+                new DataSet[] { ds, null, null, rds, ds },
+                new List<DataSet> { null, rds, ds },
+                new Dictionary<string, DataSet> { { ds.DataSetName, ds }, { rds.DataSetName, rds } },
             };
 
             // make sure that the round-trip doesn't damage any of them

diff --git a/SafeDeserializationHelpers.Tests/TestBase.cs b/SafeDeserializationHelpers.Tests/TestBase.cs
@@ -4,14 +4,15 @@
 using System.IO;
 using System.Linq;
 using System.Management.Automation;
+using System.Runtime.Serialization;
 using System.Runtime.Serialization.Formatters.Binary;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 
 namespace SafeDeserializationHelpers.Tests
 {
     public class TestBase
     {
-        protected void Roundtrip(object graph, bool useBinder)
+        protected void Roundtrip(object graph, bool safe)
         {
             var data = default(byte[]);
             var fmt = new BinaryFormatter();
@@ -21,15 +22,15 @@ protected void Roundtrip(object graph, bool useBinder)
                 data = stream.ToArray();
             }
 
-            if (useBinder)
+            if (safe)
             {
-                fmt.Binder = new SafeSerializationBinder(fmt.Binder);
+                fmt = fmt.Safe();
             }
 
             using (var stream = new MemoryStream(data))
             {
                 var deserialized = fmt.Deserialize(stream);
-                var msg = $"Deserialized data doesn't match when {(useBinder ? string.Empty : "not ")}using binder.";
+                var msg = $"Deserialized data doesn't match when {(safe ? string.Empty : "not ")}using binder.";
                 Assert_AreEqual(graph, deserialized, msg);
             }
         }

diff --git a/SafeDeserializationHelpers.Tests/YsoserialGadgetTests.cs b/SafeDeserializationHelpers.Tests/YsoserialGadgetTests.cs
@@ -13,8 +13,7 @@ private void DeserializeMaliciousPayload(string comment, string base64)
             var payload = Convert.FromBase64String(base64);
             using (var stream = new MemoryStream(payload))
             {
-                var fmt = new BinaryFormatter();
-                fmt.Binder = new SafeSerializationBinder();
+                var fmt = new BinaryFormatter().Safe();
 
                 Assert_Throws<UnsafeDeserializationException>(() => fmt.Deserialize(stream),
                     $"Failed to detect malicious payload: {comment}");
@@ -35,7 +34,7 @@ public void PSObjectTest()
                 @"AAEAAAD/////AQAAAAAAAAAMAgAAAF9TeXN0ZW0uTWFuYWdlbWVudC5BdXRvbWF0aW9uLCBWZXJzaW9uPTMuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49MzFiZjM4NTZhZDM2NGUzNQUBAAAAJVN5c3RlbS5NYW5hZ2VtZW50LkF1dG9tYXRpb24uUFNPYmplY3QBAAAABkNsaVhtbAECAAAABgMAAACJFQ0KPE9ianMgVmVyc2lvbj0iMS4xLjAuMSIgeG1sbnM9Imh0dHA6Ly9zY2hlbWFzLm1pY3Jvc29mdC5jb20vcG93ZXJzaGVsbC8yMDA0LzA0Ij4mI3hEOw0KPE9iaiBSZWZJZD0iMCI+JiN4RDsNCiAgICA8VE4gUmVmSWQ9IjAiPiYjeEQ7DQogICAgICA8VD5NaWNyb3NvZnQuTWFuYWdlbWVudC5JbmZyYXN0cnVjdHVyZS5DaW1JbnN0YW5jZSNTeXN0ZW0uTWFuYWdlbWVudC5BdXRvbWF0aW9uL1J1bnNwYWNlSW52b2tlNTwvVD4mI3hEOw0KICAgICAgPFQ+TWljcm9zb2Z0Lk1hbmFnZW1lbnQuSW5mcmFzdHJ1Y3R1cmUuQ2ltSW5zdGFuY2UjUnVuc3BhY2VJbnZva2U1PC9UPiYjeEQ7DQogICAgICA8VD5NaWNyb3NvZnQuTWFuYWdlbWVudC5JbmZyYXN0cnVjdHVyZS5DaW1JbnN0YW5jZTwvVD4mI3hEOw0KICAgICAgPFQ+U3lzdGVtLk9iamVjdDwvVD4mI3hEOw0KICAgIDwvVE4+JiN4RDsNCiAgICA8VG9TdHJpbmc+UnVuc3BhY2VJbnZva2U1PC9Ub1N0cmluZz4mI3hEOw0KICAgIDxPYmogUmVmSWQ9IjEiPiYjeEQ7DQogICAgICA8VE5SZWYgUmVmSWQ9IjAiIC8+JiN4RDsNCiAgICAgIDxUb1N0cmluZz5SdW5zcGFjZUludm9rZTU8L1RvU3RyaW5nPiYjeEQ7DQogICAgICA8UHJvcHM+JiN4RDsNCiAgICAgICAgPE5pbCBOPSJQU0NvbXB1dGVyTmFtZSIgLz4mI3hEOw0KCQk8T2JqIE49InRlc3QxIiBSZWZJZCA9IjIwIiA+ICYjeEQ7DQogICAgICAgICAgPFROIFJlZklkPSIxIiA+ICYjeEQ7DQogICAgICAgICAgICA8VD5TeXN0ZW0uV2luZG93cy5NYXJrdXAuWGFtbFJlYWRlcltdLCBQcmVzZW50YXRpb25GcmFtZXdvcmssIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj0zMWJmMzg1NmFkMzY0ZTM1PC9UPiYjeEQ7DQogICAgICAgICAgICA8VD5TeXN0ZW0uQXJyYXk8L1Q+JiN4RDsNCiAgICAgICAgICAgIDxUPlN5c3RlbS5PYmplY3Q8L1Q+JiN4RDsNCiAgICAgICAgICA8L1ROPiYjeEQ7DQogICAgICAgICAgPExTVD4mI3hEOw0KICAgICAgICAgICAgPFMgTj0iSGFzaCIgPiAgDQoJCSZsdDtSZXNvdXJjZURpY3Rpb25hcnkNCiAgeG1sbnM9Imh0dHA6Ly9zY2hlbWFzLm1pY3Jvc29mdC5jb20vd2luZngvMjAwNi94YW1sL3ByZXNlbnRhdGlvbiINCiAgeG1sbnM6eD0iaHR0cDovL3NjaGVtYXMubWljcm9zb2Z0LmNvbS93aW5meC8yMDA2L3hhbWwiDQogIHhtbG5zOlN5c3RlbT0iY2xyLW5hbWVzcGFjZTpTeXN0ZW07YXNzZW1ibHk9bXNjb3JsaWIiDQogIHhtbG5zOkRpYWc9ImNsci1uYW1lc3BhY2U6U3lzdGVtLkRpYWdub3N0aWNzO2Fzc2VtYmx5PXN5c3RlbSImZ3Q7DQoJICZsdDtPYmplY3REYXRhUHJvdmlkZXIgeDpLZXk9IkxhdW5jaENhbGMiIE9iamVjdFR5cGUgPSAieyB4OlR5cGUgRGlhZzpQcm9jZXNzfSIgTWV0aG9kTmFtZSA9ICJTdGFydCIgJmd0Ow0KICAgICAmbHQ7T2JqZWN0RGF0YVByb3ZpZGVyLk1ldGhvZFBhcmFtZXRlcnMmZ3Q7DQogICAgICAgICZsdDtTeXN0ZW06U3RyaW5nJmd0O2NtZCZsdDsvU3lzdGVtOlN0cmluZyZndDsNCiAgICAgICAgJmx0O1N5c3RlbTpTdHJpbmcmZ3Q7L2MgImNhbGMiICZsdDsvU3lzdGVtOlN0cmluZyZndDsNCiAgICAgJmx0Oy9PYmplY3REYXRhUHJvdmlkZXIuTWV0aG9kUGFyYW1ldGVycyZndDsNCiAgICAmbHQ7L09iamVjdERhdGFQcm92aWRlciZndDsNCiZsdDsvUmVzb3VyY2VEaWN0aW9uYXJ5Jmd0Ow0KCQkJPC9TPiYjeEQ7DQogICAgICAgICAgPC9MU1Q+JiN4RDsNCiAgICAgICAgPC9PYmo+JiN4RDsNCiAgICAgIDwvUHJvcHM+JiN4RDsNCiAgICAgIDxNUz4mI3hEOw0KICAgICAgICA8T2JqIE49Il9fQ2xhc3NNZXRhZGF0YSIgUmVmSWQgPSIyIj4gJiN4RDsNCiAgICAgICAgICA8VE4gUmVmSWQ9IjEiID4gJiN4RDsNCiAgICAgICAgICAgIDxUPlN5c3RlbS5Db2xsZWN0aW9ucy5BcnJheUxpc3Q8L1Q+JiN4RDsNCiAgICAgICAgICAgIDxUPlN5c3RlbS5PYmplY3Q8L1Q+JiN4RDsNCiAgICAgICAgICA8L1ROPiYjeEQ7DQogICAgICAgICAgPExTVD4mI3hEOw0KICAgICAgICAgICAgPE9iaiBSZWZJZD0iMyI+ICYjeEQ7DQogICAgICAgICAgICAgIDxNUz4mI3hEOw0KICAgICAgICAgICAgICAgIDxTIE49IkNsYXNzTmFtZSI+UnVuc3BhY2VJbnZva2U1PC9TPiYjeEQ7DQogICAgICAgICAgICAgICAgPFMgTj0iTmFtZXNwYWNlIj5TeXN0ZW0uTWFuYWdlbWVudC5BdXRvbWF0aW9uPC9TPiYjeEQ7DQogICAgICAgICAgICAgICAgPE5pbCBOPSJTZXJ2ZXJOYW1lIiAvPiYjeEQ7DQogICAgICAgICAgICAgICAgPEkzMiBOPSJIYXNoIj40NjA5MjkxOTI8L0kzMj4mI3hEOw0KICAgICAgICAgICAgICAgIDxTIE49Ik1pWG1sIj4gJmx0O0NMQVNTIE5BTUU9IlJ1bnNwYWNlSW52b2tlNSIgJmd0OyZsdDtQUk9QRVJUWSBOQU1FPSJ0ZXN0MSIgVFlQRSA9InN0cmluZyIgJmd0OyZsdDsvUFJPUEVSVFkmZ3Q7Jmx0Oy9DTEFTUyZndDs8L1M+JiN4RDsNCiAgICAgICAgICAgICAgPC9NUz4mI3hEOw0KICAgICAgICAgICAgPC9PYmo+JiN4RDsNCiAgICAgICAgICA8L0xTVD4mI3hEOw0KICAgICAgICA8L09iaj4mI3hEOw0KICAgICAgPC9NUz4mI3hEOw0KICAgIDwvT2JqPiYjeEQ7DQogICAgPE1TPiYjeEQ7DQogICAgICA8UmVmIE49Il9fQ2xhc3NNZXRhZGF0YSIgUmVmSWQgPSIyIiAvPiYjeEQ7DQogICAgPC9NUz4mI3hEOw0KICA8L09iaj4mI3hEOw0KPC9PYmpzPgs=");
         }
 
-        [TestMethod, Ignore] // fails
+        [TestMethod] // fails
         public void ActivitySurrogateTest()
         {
             DeserializeMaliciousPayload("ysoserial.exe -o base64 -g ActivitySurrogateSelector -f BinaryFormatter -c calc",

diff --git a/SafeDeserializationHelpers/BinaryFormatterExtensions.cs b/SafeDeserializationHelpers/BinaryFormatterExtensions.cs
@@ -0,0 +1,32 @@
+namespace SafeDeserializationHelpers
+{
+    using System.Data;
+    using System.Runtime.Serialization;
+    using System.Runtime.Serialization.Formatters.Binary;
+
+    /// <summary>
+    /// Extension methods for the
+    /// </summary>
+    public static class BinaryFormatterExtensions
+    {
+        /// <summary>
+        /// Makes the <see cref="BinaryFormatter"/> safe.
+        /// </summary>
+        /// <param name="fmt">The <see cref="BinaryFormatter"/> to guard.</param>
+        /// <returns>The safe version of the <see cref="BinaryFormatter"/>.</returns>
+        public static BinaryFormatter Safe(this BinaryFormatter fmt)
+        {
+            // safe type binder prevents delegate deserialization attacks
+            var binder = new SafeSerializationBinder(fmt.Binder);
+            fmt.Binder = binder;
+
+            // DataSet surrogate validates binary-serialized datasets
+            var ss = new SurrogateSelector();
+            ss.AddSurrogate(typeof(DataSet), new StreamingContext(StreamingContextStates.All), new DataSetSurrogate());
+            fmt.SurrogateSelector = ss;
+
+            // TODO: do we need to chain surrogate selectors?
+            return fmt;
+        }
+    }
+}
diff --git a/SafeDeserializationHelpers/CustomDataSetDeserializer.cs b/SafeDeserializationHelpers/CustomDataSetDeserializer.cs
diff --git a/SafeDeserializationHelpers/DataSetSurrogate.cs b/SafeDeserializationHelpers/DataSetSurrogate.cs
@@ -0,0 +1,89 @@
+namespace SafeDeserializationHelpers
+{
+    using System.Data;
+    using System.IO;
+    using System.Reflection;
+    using System.Runtime.Serialization;
+    using System.Runtime.Serialization.Formatters.Binary;
+    using System.Security.Permissions;
+
+    /// <summary>
+    /// Deserialization surrogate for the DataSet class.
+    /// </summary>
+    public class DataSetSurrogate : ISerializationSurrogate
+    {
+        private static ConstructorInfo Constructor { get; } = typeof(DataSet).GetConstructor(
+            BindingFlags.Instance | BindingFlags.NonPublic,
+            null,
+            new[] { typeof(SerializationInfo), typeof(StreamingContext) },
+            null);
+
+        /// <inheritdoc cref="ISerializationSurrogate" />
+        [SecurityPermission(SecurityAction.LinkDemand, Flags = SecurityPermissionFlag.SerializationFormatter)]
+        public void GetObjectData(object obj, SerializationInfo info, StreamingContext context)
+        {
+            var ds = obj as DataSet;
+            ds.GetObjectData(info, context);
+        }
+
+        /// <inheritdoc cref="ISerializationSurrogate" />
+        [SecurityPermission(SecurityAction.LinkDemand, Flags = SecurityPermissionFlag.SerializationFormatter)]
+        public object SetObjectData(object obj, SerializationInfo info, StreamingContext context, ISurrogateSelector selector)
+        {
+            Validate(info, context);
+
+            // discard obj
+            var ds = Constructor.Invoke(new object[] { info, context });
+            return ds;
+        }
+
+        private void Validate(SerializationInfo info, StreamingContext context)
+        {
+            var remotingFormat = SerializationFormat.Xml;
+            var schemaSerializationMode = SchemaSerializationMode.IncludeSchema;
+
+            var e = info.GetEnumerator();
+            while (e.MoveNext())
+            {
+                switch (e.Name)
+                {
+                    case "DataSet.RemotingFormat": // DataSet.RemotingFormat does not exist in V1/V1.1 versions
+                        remotingFormat = (SerializationFormat)e.Value;
+                        break;
+
+                    case "SchemaSerializationMode.DataSet": // SchemaSerializationMode.DataSet does not exist in V1/V1.1 versions
+                        schemaSerializationMode = (SchemaSerializationMode)e.Value;
+                        break;
+                }
+            }
+
+            // XML dataset serialization isn't known to be vulnerable
+            if (remotingFormat == SerializationFormat.Xml)
+            {
+                return;
+            }
+
+            // binary dataset serialization should be double-checked
+            var tableCount = info.GetInt32("DataSet.Tables.Count");
+            for (int i = 0; i < tableCount; i++)
+            {
+                var key = $"DataSet.Tables_{i}";
+                var buffer = info.GetValue(key, typeof(byte[])) as byte[];
+
+                // check the serialized data table using a guarded BinaryFormatter
+                var fmt = new BinaryFormatter(null, new StreamingContext(context.State, false)).Safe();
+                using (var ms = new MemoryStream(buffer))
+                {
+                    var dt = fmt.Deserialize(ms);
+                    if (dt is DataTable)
+                    {
+                        continue;
+                    }
+
+                    // the deserialized data doesn't appear to be a data table
+                    throw new UnsafeDeserializationException("Serialized DataSet probably includes malicious data.");
+                }
+            }
+        }
+    }
+}
diff --git a/SafeDeserializationHelpers/SafeDeserializationHelpers.csproj b/SafeDeserializationHelpers/SafeDeserializationHelpers.csproj
@@ -42,10 +42,9 @@
     <Reference Include="System.Xml" />
   </ItemGroup>
   <ItemGroup>
-    <Compile Include="CustomDataSetDeserializer.cs">
-      <SubType>Component</SubType>
-    </Compile>
+    <Compile Include="BinaryFormatterExtensions.cs" />
     <Compile Include="CustomDelegateSerializationHolder.cs" />
+    <Compile Include="DataSetSurrogate.cs" />
     <Compile Include="DelegateValidator.cs" />
     <Compile Include="GlobalSuppressions.cs" />
     <Compile Include="IDelegateValidator.cs" />

diff --git a/SafeDeserializationHelpers/SafeSerializationBinder.cs b/SafeDeserializationHelpers/SafeSerializationBinder.cs
@@ -47,13 +47,6 @@ public override Type BindToType(string assemblyName, string typeName)
                 return typeof(CustomDelegateSerializationHolder);
             }
 
-            ////// prevent DataSet-based deserialization attack
-            ////if (typeName == DataSetTypeName &&
-            ////    assemblyName.StartsWith(SystemDataAssemblyName, StringComparison.InvariantCultureIgnoreCase))
-            ////{
-            ////    return typeof(CustomDataSetDeserializer);
-            ////}
-
             // suppress known blacklisted types
             TypeNameValidator.Default.ValidateTypeName(assemblyName, typeName);
 

diff --git a/SafeDeserializationHelpers/TypeNameValidator.cs b/SafeDeserializationHelpers/TypeNameValidator.cs
@@ -16,6 +16,7 @@ public class TypeNameValidator : ITypeNameValidator
         private static readonly string[] DefaultBlacklistedTypes = new[]
         {
             "System.Management.Automation.PSObject, System.Management.Automation, Version=3.0.0.0",
+            "System.Workflow.ComponentModel.Serialization.ActivitySurrogateSelector+ObjectSurrogate+ObjectSerializedRef, System.Workflow.ComponentModel, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35",
         };
 
         /// <summary>