Skip to content

Commit bad7fc1

Browse files
committed
Merged PR 6983: MSRC 54120: XAMLReader.Load used by GetFixedDocumentSequence method which could lead to code execution [.Net Core 3.1] - Missing variant fix
[Bug 1092072](https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1092072): MSRC 54120: XAMLReader.Load used by `GetFixedDocumentSequence` method which could lead to code execution [.Net Core 3.1] - Missing variant fix ## **Description** Loose xaml can contain executable payload e.g. `ObjectDataProvider`. This XAML can be included as part of `XpsDocument`s in their `FixedDocumentSequence` or `FixedPage`. In WPF, we were allowing `XpsDocument`s 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 `Type`s and limit them from being deserialized during XAML loading. In order to accomplish this, we previously added new non-public overloads to the `XamlReader.Load` method to enable the use of `RestrictiveXamlXmlReader`.`RestrictiveXamlXmlReader` restricts known dangerous `Type`s from being loaded while deserializing xaml. One of these dangerous `Type`s is `System.Windows.ResourceDictionary`, which is a valid `Type` in XML schema. To allow this valid `Type` we added another non-public overload to the `XamlReader.Load` method which takes an additional parameter, safeTypes, a list of `Type`s which can be loaded safely. We also added a new constructor to the `RestrictiveXamlXmlReader` which takes an additional parameter of safeTypes which are marked as safe to load for this instance of the `RestrictiveXamlXmlReader`. ### **Customer Impact** Customers would be protected from opening potentially-compromised XPS documents. ### **Regression** No. This security issue was reported by an external party. ### **Risk - Low** - This change only affects loading XPS documents. - 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.
1 parent 03203cc commit bad7fc1

File tree

3 files changed

+55
-13
lines changed

3 files changed

+55
-13
lines changed

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

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,17 @@ internal void Validate(Stream stream, Uri parentUri, ParserContext pc, ContentTy
5959
/// <returns></returns>
6060
private object Load(Stream stream, Uri parentUri, ParserContext pc, ContentType mimeType, string rootElement)
6161
{
62-
object obj = null;
62+
object obj = null;
63+
64+
List<Type> safeTypes = new List<Type> { typeof(System.Windows.ResourceDictionary) };
6365

6466
if (!DocumentMode)
6567
{ // Loose XAML, just check against schema, don't check content type
6668
if (rootElement==null)
67-
{
68-
obj = XamlReader.Load(stream, pc);
69+
{
70+
XmlReader reader = XmlReader.Create(stream, null, pc);
71+
obj = XamlReader.Load(reader, pc, XamlParseMode.Synchronous, true, safeTypes);
72+
stream.Close();
6973
}
7074
}
7175
else
@@ -148,10 +152,10 @@ private object Load(Stream stream, Uri parentUri, ParserContext pc, ContentType
148152
;
149153
}
150154
else
151-
{
152-
obj = XamlReader.Load(xpsSchemaValidator.XmlReader,
153-
pc,
154-
XamlParseMode.Synchronous);
155+
{
156+
obj = XamlReader.Load(xpsSchemaValidator.XmlReader,
157+
pc,
158+
XamlParseMode.Synchronous, true, safeTypes);
155159
}
156160
_validResources.Pop();
157161
}

src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Markup/RestrictiveXamlXmlReader.cs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,20 @@ static RestrictiveXamlXmlReader()
6161
/// </summary>
6262
public RestrictiveXamlXmlReader(XmlReader xmlReader, XamlSchemaContext schemaContext, XamlXmlReaderSettings settings) : base(xmlReader, schemaContext, settings)
6363
{
64+
}
65+
66+
/// <summary>
67+
/// Builds the restricted set based on RestrictedTypes that have already been loaded but adds the list of Types passed in in safeTypes to the instance of _safeTypesSet
68+
/// </summary>
69+
internal RestrictiveXamlXmlReader(XmlReader xmlReader, XamlSchemaContext schemaContext, XamlXmlReaderSettings settings, List<Type> safeTypes) : base(xmlReader, schemaContext, settings)
70+
{
71+
if (safeTypes != null)
72+
{
73+
foreach (Type safeType in safeTypes)
74+
{
75+
_safeTypesSet.Add(safeType);
76+
}
77+
}
6478
}
6579

6680
/// <summary>

src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Markup/XamlReader.cs

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@
1313
using System.IO.Packaging;
1414
using System.Windows;
1515
using System.ComponentModel;
16-
using System.Collections;
16+
using System.Collections;
17+
using System.Collections.Generic;
1718
using System.Diagnostics;
1819
using System.Reflection;
1920

@@ -744,10 +745,33 @@ internal static object Load(
744745
/// RestrictiveXamlXmlReader to restrict instantiation of potentially dangerous types</param>
745746
/// <returns>object root generated after xml parsed</returns>
746747
internal static object Load(
747-
XmlReader reader,
748-
ParserContext parserContext,
749-
XamlParseMode parseMode,
750-
bool useRestrictiveXamlReader)
748+
XmlReader reader,
749+
ParserContext parserContext,
750+
XamlParseMode parseMode,
751+
bool useRestrictiveXamlReader)
752+
{
753+
return Load(reader, parserContext, parseMode, useRestrictiveXamlReader, null);
754+
}
755+
756+
/// <summary>
757+
/// Reads XAML from the passed stream, building an object tree and returning the
758+
/// root of that tree. Wrap a CompatibilityReader with another XmlReader that
759+
/// uses the passed reader settings to allow validation of xaml.
760+
/// </summary>
761+
/// <param name="reader">XmlReader to use. This is NOT wrapped by any
762+
/// other reader</param>
763+
/// <param name="context">Optional parser context. May be null </param>
764+
/// <param name="parseMode">Sets synchronous or asynchronous parsing</param>
765+
/// <param name="useRestrictiveXamlReader">Whether or not this method should use
766+
/// RestrictiveXamlXmlReader to restrict instantiation of potentially dangerous types</param>
767+
/// <param name="safeTypes">List of known safe Types to be allowed through the RestrictiveXamlXmlReader</param>
768+
/// <returns>object root generated after xml parsed</returns>
769+
internal static object Load(
770+
XmlReader reader,
771+
ParserContext parserContext,
772+
XamlParseMode parseMode,
773+
bool useRestrictiveXamlReader,
774+
List<Type> safeTypes)
751775
{
752776
if (parseMode == XamlParseMode.Uninitialized ||
753777
parseMode == XamlParseMode.Asynchronous)
@@ -805,7 +829,7 @@ internal static object Load(
805829

806830
XamlSchemaContext schemaContext = parserContext.XamlTypeMapper != null ?
807831
parserContext.XamlTypeMapper.SchemaContext : GetWpfSchemaContext();
808-
System.Xaml.XamlXmlReader xamlXmlReader = (useRestrictiveXamlReader) ? new RestrictiveXamlXmlReader(reader, schemaContext, settings):
832+
System.Xaml.XamlXmlReader xamlXmlReader = (useRestrictiveXamlReader) ? new RestrictiveXamlXmlReader(reader, schemaContext, settings, safeTypes) :
809833
new System.Xaml.XamlXmlReader(reader, schemaContext, settings);
810834
root = Load(xamlXmlReader, parserContext);
811835
reader.Close();

0 commit comments

Comments
 (0)