From f4c81b0aa5fd1528be9baf2603fe54dd66a23954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Roukala=20=28n=C3=A9=20Peres=29?= Date: Tue, 10 Jan 2023 12:10:13 +0200 Subject: [PATCH] Only prevent VTs to be mounted inside privileged systemd containers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While mounting virtual console devices in a systemd container is a recipe for disaster (I experienced it first hand), mounting serial console devices, modems, and others should still be done by default for privileged systemd-based containers. v2, addressing the review from @fho: - use backticks in the regular expression to remove backslashes - pre-compile the regex at the package level - drop IsVirtualTerminalDevice (not needed for a one-liner) v3, addressing the review from @fho and @rhatdan: - re-introduce a private function for matching the device names - use path.Match rather than a regex not to slow down startup time Closes #16925. Fixes: 5a2405ae1b3a ("Don't mount /dev/tty* inside privileged...") Signed-off-by: Martin Roukala (né Peres) --- pkg/util/utils_linux.go | 19 ++++++++++++++++++- test/system/030-run.bats | 35 +++++++++++++++++++++++++++++------ 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/pkg/util/utils_linux.go b/pkg/util/utils_linux.go index 07927db1ce..27ab162e4a 100644 --- a/pkg/util/utils_linux.go +++ b/pkg/util/utils_linux.go @@ -5,6 +5,7 @@ import ( "fmt" "io/fs" "os" + "path" "path/filepath" "strings" "syscall" @@ -70,6 +71,22 @@ func FindDeviceNodes() (map[string]string, error) { return nodes, nil } +func isVirtualConsoleDevice(device string) bool { + /* + Virtual consoles are of the form `/dev/tty\d+`, any other device such as + /dev/tty, ttyUSB0, or ttyACM0 should not be matched. + See `man 4 console` for more information. + + NOTE: Matching is done using path.Match even though a regular expression + would have been more accurate. This is because a regular + expression would have required pre-compilation, which would have + increase the startup time needlessly or made the code more complex + than needed. + */ + matched, _ := path.Match("/dev/tty[0-9]*", device) + return matched +} + func AddPrivilegedDevices(g *generate.Generator, systemdMode bool) error { hostDevices, err := getDevices("/dev") if err != nil { @@ -104,7 +121,7 @@ func AddPrivilegedDevices(g *generate.Generator, systemdMode bool) error { } } else { for _, d := range hostDevices { - if systemdMode && strings.HasPrefix(d.Path, "/dev/tty") { + if systemdMode && isVirtualConsoleDevice(d.Path) { continue } g.AddDevice(d) diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 3094cf3211..bcc78c907f 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -928,15 +928,16 @@ $IMAGE--c_ok" \ # ('skip' would be nicer in some sense... but could hide a regression. # Fedora, RHEL, Debian, Ubuntu, Gentoo, all have /dev/ttyN, so if # this ever triggers, it means a real problem we should know about.) - assert "$(ls /dev/tty* | grep -vx /dev/tty)" != "" \ + vt_tty_devices_count=$(find /dev -regex '/dev/tty[0-9].*' | wc -w) + assert "$vt_tty_devices_count" != "0" \ "Expected at least one /dev/ttyN device on host" # Ok now confirm that without --systemd, podman exposes ttyNN devices run_podman run --rm -d --privileged $IMAGE ./pause cid="$output" - run_podman exec $cid sh -c 'ls /dev/tty*' - assert "$output" != "/dev/tty" \ + run_podman exec $cid sh -c "find /dev -regex '/dev/tty[0-9].*' | wc -w" + assert "$output" = "$vt_tty_devices_count" \ "ls /dev/tty* without systemd; should have lots of ttyN devices" run_podman stop -t 0 $cid @@ -944,13 +945,35 @@ $IMAGE--c_ok" \ run_podman run --rm -d --privileged --systemd=always $IMAGE ./pause cid="$output" - run_podman exec $cid sh -c 'ls /dev/tty*' - assert "$output" = "/dev/tty" \ - "ls /dev/tty* with --systemd=always: should have no ttyN devices" + run_podman exec $cid sh -c "find /dev -regex '/dev/tty[0-9].*' | wc -w" + assert "$output" = "0" \ + "ls /dev/tty[0-9] with --systemd=always: should have no ttyN devices" run_podman stop -t 0 $cid } +# 16925: --privileged + --systemd = share non-virtual-terminal TTYs +@test "podman run --privileged as root with systemd mounts non-vt /dev/tty devices" { + skip_if_rootless "this test only makes sense as root" + + # First, confirm that we _have_ non-virtual terminal /dev/tty* devices on + # the host. + non_vt_tty_devices_count=$(find /dev -regex '/dev/tty[^0-9].*' | wc -w) + if [ "$non_vt_tty_devices_count" -eq 0 ]; then + skip "The server does not have non-vt TTY devices" + fi + + # Verify that all the non-vt TTY devices got mounted in the container + run_podman run --rm -d --privileged --systemd=always $IMAGE ./pause + cid="$output" + run_podman '?' exec $cid find /dev -regex '/dev/tty[^0-9].*' + assert "$status" = 0 \ + "No non-virtual-terminal TTY devices got mounted in the container" + assert "$(echo "$output" | wc -w)" = "$non_vt_tty_devices_count" \ + "Some non-virtual-terminal TTY devices are missing in the container" + run_podman stop -t 0 $cid +} + @test "podman run read-only from containers.conf" { containersconf=$PODMAN_TMPDIR/containers.conf cat >$containersconf <