Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sane user JSON file permissions #165

Conversation

JocelynDelalande
Copy link
Contributor

Creating ClientManager.writeUser().
@JocelynDelalande JocelynDelalande added the Type: Security Security concern or PRs that must be reviewed with extra care regarding security. label Mar 8, 2016
@maxpoulin64
Copy link
Member

I think 0660 would be better than 0600. Most of the time, each user has its own group associated with it, and forcing it to 0600 can be really annoying for people who want to add users through a separate web server. With 0660, you can just add the webserver to the group and be fine, while with 0600 you're pretty much stuck having a job change them back to 0660.

I really don't think this should be our responsibility to set those anyway. Doing that kind of thing is generally very annoying for sysadmins in my opinion because it overrides all the defaults that was set by the user. Plus, I don't know of any other software that does that, so I don't understand why we would want to do that.

Additionally, this doesn't matter at all because one could also just chmod 0700 the home directory of the lounge to lock out everyone, so even if the files has been 0666 in the directory it still doesn't matter. I would rather add an instruction to the documentation that suggest putting the home directory to 0700 than being intrusive and do it our own stubborn way.

I am tempted to give this a -1, so I would like to hear the opinions of other people on this.

Avoid exposing hashed passwords to bruteforce from other system users.
@JocelynDelalande
Copy link
Contributor Author

@maxpoulin64 Thanks for being sooo fast to answer :-)

I think 0660 would be better than 0600. Most of the time, each user has its own group associated with it, and forcing it to 0600 can be really annoying for people who want to add users through a separate web server. With 0660, you can just add the webserver to the group and be fine, while with 0600 you're pretty much stuck having a job change them back to 0660.

Fair point. Edited accordingly.

I really don't think this should be our responsibility to set those anyway. Doing that kind of thing is generally very annoying for sysadmins in my opinion because it overrides all the defaults that was set by the user. Plus, I don't know of any other software that does that, so I don't understand why we would want to do that.

Check how /etc/shadow (containing hashed passwords) is stored (0640), how SSH handle stores private key (0600, even when passphrase protected). In those examples the spirit is security by default rather than security for super-skilled adminsys elite.

These are some cases where "system/user defaults" are not enough, because data is specially sensible, and thus software must make a permission choice to have security by default. On a end-user or easy-installable software (remember: lounge can even be run as a "desktop" IRC client), default behavior matter a lot in terms of security.

Additionally, this doesn't matter at all because one could also just chmod 0700 the home directory of the lounge to lock out everyone, so even if the files has been 0666 in the directory it still doesn't matter.

… but that's not the default on any distribution I know. Default settings is what matters for most users.

@astorije
Copy link
Member

astorije commented Mar 8, 2016

