Sanitize media attachment and lyric paths against traversal

4 files changed, 81 insertions, 5 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/MediaBrowser.MediaEncoding/Attachments/AttachmentExtractor.cs b/MediaBrowser.MediaEncoding/Attachments/AttachmentExtractor.cs
index 9dd3dcecba..aff4af9581 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)));
+                                                                              && PathHelper.GetSafeLeafFileName(a.FileName) != a.FileName);
             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/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));
+        }
+    }
+}