Improve hardening in tor.service

changed milestone to %Tor: 0.2.6.x-final

added component::core tor/tor milestone::Tor: 0.2.6.x-final owner::candrews priority::medium reporter::candrews resolution::implemented status::closed systemd type::defect labels

Trac:
Cc: N/A to blueness@gentoo.org

intrigeri, can you have a look at this?

Trac:
Milestone: N/A to Tor: 0.2.6.x-final
Cc: blueness@gentoo.org to blueness@gentoo.org, intrigeri

Gentoo also suggests these 2 changes: 1)

"Restart = on-failure"

be removed as it's unnecessary and confusing.

TimeoutSec = 30

be removed because systemd has a default timeout, and user can change it in system.conf. Service setting overrides the user-set global timeout, and it should do that only when there's a good reason to -- for example, when a specific service takes real long to shutdown or start.

Trac:
Username: candrews

For the sake of clarity, here is the complete tor.service suggested by and currently used by Gentoo:

[Unit]
Description=The Onion Router

[Service]
ExecStartPre=/usr/bin/tor --verify-config -f /etc/tor/torrc
ExecStart=/usr/bin/tor --RunAsDaemon 0 -f /etc/tor/torrc
ExecReload=/bin/kill -HUP $MAINPID
KillSignal=SIGINT
TimeoutStopSec=32
LimitNOFILE=30000

# Hardening options:
CapabilityBoundingSet = CAP_SETUID CAP_SETGID CAP_NET_BIND_SERVICE
PrivateTmp = yes
PrivateDevices = yes
ProtectHome = yes
ProtectSystem = full
NoNewPrivileges = yes

[Install]
WantedBy=multi-user.target

The timeout of 32 seconds was chosen to ensure that the daemon receives a SIGNINT and after that has 30 seconds to gracefully close connections. After that a SIGTERM is sent with another 32 seconds. Finally a SIGKILL. This is more than the usual systemd default - but ensures that extremely short user overrides in /etc/systemd/system.conf are overruled. Gentoo used LimitNOFILE=30000 because that's what the sysvinit script they have uses - but they don't care if it's 32768 or 30000 so whatever Tor wants is good.

Hopefully we can get these changes upstream and Tor and all its distros win - thanks again!

Trac:
Username: candrews

Trac:
Owner: N/A to intrigeri
Status: new to assigned

Replying to candrews:

Gentoo also suggests these 2 changes:

Let's focus on the filesystem and capabilities hardening here. (I may even split this ticket into two, one about filesystem hardening, another one about the capabilities thing.)

So, please file one ticket per set of suggested independent changes. Same TimeoutStopSec and LimitNOFILE. Thanks!

Replying to candrews:

Thanks for taking this upstream!

Using PrivateDevices instead of DeviceAllow's is more secure as it create a totally separate /dev as well as removing the CAP_MKNOD capability.

ACK. Now that Debian testing/sid have a systemd that supports this directive (introduced in v209), let's do that. Do you want to prepare a branch that implements this specific change?

ProtectHome makes /home inaccessible, equivalent to "InaccessibleDirectories = /home" but (arguably) more comprehensible.

ACK. Now that Debian testing/sid have a systemd that supports this directive (introduced in v214), let's do that. This will break on distros that ship an older systemd, but well, they can still ship a drop-in to revert this change if they wish. Still, this can be undone if the process has CAP_SYS_ADMIN, so this is blocked by the CapabilityBoundingSet change (see below).

ProtectSystem=full make /usr and /etc read only.

I'm unsure about this one.

This looks slightly weaker than what we already have:

we currently make all the filesystem read-only, except the few directories we need write access to. The only justification I've found on https://bugs.gentoo.org/show_bug.cgi?id=529212#c16 for this change is that ReadWriteDirectories "will fail with a cryptic error message in case one of one those directories not being available", which sounds like something that should be improved in systemd itself, instead of discouraging us from using this directive, no?
AFAICT after reading systemd.exec(5), contrary to ReadOnlyDirectories and ReadWriteDirectories, ProtectSystem can be undone if the process has CAP_SYS_ADMIN, so this is blocked by the CapabilityBoundingSet change (see below).

