Merge pull request #17368 from Shadowghost/security-path-traversal-fixes

Backport and extend path traversal fixes

11 files changed, 222 insertions, 8 deletions

diff --git a/Emby.Server.Implementations/Library/PathManager.cs b/Emby.Server.Implementations/Library/PathManager.cs
index fad948ad97..2a50fcc7fe 100644
--- a/Emby.Server.Implementations/Library/PathManager.cs
+++ b/Emby.Server.Implementations/Library/PathManager.cs
@@ -2,6 +2,7 @@ using System;
 using System.Collections.Generic;
 using System.Globalization;
 using System.IO;
+using Jellyfin.Extensions;
 using MediaBrowser.Common.Configuration;
 using MediaBrowser.Controller.Configuration;
 using MediaBrowser.Controller.Entities;
@@ -43,7 +44,19 @@ public class PathManager : IPathManager
     public string? GetAttachmentPath(string mediaSourceId, string fileName)
     {
         var folder = GetAttachmentFolderPath(mediaSourceId);
-        return folder is null ? null : Path.Combine(folder, fileName);
+        if (folder is null)
+        {
+            return null;
+        }
+
+        var safeName = PathHelper.GetSafeLeafFileName(fileName);
+        if (safeName is null)
+        {
+            _logger.LogWarning("Rejecting attachment filename '{FileName}' for MediaSource {MediaSourceId}: not a valid leaf name.", fileName, mediaSourceId);
+            return null;
+        }
+
+        return Path.Combine(folder, safeName);
     }
 
     /// <inheritdoc />
