fix(monitor): isolate local icon fetching and image decoding

2026-11-02 10:52:22 +00:00 · 2026-10-19 17:44:53 +00:00
parent 361f26bf70
commit d7cc914c35
5 changed files with 401 additions and 31 deletions
@@ -1,6 +1,7 @@
 """Explicit LXC icon lookup. No guest execution, background scan or app writes.

-Untrusted HTTP and image parsing run in a bounded child. Cache objects are
+Untrusted HTTP and image parsing run in an unprivileged bounded child. Only
+the target CT's pinned network namespace may be used for HTTP. Cache objects are
 immutable: fetching a draft cannot overwrite a registered application's icon.
 """
 from __future__ import annotations
@@ -15,6 +16,7 @@ import json
 import os
 from pathlib import Path
 import re
+import signal
 import socket
 import ssl
 import subprocess
@@ -30,6 +32,7 @@ from html.parser import HTMLParser
 CACHE = Path('/etc/proxmenux/apps/logo-cache')
 APPS = Path('/etc/proxmenux/apps')
 CONFIG = Path('/etc/pve/lxc')
+PROC = Path('/proc')
 MAX_BYTES = 256 * 1024
 MAX_ENTRIES = 32
 MAX_DIMENSION = 2048
@@ -37,10 +40,13 @@ MAX_PIXELS = 1024 * 1024
 WORKER_TIMEOUT = 8
 UNUSED_TTL = 24 * 3600
 SLOTS = threading.BoundedSemaphore(4)
+DECODERS = threading.BoundedSemaphore(4)
 TOKEN = re.compile(r'^[a-f0-9]{32}$')
 URL = re.compile(r'^/api/vms/([1-9][0-9]*)/apps/logo-cache/([a-f0-9]{32})$')
 MIMES = {'image/png', 'image/jpeg', 'image/gif', 'image/webp',
         'image/x-icon', 'image/vnd.microsoft.icon', 'image/svg+xml'}
+RASTER_FORMATS = {'image/png': 'PNG', 'image/jpeg': 'JPEG', 'image/gif': 'GIF',
+                 'image/webp': 'WEBP', 'image/x-icon': 'ICO', 'image/vnd.microsoft.icon': 'ICO'}


 class IconError(ValueError):
@@ -106,6 +112,62 @@ def _origin(vmid, payload):
    return f"{payload['scheme']}://{'[' + host + ']' if ':' in host else host}:{payload['port']}"


+def _init_pid(vmid):
+    value = _run(['lxc-info', '-n', str(vmid), '-pH']).strip()
+    if not re.fullmatch(r'[1-9][0-9]{0,9}', value) or int(value) <= 1:
+        raise IconError('container init process unavailable')
+    return int(value)
+
+
+def _process_start(pid):
+    # comm may contain spaces and parentheses; field 22 follows the last ')'.
+    value = (PROC / str(pid) / 'stat').read_text().rsplit(')', 1)[1].split()[19]
+    if not value.isdecimal():
+        raise IconError('container process identity unavailable')
+    return value
+
+
+def _ns_id(info):
+    return info.st_dev, info.st_ino
+
+
+def _network_unchanged(vmid, identity):
+    pid, started, namespace = identity
+    return (_init_pid(vmid) == pid and _process_start(pid) == started
+            and _ns_id((PROC / str(pid) / 'ns/net').stat()) == namespace)
+
+
+@contextlib.contextmanager
+def _network(vmid):
+    """Pin nsfs before launching; revalidate PID/start time to reject PID reuse."""
+    fd = None
+    try:
+        pid = _init_pid(vmid)
+        started = _process_start(pid)
+        fd = os.open(PROC / str(pid) / 'ns/net', os.O_RDONLY | os.O_CLOEXEC)
+        identity = pid, started, _ns_id(os.fstat(fd))
+        if (identity[2] == _ns_id((PROC / 'self/ns/net').stat())
+                or not _network_unchanged(vmid, identity)):
+            raise IconError('container network is not isolated or changed')
+        yield fd, identity
+    except (OSError, IndexError, ValueError) as exc:
+        raise IconError('container network unavailable') from exc
+    finally:
+        if fd is not None:
+            os.close(fd)
+
+
+def _owns_address(value):
+    """Called in the CT network namespace, never trust lxc-info as a target."""
+    try:
+        interfaces = json.loads(_run(['/usr/sbin/ip', '-json', 'address', 'show']))
+        owned = {str(ipaddress.ip_address(a['local'])) for interface in interfaces
+                 for a in interface.get('addr_info', []) if 'local' in a}
+        return str(ipaddress.ip_address(value)) in owned
+    except (ValueError, TypeError, KeyError, AttributeError) as exc:
+        raise IconError('container address ownership unavailable') from exc
+
+
 def _same_origin(left, right):
    try:
        a, b = urllib.parse.urlsplit(left), urllib.parse.urlsplit(right)
@@ -200,7 +262,7 @@ def _svg(body):
                raster = base64.b64decode(match[2], validate=True)
            except ValueError as exc:
                raise IconError('invalid embedded image') from exc
-            raster, _mime = normalize(raster, match[1])
+            raster, _mime = _normalize(raster, match[1])
            embedded = 'data:image/png;base64,' + base64.b64encode(raster).decode()
        out = xml.Element('{' + ns + '}' + name)
        for key, value in node.attrib.items():
@@ -243,16 +305,29 @@ def _svg(body):


 def normalize(body, mime):
+    if not isinstance(body, bytes) or not 0 < len(body) <= MAX_BYTES or mime not in MIMES:
+        raise IconError('invalid icon type or size')
+    # Cache GETs, including legacy data, use the same bounded isolation even
+    # when the Monitor itself has been configured to run without root.
+    if not DECODERS.acquire(timeout=WORKER_TIMEOUT):
+        raise IconError('local icon decoder busy')
+    try:
+        return _decode_worker(body, mime)
+    finally:
+        DECODERS.release()
+
+
+def _normalize(body, mime):
+    if os.geteuid() == 0:
+        raise IconError('image decoding as root is forbidden')
    if not isinstance(body, bytes) or not 0 < len(body) <= MAX_BYTES or mime not in MIMES:
        raise IconError('invalid icon type or size')
    if mime == 'image/svg+xml':
        return _svg(body)
    from PIL import Image
-    expected = {'image/png': 'PNG', 'image/jpeg': 'JPEG', 'image/gif': 'GIF',
-                'image/webp': 'WEBP', 'image/x-icon': 'ICO', 'image/vnd.microsoft.icon': 'ICO'}
    try:
-        with Image.open(io.BytesIO(body)) as picture:
-            if picture.format != expected[mime]:
+        with Image.open(io.BytesIO(body), formats=[RASTER_FORMATS[mime]]) as picture:
+            if picture.format != RASTER_FORMATS[mime]:
                raise IconError('image format does not match content type')
            w, h = picture.size
            if not 0 < w <= MAX_DIMENSION or not 0 < h <= MAX_DIMENSION or w * h > MAX_PIXELS:
@@ -278,6 +353,8 @@ def _fetch(origin):
            or parsed.username is not None or parsed.password is not None or parsed.path
            or parsed.query or parsed.fragment or not parsed.port):
        raise IconError('invalid local origin')
+    if not _owns_address(parsed.hostname):
+        raise IconError('selected address does not belong to this container')
    with _open(origin + '/', page=True) as response:
        final = response.geturl()
        if not _same_origin(origin, final) or response.headers.get_content_type() not in ('text/html', 'application/xhtml+xml'):
@@ -293,22 +370,72 @@ def _fetch(origin):
    with _open(icon_url) as response:
        if response.geturl() != icon_url:
            raise IconError('icon redirect not permitted')
-        return normalize(_read(response), response.headers.get_content_type())
+        return _normalize(_read(response), response.headers.get_content_type())


-def _worker(origin):
+def _worker_command(arguments, network_fd):
+    # No guest binaries, rootfs, environment, user namespace or mount namespace.
+    # setpriv execs the host Python only after dropping credentials/capabilities.
+    command = ['/usr/bin/setpriv', '--no-new-privs']
+    if os.geteuid() == 0:
+        command += ['--reuid=65534', '--regid=65534', '--clear-groups',
+                    '--bounding-set=-all', '--inh-caps=-all', '--ambient-caps=-all']
+    command += ['--', sys.executable, '-B', str(Path(__file__).resolve()), *arguments]
+    if network_fd is not None:
+        return ['/usr/bin/nsenter', f'--net=/proc/self/fd/{network_fd}', '--no-fork', '--', *command]
+    if os.geteuid() == 0:
+        # Legacy/cache decoding cannot make any network request, even if a
+        # decoder is compromised. No CT probe is needed for a cache-only GET.
+        return ['/usr/bin/unshare', '--net', '--', *command]
+    return command
+
+
+def _run_worker(arguments, *, network_fd=None, data=None):
+    process = None
    try:
-        result = subprocess.run([sys.executable, str(Path(__file__).resolve()), '--fetch', origin],
-            capture_output=True, timeout=WORKER_TIMEOUT)
-        if result.returncode or len(result.stdout) > MAX_BYTES * 2:
-            raise IconError('local icon unavailable')
-        data = json.loads(result.stdout)
-        body = base64.b64decode(data['body'], validate=True)
-        if not body or len(body) > MAX_BYTES or data['mime'] not in ('image/png', 'image/svg+xml'):
-            raise IconError('invalid image worker response')
-        return body, data['mime']
+        with tempfile.TemporaryFile() as output:
+            process = subprocess.Popen(_worker_command(arguments, network_fd),
+                stdin=subprocess.PIPE if data is not None else subprocess.DEVNULL,
+                stdout=output, stderr=subprocess.DEVNULL, cwd='/', start_new_session=True,
+                pass_fds=() if network_fd is None else (network_fd,))
+            process.communicate(input=data, timeout=WORKER_TIMEOUT)
+            output.seek(0)
+            raw = output.read(MAX_BYTES * 2 + 1)
+            if process.returncode or len(raw) > MAX_BYTES * 2:
+                raise IconError('local icon unavailable')
+            response = json.loads(raw)
+            body = base64.b64decode(response['body'], validate=True)
+            if not body or len(body) > MAX_BYTES or response['mime'] not in ('image/png', 'image/svg+xml'):
+                raise IconError('invalid image worker response')
+            return body, response['mime']
    except (OSError, subprocess.SubprocessError, ValueError, KeyError, TypeError) as exc:
        raise IconError('local icon unavailable') from exc
+    finally:
+        if process is not None:
+            # The nsenter/setpriv chain execs without forking; also terminate
+            # any descendants a faulty decoder could have left behind.
+            try:
+                os.killpg(process.pid, signal.SIGKILL)
+            except ProcessLookupError:
+                pass
+            process.wait()
+
+
+def _worker(origin, network_fd):
+    return _run_worker(['--fetch', origin, str(network_fd)], network_fd=network_fd)
+
+
+def _decode_worker(body, mime):
+    data = json.dumps({'body': base64.b64encode(body).decode(), 'mime': mime}).encode()
+    return _run_worker(['--decode'], data=data)
+
+
+def _assert_worker():
+    fields = dict(line.split(':', 1) for line in Path('/proc/self/status').read_text().splitlines() if ':' in line)
+    if (os.geteuid() == 0 or os.getegid() == 0 or 0 in os.getgroups()
+            or fields.get('NoNewPrivs', '').strip() != '1'
+            or any(int(fields.get(key, '1').strip(), 16) for key in ('CapEff', 'CapPrm', 'CapAmb'))):
+        raise IconError('image worker is not isolated')


@contextlib.contextmanager
@@ -408,11 +535,11 @@ def discover(vmid, payload):
        raise IconError('local icon lookup busy')
    try:
        origin = _origin(vmid, payload)
-        body, mime = _worker(origin)
-        # Confirm that the guest still owns the chosen address before caching.
-        if _origin(vmid, payload) != origin:
-            raise IconError('container address changed')
-        return {'logo_url': store(vmid, body, mime)}
+        with _network(vmid) as (network_fd, identity):
+            body, mime = _worker(origin, network_fd)
+            if _origin(vmid, payload) != origin or not _network_unchanged(vmid, identity):
+                raise IconError('container address or network changed')
+            return {'logo_url': store(vmid, body, mime)}
    except OSError as exc:
        raise IconError('could not cache the icon') from exc
    finally:
@@ -449,10 +576,23 @@ if __name__ == '__main__':
    import resource
    resource.setrlimit(resource.RLIMIT_AS, (512 * 1024 * 1024,) * 2)
    resource.setrlimit(resource.RLIMIT_CPU, (4, 4))
+    resource.setrlimit(resource.RLIMIT_FSIZE, (MAX_BYTES * 2, MAX_BYTES * 2))
    try:
-        if len(sys.argv) != 3 or sys.argv[1] != '--fetch':
+        _assert_worker()
+        if len(sys.argv) == 4 and sys.argv[1] == '--fetch':
+            fd = int(sys.argv[3])
+            if _ns_id(os.fstat(fd)) != _ns_id(Path('/proc/self/ns/net').stat()):
+                raise IconError('image worker network mismatch')
+            os.close(fd)
+            body, mime = _fetch(sys.argv[2])
+        elif len(sys.argv) == 2 and sys.argv[1] == '--decode':
+            raw = sys.stdin.buffer.read(MAX_BYTES * 2 + 1)
+            if len(raw) > MAX_BYTES * 2:
+                raise IconError('image input too large')
+            data = json.loads(raw)
+            body, mime = _normalize(base64.b64decode(data['body'], validate=True), data['mime'])
+        else:
            raise IconError('invalid worker arguments')
-        body, mime = _fetch(sys.argv[2])
        print(json.dumps({'mime': mime, 'body': base64.b64encode(body).decode()}))
    except Exception:
        sys.exit(1)