Only prevent VTs to be mounted inside privileged systemd containers #17055

mupuf · 2023-01-10T13:02:21Z

While mounting virtual terminal 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.

Closes #16925.

Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...")
Signed-off-by: Martin Roukala (né Peres) [email protected]

Does this PR introduce a user-facing change?

Privileged containers running systemd will once again mount /dev/tty* devices into the container, ignoring virtual terminals (/dev/tty[0-9]+) (https://github.com/containers/podman/issues/16925).

fho · 2023-01-10T13:49:22Z

pkg/util/utils_linux.go

@@ -70,6 +71,11 @@ func FindDeviceNodes() (map[string]string, error) {
 	return nodes, nil
 }

+func IsVirtualTerminalDevice(deviceName string) bool {
+	isMatched, _ := regexp.MatchString("/dev/tty[\\d]+", deviceName)


Suggested change

isMatched, _ := regexp.MatchString("/dev/tty[\\d]+", deviceName)

isMatched, _ := regexp.MatchString(`/dev/tty[\d]+`, deviceName)

With backquotes \ does not need to be escaped

fho · 2023-01-10T13:51:49Z

pkg/util/utils_linux.go

@@ -70,6 +71,11 @@ func FindDeviceNodes() (map[string]string, error) {
 	return nodes, nil
 }

+func IsVirtualTerminalDevice(deviceName string) bool {
+	isMatched, _ := regexp.MatchString("/dev/tty[\\d]+", deviceName)


What do you think about making this a precompiled regex on package lvl (regexp.MustCompile())?
Then the regexp would only be compiled once instead of on every invocation of IsVirtualTerminalDevice().

Sounds like a good idea! I was operating under the assumption that it would get cached anyway, but I can't seem to find any reference of that!

After you pointed out that /dev/tty must not match, path.Match("/dev/tty[0-9]*" became an alternative. It should be faster then using a regex and a global variable would also not be needed.

fho · 2023-01-10T13:53:22Z

pkg/util/utils_linux.go

@@ -70,6 +71,11 @@ func FindDeviceNodes() (map[string]string, error) {
 	return nodes, nil
 }

+func IsVirtualTerminalDevice(deviceName string) bool {
+	isMatched, _ := regexp.MatchString("/dev/tty[\\d]+", deviceName)


Suggested change

isMatched, _ := regexp.MatchString("/dev/tty[\\d]+", deviceName)

isMatched, _ := regexp.MatchString("^/dev/tty[\\d]*$", deviceName)

/dev/tty exists on my system, it probably also must be matched?

Use begin and end markers to prevent that it also matches weird unexpected dev paths like /dev/tty100ABC, better be safe :-)

/dev/tty is not a virtual terminal though, so we definitely want to keep it, so we shouldn't match it.

I agree with the idea of using the begin/end markers, but we should be very precise as to what a virtual terminal would be, and was isn't. Virtual terminals are /dev/ttyN+, nothing else.

So, here would be the final result:

regexp.MatchString(`^/dev/tty[\d]+$`, deviceName)

I agree, reference for others: https://man7.org/linux/man-pages/man4/tty.4.html

fho · 2023-01-10T13:59:10Z

pkg/util/utils_linux.go

@@ -70,6 +71,11 @@ func FindDeviceNodes() (map[string]string, error) {
 	return nodes, nil
 }

+func IsVirtualTerminalDevice(deviceName string) bool {


I recommend to make it a private function because there is no usecase for it outside of the package currently.

mupuf · 2023-01-10T14:41:07Z

Thanks for the review, @fho! That was great feedback, as I am a noob in golang!

I ended up removing the function altogether, since I could pre-compile the regex, and the function would end up being a single line!

pkg/util/utils_linux.go

@fho

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 containers#16925. Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...") Signed-off-by: Martin Roukala (né Peres) <[email protected]>

mheon · 2023-01-11T15:08:32Z

Code LGTM. I think this technically matches paths like /dev/tty0abcd, but I'm not aware of any devices that actually use a path layout like that, so we ought to be fine.

fho · 2023-01-11T17:35:25Z

@mheon true,

I suggest the following instead:

func isVirtualConsoleDevice(path string) bool {
	suffix := strings.TrimPrefix(path, "/dev/tty")
	if suffix == path || suffix == "" {
		return false
	}

        // bitsize could be lowered to max. number of tty devices
	_, err := strconv.ParseUint(suffix, 10, 64)
	return err == nil
}

and a unit-test:

func TestIsVirtualConsoleDevice(t *testing.T) {
	testcases := []struct {
		expectedResult bool
		path           string
	}{
		{
			expectedResult: true,
			path:           "/dev/tty10",
		},
		{
			expectedResult: false,
			path:           "/dev/tty",
		},
		{
			expectedResult: false,
			path:           "/dev/ttyUSB0",
		},

		{
			expectedResult: false,
			path:           "/dev/tty0abcd",
		},
	}

	for _, tc := range testcases {
		t.Run(tc.path, func(t *testing.T) {
			result := isVirtualConsoleDevice(tc.path)
			if result != tc.expectedResult {
				t.Errorf("isVirtualConsoleDevice returned %t, expected %t", result, tc.expectedResult)
			}
		})
	}
}

rhatdan · 2023-01-11T19:30:43Z

LGTM

rhatdan · 2023-01-11T21:23:44Z

/approve

openshift-ci · 2023-01-11T21:23:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mupuf, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

TomSweeneyRedHat · 2023-01-11T23:46:22Z

Changes LGTM, but some of the tests aren't happy. I don't think the test failure is related, but I don't know for sure.

edsantiago · 2023-01-12T01:07:59Z

podman-machine flake is #16789, which is hitting us multiple times a day. Restarted.

rhatdan · 2023-01-12T01:54:31Z

/lgtm

mupuf · 2023-01-12T04:31:28Z

@mheon true,

I suggest the following instead:

func isVirtualConsoleDevice(path string) bool {
	suffix := strings.TrimPrefix(path, "/dev/tty")
	if suffix == path || suffix == "" {
		return false
	}

        // bitsize could be lowered to max. number of tty devices
	_, err := strconv.ParseUint(suffix, 10, 64)
	return err == nil
}

and a unit-test:

func TestIsVirtualConsoleDevice(t *testing.T) {
	testcases := []struct {
		expectedResult bool
		path           string
	}{
		{
			expectedResult: true,
			path:           "/dev/tty10",
		},
		{
			expectedResult: false,
			path:           "/dev/tty",
		},
		{
			expectedResult: false,
			path:           "/dev/ttyUSB0",
		},

		{
			expectedResult: false,
			path:           "/dev/tty0abcd",
		},
	}

	for _, tc := range testcases {
		t.Run(tc.path, func(t *testing.T) {
			result := isVirtualConsoleDevice(tc.path)
			if result != tc.expectedResult {
				t.Errorf("isVirtualConsoleDevice returned %t, expected %t", result, tc.expectedResult)
			}
		})
	}
}

This looks like an excellent idea and would protect ourselves against /dev/tty1abc!

Since this code already landed, mind sending it as a new MR? I can offer some testing and whatever my review is worth.

mheon · 2023-01-12T06:29:15Z

Sure, new PR would be fine.

fho · 2023-01-13T09:55:29Z

@mheon true,
I suggest the following instead:
[...]
Since this code already landed, mind sending it as a new MR? I can offer some testing and whatever my review is worth.

I created a PR:
#17104

fho · 2023-01-16T08:50:26Z

We forgot a call.

We also have to use isVirtualConsoleDevice here:

podman/pkg/util/utils_linux.go

Line 110 in cdcd2ed

if d.Path == "/dev/ptmx" || strings.HasPrefix(d.Path, "/dev/tty") {

).
Otherwise it won't work with rootless container.

I can create a PR when I find some time in my off hours. I also would not mind if somebody is faster :-)

mupuf · 2023-01-16T08:55:00Z

We forgot a call.

We also have to use isVirtualConsoleDevice here:

podman/pkg/util/utils_linux.go

Line 110 in cdcd2ed

if d.Path == "/dev/ptmx" || strings.HasPrefix(d.Path, "/dev/tty") {

).
Otherwise it won't work with rootless container.

I can create a PR when I find some time in my off hours. I also would not mind if somebody is faster :-)

Good find! I was initially developing the patch for my project which only cared about the rootful path, and I completely forgot about this path... Silly me...

I think I'll spin up a new series to get this fixed, since you already have an active MR of your own! Thanks again!

mupuf · 2023-01-16T10:58:06Z

@fho: Weeeeellll, I am currently investigating why there is this filtering in place. Here are my findings:

It was not introduced by the patch we were fixing (5a2405a)
It was introduced by a commit that vendored some of buildah's code, which mentioned it was duplicating the behavior of podman (https://github.com/containers/buildah/blob/main/run_linux.go#L167) (ROFL)
Looking more into podman's history, I found the same code was introduced by a commit which was vendoring some libpod code (ba430bf)

And now I can't figure out where this code originally comes from... so I can't figure out why it is there... and thus I don't feel confident changing it to unify the codepath. What are your thoughts?

fho · 2023-01-16T11:40:51Z

@mupuf
It looks to me like it exist to prevent the same systemd issue and only was introduced separately.
In my local version I adapted that part and tested it in non-root mode with a ttyACM device manually and it worked.

I think we should create a PR to also adapt that. If there is an issue with it, it could also be caught during the codereview or by the tests.

@mheon

As @mheon pointed out in PR containers#17055[^1], isVirtualConsoleDevice() does not only matches VT device paths but also devices named like /dev/tty0abcd. This causes that non VT device paths named /dev/tty[0-9]+[A-Za-z]+ are not mounted into privileged container and systemd containers accidentally. This is an unlikely issue because the Linux kernel does not use device paths like that. To make it failproof and prevent issues in unlikely scenarios, change isVirtualConsoleDevice() to exactly match ^/dev/tty[0-9]+$ paths. Because it is not possible to match this path exactly with Glob syntax, the path is now checked with strings.TrimPrefix() and strconv.ParseUint(). ParseUint uses a bitsize of 16, this is sufficient because the max number of TTY devices is 512 in Linux 6.1.5. (Checked via 'git grep -e '#define' --and -e 'TTY_MINORS'). The commit also adds a unit-test for isVirtualConsoleDevice(). Fixes: f4c81b0 ("Only prevent VTs to be mounted inside...") [^1]: containers#17055 (comment) Signed-off-by: Fabian Holler <[email protected]>

@mheon

As @mheon pointed out in PR containers#17055[^1], isVirtualConsoleDevice() does not only matches VT device paths but also devices named like /dev/tty0abcd. This causes that non VT device paths named /dev/tty[0-9]+[A-Za-z]+ are not mounted into privileged container and systemd containers accidentally. This is an unlikely issue because the Linux kernel does not use device paths like that. To make it failproof and prevent issues in unlikely scenarios, change isVirtualConsoleDevice() to exactly match ^/dev/tty[0-9]+$ paths. Because it is not possible to match this path exactly with Glob syntax, the path is now checked with strings.TrimPrefix() and strconv.ParseUint(). ParseUint uses a bitsize of 16, this is sufficient because the max number of TTY devices is 512 in Linux 6.1.5. (Checked via 'git grep -e '#define' --and -e 'TTY_MINORS'). The commit also adds a unit-test for isVirtualConsoleDevice(). Fixes: f4c81b0 ("Only prevent VTs to be mounted inside...") [^1]: containers#17055 (comment) Signed-off-by: Fabian Holler <[email protected]> Signed-off-by: Daniel J Walsh <[email protected]>

@mheon

As @mheon pointed out in PR containers#17055[^1], isVirtualConsoleDevice() does not only matches VT device paths but also devices named like /dev/tty0abcd. This causes that non VT device paths named /dev/tty[0-9]+[A-Za-z]+ are not mounted into privileged container and systemd containers accidentally. This is an unlikely issue because the Linux kernel does not use device paths like that. To make it failproof and prevent issues in unlikely scenarios, change isVirtualConsoleDevice() to exactly match ^/dev/tty[0-9]+$ paths. Because it is not possible to match this path exactly with Glob syntax, the path is now checked with strings.TrimPrefix() and strconv.ParseUint(). ParseUint uses a bitsize of 16, this is sufficient because the max number of TTY devices is 512 in Linux 6.1.5. (Checked via 'git grep -e '#define' --and -e 'TTY_MINORS'). The commit also adds a unit-test for isVirtualConsoleDevice(). Fixes: f4c81b0 ("Only prevent VTs to be mounted inside...") [^1]: containers#17055 (comment) Signed-off-by: Fabian Holler <[email protected]> Signed-off-by: Daniel J Walsh <[email protected]>

openshift-ci bot added the release-note label Jan 10, 2023

mupuf force-pushed the mount-non-vt-tty branch from 69f6821 to f5bb22b Compare January 10, 2023 13:41

fho reviewed Jan 10, 2023

View reviewed changes

mupuf force-pushed the mount-non-vt-tty branch from f5bb22b to b856b64 Compare January 10, 2023 14:39

mupuf force-pushed the mount-non-vt-tty branch from b856b64 to fa80132 Compare January 10, 2023 14:59

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2023

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jan 10, 2023

mupuf force-pushed the mount-non-vt-tty branch from fa80132 to 3107c85 Compare January 10, 2023 15:00

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2023

github-actions bot removed the kind/api-change Change to remote API; merits scrutiny label Jan 10, 2023

rhatdan reviewed Jan 10, 2023

View reviewed changes

pkg/util/utils_linux.go Outdated Show resolved Hide resolved

mupuf force-pushed the mount-non-vt-tty branch from 3107c85 to f4c81b0 Compare January 11, 2023 14:59

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2023

openshift-ci bot assigned rhatdan Jan 12, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2023

openshift-merge-robot merged commit a7ba63d into containers:main Jan 12, 2023

edsantiago mentioned this pull request Jan 12, 2023

podman machine: flaky start #16789

Closed

fho mentioned this pull request Jan 13, 2023

Match VT device paths to be blocked from mounting exactly #17104

Closed

edsantiago mentioned this pull request Jan 25, 2023

placeholder issue for quay.io flakes #16973

Closed

rhatdan mentioned this pull request Jan 28, 2023

Match VT device paths to be blocked from mounting exactly #17265

Merged

fho mentioned this pull request Jan 31, 2023

[4.4] fix regression with runc --privileged rootless containers #17301

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 15, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only prevent VTs to be mounted inside privileged systemd containers #17055

Only prevent VTs to be mounted inside privileged systemd containers #17055

mupuf commented Jan 10, 2023

fho Jan 10, 2023

fho Jan 10, 2023

mupuf Jan 10, 2023

fho Jan 10, 2023

fho Jan 10, 2023

mupuf Jan 10, 2023 •

edited

Loading

fho Jan 10, 2023

fho Jan 10, 2023

mupuf commented Jan 10, 2023

mheon commented Jan 11, 2023

fho commented Jan 11, 2023

rhatdan commented Jan 11, 2023

rhatdan commented Jan 11, 2023

openshift-ci bot commented Jan 11, 2023

TomSweeneyRedHat commented Jan 11, 2023

edsantiago commented Jan 12, 2023

rhatdan commented Jan 12, 2023

mupuf commented Jan 12, 2023

mheon commented Jan 12, 2023

fho commented Jan 13, 2023

fho commented Jan 16, 2023

mupuf commented Jan 16, 2023

mupuf commented Jan 16, 2023

fho commented Jan 16, 2023

	isMatched, _ := regexp.MatchString("/dev/tty[\\d]+", deviceName)
	isMatched, _ := regexp.MatchString(`/dev/tty[\d]+`, deviceName)

	isMatched, _ := regexp.MatchString("/dev/tty[\\d]+", deviceName)
	isMatched, _ := regexp.MatchString("^/dev/tty[\\d]*$", deviceName)

Only prevent VTs to be mounted inside privileged systemd containers #17055

Only prevent VTs to be mounted inside privileged systemd containers #17055

Conversation

mupuf commented Jan 10, 2023

Does this PR introduce a user-facing change?

fho Jan 10, 2023

Choose a reason for hiding this comment

fho Jan 10, 2023

Choose a reason for hiding this comment

mupuf Jan 10, 2023

Choose a reason for hiding this comment

fho Jan 10, 2023

Choose a reason for hiding this comment

fho Jan 10, 2023

Choose a reason for hiding this comment

mupuf Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

fho Jan 10, 2023

Choose a reason for hiding this comment

fho Jan 10, 2023

Choose a reason for hiding this comment

mupuf commented Jan 10, 2023

mheon commented Jan 11, 2023

fho commented Jan 11, 2023

rhatdan commented Jan 11, 2023

rhatdan commented Jan 11, 2023

openshift-ci bot commented Jan 11, 2023

TomSweeneyRedHat commented Jan 11, 2023

edsantiago commented Jan 12, 2023

rhatdan commented Jan 12, 2023

mupuf commented Jan 12, 2023

mheon commented Jan 12, 2023

fho commented Jan 13, 2023

fho commented Jan 16, 2023

mupuf commented Jan 16, 2023

mupuf commented Jan 16, 2023

fho commented Jan 16, 2023

mupuf Jan 10, 2023 •

edited

Loading