harden: bound docker-compose parsing + validate /config path (#13); XXE-safe camera XML reader (#36)

CHANGELOG.md

@@ -50,6 +50,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Email notifications enforce TLS.** The notification mailer moved off the obsolete `System.Net.Mail.SmtpClient` onto MailKit: port 465 uses implicit TLS and every other port requires STARTTLS, so a misconfigured or downgrade-inducing server fails the send instead of transmitting credentials in cleartext.
 - **Dependabot auto-merge is gated on the PR's head repository, not just the actor.** The auto-merge workflow additionally requires the pull request's head branch to live in this repository, so a fork PR cannot ride the auto-merge path by presenting the bot as author.
 - **Traced SVGs are no longer served as navigable same-origin content.** The Tracing tool wrote its generated SVG under `wwwroot/temp` and pointed both the preview `<img>` and the Download Copy link at that `/temp/<id>.svg` URL. An SVG fetched from a same-origin URL is active content — an embedded `<script>` executes in the app's origin — so that URL was a stored-XSS primitive (reachable if the user opened or navigated to it). The preview and download now use an inert `data:image/svg+xml` URL (script-safe inside `<img>`, and forced to download rather than inline-rendered via the link's `download` attribute), and no SVG is written under `wwwroot/temp`. The SVG-Rasterize tool was reviewed too and is unaffected — it only writes its rasterized PNG/ICO output there, which is inert.
