Fixed xml spoofing bug for using DataContractSerializer to deserialize messages.

Added tests to verify correct behavior.

6 files changed, 170 insertions, 10 deletions

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>