runtime: readonly volume should be bind mounted readonly on the host by bergwolf · Pull Request #1062 · kata-containers/kata-containers

bergwolf · 2020-10-30T09:49:12Z

So that even if the guest is compromised, we still ensure that readonly volumes cannot be modified.

bergwolf · 2020-10-30T09:51:55Z

/test

bergwolf · 2020-10-30T10:20:22Z

/test

jodh-intel

Thanks @bergwolf.

Any way you could create a new test explicitly to check for this behaviour?

amshinde · 2020-10-30T17:21:28Z

@awprice PTAL

To make sure readonly volumes are mounted readonly both inside the guest and on the host. Depends-on: github.com/kata-containers/kata-containers#1062 Fixes: kata-containers#3022 Signed-off-by: Peng Tao <bergwolf@hyper.sh>

bergwolf · 2020-11-02T06:25:15Z

@jodh-intel sure, pls see kata-containers/tests#3023

To make sure readonly volumes are mounted readonly both inside the guest and on the host. Depends-on: github.com/kata-containers/kata-containers#1062 Fixes: kata-containers#3022 Signed-off-by: Peng Tao <bergwolf@hyper.sh>

awprice · 2020-11-02T06:55:42Z

Thanks for raising this so quickly @bergwolf! It looks good to me, but I haven't had a chance to test this as I'm still on 1.11.3

bergwolf · 2020-11-02T07:09:11Z

@awprice ah, I see. I just backported the PR here kata-containers/runtime#3042
Please try the 1.x version. The two repo codes are almost identical and it is pretty easy to port things between them ;)

awprice · 2020-11-02T07:19:05Z

@bergwolf Thanks! I'll test out the 1.x code tomorrow

amshinde · 2020-11-02T19:47:41Z

 		c.mounts[idx].HostPath = mountDest
+		// bindmount remount event is not propagated to mount subtrees, so we have to remount the shared dir mountpoint directly.
+		if m.ReadOnly {
+			mountDest = filepath.Join(hostSharedDir, filename)


@bergwolf I suppose this is a newly added commit. Will this allow the guest to perform an unmount in case of a container escape, the vulnerability CVE-2020-2024 that we addressed before?

nop, this is host mount. guest cannot change it and only sees the readonly directory.

awprice · 2020-11-03T05:39:14Z

I've tested the 1.x code in my environment and it fixes the issue.

To make sure readonly volumes are mounted readonly both inside the guest and on the host. Depends-on: github.com/kata-containers/kata-containers#1062 Fixes: kata-containers#3022 Signed-off-by: Peng Tao <bergwolf@hyper.sh>

devimc

othanks @bergwolf - one nit

bergwolf · 2020-11-03T15:10:17Z

/test

bergwolf · 2020-11-04T09:01:12Z

/test

So that we get protected at the VM boundary not just the guest kernel. Signed-off-by: Peng Tao <bergwolf@hyper.sh>

bindmount remount events are not propagated through mount subtrees, so we have to remount the shared dir mountpoint directly. E.g., ``` mkdir -p source dest foo source/foo mount -o bind --make-shared source dest mount -o bind foo source/foo echo bind mount rw mount | grep foo echo remount ro mount -o remount,bind,ro source/foo mount | grep foo ``` would result in: ``` bind mount rw /dev/xvda1 on /home/ubuntu/source/foo type ext4 (rw,relatime,discard,data=ordered) /dev/xvda1 on /home/ubuntu/dest/foo type ext4 (rw,relatime,discard,data=ordered) remount ro /dev/xvda1 on /home/ubuntu/source/foo type ext4 (ro,relatime,discard,data=ordered) /dev/xvda1 on /home/ubuntu/dest/foo type ext4 (rw,relatime,discard,data=ordered) ``` The reason is that bind mount creats new mount structs and attaches them to different mount subtrees. However, MS_REMOUNT only looks for existing mount structs to modify and does not try to propagate the change to mount structs in other subtrees. Fixes: kata-containers#1061 Signed-off-by: Peng Tao <bergwolf@hyper.sh>

bergwolf · 2020-11-04T09:53:08Z

/test

bergwolf · 2020-11-04T09:58:13Z

There is a regression introduced by #1037 and it is causing crio + k8s configmap to fail a lot on multiple platforms in different PRs. I've added an extra commit to revert the offending commit:

commit 87848e874e10b692c0efdf0709158fb1f9df72a3 (origin/pr/1037)
Author: Gabriela Cervantes <gabriela.cervantes.tellez@intel.com>
Date:   Tue Oct 27 10:50:09 2020 -0600

    versions: Update crio version

    This PR updates the crio version from 1.18.3 to 1.18.4 in order to include
    the fix https://github.com/cri-o/cri-o/pull/4284.

    Fixes #1036

    Signed-off-by: Gabriela Cervantes <gabriela.cervantes.tellez@intel.com>

ref: #1080

bergwolf · 2020-11-04T11:12:46Z

/test-vfio

bergwolf · 2020-11-04T15:18:39Z

vfio test is failing at

19:44:58 kubectl exec [POD] [COMMAND] is DEPRECATED and will be removed in a future version. Use kubectl kubectl exec [POD] -- [COMMAND] instead.
19:44:58 Unable to use a TTY - input is not a terminal or the right kind of file

Should we just run kubectl exec without the -i option? /cc @devimc

The same is failing on other unrelated PRs as well, e.g., http://jenkins.katacontainers.io/job/kata-containers-2.0-vfio-PR/184/console

devimc · 2020-11-04T15:30:47Z

@bergwolf yeah I'm on that kata-containers/tests#3026

bergwolf · 2020-11-05T01:57:07Z

/test

This reverts commit 87848e8 as it is breaking the k8s configMap test. Fixex: kata-containers#1080 Signed-off-by: Peng Tao <bergwolf@hyper.sh>

bergwolf · 2020-11-05T06:49:02Z

/test

amshinde · 2020-11-05T20:02:06Z

/test

bergwolf · 2020-11-06T01:44:01Z

/test-vfio

bergwolf added needs-backport labels Oct 30, 2020

bergwolf force-pushed the ro-volume branch from d30d24b to f06a31a Compare October 30, 2020 10:20

jodh-intel approved these changes Oct 30, 2020

View reviewed changes

amshinde approved these changes Oct 30, 2020

View reviewed changes

amshinde added the do-not-merge PR has problems or depends on another label Oct 30, 2020

bergwolf mentioned this pull request Nov 2, 2020

k8s: validate readonly volumes kata-containers/tests#3023

Closed

This was referenced Nov 2, 2020

runtime: readonly volume should be bind mounted readonly on the host kata-containers/runtime#3041

Closed

readonly volume should be bind mounted readonly on the host kata-containers/runtime#3042

Merged

amshinde reviewed Nov 2, 2020

View reviewed changes

devimc reviewed Nov 3, 2020

View reviewed changes

Comment thread src/runtime/virtcontainers/mount.go Outdated

bergwolf force-pushed the ro-volume branch from f06a31a to 20d1bc9 Compare November 3, 2020 15:07

egernst approved these changes Nov 3, 2020

View reviewed changes

bergwolf force-pushed the ro-volume branch from 20d1bc9 to 5f256f3 Compare November 4, 2020 09:00

bergwolf added 2 commits November 4, 2020 17:51

runtime: readonly mounts should be readonly bindmount on the host

125e21c

So that we get protected at the VM boundary not just the guest kernel. Signed-off-by: Peng Tao <bergwolf@hyper.sh>

bergwolf force-pushed the ro-volume branch from 5f256f3 to 4d96d46 Compare November 4, 2020 09:52

bergwolf force-pushed the ro-volume branch from 4d96d46 to 56fa1b5 Compare November 5, 2020 01:56

version: revert back to crio 1.8.3

ff13bde

This reverts commit 87848e8 as it is breaking the k8s configMap test. Fixex: kata-containers#1080 Signed-off-by: Peng Tao <bergwolf@hyper.sh>

bergwolf force-pushed the ro-volume branch from 56fa1b5 to ff13bde Compare November 5, 2020 06:48

bergwolf removed the do-not-merge PR has problems or depends on another label Nov 6, 2020

liubin merged commit a68e200 into kata-containers:2.0-dev Nov 6, 2020

bergwolf deleted the ro-volume branch November 6, 2020 02:28

tatianab mentioned this pull request Nov 8, 2023

x/vulndb: potential Go vuln in github.com/kata-containers/kata-containers: CVE-2020-28914 golang/vulndb#2297

Closed

Conversation

bergwolf commented Oct 30, 2020

Uh oh!

bergwolf commented Oct 30, 2020

Uh oh!

bergwolf commented Oct 30, 2020

Uh oh!

jodh-intel left a comment

Choose a reason for hiding this comment

Uh oh!

amshinde commented Oct 30, 2020

Uh oh!

bergwolf commented Nov 2, 2020

Uh oh!

awprice commented Nov 2, 2020

Uh oh!

bergwolf commented Nov 2, 2020

Uh oh!

awprice commented Nov 2, 2020

Uh oh!

amshinde Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

bergwolf Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

awprice commented Nov 3, 2020

Uh oh!

devimc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bergwolf commented Nov 3, 2020

Uh oh!

bergwolf commented Nov 4, 2020

Uh oh!

bergwolf commented Nov 4, 2020

Uh oh!

bergwolf commented Nov 4, 2020

Uh oh!

bergwolf commented Nov 4, 2020

Uh oh!

bergwolf commented Nov 4, 2020

Uh oh!

devimc commented Nov 4, 2020

Uh oh!

bergwolf commented Nov 5, 2020

Uh oh!

bergwolf commented Nov 5, 2020

Uh oh!

amshinde commented Nov 5, 2020

Uh oh!

bergwolf commented Nov 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants