From 46c189fcede31983e50aaaf04ae1b1779d079162 Mon Sep 17 00:00:00 2001
From: Josh Hawkins <32435876+hawkeye217@users.noreply.github.com>
Date: Sat, 29 Aug 2026 17:26:17 -0500
Subject: [PATCH] harden against symlink attacks

/config is owned by the unprivileged runtime user after the ownership sweep, so root operations on files there could be redirected by a planted symlink.

- go2rtc HomeKit setup: replace the root yq/jq normalization and chown with an O_NOFOLLOW helper (prepare_homekit.py), so a symlink at go2rtc_homekit.yml can't redirect a root write or chown onto another file
- go2rtc binary override: ignore /config/go2rtc whenever the service runs as root, so a planted binary can't exec as root under FRIGATE_ROOT_SERVICES
- sweep sentinel: read and write it through safe-sentinel, which trusts only a root-owned regular file and never follows a symlink, so it can't be forged to skip the migration or symlinked to clobber a root file
- ownership sweep: chown with -execdir so a parent directory swapped for a symlink mid-walk can't redirect the chown out of the volume
- validate inputs: restrict DEVICE_ACL_PATHS to /dev, require nonzero numeric EXTRA_GROUPS, and reject PUID/PGID that collide with the go2rtc ids
- docs: correct the TLS key ownership note to match what actually happens
---
 .../rootfs/etc/s6-overlay/s6-rc.d/go2rtc/run  | 54 ++---------
 .../etc/s6-overlay/s6-rc.d/init-devices/run   |  9 +-
 .../etc/s6-overlay/s6-rc.d/init-usermod/run   | 13 +++
 .../main/rootfs/usr/local/bin/fix-ownership   | 20 ++--
 .../main/rootfs/usr/local/bin/safe-sentinel   | 74 ++++++++++++++
 .../usr/local/go2rtc/prepare_homekit.py       | 97 +++++++++++++++++++
 docs/docs/configuration/non_root.md           |  2 +-
 7 files changed, 214 insertions(+), 55 deletions(-)
 create mode 100755 docker/main/rootfs/usr/local/bin/safe-sentinel
 create mode 100644 docker/main/rootfs/usr/local/go2rtc/prepare_homekit.py

diff --git a/docker/main/rootfs/etc/s6-overlay/s6-rc.d/go2rtc/run b/docker/main/rootfs/etc/s6-overlay/s6-rc.d/go2rtc/run
index 50fe9a30de..4a193dad13 100755
--- a/docker/main/rootfs/etc/s6-overlay/s6-rc.d/go2rtc/run
+++ b/docker/main/rootfs/etc/s6-overlay/s6-rc.d/go2rtc/run
@@ -57,42 +57,6 @@ function set_libva_version() {
     export LIBAVFORMAT_VERSION_MAJOR
 }
 
-function setup_homekit_config() {
-    local config_path="$1"
-
-    if [[ ! -f "${config_path}" ]]; then
-        echo "[INFO] Creating empty config file for HomeKit..."
-        : > "${config_path}"
-    fi
-
-    # Convert YAML to JSON for jq processing
-    local temp_json="/tmp/cache/homekit_config.json"
-    yq eval -o=json "${config_path}" > "${temp_json}" 2>/dev/null || {
-        echo "[WARNING] Failed to convert HomeKit config to JSON, skipping cleanup"
-        return 0
-    }
-
-    # Use jq to extract the homekit section, if it exists
-    local homekit_json
-    homekit_json=$(jq '
-        if has("homekit") then {homekit: .homekit} else null end
-    ' "${temp_json}" 2>/dev/null) || homekit_json="null"
-
-    # If no homekit section, write an empty config file
-    if [[ "${homekit_json}" == "null" ]]; then
-        : > "${config_path}"
-    else
-        # Convert homekit JSON back to YAML and write to the config file
-        echo "${homekit_json}" | yq eval -P - > "${config_path}" 2>/dev/null || {
-            echo "[WARNING] Failed to convert cleaned config to YAML, creating minimal config"
-            : > "${config_path}"
-        }
-    fi
-
-    # Clean up temp files
-    rm -f "${temp_json}"
-}
-
 set_libva_version
 
 if [[ -f "/dev/shm/go2rtc.yaml" ]]; then
@@ -113,21 +77,23 @@ else
     echo "[WARNING] Unable to remove existing go2rtc config. Changes made to your frigate config file may not be recognized. Please remove the /dev/shm/go2rtc.yaml from your docker host manually."
 fi
 
-# HomeKit configuration persistence setup
+# HomeKit persistence. The helper is symlink-safe; hand off to go2rtc only when dropping.
 readonly homekit_config_path="/config/go2rtc_homekit.yml"
-setup_homekit_config "${homekit_config_path}"
-
 if [[ "$(id -u)" -eq 0 && "$runs_as_root" -eq 0 ]]; then
+    python3 /usr/local/go2rtc/prepare_homekit.py "${homekit_config_path}" --chown
     chown go2rtc:go2rtc /dev/shm/go2rtc.yaml 2>/dev/null || true
-    # go2rtc rewrites this in place, so owning the file is enough. Tolerated so
-    # a chown-refusing mount (NFS root_squash) degrades pairing, not the service
-    chown go2rtc:frigate-data "${homekit_config_path}" 2>/dev/null && chmod 664 "${homekit_config_path}" 2>/dev/null || \
-        echo "[WARN] Could not hand ${homekit_config_path} to the go2rtc user; HomeKit pairing changes may not persist"
+else
+    python3 /usr/local/go2rtc/prepare_homekit.py "${homekit_config_path}"
 fi
 
 readonly config_path="/config"
 
-if [[ -x "${config_path}/go2rtc" ]]; then
+# frigate owns /config after the sweep, so a /config/go2rtc override must not
+# be honored while this service runs as root
+if [[ "$runs_as_root" -eq 1 && -x "${config_path}/go2rtc" ]]; then
+  echo "[WARN] Ignoring '${config_path}/go2rtc' because this service is running as root; using the embedded binary"
+  readonly binary_path="/usr/local/go2rtc/bin/go2rtc"
+elif [[ -x "${config_path}/go2rtc" ]]; then
   readonly binary_path="${config_path}/go2rtc"
   echo "[WARN] Using go2rtc binary from '${binary_path}' instead of the embedded one"
 else
diff --git a/docker/main/rootfs/etc/s6-overlay/s6-rc.d/init-devices/run b/docker/main/rootfs/etc/s6-overlay/s6-rc.d/init-devices/run
index e145072c8d..b1591a1e1f 100755
--- a/docker/main/rootfs/etc/s6-overlay/s6-rc.d/init-devices/run
+++ b/docker/main/rootfs/etc/s6-overlay/s6-rc.d/init-devices/run
@@ -41,9 +41,14 @@ device_globs=(
 IFS=',' read -ra extra_globs <<< "${DEVICE_ACL_PATHS:-}"
 for extra in "${extra_globs[@]}"; do
     extra="${extra//[[:space:]]/}"
-    if [[ -n "$extra" ]]; then
-        device_globs+=("$extra")
+    if [[ -z "$extra" ]]; then
+        continue
     fi
+    if [[ "$extra" != /dev/* || "$extra" == *..* ]]; then
+        echo "[ERROR] DEVICE_ACL_PATHS entries must be under /dev, got '${extra}'" >&2
+        exit 1
+    fi
+    device_globs+=("$extra")
 done
 
 granted=0
diff --git a/docker/main/rootfs/etc/s6-overlay/s6-rc.d/init-usermod/run b/docker/main/rootfs/etc/s6-overlay/s6-rc.d/init-usermod/run
index 98000f1917..ab93a6c9de 100755
--- a/docker/main/rootfs/etc/s6-overlay/s6-rc.d/init-usermod/run
+++ b/docker/main/rootfs/etc/s6-overlay/s6-rc.d/init-usermod/run
@@ -53,6 +53,15 @@ if [[ "$puid" -eq 0 || "$pgid" -eq 0 ]]; then
     exit 1
 fi
 
+# Colliding with the go2rtc ids would merge the two users and collapse the
+# separation between the main process and the network-facing restreamer.
+go2rtc_uid="$(id -u go2rtc)"
+go2rtc_gid="$(id -g go2rtc)"
+if [[ "$puid" -eq "$go2rtc_uid" || "$pgid" -eq "$go2rtc_gid" ]]; then
+    echo "[ERROR] PUID/PGID must not equal the go2rtc service ids (${go2rtc_uid}:${go2rtc_gid})." >&2
+    exit 1
+fi
+
 current_uid="$(id -u frigate)"
 current_gid="$(id -g frigate)"
 
@@ -71,6 +80,10 @@ fi
 # EXTRA_GROUPS: numeric host GIDs granting device access (e.g. host render/video)
 if [[ -n "${EXTRA_GROUPS:-}" ]]; then
     for gid in ${EXTRA_GROUPS//,/ }; do
+        if ! [[ "$gid" =~ ^[0-9]+$ ]] || [[ "$gid" -eq 0 ]]; then
+            echo "[ERROR] EXTRA_GROUPS must be nonzero numeric GIDs, got '${gid}'" >&2
+            exit 1
+        fi
         if ! getent group "$gid" >/dev/null; then
             groupadd -o -g "$gid" "frigate-extra-${gid}"
         fi
diff --git a/docker/main/rootfs/usr/local/bin/fix-ownership b/docker/main/rootfs/usr/local/bin/fix-ownership
index 26ca7bf07d..b6d9ce432d 100755
--- a/docker/main/rootfs/usr/local/bin/fix-ownership
+++ b/docker/main/rootfs/usr/local/bin/fix-ownership
@@ -70,11 +70,14 @@ if [[ -n "$mode" ]]; then
     sentinel_content="${sentinel_content}:${mode}"
 fi
 
-# A dry run always inspects: the sentinel records what a past sweep did, not
-# what the volume looks like now, and reporting from it would hide later drift.
-if [[ "$dry_run" -eq 0 && -n "$sentinel" && -f "$sentinel" && "$(cat "$sentinel")" == "$sentinel_content" ]]; then
-    echo "[INFO] fix-ownership: ${target_uid}:${target_gid} (schema ${schema}) already applied, skipping"
-    exit 0
+# safe-sentinel reports only a root-owned regular file, so a forged or
+# symlinked sentinel in the runtime-user-owned /config can't suppress the sweep
+if [[ "$dry_run" -eq 0 && -n "$sentinel" ]]; then
+    if existing=$(/usr/local/bin/safe-sentinel read "$sentinel" 2>/dev/null) && \
+        [[ "$existing" == "$sentinel_content" ]]; then
+        echo "[INFO] fix-ownership: ${target_uid}:${target_gid} (schema ${schema}) already applied, skipping"
+        exit 0
+    fi
 fi
 
 # A sweep that could not chown everything must not be recorded as complete:
@@ -122,10 +125,11 @@ for path in "$@"; do
         continue
     fi
 
-    # -print feeds the progress counter; -exec {} + keeps the chown batched
+    # -execdir chowns from the entry's own directory, so a parent swapped for a
+    # symlink mid-walk can't redirect the chown out of the volume
     started=$SECONDS
     if find "$path" -name lost+found -prune -o \( -not -uid "$target_uid" -o -not -gid "$target_gid" \) \
-        -print -exec chown -h "${target_uid}:${target_gid}" {} + \
+        -print -execdir chown -h "${target_uid}:${target_gid}" {} + \
         | awk -v total="$count" -v path="$path" '
             BEGIN { next_pct = 5 }
             {
@@ -165,6 +169,6 @@ if [[ "$dry_run" -eq 0 && -d /config ]]; then
 fi
 
 if [[ "$dry_run" -eq 0 && -n "$sentinel" && "$swept_clean" -eq 1 ]]; then
-    echo "$sentinel_content" > "$sentinel" || \
+    /usr/local/bin/safe-sentinel write "$sentinel" "$sentinel_content" || \
         echo "[WARN] fix-ownership: could not write ${sentinel}; the sweep will run again on next boot"
 fi
diff --git a/docker/main/rootfs/usr/local/bin/safe-sentinel b/docker/main/rootfs/usr/local/bin/safe-sentinel
new file mode 100755
index 0000000000..13fe5ab1f4
--- /dev/null
+++ b/docker/main/rootfs/usr/local/bin/safe-sentinel
@@ -0,0 +1,74 @@
+#!/usr/bin/env python3
+"""Read or write the ownership sweep sentinel without following symlinks.
+
+The sentinel lives in /config, which the unprivileged runtime user owns, so it
+can be swapped for a symlink. read trusts only a root-owned regular file; write
+never follows a symlink or fifo onto another file.
+
+Usage:
+  safe-sentinel read PATH            print content, exit 0 only if root-owned regular file
+  safe-sentinel write PATH CONTENT   write CONTENT to a regular file at PATH
+"""
+
+import errno
+import os
+import stat
+import sys
+
+MODE = 0o644
+
+
+def do_read(path: str) -> int:
+    try:
+        fd = os.open(path, os.O_RDONLY | os.O_NOFOLLOW)
+    except OSError:
+        return 1
+    try:
+        st = os.fstat(fd)
+        if not stat.S_ISREG(st.st_mode) or st.st_uid != 0:
+            return 1
+        sys.stdout.buffer.write(os.read(fd, 4096))
+    finally:
+        os.close(fd)
+    return 0
+
+
+def do_write(path: str, content: str) -> int:
+    # O_NONBLOCK so a fifo fails fast (ENXIO) instead of blocking the open.
+    flags = os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW | os.O_NONBLOCK
+    replace = (errno.ELOOP, errno.ENXIO)
+    try:
+        fd = os.open(path, flags, MODE)
+        if not stat.S_ISREG(os.fstat(fd).st_mode):
+            os.close(fd)
+            raise OSError(errno.ELOOP, "not a regular file")
+    except OSError as err:
+        if err.errno not in replace:
+            raise
+        os.unlink(path)
+        fd = os.open(path, flags | os.O_EXCL, MODE)
+    try:
+        os.ftruncate(fd, 0)
+        os.write(fd, content.encode())
+        # keep it root-owned so a later sweep that chowned the old sentinel to
+        # the runtime user can't make the next read reject and re-sweep
+        os.fchown(fd, 0, 0)
+    finally:
+        os.close(fd)
+    return 0
+
+
+def main(argv: list[str]) -> int:
+    if len(argv) == 3 and argv[1] == "read":
+        return do_read(argv[2])
+    if len(argv) == 4 and argv[1] == "write":
+        try:
+            return do_write(argv[2], argv[3])
+        except OSError:
+            return 1
+    print("usage: safe-sentinel read PATH | write PATH CONTENT", file=sys.stderr)
+    return 2
+
+
+if __name__ == "__main__":
+    sys.exit(main(sys.argv))
diff --git a/docker/main/rootfs/usr/local/go2rtc/prepare_homekit.py b/docker/main/rootfs/usr/local/go2rtc/prepare_homekit.py
new file mode 100644
index 0000000000..cda82f08cf
--- /dev/null
+++ b/docker/main/rootfs/usr/local/go2rtc/prepare_homekit.py
@@ -0,0 +1,97 @@
+"""Normalize the go2rtc HomeKit file and hand it to go2rtc, as root.
+
+Runs before the drop. The file is in the runtime-user-owned /config, so a
+planted symlink could redirect the root write or chown onto another file;
+every operation goes through an O_NOFOLLOW fd to prevent that.
+
+Usage: prepare_homekit.py PATH [--chown]
+"""
+
+import errno
+import grp
+import io
+import os
+import pwd
+import stat
+import sys
+
+from ruamel.yaml import YAML
+
+RUNTIME_OWNER = "go2rtc"
+SHARED_GROUP = "frigate-data"
+MODE = 0o664
+MAX_BYTES = 10 * 1024 * 1024
+
+
+def open_nofollow(path: str) -> int:
+    """Return an fd to a regular file at path, never following a symlink."""
+    flags = os.O_RDWR | os.O_CREAT | os.O_NOFOLLOW
+    try:
+        fd = os.open(path, flags, MODE)
+    except OSError as err:
+        if err.errno != errno.ELOOP:
+            raise
+        os.unlink(path)
+        return os.open(path, flags | os.O_EXCL, MODE)
+
+    # A fifo or other non-regular file would hang or misbehave on read; replace it.
+    if not stat.S_ISREG(os.fstat(fd).st_mode):
+        os.close(fd)
+        os.unlink(path)
+        return os.open(path, flags | os.O_EXCL, MODE)
+    return fd
+
+
+def normalize(content: str) -> str:
+    """Keep only the homekit section, matching the previous yq/jq behavior."""
+    yaml = YAML(typ="safe")
+    try:
+        data = yaml.load(content)
+    except Exception:
+        return ""
+
+    if not isinstance(data, dict) or "homekit" not in data:
+        return ""
+
+    buf = io.StringIO()
+    yaml.dump({"homekit": data["homekit"]}, buf)
+    return buf.getvalue()
+
+
+def main() -> int:
+    if len(sys.argv) < 2:
+        print("[ERROR] prepare_homekit: PATH is required", file=sys.stderr)
+        return 2
+
+    path = sys.argv[1]
+    do_chown = "--chown" in sys.argv[2:]
+
+    fd = open_nofollow(path)
+    try:
+        content = os.read(fd, MAX_BYTES).decode("utf-8", "replace")
+        normalized = normalize(content)
+        os.ftruncate(fd, 0)
+        os.lseek(fd, 0, os.SEEK_SET)
+        os.write(fd, normalized.encode("utf-8"))
+
+        if do_chown:
+            # tolerate a chown-refusing mount (NFS root_squash): pairing
+            # persistence degrades, the service does not
+            try:
+                uid = pwd.getpwnam(RUNTIME_OWNER).pw_uid
+                gid = grp.getgrnam(SHARED_GROUP).gr_gid
+                os.fchown(fd, uid, gid)
+                os.fchmod(fd, MODE)
+            except (KeyError, OSError):
+                print(
+                    f"[WARN] Could not hand {path} to the go2rtc user; "
+                    "HomeKit pairing changes may not persist"
+                )
+    finally:
+        os.close(fd)
+
+    return 0
+
+
+if __name__ == "__main__":
+    sys.exit(main())
diff --git a/docs/docs/configuration/non_root.md b/docs/docs/configuration/non_root.md
index e7d999a574..8b1fc6a219 100644
--- a/docs/docs/configuration/non_root.md
+++ b/docs/docs/configuration/non_root.md
@@ -268,7 +268,7 @@ What each device needs when you're setting it up by hand. The automatic grant co
 
 go2rtc's ffmpeg processes no longer appear in Intel GPU stats. Frigate reads per-process GPU usage from `/proc/<pid>/fdinfo`, which the kernel won't let one user read for another user's processes, so anything go2rtc spawns is invisible to it. Overall GPU utilization is unaffected.
 
-If you mount your own TLS certificate at `/etc/letsencrypt/live/frigate`, the private key has to be readable by the runtime user. Frigate won't change ownership of a certificate you supplied, since the mount may be read-only.
+If you mount your own TLS certificate at `/etc/letsencrypt/live/frigate`, the private key has to be readable by the runtime user, which runs nginx. Frigate hands the key to that user at startup if the mount is writable; on a read-only mount, make the key readable by uid 1000 (or your `PUID`) yourself.
 
 If you're debugging nginx, run the config check as the runtime user with stdout discarded: