Merge pull request #2426 from dotnet/dev/arpit/release30_1b

Description

Loose xaml can contain executable payload e.g. ObjectDataProvider. This Xaml can be included as part of XpsDocuments or base-64 encoded and then included in StickyNotes' annotation xml files.

In WPF, we were allowing XpsDocuments and StickyNotes' annotation xml files to be loaded freely via XamlReader.Load. 

This exposes an attack vector - when a user downloads an XPS file from the internet for viewing, they could end up executing untrusted code. 

The fix is to identify known dangerous types and limit them from being deserialized during XAML loading. 

In order to accomplish this, we add new non-public overloads to the XamlReader.Load method to enable the use of RestrictiveXamlXmlReader. RestrictiveXamlXmlReader restricts known dangerous types from being loaded while deserializing xaml. 

We then call XamlReader.Load via XamlReaderProxy, which is an adapter for XamlReader type and uses reflection to access XamlReader.Load. Reflection is used to avoid adding additional public surface area to XamlReader in servicing. 

Small changes are made to TextRange as well since the call-site for the StickyNotes case was through a call to TextRange which in turn calls into XamlReader.Load. 

Customer Impact

Customers would be protected from opening potentially-compromised XPS documents and stickynotes annotation xml files.

Regression

No. This security issue was reported by an external party.

Risk - Low
o	This change only affects loading XPS documents and loading stickynotes annotation data. 
o	The change has been tested well internally. 
o	We ran regression tests to ensure nothing else is inadvertently broken. 
o	Validated against POC to ensure that the fix works as intended. 

In .NET Framework, we are introducing a quirk to give developers/cusotmers the option of going back to the old (i.e., unsecure) behavior where deserializing dangerous types like ObjectDataProvider will be allowed. In .NET Core, no quirks are being provided - we do not believe that this is a scenario that should be supported for compatibility in a relatively new platform.

Author: arpitmathur

src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Controls/StickyNote/StickyNoteContentControl.cs

@@ -234,7 +234,7 @@ public override void Load(XmlNode node)
                 RichTextBox richTextBox = (RichTextBox)InnerControl;
 
                 FlowDocument document = new FlowDocument();
-                TextRange rtbRange = new TextRange(document.ContentStart, document.ContentEnd);
+                TextRange rtbRange = new TextRange(document.ContentStart, document.ContentEnd, useRestrictiveXamlXmlReader:true);
                 using (MemoryStream buffer = new MemoryStream(Convert.FromBase64String(node.InnerText)))
                 {
                     rtbRange.Load(buffer, DataFormats.Xaml);

src/Microsoft.DotNet.Wpf/src/PresentationFramework/PresentationFramework.csproj

@@ -66,6 +66,7 @@
     <Compile Include="$(WpfSharedDir)\MS\Utility\ItemMap.cs" />
     <Compile Include="$(WpfSharedDir)\System\Windows\Interop\OSVersionHelper.cs" />
     <Compile Include="$(WpfSharedDir)\System\Windows\Interop\OperatingSystemVersion.cs" />
+    <Compile Include="$(WpfSharedDir)\MS\Internal\Markup\XamlReaderProxy.cs " />
     <Compile Include="$(WpfSharedDir)\Telemetry\Managed\EventSourceActivity.cs" />
     <Compile Include="$(WpfSharedDir)\Telemetry\Managed\TelemetryEventSource.cs" />
     <Compile Include="$(WpfSharedDir)\Telemetry\Managed\TraceLoggingProvider.cs" />

src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextRange.cs

@@ -13,6 +13,7 @@
 using System.Xml;
 using System.IO;
 using System.Windows.Markup; // Parser
+using MS.Internal.PresentationFramework.Markup;
 
 #pragma warning disable 1634, 1691  // suppressing PreSharp warnings
 
@@ -87,6 +88,14 @@ internal TextRange(ITextPointer position1, ITextPointer position2, bool ignoreTe
             ValidationHelper.VerifyPosition(position1.TextContainer, position2, "position2");
 
             TextRangeBase.Select(this, position1, position2);
+        }
+
+        // useRestrictiveXamlXmlReader - false by default
+        // set to true to disable external xaml loading in specific scenarios like StickyNotes annotation loading
+        internal TextRange(TextPointer position1, TextPointer position2, bool useRestrictiveXamlXmlReader) :
+            this((ITextPointer)position1, (ITextPointer)position2)
+        {
+            _useRestrictiveXamlXmlReader = useRestrictiveXamlXmlReader;
         }
 
         #endregion Constructors   
@@ -1364,9 +1373,9 @@ internal string Xml
                 // so we use virtual mechanism for extensibility in TextSelection
                 TextRangeBase.BeginChange(this);
                 try
-                {
-                    // Parse the fragment into a separate subtree
-                    object xamlObject = XamlReader.Load(new XmlTextReader(new System.IO.StringReader(value)));
+                {
+                    // Parse the fragment into a separate subtree
+                    object xamlObject = XamlReaderProxy.Load(new XmlTextReader(new System.IO.StringReader(value)), _useRestrictiveXamlXmlReader);
                     TextElement fragment = xamlObject as TextElement;
 
                     if (fragment != null)
@@ -1900,6 +1909,9 @@ private enum Flags
         // Boolean flags, set with Flags enum.
         private Flags _flags;
 
+        //boolean flag, set to true via constructor when you want to use the restrictive xamlxmlreader  
+        private bool _useRestrictiveXamlXmlReader;
+
         #endregion Private Fields
     }
 }

src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsDocument.cs

@@ -31,8 +31,9 @@ reading and/or writing Xps packages.
 using System.Xml;
 using System.Security;
 using MS.Internal;
-using MS.Internal.Security;
 using MS.Internal.IO.Packaging;
+using MS.Internal.ReachFramework.Markup;
+using MS.Internal.Security;
 
 using MS.Internal.IO.Packaging.Extensions;
 using Package = System.IO.Packaging.Package;
@@ -629,7 +630,7 @@ XpsImageType imageType
 
             parserContext.BaseUri = PackUriHelper.Create(Uri, CurrentXpsManager.StartingPart.Uri);
 
-            object fixedObject = XamlReader.Load(CurrentXpsManager.StartingPart.GetStream(), parserContext);
+            object fixedObject = XamlReaderProxy.Load(CurrentXpsManager.StartingPart.GetStream(), parserContext, useRestrictiveXamlReader:true);
             if (!(fixedObject is FixedDocumentSequence) )
             {
                  throw new XpsPackagingException(SR.Get(SRID.ReachPackaging_NotAFixedDocumentSequence));

src/Microsoft.DotNet.Wpf/src/ReachFramework/ReachFramework.csproj

@@ -55,6 +55,7 @@
     <Compile Include="$(WpfSharedDir)\MS\Internal\SecurityHelper.cs" />
     <Compile Include="$(WpfSharedDir)\MS\Internal\UriComparer.cs " />
     <Compile Include="$(WpfSharedDir)\MS\Utility\BindUriHelper.cs" />
+    <Compile Include="$(WpfSharedDir)\MS\Internal\Markup\XamlReaderProxy.cs " />
     <Compile Include="SR.cs" />
     <Compile Include="Serialization\Manager\IXpsSerializationManager.cs" />
     <Compile Include="Serialization\Manager\IXpsSerializationManagerAsync.cs" />

src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/Markup/XamlReaderProxy.cs

@@ -0,0 +1,79 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+//
+// Description: This helper class reflects into internal overloads of XamlReader.Load to use the RestrictiveXamlXmlReader
+// to avoid loading unsafe loose xaml.
+//
+
+using System;
+using System.IO;
+using System.Xml;
+using System.Reflection;
+using System.Windows.Markup;
+
+#if REACHFRAMEWORK
+namespace MS.Internal.ReachFramework
+#elif PRESENTATIONFRAMEWORK
+namespace MS.Internal.PresentationFramework
+#else
+namespace MS.Internal
+#endif
+{
+    namespace Markup
+    {
+        /// <summary>
+        /// Provides a helper class to create delegates to reflect into XamlReader.Load
+        /// </summary>
+        internal class XamlReaderProxy
+        {
+            /// <summary>
+            /// The static constructor creates and stores delegates for overloads of <see cref="XamlReader.Load"/> that need to be reflected into
+            /// so that we can safeguard entry-points for external xaml loading.
+            /// </summary>
+            /// <remark> Doing this in the static constructor guarantees thread safety.</remark>
+
+            static XamlReaderProxy()
+            {
+                MethodInfo method = _xamlReaderType.GetMethod(XamlLoadMethodName, BindingFlags.NonPublic | BindingFlags.Static, null, new Type[] { typeof(Stream), typeof(ParserContext), typeof(bool) }, null);
+
+                if (method == null)
+                {
+                    throw new MissingMethodException(XamlLoadMethodName);
+                }
+
+                _xamlLoad3 = (XamlLoadDelegate3)method.CreateDelegate(typeof(XamlLoadDelegate3));
+
+                method = _xamlReaderType.GetMethod(XamlLoadMethodName, BindingFlags.NonPublic | BindingFlags.Static, null, new Type[] { typeof(XmlReader), typeof(bool) }, null);
+
+                if (method == null)
+                {
+                    throw new MissingMethodException(XamlLoadMethodName);
+                }
+
+                _xamlLoad2 = (XamlLoadDelegate2)method.CreateDelegate(typeof(XamlLoadDelegate2));
+            }
+    
+            public static object Load(Stream stream, ParserContext parserContext, bool useRestrictiveXamlReader)
+            {
+                return _xamlLoad3.Invoke(stream, parserContext, useRestrictiveXamlReader);
+            }
+
+            public static object Load(XmlReader reader, bool useRestrictiveXamlReader)
+            {
+                return _xamlLoad2.Invoke(reader, useRestrictiveXamlReader);
+            }
+
+            private delegate object XamlLoadDelegate3(Stream stream, ParserContext parserContext, bool useRestrictiveXamlReader);
+            private static XamlLoadDelegate3 _xamlLoad3;
+
+            private delegate object XamlLoadDelegate2(XmlReader reader, bool useRestrictiveXamlReader);
+            private static XamlLoadDelegate2 _xamlLoad2;
+
+            private static readonly Type _xamlReaderType = typeof(XamlReader);
+            private const string XamlLoadMethodName = nameof(Load);
+
+        }
+    }
+}