From cefa78fc1de2410e5c5c6da5062c98fe98b22d17 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Tue, 25 Aug 2026 11:04:24 +0200 Subject: Prevent SSRF, local file disclosure and DoS via external references in SVG rendering --- Jellyfin.sln | 7 + src/Jellyfin.Drawing.Skia/SkiaEncoder.cs | 20 ++ src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs | 304 +++++++++++++++++++++ .../Jellyfin.Drawing.Skia.Tests.csproj | 26 ++ .../SvgSecurityValidatorTests.cs | 93 +++++++ 5 files changed, 450 insertions(+) create mode 100644 src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs create mode 100644 tests/Jellyfin.Drawing.Skia.Tests/Jellyfin.Drawing.Skia.Tests.csproj create mode 100644 tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs diff --git a/Jellyfin.sln b/Jellyfin.sln index b0d5a5eb47..b666e4ae16 100644 --- a/Jellyfin.sln +++ b/Jellyfin.sln @@ -99,6 +99,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Jellyfin.Database.Implement EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Jellyfin.CodeAnalysis", "src\Jellyfin.CodeAnalysis\Jellyfin.CodeAnalysis.csproj", "{11643D0F-6761-4EF7-AB71-6F9F8DE00714}" EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Jellyfin.Drawing.Skia.Tests", "tests\Jellyfin.Drawing.Skia.Tests\Jellyfin.Drawing.Skia.Tests.csproj", "{E24A279C-9A37-419A-8F9C-853C11FBE753}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -265,6 +267,10 @@ Global {11643D0F-6761-4EF7-AB71-6F9F8DE00714}.Debug|Any CPU.Build.0 = Debug|Any CPU {11643D0F-6761-4EF7-AB71-6F9F8DE00714}.Release|Any CPU.ActiveCfg = Release|Any CPU {11643D0F-6761-4EF7-AB71-6F9F8DE00714}.Release|Any CPU.Build.0 = Release|Any CPU + {E24A279C-9A37-419A-8F9C-853C11FBE753}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {E24A279C-9A37-419A-8F9C-853C11FBE753}.Debug|Any CPU.Build.0 = Debug|Any CPU + {E24A279C-9A37-419A-8F9C-853C11FBE753}.Release|Any CPU.ActiveCfg = Release|Any CPU + {E24A279C-9A37-419A-8F9C-853C11FBE753}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -297,6 +303,7 @@ Global {A5590358-33CC-4B39-BDE7-DC62FEB03C76} = {4C54CE05-69C8-48FA-8785-39F7F6DB1CAD} {8C9F9221-8415-496C-B1F5-E7756F03FA59} = {4C54CE05-69C8-48FA-8785-39F7F6DB1CAD} {11643D0F-6761-4EF7-AB71-6F9F8DE00714} = {C9F0AB5D-F4D7-40C8-A353-3305C86D6D4C} + {E24A279C-9A37-419A-8F9C-853C11FBE753} = {FBBB5129-006E-4AD7-BAD5-8B7CA1D10ED6} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {3448830C-EBDC-426C-85CD-7BBB9651A7FE} diff --git a/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs b/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs index b6d2914efa..64329feabd 100644 --- a/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs +++ b/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs @@ -11,6 +11,7 @@ using MediaBrowser.Controller.Drawing; using MediaBrowser.Model.Drawing; using Microsoft.Extensions.Logging; using SkiaSharp; +using Svg; using Svg.Skia; namespace Jellyfin.Drawing.Skia; @@ -48,6 +49,13 @@ public class SkiaEncoder : IImageEncoder /// public static readonly SKSamplingOptions DefaultSamplingOptions = new SKSamplingOptions(SKFilterMode.Linear, SKMipmapMode.Linear); + static SkiaEncoder() + { + SvgDocument.ResolveExternalElements = ExternalType.None; + SvgDocument.ResolveExternalImages = ExternalType.None; + SvgDocument.ResolveExternalXmlEntites = ExternalType.None; + } + /// /// Initializes a new instance of the class. /// @@ -183,6 +191,12 @@ public class SkiaEncoder : IImageEncoder var extension = Path.GetExtension(path.AsSpan()); if (extension.Equals(".svg", StringComparison.OrdinalIgnoreCase)) { + if (!SvgSecurityValidator.IsSafe(path, _logger)) + { + _logger.LogError("Refusing to determine dimensions for SVG with external references {FilePath}", path); + return default; + } + using var svg = new SKSvg(); try { @@ -445,6 +459,12 @@ public class SkiaEncoder : IImageEncoder throw new FileNotFoundException("File not found", path); } + if (!SvgSecurityValidator.IsSafe(path, _logger)) + { + _logger.LogError("Refusing to render SVG with external references {FilePath}", path); + return null; + } + using var svg = SKSvg.CreateFromFile(path); if (svg.Drawable is null) { diff --git a/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs b/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs new file mode 100644 index 0000000000..354c761eeb --- /dev/null +++ b/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs @@ -0,0 +1,304 @@ +using System; +using System.IO; +using System.IO.Compression; +using System.Runtime.CompilerServices; +using System.Text; +using System.Xml; +using Microsoft.Extensions.Logging; + +[assembly: InternalsVisibleTo("Jellyfin.Drawing.Skia.Tests")] + +namespace Jellyfin.Drawing.Skia; + +/// +/// Validates that an SVG document does not reference external resources before it is rasterized. +/// +internal static class SvgSecurityValidator +{ + // Guards against a chain of nested data:image/svg+xml payloads. + private const int MaxDataUriDepth = 4; + + // Upper bound for a decompressed svgz payload carried inside a data URI, to guard against decompression bombs. + private const int MaxDecompressedBytes = 16 * 1024 * 1024; + + private static readonly XmlReaderSettings _scanSettings = new() + { + DtdProcessing = DtdProcessing.Parse, + XmlResolver = null, + MaxCharactersFromEntities = 1024 * 1024, + IgnoreComments = true, + IgnoreProcessingInstructions = true, + IgnoreWhitespace = true, + CloseInput = false + }; + + /// + /// Determines whether the SVG at the given path is safe to rasterize, i.e. contains no references + /// to external resources. + /// + /// The path to the SVG file. + /// The logger. + /// true if the document is free of external references; otherwise false. + public static bool IsSafe(string path, ILogger logger) + { + try + { + using var stream = File.OpenRead(path); + return IsSafe(stream, logger); + } + catch (IOException ex) + { + logger.LogWarning(ex, "Unable to read SVG {Path} for validation, refusing to render", path); + return false; + } + catch (UnauthorizedAccessException ex) + { + logger.LogWarning(ex, "Unable to read SVG {Path} for validation, refusing to render", path); + return false; + } + } + + /// + /// Determines whether the SVG in the given stream is safe to rasterize. + /// + /// The stream containing the SVG document. + /// The logger. + /// true if the document is free of external references; otherwise false. + public static bool IsSafe(Stream stream, ILogger logger) + => IsSafe(stream, logger, 0); + + private static bool IsSafe(Stream stream, ILogger logger, int depth) + { + try + { + using var reader = XmlReader.Create(stream, _scanSettings); + while (reader.Read()) + { + switch (reader.NodeType) + { + case XmlNodeType.DocumentType: + var subset = reader.Value; + if (!string.IsNullOrEmpty(subset) + && (subset.Contains("SYSTEM", StringComparison.OrdinalIgnoreCase) + || subset.Contains("PUBLIC", StringComparison.OrdinalIgnoreCase))) + { + logger.LogWarning("Refusing to render SVG declaring an external DTD entity"); + return false; + } + + break; + + case XmlNodeType.Element when reader.HasAttributes: + for (var i = 0; i < reader.AttributeCount; i++) + { + reader.MoveToAttribute(i); + var value = reader.Value; + if (reader.LocalName.Equals("href", StringComparison.OrdinalIgnoreCase)) + { + if (!IsReferenceSafe(value, logger, depth)) + { + logger.LogWarning("Refusing to render SVG referencing external resource via href"); + return false; + } + } + else if (HasUnsafeCssReference(value, logger, depth)) + { + logger.LogWarning("Refusing to render SVG referencing external resource via style/url()"); + return false; + } + } + + reader.MoveToElement(); + break; + + case XmlNodeType.Text: + case XmlNodeType.CDATA: + if (HasUnsafeCssReference(reader.Value, logger, depth)) + { + logger.LogWarning("Refusing to render SVG referencing external resource in style block"); + return false; + } + + break; + } + } + + return true; + } + catch (XmlException ex) + { + // Malformed markup, a forbidden DTD construct or an unresolved external entity: refuse to render. + logger.LogWarning(ex, "Refusing to render SVG that could not be safely parsed"); + return false; + } + } + + private static bool IsReferenceSafe(string? value, ILogger logger, int depth) + { + if (string.IsNullOrEmpty(value)) + { + return true; + } + + var trimmed = value.Trim(); + if (trimmed.Length == 0 || trimmed[0] == '#') + { + return true; + } + + if (trimmed.StartsWith("data:", StringComparison.OrdinalIgnoreCase)) + { + return IsDataUriSafe(trimmed, logger, depth); + } + + return false; + } + + private static bool IsDataUriSafe(string dataUri, ILogger logger, int depth) + { + // "data:[][;base64]," (mirrors Svg.Model's data URI parsing). + var comma = dataUri.IndexOf(',', StringComparison.Ordinal); + if (comma < 0) + { + return false; + } + + var header = dataUri[5..comma]; + var segments = header.Split(';'); + var mediaType = segments.Length > 0 && segments[0].Contains('/', StringComparison.Ordinal) + ? segments[0].Trim() + : "text/plain"; + + // Only "image/svg+xml" is re-parsed as SVG by the renderer; any other type is treated as raster data. + if (!mediaType.Equals("image/svg+xml", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + if (depth >= MaxDataUriDepth) + { + logger.LogWarning("Refusing to render SVG with nested data URIs exceeding the allowed depth"); + return false; + } + + var isBase64 = segments.Length > 0 && segments[^1].Trim().Equals("base64", StringComparison.OrdinalIgnoreCase); + try + { + var payload = dataUri[(comma + 1)..]; + byte[] bytes = isBase64 + ? Convert.FromBase64String(payload.Trim()) + : Encoding.UTF8.GetBytes(Uri.UnescapeDataString(payload)); + + if (bytes.Length > 2 && bytes[0] == 0x1F && bytes[1] == 0x8B) + { + bytes = Decompress(bytes); + } + + using var ms = new MemoryStream(bytes, false); + return IsSafe(ms, logger, depth + 1); + } + catch (FormatException ex) + { + logger.LogWarning(ex, "Refusing to render SVG with an undecodable data URI"); + return false; + } + catch (InvalidDataException ex) + { + logger.LogWarning(ex, "Refusing to render SVG with an invalid compressed data URI"); + return false; + } + } + + private static byte[] Decompress(byte[] compressed) + { + using var input = new MemoryStream(compressed, false); + using var gzip = new GZipStream(input, CompressionMode.Decompress); + using var output = new MemoryStream(); + var buffer = new byte[81920]; + var total = 0; + int read; + while ((read = gzip.Read(buffer, 0, buffer.Length)) > 0) + { + total += read; + if (total > MaxDecompressedBytes) + { + throw new InvalidDataException("Compressed data URI exceeds the allowed size"); + } + + output.Write(buffer, 0, read); + } + + return output.ToArray(); + } + + private static bool HasUnsafeCssReference(string? value, ILogger logger, int depth) + { + if (string.IsNullOrEmpty(value)) + { + return false; + } + + var span = value.AsSpan(); + var index = 0; + while (true) + { + var found = span[index..].IndexOf("url(", StringComparison.OrdinalIgnoreCase); + if (found < 0) + { + break; + } + + var start = index + found + 4; + var close = span[start..].IndexOf(')'); + if (close < 0) + { + break; + } + + var target = span.Slice(start, close).Trim(); + target = target.Trim('\''); + target = target.Trim('"').Trim(); + if (!IsReferenceSafe(target.ToString(), logger, depth)) + { + return true; + } + + index = start + close + 1; + if (index >= span.Length) + { + break; + } + } + + // Handle the bare "@import '...';" form (the "@import url(...)" form is covered above). + index = 0; + while (true) + { + var found = span[index..].IndexOf("@import", StringComparison.OrdinalIgnoreCase); + if (found < 0) + { + break; + } + + var rest = span[(index + found + 7)..]; + var quote = rest.IndexOfAny('\'', '"'); + if (quote >= 0) + { + var afterQuote = rest[(quote + 1)..]; + var end = afterQuote.IndexOfAny('\'', '"'); + if (end >= 0 && !IsReferenceSafe(afterQuote[..end].Trim().ToString(), logger, depth)) + { + return true; + } + } + + index = index + found + 7; + if (index >= span.Length) + { + break; + } + } + + return false; + } +} diff --git a/tests/Jellyfin.Drawing.Skia.Tests/Jellyfin.Drawing.Skia.Tests.csproj b/tests/Jellyfin.Drawing.Skia.Tests/Jellyfin.Drawing.Skia.Tests.csproj new file mode 100644 index 0000000000..b6dc5dfb92 --- /dev/null +++ b/tests/Jellyfin.Drawing.Skia.Tests/Jellyfin.Drawing.Skia.Tests.csproj @@ -0,0 +1,26 @@ + + + + + {E24A279C-9A37-419A-8F9C-853C11FBE753} + Exe + + + + + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + + + + + + diff --git a/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs b/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs new file mode 100644 index 0000000000..62f35694f1 --- /dev/null +++ b/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs @@ -0,0 +1,93 @@ +using System.IO; +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; + +namespace Jellyfin.Drawing.Skia.Tests; + +public static class SvgSecurityValidatorTests +{ + public static TheoryData ExternalReferenceSvgs => new() + { + // SSRF via (xlink:href and plain href) + "", + "", + // Local file disclosure + "", + // Memory exhaustion DoS + "", + // external reference + "", + // CSS url() external reference in an attribute + "", + // @import in a style block + "", + // Relative path traversal (resolves against the document location -> local file read) + "", + // XXE via external entity + "]>&xxe;", + // Entity-expansion (billion laughs) denial of service + "]>&f;", + // Nested SVG in a base64 data: URI whose inner document references an external resource + "", + // Nested SVG in a URL-encoded (non-base64) data: URI referencing an external resource + "", + }; + + public static TheoryData SafeSvgs => new() + { + "", + // Same-document fragment references are allowed + "", + // Inline data URIs are allowed + "", + // A DOCTYPE without external entities is allowed + "", + // An internal general entity with no external reference is allowed (and is expanded by the renderer) + "]>", + // A nested data:image/svg+xml payload that is itself self-contained is allowed + "", + }; + + [Theory] + [MemberData(nameof(ExternalReferenceSvgs))] + public static void IsSafe_ExternalReference_ReturnsFalse(string svg) + { + var path = WriteTemp(svg); + try + { + Assert.False(SvgSecurityValidator.IsSafe(path, NullLogger.Instance)); + } + finally + { + File.Delete(path); + } + } + + [Theory] + [MemberData(nameof(SafeSvgs))] + public static void IsSafe_NoExternalReference_ReturnsTrue(string svg) + { + var path = WriteTemp(svg); + try + { + Assert.True(SvgSecurityValidator.IsSafe(path, NullLogger.Instance)); + } + finally + { + File.Delete(path); + } + } + + [Fact] + public static void IsSafe_MissingFile_ReturnsFalse() + { + Assert.False(SvgSecurityValidator.IsSafe(Path.Combine(Path.GetTempPath(), "does-not-exist-" + Path.GetRandomFileName() + ".svg"), NullLogger.Instance)); + } + + private static string WriteTemp(string svg) + { + var path = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName() + ".svg"); + File.WriteAllText(path, svg); + return path; + } +} -- cgit v1.2.3 From 99f21f16627e4899fbdf10aa0fa340b70452561e Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Fri, 28 Aug 2026 07:21:37 +0200 Subject: Apply review suggestions --- src/Jellyfin.Drawing.Skia/SkiaEncoder.cs | 8 +- src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs | 225 ++++++++++++--------- .../SvgSecurityValidatorTests.cs | 14 +- 3 files changed, 144 insertions(+), 103 deletions(-) diff --git a/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs b/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs index 64329feabd..3e353db8de 100644 --- a/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs +++ b/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs @@ -191,9 +191,9 @@ public class SkiaEncoder : IImageEncoder var extension = Path.GetExtension(path.AsSpan()); if (extension.Equals(".svg", StringComparison.OrdinalIgnoreCase)) { - if (!SvgSecurityValidator.IsSafe(path, _logger)) + if (!SvgSecurityValidator.IsSafe(path, out var reason)) { - _logger.LogError("Refusing to determine dimensions for SVG with external references {FilePath}", path); + _logger.LogError("Refusing to determine dimensions for SVG {FilePath}: {Reason}", path, reason); return default; } @@ -459,9 +459,9 @@ public class SkiaEncoder : IImageEncoder throw new FileNotFoundException("File not found", path); } - if (!SvgSecurityValidator.IsSafe(path, _logger)) + if (!SvgSecurityValidator.IsSafe(path, out var reason)) { - _logger.LogError("Refusing to render SVG with external references {FilePath}", path); + _logger.LogError("Refusing to render SVG {FilePath}: {Reason}", path, reason); return null; } diff --git a/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs b/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs index 354c761eeb..f8a1d7d443 100644 --- a/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs +++ b/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs @@ -1,10 +1,11 @@ using System; +using System.Buffers; +using System.Diagnostics.CodeAnalysis; using System.IO; using System.IO.Compression; using System.Runtime.CompilerServices; using System.Text; using System.Xml; -using Microsoft.Extensions.Logging; [assembly: InternalsVisibleTo("Jellyfin.Drawing.Skia.Tests")] @@ -21,6 +22,8 @@ internal static class SvgSecurityValidator // Upper bound for a decompressed svgz payload carried inside a data URI, to guard against decompression bombs. private const int MaxDecompressedBytes = 16 * 1024 * 1024; + private const int DecompressBufferSize = 81920; + private static readonly XmlReaderSettings _scanSettings = new() { DtdProcessing = DtdProcessing.Parse, @@ -37,37 +40,40 @@ internal static class SvgSecurityValidator /// to external resources. /// /// The path to the SVG file. - /// The logger. + /// When this method returns false, the reason the document was rejected. /// true if the document is free of external references; otherwise false. - public static bool IsSafe(string path, ILogger logger) + public static bool IsSafe(string path, [NotNullWhen(false)] out string? reason) { try { using var stream = File.OpenRead(path); - return IsSafe(stream, logger); + reason = Validate(stream, 0); } catch (IOException ex) { - logger.LogWarning(ex, "Unable to read SVG {Path} for validation, refusing to render", path); - return false; + reason = "Unable to read the file for validation: " + ex.Message; } catch (UnauthorizedAccessException ex) { - logger.LogWarning(ex, "Unable to read SVG {Path} for validation, refusing to render", path); - return false; + reason = "Unable to read the file for validation: " + ex.Message; } + + return reason is null; } /// /// Determines whether the SVG in the given stream is safe to rasterize. /// /// The stream containing the SVG document. - /// The logger. + /// When this method returns false, the reason the document was rejected. /// true if the document is free of external references; otherwise false. - public static bool IsSafe(Stream stream, ILogger logger) - => IsSafe(stream, logger, 0); + public static bool IsSafe(Stream stream, [NotNullWhen(false)] out string? reason) + { + reason = Validate(stream, 0); + return reason is null; + } - private static bool IsSafe(Stream stream, ILogger logger, int depth) + private static string? Validate(Stream stream, int depth) { try { @@ -77,194 +83,219 @@ internal static class SvgSecurityValidator switch (reader.NodeType) { case XmlNodeType.DocumentType: + { var subset = reader.Value; if (!string.IsNullOrEmpty(subset) && (subset.Contains("SYSTEM", StringComparison.OrdinalIgnoreCase) || subset.Contains("PUBLIC", StringComparison.OrdinalIgnoreCase))) { - logger.LogWarning("Refusing to render SVG declaring an external DTD entity"); - return false; + return "The document declares an external DTD entity"; } break; + } case XmlNodeType.Element when reader.HasAttributes: + { for (var i = 0; i < reader.AttributeCount; i++) { reader.MoveToAttribute(i); - var value = reader.Value; - if (reader.LocalName.Equals("href", StringComparison.OrdinalIgnoreCase)) + var isHref = reader.LocalName.Equals("href", StringComparison.OrdinalIgnoreCase); + var reason = isHref + ? ValidateReference(reader.Value, depth, "href") + : ValidateCss(reader.Value, depth); + if (reason is not null) { - if (!IsReferenceSafe(value, logger, depth)) - { - logger.LogWarning("Refusing to render SVG referencing external resource via href"); - return false; - } - } - else if (HasUnsafeCssReference(value, logger, depth)) - { - logger.LogWarning("Refusing to render SVG referencing external resource via style/url()"); - return false; + return reason; } } reader.MoveToElement(); break; + } case XmlNodeType.Text: case XmlNodeType.CDATA: - if (HasUnsafeCssReference(reader.Value, logger, depth)) + { + var reason = ValidateCss(reader.Value, depth); + if (reason is not null) { - logger.LogWarning("Refusing to render SVG referencing external resource in style block"); - return false; + return reason; } break; + } } } - return true; + return null; } catch (XmlException ex) { // Malformed markup, a forbidden DTD construct or an unresolved external entity: refuse to render. - logger.LogWarning(ex, "Refusing to render SVG that could not be safely parsed"); - return false; + return "The document could not be safely parsed: " + ex.Message; } } - private static bool IsReferenceSafe(string? value, ILogger logger, int depth) + private static string? ValidateReference(ReadOnlySpan value, int depth, string context) { - if (string.IsNullOrEmpty(value)) - { - return true; - } - var trimmed = value.Trim(); - if (trimmed.Length == 0 || trimmed[0] == '#') + if (trimmed.IsEmpty || trimmed[0] == '#') { - return true; + return null; } if (trimmed.StartsWith("data:", StringComparison.OrdinalIgnoreCase)) { - return IsDataUriSafe(trimmed, logger, depth); + return ValidateDataUri(trimmed, depth, context); } - return false; + return "An external resource is referenced via " + context; } - private static bool IsDataUriSafe(string dataUri, ILogger logger, int depth) + private static string? ValidateDataUri(ReadOnlySpan dataUri, int depth, string context) { // "data:[][;base64]," (mirrors Svg.Model's data URI parsing). - var comma = dataUri.IndexOf(',', StringComparison.Ordinal); + var comma = dataUri.IndexOf(','); if (comma < 0) { - return false; + return "A malformed data URI is referenced via " + context; } var header = dataUri[5..comma]; - var segments = header.Split(';'); - var mediaType = segments.Length > 0 && segments[0].Contains('/', StringComparison.Ordinal) - ? segments[0].Trim() - : "text/plain"; + var firstSeparator = header.IndexOf(';'); + var mediaType = (firstSeparator < 0 ? header : header[..firstSeparator]).Trim(); // Only "image/svg+xml" is re-parsed as SVG by the renderer; any other type is treated as raster data. - if (!mediaType.Equals("image/svg+xml", StringComparison.OrdinalIgnoreCase)) + if (!mediaType.Contains('/') || !mediaType.Equals("image/svg+xml", StringComparison.OrdinalIgnoreCase)) { - return true; + return null; } if (depth >= MaxDataUriDepth) { - logger.LogWarning("Refusing to render SVG with nested data URIs exceeding the allowed depth"); - return false; + return "Nested data URIs exceed the allowed depth"; } - var isBase64 = segments.Length > 0 && segments[^1].Trim().Equals("base64", StringComparison.OrdinalIgnoreCase); + var lastSeparator = header.LastIndexOf(';'); + var isBase64 = lastSeparator >= 0 + && header[(lastSeparator + 1)..].Trim().Equals("base64", StringComparison.OrdinalIgnoreCase); + + var payload = dataUri[(comma + 1)..].Trim(); + byte[]? buffer = null; try { - var payload = dataUri[(comma + 1)..]; - byte[] bytes = isBase64 - ? Convert.FromBase64String(payload.Trim()) - : Encoding.UTF8.GetBytes(Uri.UnescapeDataString(payload)); + int length; + if (isBase64) + { + buffer = ArrayPool.Shared.Rent((payload.Length / 4 * 3) + 3); + if (!Convert.TryFromBase64Chars(payload, buffer, out length)) + { + return "An undecodable data URI is referenced via " + context; + } + } + else + { + var unescaped = Uri.UnescapeDataString(payload.ToString()); + buffer = ArrayPool.Shared.Rent(Encoding.UTF8.GetMaxByteCount(unescaped.Length)); + length = Encoding.UTF8.GetBytes(unescaped, buffer); + } - if (bytes.Length > 2 && bytes[0] == 0x1F && bytes[1] == 0x8B) + if (length > 2 && buffer[0] == 0x1F && buffer[1] == 0x8B) { - bytes = Decompress(bytes); + using var decompressed = Decompress(buffer, length); + return Validate(decompressed, depth + 1); } - using var ms = new MemoryStream(bytes, false); - return IsSafe(ms, logger, depth + 1); + using var stream = new MemoryStream(buffer, 0, length, false); + return Validate(stream, depth + 1); } catch (FormatException ex) { - logger.LogWarning(ex, "Refusing to render SVG with an undecodable data URI"); - return false; + return "An undecodable data URI is referenced via " + context + ": " + ex.Message; } catch (InvalidDataException ex) { - logger.LogWarning(ex, "Refusing to render SVG with an invalid compressed data URI"); - return false; + return "An invalid compressed data URI is referenced via " + context + ": " + ex.Message; + } + finally + { + if (buffer is not null) + { + ArrayPool.Shared.Return(buffer); + } } } - private static byte[] Decompress(byte[] compressed) + private static MemoryStream Decompress(byte[] compressed, int length) { - using var input = new MemoryStream(compressed, false); + using var input = new MemoryStream(compressed, 0, length, false); using var gzip = new GZipStream(input, CompressionMode.Decompress); - using var output = new MemoryStream(); - var buffer = new byte[81920]; - var total = 0; - int read; - while ((read = gzip.Read(buffer, 0, buffer.Length)) > 0) + var output = new MemoryStream(); + var buffer = ArrayPool.Shared.Rent(DecompressBufferSize); + try { - total += read; - if (total > MaxDecompressedBytes) + var total = 0; + int read; + while ((read = gzip.Read(buffer, 0, buffer.Length)) > 0) { - throw new InvalidDataException("Compressed data URI exceeds the allowed size"); - } + total += read; + if (total > MaxDecompressedBytes) + { + throw new InvalidDataException("Compressed data URI exceeds the allowed size"); + } - output.Write(buffer, 0, read); + output.Write(buffer, 0, read); + } + } + catch + { + output.Dispose(); + throw; + } + finally + { + ArrayPool.Shared.Return(buffer); } - return output.ToArray(); + output.Position = 0; + return output; } - private static bool HasUnsafeCssReference(string? value, ILogger logger, int depth) + private static string? ValidateCss(ReadOnlySpan value, int depth) { - if (string.IsNullOrEmpty(value)) + if (value.IsEmpty) { - return false; + return null; } - var span = value.AsSpan(); var index = 0; while (true) { - var found = span[index..].IndexOf("url(", StringComparison.OrdinalIgnoreCase); + var found = value[index..].IndexOf("url(", StringComparison.OrdinalIgnoreCase); if (found < 0) { break; } var start = index + found + 4; - var close = span[start..].IndexOf(')'); + var close = value[start..].IndexOf(')'); if (close < 0) { break; } - var target = span.Slice(start, close).Trim(); + var target = value.Slice(start, close).Trim(); target = target.Trim('\''); target = target.Trim('"').Trim(); - if (!IsReferenceSafe(target.ToString(), logger, depth)) + var reason = ValidateReference(target, depth, "url()"); + if (reason is not null) { - return true; + return reason; } index = start + close + 1; - if (index >= span.Length) + if (index >= value.Length) { break; } @@ -274,31 +305,35 @@ internal static class SvgSecurityValidator index = 0; while (true) { - var found = span[index..].IndexOf("@import", StringComparison.OrdinalIgnoreCase); + var found = value[index..].IndexOf("@import", StringComparison.OrdinalIgnoreCase); if (found < 0) { break; } - var rest = span[(index + found + 7)..]; + var rest = value[(index + found + 7)..]; var quote = rest.IndexOfAny('\'', '"'); if (quote >= 0) { var afterQuote = rest[(quote + 1)..]; var end = afterQuote.IndexOfAny('\'', '"'); - if (end >= 0 && !IsReferenceSafe(afterQuote[..end].Trim().ToString(), logger, depth)) + if (end >= 0) { - return true; + var reason = ValidateReference(afterQuote[..end], depth, "@import"); + if (reason is not null) + { + return reason; + } } } index = index + found + 7; - if (index >= span.Length) + if (index >= value.Length) { break; } } - return false; + return null; } } diff --git a/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs b/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs index 62f35694f1..30b7983ece 100644 --- a/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs +++ b/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs @@ -1,5 +1,4 @@ using System.IO; -using Microsoft.Extensions.Logging.Abstractions; using Xunit; namespace Jellyfin.Drawing.Skia.Tests; @@ -31,6 +30,8 @@ public static class SvgSecurityValidatorTests "", // Nested SVG in a URL-encoded (non-base64) data: URI referencing an external resource "", + // Nested gzip-compressed (svgz) data: URI whose inner document references an external resource + "", }; public static TheoryData SafeSvgs => new() @@ -46,6 +47,8 @@ public static class SvgSecurityValidatorTests "]>", // A nested data:image/svg+xml payload that is itself self-contained is allowed "", + // A self-contained gzip-compressed (svgz) data: URI is allowed + "", }; [Theory] @@ -55,7 +58,8 @@ public static class SvgSecurityValidatorTests var path = WriteTemp(svg); try { - Assert.False(SvgSecurityValidator.IsSafe(path, NullLogger.Instance)); + Assert.False(SvgSecurityValidator.IsSafe(path, out var reason)); + Assert.NotNull(reason); } finally { @@ -70,7 +74,8 @@ public static class SvgSecurityValidatorTests var path = WriteTemp(svg); try { - Assert.True(SvgSecurityValidator.IsSafe(path, NullLogger.Instance)); + Assert.True(SvgSecurityValidator.IsSafe(path, out var reason)); + Assert.Null(reason); } finally { @@ -81,7 +86,8 @@ public static class SvgSecurityValidatorTests [Fact] public static void IsSafe_MissingFile_ReturnsFalse() { - Assert.False(SvgSecurityValidator.IsSafe(Path.Combine(Path.GetTempPath(), "does-not-exist-" + Path.GetRandomFileName() + ".svg"), NullLogger.Instance)); + Assert.False(SvgSecurityValidator.IsSafe(Path.Combine(Path.GetTempPath(), "does-not-exist-" + Path.GetRandomFileName() + ".svg"), out var reason)); + Assert.NotNull(reason); } private static string WriteTemp(string svg) -- cgit v1.2.3