From 8698172f8b907695be06798c02174fe9908a1e54 Mon Sep 17 00:00:00 2001 From: Vatsan Madhavan Date: Tue, 16 Jul 2019 13:12:19 -0700 Subject: [PATCH 1/3] Remove support for `UseLegacyDangerousClipboardDeserializationMode` and permanently disallow deserialization of dangerous types. - Removes support for AppContext flag `UseLegacyDangerousClipboardDeserializationMode` - Permanently limits clipboard-deserialziation to primitive non-text types only. Fixes #1132 --- .../MS/internal/CoreAppContextSwitches.cs | 19 ----------- .../System/AppContextDefaultValues.cs | 1 - .../System/Windows/clipboard.cs | 11 ------- .../System/Windows/dataobject.cs | 32 +++++++++++++------ .../MS/Internal/Ink/XamlClipboardData.cs | 3 +- .../Windows/Documents/TextEditorCopyPaste.cs | 3 +- .../System/Windows/Documents/WpfPayload.cs | 3 +- 7 files changed, 25 insertions(+), 47 deletions(-) diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/CoreAppContextSwitches.cs b/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/CoreAppContextSwitches.cs index 69fa8116692..2202f636325 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/CoreAppContextSwitches.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/CoreAppContextSwitches.cs @@ -337,25 +337,6 @@ public static bool AllowExternalProcessToBlockAccessToTemporaryFiles #endregion - #region EnableLegacyDangerousClipboardDeserializationMode - - internal const string EnableLegacyDangerousClipboardDeserializationModeSwitchName = "Switch.System.Windows.EnableLegacyDangerousClipboardDeserializationMode"; - private static int _enableLegacyDangerousClipboardDeserializationMode; - public static bool EnableLegacyDangerousClipboardDeserializationMode - { - [MethodImpl(MethodImplOptions.AggressiveInlining)] - get - { - /// - /// Malicious managed objects could be placed in the clipboard lying about its format, - /// to fix this OleConverter now restricts object deserialization in some cases. - /// When this switch is enabled behavior falls back to deserializing without restriction. - /// - return LocalAppContext.GetCachedSwitchValue(EnableLegacyDangerousClipboardDeserializationModeSwitchName, ref _enableLegacyDangerousClipboardDeserializationMode); - } - } - - #endregion } #pragma warning restore 436 } diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/AppContextDefaultValues.cs b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/AppContextDefaultValues.cs index 831ae48a00a..658044e25b8 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/AppContextDefaultValues.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/AppContextDefaultValues.cs @@ -60,7 +60,6 @@ private static void InitializeNetFxSwitchDefaultsForNetCoreRuntime() LocalAppContext.DefineSwitchDefault(CoreAppContextSwitches.ShouldNotRenderInNonInteractiveWindowStationSwitchName, false); LocalAppContext.DefineSwitchDefault(CoreAppContextSwitches.DoNotUsePresentationDpiCapabilityTier3OrGreaterSwitchName, false); LocalAppContext.DefineSwitchDefault(CoreAppContextSwitches.AllowExternalProcessToBlockAccessToTemporaryFilesSwitchName, false); - LocalAppContext.DefineSwitchDefault(CoreAppContextSwitches.EnableLegacyDangerousClipboardDeserializationModeSwitchName, false); } } #pragma warning restore 436 diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/clipboard.cs b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/clipboard.cs index bd0b14d1ba9..9f30b4b16b9 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/clipboard.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/clipboard.cs @@ -490,17 +490,6 @@ public static void SetDataObject(object data, bool copy) // //------------------------------------------------------ - /// - /// Determines whether the legacy dangerous clipboard deserialization mode should be used based on the AppContext switch and Device Guard policies. - /// - /// - /// If Device Guard is enabled this method returns false, otherwise it returns the AppContext switch value. - /// - internal static bool UseLegacyDangerousClipboardDeserializationMode() - { - return !IsDeviceGuardEnabled && CoreAppContextSwitches.EnableLegacyDangerousClipboardDeserializationMode; - } - /// /// Places data on the system Clipboard and uses copy to specify whether the data /// should remain on the Clipboard after the application exits. diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs index 0a8d2c3e0a6..584667a6f33 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs @@ -2863,13 +2863,29 @@ private object GetDataFromHGLOBAL(string format, IntPtr hglobal) { data = ReadBitmapSourceFromHandle(hglobal); } - // Restrict deserialization to only primitives - // and strings here to prevent potentially malicious objects from + // Limit deserialization to only primitive types, which consist of the following: + // + // DataFormats.CommaSeparatedValue + // DataFormats.FileDrop + // DataFormats.Html + // DataFormats.OemText + // DataFormats.PenData + // DataFormats.Rtf + // DataFormats.Serializable + // DataFormats.Text + // DataFormats.UnicodeText + // DataFormats.WaveAudio + // DataFormats.Xaml + // DataFormats.XamlPackage + // DataFormats.StringFormat + // + // Out of these primitive types, we will disallow deserialization of + // DataFormats.StringFormat to prevent potentially malicious objects from // being deserialized as part of a "text" copy-paste or drag-drop. + // // The rest of the following formats are pre-defined in the OS, - // they are not managed objects so we shouldn't try to deserialize them as such, - // allow primitives in a best effort for compat, but restrict other types. - else if (!Clipboard.UseLegacyDangerousClipboardDeserializationMode()) + // they are not managed objects - an so we will not attempt to deserialize them. + else { bool restrictDeserialization = (IsFormatEqual(format, DataFormats.StringFormat) || @@ -2888,11 +2904,7 @@ private object GetDataFromHGLOBAL(string format, IntPtr hglobal) data = ReadObjectFromHandle(hglobal, restrictDeserialization); } - else - { - data = ReadObjectFromHandle(hglobal, restrictDeserialization: false); - } -} + } return data; } diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Ink/XamlClipboardData.cs b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Ink/XamlClipboardData.cs index 780889e83b7..2ca03e10a10 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Ink/XamlClipboardData.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Ink/XamlClipboardData.cs @@ -100,8 +100,7 @@ protected override void DoPaste(IDataObject dataObject) if ( !String.IsNullOrEmpty(xml) ) { - bool useRestrictiveXamlReader = !Clipboard.UseLegacyDangerousClipboardDeserializationMode(); - UIElement element = XamlReader.Load(new System.Xml.XmlTextReader(new System.IO.StringReader(xml)), useRestrictiveXamlReader) as UIElement; + UIElement element = XamlReader.Load(new System.Xml.XmlTextReader(new System.IO.StringReader(xml)), useRestrictiveXamlReader: true) as UIElement; if (element != null) { ElementList.Add(element); diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextEditorCopyPaste.cs b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextEditorCopyPaste.cs index c619adef732..bd3cf4c9af7 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextEditorCopyPaste.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextEditorCopyPaste.cs @@ -923,8 +923,7 @@ private static bool PasteXaml(TextEditor This, string pasteXaml) try { // Parse the fragment into a separate subtree - bool useRestrictiveXamlReader = !Clipboard.UseLegacyDangerousClipboardDeserializationMode(); - object xamlObject = XamlReader.Load(new XmlTextReader(new System.IO.StringReader(pasteXaml)), useRestrictiveXamlReader); + object xamlObject = XamlReader.Load(new XmlTextReader(new System.IO.StringReader(pasteXaml)), useRestrictiveXamlReader: true); TextElement flowContent = xamlObject as TextElement; success = flowContent == null ? false : PasteTextElement(This, flowContent); diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/WpfPayload.cs b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/WpfPayload.cs index 40a69e8a890..d38793e2b22 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/WpfPayload.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/WpfPayload.cs @@ -343,8 +343,7 @@ internal static object LoadElement(Stream stream) parserContext.BaseUri = entryPartUri; // Call xaml parser - bool useRestrictiveXamlReader = !Clipboard.UseLegacyDangerousClipboardDeserializationMode(); - xamlObject = XamlReader.Load(xamlEntryPart.GetStream(), parserContext, useRestrictiveXamlReader); + xamlObject = XamlReader.Load(xamlEntryPart.GetStream(), parserContext, useRestrictiveXamlReader: true); // Remove the temporary uri from the PackageStore PackageStore.RemovePackage(packageUri); From 36ad5da07be3ccd2ca0b174bfc48bc50c1036d4a Mon Sep 17 00:00:00 2001 From: Vatsan Madhavan Date: Thu, 18 Jul 2019 18:49:04 -0700 Subject: [PATCH 2/3] Update comment --- .../src/PresentationCore/System/Windows/dataobject.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs index 584667a6f33..8c9a9f6f958 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs @@ -2863,7 +2863,7 @@ private object GetDataFromHGLOBAL(string format, IntPtr hglobal) { data = ReadBitmapSourceFromHandle(hglobal); } - // Limit deserialization to only primitive types, which consist of the following: + // Limit deserialization to DataFormats that correspond to primitives, which are: // // DataFormats.CommaSeparatedValue // DataFormats.FileDrop @@ -2877,9 +2877,9 @@ private object GetDataFromHGLOBAL(string format, IntPtr hglobal) // DataFormats.WaveAudio // DataFormats.Xaml // DataFormats.XamlPackage - // DataFormats.StringFormat + // DataFormats.StringFormat * // - // Out of these primitive types, we will disallow deserialization of + // * Out of these, we will disallow deserialization of // DataFormats.StringFormat to prevent potentially malicious objects from // being deserialized as part of a "text" copy-paste or drag-drop. // From 54aedb7ccddb5c3f64c1394e2506434f1bfd7825 Mon Sep 17 00:00:00 2001 From: Vatsan Madhavan Date: Thu, 18 Jul 2019 19:03:12 -0700 Subject: [PATCH 3/3] Another update to comments --- .../src/PresentationCore/System/Windows/dataobject.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs index 8c9a9f6f958..4874a0211e0 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/dataobject.cs @@ -2882,6 +2882,10 @@ private object GetDataFromHGLOBAL(string format, IntPtr hglobal) // * Out of these, we will disallow deserialization of // DataFormats.StringFormat to prevent potentially malicious objects from // being deserialized as part of a "text" copy-paste or drag-drop. + // TypeRestrictingSerializationBinder will throw when it encounters + // anything other than strings and primitives - this ensures that we will + // continue successfully deserializing basic strings while rejecting other + // data types that advertise themselves as DataFormats.StringFormat. // // The rest of the following formats are pre-defined in the OS, // they are not managed objects - an so we will not attempt to deserialize them.