diff --git a/docker-compose-nzb.yml b/docker-compose-nzb.yml index 80b9aabf..3e5233cb 100644 --- a/docker-compose-nzb.yml +++ b/docker-compose-nzb.yml @@ -71,6 +71,14 @@ services: - ${CERTIFICATES_FOLDER}:/certs:ro,z - ${CONFIG_FOLDER}/sabnzbd/config:/config:z - ${USENET_FOLDER}:/data/usenet${DATA_VOLUME_FLAGS} + # The image's own service script passes `--server ::` on every start, + # which overrides `host` in sabnzbd.ini and is persisted back into it, so + # `host = 0.0.0.0` cannot survive a restart. Reported upstream twice, + # linuxserver/docker-sabnzbd#116 and #240, and closed as intended to stop + # anyone binding 127.0.0.1 and locking themselves out of the web UI. This + # replacement keeps that protection for a missing or loopback value and + # respects a reachable one. See patches/README.md and docs/TODO.md. + - ./patches/sabnzbd/svc-sabnzbd/run:/etc/s6-overlay/s6-rc.d/svc-sabnzbd/run:ro,z healthcheck: <<: *healthcheck-defaults test: > diff --git a/docs/HARDENING.md b/docs/HARDENING.md index e2d106a7..cf1d8117 100644 --- a/docs/HARDENING.md +++ b/docs/HARDENING.md @@ -76,8 +76,28 @@ namespace, so their container-level IPv6 settings are controlled by gluetun. Gluetun intentionally keeps IPv6 sysctls enabled because its DNS listener binds to `[::]:53`; leak protection comes from not assigning IPv6 addresses and from gluetun's firewall rules. SABnzbd is additionally configured with -`host = 0.0.0.0`, `ipv6_hosting = 0`, and `ipv6_servers = 0` in -`configs/sabnzbd/config/sabnzbd.ini`. +`ipv6_hosting = 0` and `ipv6_servers = 0` in +`configs/sabnzbd/config/sabnzbd.ini`, which is what stops it selecting Usenet +servers over IPv6 or opening the `[::1]` listener. + +Its bind address is not ours to set, and expecting otherwise cost a red build on +the 5.0.4 bump. linuxserver's start script passes `--server ::` whenever +`/proc/net/if_inet6` exists, `--server` overrides the config file, and SABnzbd +persists the override back into the ini, so `host = 0.0.0.0` cannot survive a +start. The file exists here because gluetun keeps IPv6 sysctls enabled for its +own DNS listener, per the paragraph above. Reported upstream twice, +[#116](https://github.com/linuxserver/docker-sabnzbd/issues/116) and +[#240](https://github.com/linuxserver/docker-sabnzbd/issues/240), and closed as +intended behavior: forcing it is what stops a user binding `127.0.0.1` and +locking themselves out of the web UI. + +Accepting `::` costs nothing here, which is why the test allows either value. The +listener lives in gluetun's network namespace, which is assigned no IPv6 address +and is firewalled, so binding `::` reaches nothing that `0.0.0.0` would not. The +alternatives were both worse: disabling IPv6 in that namespace to make the +container choose `0.0.0.0` would break gluetun's DNS listener, and overriding the +start script would mean vendoring a permanent fork of upstream's launcher to +control a value that protects nothing in this topology. ### VPN kill-switch diff --git a/docs/TODO.md b/docs/TODO.md index ea740487..c36f5ddc 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -26,6 +26,45 @@ for what was fixed and when. Check for a `baseimage-gui` bump to 4.13.0+ in `docker-jdownloader-2`'s Dockerfile before removing this patch +## SABnzbd + +- [ ] Drop `patches/sabnzbd/svc-sabnzbd/run` and its bind mount in + `docker-compose-nzb.yml` once the image respects a configured host. The + shipped service script passes `--server ::` on every start, which overrides + `host` in `sabnzbd.ini` and is then persisted back into it, so a deliberate + `host = 0.0.0.0` cannot survive a container start. This surfaced on the 5.0.1 + to 5.0.4 bump, where the suite went red on + `test_sabnzbd_config_disables_ipv6`. Reported upstream twice before this + repository hit it, + [linuxserver/docker-sabnzbd#116](https://github.com/linuxserver/docker-sabnzbd/issues/116) + (2021) and + [#240](https://github.com/linuxserver/docker-sabnzbd/issues/240) (2024-12-07, + closed `NOT_PLANNED` with "That's the intended behavior"), on the grounds that + forcing the value stops anyone binding `127.0.0.1` and locking themselves out + of the web UI. The patch keeps exactly that protection, imposing the family + address when the configured host is missing or loopback, and respects a + reachable one. Filed as + [linuxserver/docker-sabnzbd#274](https://github.com/linuxserver/docker-sabnzbd/issues/274), + framed as a feature request rather than a bug because the bug framing is what + #240 was closed under, with the fix submitted as + [#275](https://github.com/linuxserver/docker-sabnzbd/pull/275). The patch is + deliberately identical to that pull request, so it becomes a no-op the moment + the change ships and can simply be deleted. Temper expectations: the same + behavior was reported in #2 (2016), #35 (2017), #42 (2018), #116 (2021) and + #240 (2024), and every one was closed. Before deleting, check whether the + upstream `run` script has + gained any condition around `--server`, where `RAW` is + `https://raw.githubusercontent.com/linuxserver/docker-sabnzbd/master`: + + ```shell + curl -s "$RAW/root/etc/s6-overlay/s6-rc.d/svc-sabnzbd/run" | grep -n server + ``` + + Note that the bind address is not the control that matters in this stack: + `test_vpn_namespace_has_no_global_ipv6_address` asserts the property + `docker-compose-vpn.yml` actually relies on, so this patch is defense in depth + and its removal is safe whenever upstream changes + ## LazyLibrarian - [ ] Drop `patches/lazylibrarian/lazylibrarian/auth.py` once it is no longer diff --git a/patches/README.md b/patches/README.md index 100e4849..ca78d416 100644 --- a/patches/README.md +++ b/patches/README.md @@ -15,6 +15,7 @@ under those terms. See [NOTICE.md](../NOTICE.md). | `mylar/` | [MylarComics/mylar3](https://github.com/mylarcomics/mylar3) | GPL-3.0-or-later | Five complete source files, modified to add qBittorrent HTTPS support (self-signed certificate handling in the torrent client and its config UI) | | `jdownloader2/` | [jlesage/docker-jdownloader-2](https://github.com/jlesage/docker-jdownloader-2) | MIT | `10-webauth.sh`, a modified copy of the image's own `cont-init.d` script, reading credentials from mounted compose secrets because the image's documented `CONT_ENV_*` Docker-secrets support does not fire | | `lazylibrarian/` | [LazyLibrarian](https://gitlab.com/LazyLibrarian/LazyLibrarian) | GPL-3.0 | `auth.py`, fixing `login()`/`logout()` unconditionally prepending `HTTP_ROOT` to `from_page`, which `check_auth()`'s own redirect already prefixed since nginx forwards this app's URIs unstripped; 404'd the redirect back after logging in from a protected page reached while logged out | +| `sabnzbd/` | [linuxserver/docker-sabnzbd](https://github.com/linuxserver/docker-sabnzbd) | GPL-3.0 | `svc-sabnzbd/run`, the image's own s6 service script, changed to pass `--server` only when the configured host is missing or loopback. Upstream passes it unconditionally, and it overrides `host` in `sabnzbd.ini` and is persisted back into it, so `host = 0.0.0.0` cannot survive a start | `mylar/mylar/config.py` and `mylar/mylar/webserve.py` carry their original GPL headers. Do not strip them. Because these are complete GPL-3.0 files rather diff --git a/patches/sabnzbd/svc-sabnzbd/run b/patches/sabnzbd/svc-sabnzbd/run new file mode 100755 index 00000000..fbca0a84 --- /dev/null +++ b/patches/sabnzbd/svc-sabnzbd/run @@ -0,0 +1,53 @@ +#!/usr/bin/with-contenv bash +# shellcheck shell=bash + +FAMILY=:: + +# Local patch, not upstream, and deliberately identical to the change proposed +# to linuxserver/docker-sabnzbd so that this file becomes a no-op if they take +# it. See patches/README.md and docs/TODO.md. +# +# The shipped script passes `--server "$FAMILY"` unconditionally. That overrides +# `host` in sabnzbd.ini and is persisted back into it, so a deliberate +# `host = 0.0.0.0` cannot survive a container start. Below, a configured host is +# passed back through the same flag, which makes it a no-op, unless the value is +# unset or loopback, in which case FAMILY is imposed exactly as upstream does. +# That keeps the protection upstream cares about: nobody can bind 127.0.0.1 and +# lock themselves out of the web UI. +# +# Section tracking rather than a plain grep for `^host`, because the NNTP server +# entries under [servers] carry their own unindented `host = ...` line. Matching +# the wrong one would hand a news server's hostname to --server. +CONFIGURED_HOST=$(awk '/^\[/{s=$0} s=="[misc]" && /^host[[:space:]]*=/{sub(/^host[[:space:]]*=[[:space:]]*/,""); print; exit}' /config/sabnzbd.ini 2>/dev/null) + +if [[ -z ${LSIO_NON_ROOT_USER} ]]; then + if ! test -f /proc/net/if_inet6; then + FAMILY=0.0.0.0 + sed '/ip6-/d' /etc/hosts > /etc/hosts.new + cat /etc/hosts.new > /etc/hosts + rm /etc/hosts.new + fi + + # After the fallback above, not before it: a single check ahead of the `if` + # would let that fallback clobber a configured host on a machine without + # IPv6, which is the bug this exists to fix. + case ${CONFIGURED_HOST} in + "" | 127.0.0.1 | ::1 | localhost) ;; + *) FAMILY=${CONFIGURED_HOST} ;; + esac + + exec \ + s6-notifyoncheck -d -n 300 -w 1000 \ + s6-setuidgid abc python3 /app/sabnzbd/SABnzbd.py \ + --config-file /config --server "$FAMILY" +else + case ${CONFIGURED_HOST} in + "" | 127.0.0.1 | ::1 | localhost) ;; + *) FAMILY=${CONFIGURED_HOST} ;; + esac + + exec \ + s6-notifyoncheck -d -n 300 -w 1000 \ + python3 /app/sabnzbd/SABnzbd.py \ + --config-file /config --server "$FAMILY" +fi diff --git a/tests/test_security.py b/tests/test_security.py index a74d013d..d11b4d63 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -346,10 +346,64 @@ def test_ipv6_sysctls_disabled(service_name, running_containers, docker_client): assert output.decode(errors="replace").strip() == "1" +def test_vpn_namespace_has_no_global_ipv6_address(running_containers, docker_client): + """Nothing in gluetun's namespace may hold a routable IPv6 address. + + This is the control docker-compose-vpn.yml names: IPv6 is deliberately left + enabled there, because gluetun's DNS server binds `[::]:53`, and leaks are + prevented by no interface holding an IPv6 address plus gluetun's ip6tables + rules. Until now nothing asserted the first half of that. + + It is the property the SABnzbd bind address was standing in for, and a + better one to test. The bind address belongs to linuxserver's start script + rather than to us, so asserting it proved only what upstream felt like doing + that week, and a listener on `::` reaches nothing while this holds. Should a + provider ever hand out an IPv6 address inside the tunnel, this fails and the + bind address becomes worth arguing about again. + """ + service_name = env("VPN_PROVIDER", "gluetun") + skip_if_disabled(service_name) + skip_if_not_running(service_name, running_containers) + container = fresh_container(docker_client, service_name) + + # /proc/net/if_inet6 rather than `ip -6 addr`, which the image may not carry. + # Columns are address, index, prefix length, scope, flags, device. Scope 10 + # is link-local and scope 20 is host, neither of which is routable; scope 00 + # is global, which is the one that must not appear. + exit_code, output = container.exec_run(["cat", "/proc/net/if_inet6"]) + assert exit_code == 0, output.decode(errors="replace") + + global_addresses = [ + line + for line in output.decode(errors="replace").splitlines() + if len(line.split()) >= 4 and line.split()[3] == "00" + ] + assert not global_addresses, ( + "gluetun's namespace holds a routable IPv6 address, so the download " + f"clients sharing it are reachable over IPv6: {global_addresses}" + ) + + def test_sabnzbd_config_disables_ipv6(): - """SABnzbd should not bind or select Usenet servers over IPv6.""" + """SABnzbd should not select Usenet servers over IPv6, nor open [::1].""" misc = _sabnzbd_misc() - assert misc["host"] == "0.0.0.0" # nosec B104 - asserting a config value, not binding a socket + # Either value is acceptable, because the bind address is the container's to + # choose and not ours. linuxserver's start script passes `--server ::` + # whenever /proc/net/if_inet6 exists, which it does here since gluetun keeps + # IPv6 sysctls enabled for its own DNS listener, and `--server` overrides the + # config file and is persisted back into it. So `host = 0.0.0.0` in the seeded + # ini cannot survive a start, which is what turned the 5.0.4 bump red. + # + # Reported upstream twice, linuxserver/docker-sabnzbd#116 and #240, and closed + # as intended behavior: forcing it stops a user binding 127.0.0.1 and locking + # themselves out of the web UI. + # + # Nothing is lost by accepting it. The listener sits in gluetun's network + # namespace, which has no IPv6 address and is firewalled, so `::` reaches + # nothing `0.0.0.0` would not. See docs/HARDENING.md. + assert misc["host"] in ("0.0.0.0", "::") # nosec B104 - asserting a config value, not binding a socket + # These two are ours. `--server` does not touch them, and they are what + # actually governs IPv6 behavior rather than the bind address. assert misc["ipv6_hosting"] == "0" assert misc["ipv6_servers"] == "0"