core: add TemporaryFileSystem= setting and 'tmpfs' option to ProtectHome= #7908

yuwata · 2018-01-17T02:12:33Z

This introduces a new setting TemporaryFileSystem=. The unit process can access the specified directories, but cannot access the files stored in the directories.
Also, this adds a new option tmpfs to ProtectHome=, which is equivalent to TemporaryFileSystem=/root /run/user /home.

This is an important preparation to support #7895, #7153 and #2780.

vcaputo · 2018-01-17T04:47:32Z

src/core/execute.c

@@ -2295,6 +2294,12 @@ static int compile_bind_mounts(

        assert(h == n);

+        if (!strv_isempty(empty_directories) && !strv_isempty(context->empty_directories)) {


I admit I haven't spent much time looking at this code, but at a casual glance shouldn't context->empty_directories still be applied when empty_directories is empty at this point? It looks like a non-empty context->empty_directories will just be ignored by this function if empty_directories is empty. Is that intentional?

yeah, i think @vcaputo is right

vcaputo · 2018-01-17T04:49:59Z

src/core/execute.c

@@ -2369,7 +2374,7 @@ static int apply_mount_namespace(
                            &ns_info, context->read_write_paths,
                            needs_sandboxing ? context->read_only_paths : NULL,
                            needs_sandboxing ? context->inaccessible_paths : NULL,
-                            empty_directories,
+                            empty_directories ? : context->empty_directories,


Oh, is this why compile_bind_mounts() doesn't return empty_directories when context->empty_directories is the only one?

That's kind of ugly IMHO.

It might be. But this logic can reduce creating unnecessary copy of context->empty_directories.

vcaputo · 2018-01-17T04:51:02Z

test/test-execute/exec-emptydirectories.service

@@ -0,0 +1,22 @@
+[Unit]
+Description=Test for BindPaths= and BindReadOnlyPaths=


Unimportant nit, but I think this is stale from whatever was used as a template in creating this...

Oh, thanks. I will update.

vcaputo · 2018-01-17T04:52:34Z

I gave a casual look over the code, made some comments, but it generally lgtm.

yuwata · 2018-01-17T07:03:04Z

Also added ProtectHome=empty, which is equivalent to EmptyDirectories=/root /run/user /home.

poettering · 2018-01-17T11:12:34Z

Hmm, what's the difference and benefit over InaccessiblePaths=?

ludovic-gasc · 2018-01-17T14:46:40Z

Hi @poettering,

To my understanding, it's impossible to do that with InaccessiblePaths=: It works for read-only access limitations on the file system, but not for a complete black-out, like you have in a chroot, where you can see only specific folders.
Another example : https://unix.stackexchange.com/questions/305586/how-to-whitelist-directories-for-units-in-systemd/387051#387051
As you can read in the answer: "According to this quote, it doesn't work".

The main interest: With this option, it will be possible to build easily a new complete view on the filesystem without the need to maintain a separate chroot.

For the sysadmins, they can apply the security upgrades on the "host", all daemons will have an updated view of their part of root, not need to upgrade each container, with more or less the same security level of a "complete" container with a separate root.

We use intensively systemd features to reduce our number of VMs and containers.
Our final goal is to enable the maximum of hardening options available in systemd to increase the security of daemons we are using on production, to try to reduce the risk of 0-day attack.

I have started this project with bind, I'm discussing on their mailing-list about this: https://lists.isc.org/pipermail/bind-users/2018-January/099437.html

Thanks for your remarks.

topimiettinen · 2018-01-17T16:30:36Z

I think #2780 is also related.

topimiettinen · 2018-01-17T16:45:39Z

What about DAC permissions, ACLs, SELinux labels, mount flags (noexec/nodev/nosuid/ro/rw) of the original directory? Shouldn't they also be reapplied to the empty directory and/or let the admin configure them in the service file?

sourcejedi · 2018-01-17T16:55:14Z

Documentation here doesn't mention the permissions of the created tmpfs, i.e. who can write to it. I'm not saying it's wrong to set it to 0755, but that's not the default permission for a tmpfs, so I think the docs should be more explicit somehow.

EDIT: ninja'd

yuwata · 2018-01-18T13:08:23Z

Updated. Mainly adds more documentation.

@poettering
Example,

EmptyDirectories=/etc
ReadOnlyPaths=/etc/systemd

This hides any configuration under /etc except for /etc/systemd. Several RFEs request such a function, and we already internally use it for '/private' StateDirectory=. This just expose it as a new setting.

@topimiettinen Sorry, I do not have enough knowledge about them. What kind of information should be inherit from the original directory?

@sourcejedi Initially they are mounted with mode=755, but after that they are remounted as read-only.
I've updated the documents to mention that.

topimiettinen · 2018-01-20T19:02:31Z

@yuwata I think safest would be all of them, so that the result is not different from taking the original directory and removing its contents.

yuwata · 2018-01-22T09:05:49Z

@topimiettinen Do you concern them on just EmptyDirectories= setting? Or on BindPaths= ?

poettering · 2018-01-22T16:03:38Z

I can see the usecase, but I am not sure the name "EmptyDirectory=" is really that great for this, after all for it to be useful (i.e. to be more than InaccessibleDirectory=) it will not be empty. I mean, the fact that it doesn't have to be entirely empty, and doesn't cancel out any mounts below it is kinda the key feature of it, no?

In "systemd-nspawn" we already have the --tmpfs= option (which is exposes as TemporaryFileSystem= in .nspawn files), which pretty much is what you are trying to do here too, right? Maybe we should just reuse that name for the same concept here? i.e. introduce this as TemporaryFileSystem= option in unit files?

yuwata · 2018-01-22T16:06:04Z

@poettering Yeah, sounds good. I will rename them.

poettering · 2018-01-22T16:06:27Z

(also I have the suspicion this should take an optional mount option string too, the same way as nspawn's --tmpfs= takes one, separated by a colon)

poettering · 2018-01-22T16:04:57Z

TRANSIENT-SETTINGS.md

@@ -168,6 +168,7 @@ All execution-related settings are available for transient units.
 ✓ ReadWritePaths=
 ✓ ReadOnlyPaths=
 ✓ InaccessiblePaths=
+✓ EmptyDirectories=


the commit's title is wrong btw, it says "ExecDirectories=", but should say EmptyDirectories=

poettering · 2018-01-22T16:07:49Z

src/core/execute.c

@@ -2250,7 +2251,7 @@ static int compile_bind_mounts(
                         * directory. For that we overmount the usually inaccessible "private" subdirectory with a
                         * tmpfs that makes it accessible and is empty except for the submounts we do this for. */

-                        private_root = strjoin(params->prefix[t], "/private");
+                        private_root = strjoin("+", params->prefix[t], "/private");


hmm, why this?

poettering · 2018-01-22T16:09:34Z

src/core/execute.c

@@ -2295,6 +2294,12 @@ static int compile_bind_mounts(

        assert(h == n);

+        if (!strv_isempty(empty_directories) && !strv_isempty(context->empty_directories)) {


yeah, i think @vcaputo is right

ludovic-gasc · 2018-02-07T23:28:24Z

Hi @yuwata, I permitted me to send you a gentle ping in case of you have some time to answer on review questions :-)
I'm interested by this feature, but I don't have the technical level to contribute on it :-(
However, I can do some tests with it.

Have a nice week.

yuwata · 2018-02-08T02:15:27Z

@GMLudo I was tackling other issues recently. I guess that I can update this not so far future, I hope this weekend :-) Testing, commenting, etc., any feedbacks are of course welcome. Thank you.

It is not necessary to re-assign error code.

This is used in the later commits.

Each path in `Bind{ReadOnly}Paths=` accept '-' prefix. However, the prefix is completely ignored. This makes it work as expected.

This introduces a new setting TemporaryFileSystem=. This is useful to hide files not relevant to the processes invoked by unit, while necessary files or directories can be still accessed by combining with Bind{,ReadOnly}Paths=.

This make ProtectHome= setting can take 'tmpfs'. This is mostly equivalent to `TemporaryFileSystem=/home /run/user /root`.

yuwata · 2018-02-21T00:27:59Z

@poettering and @sourcejedi Thank you for reviewing this so many times. I've force-pushed a new version. Main changes are

drop bind_remount_recursive_with_mountinfo() for TMPFS and EMPTY_DIR,
drop unnecessary !! to cast from int to bool, and
man page fixes.

Note that still Read{Write,Only}Paths= cannot use with TemporaryFileSystem=. I have a plan to support this, but I want to do it in another PR.

sourcejedi · 2018-02-21T08:56:57Z

The one CI instance is just another kernel GPF.

I'll be interested to see what's stopping Read{Write,Only}Paths=, I didn't guess it. You can tag me in to that PR, if you're not fed up reading my comments :). Ok, let's try merging this now.

evverx · 2018-02-21T09:55:31Z

test/test-execute/exec-temporaryfilesystem-options.service

+
+# Check /proc/self/mountinfo
+ExecStart=/bin/sh -c 'test $$(awk \'$$5 == "/var" { print $$6 }\' /proc/self/mountinfo) = "ro,nodev,relatime"'
+ExecStart=/bin/sh -c 'test $$(awk \'$$5 == "/var" { print $$11 }\' /proc/self/mountinfo) = "ro,mode=700"'


I'm wondering why test-execute didn't fail on Fedora. There is usually seclabel in the 11th field of /proc/self/mountinfo, which makes it impossible for ro,mode=700 to pass the check.

exec-temporaryfilesystem-options.service: Passing 0 fds to service exec-temporaryfilesystem-options.service: About to execute: /bin/sh -c 'test $$(awk '$$5 == "/var" { print $$6 }' /proc/self/mountinfo) = "ro,nodev,relatime"' exec-temporaryfilesystem-options.service: Forked /bin/sh as 10524 Followed symlinks /run/systemd/unit-root/var → /run/systemd/unit-root/var. Applying namespace mount on /run/systemd/unit-root/var exec-temporaryfilesystem-options.service: Executing: /bin/sh -c 'test $(awk '$5 == "/var" { print $6 }' /proc/self/mountinfo) = "ro,nodev,relatime"' Received SIGCHLD from PID 10524 (sh). Child 10524 (sh) died (code=exited, status=0/SUCCESS) exec-temporaryfilesystem-options.service: Child 10524 belongs to exec-temporaryfilesystem-options.service. exec-temporaryfilesystem-options.service: Main process exited, code=exited, status=0/SUCCESS exec-temporaryfilesystem-options.service: Running next main command for state start. exec-temporaryfilesystem-options.service: Passing 0 fds to service exec-temporaryfilesystem-options.service: About to execute: /bin/sh -c 'test $$(awk '$$5 == "/var" { print $$11 }' /proc/self/mountinfo) = "ro,mode=700"' exec-temporaryfilesystem-options.service: Forked /bin/sh as 10527 Followed symlinks /run/systemd/unit-root/var → /run/systemd/unit-root/var. Applying namespace mount on /run/systemd/unit-root/var exec-temporaryfilesystem-options.service: Executing: /bin/sh -c 'test $(awk '$5 == "/var" { print $11 }' /proc/self/mountinfo) = "ro,mode=700"' Received SIGCHLD from PID 10527 (sh). Child 10527 (sh) died (code=exited, status=1/FAILURE) exec-temporaryfilesystem-options.service: Child 10527 belongs to exec-temporaryfilesystem-options.service. exec-temporaryfilesystem-options.service: Main process exited, code=exited, status=1/FAILURE exec-temporaryfilesystem-options.service: Failed with result 'exit-code'. exec-temporaryfilesystem-options.service: Changed start -> failed exec-temporaryfilesystem-options.service: Unit entered failed state. PID: 10527 Start Timestamp: Wed 2018-02-21 09:24:08 UTC Exit Timestamp: Wed 2018-02-21 09:24:08 UTC Exit Code: exited Exit Status: 1 Assertion 'service->main_exec_status.status == status_expected' failed at ../src/test/test-execute.c:74, function check(). Aborting. Aborted

This worked for me after changing to

# The mount options default to "mode=0755,nodev,strictatime". # Let's override some of them, and test the behaviour of "ro". TemporaryFileSystem=/var:ro,mode=0700,nostrictatime # Check /proc/self/mountinfo ExecStart=/bin/sh -x -c 'test "$$(awk \'$$5 == "/var" && $$11 !~ /(^|,)ro(,|$)/ { print $$6 }\' /proc/self/mountinfo)" == ""' ExecStart=/bin/sh -x -c 'test "$$(awk \'$$5 == "/var" && $$11 !~ /(^|,)mode=700(,|$)/ { print $$6 }\' /proc/self/mountinfo)" == ""' ExecStart=/bin/sh -x -c 'test "$$(awk \'$$5 == "/var" && $$6 !~ /(^|,)ro(,|$)/ { print $$6 }\' /proc/self/mountinfo)" == ""' ExecStart=/bin/sh -x -c 'test "$$(awk \'$$5 == "/var" && $$6 !~ /(^|,)nodev(,|$)/ { print $$6 }\' /proc/self/mountinfo)" == ""' ExecStart=/bin/sh -x -c 'test "$$(awk \'$$5 == "/var" && $$6 ~ /(^|,)strictatime(,|$)/ { print $$6 }\' /proc/self/mountinfo)" == ""'

@yuwata are you able to resolve this soon or should I submit something?

looks good. Can I include this in a PR that makes ReadWritePaths= can work with TemporaryFileSystem=.

You are welcome to take the above code, I just want to make sure the tests not left broken (on Fedora) for too long. If you think the ReadWritePaths= will be quick to merge then I guess you can include them together.

I just changed my mind. This is now pushed as #8241. Please take a look.

@sourcejedi, @yuwata thank you for looking at this. I've already fixed the test locally, but I hesitated to show it because, well, here it is:

diff --git a/test/test-execute/exec-temporaryfilesystem-options.service b/test/test-execute/exec-temporaryfilesystem-options.service index 1d5d76c81..d63f66fd2 100644 --- a/test/test-execute/exec-temporaryfilesystem-options.service +++ b/test/test-execute/exec-temporaryfilesystem-options.service @@ -5,7 +5,13 @@ Description=Test for TemporaryFileSystem with mount options Type=oneshot # Check /proc/self/mountinfo -ExecStart=/bin/sh -c 'test $$(awk \'$$5 == "/var" { print $$6 }\' /proc/self/mountinfo) = "ro,nodev,relatime"' -ExecStart=/bin/sh -c 'test $$(awk \'$$5 == "/var" { print $$11 }\' /proc/self/mountinfo) = "ro,mode=700"' +ExecStart=/usr/bin/perl -alne 'if ($$F[4] eq "/var") { \ + my %%a = map { $$_ => 1 } split(",", $$F[10]); \ + if (!@a{ro,"mode=700"}) {next}; \ + my %%b = map { $$_ => 1 } split(",", $$F[5]); \ + if (@b{ro,nodev,relatime}) {$$found=1; exit}; \ +} \ +END {exit($$found ? 0 : 1)}' \ +/proc/self/mountinfo TemporaryFileSystem=/var:ro,mode=0700,nostrictatime

Don't ask me how and why it works. I don't know :-)

This makes test-execute work on SELinux enabled systems. Fixes the issue reported at systemd#7908 (comment)

This makes test-execute work on SELinux enabled systems. Fixes the issue reported at #7908 (comment)

…stem= This makes Read{Write,Only}Paths= work with TemporaryFileSystem= or ProtectHome=tmpfs. Follow-up for systemd#7908. Closes systemd#7895, systemd#7153, and systemd#2780.

yuwata added the pid1 label Jan 17, 2018

yuwata mentioned this pull request Jan 17, 2018

Combine InaccessiblePaths with ReadOnlyPaths for a chroot equivalent #7895

Closed

yuwata force-pushed the rfe-7895 branch 2 times, most recently from 4333915 to 46c682a Compare January 17, 2018 03:52

vcaputo reviewed Jan 17, 2018

View reviewed changes

yuwata force-pushed the rfe-7895 branch 2 times, most recently from 3f3ec5c to a34df8d Compare January 17, 2018 06:59

yuwata mentioned this pull request Jan 17, 2018

RFE: ReadWrite{Directories,Paths}= should override ProtectHome=yes for child directories of /home #7153

Closed

yuwata force-pushed the rfe-7895 branch from a34df8d to 70fb956 Compare January 18, 2018 12:44

yuwata changed the title ~~core: introduce EmptyDirectories= option~~ core: add EmptyDirectories= setting and 'empty' option to ProtectHome= Jan 18, 2018

yuwata force-pushed the rfe-7895 branch from 70fb956 to 6cefe5c Compare January 18, 2018 12:54

poettering requested changes Jan 22, 2018

View reviewed changes

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jan 22, 2018

yuwata added 15 commits February 21, 2018 09:05

core/execute: simplify compile_bind_mounts()

a635a7a

It is not necessary to re-assign error code.

core/execute: clear bind_mounts

8e06d57

core: fix DBus API for AppArmorProfile= and SmackProcessLabel=

280921f

core: simplify DBus API for BindPaths=

4ff4c98

mount-util: add mount_option_mangle()

9e7f941

This is used in the later commits.

test: add tests for mount_option_mangle()

f27b437

mount-util: call mount_option_mangle() in mount_verbose()

6ef8df2

nspawn: remove unnecessary mount option parsing logic

72d967d

core/namespace: make '-' prefix in Bind{,ReadOnly}Paths= work

4ca763a

Each path in `Bind{ReadOnly}Paths=` accept '-' prefix. However, the prefix is completely ignored. This makes it work as expected.

core: add new setting TemporaryFileSystem=

2abd4e3

This introduces a new setting TemporaryFileSystem=. This is useful to hide files not relevant to the processes invoked by unit, while necessary files or directories can be still accessed by combining with Bind{,ReadOnly}Paths=.

man: add documents for TemporaryFileSystem=

c10b460

test: add tests for TemporaryFileSystem=

4cac89b

core: add new option 'tmpfs' to ProtectHome=

e4da7d8

This make ProtectHome= setting can take 'tmpfs'. This is mostly equivalent to `TemporaryFileSystem=/home /run/user /root`.

core: add DBus API for TemporaryFileSystem=

784ad25

doc: update TRANSIENT-SETTINGS.md

24743ef

yuwata force-pushed the rfe-7895 branch from bd2e163 to 24743ef Compare February 21, 2018 00:20

sourcejedi approved these changes Feb 21, 2018

View reviewed changes

sourcejedi merged commit 59e00b2 into systemd:master Feb 21, 2018

yuwata deleted the rfe-7895 branch February 21, 2018 08:58

evverx reviewed Feb 21, 2018

View reviewed changes

yuwata pushed a commit to yuwata/systemd that referenced this pull request Feb 21, 2018

test: fix test for TemporaryFileSystem=

70b3a63

This makes test-execute work on SELinux enabled systems. Fixes the issue reported at systemd#7908 (comment)

This was referenced Feb 21, 2018

test: fix test for TemporaryFileSystem= #8241

Merged

core: make Read{Write,Only}Paths= work with TemporaryFileSystem= #8242

Closed

poettering pushed a commit that referenced this pull request Feb 21, 2018

test: fix test for TemporaryFileSystem= (#8241)

9323298

This makes test-execute work on SELinux enabled systems. Fixes the issue reported at #7908 (comment)

yuwata mentioned this pull request Dec 28, 2018

ProtectSystem=strict disables ProtectHome=tmpfs #11276

Closed

		@@ -2295,6 +2294,12 @@ static int compile_bind_mounts(

		assert(h == n);

		if (!strv_isempty(empty_directories) && !strv_isempty(context->empty_directories)) {

		@@ -0,0 +1,22 @@
		[Unit]
		Description=Test for BindPaths= and BindReadOnlyPaths=

core: add TemporaryFileSystem= setting and 'tmpfs' option to ProtectHome= #7908

core: add TemporaryFileSystem= setting and 'tmpfs' option to ProtectHome= #7908

Conversation

yuwata commented Jan 17, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcaputo commented Jan 17, 2018

yuwata commented Jan 17, 2018

poettering commented Jan 17, 2018

ludovic-gasc commented Jan 17, 2018

topimiettinen commented Jan 17, 2018

topimiettinen commented Jan 17, 2018

sourcejedi commented Jan 17, 2018 • edited

yuwata commented Jan 18, 2018

topimiettinen commented Jan 20, 2018

yuwata commented Jan 22, 2018

poettering commented Jan 22, 2018

yuwata commented Jan 22, 2018

poettering commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludovic-gasc commented Feb 7, 2018

yuwata commented Feb 8, 2018

yuwata commented Feb 21, 2018

sourcejedi commented Feb 21, 2018

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

yuwata commented Jan 17, 2018 •

edited

sourcejedi commented Jan 17, 2018 •

edited