Merge pull request #9564 from AmbulantRex/whitelist-dlls

Emby.Server.Implementations/Library/PathExtensions.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Diagnostics.CodeAnalysis;
+using System.IO;
 using MediaBrowser.Common.Providers;
 
 namespace Emby.Server.Implementations.Library
@@ -86,24 +87,8 @@ namespace Emby.Server.Implementations.Library
                 return false;
             }
 
-            char oldDirectorySeparatorChar;
-            char newDirectorySeparatorChar;
-            // True normalization is still not possible https://github.com/dotnet/runtime/issues/2162
-            // The reasoning behind this is that a forward slash likely means it's a Linux path and
-            // so the whole path should be normalized to use / and vice versa for Windows (although Windows doesn't care much).
-            if (newSubPath.Contains('/', StringComparison.Ordinal))
-            {
-                oldDirectorySeparatorChar = '\\';
-                newDirectorySeparatorChar = '/';
-            }
-            else
-            {
-                oldDirectorySeparatorChar = '/';
-                newDirectorySeparatorChar = '\\';
-            }
-
-            path = path.Replace(oldDirectorySeparatorChar, newDirectorySeparatorChar);
-            subPath = subPath.Replace(oldDirectorySeparatorChar, newDirectorySeparatorChar);
+            subPath = subPath.NormalizePath(out var newDirectorySeparatorChar);
+            path = path.NormalizePath(newDirectorySeparatorChar);
 
             // We have to ensure that the sub path ends with a directory separator otherwise we'll get weird results
             // when the sub path matches a similar but in-complete subpath
@@ -127,5 +112,82 @@ namespace Emby.Server.Implementations.Library
 
             return true;
         }
+
+        /// <summary>
+        /// Retrieves the full resolved path and normalizes path separators to the <see cref="Path.DirectorySeparatorChar"/>.
+        /// </summary>
+        /// <param name="path">The path to canonicalize.</param>
+        /// <returns>The fully expanded, normalized path.</returns>
+        public static string Canonicalize(this string path)
+        {
+            return Path.GetFullPath(path).NormalizePath();
+        }
+
+        /// <summary>
+        /// Normalizes the path's directory separator character to the currently defined <see cref="Path.DirectorySeparatorChar"/>.
+        /// </summary>
+        /// <param name="path">The path to normalize.</param>
+        /// <returns>The normalized path string or <see langword="null"/> if the input path is null or empty.</returns>
+        [return: NotNullIfNotNull(nameof(path))]
+        public static string? NormalizePath(this string? path)
+        {
+            return path.NormalizePath(Path.DirectorySeparatorChar);
+        }
+
+        /// <summary>
+        /// Normalizes the path's directory separator character.
+        /// </summary>
+        /// <param name="path">The path to normalize.</param>
+        /// <param name="separator">The separator character the path now uses or <see langword="null"/>.</param>
+        /// <returns>The normalized path string or <see langword="null"/> if the input path is null or empty.</returns>
+        [return: NotNullIfNotNull(nameof(path))]
+        public static string? NormalizePath(this string? path, out char separator)
+        {
+            if (string.IsNullOrEmpty(path))
+            {
+                separator = default;
+                return path;
+            }
+
+            var newSeparator = '\\';
+
+            // True normalization is still not possible https://github.com/dotnet/runtime/issues/2162
+            // The reasoning behind this is that a forward slash likely means it's a Linux path and
+            // so the whole path should be normalized to use / and vice versa for Windows (although Windows doesn't care much).
+            if (path.Contains('/', StringComparison.Ordinal))
+            {
+                newSeparator = '/';
+            }
+
+            separator = newSeparator;
+
+            return path.NormalizePath(newSeparator);
+        }
+
+        /// <summary>
+        /// Normalizes the path's directory separator character to the specified character.
+        /// </summary>
+        /// <param name="path">The path to normalize.</param>
+        /// <param name="newSeparator">The replacement directory separator character. Must be a valid directory separator.</param>
+        /// <returns>The normalized path.</returns>
+        /// <exception cref="ArgumentException">Thrown if the new separator character is not a directory separator.</exception>
+        [return: NotNullIfNotNull(nameof(path))]
+        public static string? NormalizePath(this string? path, char newSeparator)
+        {
+            const char Bs = '\\';
+            const char Fs = '/';
+
+            if (!(newSeparator == Bs || newSeparator == Fs))
+            {
+                throw new ArgumentException("The character must be a directory separator.");
+            }
+
+            if (string.IsNullOrEmpty(path))
+            {
+                return path;
+            }
+
+            return newSeparator == Bs ? path.Replace(Fs, newSeparator) : path.Replace(Bs, newSeparator);
+        }
     }
 }

Emby.Server.Implementations/Plugins/PluginManager.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Data;
 using System.Globalization;
 using System.IO;
 using System.Linq;
@@ -9,6 +10,8 @@ using System.Runtime.Loader;
 using System.Text;
 using System.Text.Json;
 using System.Threading.Tasks;
+using Emby.Server.Implementations.Library;
+using Jellyfin.Extensions;
 using Jellyfin.Extensions.Json;
 using Jellyfin.Extensions.Json.Converters;
 using MediaBrowser.Common;
@@ -29,6 +32,8 @@ namespace Emby.Server.Implementations.Plugins
     /// </summary>
     public class PluginManager : IPluginManager
     {
+        private const string MetafileName = "meta.json";
+
         private readonly string _pluginsPath;
         private readonly Version _appVersion;
         private readonly List<AssemblyLoadContext> _assemblyLoadContexts;
@@ -44,7 +49,7 @@ namespace Emby.Server.Implementations.Plugins
         /// <summary>
         /// Initializes a new instance of the <see cref="PluginManager"/> class.
         /// </summary>
-        /// <param name="logger">The <see cref="ILogger"/>.</param>
+        /// <param name="logger">The <see cref="ILogger{PluginManager}"/>.</param>
         /// <param name="appHost">The <see cref="IApplicationHost"/>.</param>
         /// <param name="config">The <see cref="ServerConfiguration"/>.</param>
         /// <param name="pluginsPath">The plugin path.</param>
@@ -371,7 +376,7 @@ namespace Emby.Server.Implementations.Plugins
             try
             {
                 var data = JsonSerializer.Serialize(manifest, _jsonOptions);
-                File.WriteAllText(Path.Combine(path, "meta.json"), data);
+                File.WriteAllText(Path.Combine(path, MetafileName), data);
                 return true;
             }
             catch (ArgumentException e)
@@ -382,7 +387,7 @@ namespace Emby.Server.Implementations.Plugins
         }
 
         /// <inheritdoc/>
-        public async Task<bool> GenerateManifest(PackageInfo packageInfo, Version version, string path, PluginStatus status)
+        public async Task<bool> PopulateManifest(PackageInfo packageInfo, Version version, string path, PluginStatus status)
         {
             var versionInfo = packageInfo.Versions.First(v => v.Version == version.ToString());
             var imagePath = string.Empty;
@@ -427,9 +432,71 @@ namespace Emby.Server.Implementations.Plugins
                 ImagePath = imagePath
             };
 
+            if (!await ReconcileManifest(manifest, path))
+            {
+                // An error occurred during reconciliation and saving could be undesirable.
+                return false;
+            }
+
             return SaveManifest(manifest, path);
         }
 
+        /// <summary>
+        /// Reconciles the manifest against any properties that exist locally in a pre-packaged meta.json found at the path.
+        /// If no file is found, no reconciliation occurs.
+        /// </summary>
+        /// <param name="manifest">The <see cref="PluginManifest"/> to reconcile against.</param>
+        /// <param name="path">The plugin path.</param>
+        /// <returns>The reconciled <see cref="PluginManifest"/>.</returns>
+        private async Task<bool> ReconcileManifest(PluginManifest manifest, string path)
+        {
+            try
+            {
+                var metafile = Path.Combine(path, MetafileName);
+                if (!File.Exists(metafile))
+                {
+                    _logger.LogInformation("No local manifest exists for plugin {Plugin}. Skipping manifest reconciliation.", manifest.Name);
+                    return true;
+                }
+
+                using var metaStream = File.OpenRead(metafile);
+                var localManifest = await JsonSerializer.DeserializeAsync<PluginManifest>(metaStream, _jsonOptions);
+                localManifest ??= new PluginManifest();
+
+                if (!Equals(localManifest.Id, manifest.Id))
+                {
+                    _logger.LogError("The manifest ID {LocalUUID} did not match the package info ID {PackageUUID}.", localManifest.Id, manifest.Id);
+                    manifest.Status = PluginStatus.Malfunctioned;
+                }
+
+                if (localManifest.Version != manifest.Version)
+                {
+                    // Package information provides the version and is the source of truth. Pre-packages meta.json is assumed to be a mistake in this regard.
+                    _logger.LogWarning("The version of the local manifest was {LocalVersion}, but {PackageVersion} was expected. The value will be replaced.", localManifest.Version, manifest.Version);
+                }
+
+                // Explicitly mapping properties instead of using reflection is preferred here.
+                manifest.Category = string.IsNullOrEmpty(localManifest.Category) ? manifest.Category : localManifest.Category;
+                manifest.AutoUpdate = localManifest.AutoUpdate; // Preserve whatever is local. Package info does not have this property.
+                manifest.Changelog = string.IsNullOrEmpty(localManifest.Changelog) ? manifest.Changelog : localManifest.Changelog;
+                manifest.Description = string.IsNullOrEmpty(localManifest.Description) ? manifest.Description : localManifest.Description;
+                manifest.Name = string.IsNullOrEmpty(localManifest.Name) ? manifest.Name : localManifest.Name;
+                manifest.Overview = string.IsNullOrEmpty(localManifest.Overview) ? manifest.Overview : localManifest.Overview;
+                manifest.Owner = string.IsNullOrEmpty(localManifest.Owner) ? manifest.Owner : localManifest.Owner;
+                manifest.TargetAbi = string.IsNullOrEmpty(localManifest.TargetAbi) ? manifest.TargetAbi : localManifest.TargetAbi;
+                manifest.Timestamp = localManifest.Timestamp.Equals(default) ? manifest.Timestamp : localManifest.Timestamp;
+                manifest.ImagePath = string.IsNullOrEmpty(localManifest.ImagePath) ? manifest.ImagePath : localManifest.ImagePath;
+                manifest.Assemblies = localManifest.Assemblies;
+
+                return true;
+            }
+            catch (Exception e)
+            {
+                _logger.LogWarning(e, "Unable to reconcile plugin manifest due to an error. {Path}", path);
+                return false;
+            }
+        }
+
         /// <summary>
         /// Changes a plugin's load status.
         /// </summary>
