Remove attempts to set file modes

[maxpoulin64]

After some testing and manually trying to set sane file modes, it turns out the umask still applies. So it seems the logical way to handle this is to actually rely on the OS umask to set proper permissions.

I have set mine to 0007, which results in the following structure:

drwxrwx--- 3 thelounge thelounge 4096 27 fév 21:34 ./
-rw-rw---- 1 thelounge thelounge 4040 27 fév 21:34 config.js
drwxrwx--- 2 thelounge thelounge 4096 27 fév 21:34 users/
-rw-rw---- 1 thelounge thelounge  133 27 fév 21:34 users/Max-P.json

This seems to be the best permissions for me, as the files are restricted to The Lounge, but the user can still allow a web server or other utilities to change the files by adding them to the lounge group.

[astorije]

I think this is a great idea, but I haven't tested so I can't give my +1 as is. For example, we should make sure it doesn't prevent an already existing configuration folder/files to be read or written, or that would break all instances already deployed.

@JocelynDelalande, assigning to you as you clearly know edge cases better than me :-) (also, you're marked as security reviewer :P).

[maxpoulin64]

I kinda of doubt out, as what I removed was literally setting the files at 0777. Technically it shouldn't actually even do anything, as I found out the umask applies on top of that anyway. It just makes it clear that the file is not going to be 0777, as we were a bunch to be "WTF??" on it then realize it litterally doesn't do anything.

[xPaw]

This change was introduced without any explanation, so it's hard to tell why it's here in the first place. 👍

[JocelynDelalande]

It works, and agreeing with @maxpoulin64 on the fact umask should apply.

I don't see obvious failure this patch could trigger : lounge is designed to be run by a specific user anyway (using ~/.lounge by default). Even if it may break some password change on some setups, I'd suggest merging this quickly over trying to imagine every deployment issue it can trigger. that 777 permission is really bad.

In a nutshell, this is an important security fix. 👍

A note should be introduced in next changelog (adding security section in changelog message ?). I suggest :

> A security breach has been discovered in #117 and fixed in this release.

It allowed any local system user to read and edit ~/.lounge/users directory and the files it contains.
If you upgrade from lounge <= v1.3.0 , make sure you fix the permission on ~/.lounge/users and the file it contains, it might have been set to 0777 by previous versions.

[maxpoulin64]

@JocelynDelalande umask always applies, so it is not actually a security breach. Even if the code said 0777, the actual permissions that got applied were different because 0777 is already the default. So this fix does literally nothing apart from removing the false impression that files are indeed 0777. If we're really wanted to put 0777 we'd have to create the file and then chmod it after the fact.

[astorije]

@JocelynDelalande, this is definitely something that would have gone in a Security section (see the documented paragraph of the raw change log) and properly highlighted as you suggested, but I didn't realize what @maxpoulin64 clarified until he did: this PR is essentially a no-op, which is a good thing for previous versions :-)
No security breach then, but still merging!

Thanks all!

[JocelynDelalande]

@JocelynDelalande umask always applies, so it is not actually a security breach. Even if the code said 0777, the actual permissions that got applied were different because 0777 is already the default. So this fix does literally nothing apart from removing the false impression that files are indeed 0777. If we're really wanted to put 0777 we'd have to create the file and then chmod it after the fact.

@maxpoulin64 Oh, after reality check, you are right.

@maxpoulin64 it's realy weird then, what is this mode option supposed to do ? The doc is not chatty about it. Could it be os-dependent ? I'm a bit dry here, and we have to figure that out before releasing (and adding or not a security note). Any node-ninja around ?

[JocelynDelalande]

EDIT: keeping this comment for the record only, suggestions expressed are no longer relevant, see my later comment

@astorije @maxpoulin64 Ok, we released v1.3.1 version w/o the security note, that may have been necessary, as there is doubt about some platforms.

Now it's done. Too late, let's just do our best on top of that :-)

We are in a state where the code intended to do something bad (the 777), and we don't know for sure that it wasn't honored on all platforms. Security common sense urge us to add a bold note about that. @maxpoulin64 offered to write test scripts to check non-GNU/Linux platform, but may not had time to do it. I myself don't have time and platforms to conduct those tests on non-GNU/Linux platforms. Anyway we have to be fast communicating with users.

Quick release

I'd suggest releasing very soon another release (it may be almost empty, doesn't matter, must include #143 + the PR spoken behind though), and add a proper release note.

Umask is not enough

Moreover, we have hashed password inside those files. So relying on umask is not enough, we should force the files to be at least 660. If not 600.

For the record, Debian/Ubuntu default umask 0is 022, which creates 0755 files.

I'm working (right after posting this) on a PR that I hope can make it quickly to that next security release.

Release-note warning (modified) proposal

I suggest re-wording as this :

Security note: a security breach has been discovered and fixed in this release (see #143 and #117 #FIXME-TO-COME).

Depending on your OS, it may have allowed any local system user to read and edit ~/.lounge/users directory and the files it contains. That is lounge users metadata. So it may have allowed unattended user creation/edition/deletion. Happily, password are not stored cleartext in those files.

If you upgrade from lounge <= v1.3.0

• check the permission on ~/.lounge/users and the file it contains. If set to 0777, you might want to fix permission to 0600 (read/write only for user), also
• check that no undesired user creation/modification happened.

Reach us on #thelounge IRC if any doubt.

Other measure would include mentioning it in #thelounge topic.

[maxpoulin64]

Happily, password are not stored cleartext in those files.

Not true, server passwords are stored in cleartext when used. This is there to stay, because you need it in order to log into some servers. Lots of servers supports using that field to log in to NickServ, and it is also used to log into ZNC if you use that.

check the permission on ~/.lounge/users and the file it contains. If set to 0777, you might want to fix permission to 0600 (read/write only for user), also

You need 0700, not 0600. You need the execute flag on directories to be able to traverse it, so without it even if the files were all 0666 inside it, you'd still wouldn't be able to access them.

[JocelynDelalande]

Finaly… I sorted it without any need to write test programs, but only reading low-level docs :

• On Windows : UNIX file permissions are almost ignored (such odd design from nodejs). -> OK
• On OSX : It uses the same semantics than GNU/Linux (it's to say: glibc) on the open system call -> the mode given on fs.writeFileSync is substracted bit-wise with the user's umask (which generaly by default prevents at least other users from writing to the files). -> OK

So… that whole PR is still legit, as a clarification, and because the user JSON files do not require x permission bit (I think original "777"s author did not understand their effect).

But this PR brings nothing new in terms of security (thus removing tags and the need on a quick release and release note).

The still relevant PR bringing actual saner security defaults is #165.

What a mess… But an interesting one :-)