diff --git a/Emby.Server.Implementations/Library/PathManager.cs b/Emby.Server.Implementations/Library/PathManager.cs index a9b7a1274b..32517b7023 100644 --- a/Emby.Server.Implementations/Library/PathManager.cs +++ b/Emby.Server.Implementations/Library/PathManager.cs @@ -2,10 +2,12 @@ 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; using MediaBrowser.Controller.IO; +using Microsoft.Extensions.Logging; namespace Emby.Server.Implementations.Library; @@ -14,18 +16,22 @@ namespace Emby.Server.Implementations.Library; /// public class PathManager : IPathManager { + private readonly ILogger _logger; private readonly IServerConfigurationManager _config; private readonly IApplicationPaths _appPaths; /// /// Initializes a new instance of the class. /// + /// The logger. /// The server configuration manager. /// The application paths. public PathManager( + ILogger logger, IServerConfigurationManager config, IApplicationPaths appPaths) { + _logger = logger; _config = config; _appPaths = appPaths; } @@ -35,9 +41,16 @@ public class PathManager : IPathManager private string AttachmentCachePath => Path.Combine(_appPaths.DataPath, "attachments"); /// - public string GetAttachmentPath(string mediaSourceId, string fileName) + public string? GetAttachmentPath(string mediaSourceId, string fileName) { - return Path.Combine(GetAttachmentFolderPath(mediaSourceId), fileName); + 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(GetAttachmentFolderPath(mediaSourceId), safeName); } /// diff --git a/Jellyfin.Server/Migrations/Routines/MoveExtractedFiles.cs b/Jellyfin.Server/Migrations/Routines/MoveExtractedFiles.cs index fbf9c16377..961e2f9094 100644 --- a/Jellyfin.Server/Migrations/Routines/MoveExtractedFiles.cs +++ b/Jellyfin.Server/Migrations/Routines/MoveExtractedFiles.cs @@ -181,7 +181,9 @@ public class MoveExtractedFiles : IAsyncMigrationRoutine } } - var newAttachmentPath = _pathManager.GetAttachmentPath(itemIdString, attachment.Filename ?? attachmentIndex.ToString(CultureInfo.InvariantCulture)); + var attachmentIndexName = attachmentIndex.ToString(CultureInfo.InvariantCulture); + var newAttachmentPath = _pathManager.GetAttachmentPath(itemIdString, attachment.Filename ?? attachmentIndexName) + ?? _pathManager.GetAttachmentPath(itemIdString, attachmentIndexName)!; if (File.Exists(newAttachmentPath)) { File.Delete(oldAttachmentPath); diff --git a/MediaBrowser.Controller/IO/IPathManager.cs b/MediaBrowser.Controller/IO/IPathManager.cs index eb67437545..fcb0235573 100644 --- a/MediaBrowser.Controller/IO/IPathManager.cs +++ b/MediaBrowser.Controller/IO/IPathManager.cs @@ -37,8 +37,8 @@ public interface IPathManager /// /// The media source id. /// The attachmentFileName index. - /// The absolute path. - public string GetAttachmentPath(string mediaSourceId, string fileName); + /// The absolute path, or null if cannot be reduced to a safe leaf name. + public string? GetAttachmentPath(string mediaSourceId, string fileName); /// /// Gets the path to the attachment folder. diff --git a/MediaBrowser.MediaEncoding/Attachments/AttachmentExtractor.cs b/MediaBrowser.MediaEncoding/Attachments/AttachmentExtractor.cs index 48a0654bb1..d7e0484982 100644 --- a/MediaBrowser.MediaEncoding/Attachments/AttachmentExtractor.cs +++ b/MediaBrowser.MediaEncoding/Attachments/AttachmentExtractor.cs @@ -6,6 +6,7 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; using AsyncKeyedLock; +using Jellyfin.Extensions; using MediaBrowser.Common.Extensions; using MediaBrowser.Controller.Entities; using MediaBrowser.Controller.IO; @@ -98,9 +99,21 @@ namespace MediaBrowser.MediaEncoding.Attachments MediaSourceInfo mediaSource, CancellationToken cancellationToken) { - var shouldExtractOneByOne = mediaSource.MediaAttachments.Any(a => !string.IsNullOrEmpty(a.FileName) - && (a.FileName.Contains('/', StringComparison.OrdinalIgnoreCase) || a.FileName.Contains('\\', StringComparison.OrdinalIgnoreCase))); - if (shouldExtractOneByOne && !inputFile.EndsWith(".mks", StringComparison.OrdinalIgnoreCase)) + var hasUnsafeFileName = mediaSource.MediaAttachments.Any(a => !IsSafeBulkAttachmentFileName(a.FileName)); + var isMatroskaSubtitles = inputFile.EndsWith(".mks", StringComparison.OrdinalIgnoreCase); + if (hasUnsafeFileName && isMatroskaSubtitles) + { + _logger.LogError( + "Refusing attachment extraction for .mks input {InputFile}: an attachment FileName tag is not a safe leaf name.", + inputFile); + throw new InvalidOperationException( + string.Format( + CultureInfo.InvariantCulture, + "Refusing attachment extraction for .mks input {0}: unsafe attachment FileName tag.", + inputFile)); + } + + if (hasUnsafeFileName) { foreach (var attachment in mediaSource.MediaAttachments) { @@ -241,7 +254,9 @@ namespace MediaBrowser.MediaEncoding.Attachments var attachmentFolderPath = _pathManager.GetAttachmentFolderPath(mediaSource.Id); 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( @@ -341,6 +356,27 @@ namespace MediaBrowser.MediaEncoding.Attachments _logger.LogInformation("ffmpeg attachment extraction completed for {InputPath} to {OutputPath}", inputPath, outputPath); } + /// + /// Decides whether an attachment FILENAME tag is safe to feed into ffmpeg's + /// bulk -dump_attachment:t "" mode, which writes each attachment using + /// its filename tag verbatim relative to the working directory. Anything that + /// could escape the working directory - path separators, "..", or empty + /// leaves - is rerouted to the one-by-one path where the filename is + /// sanitised via . + /// + private static bool IsSafeBulkAttachmentFileName(string? fileName) + { + if (string.IsNullOrEmpty(fileName)) + { + return true; + } + + // PathHelper.GetSafeLeafFileName collapses to the leaf component; + // the bulk path is only safe when the supplied name already _was_ + // that leaf (no separators, no "."/".."). + return PathHelper.GetSafeLeafFileName(fileName) == fileName; + } + /// public void Dispose() { 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); }