@@ -594,7 +661,7 @@ namespace Emby.Server.Implementations.Plugins
         {
             Version? version;
             PluginManifest? manifest = null;
-            var metafile = Path.Combine(dir, "meta.json");
+            var metafile = Path.Combine(dir, MetafileName);
             if (File.Exists(metafile))
             {
                 // Only path where this stays null is when File.ReadAllBytes throws an IOException
@@ -688,7 +755,15 @@ namespace Emby.Server.Implementations.Plugins
                 var entry = versions[x];
                 if (!string.Equals(lastName, entry.Name, StringComparison.OrdinalIgnoreCase))
                 {
-                    entry.DllFiles = Directory.GetFiles(entry.Path, "*.dll", SearchOption.AllDirectories);
+                    if (!TryGetPluginDlls(entry, out var allowedDlls))
+                    {
+                        _logger.LogError("One or more assembly paths was invalid. Marking plugin {Plugin} as \"Malfunctioned\".", entry.Name);
+                        ChangePluginState(entry, PluginStatus.Malfunctioned);
+                        continue;
+                    }
+
+                    entry.DllFiles = allowedDlls;
+
                     if (entry.IsEnabledAndSupported)
                     {
                         lastName = entry.Name;
@@ -734,6 +809,68 @@ namespace Emby.Server.Implementations.Plugins
             return versions.Where(p => p.DllFiles.Count != 0);
         }
 
+        /// <summary>
+        /// Attempts to retrieve valid DLLs from the plugin path. This method will consider the assembly whitelist
+        /// from the manifest.
+        /// </summary>
+        /// <remarks>
+        /// Loading DLLs from externally supplied paths introduces a path traversal risk. This method
+        /// uses a safelisting tactic of considering DLLs from the plugin directory and only using
+        /// the plugin's canonicalized assembly whitelist for comparison. See
+        /// <see href="https://owasp.org/www-community/attacks/Path_Traversal"/> for more details.
+        /// </remarks>
+        /// <param name="plugin">The plugin.</param>
+        /// <param name="whitelistedDlls">The whitelisted DLLs. If the method returns <see langword="false"/>, this will be empty.</param>
+        /// <returns>
+        /// <see langword="true"/> if all assemblies listed in the manifest were available in the plugin directory.
+        /// <see langword="false"/> if any assemblies were invalid or missing from the plugin directory.
+        /// </returns>
+        /// <exception cref="ArgumentNullException">If the <see cref="LocalPlugin"/> is null.</exception>
+        private bool TryGetPluginDlls(LocalPlugin plugin, out IReadOnlyList<string> whitelistedDlls)
+        {
+            ArgumentNullException.ThrowIfNull(nameof(plugin));
+
+            IReadOnlyList<string> pluginDlls = Directory.GetFiles(plugin.Path, "*.dll", SearchOption.AllDirectories);
+
+            whitelistedDlls = Array.Empty<string>();
+            if (pluginDlls.Count > 0 && plugin.Manifest.Assemblies.Count > 0)
+            {
+                _logger.LogInformation("Registering whitelisted assemblies for plugin \"{Plugin}\"...", plugin.Name);
+
+                var canonicalizedPaths = new List<string>();
+                foreach (var path in plugin.Manifest.Assemblies)
+                {
+                    var canonicalized = Path.Combine(plugin.Path, path).Canonicalize();
+
+                    // Ensure we stay in the plugin directory.
+                    if (!canonicalized.StartsWith(plugin.Path.NormalizePath(), StringComparison.Ordinal))
+                    {
+                        _logger.LogError("Assembly path {Path} is not inside the plugin directory.", path);
+                        return false;
+                    }
+
+                    canonicalizedPaths.Add(canonicalized);
+                }
+
+                var intersected = pluginDlls.Intersect(canonicalizedPaths).ToList();
+
+                if (intersected.Count != canonicalizedPaths.Count)
+                {
+                    _logger.LogError("Plugin {Plugin} contained assembly paths that were not found in the directory.", plugin.Name);
+                    return false;
+                }
+
+                whitelistedDlls = intersected;
+            }
+            else
+            {
+                // No whitelist, default to loading all DLLs in plugin directory.
+                whitelistedDlls = pluginDlls;
+            }
+
+            return true;
+        }
+
         /// <summary>
         /// Changes the status of the other versions of the plugin to "Superceded".
         /// </summary>

Emby.Server.Implementations/Updates/InstallationManager.cs

@@ -183,7 +183,7 @@ namespace Emby.Server.Implementations.Updates
                             var plugin = _pluginManager.GetPlugin(package.Id, version.VersionNumber);
                             if (plugin is not null)
                             {
-                                await _pluginManager.GenerateManifest(package, version.VersionNumber, plugin.Path, plugin.Manifest.Status).ConfigureAwait(false);
+                                await _pluginManager.PopulateManifest(package, version.VersionNumber, plugin.Path, plugin.Manifest.Status).ConfigureAwait(false);
                             }
 
                             // Remove versions with a target ABI greater then the current application version.
@@ -555,7 +555,10 @@ namespace Emby.Server.Implementations.Updates
             stream.Position = 0;
             using var reader = new ZipArchive(stream);
             reader.ExtractToDirectory(targetDir, true);
-            await _pluginManager.GenerateManifest(package.PackageInfo, package.Version, targetDir, status).ConfigureAwait(false);
+
+            // Ensure we create one or populate existing ones with missing data.
+            await _pluginManager.PopulateManifest(package.PackageInfo, package.Version, targetDir, status);
+
             _pluginManager.ImportPluginFrom(targetDir);
         }