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

Restrict access to the home directory by default #205

Merged
merged 1 commit into from
Apr 27, 2016

Conversation

maxpoulin64
Copy link
Member

As discussed in #165, this PR restricts the permissions on the entire home directory, making it secure by default without being instrusive for sysadmins.

@maxpoulin64 maxpoulin64 added Type: Feature Tickets that describe a desired feature or PRs that add them to the project. second review needed Type: Security Security concern or PRs that must be reviewed with extra care regarding security. labels Mar 19, 2016
@maxpoulin64 maxpoulin64 self-assigned this Mar 19, 2016
@@ -23,7 +23,7 @@ if (program.home) {

var config = Helper.HOME + "/config.js";
if (!fs.existsSync(config)) {
mkdirp.sync(Helper.HOME);
mkdirp.sync(Helper.HOME, {mode: 0o0700});
Copy link
Member

Choose a reason for hiding this comment

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

Could you use '0700' instead please?
ESLint is not too happy with this and octal mode is not allowed in strict mode (not that we enforce it at the moment, but it would be nice as it's a simple addition and helps finding out issues...).

@astorije
Copy link
Member

@JocelynDelalande, would be great if you could take this one!

@astorije
Copy link
Member

(@maxpoulin64, I'll take ownership because @JocelynDelalande is unavailable at the moment. @JocelynDelalande, feel free to take it back if you happen to be back before it gets merged.)

Just wondering about the following:

  • Why 0700 and not 0770, or 0750? Why restricting all group permissions?
  • Same question for the initial 0, what if they want to set the sticky bit or else?

It might be that I am ignorant, and/or that this just applies when the folder gets created the first time, but since you specify "without being instrusive for sysadmins, I just want to make sure we're on the same page :-)
Thanks!

@astorije astorije self-assigned this Mar 25, 2016
@JocelynDelalande
Copy link
Contributor

First, sorry for not having time to discuss that quickly. That's life :)

As discussed in #165, this PR restricts the permissions on the entire home directory, making it secure by default without being instrusive for sysadmins.

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).

In that, this proposal is quite exotic (look at /etc/fstab, nginx config files, or… any software having shared/config files).

I don't say doing as others is always the best, but there is a reason : leaving by default access to non-sensitive information to other unix user allows them to know what are the settings of the software they may be using . It also prevents getting the admin/sudo rights when not strictly necessary (eg: reading the config files). Moreover, . So for now it's a 👎 for me.

It doesn't prevent from hardening if an adminsys wants to. I'm just suggesting that our default rights policy matches with other software default practices, for a consistent adminsys/user experience.

So I still think #165 amended with @maxpoulin64 suggestion, is less intrusive and more "right" than this one. I resume discussion on #165.

@xPaw
Copy link
Member

xPaw commented Mar 25, 2016

@JocelynDelalande What if stricter mode was set on the users folder? https://github.com/maxpoulin64/lounge/blob/I-194/src/command-line/add.js#L14

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.

@astorije
Copy link
Member

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).

I don't think it is as it's only set when the folder gets created. But when it gets applied, indeed everything goes in the dark.

@JocelynDelalande: In that, this proposal is quite exotic (look at /etc/fstab, nginx config files, or… any software having shared/config files).
@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.

Software you mention usually leave modes alone and therefore rely on the umask, unless it is critical that a file/folder must be kept secret.

So in short I agree with @xPaw here: let's leave modes alone for the configuration file, and harden them on the users folder only when it gets created. In addition to that we can add a note in the release telling people who upgrade that they can harden their users folder by setting the same mode than the one we start using.

All that being said, my concerns above still apply, mainly: why setting 0 to the group, and why setting 0 to the mode prefix (the one where one sets the sitcky bit, setgid, ...)?
Just being curious, to make sure I understand the intent :-)

@xPaw
Copy link
Member

xPaw commented Mar 25, 2016

@astorije 700 is not a bad default for users folder. I was looking at znc, and they default creating folders to that too.

@astorije
Copy link
Member

@xPaw, fair enough, and admins can set specific group permissions directly if needed (but then 700 instead of 0700 seems more appropriate to me).

@xPaw
Copy link
Member

xPaw commented Apr 12, 2016

I'll 👍 this. We store passwords in the configuration files, and the config folder is inside of the user folder.

On top of that, this change only affects creation of the folder. If a user wishes to set wider permissions on the folder, they absolutely can.

@maxpoulin64
Copy link
Member Author

So, can we finally merge this?

@MaxLeiter
Copy link
Member

Bump?

@astorije
Copy link
Member

Yes. After all, this is very innocent, and we haven't heard from @JocelynDelalande in more than a month.
@JocelynDelalande, if you still have concerns, please open an issue or a new PR or continue the discussion here, but in the meantime, merging this. 👍

@astorije astorije merged commit 1150d64 into thelounge:master Apr 27, 2016
@maxpoulin64 maxpoulin64 deleted the I-194 branch April 27, 2016 07:46
@astorije astorije added this to the ★ Next Release milestone May 15, 2016
awfulcooking added a commit to awfulcooking/thelounge-deb that referenced this pull request Sep 5, 2021
Sensitive information is protected by thelounge/thelounge#205 for
Docker builds, but deb installs did not benefit due to the pre-existence
of the home directory, created at build time.
awfulcooking added a commit to awfulcooking/thelounge-deb that referenced this pull request Sep 5, 2021
Sensitive information is protected by thelounge/thelounge#205 for
Docker builds, but deb installs did not benefit due to the pre-existence
of the home directory, created at build time.
awfulcooking added a commit to awfulcooking/thelounge-deb that referenced this pull request Sep 5, 2021
Sensitive information is protected by thelounge/thelounge#205 for
Docker builds, but deb installs didn't benefit, due to the home dir
already existing, created at build time.
awfulcooking added a commit to awfulcooking/thelounge-deb that referenced this pull request Sep 5, 2021
Sensitive information is protected by thelounge/thelounge#205 for
Docker builds, but deb installs didn't benefit, due to the home dir
already existing - created at build time.
awfulcooking added a commit to awfulcooking/thelounge-deb that referenced this pull request Sep 11, 2021
Sensitive information is protected by thelounge/thelounge#205 for
Docker builds, but deb installs didn't benefit, due to the home dir
already existing - created at build time.
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.

5 participants