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

[RFC] Improve systemd service unit #477

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

paulmenzel
Copy link
Contributor

No description provided.

If the build is configured with a non-default pathname to the root key
file, for example, `--with-rootkey-file=/var/unbound/root.key`, that
path needs to be read and writable by Unbound, so add it to the list of
directories Unbound can read from and write to.

    ReadWritePaths=/etc/unbound /etc/unbound

    unbound[6269]: [1619522080] unbound[6269:0] fatal error: could not open autotrust file for writing, /var/unbound/root.key.6269-0-593e60: Read-only file system

Resolves: NLnetLabs#475
That’s semantically more correct than putting it in `ReadWritePaths`.

It normally maps to `/run` and is cleared when the service is stopped.
This is configured when building Unbound, so use it.
@paulmenzel
Copy link
Contributor Author

This tries to address issues #475 and #476, but it’s incomplete. See the issues for discussion.

@paulmenzel
Copy link
Contributor Author

@Maryse47, @hardfalcon, it’d be great if you shared your expertise.

@@ -63,8 +63,8 @@ ProtectHome=true
ProtectControlGroups=true
ProtectKernelModules=true
ProtectSystem=strict
RuntimeDirectory=unbound
ConfigurationDirectory=unbound
RuntimeDirectory=@UNBOUND_RUN_DIR@
Copy link
Contributor

@Maryse47 Maryse47 Apr 27, 2021

Choose a reason for hiding this comment

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

In unbound UNBOUND_RUN_DIR points to where config is stored (also default working/chroot directory) which is usually /etc/unbound. systemd RuntimeDirectory points to path under /run so those aren't related beside similar name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm mistaken, all of these directories must be relative.

Excerpt from https://www.freedesktop.org/software/systemd/man/systemd.exec.html#RuntimeDirectory=:

RuntimeDirectory=, StateDirectory=, CacheDirectory=, LogsDirectory=, ConfigurationDirectory=

These options take a whitespace-separated list of directory names. The specified directory names must be relative, and may not include "..".

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, which means it's not even possible to set it this way.

RuntimeDirectory=unbound
ConfigurationDirectory=unbound
RuntimeDirectory=@UNBOUND_RUN_DIR@
ConfigurationDirectory=@UNBOUND_SYSCONF_DIR@
Copy link
Contributor

Choose a reason for hiding this comment

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

UNBOUND_SYSCONF_DIR is the upper system sysconfdir, usually /etc while ConfigurationDirectory expects specific lower dir name beneath.

@@ -73,7 +73,7 @@ SystemCallFilter=~@clock @cpu-emulation @debug @keyring @module mount @obsolete
RestrictNamespaces=yes
LockPersonality=yes
RestrictSUIDSGID=yes
ReadWritePaths=@UNBOUND_RUN_DIR@ @UNBOUND_CHROOT_DIR@
ReadWritePaths=@UNBOUND_CHROOT_DIR@ @UNBOUND_ROOTKEY_FILE@
Copy link
Contributor

@Maryse47 Maryse47 Apr 27, 2021

Choose a reason for hiding this comment

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

UNBOUND_RUN_DIR should stay for reasons stated above.

As you pointed out in #475 UNBOUND_ROOTKEY_FILE won't add write access to whole dir where key is stored when it's changed from default UNBOUND_RUN_DIR/root.key. When it's not changed then it isn't needed as UNBOUND_RUN_DIR is already covered so adding UNBOUND_ROOTKEY_FILE here doesn't fix anything really.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not an expert in this field, but it appears to me that chrooting into a directory that is writable to the user you are trying to constrain is a potential security issue. Here's a StackExchange post that explains the issue better than I could:
https://unix.stackexchange.com/a/332571

This tl;dr is: If an attacker manages to exploit a vulnerability in the unbound process and run malicious code within the context of that process, (s)he could potentially gain persistence across daemon restarts by creating a specially crafted /etc and/or /lib and/or /usr within the chroot directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, chroot is considered as weak security boundary but using it is default unbound behavior. The service doesn't make anything worse here.

Choose a reason for hiding this comment

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

We definitely need to use TemporaryFileSystem=/:ro without ProtectSystem - I think the system would be again mounted over a TemporaryFileSystem as a read-only. I'll test this configuration and will try to come up with the solution.

@ArchangeGabriel
Copy link
Contributor

I don’t see either #475 nor #476 being Unbound issues, nor this PR to provide improvements (actually it’s the opposite as stated in review comments).

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

Successfully merging this pull request may close these issues.

None yet

5 participants