(chocolateyGH-1505) Ensure package information is escaped

- Also, when de-serializing, if a .backup file exists, remove original - Warn user that .backup file exists and needs to be corrected
gep13 · Feb 22, 2018 · 87e7978 · 87e7978
1 parent e9dc026
commit 87e7978
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 23 deletions.
diff --git a/src/chocolatey/infrastructure.app/domain/RegistryValueExtensions.cs b/src/chocolatey/infrastructure.app/domain/RegistryValueExtensions.cs
@@ -24,7 +24,11 @@ public static string get_value_as_string(this RegistryKey key, string name)
         {
             if (key == null) return string.Empty;
 
-            return key.GetValue(name).to_string().Replace("\0", string.Empty);
+            // Since it is possible that registry keys contain characters that are not valid
+            // in XML files, ensure that all content is escaped, prior to serialization
+            var escapedXml = System.Security.SecurityElement.Escape(key.GetValue(name).to_string());
+
+            return escapedXml?.Replace("\0", string.Empty);
         }
     }
 }
diff --git a/src/chocolatey/infrastructure.app/services/ChocolateyPackageInformationService.cs b/src/chocolatey/infrastructure.app/services/ChocolateyPackageInformationService.cs
@@ -29,6 +29,7 @@ public class ChocolateyPackageInformationService : IChocolateyPackageInformation
         private readonly IRegistryService _registryService;
         private readonly IFilesService _filesService;
         private const string REGISTRY_SNAPSHOT_FILE = ".registry";
+        private const string REGISTRY_SNAPSHOT_BACKUP_FILE = ".registry.backup";
         private const string FILES_SNAPSHOT_FILE = ".files";
         private const string SILENT_UNINSTALLER_FILE = ".silentUninstaller";
         private const string SIDE_BY_SIDE_FILE = ".sxs";
@@ -62,7 +63,17 @@ public ChocolateyPackageInformation get_package_information(IPackage package)
             FaultTolerance.try_catch_with_logging_exception(
                 () =>
                     {
-                        packageInformation.RegistrySnapshot = _registryService.read_from_file(_fileSystem.combine_paths(pkgStorePath, REGISTRY_SNAPSHOT_FILE));
+                        if (_fileSystem.file_exists(_fileSystem.combine_paths(pkgStorePath, REGISTRY_SNAPSHOT_BACKUP_FILE)))
+                        {
+                            // Remove original file, since it was corrupt
+                            _fileSystem.delete_file(_fileSystem.combine_paths(pkgStorePath, REGISTRY_SNAPSHOT_FILE));
+
+                            this.Log().Warn("A corrupt .registry file exists at {0}, please correct the file and rename.", _fileSystem.combine_paths(pkgStorePath, REGISTRY_SNAPSHOT_FILE));
+                        }
+                        else
+                        {
+                            packageInformation.RegistrySnapshot = _registryService.read_from_file(_fileSystem.combine_paths(pkgStorePath, REGISTRY_SNAPSHOT_FILE));
+                        }
                     },
                     "Unable to read registry snapshot file for {0} (located at {1})".format_with(package.Id, _fileSystem.combine_paths(pkgStorePath, REGISTRY_SNAPSHOT_FILE)),
                     throwError: false,

diff --git a/src/chocolatey/infrastructure/services/XmlService.cs b/src/chocolatey/infrastructure/services/XmlService.cs
@@ -45,7 +45,7 @@ public XmlService(IFileSystem fileSystem, IHashProvider hashProvider)
 
         public XmlType deserialize<XmlType>(string xmlFilePath)
         {
-            return FaultTolerance.retry(3, () => GlobalMutex.enter(
+            return  FaultTolerance.retry(3, () => GlobalMutex.enter(
                 () =>
                 {
                     this.Log().Trace("Entered mutex to deserialize '{0}'".format_with(xmlFilePath));
@@ -73,27 +73,17 @@ public XmlType deserialize<XmlType>(string xmlFilePath)
                                 // Check if its just a malformed document.
                                 if (ex.Message.Contains("There is an error in XML document"))
                                 {
-                                    // If so, check for a backup file and try an parse that.
-                                    if (_fileSystem.file_exists(xmlFilePath + ".backup"))
-                                    {
-                                        using (var backupStream = _fileSystem.open_file_readonly(xmlFilePath + ".backup"))
-                                        using (var backupReader = new StreamReader(backupStream))
-                                        using (var backupXmlReader = XmlReader.Create(backupReader))
-                                        {
-                                            var validConfig = (XmlType)xmlSerializer.Deserialize(backupXmlReader);
-
-                                            // If there's no errors and it's valid, go ahead and replace the bad file with the backup.
-                                            if (validConfig != null)
-                                            {
-                                                _fileSystem.copy_file(xmlFilePath + ".backup", xmlFilePath, overwriteExisting: true);
-                                            }
-
-                                            return validConfig;
-                                        }
-                                    }
-                                }
+                                    // Move file to a backup location, so that it isn't parsed again
+                                    _fileSystem.copy_file(xmlFilePath, xmlFilePath + ".backup", overwriteExisting: true);
 
-                                throw;
+                                    this.Log().Warn("Unable to deserialize XML located at {0}", xmlFilePath);
+                                    this.Log().Warn("This file has been moved to {0}.  Please correct the file, and then rename the file, removing .backup", xmlFilePath + ".backup");
+                                    return default(XmlType);
+                                }
+                                else
+                                {
+                                    throw;
+                                }
                             }
                             finally
                             {