Opened 3 years ago

Closed 2 years ago

Last modified 21 months ago

#17562 closed defect (fixed)

DataDirectory permissions are too restrictive when using CapabilityBoundingSet or SELinux

Reported by: jamielinux Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: tor-core
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Directories created by Tor have 0700 and TorUser:TorUser permissions. Tor also checks the permissions again at runtime, reducing the permissions if they aren't 0700 and refusing to run if the directory UID and GID aren't both TorUser.

These precautions protect the security of the Tor files. However, the DataDirectory (ie, /var/lib/tor) is unreadable by the root user. When Tor is started as root, it accesses the DataDirectory before dropping root permissions. Normally this wouldn't cause any problems, but there are two situations in which Tor is prevented from running:

  1. If the systemd CapabilityBoundingSet option is set but CAP_READ_SEARCH isn't listed, root is denied access to the DataDirectory.
  1. If SELinux is enabled but tor_t domain isn't allowed dac_read_search permissions, root is denied access to the DataDirectory.

CAP_READ_SEARCH and dac_read_search should be avoided; a process with these permissions can read arbitrary files regardless of DAC permissions. The solution proposed in this patch is to default to creating the DataDirectory with 0750 permissions, while also allowing the group to be either TorUser or root (but nobody else).

Also see: https://bugzilla.redhat.com/show_bug.cgi?id=1279222

I notice that Debian fixed this issue on Stretch/Sid by giving Tor CAP_DAC_OVERRIDE, CAP_CHOWN and CAP_FOWNER. These dangerous capabilities are effectively equal to root, and kind of defeats the point of using CapabilityBoundingSet in the first place. I've chosen different solution.

Child Tickets

Change History (19)

comment:1 Changed 3 years ago by yawning

Component: - Select a componentTor
Keywords: tor-core added
Milestone: Tor: 0.2.8.x-final
Version: Tor: unspecified

Triaging to 0.2.8.x for now. I would be interested in hearing what people like weasel think about such a change.

comment:2 Changed 3 years ago by nickm

Yeah, those capabilities should not be given to Tor. Tor shouldn't really need anything besides CAP_NET_BIND_SERVICE.

I think that it's probably okay to allow gid == 0 as an exception for when group readable is okay? Not 100% sure there, for all I know there's some horrible unix where gid 0 is unprivileged. (Is that possible?)

I'm not too thrilled with changing the default permissions, though: those are locked down for a pretty solid reason. If we'd like to override that, I'd prefer to have it be based on an option than having it be always-750.

comment:3 in reply to:  2 ; Changed 3 years ago by jamielinux

Replying to nickm:

Yeah, those capabilities should not be given to Tor. Tor shouldn't really need anything besides CAP_NET_BIND_SERVICE.

Also CAP_SETUID/CAP_SETGID for dropping permissions.

I think that it's probably okay to allow gid == 0 as an exception for when group readable is okay? Not 100% sure there, for all I know there's some horrible unix where gid 0 is unprivileged. (Is that possible?)

That would be horrific.

I'm not too thrilled with changing the default permissions, though: those are locked down for a pretty solid reason. If we'd like to override that, I'd prefer to have it be based on an option than having it be always-750.

I agree. I propose an alternative patch for it to be optional instead. Distribution packagers or administrators who want to use CapabilityBoundingSet or SELinux (or some other MAC) can create a DataDirectory with the 0750 TorUser:root permissions.

comment:4 in reply to:  3 ; Changed 3 years ago by nickm

Status: newneeds_review

Replying to jamielinux:

Replying to nickm:

Yeah, those capabilities should not be given to Tor. Tor shouldn't really need anything besides CAP_NET_BIND_SERVICE.

Also CAP_SETUID/CAP_SETGID for dropping permissions.

Ah true. BTW, if you know your way around the Privileges system, could I ask you to take a look at the most recent patch on the #8195 ticket if you have a chance? It adds an option for Tor servers to keep CAP_NET_BIND_SERVICE after setuid, and I'd like to make sure we did it right.

I think that it's probably okay to allow gid == 0 as an exception for when group readable is okay? Not 100% sure there, for all I know there's some horrible unix where gid 0 is unprivileged. (Is that possible?)

That would be horrific.

I'm not too thrilled with changing the default permissions, though: those are locked down for a pretty solid reason. If we'd like to override that, I'd prefer to have it be based on an option than having it be always-750.

I agree. I propose an alternative patch for it to be optional instead. Distribution packagers or administrators who want to use CapabilityBoundingSet or SELinux (or some other MAC) can create a DataDirectory with the 0750 TorUser:root permissions.

Thanks, I'll have a look!

comment:5 in reply to:  4 Changed 3 years ago by jamielinux

Replying to nickm:

Ah true. BTW, if you know your way around the Privileges system, could I ask you to take a look at the most recent patch on the #8195 ticket if you have a chance? It adds an option for Tor servers to keep CAP_NET_BIND_SERVICE after setuid, and I'd like to make sure we did it right.

I'd love to, but sadly I don't think I have enough knowledge of either C or the kernel to perform a meaningful review.

comment:6 Changed 3 years ago by nickm

Are you sure you uploaded the right patch the second time around? It looks just like the first one you sent in.

comment:7 in reply to:  6 Changed 3 years ago by jamielinux

Replying to nickm:

Are you sure you uploaded the right patch the second time around? It looks just like the first one you sent in.

It's only a tiny change.

In the second patch (0001-Optionally-allow-root-group-access-to-DataDirectory.patch) I use CPD_GROUP_OK instead of CPD_GROUP_READ. The log message and changes file are also slightly amended.

comment:8 Changed 3 years ago by nickm

ah, I see what you're going for there. But I meant that it might make sense to have a torrc option that controlled this behavior. Do you think that would be okay for the use cases you have in mind?

comment:9 Changed 3 years ago by jamielinux

Oh I misunderstood! How about these patches instead.

Changed 3 years ago by jamielinux

comment:10 Changed 3 years ago by jamielinux

I've also realised that CAP_CHOWN and CAP_FOWNER are required (or the equivalent SELinux capabilities chown and fowner) if creating a ControlSocket.

The parent directory for the socket (eg, /var/run/tor) must only be writable by the default GID. However, Tor tries to create the Unix socket before dropping root permissions and so the root user is trying to chown/chmod files that it doesn't have access to.

What do you think about this third patch to fix this issue? It seems to work, but I'll admit I don't know what the repercussions may be or whether I've broken something else.

comment:11 Changed 2 years ago by jamielinux

@nickm, what do you think of these patches?

comment:12 Changed 2 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

These look good to me; I have a small cleanup to make, so I'll merge, then tweak a little.

If you tested that the third one really does open up the unix socket eventually, it should be fine.

comment:13 in reply to:  12 Changed 2 years ago by jamielinux

Replying to nickm:

These look good to me; I have a small cleanup to make, so I'll merge, then tweak a little.

Great, thanks!

If you tested that the third one really does open up the unix socket eventually, it should be fine.

Yes, the unix socket is created with a small delay (around 1 second).

comment:14 Changed 21 months ago by redfish

FYI: re DataDirectoryGroupReadable: #19953

Note: See TracTickets for help on using tickets.