+- **The Jellyfin docker-compose parser bounds its input and validates the config path.** It read the compose file with no size limit and accepted whatever host path was mounted at `/config` — a path that then flows into `Path.Combine` and the sqlite3 cast/crew update. It now rejects a compose file larger than 1 MB before reading it, and rejects a `/config` host path that isn't a plain fully-qualified path (no quotes or control characters). A relative or Unix-style path that couldn't resolve to a local `jellyfin.db` on this host now returns a clear error instead of a silently-wrong path.
+- **Camera/ONVIF XML responses are parsed with a hardened reader (closes an entity-expansion DoS).** The ONVIF SOAP, WS-Discovery, and Hikvision ISAPI responses were parsed with `XDocument.Parse`. Contrary to the common "DTDs are prohibited by default" guidance — which applies to `XmlReader.Create`, **not** to `XDocument.Parse`/`Load(string)` — `Parse` allows DTDs and expands internal entities on .NET 10 (verified). A malicious device answering discovery could feed a billion-laughs-style payload (bounded by the framework's default entity cap, but still a real amplification); external entities were not fetched by default, so file-exfil XXE was not reachable. All three responses now route through one shared `SafeXml.Parse` that sets `DtdProcessing.Prohibit` (no DTD, no entity expansion), a null `XmlResolver`, and a ~1 MB document cap — closing the DoS and hardening against any future XXE regression.
 
 ## [1.2.0] - 2026-06-11
 

src/ControlMenu/Modules/Cameras/Network/HikvisionIsapiClient.cs

@@ -80,7 +80,7 @@ private static async Task<HttpResponseMessage> SendAuthenticatedAsync(
     {
         try
         {
-            var doc = XDocument.Parse(xml);
+            var doc = SafeXml.Parse(xml);
             var root = doc.Root;
             if (root is null) return null;
 

src/ControlMenu/Modules/Cameras/Network/OnvifClient.cs

@@ -107,7 +107,7 @@ private async Task<XDocument> PostAsync(string serviceUrl, XDocument envelope, C
             throw new OnvifAuthenticationException($"HTTP 401 from {serviceUrl}");
 
         XDocument doc;
-        try { doc = XDocument.Parse(body); }
+        try { doc = SafeXml.Parse(body); }
         catch (Exception ex) { throw new OnvifSoapFaultException($"Invalid SOAP response: {ex.Message}"); }
 
         var fault = doc.Descendants(SoapNs + "Fault").FirstOrDefault();

src/ControlMenu/Modules/Cameras/Network/OnvifDiscoveryClient.cs

@@ -138,7 +138,7 @@ private static string BuildProbeEnvelope()
     private static OnvifProbeResponse? TryParseProbeMatch(string sourceIp, string responseXml)
     {
         XDocument doc;
-        try { doc = XDocument.Parse(responseXml); }
+        try { doc = SafeXml.Parse(responseXml); }
         catch (Exception) { return null; }
 
         var probeMatch = doc.Descendants(WsdNs + "ProbeMatch").FirstOrDefault();

src/ControlMenu/Modules/Cameras/Network/SafeXml.cs

@@ -0,0 +1,37 @@
+using System.Xml;
+using System.Xml.Linq;
+
+namespace ControlMenu.Modules.Cameras.Network;
+
+/// <summary>
+/// Parses untrusted XML (camera / device responses) with hardened reader settings: DTDs
+/// prohibited (no entity expansion / no XXE), no external resolver, and a size cap.
+/// <para>
+/// This is a real hardening, not just style. <see cref="XDocument.Parse(string)"/> does NOT
+/// prohibit DTDs by default — that guidance applies to <see cref="XmlReader.Create(System.IO.TextReader)"/>,
+/// not to <c>Parse</c>/<c>Load(string)</c>, which allow the DTD and EXPAND internal entities
+/// (verified on .NET 10). That is a bounded entity-expansion DoS vector reachable from any device
+/// answering camera discovery. (External entities are not fetched by default, so file-exfil XXE was
+/// not reachable.) Routing every camera-response parse through this reader prohibits the DTD
+/// entirely, drops any external resolver, and caps the document size.
+/// </para>
+/// </summary>
+internal static class SafeXml
+{
+    // Cap on a single parsed response document. Camera / ISAPI / WS-Discovery responses are small;
+    // this stops a hostile or runaway body from being materialised.
+    private const int MaxXmlChars = 1_048_576; // ~1 MB
+
+    public static XDocument Parse(string xml)
+    {
+        var settings = new XmlReaderSettings
+        {
+            DtdProcessing = DtdProcessing.Prohibit, // no DTD => no external-entity / XXE expansion
+            XmlResolver = null,                     // never resolve external resources
+            MaxCharactersInDocument = MaxXmlChars,  // size cap
+        };
+        using var stringReader = new StringReader(xml);
+        using var xmlReader = XmlReader.Create(stringReader, settings);
+        return XDocument.Load(xmlReader);
+    }
+}

src/ControlMenu/Modules/Jellyfin/Services/ComposeParser.cs

@@ -8,11 +8,27 @@ public record ComposeParseResult(
 
 public static class ComposeParser
 {
+    // A docker-compose.yml is a small text file; cap the read so a pathological or hostile
+    // file can't be slurped whole into memory.
+    private const long MaxComposeBytes = 1_048_576; // 1 MB
+
     public static ComposeParseResult Parse(string composePath)
     {
         if (!File.Exists(composePath))
             return new(null, null, null, $"File not found: {composePath}");
 
+        long fileLength;
+        try
+        {
+            fileLength = new FileInfo(composePath).Length;
+        }
+        catch (Exception ex)
+        {
+            return new(null, null, null, $"Cannot read file: {ex.Message}");
+        }
+        if (fileLength > MaxComposeBytes)
+            return new(null, null, null, $"Compose file is too large (> 1 MB): {composePath}");
+
         string[] lines;
         try
         {
@@ -111,10 +127,35 @@ public static ComposeParseResult Parse(string composePath)
         if (configHostPath is null)
             return new(containerName, null, null, "No volume mount to /config found in compose file");
 
+        if (!IsValidHostPath(configHostPath))
+            return new(containerName, null, null,
+                $"Unsupported /config host path (must be a fully-qualified path with no quotes or control characters): {configHostPath}");
+
         var dbPath = Path.Combine(configHostPath, "data", "jellyfin.db");
         return new(containerName, configHostPath, dbPath, null);
     }
 
+    /// <summary>
+    /// The /config host path flows to <see cref="Path.Combine(string,string,string)"/> and the
+    /// sqlite3 update sink, so reject anything that isn't a plain fully-qualified path: no quotes
+    /// or control characters, and rooted enough to resolve to a real local jellyfin.db (a relative
+    /// or Unix-style path can't on this host).
+    /// <para>
+    /// A <c>..</c> segment in an otherwise fully-qualified path is intentionally allowed: it just
+    /// normalises to another real local path the user already controls (it's their own compose
+    /// file), and the sqlite3 sink it feeds is structured-arg (no shell, no injection), so there is
+    /// nothing to escalate. Don't "tighten" this to ban <c>..</c> without that context.
+    /// </para>
+    /// </summary>
+    private static bool IsValidHostPath(string path)
+    {
+        if (string.IsNullOrWhiteSpace(path)) return false;
+        if (path.Contains('"') || path.Contains('\'')) return false;
+        foreach (var c in path)
+            if (char.IsControl(c)) return false;
+        return Path.IsPathFullyQualified(path);
+    }
+
     private static int FindMountSeparator(string mount)
     {
         // Find the colon that separates host:container (not the Windows drive letter colon)

tests/ControlMenu.Tests/Modules/Cameras/Network/SafeXmlTests.cs

@@ -0,0 +1,51 @@
+using System.Xml;
+using ControlMenu.Modules.Cameras.Network;
+
+namespace ControlMenu.Tests.Modules.Cameras.Network;
+
+public class SafeXmlTests
+{
+    [Fact]
+    public void Parse_ValidXml_ReturnsDocument()
+    {
+        var doc = SafeXml.Parse("<root><child>hello</child></root>");
+        Assert.Equal("root", doc.Root!.Name.LocalName);
+        Assert.Equal("hello", doc.Root!.Element("child")!.Value);
+    }
+
+    [Fact]
+    public void Parse_OversizedXml_Throws()
+    {
+        var huge = "<root>" + new string('a', 1_100_000) + "</root>"; // > 1 MB
+        Assert.Throws<XmlException>(() => SafeXml.Parse(huge));
+    }
+
+    [Fact]
+    public void Parse_WithInternalDtd_Throws()
+    {
+        var dtd = "<?xml version=\"1.0\"?><!DOCTYPE root [<!ENTITY x \"y\">]><root>&x;</root>";
+        Assert.Throws<XmlException>(() => SafeXml.Parse(dtd));
+    }
+
+    [Fact]
+    public void Parse_WithExternalEntity_Throws_AndNeverResolves()
+    {
+        // XXE attempt. DTD processing is prohibited, so parsing throws before the external
+        // resource is ever fetched.
+        var xxe = "<?xml version=\"1.0\"?><!DOCTYPE root [<!ENTITY xxe SYSTEM \"file:///C:/Windows/win.ini\">]><root>&xxe;</root>";
+        Assert.Throws<XmlException>(() => SafeXml.Parse(xxe));
+    }
+
+    [Fact]
+    public void RawXDocumentParse_ExpandsInternalEntities_WhichIsWhySafeXmlExists()
+    {
+        // Characterizes the framework default this hardening actually guards against:
+        // XDocument.Parse(string) does NOT prohibit DTDs by default — it allows the DTD and
+        // EXPANDS internal entities (the "prohibit by default" guidance applies to
+        // XmlReader.Create, not XDocument.Parse/XElement.Parse). That is a real, bounded
+        // entity-expansion DoS vector. SafeXml.Parse prohibits the DTD, so the same input throws.
+        const string dtd = "<?xml version=\"1.0\"?><!DOCTYPE root [<!ENTITY x \"EXPANDED\">]><root>&x;</root>";
+        Assert.Equal("EXPANDED", System.Xml.Linq.XDocument.Parse(dtd).Root!.Value); // framework default expands
+        Assert.Throws<XmlException>(() => SafeXml.Parse(dtd));                        // SafeXml blocks it
+    }
+}

tests/ControlMenu.Tests/Modules/Jellyfin/ComposeParserTests.cs

@@ -0,0 +1,78 @@
+using ControlMenu.Modules.Jellyfin.Services;
+
+namespace ControlMenu.Tests.Modules.Jellyfin;
+
+public class ComposeParserTests : IDisposable
+{
+    private readonly string _dir;
+
+    public ComposeParserTests()
+    {
+        _dir = Path.Combine(Path.GetTempPath(), "cm-compose-tests", Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(_dir);
+    }
+
+    public void Dispose()
+    {
+        try { Directory.Delete(_dir, recursive: true); } catch { /* best-effort */ }
+    }
+
+    private string WriteCompose(string content)
+    {
+        var path = Path.Combine(_dir, "docker-compose.yml");
+        File.WriteAllText(path, content);
+        return path;
+    }
+
+    [Fact]
+    public void Parse_ValidCompose_ExtractsContainerConfigAndDbPath()
+    {
+        // Windows-rooted host path (tests run on Windows; CM reads the db locally).
+        var path = WriteCompose(
+            "services:\n" +
+            "  jellyfin:\n" +
+            "    container_name: jellyfin\n" +
+            "    volumes:\n" +
+            "      - C:/srv/jellyfin/config:/config\n" +
+            "      - C:/media:/media\n");
+
+        var result = ComposeParser.Parse(path);
+
+        Assert.Null(result.ErrorMessage);
+        Assert.Equal("jellyfin", result.ContainerName);
+        Assert.Equal("C:/srv/jellyfin/config", result.ConfigHostPath);
+        Assert.Equal(Path.Combine("C:/srv/jellyfin/config", "data", "jellyfin.db"), result.DbPath);
+    }
+
+    [Fact]
+    public void Parse_FileOverSizeCap_ReturnsError_WithoutParsing()
+    {
+        // > 1 MB; the size guard must reject before reading the file into memory.
+        var big = "services:\n" + new string('#', 1_100_000) + "\n";
+        var path = WriteCompose(big);
+
+        var result = ComposeParser.Parse(path);
+
+        Assert.NotNull(result.ErrorMessage);
+        Assert.Contains("too large", result.ErrorMessage!);
+        Assert.Null(result.ConfigHostPath);
+    }
+
+    [Fact]
+    public void Parse_RelativeConfigHostPath_ReturnsError()
+    {
+        // A non-fully-qualified host path can't be resolved to the local jellyfin.db, and is
+        // rejected as defense-in-depth before it reaches Path.Combine / the sqlite3 sink.
+        var path = WriteCompose(
+            "services:\n" +
+            "  jellyfin:\n" +
+            "    container_name: jellyfin\n" +
+            "    volumes:\n" +
+            "      - ./config:/config\n");
+
+        var result = ComposeParser.Parse(path);
+
+        Assert.NotNull(result.ErrorMessage);
+        Assert.Null(result.DbPath);
+    }
+}