#725 assigned enhancement

We should whine if we're running as root.

Reported by: zandr Owned by: davidsarah
Priority: major Milestone: soon
Component: code-nodeadmin Version: 1.4.1
Keywords: easy security usability unix test-needed Cc: zancas
Launchpad Bug:

Description (last modified by Lcstyle)

Just spent some time unravelling a problem at ScanCafe? where nodes were refusing to start because they couldn't open storage/shares/incoming.

It turns out that directory was owned by root. I'm guessing that someone tried to run the nodes as root, thus hosing things up. Not sure why that was the only thing affected, but it seemed to be.

We should complain if we think we're running as root, possibly with a commandline switch or config entry to force it... or possibly not. :)

Attachments (2)

725.patch (1.1 KB) - added by mcoppola at 2011-05-04T07:17:51Z.
725.2.2.patch (1.1 KB) - added by mcoppola at 2011-05-05T18:57:20Z.
Updated patch, considers Windows users

Download all attachments as: .zip

Change History (32)

comment:1 Changed at 2009-06-01T14:45:45Z by zooko

Good idea. Also, should storage servers be more loud and explicit about the fact that they are having a problem with "storage/shares/incoming", or are they already good on that?

comment:2 Changed at 2009-06-01T19:32:41Z by zandr

No, that error is pretty explicit. The slient failure that annoys me is if we can't bind to one of our many ports (manhole seems to get me most often). That's a different, ticket, though. :)

comment:3 Changed at 2009-06-30T12:40:46Z by zooko

  • Milestone changed from 1.5.0 to eventually

comment:4 Changed at 2009-07-10T02:41:32Z by zooko

  • Keywords easy added

comment:5 Changed at 2009-11-08T05:09:50Z by davidsarah

So what is the criterion for runnning as root? os.getuid() == 0 and does not throw an exception?

comment:6 Changed at 2009-11-08T05:13:52Z by davidsarah

No, should be os.geteuid() (being setuid root is just as bad).

comment:7 Changed at 2009-11-21T06:31:50Z by davidsarah

  • Keywords security usability added

comment:8 Changed at 2009-11-21T06:32:30Z by davidsarah

  • Milestone changed from eventually to 1.6.0

comment:9 Changed at 2009-12-13T05:33:20Z by zooko

  • Milestone changed from 1.6.0 to eventually

comment:10 Changed at 2010-02-02T02:56:58Z by davidsarah

  • Milestone changed from eventually to 1.7.0
  • Owner set to davidsarah
  • Status changed from new to assigned

comment:11 Changed at 2010-06-12T20:56:26Z by davidsarah

  • Milestone changed from 1.7.0 to 1.7.1

comment:12 Changed at 2010-07-17T03:53:00Z by davidsarah

  • Milestone changed from 1.7.1 to soon

Changed at 2011-05-04T07:17:51Z by mcoppola

comment:13 Changed at 2011-05-04T07:18:54Z by mcoppola

I think that alerting the user about running as root should instead be addressed as a security concern. It makes sense that initially running as root would mess with the file permissions, but I believe the issue is generalized to first running as X user and then as Y user, not just root. Anyways, here is the patch I wrote.

comment:14 Changed at 2011-05-04T16:51:38Z by mcoppola

  • Keywords review-needed added

comment:15 Changed at 2011-05-05T18:17:16Z by davidsarah

  • Keywords review-needed removed

os.geteuid isn't available in native-Windows Python:

Python 2.7.1 (r271:86832, Nov 27 2010, 18:30:46) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.geteuid()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute 'geteuid'

Perhaps "if hasattr(os, 'geteuid') and os.geteuid() == 0:".

In principle you also shouldn't run as Administrator on Windows, but that's probably too common (and Windows security is beyond fixing anyway) to be worth warning about.

comment:16 Changed at 2011-05-05T18:50:27Z by mcoppola

That's a very good suggestion - I forgot to consider if the user is running on a Windows system. I also agree with not warning the user on Windows systems. I've found that non-Administrator users still have quite enough access to cause significant harm anyways, so it's probably a moot point to restrict it. I updated the patch.

EDIT: Sorry, something screwy happened with the diff. Just a sec

Version 1, edited at 2011-05-05T18:53:29Z by mcoppola (previous) (next) (diff)

Changed at 2011-05-05T18:57:20Z by mcoppola

Updated patch, considers Windows users

comment:17 Changed at 2011-05-10T05:55:24Z by mcoppola

I'm in the process of writing tests, and zooko and I came across the question of whether or not the '--quiet' flag should suppress the "running as root" error. It should also be known that the error is going to be printed out to STDERR, not STDOUT. What do you guys think?

comment:18 Changed at 2011-05-10T20:38:05Z by zooko

Hi! I think that the --quiet flag should suppress the warning, and also that the warning should go to stderr, not stdout. Want more help writing tests? Let's get together on IRC for that.

comment:19 Changed at 2011-08-18T03:50:04Z by zooko

  • Cc zancas added

comment:20 Changed at 2011-08-18T03:56:20Z by davidsarah

We also need a test that mocks os.geteuid to return 0, and checks that the warning is printed (also, that it is not printed when --quiet is used).

comment:21 Changed at 2011-08-18T20:43:03Z by zooko

  • Owner changed from davidsarah to mcoppola
  • Status changed from assigned to new

Hey mcoppola: still interested? If you don't indicate interest on this ticket soon, I'll feel free to let someone else do it.

comment:22 Changed at 2011-08-19T17:33:21Z by mcoppola

Hey, sorry about the lapse. Thanks for giving me a kick in the behind. I'll try to finish it up.

EDIT: Some guidance with how to navigate and add units to the testing utility in Tahoe would be appreciated, though.

Last edited at 2011-08-19T17:34:37Z by mcoppola (previous) (diff)

comment:23 Changed at 2011-08-19T18:41:58Z by zooko

mcoppola: great!

Let's get together on IRC sometime to walk you through how to write these tests. In the meantime, you can see other tests which do similar things, such as test_error_on_old_config_files.

comment:24 Changed at 2011-09-01T06:34:29Z by mcoppola

Looks like I'm busier than I thought. I'd still love to help out with the project, but for the sake of this bug someone else should take care of it. Sorry 'bout that.

comment:25 Changed at 2011-09-01T20:29:55Z by davidsarah

  • Owner changed from mcoppola to davidsarah
  • Status changed from new to assigned

comment:26 Changed at 2012-07-31T16:39:51Z by davidsarah

  • Keywords unix added
  • Milestone changed from soon to 1.10.0

On tahoe-dev, davidsarah wrote:

On 31/07/12 07:59, Two Spirit wrote:

And people do what they are expected to do? I can't speak for the rest of the world, but yea, I guess there are a lot of "users" like myself who run as root and have no clue what we are doing. My experience with file systems is that you have to run as root for any file system stuff. I'm sure there are a lot of people who share my background.

My idea was a one sentance, standard WARNING disclaimer indicating 1) this should be done as a non-root user or 2) this doesn't need to be done as root somewhere in the running.rst maybe before the first command 'To construct a client node, run "tahoe create-client"....'

"We should whine if we're running as root." https://tahoe-lafs.org/trac/tahoe-lafs/ticket/725

There's a patch, and I see the ticket is assigned to me; it just needs tests. I'll put it in the 1.10 milestone.

What would your idea of said short warning look like?

The one in the current patch says:

###############################################################
WARNING: You should not be running Tahoe-LAFS as root!
This poses an unnecessary security risk and is NOT recommended.
###############################################################

There's an argument for saying that this shouldn't just be a warning; it should be an error, because running as root once may already do things that need to be undone (e.g. creating files owned by root, as in the case that motivated the ticket). If we made it an error then we could add an --allow-root option to suppress it; is that necessary, or overcomplicated?

comment:27 Changed at 2012-09-04T15:47:27Z by zooko

  • Milestone changed from 1.10.0 to soon

comment:28 Changed at 2012-09-04T15:55:14Z by davidsarah

The error message needs to be printed to stderr.

comment:29 Changed at 2012-09-04T15:57:44Z by davidsarah

  • Keywords test-needed added

comment:30 Changed at 2014-09-27T02:22:56Z by Lcstyle

  • Description modified (diff)

i like making it an error, makes sense for unprivileged users.

Last edited at 2014-09-27T02:23:06Z by Lcstyle (previous) (diff)
Note: See TracTickets for help on using tickets.