cmd/snap-confine: prevent cwd restore permission bypass #6642

zyga · 2019-03-22T19:48:46Z

It was brought to our attention that specially crafted current working
directory can be used to bypass permissions checks on a multi-user
system. The principal reason for that was that snap-confine, during the
course of execution, changes the current working directory, either
explicitly or implicitly (e.g. via setns) and then tries to restore the
original working directory while executing with root privileges.

The recently introduced central handling of umask is now extended to
handle current working directory. The code replaces individual
implementation in several other places in the codebase.

The new implementation is better than the original in two ways. First
of all, it works based on a different principle which is discussed
below. Second of all, it runs late, after the original user and group
IDs have been restored (that is, it no longer runs as privileged user).

The new method has one of three possible outcomes:

The original working directory cannot be represented in the execution
environment. The process is moved to /var/lib/snapd/void
The original working directory can be represented in the execution
environment but it now points to a different inode. The new inode is
used and a filesystem permission check is performed by the kernel.
The original working directory can be represented in the execution
environment and points to the same inode. The same inode is used.

Two tests were affected by this change as they were running commands
inside a LXD container as the root user, with the initial working
directory set to /root, then using su to switch to the ubuntu user.
Those commands were changed to go to /home/ubuntu instead.

To show this all in operation I've added a new test that verifies each
of the variant cases.

Signed-off-by: Zygmunt Krynicki [email protected]
Related-To: https://bugzilla.suse.com/show_bug.cgi?id=1127368

It was brought to our attention that specially crafted current working directory can be used to bypass permissions checks on a multi-user system. The principal reason for that was that snap-confine, during the course of execution, changes the current working directory, either explicitly or implicitly (e.g. via setns) and then tries to restore the original working directory while executing with root privileges. The recently introduced central handling of umask is now extended to handle current working directory. The code replaces individual implementation in several other places in the codebase. The new implementation is better than the original in two ways. First of all, it works based on a different principle which is discussed below. Second of all, it runs late, after the original user and group IDs have been restored (that is, it no longer runs as privileged user). The new method has one of three possible outcomes: 1) The original working directory cannot be represented in the execution environment. The process is moved to /var/lib/snapd/void 2) The original working directory can be represented in the execution environment but it now points to a different inode. The new inode is used and a filesystem permission check is performed by the kernel. 3) The original working directory can be represented in the execution environment and points to the same inode. The same inode is used. Two tests were affected by this change as they were running commands inside a LXD container as the root user, with the initial working directory set to /root, then using su to switch to the ubuntu user. Those commands were changed to go to /home/ubuntu instead. To show this all in operation I've added a new test that verifies each of the variant cases. Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga · 2019-03-22T19:49:06Z

This branch is stacked on top of #6636
The principal new commit is 2fcfef2 -- please review that

cmd/snap-confine/snap-confine.c

bboozzoo · 2019-03-25T09:44:25Z

cmd/snap-confine/snap-confine.c

+	if (nread == sizeof orig_cwd) {
+		die("cannot fit symbolic link target %s", fd_path);
+	}
+


For the sake of debugging, I'd add:

debug("original cwd in parent namespace: %s", orig_cwd);

We cannot do that, that string is not terminated.

I meant adding it below:

if (nread == sizeof orig_cwd) { die("cannot fit symbolic link target %s", fd_path); } debug("original cwd in parent namespace: %s", orig_cwd);

Ah, but that is logged in several cases below?

cmd/snap-confine/snap-confine.c

Signed-off-by: Zygmunt Krynicki <[email protected]>

jdstrand

Thanks for your work on this. Comments inline :)

cmd/snap-confine/snap-confine.c

tests/main/lxd/task.yaml

tests/regression/lp-1815869/task.yaml

We use O_PATH so it doesn't matter much but we can be stricter by passing O_DIRECTORY when we open with the current working directory. Signed-off-by: Zygmunt Krynicki <[email protected]>

Signed-off-by: Zygmunt Krynicki <[email protected]>

Inspect the current working directory in the initial mount namespace. While this doesn't change anything it is more natural and more resilient to breakage in case kernel changes semantics of fstatfs invoked across unshare/setns. Signed-off-by: Zygmunt Krynicki <[email protected]>

When originally implementing the first patch in this series I was considering using the file descriptor for the current working directory in the original mount namespace so I wanted to differentiate the cases where the inode it points to now and before is different from that when they are the same. Over time I realised that I cannot use fchdir with the original descriptor and implemented separate cases for fchdir + chdir but in doing so I didn't realise that fchdir and chdir are equivalent here because the path descriptor used is from the per-snap mount namespace and that it is opened just moments before calling fchdir. As such the whole fragment can be reduced to unconditional fchdir with the inode check serving just as UX differentiation. Signed-off-by: Zygmunt Krynicki <[email protected]>

Nothing is changed but it's clearer what is going on. Signed-off-by: Zygmunt Krynicki <[email protected]>

Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga · 2019-03-26T06:19:08Z

The new test is failing in the most interesting way:

2019-03-25 21:54:10 Error executing google:ubuntu-core-18-64:tests/main/pwd : 
-----
++ cd /root
++ test-snapd-sh -c pwd
+ test /root = /root
++ cd /root
++ test-snapd-sh -c 'stat -c %i .'
++ cd /root
++ stat -c %i .
+ test 7127 = 7127
++ cd /tmp
++ test-snapd-sh -c pwd
+ test /tmp = /tmp
++ cd /tmp
++ test-snapd-sh -c 'stat -c %i .'
++ cd /tmp
++ stat -c %i .
+ test 22 '!=' 2
++ mkdir -p /tmp/test
++ cd /tmp/test
++ test-snapd-sh -c pwd
cannot move to fallback directory /var/lib/snapd/void: No such file or directory
+ test '' = /var/lib/snapd/void

There's no void directory on core18!

bboozzoo · 2019-03-26T06:58:47Z

There's quite a difference in the contents of /var/lib/snapd between core and core18:

$ tree /snap/core/current/var/lib/snapd 
/snap/core/current/var/lib/snapd
├── apparmor
│   └── snap-confine
├── auto-import
├── desktop
├── environment
├── firstboot
├── hostfs
├── lib
│   ├── gl
│   ├── gl32
│   ├── glvnd
│   └── vulkan
├── snaps
│   └── partial
└── void

15 directories, 0 files
$ tree /snap/core18/current/var/lib/snapd
/snap/core18/current/var/lib/snapd
├── apparmor
│   └── profiles
└── hostfs

3 directories, 0 files

When the invoking user has no permission to stay in a directory (e.g. a symlink attack in /tmp or a su invocation from /root) then instead of failing the whole command chain, move the invoked process to the void directory. The void directory is empty and has no permissions to do anything inside. The set of affected tests were changed not to require a directory change anymore. In addition the pwd test was expanded to cover this specific situation. Signed-off-by: Zygmunt Krynicki <[email protected]>

Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga · 2019-03-26T12:18:42Z

I will make the required changes to core18 to make this test pass.

jdstrand

Thanks!

jdstrand · 2019-03-26T13:00:48Z

cmd/snap-confine/snap-confine.c

+	}
+	if (inner_cwd_fd < 0) {
+		debug("cannot represent original working directory %s", orig_cwd);
+		goto the_void;


cmd/snap-confine/snap-confine.c

jdstrand · 2019-03-26T13:06:51Z

cmd/snap-confine/snap-confine.c

+	}
+	return;
+the_void:
+	if (chdir(sc_void_dir) < 0) {


Of course, if you are root and not confined, then this could be raced, but unconfined root can just do that without the race, so not a problem.

zyga · 2019-03-27T14:23:39Z

I found some more issues here. I'm iterating on this now.

The void directory cannot be 000 anymore. When snap-confine chooses to restore the orignal working directory inside the mount namespace it must do so without abusing the root power it wields. As such it drops extra permissions and moves to the desired directory as the invoking user. This can fail for lack of permissions to access said directory. When this happens, instead of aborting execution, snap-confine moves to the special fallback directory, the void. This raises the problem that it cannot access the void directory as originally designed (with mode 000) so something must yield. We chose to yield the permissions by changing them to 111, that is, anyone can browse but nothing else. The change is done across many layers of packaging and test preparation but even that is insufficient. What we noticed is that the originally desired permissions were not really applied. I will separately address this in core and core18 so that the directory exists and with correct permissions but for now this should unblock the effort. Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga · 2019-03-28T21:30:18Z

This failed in an odd way I didn't see locally. It would imply that the user test has permissions to be in /root, which is unexpected. I will investigate tomorrow.

++ cd /root
++ su -c 'snap run test-snapd-sh -c pwd' test
+ test /root = /var/lib/snapd/void

zyga · 2019-04-02T11:38:00Z

I'm debugging this now.

Signed-off-by: Zygmunt Krynicki <[email protected]>

The cwd test detected that core18 didn't ship the skeleton directory structure required by snapd. Since that was fixed the corresponding workaround can be now removed. Signed-off-by: Zygmunt Krynicki <[email protected]>

When the cwd test fails it can do so because of incorrect permissions in the image. Adding a debug section will allow us to track down the problem. Signed-off-by: Zygmunt Krynicki <[email protected]>

The test setup was incorrectly creating an open-access /root directory that is not representative of a real core or classic system. This patch fixes that. Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga · 2019-04-04T09:20:08Z

This should be fixed now.

tests/lib/prepare.sh

bboozzoo · 2019-04-04T10:54:29Z

packaging/fedora/snapd.spec

@@ -762,7 +762,7 @@ popd
 %{_mandir}/man8/snap-confine.8*
 %{_mandir}/man8/snap-discard-ns.8*
 %{_systemdgeneratordir}/snapd-generator
-%attr(0000,root,root) %{_sharedstatedir}/snapd/void
+%attr(0111,root,root) %{_sharedstatedir}/snapd/void


Shall we double check in test/upgrade/basic that the permissions are actually updated?

Not in this pull request. This is not attempting to make that work across the distributions, just in our internal test suite. There are distributions that are known to have this broken, most notably ubuntu and ubuntu-core16

bboozzoo · 2019-04-04T10:57:18Z

packaging/arch/PKGBUILD

@@ -166,7 +166,7 @@ package() {
  install -dm755 "$pkgdir/var/lib/snapd/lib/vulkan"
  install -dm755 "$pkgdir/var/lib/snapd/lib/glvnd"
  # these dirs have special permissions
-  install -dm000 "$pkgdir/var/lib/snapd/void"
+  install -dm111 "$pkgdir/var/lib/snapd/void"


Thanks for updating this. I'll need to check whether pacman does the right thing on upgrade though.

I'm sure we need to update all non-RPM distribution packaging to behave correctly. I doubt this is correct yet.

cmd/snap-confine/snap-confine.c

Maciej Borzęcki rightfully noted that we can fail to open the path (using O_PATH flag) when we don't have permissions to navigate the directory chain. Instead of dying like we do on most errors, we will now go to the void directory. This is more in line with similar case when fchdir fails to change the directory for permission reasons. Signed-off-by: Zygmunt Krynicki <[email protected]>

The -m option applies only to the leaf element. The -p option means that we can create additional components. Shellcheck recommends against that. Signed-off-by: Zygmunt Krynicki <[email protected]>

bboozzoo · 2019-04-04T14:12:25Z

cmd/snap-confine/snap-confine.c

+	    open(orig_cwd, O_PATH | O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW);
+	if (inner_cwd_fd < 0) {
+		if (errno == EPERM || errno == EACCES || errno == ENOENT) {
+			debug


Wow, indent really does unspeakable things to the code.

Yes... I'm on the edge of using clang-format for snap-confine.c because this is just silly.

On top of that, it's not stable, run indent a few times and you get different output.

zyga · 2019-04-05T09:15:29Z

This is still failing on core18 because it uncovered a deeper problem about core18 and directory layout of the initial mount namespace. We need to discuss possible solutions with @mvo5 and @pedronis before this can proceed.

jdstrand

I reviewed all the changes since my last review (thanks @bboozzoo for the O_PATH find :) and they look fine. Approving so long as the tests pass.

jdstrand · 2019-04-05T20:46:47Z

tests/lib/prepare.sh

@@ -471,6 +480,9 @@ EOF
    # - make sure the group matches
    # - bind mount /root/test-etc/* to /etc/* via custom systemd job
    # We also create /var/lib/extrausers/* and append ubuntu,test there
+    ! test -e /mnt/system-data/root
+    mkdir -m 700 /mnt/system-data/root
+    test -d /mnt/system-data/root


This reminded me that for many years Ubuntu shipped an 0755 /root directory. It's possible that other distros might do this today. That obviously doesn't affect this hunk of the test so just fyi.

jdstrand · 2019-04-05T20:51:17Z

cmd/snap-confine/snap-confine.c

+	inner_cwd_fd =
+	    open(orig_cwd, O_PATH | O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW);
+	if (inner_cwd_fd < 0) {
+		if (errno == EPERM || errno == EACCES || errno == ENOENT) {


This was a nice find. From 'man open' - Opening a file or directory with the O_PATH flag requires no permissions on the object itself (but does require execute permission on the directories in the path prefix)

On core systems the directory /var/lib/snapd/void may not be present. When it is absent it must be created by snap-confine on demand. Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga · 2019-04-08T08:50:59Z

I force pushed to fix a typo that was upsetting the spell checker.

The void directory is created by the test so we cannot check for it early. We can and should check for it late though. Signed-off-by: Zygmunt Krynicki <[email protected]>

I had earlier placed the definition of the variable across a goto jump that clang, probably rightfully, complains about. The jump supposedly jumps over the initialization so to stay safe, move the initialization and indeed the definition, earlier. Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga · 2019-04-08T11:39:56Z

Force pushed to correct two misspellings in the last commit message.

jdstrand · 2019-04-08T18:15:13Z

cmd/snap-confine/snap-confine.c

@@ -180,6 +180,7 @@ static void sc_restore_process_state(const sc_preserved_process_state *
 	/* If the original working directory cannot be used for whatever reason then
 	 * move the process to a special void directory. */
 	const char *sc_void_dir = "/var/lib/snapd/void";
+	int void_dir_fd SC_CLEANUP(sc_cleanup_close) = -1;


This is fine, but perhaps it makes sense to declare this nearer its first use. Eg:

the_void: /* The void directory may be absent. On core18 system, and other * systems using bootable base snap coupled with snapd snap, the * /var/lib/snapd directory structure is not provided with packages but * created on demand. */ int void_dir_fd SC_CLEANUP(sc_cleanup_close) = -1; void_dir_fd = open(sc_void_dir, O_DIRECTORY | O_PATH | O_NOFOLLOW | O_CLOEXEC); ...

Not a blocker.

This is invalid C. Labels cannot denote variable declarations. I tried that earlier :)

zyga mentioned this pull request Mar 24, 2019

cmd: prevent umask from breaking snap-run chain #6636

Merged

pedronis self-requested a review March 25, 2019 09:01

bboozzoo reviewed Mar 25, 2019

View reviewed changes

zyga added 4 commits March 25, 2019 17:57

Merge branch 'fix/suse-audit-5' into fix/suse-audit-2

4128481

Merge branch 'fix/suse-audit-5' into fix/suse-audit-2

29397b7

cmd/snap-confine: add missing fd subdirectory

ce983c2

Signed-off-by: Zygmunt Krynicki <[email protected]>

cmd/snap-confine: fix typo "than"

d6ec606

Signed-off-by: Zygmunt Krynicki <[email protected]>

jdstrand suggested changes Mar 25, 2019

View reviewed changes

zyga added 10 commits March 25, 2019 21:15

Merge branch 'master' into fix/suse-audit-2

6a35141

cmd/snap-confine: use O_DIRECTORY for opening cwd

9f55031

We use O_PATH so it doesn't matter much but we can be stricter by passing O_DIRECTORY when we open with the current working directory. Signed-off-by: Zygmunt Krynicki <[email protected]>

cmd/snap-confine: reword paragraph entry

6920052

Signed-off-by: Zygmunt Krynicki <[email protected]>

cmd/snap-confine: remove redundant paragraph

2336a77

Signed-off-by: Zygmunt Krynicki <[email protected]>

cmd/snap-confine: use definite article for the void dir

0fe6571

Signed-off-by: Zygmunt Krynicki <[email protected]>

cmd/snap-confine: explicit ENOENT handling

7d63105

Nothing is changed but it's clearer what is going on. Signed-off-by: Zygmunt Krynicki <[email protected]>

cmd/snap-confine: reindent new code

c373fa6

Signed-off-by: Zygmunt Krynicki <[email protected]>

cmd/snap-confine: comment when changing state

47a5be8

Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga added 2 commits March 26, 2019 13:17

tests: remove trailing whitespace

7098613

Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga requested a review from jdstrand March 26, 2019 12:28

jdstrand approved these changes Mar 26, 2019

View reviewed changes

pedronis approved these changes Mar 26, 2019

View reviewed changes

zyga added 5 commits April 4, 2019 09:25

tests: rename pwd to cwd

ddaf146

Signed-off-by: Zygmunt Krynicki <[email protected]>

tests: remove core18 workaround

773d1d8

The cwd test detected that core18 didn't ship the skeleton directory structure required by snapd. Since that was fixed the corresponding workaround can be now removed. Signed-off-by: Zygmunt Krynicki <[email protected]>

tests: show permissions on failure

e82323d

When the cwd test fails it can do so because of incorrect permissions in the image. Adding a debug section will allow us to track down the problem. Signed-off-by: Zygmunt Krynicki <[email protected]>

Merge branch 'master' of github.com:snapcore/snapd into fix/suse-audit-2

803da81

tests: system-data/root should be 0755

07cd4fa

The test setup was incorrectly creating an open-access /root directory that is not representative of a real core or classic system. This patch fixes that. Signed-off-by: Zygmunt Krynicki <[email protected]>

bboozzoo reviewed Apr 4, 2019

View reviewed changes

tests/lib/prepare.sh Outdated Show resolved Hide resolved

bboozzoo reviewed Apr 4, 2019

View reviewed changes

zyga added 2 commits April 4, 2019 15:36

tests: use mkdir -m without -p

b05a2aa

The -m option applies only to the leaf element. The -p option means that we can create additional components. Shellcheck recommends against that. Signed-off-by: Zygmunt Krynicki <[email protected]>

bboozzoo approved these changes Apr 4, 2019

View reviewed changes

pedronis self-requested a review April 5, 2019 09:38

jdstrand approved these changes Apr 5, 2019

View reviewed changes

cmd/snap-confine: create the void directory on demand

47018f0

On core systems the directory /var/lib/snapd/void may not be present. When it is absent it must be created by snap-confine on demand. Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga force-pushed the fix/suse-audit-2 branch from 53dd431 to 47018f0 Compare April 8, 2019 08:48

zyga added 2 commits April 8, 2019 11:44

tests: move early check for void dir

44a0f52

The void directory is created by the test so we cannot check for it early. We can and should check for it late though. Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga force-pushed the fix/suse-audit-2 branch from b5e50b0 to 6c81b9b Compare April 8, 2019 11:39

pedronis requested a review from jdstrand April 8, 2019 12:52

jdstrand approved these changes Apr 8, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into fix/suse-audit-2

c9edbe5

pedronis removed their request for review April 10, 2019 17:14

zyga merged commit 187893d into canonical:master Apr 10, 2019

zyga deleted the fix/suse-audit-2 branch April 10, 2019 18:48

tatianab mentioned this pull request Nov 8, 2023

x/vulndb: potential Go vuln in github.com/snapcore/snapd: CVE-2019-11503 golang/vulndb#2233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/snap-confine: prevent cwd restore permission bypass #6642

cmd/snap-confine: prevent cwd restore permission bypass #6642

zyga commented Mar 22, 2019 •

edited

Loading

zyga commented Mar 22, 2019 •

edited

Loading

bboozzoo Mar 25, 2019

zyga Mar 25, 2019

bboozzoo Mar 25, 2019

zyga Mar 25, 2019

jdstrand left a comment

zyga commented Mar 26, 2019

bboozzoo commented Mar 26, 2019

zyga commented Mar 26, 2019

jdstrand left a comment •

edited

Loading

jdstrand Mar 26, 2019

jdstrand Mar 26, 2019

zyga commented Mar 27, 2019

zyga commented Mar 28, 2019

zyga commented Apr 2, 2019

zyga commented Apr 4, 2019

bboozzoo Apr 4, 2019

zyga Apr 4, 2019

bboozzoo Apr 4, 2019

zyga Apr 4, 2019

bboozzoo Apr 4, 2019

zyga Apr 4, 2019

zyga commented Apr 5, 2019

jdstrand left a comment

jdstrand Apr 5, 2019

jdstrand Apr 5, 2019 •

edited

Loading

zyga commented Apr 8, 2019

zyga commented Apr 8, 2019

jdstrand Apr 8, 2019

zyga Apr 9, 2019

jdstrand Apr 9, 2019

cmd/snap-confine: prevent cwd restore permission bypass #6642

cmd/snap-confine: prevent cwd restore permission bypass #6642

Conversation

zyga commented Mar 22, 2019 • edited Loading

zyga commented Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand left a comment

Choose a reason for hiding this comment

zyga commented Mar 26, 2019

bboozzoo commented Mar 26, 2019

zyga commented Mar 26, 2019

jdstrand left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga commented Mar 27, 2019

zyga commented Mar 28, 2019

zyga commented Apr 2, 2019

zyga commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga commented Apr 5, 2019

jdstrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

zyga commented Apr 8, 2019

zyga commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga commented Mar 22, 2019 •

edited

Loading

zyga commented Mar 22, 2019 •

edited

Loading

jdstrand left a comment •

edited

Loading

jdstrand Apr 5, 2019 •

edited

Loading