Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#13805 closed defect (implemented)

Improve hardening in tor.service

Reported by: candrews Owned by: candrews
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: systemd
Cc: blueness@…, intrigeri Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I suggest that tor.service's hardening implementation be changed. These lines would be replaced:

[Service]
DeviceAllow = /dev/null rw
DeviceAllow = /dev/urandom r
InaccessibleDirectories = /home
ReadOnlyDirectories = /
ReadWriteDirectories = /var/lib/tor
ReadWriteDirectories = /var/log/tor
ReadWriteDirectories = /var/run/tor
ReadWriteDirectories = /proc

With these lines:

PrivateDevices = yes
ProtectHome = yes
ProtectSystem = full
CapabilityBoundingSet=CAP_NET_BIND_SERVICE CAP_SETUID CAP_SETGID

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

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

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

CapabilityBoundingSet reduces the process capability to just what it needs.

See http://www.freedesktop.org/software/systemd/man/systemd.exec.html

This discussion was started at https://bugs.gentoo.org/show_bug.cgi?id=529212 and the suggestion to use the higher level constructs was made by the Gentoo systemd team.

For historical reference, tor.service was added in #8368

Child Tickets

Change History (13)

comment:1 Changed 5 years ago by blueness

Cc: blueness@… added

comment:2 Changed 5 years ago by nickm

Cc: intrigeri added
Milestone: Tor: 0.2.6.x-final

intrigeri, can you have a look at this?

comment:3 Changed 5 years ago by candrews

Gentoo also suggests these 2 changes:
1)

"Restart = on-failure"

be removed as it's unnecessary and confusing.

2)

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.

comment:4 Changed 5 years ago by 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!

comment:5 Changed 5 years ago by intrigeri

Owner: set to intrigeri
Status: newassigned

comment:6 in reply to:  3 Changed 5 years ago by intrigeri

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!

comment:7 in reply to:  description Changed 5 years ago by intrigeri

Owner: changed from intrigeri to candrews

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?

comment:8 Changed 5 years ago by 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?

comment:9 Changed 5 years ago by nickm

Status: assignedneeds_review

comment:10 Changed 5 years ago by tomek@…

Hi,

I generally ACK these changes, although:

1) 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.

2) 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.

comment:11 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

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

comment:12 Changed 5 years ago by tomek@…

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

comment:13 Changed 5 years ago by nickm

Whoops; 2dac77c should be better. Thanks !

Note: See TracTickets for help on using tickets.