OTOH, our current combination of ReadOnlyDirectories and ReadWriteDirectories are not compatible with the AppArmorProfile directive we'll add in Debian, so there, I think we'll do want to replace them with ProtectSystem=full... and then it may make sense to do the same upstream, as suggested here, once we get the CapabilityBoundingSet right.

CapabilityBoundingSet reduces the process capability to just what it needs.

This breaks things for me. I get:

[warn] Could not open "/etc/tor/torrc": Permission denied
[warn] Could not unlink /var/run/tor/control: Permission denied

That's with tor 0.2.5.10-1 on Debian sid. Any idea what goes wrong? Perhaps a missing capability in the list?

Also, has this been tested with pluggable transports, such as obfsproxy?

Trac:
Owner: intrigeri to candrews

I've put my changes on github, you can see them at: https://github.com/candrews/tor/blob/issue13805/contrib/dist/tor.service.in

I'll stick to the hardening options and deal with the timeout stuff another time :-)

Given what you've said, I think having both ProtectSystem=full and the ReadWriteDirectories restrictions makes the most sense as that would provide the security benefits of both. To fix the "cryptic error" problem, I think we can safely not error out if a directory doesn't exist, so I've prefixed the directories in ReadWriteDirectories with a "-" as described at http://www.freedesktop.org/software/systemd/man/systemd.exec.html

However, I'm unclear as to why the current combination of ReadOnlyDirectories and ReadWriteDirectories are not compatible with the AppArmorProfile. Perhaps that's something that we can fix? For my curiosity, is there an bug report about that problem in AppArmor, Debian, systemd, or elsewhere that I could check out?

Regarding CapabilityBoundingSet, I'm using it on my system now without a problem. I looked over http://linux.die.net/man/7/capabilities and none of the capabilities except for CAP_SETUID CAP_SETGID CAP_NET_BIND_SERVICE seem like they should be used by Tor. I'm using systemd 217 and Sid has 215, and I'm using Tor 0.2.6.1-alpha, so these may be significant differences as well.

I just tried it with obfsproxy and it seems to work fine.

Finally, when I build Tor (and this is noted in the Gentoo ticket - it's not just me), the ReadWriteDirectories entries in tor.service end up looking like this: ReadWriteDirectories = /var/lib/lib/tor I'm not an autotools/make expert, so I don't know why that is... it looks like @LOCALSTATEDIR@ is expanding to "/var/lib" but should expand to just "/var". Is that something you can help with?

Trac:
Username: candrews

Trac:
Status: assigned to needs_review

Hi,

I generally ACK these changes, although:

I would drop the line: ReadWriteDirectories = -@LOCALSTATEDIR@/run/tor This (/var)/run/tor directory doesn't seem to be used anywhere in Tor source. It's only used by some init scripts to drop PIDFile there. As we discussing configuration which will only be used by systemd, this directory is not needed at all. If there's really a need to have it, I suggest putting RuntimeDirectory=tor in unit file, but I think it would be unnecesary.
Directives introduced in v217, like ProtectHome=, can be used on earlier versions. Systemd will report "unknown directive" but it won't stop the unit from working. I expect when Tor with above changes hit the distributions, they will be already running recent systemd or backported the ProtectHome= options.

I run Tor with the changes as in comment:8, with:

removed the line as in 1)
added CapabilityBoundingSet = CAP_SETUID CAP_SETGID CAP_NET_BIND_SERVICE

And everything seem to work fine. Please apply.

Trac:
Username: tomek@pipebreaker.pl

okay; thanks for the review! (And thanks for the patches!)

Trac:
Status: needs_review to closed
Resolution: N/A to implemented

In the commit, you have moved the ReadWriteDirectories = -@LOCALSTATEDIR@/run/tor line. I suggest removing it.

Trac:
Username: tomek@pipebreaker.pl

Whoops; 2dac77c should be better. Thanks !

closed

mentioned in issue #14141 (moved)

mentioned in issue #15431 (moved)

moved to tpo/core/tor#13805 (closed)

mentioned in issue tpo/core/tor#15431 (closed)

Improve hardening in tor.service

Child items ...

Activity