(Not that it's any relevant for this PR, but the Ansible role I maintain for The Lounge sets permissions of the user configuration files to 0660. Should it stay that way?)

@astorije astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Mar 8, 2016
var users_path = Helper.HOME + "/users" ;
var user_path = users_path + "/" + userDict.user + ".json";

mkdirp(users_path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we set correct chmod here too? https://www.npmjs.com/package/mkdirp#mkdirpdir-opts-cb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. I'd stick to UNIX practices : user list can be viewed or not : responsibility of system/user conf (see /etc/passwd), but password are sensitive, so prevent world-reading (see /etc/shadow).

@maxpoulin64
Copy link
Member

In light of the debate we are having, I think a better solution to avoid any problems would be to simply set 0770 here, and let the defaults everywhere else. Since the execute flag on directories controls whether it can be traversed at all or not, it wouldn't matter which permissions any of the other files have since the base directory isn't traversable in the first place. This is safe from our perspective, and also not intrusive at all for sysadmins and allows finer access control to config.js, users/ and also each user profile individually if they want to while remaining secure by default.

A quick example of things that could be done that I would like to avoid breaking:

  • config.js can be made 0660 to thelounge:thelounge
  • users/ can be made 0771 to thelounge:webserver
  • users/Max-P.json can be made 0660 to max-p:thelounge

Which I repeat, would still be safe as long as ~/.lounge is 0770 by default (restricted to thelounge:thelounge). What do you guys think of that solution? I think it pretty much solves it.

@JocelynDelalande
Copy link
Contributor Author

In light of the debate we are having, I think a better solution to avoid any problems would be to simply set 0770 here, and let the defaults everywhere else.

The PR only deals with files, no dirs.

@maxpoulin64
Copy link
Member

The PR only deals with files, no dirs.

I know, but I don't like it because it's really intrusive. So I suggest changing the permissions of only one directory instead, which is just as good.

@JocelynDelalande
Copy link
Contributor Author

(Not that it's any relevant for this PR, but the Ansible role I maintain for The Lounge sets permissions of the user configuration files to 0660. Should it stay that way?)

I'd say… do as you fancy, according to the needs of your role. Seems a sane setting for standard use though. To harden your configuration a bit you may want to tweak users directory, to something like 0770 or 0700 as well, depending if you use the group or not. It will protect even a user your create without ansible.

@astorije
Copy link
Member

@JocelynDelalande, @maxpoulin64, any news on this?

@maxpoulin64
Copy link
Member

I'd say… do as you fancy, according to the needs of your role.

@astorije All veto this and close it then.

maxpoulin64 added a commit to maxpoulin64/thelounge that referenced this pull request Mar 19, 2016
maxpoulin64 added a commit to maxpoulin64/thelounge that referenced this pull request Mar 19, 2016
maxpoulin64 added a commit to maxpoulin64/thelounge that referenced this pull request Mar 19, 2016
@JocelynDelalande
Copy link
Contributor Author

I'd say… do as you fancy, according to the needs of your role.

@astorije All veto this and close it then.

I don't get the link between the quote where I was answering astorije about his ansible role and the fact you veto & close. Maybe we misunderstood each other somewhere :-)

@JocelynDelalande
Copy link
Contributor Author

I'd really like that we reach some consensus. So, here is a compromise proposal that may make it acceptable to you @maxpoulin64:

  • I fulfill my remaining TODO (not reseting modes on all writes)
  • I add a config option with default mode to set on user files (for exigent sysadmins ;-)

@maxpoulin64 and others, what do you think ?

I'd wait for you approval before writing any line, don't want to waste time

nb: unix ACL do the job the advanced needs you mention @maxpoulin64 , without adding a config option.

@maxpoulin64
Copy link
Member

Continuing from #205:

IMHO, it's more intrusive than #165 : by default it shadows all files from every user, even for non-secret settings (I'm thinking specially about config.js).

Absolutely not. This does nothing at all if the directory already exists, so you can set whatever you choose to. Once, and only if you didn't create it a dedicated home directory for your system service. This is not intrusive at all compared to shoving what we think is the best down the throat of everyone.

It also prevents getting the admin/sudo rights when not strictly necessary (eg: reading the config files).

Why would you need sudo to read your own home directory? If you install The Lounge system-wide, you'll already have to create the appropriate directory in /etc and /var/lib, with appropriate permissions, so my PR wouldn't touch those at all beacuse it's a one-time operation.

I fulfill my remaining TODO (not reseting modes on all writes)
I add a config option with default mode to set on user files (for exigent sysadmins ;-)

That's getting really complicated to fix an edge case for people who sysadmin that shouldn't be allowed to admin a server. Linux has this config option built-in, and it's called the umask. The user shouldn't have to change options because we're doing stupid things we shouldn't be doing in the first place.

nb: unix ACL do the job the advanced needs you mention @maxpoulin64 , without adding a config option.

If I have to use ACLs to work around other software doing stupid things, something is seriously wrong.

@xPaw I'm of opinion that we don't really need to be messing with modes at all, but if we are going to, doing it on users folder should be a good compromise.

That. A whole lot of that. Linux gives us all the tools needed to deal with that nativley, we have no reason to interfere with that. Literally no other software that I know of does that.

@astorije
Copy link
Member

Urgh, I must admit it's very confusing to revive a discussion on a closed PR while there is an open one with a start of discussion. I wish we had stayed on #205 for clarity...
I'll comment here what is about #165 and will comment on #205 what is about #205. After all, these are 2 different solutions and can be commented independently. Also, I'm convinced we can reach a solution without yet another debate... well, I'm hopeful at least.

I add a config option with default mode to set on user files (for exigent sysadmins ;-)

I know I'm gonna be painted as the guy always against settings again (which is not true :p), but I'm very much against it.
First, you mentioned avoiding exotic situations: I personally don't know of any well-designed software that does that (please don't quote me isolated examples, it's not the point). It seems to me like an extra level of indirection, considering there is already a place where to store the file permissions: the permissions themselves (including sticky bit, etc.).
Also, I am convinced such solution can quickly become a sysadmin nightmare when your system configuration is managed other than by hand (configuration tools such as Chef, Puppet, Ansible, which creates and permissions folders independently of the insall, or packaged apps such as Cozy, Bytesized Hosting). I'm sure I'm forgetting examples, but my point is that it will be easy to get this wrong.

nb: unix ACL do the job the advanced needs you mention @maxpoulin64 , without adding a config option.

I see ACLs as a powerful tool to match complex or very specific use cases. We are trying to find a go-to solution for a simple problem, so we should not expect admins to rely on ACLs to get things right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project. Type: Security Security concern or PRs that must be reviewed with extra care regarding security.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants