Opened 6 years ago

Closed 9 months ago

#3629 closed enhancement (wontfix)

Arm/Tor Deb Torrc Configuration

Reported by: atagar Owned by: ioerror
Priority: Medium Milestone:
Component: Archived/Torouter Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: #20747 Points:
Reviewer: Sponsor:

Description

Hi Jake. Thanks for this! The only part I'll comment on much is python and arm since the change itself mostly concerns the arm deb -> tor deb interaction (which treads on areas I'm not too familiar with).

See attached for a rewrite of the python script you sent me. Writing manual copy methods were unnecessary due to shutil, the group check is simplified a bit, and some minor syntax issues would have prevented it from running. This checks out with pylint but *I haven't exercised it* (not on a good test system).

My understanding of your change is as follows. I'm sure I'm misunderstanding a few parts so corrections appreciated!

Step 1: The resources you're providing will only be included or used in the arm deb. As such they'll be checked into the packaging branch under...

/resources/replaceTorrc/Makefile
/resources/replaceTorrc/tor-arm-replace-torrc.c
/resources/replaceTorrc/tor-arm-replace-torrc.h
/resources/replaceTorrc/replaceTorrc.py

Step 2: In deb-prep.sh [1] we'll copy it into release_deb/src/resources via something like the following on line 33...

(cd resources && git archive --format=tar packaging replaceTorrc) | (cd ./release_deb/src/resources && tar xf -)

Step 3: Also in deb-prep.sh we change our default data directory from "~/.arm" to "/var/lib/tor-arm".

Step 4: I build and send debs to Peter as normal, the only difference being that the arm deb has these "src/resources/replaceTorrc/*" contents. The tor-arm-replace-torrc is still uncompiled at this point.

Step 5: Part of installing the deb is that a "tor-arm" group is created, "tor-arm-replace-torrc" is compiled and placed in "<DESTDIR>/bin/tor-arm-replace-torrc", and '/var/lib/tor-arm' is made under "root:tor-arm".

Detail that I'm not clear on: if the user just runs 'arm' then it's under their user rather than tor-arm and hence won't be able to access the arm data directory, causing arm lots of problems (it won't die, but worse performance and many things will not work). Clarification here would be nice.

Step 6: I add an "isDebHack" check which governs if we're gonna be using this or not. The conditional is:

  1. "tor-arm-replace-torrc" is in the PATH
  2. we're either not connected to tor *or* torrc path for the attached instance is "/etc/tor/torrc"

Step 7: If "isDebHack" is true then when the wizard is finished [2] it calls "tor-arm-replace-torrc". If that's successful then HUP tor, otherwise show the user an error. This just means a little change around line 376.

Step 8: My understanding is that the tor process is unable to write to its torrc, so SAVECONF calls fail on debian. Is that right? If so, then arm's saveConf function [3] will need to be modified so the configuration panel can write custom configs.

If this is right then I can do the changes to make arm do the above with the exception of step 5. That deb change *and the testing* I'll be leaving up to you. My understanding is that this isn't impacting my deb prep process and that you're taking ownership of this feature. Please let me know if that isn't the case!

I'd like an ok from Peter, Nick, and confirmation from Jake that he's owning this before I implement the above. Besides that, I'm thrilled arm will be getting better tor deb integration! -Damian

[1] https://gitweb.torproject.org/arm.git/blob/packaging:/deb-prep.sh
[2] https://gitweb.torproject.org/arm.git/blob/HEAD:/src/cli/wizard.py#l324
[3] https://gitweb.torproject.org/arm.git/blob/HEAD:/src/util/torConfig.py#l395

Child Tickets

Attachments (2)

tor-arm-replace-safer.tar.gz (5.9 KB) - added by ioerror 6 years ago.
version for review by atagar
tor-arm-replace.c (6.1 KB) - added by acar_can 6 years ago.
C version of the setuid-wrapper + python script.

Download all attachments as: .zip

Change History (25)

comment:1 in reply to:  description Changed 6 years ago by ioerror

Replying to atagar:

Hi Jake. Thanks for this! The only part I'll comment on much is python and arm since the change itself mostly concerns the arm deb -> tor deb interaction (which treads on areas I'm not too familiar with).

I was hoping that you would adopt this helper as a thing arm installs on more than just Debian. I have used arm on systems that are not Debian and it would have been quite helpful there too.

See attached for a rewrite of the python script you sent me. Writing manual copy methods were unnecessary due to shutil, the group check is simplified a bit, and some minor syntax issues would have prevented it from running. This checks out with pylint but *I haven't exercised it* (not on a good test system).

It looks better, I agree. I will add some stuff to it today. After sleeping on it, I realized that I left a gaping security hole in the file copy functions, so I'm going to fix those as much as is possible.

My understanding of your change is as follows. I'm sure I'm misunderstanding a few parts so corrections appreciated!

Step 1: The resources you're providing will only be included or used in the arm deb. As such they'll be checked into the packaging branch under...

/resources/replaceTorrc/Makefile
/resources/replaceTorrc/tor-arm-replace-torrc.c
/resources/replaceTorrc/tor-arm-replace-torrc.h
/resources/replaceTorrc/replaceTorrc.py

I was hoping that this would be in your git tree and that it would be part of arm. I am submitting it as a general improvement that when arm detects it's useful, it will use it. This might be at compile/build/packaging time or some other time.

Step 2: In deb-prep.sh [1] we'll copy it into release_deb/src/resources via something like the following on line 33...

(cd resources && git archive --format=tar packaging replaceTorrc) | (cd ./release_deb/src/resources && tar xf -)

I'm not sure I understand what you mean here.

Step 3: Also in deb-prep.sh we change our default data directory from "~/.arm" to "/var/lib/tor-arm".

Ah, I'm not suggesting this - I'm only suggesting that if the user is part of the "tor-arm" group, then the user uses the system wide arm data directory. This is similar to how Tor does things; you can run tor as a user or run tor as a daemon system wide. I suspect arm will not be a daemon anytime soon but the basic idea is the same: we need a system wide arm config where some users can share the auth/other details.

Step 4: I build and send debs to Peter as normal, the only difference being that the arm deb has these "src/resources/replaceTorrc/*" contents. The tor-arm-replace-torrc is still uncompiled at this point.

If you build a deb, it should have the C wrapper compiled. Peter should have to do zero work other than reviewing it, I think. Perhaps not even that if we can review it well enough before hand!

Step 5: Part of installing the deb is that a "tor-arm" group is created, "tor-arm-replace-torrc" is compiled and placed in "<DESTDIR>/bin/tor-arm-replace-torrc", and '/var/lib/tor-arm' is made under "root:tor-arm".

Sure or any time you want to make a system wide user that can use arm in this manner, you'd add the user. '/var/lib/tor-arm' needs to be mode 0770 or something similar, I think.

Detail that I'm not clear on: if the user just runs 'arm' then it's under their user rather than tor-arm and hence won't be able to access the arm data directory, causing arm lots of problems (it won't die, but worse performance and many things will not work). Clarification here would be nice.

arm needs to catch this and know that they aren't going to use a systemwide config, they're not allowed to do so (or they'd be in the "tor-arm" group). This is the case where it should fall back to ~/.arm/ or something similar.

Step 6: I add an "isDebHack" check which governs if we're gonna be using this or not. The conditional is:

  1. "tor-arm-replace-torrc" is in the PATH
  2. we're either not connected to tor *or* torrc path for the attached instance is "/etc/tor/torrc"

I'm not sure I'd search the path. I'd only execute it if:
it's a setuid binary owned by root
It has the proper group ("tor-arm" or whatever it's configured to be)
The current users in in the "tor-arm" group
We have already written a valid configuration file out to /var/lib/arm/torrc

And then the checks you suggest...

Step 7: If "isDebHack" is true then when the wizard is finished [2] it calls "tor-arm-replace-torrc". If that's successful then HUP tor, otherwise show the user an error. This just means a little change around line 376.

That sounds right. This is the change I had hoped you would make.

Step 8: My understanding is that the tor process is unable to write to its torrc, so SAVECONF calls fail on debian. Is that right? If so, then arm's saveConf function [3] will need to be modified so the configuration panel can write custom configs.

Yes, that's correct. If SAVECONF worked on Debian, we would not need this at all after our first configuration for authenticate purposes, I think.

If this is right then I can do the changes to make arm do the above with the exception of step 5. That deb change *and the testing* I'll be leaving up to you. My understanding is that this isn't impacting my deb prep process and that you're taking ownership of this feature. Please let me know if that isn't the case!

I'm happy to test it but the hook in point seven is probably best coded by you. I will provide you with a stable API of fancy unix return codes to indicate success or failure.

I'll change the tor-arm-replace-torrc.py a bit and upload a newer version shortly.

comment:2 Changed 6 years ago by atagar

I was hoping that you would adopt this helper as a thing arm installs on more than just Debian.

Woah, that changes this. Just to make sure I'm understanding this right, you want arm's install script to automatically execute gcc on the user's system? If so then this is the first I've ever heard of an installer doing compilation on an end user system...

If this was just for Debian then I'd be happy to go with pretty much any hack you and Peter wanted, but if this is for arm in general then I'm much more concerned. It's a pity the setuid bit is specifically disabled on most systems for shell scripts. I'd find that much less objectionable.

My feeling is this:

  • If Tor decides to have root permissions on its torrc then Tor is clearly indicating that they want it to only be hand edited. Personally I think this is dumb since it breaks SAVECONF and hurts users, but oh well. Controllers (both arm and Vidalia) should treat this torrc as being kinda useless and work around it.
  • When I scripted arm's setup wizard I took the same tactic as Vidalia, making a torrc in my own data directory for an arm managed Tor instance. If fixing the permissions on the system torrc and editing the init.d script are both forbidden then the only real option left to controllers is to ignore those and make their own.
  • This setuid hack is in effect the same as a chown on the system torrc. That in itself I'm fine with (assuming Nick was on board). However, this is introducing an extremely ugly song-and-dance for working around Tor's permission issue and, in my opinion, end user compilation pushes the hack way too far. If we think that this hack is ok then we should just skip the overly complicated workaround and issue what's in effect a chown directly.

All this said, if others in the community really think that this is the best route forward then I'll bite the bullet in the name of interoperability. Sebastian, Nick: what are your opinions?

-Damian

PS. No offense at all intended toward your work, Jake. I'm just finding that the more I think about this change the more I'm uncomfortable with it.

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

Replying to atagar:

I was hoping that you would adopt this helper as a thing arm installs on more than just Debian.

Woah, that changes this. Just to make sure I'm understanding this right, you want arm's install script to automatically execute gcc on the user's system? If so then this is the first I've ever heard of an installer doing compilation on an end user system...

install-time compilation doesn't make sense. But I don't think this is what was proposed here? I took it as build-time compilation, which is standard

If this was just for Debian then I'd be happy to go with pretty much any hack you and Peter wanted, but if this is for arm in general then I'm much more concerned. It's a pity the setuid bit is specifically disabled on most systems for shell scripts. I'd find that much less objectionable.

suid root scripts are generally a sad thing from experience

My feeling is this:

  • If Tor decides to have root permissions on its torrc then Tor is clearly indicating that they want it to only be hand edited. Personally I think this is dumb since it breaks SAVECONF and hurts users, but oh well. Controllers (both arm and Vidalia) should treat this torrc as being kinda useless and work around it.

Tor doesn't decide this. It is whoever is packaging Tor that makes the decision. Personally I think it is a great idea, I find it horrible that some kind of networking daemon with the power of Tor should have the power to change its own configuration. This is, as far as I know, only implemented as a hack so that controllers on remote systems can work and change the configuration persistently.

  • When I scripted arm's setup wizard I took the same tactic as Vidalia, making a torrc in my own data directory for an arm managed Tor instance. If fixing the permissions on the system torrc and editing the init.d script are both forbidden then the only real option left to controllers is to ignore those and make their own.

I feel like this is a good approach for a TBB-like client instance. I'd never want to run a relay like that, just for port opening reasons and resource consumption etc.

  • This setuid hack is in effect the same as a chown on the system torrc. That in itself I'm fine with (assuming Nick was on board). However, this is introducing an extremely ugly song-and-dance for working around Tor's permission issue and, in my opinion, end user compilation pushes the hack way too far. If we think that this hack is ok then we should just skip the overly complicated workaround and issue what's in effect a chown directly.

I think nick doesn't have anything to do with it - it's a packaging decision. (I don't mean nick won't have a useful opinion here, just that this is not something that Tor decided to do a certain way).

All this said, if others in the community really think that this is the best route forward then I'll bite the bullet in the name of interoperability. Sebastian, Nick: what are your opinions?

I think the only sane way forward is to prompt for root when a root-owned file is supposed to be overwritten

comment:4 in reply to:  2 Changed 6 years ago by ioerror

Replying to atagar:

I was hoping that you would adopt this helper as a thing arm installs on more than just Debian.

Woah, that changes this. Just to make sure I'm understanding this right, you want arm's install script to automatically execute gcc on the user's system? If so then this is the first I've ever heard of an installer doing compilation on an end user system...

No, I think you're confused. I am suggesting build time compilation; users don't run setup.py, they install the deb which has everything all setup - this is totally standard for C programs that need to be compiled. In general, a user installs a C program that is compiled and available as a binary package. I am suggesting that arm should compile the setuid wrapper at build time (deb build time, src unpack/whatever package build time, etc).

If this was just for Debian then I'd be happy to go with pretty much any hack you and Peter wanted, but if this is for arm in general then I'm much more concerned.

The implementation is off by default for every user except those that are already root. You must explicitly be a part of the "tor-arm" group to even execute the C wrapper _at all_ and it will not work on Windows.

I think this is reasonably safe and it at least does a sanity check on the config, backs up the old config and *it does not restart Tor* - the program is a merely a wrapper that allows automatic authorization with a VERY small attack surface. Thus, if a user is not already root or the user is not in "tor-arm" - they are not even able to execute the program - they will get a permission denied error from the OS.

It's a pity the setuid bit is specifically disabled on most systems for shell scripts. I'd find that much less objectionable.

I think this is very confusing. When you execute a shell script, you're actually executing the interpreter specified in the #! line. Thus, it is not the shell scrip that is set uid ,it's the interpreter. You'd have to be nuts to setuid python or bash and so the wrapper is a solution that allows a single script but not all of python or bash to be setuid. I have coded it in a very defensive manner and I have even made sure that by default, no user may even execute the binary. It may not be perfect and I'm open to criticisms of ways this may break.

My feeling is this:

  • If Tor decides to have root permissions on its torrc then Tor is clearly indicating that they want it to only be hand edited. Personally I think this is dumb since it breaks SAVECONF and hurts users, but oh well. Controllers (both arm and Vidalia) should treat this torrc as being kinda useless and work around it.

I understand your viewpoint. I don't even entirely disagree. I do however feel like "hand edited" is exactly what we're enabling here. It is also automatically making a backup and verifying that Tor would consider it a valid configuration file.

I tried to suggest the simple option of something like the /etc/default/tor Vidalia hack but weasel was against it. I understand his reasons and I wrote this wrapper such that a user who wants to use arm may configure Tor and it's *arm* that does the work. This releases weasel or anyone else from having to deal with this crazy shit but also allows us to actually move forward. It means that arm can now take over Tor in a safe way that persists, even if Tor starts as root from an init.d script. This is the default and it solves a major problem that users have when Vidalia takes over Tor; that is - if they don't login to the system, Tor never starts. We will not have that problem - an important problem to solve on a headless machine!

  • When I scripted arm's setup wizard I took the same tactic as Vidalia, making a torrc in my own data directory for an arm managed Tor instance. If fixing the permissions on the system torrc and editing the init.d script are both forbidden then the only real option left to controllers is to ignore those and make their own.

That means you need to make arm a daemon and it will never be able to bind to privileged ports unless arm spawns Tor as root. I think that's a HUGE attack surface and arm was not designed for this task.

  • This setuid hack is in effect the same as a chown on the system torrc. That in itself I'm fine with (assuming Nick was on board). However, this is introducing an extremely ugly song-and-dance for working around Tor's permission issue and, in my opinion, end user compilation pushes the hack way too far. If we think that this hack is ok then we should just skip the overly complicated workaround and issue what's in effect a chown directly.

Nick has nothing to do with this other than me asking for a sanity check on the general idea. There is no end user compilation.

All this said, if others in the community really think that this is the best route forward then I'll bite the bullet in the name of interoperability. Sebastian, Nick: what are your opinions?

-Damian

PS. No offense at all intended toward your work, Jake. I'm just finding that the more I think about this change the more I'm uncomfortable with it.

I think it's annoying that we can't easily make the Tor package support arm as we did with Vidalia. I also however think that weasel is correct in that our dirty hack for Vidalia was the wrong thing then and it would be wrong to add another one for arm. So I'm torn but I have written a solution. I believe it's safe and meets all the other weird requirements that we've stumbled over for a while.

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

Replying to Sebastian:

Replying to atagar:

I was hoping that you would adopt this helper as a thing arm installs on more than just Debian.

Woah, that changes this. Just to make sure I'm understanding this right, you want arm's install script to automatically execute gcc on the user's system? If so then this is the first I've ever heard of an installer doing compilation on an end user system...

install-time compilation doesn't make sense. But I don't think this is what was proposed here? I took it as build-time compilation, which is standard

Correct. I'm suggesting the standard C program compilation/build process that we use for any program. This really doesn't change very much, if the deps for the C program around available, we should just install the Python app as normal. The trick here is simply to add the build-deps/deps to the debian/control file for a basic C program and ensure that there is something to call the Makefile.

If this was just for Debian then I'd be happy to go with pretty much any hack you and Peter wanted, but if this is for arm in general then I'm much more concerned. It's a pity the setuid bit is specifically disabled on most systems for shell scripts. I'd find that much less objectionable.

suid root scripts are generally a sad thing from experience

You cannot mark a script suid, only the interpreter or a dirty hack that does something similar to what I've done.

My feeling is this:

  • If Tor decides to have root permissions on its torrc then Tor is clearly indicating that they want it to only be hand edited. Personally I think this is dumb since it breaks SAVECONF and hurts users, but oh well. Controllers (both arm and Vidalia) should treat this torrc as being kinda useless and work around it.

Tor doesn't decide this. It is whoever is packaging Tor that makes the decision. Personally I think it is a great idea, I find it horrible that some kind of networking daemon with the power of Tor should have the power to change its own configuration. This is, as far as I know, only implemented as a hack so that controllers on remote systems can work and change the configuration persistently.

Right. Weasel did not want us to make a bunch of dirty hacks to the Tor package and I wrote this patch as a way to solve the problem in arm; it's useful for many cases where arm controls a Tor but does not *start* the Tor. This happens all the time and in fact, by default, arm could just dump the current Tor config file, add a control port/auth config stanza, and then overwrite /etc/tor/torrc - so in a sense, we can use this to easily take over Tor but only when the sysadmin is OK with said takeover. In any case, we need something that does this specific task without user intervention in the shell. If my implementation is sound, I'd like to think this specific solution is safer than a generic solution that depends on a lot of external stuff.

  • When I scripted arm's setup wizard I took the same tactic as Vidalia, making a torrc in my own data directory for an arm managed Tor instance. If fixing the permissions on the system torrc and editing the init.d script are both forbidden then the only real option left to controllers is to ignore those and make their own.

I feel like this is a good approach for a TBB-like client instance. I'd never want to run a relay like that, just for port opening reasons and resource consumption etc.

Right. That's exactly why I didn't suggest we write /etc/init.d/arm as the solution. Arm was not designed to run as root and it seems like while it might happen, it's a lot lot lot more work to go down that path.

  • This setuid hack is in effect the same as a chown on the system torrc. That in itself I'm fine with (assuming Nick was on board). However, this is introducing an extremely ugly song-and-dance for working around Tor's permission issue and, in my opinion, end user compilation pushes the hack way too far. If we think that this hack is ok then we should just skip the overly complicated workaround and issue what's in effect a chown directly.

I think nick doesn't have anything to do with it - it's a packaging decision. (I don't mean nick won't have a useful opinion here, just that this is not something that Tor decided to do a certain way).

Correct. Nick said I should ask an old unix beard, I generally agree and will seek out such a sage for my next code review.

All this said, if others in the community really think that this is the best route forward then I'll bite the bullet in the name of interoperability. Sebastian, Nick: what are your opinions?

I think the only sane way forward is to prompt for root when a root-owned file is supposed to be overwritten

This view point helps me to understand why Linux is such a pain in the ass to use. :) We do lots of stuff every moment where this isn't happening - udev, NetworkManger, etc comes to mind.

The user friendly way is that you have a user that is allowed to use arm to perform a task. In this case, we're reconfigure Tor. They might mess up Tor or the system in the process. So we limit what they can do to a few things and we trust that without a lot of mucking around, they won't break it on accident from arm.

However, at some point, they might tamper with the contents of the file, regardless of what arm does with the contents. This problem is basically universal. The user can always break Tor if they are allowed to reconfigure it. However, we try to drop privs and verify that Tor would be happy. If it's happy, we're basically at a dead end - we can't really perform too many more tests - I guess we could ensure it's a small size or something similar. I'm open to making it really limited, I currently limited it to 1024*1024 just to be semi-defensive (thanks radii!).

Still this doesn't mean the config is *correct* - it merely means that it's *valid* as far as arm and Tor are concerned. In theory, if the config will crash, it will first crash as nobody, so it should be at worst, a uid switch from $user:tor-arm to nobody:nogroup. That seems pretty safe.

We do everything we can to help them out and to stop them from really screwing up the system on accident. arm will only write a valid config, we'll verify it in the python script called by the wrapper, we'll only overwrite the system config *after we backup the original config* with the config we verified and the only user that can even start it is one we explicitly added to a group that we created at install time.

If we gave them sudo or something else, they'd have a similar set of problems to solve with a much larger attack surface and with a bunch of moving parts we don't control.

In any case, we'd need to get some kind of interaction which seems nearly impossible on a headless system unless we don't prompt for passwords. I guess we'd have to solve that with a similar user or group trick and then we'd have a giant attack surface, a different program to configure and so on.

But I'm writing too much; I hope this clears everything up. Now perhaps someone can break my one trick pony and then I'll try again. :)

comment:7 Changed 6 years ago by ioerror

This is arm - the software that this program will hopefully extend:
http://www.atagar.com/arm/

comment:8 Changed 6 years ago by ioerror

A little bird suggests that we might as well run the check as a user added just for this task but that it's probably safer to use nobody than to run as root or some other account.

comment:9 Changed 6 years ago by ioerror

Sebastian has convinced me that I need to make sure that $(DESTDIR)/bin needs to be exactly what we expect or we should bail out.

comment:10 Changed 6 years ago by hellais

It seems good to me, except for added paranoia a check to see whether $(DESTDIR)/bin is writable only by root.

comment:11 Changed 6 years ago by ioerror

That permission issue was Sebastian's point. I agree entirely. I'm surprised that install lets you install something unsafely without forcing it...

I'd also add that there two main things to review here.

The first is the Python program - is it safe to call with sudo or as root? If so, great; if not, I'd like to fix that. Does it properly exit if you're not in the right t group, regardless of if you're root or not? I believe so.

The second is the C program that invokes the Python program. I believe it is a very simple setuid wrapper that has a very small attack surface. The OS that grants the setuid bit also ensures that it is only possible to execute the program if your user is in the right group. I believe this is actually safer than sudo but obviously at the cost of being less flexible. sudo does not have checking for group membership like this - it only checks an internal filter; the OS is consulted but not until after execve().

Here's sudo from an Ubuntu system:

-rwsr-xr-x 2 root root 165K 2011-05-30 02:06 /usr/bin/sudo

Anyone can call that on the system and it's up to sudo to ensure that it's safe. As we know from the history of sudo, it's hasn't always done the job properly. It's a tough job, obviously.

The python program contains similar internal checks but they're hard coded at install time to match the group that can call the program at all. This should prevent the python program from running if you're not supposed to run it and should be rather graceful in general.

Thoughts?

comment:12 Changed 6 years ago by ioerror

Someone pointed me to this paper and suggested that I adjust the (python) code:
http://research.cs.wisc.edu/mist/safefile/

That seems reasonable and I'll give it a read later.

comment:13 Changed 6 years ago by kragen

Commentary on the above tarball:

Overall, it seems like it has some extra complexity that could be removed, and a worrisome tendency to discard information about errors and continue, rather than reporting the error and stopping.

I think there is no point in exiting if you're not in the right group if you're root; it will add just as much inconvenience to legitimate users as to attackers.

I think the indirection between HELPER_NAME and TOR_ARM_REPLACE_TORRC does not provide any benefit, so you should remove it, and the .h file that it exists in.

In the Makefile, I think rather than just ignoring failure from mkdir -p, you should check for the failure that you expect, e.g.:

VARLIBDIR=$(DESTDIR)/var/lib/tor-arm
...
[ -d $(VARLIBDIR) ] || mkdir -p $(VARLIBDIR)

Also, your makefile is full of problems if DESTDIR contains spaces. Not sure if you want to do something about that; it's a tricky problem to solve in the context of make.

In the Python, I think you should get rid of all of the try: except: that don't specify which exceptions to catch, because those will produce misleading error messages if something unexpected goes wrong.

Also, I already suggested rewriting _checkId more or less as follows:

def _checkId(group=GROUP):
    return os.geteuid() == 0 and grp.getgrnam(group).gr_gid in os.getgroups()

If you want to print a friendly error message, you could do that in main where you're printing all the other friendly error messages:

try:
    group_ok = _checkId(GROUP)
except KeyError:
    print "It doesn't appear that " + GROUP + " is a valid group on this system!"
if group_ok:

Your overWriteTorConfig will leave the system with a blank or incomplete Tor config file if the machine loses power at the wrong time, which could produce security problems.

nf.flush() is implied by nf.close(); there's no point in doing it explicitly. Maybe you meant to fsync()? But you didn't.

I hope this commentary is helpful!

comment:14 in reply to:  13 Changed 6 years ago by ioerror

Replying to kragen:

Commentary on the above tarball:

Overall, it seems like it has some extra complexity that could be removed, and a worrisome tendency to discard information about errors and continue, rather than reporting the error and stopping.

Sounds good. I'll change that.

I think there is no point in exiting if you're not in the right group if you're root; it will add just as much inconvenience to legitimate users as to attackers.

Sure, I've added a check for that case.

I think the indirection between HELPER_NAME and TOR_ARM_REPLACE_TORRC does not provide any benefit, so you should remove it, and the .h file that it exists in.

Hrm, I'm not sold on this but I'll consider it.

In the Makefile, I think rather than just ignoring failure from mkdir -p, you should check for the failure that you expect, e.g.:

VARLIBDIR=$(DESTDIR)/var/lib/tor-arm
...
[ -d $(VARLIBDIR) ] || mkdir -p $(VARLIBDIR)

Sounds good. I've done that as well as updated it to ensure that $(VARLIBDIR) is owned by root:$GROUP

Also, your makefile is full of problems if DESTDIR contains spaces. Not sure if you want to do something about that; it's a tricky problem to solve in the context of make.

Suggestions? Does Debian ensure that at package build that DESTDIR is "safe" in this regard?

In the Python, I think you should get rid of all of the try: except: that don't specify which exceptions to catch, because those will produce misleading error messages if something unexpected goes wrong.

OK.

Also, I already suggested rewriting _checkId more or less as follows:

def _checkId(group=GROUP):
    return os.geteuid() == 0 and grp.getgrnam(group).gr_gid in os.getgroups()

If you want to print a friendly error message, you could do that in main where you're printing all the other friendly error messages:

try:
    group_ok = _checkId(GROUP)
except KeyError:
    print "It doesn't appear that " + GROUP + " is a valid group on this system!"
if group_ok:

The new code does this.

Your overWriteTorConfig will leave the system with a blank or incomplete Tor config file if the machine loses power at the wrong time, which could produce security problems.

Hrm, I think you might have reviewed old code - I am copying a file over the old file (after backing up the first one), that should be atomic, no?

nf.flush() is implied by nf.close(); there's no point in doing it explicitly. Maybe you meant to fsync()? But you didn't.

I've removed it.

I hope this commentary is helpful!

Very! Thank you!

comment:15 Changed 6 years ago by ioerror

I've uploaded a new version for interm review.

It sounds like atagar is not sure about the setuid C program because people have generally made it sound scary to have a setuid program. He has however said that he's happy to have the python program and understands that users need to run it as root.

I think if people like sudo, they'd be welcome to contribute a sudo config.

Changed 6 years ago by ioerror

version for review by atagar

comment:16 Changed 6 years ago by ioerror

Owner: changed from ioerror to atagar
Status: newassigned

comment:17 Changed 6 years ago by ioerror

Status: assignedneeds_review

comment:18 Changed 6 years ago by atagar

Status: needs_reviewaccepted

Thanks Jake! I've hacked on your script a bit, fixing an uncaught KeyError and merging the makefile setup/removal into the python script. It's available in the torrc-override branch of my personal repo:
https://gitweb.torproject.org/atagar/arm.git/shortlog/refs/heads/torrc-override

I tested the --init and --remove options but not the replace torrc functionality (see below). Things left to do:

  • We need to drop the usage of 'os.setresuid' and 'os.setresgid'. These were introduced in Python 2.7 (arm aims for 2.5 compatibility and torouter will also have an issue with this...).
  • I need to add the wizard hook to use this. This won't be hard (use '/usr/bin/torrc-override' if it exists and otherwise attempt 'sudo -n resources/torrc-override/override.py').
  • I'll also replace the 'tor --verify-config' check with a python alternative. Again easy, this won't be a general purpose check but rather that we got a valid *arm wizard* torrc, which means that it matches the following template:

https://gitweb.torproject.org/arm.git/blob/HEAD:/src/resources/torrcTemplate.txt

Cheers! -Damian

comment:19 Changed 6 years ago by atagar

Added some simple checks that the torrc matches what the wizard would generate. The 'tor --verify-config' still has value since it's checking the validity of values for options like RelayBandwidthRate and MaxCircuitDirtiness, but not the end of the world if you'd like to toggle the RUN_VERIFY flag.

The torrc contents are in memory after this so I'm tempted to have us write torrcContents rather than doing shutil.copy. This would avoid any timing issues of copying unvalidated data, but I suspect this would also prevent the copy itself from being atomic. :/

comment:20 Changed 6 years ago by atagar

Owner: changed from atagar to ioerror
Status: acceptedassigned

Added the arm hooks so this will be used by the wizard:
https://gitweb.torproject.org/atagar/arm.git/commitdiff/ec49c74ba52d9410d6f108a4e76dfad063d40500

If the user has run the --init argument (making the tor-arm user and /var/lib/tor-arm/torrc) then there's another option in the wizard called "Use System Instance" (defaulted to "Yes"). Having this option makes us...

  • Run override.py directly if arm's running as root (bad user, no biscuit!)
  • Run the setuid binary if it's available at '/usr/bin/torrc-override'
  • Otherwise attempt 'sudo -n' on override.py. We do a version check first since that option was *supposed* to be available starting with 1.7.0 (as per http://www.sudo.ws/pipermail/sudo-users/2009-January/003890.html) but that's a dirty lie. Ubuntu 9.10 uses 1.7.0 and even has the option in its man page, but it doesn't work so checking for 1.7.1 instead.
  • If everything above fails then logs a message asking the user to run the script manually with sudo.

Since both the hook and torrc validation is done sending this back to Jake to check this version, address the 'setresuid' issue mentioned earlier, and see if it does what he wants for the setuid use case. I haven't exercised the final torrc replacement since I'm using Python 2.6.

Cheers! -Damian

comment:21 Changed 6 years ago by acar_can

Just attached a C version I proposed earlier to ioerror last week. Functionality should be the same without the full python interpreter + the kitchen sink running as root. Privileges are dropped completely in a child process while copying the configuration file and running the test. Backup is done by a link. The temporary configuration is then moved over to the actual configuration. This makes sure there is always a valid version of the configuration in place.

Best,

Can

Changed 6 years ago by acar_can

Attachment: tor-arm-replace.c added

C version of the setuid-wrapper + python script.

comment:22 Changed 6 years ago by atagar

Component: armTorouter
Status: assignedneeds_review

Thanks, Can. I'm not good enough with C to review the change so I'll leave that to Jake. I'm happy to include it as an alternative to the python version if he feels it would be helpful.

I've done additional testing, made some fixes, and merged with master:
https://gitweb.torproject.org/arm.git/commitdiff/5b4c2765384045de454f33f390886a56483a6979?hp=ce2ff76cc352309ac4450d6dc9f94e1db7da4983

I'm not spotting a method for overwriting the saved user id prior to python 2.7 so this leaves that unset, using setreuid/gid rather than setresuid/gid, if running 2.6 or earlier. Patches welcome if this is an important issue and someone knows of an alternative.

At this point the arm components of this change are done so assigning this to torouter for review by Jake, integration testing, and to determine if there's anything more he needs from the change.

comment:23 Changed 9 months ago by irl

Parent ID: #20747
Resolution: wontfix
Severity: Normal
Status: needs_reviewclosed

No activity in 5 years and this is no longer an active project. Closing as no longer relevant. See also: #20747.

Note: See TracTickets for help on using tickets.