Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#3421 closed defect (fixed)

control socket owned by root

Reported by: weasel Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.28-beta
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Hi,

when Tor creates a ControlSocket at startup it does so before dropping its privileges which causes the socket to be owned by root:

intrepid:/var/run/tor# ls -l control
srw-rw---- 1 root debian-tor 0 Jun 17 23:08 control=

[this is 0.2.2.28 + 54d7d31c]

I would expect the socket to be owned by the user that Tor is running as.

(Obviously if one adds a second control socket at run time that one gets opened/created as and is owned by the tor user).

This isn't something we need to fix right away, but it does seem wrong.

Maybe one option is to create unix sockets after dropping privileges. But then we cannot create a socket in a root owned directory that we do not have write privileges too. (I don't think the current check_private_dir() check allos for directories like that but it could be argued it should.)

Another option would be to chown the socket. I wonder how portable that is tho.

Cheers,

Child Tickets

Attachments (2)

0001-chown-sockets-when-User-option-is-set.patch (2.1 KB) - added by lunar 9 years ago.
Patch against ecc9a364c
0001-chown-sockets-when-User-option-is-set.2.patch (2.4 KB) - added by lunar 8 years ago.
v2

Download all attachments as: .zip

Change History (8)

Changed 9 years ago by lunar

Patch against ecc9a364c

comment:1 Changed 9 years ago by lunar

Status: newneeds_review

comment:2 Changed 8 years ago by nickm

Quick review:

The approach seems basically reasonable, but:

  • The patch doesn't apply to master right now.
  • Instead of checking for HAVE_SYS_UN_H, why not check for HAVE_PWD_H ?
  • The code that calls getpwnam() needs to be wrapped in a HAVE_PWD_H check too, I think: otherwise it won't build on systems without PWD_H.
  • The tor_assert(pw != NULL) check seems a bit too aggressive. We shouldn't crash just because a user got deleted from /etc/passwd.

comment:3 Changed 8 years ago by lunar

Here is an updated patch addressing the issues nickm outlined.

The patch initially checked for HAVE_SYS_UN_H because the change was only relevant for Unix sockets. Using HAVE_PWD_H is indeed more precise.

comment:4 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good ; merged it!

comment:5 Changed 7 years ago by nickm

Keywords: tor-client added

comment:6 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.