Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 30 commits into from Mar 14, 2019

Conversation

bboozzoo
Copy link
Collaborator

@bboozzoo bboozzoo commented Dec 6, 2018

Followup on #6238. The PR contains a refactored SELinux policy for snapd.

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.

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)

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.

(cc @Conan-Kudo)

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>
…grade

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Add domain transitions for directories created by snap tools when invoked by a
regular user who is typically in unconfined_t domain.

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

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small issues...

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary, since HOME_DIR applies to /root too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about that. I recall seeing separate rules for both HOME_DIR and /root in the core policy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay.

tests/main/selinux-data-context/task.yaml Show resolved Hide resolved
summary: Check that correct SELinux file contexts were assigned on upgrade

# TODO: enable centos once we build the policy there too
systems: [fedora-*]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to build the policy for CentOS now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the only thing that's left that needs to be fixed here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I told it to be pushed, so it should sync out within the next 20 hours.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll update the test once the package is available.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or not... I forgot to switch the build to the one with backported EL7 fixes. It needs a single +1 karma to make it go into stable now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though since 2.36.3 is out, I should go ahead and build that and update everybody...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better. Ping me when you upload a new update. I'll play with it in a VM and will post karma.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snapd 2.36.3 is in the EPEL7 repo now.

/usr/libexec/snapd/snapctl -- gen_context(system_u:object_r:snappy_cli_exec_t,s0)
/usr/libexec/snapd/snap-confine -- gen_context(system_u:object_r:snappy_confine_exec_t,s0)
/usr/libexec/snapd/snap-update-ns -- gen_context(system_u:object_r:snappy_mount_exec_t,s0)
/usr/libexec/snapd/snap-discard-ns -- gen_context(system_u:object_r:snappy_mount_exec_t,s0)
/usr/libexec/snapd/.* -- gen_context(system_u:object_r:snappy_exec_t,s0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the ordering matter here? Should the generic entry be first, and then the specific entries after?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the order doesn't matter AFAIK. I'd leave the /usr/libexec/snapd/.* last to not confuse people.

/usr/lib/snapd/snapctl -- gen_context(system_u:object_r:snappy_cli_exec_t,s0)
/usr/lib/snapd/snap-confine -- gen_context(system_u:object_r:snappy_confine_exec_t,s0)
/usr/lib/snapd/snap-update-ns -- gen_context(system_u:object_r:snappy_mount_exec_t,s0)
/usr/lib/snapd/snap-discard-ns -- gen_context(system_u:object_r:snappy_mount_exec_t,s0)
/usr/lib/snapd/.* -- gen_context(system_u:object_r:snappy_exec_t,s0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the ordering matter here? Should the generic entry be first, and then the specific entries after?

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
The SELinux core policy shipped by RHEL7.6 (3.13.1-229.el7_6.6) is older than
what is in Fedora 29 (3.14.2-42.fc29) and some interfaces are missing. Update
snapd type enforcement so that it can be built with the older policy.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Now that we build the policy on CentOS too, the test can be run there too.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
…s files and directories

Add a snap that generates files and directories under $SNAP_COMMON and
$SNAP_DATA both in the service and in the hook. This is to probe for problems
with SELinux policy.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Extend the test to cover snaps with services and hooks inside. The hook is
started from snapd, while the service is started by systems. This may uncover
potential issues with policy transitions.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
The output of `ls -Zd` differs between versions of coreutils.

$ mkdir foo; ln -s foo bar

On CentOS 7.6, coreutils 8.22:

$ ls -Zd foo bar
lrwxrwxrwx. root root unconfined_u:object_r:home_root_t:s0 bar -> foo
drwxr-xr-x. root root unconfined_u:object_r:home_root_t:s0 foo

On Fedora 29, coreutils 8.30:

$ ls -Zd foo bar
unconfined_u:object_r:user_home_t:s0 bar
unconfined_u:object_r:user_home_t:s0 foo

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Systemd caches SELinux context information and when a policy is updated, the
changes are not applied by systemd until a reboot or daemon reexecs (thus
reloading the policy)

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Socket activation is finnicky. Make sure that the socket is created with proper
label.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Update and tweak the policy. Relevant fixes:
- allow snapd to run binaries from core
  - specifically, add rules for running fc-cache and link to a bug in
    selinux-policy
- allow snapd to run journalctl and transition to journalctl_t domain
- tweak snap-{update,discard}-ns profile
- allow snap-confine to setuid(), setgid()
- allow snap to poke policy to restore context of ~/snap
- allow systemd to create sockets in snappy_var_t, and sockets for snappy_cli_t,
  IOW make socket activation work

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
snapd is in EPEL now

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Workaround differnt ls -Z output in old coreutils (CentOS).

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

@stolowski stolowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! As discussed, it can land provided that we also land a solution to migrate currently running services on rpm update.

@@ -0,0 +1,33 @@
summary: Check that correct SELinux file contexts were assigned on upgrade
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@@ -0,0 +1,78 @@
summary: Check that SELinux file context transitions work
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test!

@bboozzoo
Copy link
Collaborator Author

@Conan-Kudo I hope to be able to start landing this stuff now. Can you take a look again at this PR and #6238 ?

snapd can be installed from EPEL on CentOS

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@bboozzoo bboozzoo merged commit 104d50b into snapcore:master Mar 14, 2019
@Conan-Kudo
Copy link
Contributor

🎆

@bboozzoo
Copy link
Collaborator Author

Yeah, the first bit is out. Now I need to push the rest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants