[WIP] many: add minimal SELinux support, refactor the policy #6238

bboozzoo · 2018-11-29T10:03:04Z

A week long crash course in SELinux gave birth to this larg(-ish) branch that introduces a minimal SELinux support and does a major refactoring to SELinux targeted policy for snapd.

At this point I am able to start/stop snapd cleanly and install/remove snaps, both simple tools and test-snapd-service, cleanly. All of the denials that were logged to audit are gone, meaning stuff was mostly accounted for, fixed, or the policy got extended where needed.

Major changes:

policy introduces a separate domain for snapd and each type of helper (mount, confine, cli)
each domain has a separate policy and allows only specific transitions
there is a separate file type for data in mounted snap images (snappy_snap_t, for the lack of a better name)
snaps are applied a SELinux context on mount, though I am not sure yet, how we are going to upgrade people to the new mount units
snap-confine learned some SELinux tricks, relabeling of /run/snapd which caused many issues, and process transitions on exec
snap does relabeling of ~/snap in case there is data there, but otherwise the policy contains the necessary file transition
a minimal detection of SELinux was added to snapd release package
sanity checks apply proper SELinux context option when mounting the canary image
snapd also applies a proper SELinux context for generated mount units

Some notes about transitions and starting snaps. When a regular user starts a snap, they are typically in unconfined_u:unconfined_r:unconfined_t context. This proceeds without any special transitions or limitations, since the user is already unconfined, and there is no targeted policy to close it down. The snaps that are started by systemd end up transitioning to system_u:system_r:unconfined_service_t domain, where they should not raise any more denials. While doing so, the transitions are:

init_t -> snappy_cli_t (snap run ..) -> snappy_confine_t -> ..
   ... -> snappy_mount_t (snap-update-ns) and back -> 
   ... -> unconfined_service_t

Domains permissions are limited to just the things each tool does. With the split in place, it's way easier to track which piece is doing what.

I will be splitting this up into smaller PRs, but some patches depend on one another. Specifically, pathches introducing the use of SELinux context labels need the policy to exist before.

On top of this, I used Fedora 29 as dev environment. When transitioning to CentOS 7.6, the policy should be up to date, but it may end up needing some tweaks.

There is a spread test for checking if audit logs are clean while doing basic snap operations coming up.

I'm by no means expert in SELinux, so feedback on the policy and libselinux integration is welcome.

(cc @Conan-Kudo)

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

The SELinux policy of snapd allows certain operations on objects that are in snappy_home_t domain. The trouble is that when a user runs a snap, and ~/snap does not exist, it gets created using the parent directory context, which is user_home_t, causing the policy rules fail to match. We could update the policy to allow manipulation of user_home_t but that would open the whole of user's home directory. Instead, try to restore the proper context on each run, which should DTRT for the newly created directory and ones that already exist. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Avoid picking up xattrs when repacking core snap. On systems supporting SELinux this may accidentally pick up the file context from where the core snap was initially unpack. In case of tests, the core snap is unpacked to test home directory and the repacked snap files have user_home_t context. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Refactor SELinux policy for snapd and helpers. To make it more manageable, split snapd and the helpers into separate domains: snappy_t (snapd), snappy_mount_t (snap-{update,discard}-ns), and snappy_cli_t (snap{,ctl}). Add separate local policy for each helper, trying to keep it minimal where possible. Also update file contexts so that they can be properly relabeled. Introuduce a separate file type for snap data, snappy_snap_t which is aimed to be used a mount context for snap images. This ensures that the snaps will be properly labeled and we can write policy that uses this type. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Add minimal support for running on SELinux systems. Take care to relabel the snap /run/snapd directory to ensure targeted policy rules can be properly matches to file contexts. When starting a snap application under snappy_confine_t domain , move it to unconfined_service_t upon next exec(). This makes snap services run under a domain that is not confined by SELinux and reflects the state of our SELinux support. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Conan-Kudo

Overall, this PR looks great to my (weakly trained) eyes! Thank you so much for doing this! Just a couple of small things...

Conan-Kudo · 2018-11-29T10:43:27Z

data/selinux/snappy.fc

@@ -18,30 +18,38 @@


 HOME_DIR/snap(/.*)?			gen_context(system_u:object_r:snappy_home_t,s0)
+/root/snap(/.*)?			gen_context(system_u:object_r:snappy_home_t,s0)


Why is this necessary? For the root user, HOME_DIR should equal /root...

Conan-Kudo · 2018-11-29T10:47:41Z

