Fixed xml spoofing bug for using DataContractSerializer to deserialize messages.

Added tests to verify correct behavior.

11 files changed, 291 insertions, 16 deletions

diff --git a/src/DotNetOAuth.Test/DotNetOAuth.Test.csproj b/src/DotNetOAuth.Test/DotNetOAuth.Test.csproj
index e734d18..41af758 100644
--- a/src/DotNetOAuth.Test/DotNetOAuth.Test.csproj
+++ b/src/DotNetOAuth.Test/DotNetOAuth.Test.csproj
@@ -63,6 +63,8 @@
     <Compile Include="Messaging\DictionaryXmlReaderTests.cs" />

     <Compile Include="Messaging\HttpRequestInfoTests.cs" />

     <Compile Include="Messaging\ProtocolExceptionTests.cs" />

+    <Compile Include="Mocks\TestBaseMessage.cs" />

+    <Compile Include="Mocks\TestDerivedMessage.cs" />

     <Compile Include="Mocks\TestDirectedMessage.cs" />

     <Compile Include="Mocks\TestBadChannel.cs" />

     <Compile Include="Mocks\TestWebRequestHandler.cs" />

diff --git a/src/DotNetOAuth.Test/Messaging/DictionaryXmlReaderTests.cs b/src/DotNetOAuth.Test/Messaging/DictionaryXmlReaderTests.cs
index 3dc3238..54c47f0 100644
--- a/src/DotNetOAuth.Test/Messaging/DictionaryXmlReaderTests.cs
+++ b/src/DotNetOAuth.Test/Messaging/DictionaryXmlReaderTests.cs
@@ -15,12 +15,19 @@ namespace DotNetOAuth.Test.Messaging {
 	public class DictionaryXmlReaderTests : TestBase {

 		[TestMethod, ExpectedException(typeof(ArgumentNullException))]

 		public void CreateWithNullRootElement() {

-			DictionaryXmlReader.Create(null, new Dictionary<string, string>());

+			IComparer<string> fieldSorter = new DataContractMemberComparer(typeof(Mocks.TestMessage));

+			DictionaryXmlReader.Create(null, fieldSorter, new Dictionary<string, string>());

+		}

+

+		[TestMethod, ExpectedException(typeof(ArgumentNullException))]

+		public void CreateWithNullDataContractType() {

+			DictionaryXmlReader.Create(XName.Get("name", "ns"), null, new Dictionary<string, string>());

 		}

 

 		[TestMethod, ExpectedException(typeof(ArgumentNullException))]

 		public void CreateWithNullFields() {

-			DictionaryXmlReader.Create(XName.Get("name", "ns"), null);

+			IComparer<string> fieldSorter = new DataContractMemberComparer(typeof(Mocks.TestMessage));

+			DictionaryXmlReader.Create(XName.Get("name", "ns"), fieldSorter, null);

 		}

 	}

 }

diff --git a/src/DotNetOAuth.Test/Messaging/MessageSerializerTests.cs b/src/DotNetOAuth.Test/Messaging/MessageSerializerTests.cs
index 5cb5375..f7409aa 100644
--- a/src/DotNetOAuth.Test/Messaging/MessageSerializerTests.cs
+++ b/src/DotNetOAuth.Test/Messaging/MessageSerializerTests.cs
@@ -94,20 +94,49 @@ namespace DotNetOAuth.Test.Messaging {
 			serializer.Deserialize(null);

 		}

 

-		[TestMethod()]

+		[TestMethod]

 		public void DeserializeSimple() {

 			var serializer = MessageSerializer.Get(typeof(Mocks.TestMessage));

 			Dictionary<string, string> fields = new Dictionary<string, string>(StringComparer.Ordinal);

-			// We deliberately do this OUT of alphabetical order (caps would go first),

-			// since DataContractSerializer demands things to be IN alphabetical order.

-			fields["age"] = "15";

 			fields["Name"] = "Andrew";

+			fields["age"] = "15";

 			var actual = (Mocks.TestMessage)serializer.Deserialize(fields);

 			Assert.AreEqual(15, actual.Age);

 			Assert.AreEqual("Andrew", actual.Name);

 			Assert.IsNull(actual.EmptyMember);

 		}

 

+		/// <summary>

+		/// This tests deserialization of a message that is comprised of [DataMember]'s

+		/// that are defined in multiple places in the inheritance tree.

+		/// </summary>

+		/// <remarks>

+		/// The element sorting rules are first inheritance order, then alphabetical order.

+		/// This test validates correct behavior on both.

+		/// </remarks>

+		[TestMethod]

+		public void DeserializeVerifyElementOrdering() {

+			var serializer = MessageSerializer.Get(typeof(Mocks.TestDerivedMessage));

+			Dictionary<string, string> fields = new Dictionary<string, string>(StringComparer.Ordinal);

+			// We deliberately do this OUT of order,

+			// since DataContractSerializer demands elements to be in 

+			// 1) inheritance then 2) alphabetical order.

+			// Proper xml element order would be: Name, age, Second..., TheFirst...

+			fields["TheFirstDerivedElement"] = "first";

+			fields["age"] = "15";

+			fields["Name"] = "Andrew";

+			fields["SecondDerivedElement"] = "second";

+			fields["explicit"] = "explicitValue";

+			fields["private"] = "privateValue";

+			var actual = (Mocks.TestDerivedMessage)serializer.Deserialize(fields);

+			Assert.AreEqual(15, actual.Age);

+			Assert.AreEqual("Andrew", actual.Name);

+			Assert.AreEqual("first", actual.TheFirstDerivedElement);

+			Assert.AreEqual("second", actual.SecondDerivedElement);

+			Assert.AreEqual("explicitValue", ((Mocks.IBaseMessageExplicitMembers)actual).ExplicitProperty);

+			Assert.AreEqual("privateValue", actual.PrivatePropertyAccessor);

+		}

+

 		[TestMethod]

 		public void DeserializeWithExtraFields() {

 			var serializer = MessageSerializer.Get(typeof(Mocks.TestMessage));

diff --git a/src/DotNetOAuth.Test/Mocks/TestBaseMessage.cs b/src/DotNetOAuth.Test/Mocks/TestBaseMessage.cs
new file mode 100644
index 0000000..7841357
--- /dev/null
+++ b/src/DotNetOAuth.Test/Mocks/TestBaseMessage.cs
@@ -0,0 +1,44 @@
+//-----------------------------------------------------------------------

+// <copyright file="TestBaseMessage.cs" company="Andrew Arnott">

+//     Copyright (c) Andrew Arnott. All rights reserved.

+// </copyright>

+//-----------------------------------------------------------------------

+

+namespace DotNetOAuth.Test.Mocks {

+	using System.Runtime.Serialization;

+	using DotNetOAuth.Messaging;

+

+	internal interface IBaseMessageExplicitMembers {

+		string ExplicitProperty { get; set; }

+	}

+

+	[DataContract(Namespace = Protocol.DataContractNamespaceV10)]

+	internal class TestBaseMessage : IProtocolMessage, IBaseMessageExplicitMembers {

+		[DataMember(Name = "age", IsRequired = true)]

+		public int Age { get; set; }

+

+		[DataMember]

+		public string Name { get; set; }

+

+		[DataMember(Name = "explicit")]

+		string IBaseMessageExplicitMembers.ExplicitProperty { get; set; }

+

+		Protocol IProtocolMessage.Protocol {

+			get { return Protocol.V10; }

+		}

+

+		MessageTransport IProtocolMessage.Transport {

+			get { return MessageTransport.Indirect; }

+		}

+

+		internal string PrivatePropertyAccessor {

+			get { return this.PrivateProperty; }

+			set { this.PrivateProperty = value; }

+		}

+

+		[DataMember(Name = "private")]

+		private string PrivateProperty { get; set; }

+

+		void IProtocolMessage.EnsureValidMessage() { }

+	}

+}

diff --git a/src/DotNetOAuth.Test/Mocks/TestDerivedMessage.cs b/src/DotNetOAuth.Test/Mocks/TestDerivedMessage.cs
new file mode 100644
index 0000000..afd67f6
--- /dev/null
+++ b/src/DotNetOAuth.Test/Mocks/TestDerivedMessage.cs
@@ -0,0 +1,33 @@
+//-----------------------------------------------------------------------

+// <copyright file="TestDerivedMessage.cs" company="Andrew Arnott">

+//     Copyright (c) Andrew Arnott. All rights reserved.

+// </copyright>

+//-----------------------------------------------------------------------

+

+namespace DotNetOAuth.Test.Mocks {

+	using System.Runtime.Serialization;

+

+	[DataContract(Namespace = Protocol.DataContractNamespaceV10)]

+	internal class TestDerivedMessage : TestBaseMessage {

+		/// <summary>

+		/// Gets or sets the first value.

+		/// </summary>

+		/// <remarks>

+		/// This element should appear AFTER <see cref="SecondDerivedElement"/>

+		/// due to alphabetical ordering rules, but after all the elements in the

+		/// base class due to inheritance rules.

+		/// </remarks>

+		[DataMember]

+		public string TheFirstDerivedElement { get; set; }

+

+		/// <summary>

+		/// Gets or sets the second value.

+		/// </summary>

+		/// <remarks>

+		/// This element should appear BEFORE <see cref="TheFirstDerivedElement"/>,

+		/// but after all the elements in the base class.

+		/// </remarks>

+		[DataMember]

+		public string SecondDerivedElement { get; set; }

+	}

+}

diff --git a/src/DotNetOAuth/DotNetOAuth.csproj b/src/DotNetOAuth/DotNetOAuth.csproj
index dfe95b7..41587db 100644
--- a/src/DotNetOAuth/DotNetOAuth.csproj
+++ b/src/DotNetOAuth/DotNetOAuth.csproj
@@ -68,6 +68,7 @@
   <ItemGroup>

     <Compile Include="Consumer.cs" />

     <Compile Include="IWebRequestHandler.cs" />

+    <Compile Include="Messaging\DataContractMemberComparer.cs" />

     <Compile Include="Messaging\IReplayProtectedProtocolMessage.cs" />

     <Compile Include="Messaging\IExpiringProtocolMessage.cs" />

     <Compile Include="Messaging\DictionaryXmlReader.cs" />

diff --git a/src/DotNetOAuth/Messaging/DataContractMemberComparer.cs b/src/DotNetOAuth/Messaging/DataContractMemberComparer.cs
new file mode 100644
index 0000000..4061b57
--- /dev/null
+++ b/src/DotNetOAuth/Messaging/DataContractMemberComparer.cs
@@ -0,0 +1,103 @@
+//-----------------------------------------------------------------------

+// <copyright file="DataContractMemberComparer.cs" company="Andrew Arnott">

+//     Copyright (c) Andrew Arnott. All rights reserved.

+// </copyright>

+//-----------------------------------------------------------------------

+

+namespace DotNetOAuth.Messaging {

+	using System;

+	using System.Collections.Generic;

+	using System.Diagnostics;

+	using System.IO;

+	using System.Linq;

+	using System.Reflection;

+	using System.Runtime.Serialization;

+	using System.Xml;

+	using System.Xml.Linq;

+

+	/// <summary>

+	/// A sorting tool to arrange fields in an order expected by the <see cref="DataContractSerializer"/>.

+	/// </summary>

+	internal class DataContractMemberComparer : IComparer<string> {

+		/// <summary>

+		/// The cached calculated inheritance ranking of every [DataMember] member of a type.

+		/// </summary>

+		private Dictionary<string, int> ranking;

+

+		/// <summary>

+		/// Initializes a new instance of the <see cref="DataContractMemberComparer"/> class.

+		/// </summary>

+		/// <param name="dataContractType">The data contract type that will be deserialized to.</param>

+		internal DataContractMemberComparer(Type dataContractType) {

+			// The elements must be serialized in inheritance rank and alphabetical order 

+			// so the DataContractSerializer will see them.

+			this.ranking = GetDataMemberInheritanceRanking(dataContractType);

+		}

+

+		#region IComparer<string> Members

+

+		/// <summary>

+		/// Compares to fields and decides what order they should appear in.

+		/// </summary>

+		/// <param name="field1">The first field.</param>

+		/// <param name="field2">The second field.</param>

+		/// <returns>-1 if the first field should appear first, 0 if it doesn't matter, 1 if it should appear last.</returns>

+		public int Compare(string field1, string field2) {

+			int rank1, rank2;

+			bool field1Valid = this.ranking.TryGetValue(field1, out rank1);

+			bool field2Valid = this.ranking.TryGetValue(field2, out rank2);

+

+			// If both fields are invalid, we don't care about the order.

+			if (!field1Valid && !field2Valid) {

+				return 0;

+			}

+

+			// If exactly one is valid, put that one first.

+			if (field1Valid ^ field2Valid) {

+				return field1Valid ? -1 : 1;

+			}

+

+			// First compare their inheritance ranking.

+			if (rank1 != rank2) {

+				// We want DESCENDING rank order, putting the members defined in the most

+				// base class first.

+				return -rank1.CompareTo(rank2);

+			}

+

+			// Finally sort alphabetically with case sensitivity.

+			return string.CompareOrdinal(field1, field2);

+		}

+

+		#endregion

+

+		/// <summary>

+		/// Generates a dictionary of field name and inheritance rankings for a given DataContract type.

+		/// </summary>

+		/// <param name="type">The type to generate member rankings for.</param>

+		/// <returns>The generated dictionary.</returns>

+		private static Dictionary<string, int> GetDataMemberInheritanceRanking(Type type) {

+			Debug.Assert(type != null, "type == null");

+			var ranking = new Dictionary<string, int>();

+

+			// TODO: review partial trust scenarios and this NonPublic flag.

+			Type currentType = type;

+			int rank = 0;

+			do {

+				foreach (MemberInfo member in currentType.GetMembers(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly)) {

+					if (member is PropertyInfo || member is FieldInfo) {

+						DataMemberAttribute dataMemberAttribute = member.GetCustomAttributes(typeof(DataMemberAttribute), true).OfType<DataMemberAttribute>().FirstOrDefault();

+						if (dataMemberAttribute != null) {

+							string name = dataMemberAttribute.Name ?? member.Name;

+							ranking.Add(name, rank);

+						}

+					}

+				}

+

+				rank++;

+				currentType = currentType.BaseType;

+			} while (currentType != null);

+

+			return ranking;

+		}

+	}

+}

diff --git a/src/DotNetOAuth/Messaging/DictionaryXmlReader.cs b/src/DotNetOAuth/Messaging/DictionaryXmlReader.cs
index 6ecc41e..11d553b 100644
--- a/src/DotNetOAuth/Messaging/DictionaryXmlReader.cs
+++ b/src/DotNetOAuth/Messaging/DictionaryXmlReader.cs
@@ -10,6 +10,8 @@ namespace DotNetOAuth.Messaging {
 	using System.Diagnostics;

 	using System.IO;

 	using System.Linq;

+	using System.Reflection;

+	using System.Runtime.Serialization;

 	using System.Xml;

 	using System.Xml.Linq;

 

@@ -21,32 +23,38 @@ namespace DotNetOAuth.Messaging {
 		/// Creates an XmlReader that reads data out of a dictionary instead of XML.

 		/// </summary>

 		/// <param name="rootElement">The name of the root XML element.</param>

+		/// <param name="fieldSorter">The field sorter so that the XmlReader generates xml elements in the order required by the <see cref="DataContractSerializer"/>.</param>

 		/// <param name="fields">The dictionary to read data from.</param>

 		/// <returns>The XmlReader that will read the data out of the given dictionary.</returns>

-		internal static XmlReader Create(XName rootElement, IDictionary<string, string> fields) {

+		internal static XmlReader Create(XName rootElement, IComparer<string> fieldSorter, IDictionary<string, string> fields) {

 			if (rootElement == null) {

 				throw new ArgumentNullException("rootElement");

 			}

+			if (fieldSorter == null) {

+				throw new ArgumentNullException("fieldSorter");

+			}

 			if (fields == null) {

 				throw new ArgumentNullException("fields");

 			}

 

-			return CreateRoundtripReader(rootElement, fields);

+			return CreateRoundtripReader(rootElement, fieldSorter, fields);

 		}

 

 		/// <summary>

 		/// Creates an <see cref="XmlReader"/> that will read values out of a dictionary.

 		/// </summary>

 		/// <param name="rootElement">The surrounding root XML element to generate.</param>

+		/// <param name="fieldSorter">The field sorter so that the XmlReader generates xml elements in the order required by the <see cref="DataContractSerializer"/>.</param>

 		/// <param name="fields">The dictionary to list values from.</param>

 		/// <returns>The generated <see cref="XmlReader"/>.</returns>

-		private static XmlReader CreateRoundtripReader(XName rootElement, IDictionary<string, string> fields) {

+		private static XmlReader CreateRoundtripReader(XName rootElement, IComparer<string> fieldSorter, IDictionary<string, string> fields) {

 			Debug.Assert(rootElement != null, "rootElement == null");

+			Debug.Assert(fieldSorter != null, "fieldSorter == null");

 			Debug.Assert(fields != null, "fields == null");

 

 			MemoryStream stream = new MemoryStream();

 			XmlWriter writer = XmlWriter.Create(stream);

-			SerializeDictionaryToXml(writer, rootElement, fields);

+			SerializeDictionaryToXml(writer, rootElement, fieldSorter, fields);

 			writer.Flush();

 			stream.Seek(0, SeekOrigin.Begin);

 

@@ -63,16 +71,16 @@ namespace DotNetOAuth.Messaging {
 		/// </summary>

 		/// <param name="writer">The <see cref="XmlWriter"/> to write out the XML to.</param>

 		/// <param name="rootElement">The name of the root element to use to surround the dictionary values.</param>

+		/// <param name="fieldSorter">The field sorter so that the XmlReader generates xml elements in the order required by the <see cref="DataContractSerializer"/>.</param>

 		/// <param name="fields">The dictionary with values to serialize.</param>

-		private static void SerializeDictionaryToXml(XmlWriter writer, XName rootElement, IDictionary<string, string> fields) {

+		private static void SerializeDictionaryToXml(XmlWriter writer, XName rootElement, IComparer<string> fieldSorter, IDictionary<string, string> fields) {

 			Debug.Assert(writer != null, "writer == null");

 			Debug.Assert(rootElement != null, "rootElement == null");

 			Debug.Assert(fields != null, "fields == null");

 

 			writer.WriteStartElement(rootElement.LocalName, rootElement.NamespaceName);

 

-			// The elements must be serialized in alphabetical order so the DataContractSerializer will see them.

-			foreach (var pair in fields.OrderBy(pair => pair.Key, StringComparer.Ordinal)) {

+			foreach (var pair in fields.OrderBy(pair => pair.Key, fieldSorter)) {

 				writer.WriteStartElement(pair.Key, rootElement.NamespaceName);

 				writer.WriteValue(pair.Value);

 				writer.WriteEndElement();

diff --git a/src/DotNetOAuth/Messaging/MessageSerializer.cs b/src/DotNetOAuth/Messaging/MessageSerializer.cs
index b09a253..ad3a12d 100644
--- a/src/DotNetOAuth/Messaging/MessageSerializer.cs
+++ b/src/DotNetOAuth/Messaging/MessageSerializer.cs
@@ -42,6 +42,11 @@ namespace DotNetOAuth.Messaging {
 		private XName rootElement;

 

 		/// <summary>

+		/// A field sorter that puts fields in the right order for the <see cref="DataContractSerializer"/>.

+		/// </summary>

+		private IComparer<string> fieldSorter;

+

+		/// <summary>

 		/// Initializes a new instance of the MessageSerializer class.

 		/// </summary>

 		/// <param name="messageType">The specific <see cref="IProtocolMessage"/>-derived type

@@ -62,6 +67,7 @@ namespace DotNetOAuth.Messaging {
 			this.messageType = messageType;

 			this.serializer = new DataContractSerializer(

 				messageType, this.RootElement.LocalName, this.RootElement.NamespaceName);

+			this.fieldSorter = new DataContractMemberComparer(messageType);

 		}

 

 		/// <summary>

@@ -70,7 +76,25 @@ namespace DotNetOAuth.Messaging {
 		private XName RootElement {

 			get {

 				if (this.rootElement == null) {

-					DataContractAttribute attribute = this.messageType.GetCustomAttributes(typeof(DataContractAttribute), false).OfType<DataContractAttribute>().Single();

+					DataContractAttribute attribute;

+					try {

+						attribute = this.messageType.GetCustomAttributes(typeof(DataContractAttribute), false).OfType<DataContractAttribute>().Single();

+					} catch (InvalidOperationException ex) {

+						throw new ProtocolException(

+							string.Format(

+								CultureInfo.CurrentCulture,

+								MessagingStrings.DataContractMissingFromMessageType,

+								this.messageType.FullName),

+							ex);

+					}

+

+					if (attribute.Namespace == null) {

+						throw new ProtocolException(string.Format(

+							CultureInfo.CurrentCulture,

+							MessagingStrings.DataContractMissingNamespace,

+							this.messageType.FullName));

+					}

+

 					this.rootElement = XName.Get("root", attribute.Namespace);

 				}

 

@@ -152,7 +176,7 @@ namespace DotNetOAuth.Messaging {
 				throw new ArgumentNullException("fields");

 			}

 

-			var reader = DictionaryXmlReader.Create(this.RootElement, fields);

+			var reader = DictionaryXmlReader.Create(this.RootElement, this.fieldSorter, fields);

 			IProtocolMessage result;

 			try {

 				result = (IProtocolMessage)this.serializer.ReadObject(reader, false);

diff --git a/src/DotNetOAuth/Messaging/MessagingStrings.Designer.cs b/src/DotNetOAuth/Messaging/MessagingStrings.Designer.cs
index 27d4775..2d3985a 100644
--- a/src/DotNetOAuth/Messaging/MessagingStrings.Designer.cs
+++ b/src/DotNetOAuth/Messaging/MessagingStrings.Designer.cs
@@ -61,6 +61,24 @@ namespace DotNetOAuth.Messaging {
         }

         

         /// <summary>

+        ///   Looks up a localized string similar to DataContractSerializer could not be initialized on message type {0}.  Is it missing a [DataContract] attribute?.

+        /// </summary>

+        internal static string DataContractMissingFromMessageType {

+            get {

+                return ResourceManager.GetString("DataContractMissingFromMessageType", resourceCulture);

+            }

+        }

+        

+        /// <summary>

+        ///   Looks up a localized string similar to DataContractSerializer could not be initialized on message type {0} because the DataContractAttribute.Namespace property is not set..

+        /// </summary>

+        internal static string DataContractMissingNamespace {

+            get {

+                return ResourceManager.GetString("DataContractMissingNamespace", resourceCulture);

+            }

+        }

+        

+        /// <summary>

         ///   Looks up a localized string similar to An instance of type {0} was expected, but received unexpected derived type {1}..

         /// </summary>

         internal static string DerivedTypeNotExpected {

diff --git a/src/DotNetOAuth/Messaging/MessagingStrings.resx b/src/DotNetOAuth/Messaging/MessagingStrings.resx
index b3024d4..be0b864 100644
--- a/src/DotNetOAuth/Messaging/MessagingStrings.resx
+++ b/src/DotNetOAuth/Messaging/MessagingStrings.resx
@@ -117,6 +117,12 @@
   <resheader name="writer">

     <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>

   </resheader>

+  <data name="DataContractMissingFromMessageType" xml:space="preserve">

+    <value>DataContractSerializer could not be initialized on message type {0}.  Is it missing a [DataContract] attribute?</value>

+  </data>

+  <data name="DataContractMissingNamespace" xml:space="preserve">

+    <value>DataContractSerializer could not be initialized on message type {0} because the DataContractAttribute.Namespace property is not set.</value>

+  </data>

   <data name="DerivedTypeNotExpected" xml:space="preserve">

     <value>An instance of type {0} was expected, but received unexpected derived type {1}.</value>

   </data>

@@ -159,4 +165,4 @@
   <data name="UnrecognizedEnumValue" xml:space="preserve">

     <value>{0} property has unrecognized value {1}.</value>

   </data>

-</root>
\ No newline at end of file
+</root>