google / google-authenticator-libpam

Apache License 2.0
1.8k stars 286 forks source link

Module must not require write access to folder #86

Closed malamut-ubuntu closed 6 years ago

malamut-ubuntu commented 6 years ago

If you use custom secret files (e.g. secret=/etc/google_authentificator/${USER}) google authentificator can't work because of Failed to update secret file error. But it's not true, user have write access to their GA secret file (0600)! I don't want to grant write access to the whole folder for a lot of reasons. It's look like not secure (I remember a bug with .google_authentificator~ at least), it's not comfortable (because I need to create a lot of additional folders with the only one file), and the main problem is that there aren't any reason to give GA write access to folder. You can modify secret file in place without any problems, moreover, you should do it in place in such tasks!

ThomasHabets commented 6 years ago

The updated file is written to a temp file and then moved in place. This is standard practice because it's an atomic move that either entirely succeeds or entirely fails.

If the file is opened and written to, disk may run out of space after updating half the file, thus corrupting it.

Likewise the temp file can't be written to a user's $HOME, because that may be on a separate filesystem. The best way to guarantee same filesystem is to write the temp file in the same directory.

Your problem is why the README says:

--

When using the secret= option, you might want to also set the user= option. The latter forces the PAM module to switch to a dedicated hard-coded user id prior to doing any file operations. When using the user= option, you must not include "~" or "${HOME}" in the filename.

--

And then have the file not be owned by the user in question. Does that solve your problem?

malamut-ubuntu commented 6 years ago

Make a temp file is a standard practice, yes. But also a standard practice to edit in place if you don't worry about file corruption. A lot of software can work in both modes. GA exactly should not worry about disks problems, because if you have some problems with disk - it willn't work even with temp files)) So there are no difference between tmp and in place modification. But in place modification is much more logical for such purposes. At least GA can do in place modification if it has write access only to file. Or there could be an option to switch this modes.

And yes, looks like user= option solve problem with my configuration, thank you!

ThomasHabets commented 6 years ago

if you don't worry about file corruption

To misquote Gene Kranz: "data corruption is not an option". Correctness is the number one rule, and I'd actually be surprised if partial writes here would not allow attackers to bypass second factor.

if you have some problems with disk - it willn't work even with temp files

It'll fail to log you in, which if you can't update state is working as intended in order to prevent replay. It'll start working again once the problem is solved.

There's a HUGE difference between corrupting data and temporarily preventing logins. I hope I never use software written with your attitude, and it's a bit insulting to every programmer out there to say that this is acceptable. A program is a steward of the users data. Silently corrupting that data is not OK. Databases don't break when you run out of disk space. They stop. And if they don't, that's an actionable bug.

But in place modification is much more logical for such purposes

Unfortunately POSIX in its infinite wisdom doesn't guarantee that that works. And I won't play dice with the correctness and security of a security application.