data/selinux/snappy.fc

 /usr/lib/snapd/.*  		--	gen_context(system_u:object_r:snappy_exec_t,s0)
 /etc/default/snapd		-- 	gen_context(system_u:object_r:snappy_config_t,s0)
 /lib/systemd/system/snapd.* 	--	gen_context(system_u:object_r:snappy_unit_file_t,s0)
 ')

-/var/run/snapd(/.*)?	        --	gen_context(system_u:object_r:snappy_var_run_t,s0)
+/var/run/snapd(/.*)?	        gen_context(system_u:object_r:snappy_var_run_t,s0)


Why'd we drop the -- here? We want to match on everything in here now?

@Conan-Kudo,

This will match also all objects inside the directory /var/run/snapd. This is good fix.

The `with_selinux` macro is always defined (either 1 or 0), so empty expansion %{?..} will not work as expected. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga

Quick pass over the C parts

zyga · 2018-11-29T14:27:43Z

cmd/Makefile.am

+	snap-confine/selinux-support.h
+snap_confine_snap_confine_CFLAGS += $(SELINUX_CFLAGS)
+if STATIC_LIBSELINUX
+snap_confine_snap_confine_STATIC += $(shell $(PKG_CONFIG) --static --libs libselinux)


Is linking to selinux statically a safe thing to do?

zyga · 2018-11-29T14:28:53Z

cmd/snap-confine/selinux-support.c

+
+int sc_selinux_relabel_run_dir(void)
+{
+	if (is_selinux_enabled() < 1) {


https://linux.die.net/man/3/is_selinux_enabled says that < 0 is an error. Should we die on that case?

zyga · 2018-11-29T14:29:29Z

cmd/snap-confine/selinux-support.c

+				     SELINUX_RESTORECON_RECURSE |
+				     SELINUX_RESTORECON_IGNORE_MOUNTS |
+				     SELINUX_RESTORECON_XDEV);
+	if (ret == -1) {


Nitpick, most of snap-confine tries to be consistent in using < 0 for such check, would you mind changing that across the patch please?

zyga · 2018-11-29T14:30:24Z

cmd/snap-confine/selinux-support.c

+		return 0;
+	}
+
+	char *ctx_str = NULL;


Can you please add a selinux specific cleanup handler that calls freecon

This is based on https://linux.die.net/man/3/getcon`

zyga · 2018-11-29T14:31:04Z

cmd/snap-confine/selinux-support.c

+	}
+	debug("current context: %s", ctx_str);
+
+	context_t ctx = context_new(ctx_str);


Could you please initialise this to NULL, add a selinux specific cleanup handler that calls context_free. This is based on http://man7.org/linux/man-pages/man3/context_new.3.html

zyga · 2018-11-29T14:31:28Z

cmd/snap-confine/selinux-support.c

+		die("failed to create context from context string %s", ctx_str);
+	}
+
+	if (sc_streq(context_type_get(ctx), "snappy_confine_t")) {


Nitpick: I would call this snapd_t or snap_confine_t but definitely not snappy_confine_t

The context label matches the convention for the snappy module that governs snapd, snap-confine, and snaps. So, I think that's perfectly fine.

I just ... snappy_confine_t is hideous :)

Well, we could do snappy_snap_confine_t if you want...

This reverts commit 812cc7b.

The SELinux policy of snapd allows certain operations on objects that are in snappy_home_t domain. The trouble is that when a user runs a snap, and ~/snap does not exist, it gets created using the parent directory context, which is user_home_t, causing the policy rules fail to match. We could update the policy to allow manipulation of user_home_t but that would open the whole of user's home directory. Instead, try to restore the proper context on each run, which should DTRT for the newly created directory and ones that already exist. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…xes-merged-fine-grained

Add a helper to keep the dependencies limited. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…linux-fixes-merged-fine-grained

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…-fixes-merged-fine-grained

Add a spread test to monitor if basic snap management raises any SELinux denials. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…ged-fine-grained

Importing `dirs` package in `osutil` leads to import cycles if one attempts to use `osutils` from packages that are directly or indirectly imported by `dirs`, such as `release`. It doesn't make any sense for `osutil` to depend on other non `*util` packages. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…nux-fixes-merged-fine-grained

- allow snap to exec snap-seccomp (for deriving system-key) - allow snap to manage directories/links/files under ~/snap - tweak snapd permissions to add remove links under /usr/share/bash-completion/completions (which is of usr_t type) - tweak permissions of snap-confine (can do a great deal with tmp_t, but reads were not enabled) Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…xes-merged-fine-grained

Add a spread test to monitor if basic snap management raises any SELinux denials. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…issues Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Snap logs will trigger snapd to run journalct. On a SELinux system, this will cause a domain transition to journalctl_t. However, the policy does not allow journalctl to poke /proc data of pid 1, thus causing denials to appear in the log. Until this is fixed, all we can do is have a TODO note as a reminder. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

