diff --git a/mRemoteNG/Config/Serializers/ConnectionSerializers/Xml/XmlConnectionsDeserializer.cs b/mRemoteNG/Config/Serializers/ConnectionSerializers/Xml/XmlConnectionsDeserializer.cs index 80350309a..ab6c15a05 100644 --- a/mRemoteNG/Config/Serializers/ConnectionSerializers/Xml/XmlConnectionsDeserializer.cs +++ b/mRemoteNG/Config/Serializers/ConnectionSerializers/Xml/XmlConnectionsDeserializer.cs @@ -94,8 +94,7 @@ namespace mRemoteNG.Config.Serializers.ConnectionSerializers.Xml connections = _decryptor.LegacyFullFileDecrypt(connections); if (connections != "") { - _xmlDocument = new XmlDocument(); - _xmlDocument.LoadXml(connections); + _xmlDocument = SecureXmlHelper.LoadXmlFromString(connections); } } diff --git a/mRemoteNG/Config/Serializers/MiscSerializers/PuttyConnectionManagerDeserializer.cs b/mRemoteNG/Config/Serializers/MiscSerializers/PuttyConnectionManagerDeserializer.cs index 7fa3af6b7..9be3e3ce2 100644 --- a/mRemoteNG/Config/Serializers/MiscSerializers/PuttyConnectionManagerDeserializer.cs +++ b/mRemoteNG/Config/Serializers/MiscSerializers/PuttyConnectionManagerDeserializer.cs @@ -20,8 +20,7 @@ namespace mRemoteNG.Config.Serializers.MiscSerializers RootNodeInfo root = new(RootNodeType.Connection); connectionTreeModel.AddRootNode(root); - XmlDocument xmlDocument = new(); - xmlDocument.LoadXml(puttycmConnectionsXml); + XmlDocument xmlDocument = SecureXmlHelper.LoadXmlFromString(puttycmConnectionsXml); XmlNode configurationNode = xmlDocument.SelectSingleNode("/configuration"); diff --git a/mRemoteNG/Config/Serializers/MiscSerializers/RemoteDesktopConnectionManagerDeserializer.cs b/mRemoteNG/Config/Serializers/MiscSerializers/RemoteDesktopConnectionManagerDeserializer.cs index 07207e9ad..cd5ae24fb 100644 --- a/mRemoteNG/Config/Serializers/MiscSerializers/RemoteDesktopConnectionManagerDeserializer.cs +++ b/mRemoteNG/Config/Serializers/MiscSerializers/RemoteDesktopConnectionManagerDeserializer.cs @@ -26,9 +26,7 @@ namespace mRemoteNG.Config.Serializers.MiscSerializers ConnectionTreeModel connectionTreeModel = new(); RootNodeInfo root = new(RootNodeType.Connection); - XmlDocument xmlDocument = new(); - xmlDocument.LoadXml(rdcmConnectionsXml); - + XmlDocument xmlDocument = SecureXmlHelper.LoadXmlFromString(rdcmConnectionsXml); XmlNode rdcManNode = xmlDocument.SelectSingleNode("/RDCMan"); VerifySchemaVersion(rdcManNode); diff --git a/mRemoteNG/Config/Serializers/MiscSerializers/SecureCRTFileDeserializer.cs b/mRemoteNG/Config/Serializers/MiscSerializers/SecureCRTFileDeserializer.cs index 6aa4a74de..b22efaca7 100644 --- a/mRemoteNG/Config/Serializers/MiscSerializers/SecureCRTFileDeserializer.cs +++ b/mRemoteNG/Config/Serializers/MiscSerializers/SecureCRTFileDeserializer.cs @@ -2,6 +2,7 @@ using mRemoteNG.Connection; using mRemoteNG.Connection.Protocol; using mRemoteNG.Container; +using mRemoteNG.Security; using mRemoteNG.Tree; using mRemoteNG.Tree.Root; using System; @@ -22,8 +23,7 @@ namespace mRemoteNG.Config.Serializers.MiscSerializers RootNodeInfo root = new(RootNodeType.Connection); connectionTreeModel.AddRootNode(root); - XmlDocument xmlDocument = new(); - xmlDocument.LoadXml(content); + XmlDocument xmlDocument = SecureXmlHelper.LoadXmlFromString(content); XmlNode sessionsNode = xmlDocument.SelectSingleNode("/VanDyke/key[@name=\"Sessions\"]"); diff --git a/mRemoteNG/Config/Settings/ExternalAppsLoader.cs b/mRemoteNG/Config/Settings/ExternalAppsLoader.cs index eaa6d11e5..b4f66403b 100644 --- a/mRemoteNG/Config/Settings/ExternalAppsLoader.cs +++ b/mRemoteNG/Config/Settings/ExternalAppsLoader.cs @@ -5,6 +5,7 @@ using mRemoteNG.UI.Forms; using System.IO; using System.Xml; using mRemoteNG.Messages; +using mRemoteNG.Security; using mRemoteNG.Tools; using mRemoteNG.UI.Controls; using System.Runtime.Versioning; @@ -40,18 +41,18 @@ namespace mRemoteNG.Config.Settings Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), GeneralAppInfo.ProductName, SettingsFileInfo.ExtAppsFilesName); #endif string newPath = Path.Combine(SettingsFileInfo.SettingsPath, SettingsFileInfo.ExtAppsFilesName); - XmlDocument xDom = new(); + XmlDocument xDom; if (File.Exists(newPath)) { _messageCollector.AddMessage(MessageClass.InformationMsg, $"Loading External Apps from: {newPath}", true); - xDom.Load(newPath); + xDom = SecureXmlHelper.LoadXmlFromFile(newPath); } #if !PORTABLE else if (File.Exists(oldPath)) { _messageCollector.AddMessage(MessageClass.InformationMsg, $"Loading External Apps from: {oldPath}", true); - xDom.Load(oldPath); + xDom = SecureXmlHelper.LoadXmlFromFile(oldPath); } #endif diff --git a/mRemoteNG/Config/Settings/Providers/PortableSettingsProvider.cs b/mRemoteNG/Config/Settings/Providers/PortableSettingsProvider.cs index bd0a64cbe..fac87725e 100644 --- a/mRemoteNG/Config/Settings/Providers/PortableSettingsProvider.cs +++ b/mRemoteNG/Config/Settings/Providers/PortableSettingsProvider.cs @@ -31,6 +31,7 @@ using System.Windows.Forms; using System.Collections.Specialized; using System.Xml; using System.IO; +using mRemoteNG.Security; //using mRemoteNG.App; @@ -61,8 +62,7 @@ namespace mRemoteNG.Config.Settings.Providers if (_xmlDocument != null) return _xmlDocument; try { - _xmlDocument = new XmlDocument(); - _xmlDocument.Load(_filePath); + _xmlDocument = SecureXmlHelper.LoadXmlFromFile(_filePath); } catch (Exception /*ex*/) { diff --git a/mRemoteNG/Security/SecureXmlHelper.cs b/mRemoteNG/Security/SecureXmlHelper.cs new file mode 100644 index 000000000..1f9c0e998 --- /dev/null +++ b/mRemoteNG/Security/SecureXmlHelper.cs @@ -0,0 +1,73 @@ +using System.IO; +using System.Xml; + +namespace mRemoteNG.Security +{ + /// + /// Helper class for securely loading XML documents to prevent XXE (XML External Entity) attacks + /// + public static class SecureXmlHelper + { + /// + /// Creates an XmlDocument with secure settings that prevent XXE attacks + /// + /// A new XmlDocument with secure settings + public static XmlDocument CreateSecureXmlDocument() + { + XmlDocument xmlDocument = new XmlDocument + { + XmlResolver = null // Disable external entity resolution + }; + return xmlDocument; + } + + /// + /// Safely loads XML content from a string into an XmlDocument with XXE protection + /// + /// The XML content to load + /// An XmlDocument with the loaded content + public static XmlDocument LoadXmlFromString(string xmlContent) + { + XmlReaderSettings settings = CreateSecureReaderSettings(); + using (StringReader stringReader = new StringReader(xmlContent)) + using (XmlReader reader = XmlReader.Create(stringReader, settings)) + { + XmlDocument xmlDocument = CreateSecureXmlDocument(); + xmlDocument.Load(reader); + return xmlDocument; + } + } + + /// + /// Safely loads XML content from a file into an XmlDocument with XXE protection + /// + /// The path to the XML file + /// An XmlDocument with the loaded content + public static XmlDocument LoadXmlFromFile(string filePath) + { + XmlReaderSettings settings = CreateSecureReaderSettings(); + using (XmlReader reader = XmlReader.Create(filePath, settings)) + { + XmlDocument xmlDocument = CreateSecureXmlDocument(); + xmlDocument.Load(reader); + return xmlDocument; + } + } + + /// + /// Creates XmlReaderSettings with secure defaults to prevent XXE attacks + /// + /// XmlReaderSettings configured for security + private static XmlReaderSettings CreateSecureReaderSettings() + { + return new XmlReaderSettings + { + DtdProcessing = DtdProcessing.Prohibit, // Prohibit DTD processing + XmlResolver = null, // Disable external entity resolution + MaxCharactersFromEntities = 0, // Don't allow entity expansion + IgnoreProcessingInstructions = true, // Ignore processing instructions + IgnoreComments = true // Ignore comments + }; + } + } +} diff --git a/mRemoteNG/Themes/MremoteNGPaletteManipulator.cs b/mRemoteNG/Themes/MremoteNGPaletteManipulator.cs index 7ada852e2..96111e224 100644 --- a/mRemoteNG/Themes/MremoteNGPaletteManipulator.cs +++ b/mRemoteNG/Themes/MremoteNGPaletteManipulator.cs @@ -3,6 +3,7 @@ using System.Drawing; using System.Globalization; using System.IO; using System.Xml; +using mRemoteNG.Security; using mRemoteNG.Themes; namespace mRemoteNG.Themes @@ -17,8 +18,7 @@ namespace mRemoteNG.Themes //warning, defaultpalette should always contain all the values, because when is loaded there is no default palette (parameter is null public MremoteNGPaletteManipulator(byte[] file, ExtendedColorPalette defaultPalette = null) { - _xml = new XmlDocument(); - _xml.LoadXml(new StreamReader(new MemoryStream(file)).ReadToEnd()); + _xml = SecureXmlHelper.LoadXmlFromString(new StreamReader(new MemoryStream(file)).ReadToEnd()); _defaultPalette = defaultPalette ?? new ExtendedColorPalette(); }