From e26f6140ec66c4c9e14bc679a982e9fd2a1f6357 Mon Sep 17 00:00:00 2001 From: Arpit Mathur Date: Wed, 27 Nov 2019 00:34:01 +0000 Subject: [PATCH 1/2] Merged PR 4378: [.Net Core 3.1] MSRC 54179, 54120: Reflecting into internal overloads of XamlReader.Load to use RestrictiveXamlXmlReader Bugs: - Bug [1006083](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1006083): MSRC 54120: XAMLReader.Load used by `GetFixedDocumentSequence` method which could lead to code execution [.Net Core 3.1] - Bug [1006086](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1006086): MSRC 54179: Code Execution using Malicious Annotation Files for Sticky Notes in WPF apps [.Net Core 3.1] ###Description Loose xaml can contain executable payload e.g. `ObjectDataProvider`. This Xaml can be included as part of `XpsDocument`s or base-64 encoded and then included in `StickyNote`s' annotation xml files. In WPF, we were allowing `XpsDocument`s and `StickyNote`s' 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 `StickyNote`s 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 - This change only affects loading XPS documents and loading stickynotes annotation data. - The change has been tested well internally. - We ran regression tests to ensure nothing else is inadvertently broken. - 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. --- eng/pipeline.yml | 6 +- .../StickyNote/StickyNoteContentControl.cs | 2 +- .../PresentationFramework.csproj | 1 + .../System/Windows/Documents/TextRange.cs | 18 ++++- .../ReachFramework/Packaging/XpsDocument.cs | 5 +- .../src/ReachFramework/ReachFramework.csproj | 1 + .../MS/Internal/Markup/XamlReaderProxy.cs | 79 +++++++++++++++++++ 7 files changed, 103 insertions(+), 9 deletions(-) create mode 100644 src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/Markup/XamlReaderProxy.cs diff --git a/eng/pipeline.yml b/eng/pipeline.yml index 969fa588810..42980161fdb 100644 --- a/eng/pipeline.yml +++ b/eng/pipeline.yml @@ -24,13 +24,13 @@ jobs: - job: Windows_NT timeoutInMinutes: 120 # how long to run the job before automatically cancelling; see https://github.com/dotnet/wpf/issues/952 pool: - # For public or PR jobs, use the hosted pool. For internal jobs use the internal pool. + # For public jobs, use the hosted pool. For internal jobs use the internal pool. # Will eventually change this to two BYOC pools. # agent pool can't be read from a user-defined variable (Azure DevOps limitation) - ${{ if or(eq(variables['System.TeamProject'], 'public'), in(variables['Build.Reason'], 'PullRequest')) }}: + ${{ if eq(variables['System.TeamProject'], 'public') }}: name: NetCorePublic-Pool queue: BuildPool.Windows.10.Amd64.VS2019.Pre.Open - ${{ if and(ne(variables['System.TeamProject'], 'public'), notin(variables['Build.Reason'], 'PullRequest')) }}: + ${{ if eq(variables['System.TeamProject'], 'internal') }}: name: NetCoreInternal-Pool queue: buildpool.windows.10.amd64.vs2019.pre variables: diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Controls/StickyNote/StickyNoteContentControl.cs b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Controls/StickyNote/StickyNoteContentControl.cs index a1379838bec..e444e886468 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Controls/StickyNote/StickyNoteContentControl.cs +++ b/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); diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/PresentationFramework.csproj b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/PresentationFramework.csproj index 42007b1fc43..80d36db367c 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/PresentationFramework.csproj +++ b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/PresentationFramework.csproj @@ -66,6 +66,7 @@ + diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextRange.cs b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextRange.cs index 8336e7cbfa6..45108d6f3a0 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextRange.cs +++ b/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 } } diff --git a/src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsDocument.cs b/src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsDocument.cs index 12fe4b37535..5e7c76db608 100644 --- a/src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsDocument.cs +++ b/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)); diff --git a/src/Microsoft.DotNet.Wpf/src/ReachFramework/ReachFramework.csproj b/src/Microsoft.DotNet.Wpf/src/ReachFramework/ReachFramework.csproj index 09a0d212fc2..3d253ab85b1 100644 --- a/src/Microsoft.DotNet.Wpf/src/ReachFramework/ReachFramework.csproj +++ b/src/Microsoft.DotNet.Wpf/src/ReachFramework/ReachFramework.csproj @@ -55,6 +55,7 @@ + diff --git a/src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/Markup/XamlReaderProxy.cs b/src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/Markup/XamlReaderProxy.cs new file mode 100644 index 00000000000..8aed32b90e6 --- /dev/null +++ b/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 + { + /// + /// Provides a helper class to create delegates to reflect into XamlReader.Load + /// + internal class XamlReaderProxy + { + /// + /// The static constructor creates and stores delegates for overloads of that need to be reflected into + /// so that we can safeguard entry-points for external xaml loading. + /// + /// Doing this in the static constructor guarantees thread safety. + + 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); + + } + } +} From eeb2a5ee95617c171b8051632e6454235f8bee14 Mon Sep 17 00:00:00 2001 From: Robert LaDuca Date: Tue, 14 Jan 2020 15:47:11 -0800 Subject: [PATCH 2/2] Revert "Merge pull request #2427 from dotnet/dev/arpit/release31_1b" This reverts commit c86d4b9e7e16c3de7b65990f44a52bd630e33dca, reversing changes made to 334d0abf99e76a811bff4a8cf5db5a1e4f562b17. --- .../StickyNote/StickyNoteContentControl.cs | 2 +- .../PresentationFramework.csproj | 1 - .../System/Windows/Documents/TextRange.cs | 18 +---- .../ReachFramework/Packaging/XpsDocument.cs | 5 +- .../src/ReachFramework/ReachFramework.csproj | 1 - .../MS/Internal/Markup/XamlReaderProxy.cs | 79 ------------------- 6 files changed, 6 insertions(+), 100 deletions(-) delete mode 100644 src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/Markup/XamlReaderProxy.cs diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Controls/StickyNote/StickyNoteContentControl.cs b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Controls/StickyNote/StickyNoteContentControl.cs index e444e886468..a1379838bec 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Controls/StickyNote/StickyNoteContentControl.cs +++ b/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, useRestrictiveXamlXmlReader:true); + TextRange rtbRange = new TextRange(document.ContentStart, document.ContentEnd); using (MemoryStream buffer = new MemoryStream(Convert.FromBase64String(node.InnerText))) { rtbRange.Load(buffer, DataFormats.Xaml); diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/PresentationFramework.csproj b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/PresentationFramework.csproj index 80d36db367c..42007b1fc43 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/PresentationFramework.csproj +++ b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/PresentationFramework.csproj @@ -66,7 +66,6 @@ - diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextRange.cs b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextRange.cs index 45108d6f3a0..8336e7cbfa6 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextRange.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextRange.cs @@ -13,7 +13,6 @@ using System.Xml; using System.IO; using System.Windows.Markup; // Parser -using MS.Internal.PresentationFramework.Markup; #pragma warning disable 1634, 1691 // suppressing PreSharp warnings @@ -88,14 +87,6 @@ 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 @@ -1373,9 +1364,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 = XamlReaderProxy.Load(new XmlTextReader(new System.IO.StringReader(value)), _useRestrictiveXamlXmlReader); + { + // Parse the fragment into a separate subtree + object xamlObject = XamlReader.Load(new XmlTextReader(new System.IO.StringReader(value))); TextElement fragment = xamlObject as TextElement; if (fragment != null) @@ -1909,9 +1900,6 @@ 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 } } diff --git a/src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsDocument.cs b/src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsDocument.cs index 5e7c76db608..12fe4b37535 100644 --- a/src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsDocument.cs +++ b/src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsDocument.cs @@ -31,9 +31,8 @@ reading and/or writing Xps packages. using System.Xml; using System.Security; using MS.Internal; -using MS.Internal.IO.Packaging; -using MS.Internal.ReachFramework.Markup; using MS.Internal.Security; +using MS.Internal.IO.Packaging; using MS.Internal.IO.Packaging.Extensions; using Package = System.IO.Packaging.Package; @@ -630,7 +629,7 @@ XpsImageType imageType parserContext.BaseUri = PackUriHelper.Create(Uri, CurrentXpsManager.StartingPart.Uri); - object fixedObject = XamlReaderProxy.Load(CurrentXpsManager.StartingPart.GetStream(), parserContext, useRestrictiveXamlReader:true); + object fixedObject = XamlReader.Load(CurrentXpsManager.StartingPart.GetStream(), parserContext); if (!(fixedObject is FixedDocumentSequence) ) { throw new XpsPackagingException(SR.Get(SRID.ReachPackaging_NotAFixedDocumentSequence)); diff --git a/src/Microsoft.DotNet.Wpf/src/ReachFramework/ReachFramework.csproj b/src/Microsoft.DotNet.Wpf/src/ReachFramework/ReachFramework.csproj index 3d253ab85b1..09a0d212fc2 100644 --- a/src/Microsoft.DotNet.Wpf/src/ReachFramework/ReachFramework.csproj +++ b/src/Microsoft.DotNet.Wpf/src/ReachFramework/ReachFramework.csproj @@ -55,7 +55,6 @@ - diff --git a/src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/Markup/XamlReaderProxy.cs b/src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/Markup/XamlReaderProxy.cs deleted file mode 100644 index 8aed32b90e6..00000000000 --- a/src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/Markup/XamlReaderProxy.cs +++ /dev/null @@ -1,79 +0,0 @@ -// 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 - { - /// - /// Provides a helper class to create delegates to reflect into XamlReader.Load - /// - internal class XamlReaderProxy - { - /// - /// The static constructor creates and stores delegates for overloads of that need to be reflected into - /// so that we can safeguard entry-points for external xaml loading. - /// - /// Doing this in the static constructor guarantees thread safety. - - 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); - - } - } -}