- allow snap to exec snap-seccomp (for deriving system-key) - allow snap to manage directories/links/files under ~/snap - tweak snapd permissions to add remove links under /usr/share/bash-completion/completions (which is of usr_t type) - tweak permissions of snap-confine (can do a great deal with tmp_t, but reads were not enabled) Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…ectory Add a second socket to the snap, but this time it should be created in a subdirectory. This should help uncover any problems with SELinux policy preventing systemd from creating subdirectories under $SNAP_{DATA,COMMON}. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…nux-fixes-merged-fine-grained

…boozzoo/selinux-fixes-merged-fine-grained

- allow managing char files under /var/snap, those could have been created by snaps such as lxd - allow fowner capabilty for snapd (snapshots?) - allow snap-{update,discard}-ns to read tmpfs symlinks, etc. /etc/os-release which is from /etc bind mounted on top of tmpfs Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…ns with selinux Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Tweak user.max_user_namespaces to enable some snaps, such as LXD, to work out of the box. See: https://access.redhat.com/solutions/3188102 for details. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

- allow querying SELinux enforcement mode - use proper interfaces to allow service reload/start/stop/enable/disable - account for cloud-init instance data access - account for /proc/sys/fs/may_detach_mounts probe in sanity - allow unliking incorrectly labeled socket files under /var/snap - account for polkit support poking /proc/<pid>/stat of the calling process Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

- allow snap-confine to create directories in ~/snap - allow dac_override for snap, when running as root Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Filed a bug to RHBZ Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…nal-policy-update-clean-test

…nux-fixes-merged-fine-grained

…boozzoo/selinux-fixes-merged-fine-grained

- allow managing char files under /var/snap, those could have been created by snaps such as lxd - allow fowner capabilty for snapd (snapshots?) - allow snap-{update,discard}-ns to read tmpfs symlinks, etc. /etc/os-release which is from /etc bind mounted on top of tmpfs Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

- allow querying SELinux enforcement mode - use proper interfaces to allow service reload/start/stop/enable/disable - account for cloud-init instance data access - account for /proc/sys/fs/may_detach_mounts probe in sanity - allow unliking incorrectly labeled socket files under /var/snap - account for polkit support poking /proc/<pid>/stat of the calling process Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

- allow snap-confine to create directories in ~/snap - allow dac_override for snap, when running as root Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…nal-policy-update-clean-test

…boozzoo/selinux-fixes-merged-fine-grained

…xes-merged-fine-grained

bboozzoo · 2019-04-12T07:17:53Z

The branch served it's purpose. The final bit up for a review in #6711.

bboozzoo added 12 commits November 29, 2018 10:41

spread: show AVC entries from audit log when debugging

a235863

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

spread: run tests on Fedora 28 again

9591d51

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

tests/lib/reset: restore context of removed snapd directories

c5aa915

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

spread: make sure that auditd is started on Fedora

c8837b2

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

release: detect SELinux

8a9df84

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

sanity: apply SELinux context when mounting a canary sqauashfs

63088ec

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

systemd: apply SELinux context when mounting snaps

d9b89db

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

packaging/fedora: add libselinux to build dependencies

54954fe

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Conan-Kudo approved these changes Nov 29, 2018

View reviewed changes

packaging/fedora: with_selinux is always defined, handle that properly

43690de

The `with_selinux` macro is always defined (either 1 or 0), so empty expansion %{?..} will not work as expected. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga reviewed Nov 29, 2018

View reviewed changes

bboozzoo added 2 commits December 5, 2018 12:00

Revert "tests/lib: do not pick up xatts when repacking core snap"

b69a632

This reverts commit 812cc7b.

bboozzoo mentioned this pull request Dec 5, 2018

cmd/snap: attempt to restore SELinux context of snap user directories #6265

Merged

bboozzoo added the SELinux label Dec 6, 2018

bboozzoo mentioned this pull request Dec 6, 2018

data/selinux, tests: refactor SELinux policy, add minimal tests #6270

Merged

bboozzoo added 8 commits December 10, 2018 10:13

Merge remote-tracking branch 'origin/master' into bboozzoo/selinux-fi…

5630f72

…xes-merged-fine-grained

release: add simple directory helper

640f998

Add a helper to keep the dependencies limited. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge branch 'bboozzoo/selinux-restorecon-user-data' into bboozzoo/se…

66c92e8

…linux-fixes-merged-fine-grained

cmd: fix bad merge

83c5d7b

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge branch 'bboozzoo/selinux-policy-refactor' into bboozzoo/selinux…

434cd52

…-fixes-merged-fine-grained

tests/main/selinux-clean: keep SELinux denials in check

b6b84d6

Add a spread test to monitor if basic snap management raises any SELinux denials. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

tests/main/selinux-clean: keep SELinux denials in check

8482b5e

Add a spread test to monitor if basic snap management raises any SELinux denials. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge branch 'bboozzoo/selinux-clean' into bboozzoo/selinux-fixes-mer…

37aa5fc

…ged-fine-grained

bboozzoo changed the title ~~[RFC] many: add minimal SELinux support, refactor the policy~~ [WIP] many: add minimal SELinux support, refactor the policy Dec 10, 2018

bboozzoo added 28 commits March 28, 2019 10:16

tests/upgrade/basic: drop snap pack

1210451

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge branch 'bboozzoo/selinux-mount-patch-helper' into bboozzoo/seli…

97a24ae

…nux-fixes-merged-fine-grained

Merge remote-tracking branch 'origin/master' into bboozzoo/selinux-fi…

ab69654

…xes-merged-fine-grained

tests/main/selinux-clean: keep SELinux denials in check

26210a5

Add a spread test to monitor if basic snap management raises any SELinux denials. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

tests/main/selinux-clean: install fonts to catch fc-cache invocation …

cb17df1

…issues Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

tests/main/selinux-clean: verify that socket activated snaps work

5fcff49

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

data/selinux: allow sys_admin for snap-confine

e7260e3

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge branch 'bboozzoo/selinux-mount-patch-helper' into bboozzoo/seli…

6904212

…nux-fixes-merged-fine-grained

Merge branch 'bboozzoo/selinux-final-policy-update-clean-test' into b…

e528f48

…boozzoo/selinux-fixes-merged-fine-grained

tests/main/selinux-lxd: add a test making sure that LXD from snaps ru…

e63666c

…ns with selinux Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

data/sysctl/rhel7-snap: tweak user.max_user_namespaces

f8bd00c

Tweak user.max_user_namespaces to enable some snaps, such as LXD, to work out of the box. See: https://access.redhat.com/solutions/3188102 for details. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

data/selinux: final tweak to the policy

7ce50cb

- allow snap-confine to create directories in ~/snap - allow dac_override for snap, when running as root Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

tests/main/selinux-lxd: workaround systemd map_create denial

fd53cd0

Filed a bug to RHBZ Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'origin/master' into bboozzoo/selinux-fi…

409c4cf

…nal-policy-update-clean-test

Merge branch 'bboozzoo/selinux-mount-patch-helper' into bboozzoo/seli…

7994a07

…nux-fixes-merged-fine-grained

Merge branch 'bboozzoo/selinux-final-policy-update-clean-test' into b…

3e3756a

…boozzoo/selinux-fixes-merged-fine-grained

data/selinux: final tweak to the policy

ab234f5

- allow snap-confine to create directories in ~/snap - allow dac_override for snap, when running as root Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'origin/master' into bboozzoo/selinux-fi…

c99d469

…nal-policy-update-clean-test

Merge branch 'bboozzoo/selinux-final-policy-update-clean-test' into b…

c6d7a58

…boozzoo/selinux-fixes-merged-fine-grained

Merge remote-tracking branch 'origin/master' into bboozzoo/selinux-fi…

d98d544

…xes-merged-fine-grained

bboozzoo closed this Apr 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] many: add minimal SELinux support, refactor the policy #6238

[WIP] many: add minimal SELinux support, refactor the policy #6238

bboozzoo commented Nov 29, 2018

Conan-Kudo left a comment •

edited

Conan-Kudo Nov 29, 2018

Conan-Kudo Nov 29, 2018

wrabcak Dec 3, 2018

zyga left a comment

zyga Nov 29, 2018

zyga Nov 29, 2018

zyga Nov 29, 2018

zyga Nov 29, 2018

zyga Nov 29, 2018

zyga Nov 29, 2018

Conan-Kudo Nov 29, 2018

zyga Nov 29, 2018

Conan-Kudo Nov 29, 2018

bboozzoo commented Apr 12, 2019

		@@ -18,30 +18,38 @@


		HOME_DIR/snap(/.*)? gen_context(system_u:object_r:snappy_home_t,s0)
		/root/snap(/.*)? gen_context(system_u:object_r:snappy_home_t,s0)

[WIP] many: add minimal SELinux support, refactor the policy #6238

[WIP] many: add minimal SELinux support, refactor the policy #6238

Conversation

bboozzoo commented Nov 29, 2018

Conan-Kudo left a comment • edited

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 left a 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

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

bboozzoo commented Apr 12, 2019

Conan-Kudo left a comment •

edited