diff --git a/Jellyfin.Api/Controllers/ImageController.cs b/Jellyfin.Api/Controllers/ImageController.cs
index 52d8b4dad1..d492a2f5ba 100644
--- a/Jellyfin.Api/Controllers/ImageController.cs
+++ b/Jellyfin.Api/Controllers/ImageController.cs
@@ -125,7 +125,13 @@ public class ImageController : BaseJellyfinApiController
         {
             // Handle image/png; charset=utf-8
             var mimeType = Request.ContentType?.Split(';').FirstOrDefault();
-            var userDataPath = Path.Combine(_serverConfigurationManager.ApplicationPaths.UserConfigurationDirectoryPath, user.Username);
+            var userConfigurationDirectoryPath = _serverConfigurationManager.ApplicationPaths.UserConfigurationDirectoryPath;
+            var userDataPath = Path.Combine(userConfigurationDirectoryPath, user.Username);
+            if (!PathHelper.IsContainedIn(userConfigurationDirectoryPath, userDataPath))
+            {
+                return BadRequest("Invalid user.");
+            }
+
             if (user.ProfileImage is not null)
             {
                 await _userManager.ClearProfileImageAsync(user).ConfigureAwait(false);
diff --git a/Jellyfin.Api/Controllers/PluginsController.cs b/Jellyfin.Api/Controllers/PluginsController.cs
index 2c84fde972..79b8b60cb7 100644
--- a/Jellyfin.Api/Controllers/PluginsController.cs
+++ b/Jellyfin.Api/Controllers/PluginsController.cs
@@ -6,6 +6,7 @@ using System.Linq;
 using System.Text.Json;
 using System.Threading.Tasks;
 using Jellyfin.Api.Attributes;
+using Jellyfin.Extensions;
 using Jellyfin.Extensions.Json;
 using MediaBrowser.Common.Api;
 using MediaBrowser.Common.Plugins;
diff --git a/Jellyfin.Server.Implementations/Users/UserManager.cs b/Jellyfin.Server.Implementations/Users/UserManager.cs
index 41648268a9..81408d9aa8 100644
--- a/Jellyfin.Server.Implementations/Users/UserManager.cs
+++ b/Jellyfin.Server.Implementations/Users/UserManager.cs
@@ -911,7 +911,7 @@ namespace Jellyfin.Server.Implementations.Users
 
         internal static void ThrowIfInvalidUsername(string name)
         {
-            if (!string.IsNullOrWhiteSpace(name) && ValidUsernameRegex().IsMatch(name))
+            if (!string.IsNullOrWhiteSpace(name) && ValidUsernameRegex().IsMatch(name) && !string.Equals(name, ".", StringComparison.Ordinal) && !string.Equals(name, "..", StringComparison.Ordinal))
             {
                 return;
             }
diff --git a/MediaBrowser.Controller/ClientEvent/ClientEventLogger.cs b/MediaBrowser.Controller/ClientEvent/ClientEventLogger.cs
index 14dc64dabd..36f0d2195c 100644
--- a/MediaBrowser.Controller/ClientEvent/ClientEventLogger.cs
+++ b/MediaBrowser.Controller/ClientEvent/ClientEventLogger.cs
@@ -1,6 +1,7 @@
 using System;
 using System.IO;
 using System.Threading.Tasks;
+using Jellyfin.Extensions;
 
 namespace MediaBrowser.Controller.ClientEvent
 {
@@ -21,8 +22,15 @@ namespace MediaBrowser.Controller.ClientEvent
         /// <inheritdoc />
         public async Task<string> WriteDocumentAsync(string clientName, string clientVersion, Stream fileContents)
         {
-            var fileName = $"upload_{clientName}_{clientVersion}_{DateTime.UtcNow:yyyyMMddHHmmss}_{Guid.NewGuid():N}.log";
+            var safeClientName = PathHelper.GetSafeLeafFileName(clientName) ?? "unknown-client";
+            var safeClientVersion = PathHelper.GetSafeLeafFileName(clientVersion) ?? "unknown-version";
+            var fileName = $"upload_{safeClientName}_{safeClientVersion}_{DateTime.UtcNow:yyyyMMddHHmmss}_{Guid.NewGuid():N}.log";
             var logFilePath = Path.Combine(_applicationPaths.LogDirectoryPath, fileName);
+            if (!PathHelper.IsContainedIn(_applicationPaths.LogDirectoryPath, logFilePath))
+            {
+                throw new ArgumentException("Path resolved to filename not in log directory");
+            }
+
             var fileStream = new FileStream(logFilePath, FileMode.CreateNew, FileAccess.Write, FileShare.None);
             await using (fileStream.ConfigureAwait(false))
             {
diff --git a/MediaBrowser.MediaEncoding/Attachments/AttachmentExtractor.cs b/MediaBrowser.MediaEncoding/Attachments/AttachmentExtractor.cs
index 9dd3dcecba..12a5ab877c 100644
--- a/MediaBrowser.MediaEncoding/Attachments/AttachmentExtractor.cs
+++ b/MediaBrowser.MediaEncoding/Attachments/AttachmentExtractor.cs
@@ -8,6 +8,7 @@ using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 using AsyncKeyedLock;
+using Jellyfin.Extensions;
 using MediaBrowser.Common.Extensions;
 using MediaBrowser.Controller.Entities;
 using MediaBrowser.Controller.IO;
@@ -101,7 +102,7 @@ namespace MediaBrowser.MediaEncoding.Attachments
             CancellationToken cancellationToken)
         {
             var shouldExtractOneByOne = mediaSource.MediaAttachments.Any(a => !string.IsNullOrEmpty(a.FileName)
-                                                                              && (a.FileName.Contains('/', StringComparison.OrdinalIgnoreCase) || a.FileName.Contains('\\', StringComparison.OrdinalIgnoreCase)));
+                                                                              && !string.Equals(PathHelper.GetSafeLeafFileName(a.FileName), a.FileName, StringComparison.Ordinal));
             if (shouldExtractOneByOne && !inputFile.EndsWith(".mks", StringComparison.OrdinalIgnoreCase))
             {
                 await ExtractAllAttachmentsIndividuallyInternal(
@@ -387,7 +388,9 @@ namespace MediaBrowser.MediaEncoding.Attachments
 
             using (await _semaphoreLocks.LockAsync(attachmentFolderPath, cancellationToken).ConfigureAwait(false))
             {
-                var attachmentPath = _pathManager.GetAttachmentPath(mediaSource.Id, mediaAttachment.FileName ?? mediaAttachment.Index.ToString(CultureInfo.InvariantCulture))!;
+                var indexName = mediaAttachment.Index.ToString(CultureInfo.InvariantCulture);
+                var attachmentPath = _pathManager.GetAttachmentPath(mediaSource.Id, mediaAttachment.FileName ?? indexName)
+                                     ?? _pathManager.GetAttachmentPath(mediaSource.Id, indexName)!;
                 if (!File.Exists(attachmentPath))
                 {
                     await ExtractAttachmentInternal(
diff --git a/MediaBrowser.Providers/Lyric/LyricManager.cs b/MediaBrowser.Providers/Lyric/LyricManager.cs
index 913a104a0d..af31e373ef 100644
--- a/MediaBrowser.Providers/Lyric/LyricManager.cs
+++ b/MediaBrowser.Providers/Lyric/LyricManager.cs
@@ -398,7 +398,7 @@ public class LyricManager : ILyricManager
             {
                 var mediaFolderPath = Path.GetFullPath(Path.Combine(audio.ContainingFolderPath, saveFileName));
                 // TODO: Add some error handling to the API user: return BadRequest("Could not save lyric, bad path.");
-                if (mediaFolderPath.StartsWith(audio.ContainingFolderPath, StringComparison.Ordinal))
+                if (PathHelper.IsContainedIn(audio.ContainingFolderPath, mediaFolderPath))
                 {
                     savePaths.Add(mediaFolderPath);
                 }
@@ -407,7 +407,7 @@ public class LyricManager : ILyricManager
             var internalPath = Path.GetFullPath(Path.Combine(audio.GetInternalMetadataPath(), saveFileName));
 
             // TODO: Add some error to the user: return BadRequest("Could not save lyric, bad path.");
-            if (internalPath.StartsWith(audio.GetInternalMetadataPath(), StringComparison.Ordinal))
+            if (PathHelper.IsContainedIn(audio.GetInternalMetadataPath(), internalPath))
             {
                 savePaths.Add(internalPath);
             }
diff --git a/src/Jellyfin.Extensions/PathHelper.cs b/src/Jellyfin.Extensions/PathHelper.cs
new file mode 100644
index 0000000000..ab74a7749d
--- /dev/null
+++ b/src/Jellyfin.Extensions/PathHelper.cs
@@ -0,0 +1,77 @@
+using System;
+using System.IO;
+
+namespace Jellyfin.Extensions;
+
+/// <summary>
+/// Helpers for safely composing filesystem paths from untrusted input.
+/// </summary>
+/// <remarks>
+/// <see cref="Path.Combine(string, string)"/> has two issues that matter in
+/// any code that joins a trusted directory with an externally-supplied name:
+/// it neither normalises <c>..</c> nor rejects a rooted second argument
+/// (a rooted second arg silently discards the first). Use the helpers below
+/// any time the name comes from media metadata, request input, archive
+/// entries, or any other channel that can be influenced by a third party.
+/// </remarks>
+public static class PathHelper
+{
+    /// <summary>
+    /// Reduces a possibly-untrusted file name to a safe leaf-only name with no
+    /// directory components.
+    /// </summary>
+    /// <param name="fileName">The candidate file name.</param>
+    /// <returns>
+    /// The leaf component of <paramref name="fileName"/>, or <c>null</c> if
+    /// the input has no usable leaf (empty, <c>.</c>, or <c>..</c>).
+    /// </returns>
+    public static string? GetSafeLeafFileName(string? fileName)
+    {
+        if (string.IsNullOrEmpty(fileName))
+        {
+            return null;
+        }
+
+        var leaf = Path.GetFileName(fileName);
+        if (string.IsNullOrEmpty(leaf) || string.Equals(leaf, ".", StringComparison.Ordinal) || string.Equals(leaf, "..", StringComparison.Ordinal))
+        {
+            return null;
+        }
+
+        return leaf;
+    }
+
+    /// <summary>
+    /// Returns whether <paramref name="candidate"/> resolves to a path that
+    /// equals or is contained inside <paramref name="root"/>.
+    /// </summary>
+    /// <param name="root">The directory the candidate must remain inside.</param>
+    /// <param name="candidate">The candidate absolute or relative path.</param>
+    /// <returns><c>true</c> if the candidate is inside or equal to root; otherwise <c>false</c>.</returns>
+    /// <remarks>
+    /// Both arguments are resolved via <see cref="Path.GetFullPath(string)"/>
+    /// so <c>..</c> segments are collapsed before the comparison. The root is
+    /// compared with a trailing directory separator to prevent prefix
+    /// collisions (e.g. <c>/var/data</c> must not be accepted as a parent of
+    /// <c>/var/dataset</c>).
+    /// </remarks>
+    public static bool IsContainedIn(string root, string candidate)
+    {
+        ArgumentException.ThrowIfNullOrEmpty(root);
+        ArgumentException.ThrowIfNullOrEmpty(candidate);
+
+        var fullRoot = Path.GetFullPath(root);
+        var fullCandidate = Path.GetFullPath(candidate);
+
+        if (string.Equals(fullCandidate, fullRoot, StringComparison.Ordinal))
+        {
+            return true;
+        }
+
+        var rootWithSep = fullRoot.EndsWith(Path.DirectorySeparatorChar)
+            ? fullRoot
+            : fullRoot + Path.DirectorySeparatorChar;
+
+        return fullCandidate.StartsWith(rootWithSep, StringComparison.Ordinal);
+    }
+}
diff --git a/tests/Jellyfin.Controller.Tests/ClientEventLoggerTests.cs b/tests/Jellyfin.Controller.Tests/ClientEventLoggerTests.cs
new file mode 100644
index 0000000000..5132e529dd
--- /dev/null
+++ b/tests/Jellyfin.Controller.Tests/ClientEventLoggerTests.cs
@@ -0,0 +1,44 @@
+using System;
+using System.IO;
+using System.Text;
+using System.Threading.Tasks;
+using MediaBrowser.Controller;
+using MediaBrowser.Controller.ClientEvent;
+using Moq;
+using Xunit;
+
+namespace Jellyfin.Controller.Tests
+{
+    public class ClientEventLoggerTests
+    {
+        [Theory]
+        [InlineData("../../../../etc/passwd", "1.0")]
+        [InlineData("..\\..\\windows\\system32", "1.0")]
+        [InlineData("normal-client", "../../../etc/passwd")]
+        [InlineData("/absolute/path", "1.0")]
+        public async Task WriteDocumentAsync_TraversalInput_StaysInsideLogDirectory(string clientName, string clientVersion)
+        {
+            var logDir = Path.Combine(Path.GetTempPath(), "jellyfin-clientlog-test-" + Path.GetRandomFileName());
+            Directory.CreateDirectory(logDir);
+            try
+            {
+                var paths = new Mock<IServerApplicationPaths>();
+                paths.Setup(p => p.LogDirectoryPath).Returns(logDir);
+
+                var logger = new ClientEventLogger(paths.Object);
+                using var contents = new MemoryStream(Encoding.UTF8.GetBytes("payload"));
+
+                var fileName = await logger.WriteDocumentAsync(clientName, clientVersion, contents);
+
+                var resolved = Path.GetFullPath(Path.Combine(logDir, fileName));
+                var rootWithSep = Path.GetFullPath(logDir) + Path.DirectorySeparatorChar;
+                Assert.StartsWith(rootWithSep, resolved, StringComparison.Ordinal);
+                Assert.True(File.Exists(resolved));
+            }
+            finally
+            {
+                Directory.Delete(logDir, recursive: true);
+            }
+        }
+    }
+}
diff --git a/tests/Jellyfin.Extensions.Tests/PathHelperTests.cs b/tests/Jellyfin.Extensions.Tests/PathHelperTests.cs
new file mode 100644
index 0000000000..71fd853ba2
--- /dev/null
+++ b/tests/Jellyfin.Extensions.Tests/PathHelperTests.cs
@@ -0,0 +1,60 @@
+using System.IO;
+using Jellyfin.Extensions;
+using Xunit;
+
+namespace Jellyfin.Extensions.Tests
+{
+    public static class PathHelperTests
+    {
+        [Theory]
+        [InlineData("file.txt", "file.txt")]
+        [InlineData("sub/file.txt", "file.txt")]
+        [InlineData("../../etc/passwd", "passwd")]
+        public static void GetSafeLeafFileName_ReducesToLeaf(string input, string expected)
+        {
+            Assert.Equal(expected, PathHelper.GetSafeLeafFileName(input));
+        }
+
+        [Theory]
+        [InlineData(null)]
+        [InlineData("")]
+        [InlineData(".")]
+        [InlineData("..")]
+        public static void GetSafeLeafFileName_RejectsUnusableLeaf(string? input)
+        {
+            Assert.Null(PathHelper.GetSafeLeafFileName(input));
+        }
+
+        [Fact]
+        public static void IsContainedIn_ChildPath_ReturnsTrue()
+        {
+            var root = Path.Combine(Path.GetTempPath(), "root");
+            var child = Path.Combine(root, "sub", "file.txt");
+            Assert.True(PathHelper.IsContainedIn(root, child));
+        }
+
+        [Fact]
+        public static void IsContainedIn_RootItself_ReturnsTrue()
+        {
+            var root = Path.Combine(Path.GetTempPath(), "root");
+            Assert.True(PathHelper.IsContainedIn(root, root));
+        }
+
+        [Fact]
+        public static void IsContainedIn_TraversalEscape_ReturnsFalse()
+        {
+            var root = Path.Combine(Path.GetTempPath(), "root");
+            var escape = Path.Combine(root, "..", "..", "etc", "passwd");
+            Assert.False(PathHelper.IsContainedIn(root, escape));
+        }
+
+        [Fact]
+        public static void IsContainedIn_SiblingPrefixCollision_ReturnsFalse()
+        {
+            // "/var/data" must not be accepted as a parent of "/var/dataset".
+            var root = Path.Combine(Path.GetTempPath(), "data");
+            var sibling = Path.Combine(Path.GetTempPath(), "dataset", "file.txt");
+            Assert.False(PathHelper.IsContainedIn(root, sibling));
+        }
+    }
+}
diff --git a/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerTests.cs
index 4cea53bd3d..2bf1d1d05b 100644
--- a/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerTests.cs
+++ b/tests/Jellyfin.Server.Implementations.Tests/Users/UserManagerTests.cs
@@ -27,6 +27,8 @@ namespace Jellyfin.Server.Implementations.Tests.Users
         [InlineData(" thishasaspaceatthestart")]
         [InlineData(" thishasaspaceatbothends ")]
         [InlineData(" this has a space at both ends and inbetween ")]
+        [InlineData(".")]
+        [InlineData("..")]
         public void ThrowIfInvalidUsername_WhenInvalidUsername_ThrowsArgumentException(string username)
         {
             Assert.Throws<ArgumentException>(() => UserManager.ThrowIfInvalidUsername(username));