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

Hackage credentials file is world-readable when saved #2159

Open
nh2 opened this Issue May 18, 2016 · 19 comments

Comments

Projects
None yet
8 participants
@nh2
Collaborator

nh2 commented May 18, 2016

saveCreds (

saveCreds :: Config -> HackageCreds -> IO ()
) creates ~/.stack/upload/credentials.json (containing Hackage username and password) without explicit file permissions, which results in the default umask being used, which is typically rw-rw-r, e.g. on Ubuntu.

This means that when your home directory is also world-readable (on Ubuntu it is, rwxr-xr-x), other users can grab your Hackage credentials.

For single user systems this is less problematic but not ideal for e.g. university setups where all users' homes are mounted over NFS.

The fix would be for stack to create this file with rw------- permissions (and ideally check this when reading so that upgrades from old versions of stack can notice the problem).

Independently, as a person who generally dislikes any on-disk plaintext passwords, I'd prefer if the default behaviour was not to save the credentials on disk, but use a flag for that.

@nh2

This comment has been minimized.

Collaborator

nh2 commented May 18, 2016

It's unclear to me how to safely create this credentials file (atomically with the right permissions) in a cross-platform fashion.

open(filename, O_RDWR|O_CREAT, 0600) would do on Unixes, but to do that we need the unix package (unix-compat, which stack currently uses, does not provide it).

I've asked this on http://stackoverflow.com/questions/37304650/how-to-create-a-user-only-readable-file-cross-platform to get further input.

It seems to me that without further info, we have to conditionally depend on the unix package to make this safe at least on Unix (a touch; chmod 600; write approach isn't guaranteed to be safe because somebody else can open the file for reading before the permissions change applies, especially with inotify this is very feasible).

@mgsloan mgsloan added this to the P2: Should milestone May 18, 2016

@fmthoma

This comment has been minimized.

Contributor

fmthoma commented Dec 2, 2016

In addition to restricting the file permissions, I would suggest not saving the password by default. There could be a config option, or a prompt when first running stack upload, so the user can decide whether or not to save the password to disk, but this should be opt-in and not default behavior.

@mat8913

This comment has been minimized.

mat8913 commented Apr 11, 2017

Would it be safe to do something like mkdir tmp; chmod 700 tmp; touch tmp/file; chmod 600 tmp/file; mv tmp/file .; rmdir tmp; write?

@nh2

This comment has been minimized.

Collaborator

nh2 commented Apr 12, 2017

Would it be safe to do something like mkdir tmp; chmod 700 tmp; touch tmp/file; chmod 600 tmp/file; mv tmp/file .; rmdir tmp; write?

Yes, that should work. I don't tink you necessarily need to create the temporary directory, putting your temp file into the same dir as the destination would also work.

I see your key point though, this can be done using just functions from directory, e.g. renameFile and setPermissions. Good find!

(To detail on the case of using unix: rm, setFileCreationMask, openFd with the correct permissions would also be sufficient,which is the equivalent of the C code I've mentioned above.)

@mgsloan mgsloan modified the milestones: P1: Must, P2: Should Apr 23, 2017

@lwm lwm self-assigned this Aug 20, 2017

@lwm

This comment has been minimized.

Member

lwm commented Aug 21, 2017

There could be a config option

Please note, since ca6ba8f (Stack 1.5.0) it is possible to pass:

save-hackage-creds: false

This is not the default (I believe) but a message is shown.

I believe the permissions issue is still not addressed. I'll take a look.

@lwm

This comment has been minimized.

Member

lwm commented Aug 26, 2017

@nh2, I took a quick look again here ...

Would it be safe to do something like mkdir tmp; chmod 700 tmp; touch tmp/file; chmod 600 tmp/file; mv tmp/file .; rmdir tmp; write?

Yes, that should work. I don't tink you necessarily need to create the temporary directory, putting your temp file into the same dir as the destination would also work.

Hmm, but is this not the same as you mentioned in:

(a touch; chmod 600; write approach isn't guaranteed to be safe because somebody else can open the file for reading before the permissions change applies, especially with inotify this is very feasible).

I'm really not sure what this would even mean on a Windows machine.

I'm inclined to think this whole file saving is a bad idea now as mentioned in comments above. Especially thinking cross-platform. Could we not just load username/password in from environment variables (user can deal with this safely)? Any objections?

@mattaudesse

This comment has been minimized.

Member

mattaudesse commented Aug 26, 2017

FWIW I agree with @lwm - automatically having credentials stored in the clear doesn't seem worth the risk considering how little effort it would require for users who understood the vulnerability and wanted to set it up manually anyway.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Aug 27, 2017

What about changing the default from "save to the file" to "prompt the user if the password should be saved"? We can also give the user a message that, if they're tired of getting prompted, they can set save-hackage-creds: false.

@lwm

This comment has been minimized.

Member

lwm commented Aug 28, 2017

@nh2 ^^^

Seems like we remove the default of saving with this suggested approach. save-hackage-creds could easily be set in the global project configuration for security conscious users to not have to think about this again.

I see cabal also allows to save credentials in plain text in a configuration file. To be honest, the only decent way I've seen of keeping credentials safe on disk is using the gpg tool to encrypt and then having them decrypted and piped out - which I'm almost sure this is too much work to implement here.

@nh2

This comment has been minimized.

Collaborator

nh2 commented Aug 29, 2017

Hmm, but is this not the same as you mentioned in:

@lwm The difference is the mv. I said touch + chmod + write isn't safe, but @mat8913 suggested touch + chmod + mv + write, which IIRC would be safe.

I'm really not sure what this would even mean on a Windows machine.

Which part do you mean, the inotify bit? What I meant is simply that if somebody wants to snatch your password by exploiting the fact that it's world-readable for a short moment, it is easy to do so with tools like inotify.

What about changing the default from "save to the file" to "prompt the user if the password should be saved"?

@snoyberg Sounds good!

Seems like we remove the default of saving with this suggested approach. save-hackage-creds could easily be set in the global project configuration for security conscious users to not have to think about this again.

@lwm I may have been a bit unclear in titling the issue -- my problem isn't that stack saves passwords in plain text files (this is very common and useful, e.g. ~/.aws/credentials and ~/.ssh/id_rsa also are plain text passwords); the problem is only that the permissions of those files are world-readable or a short amount of time.

Whether we ask for the password to be saved seems completely orthogonal to this -- whenever we save credentials on disk, we should ensure that the permissions are right before the contents are written. The touch + chmod + mv + write approach should solve that well.

@lwm

This comment has been minimized.

Member

lwm commented Aug 29, 2017

Thanks for clarifying!

So, if I got this right then, here's what needs to be done:

  • Change the default file save to asking to save or not via prompt
  • Supply a message which mentions save-hackage-creds to avoid the prompt
  • Using functions from directory/Prelude, use the touch + chmod + mv + write approach for creating the credentials file. I think this corresponds to writeFile "" + setPermmissions + copyFile + writeFile.
@mat8913

This comment has been minimized.

mat8913 commented Aug 29, 2017

Probably use renameFile instead of copyFile. Also, use setFileMode from unix-compat to ensure only the owner has permission on the file and temporary directory.

@nh2

This comment has been minimized.

Collaborator

nh2 commented Aug 29, 2017

Probably use renameFile instead of copyFile

Yes

Also, use setFileMode from unix-compat to ensure only the owner has permission on the file and temporary directory.

@mat8913 Wait, I still don't get why we need the temporary directory. Why isn't the file enough?

@mat8913

This comment has been minimized.

mat8913 commented Aug 29, 2017

Because the file cannot be created with the correct permissions. At best you could do this:

  1. Create empty file
  2. Set permissions on file
  3. Write data to file

However, an attacker could open the file between steps 1 and 2. They then wait for step 3 to finish before actually reading the opened file.

@mat8913

This comment has been minimized.

mat8913 commented Aug 29, 2017

Oh, wait a minute. I think I understand what you are saying. If I'm understanding correctly, your proposed strategy is:

  1. Create empty file
  2. Set permissions on file
  3. Move file
  4. Write data to file

However, I think file descriptors remain valid even if the file is moved, so maybe copying is the correct strategy.

Edit: After further testing, I can confirm that file descriptors do remain valid after file is moved. Here's some C code for Linux (might also work on other POSIX systems):

#include <stdlib.h>
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>

int main(void)
{
	char buffer[255];
	int file = open("test.txt", O_RDONLY);
	fgets(buffer, 255, stdin);
	ssize_t r;
	while ((r = read(file, buffer, 255)) > 0)
		fwrite(buffer, r, 1, stdout);
	return 0;
}

Steps to reproduce:

  1. Create empty file "test.txt"
  2. Run the C code
  3. Move "test.txt" somewhere else
  4. Write data to it
  5. Press enter in the C program

You should see the data you wrote to the moved file output to the terminal.

@mattaudesse

This comment has been minimized.

Member

mattaudesse commented Aug 29, 2017

@mat8913 I just tried your C code + steps on FreeBSD with clang (4.0.0).

The code ran fine and I got the same behavior you described with file descriptors remaining valid once a file has been moved.

@nh2

This comment has been minimized.

Collaborator

nh2 commented Aug 29, 2017

@mat8913 You are right, what I wrote is rubbish.

Another easy check with 2 terminals and python:

➤ echo hello > testfile
In [1]: f = open('testfile', 'r')
➤ mv testfile testfile2
➤ echo hello2 > testfile2
In [2]: f.read()
Out[2]: 'hello2\n'

@mat8913 Do you know if your mkdir tmp; chmod 700 tmp; touch tmp/file; chmod 600 tmp/file; mv tmp/file .; rmdir tmp; write approach is safe even in the presence of openat()?

If it's not safe, why can't we just use open() from libc? I know unix says The package is not supported under Windows but why is that? Doesn't even Windows support enough of POSIX to support int open (const char *filename, int flags, mode_t mode)? Is it just that this would work but the unix package doesn't make it work for some reason?

@mat8913

This comment has been minimized.

mat8913 commented Aug 30, 2017

As far as I can tell, my approach is safe. Here's the code I used to test:

#include <stdlib.h>
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>

int main(void)
{
	char buffer[255];
	int dir = open("dir", O_DIRECTORY);
	fgets(buffer, 255, stdin);
	int file = openat(dir, "test.txt", O_RDONLY);
	if (file == -1) {
		perror("openat");
		return -1;
	}
	ssize_t r;
	while ((r = read(file, buffer, 255)) > 0)
		fwrite(buffer, r, 1, stdout);
	return 0;
}

Steps I followed:

  1. As root: mkdir dir
  2. As normal user: run the C program
  3. As root: chmod 700 dir; echo test > dir/test.txt
  4. As normal user: press Enter in the C program

If the C program manages to read the file, it will print it's contents. Otherwise, it will print the error. On my computer, it printed the error:

openat: Permission denied

Windows does seem to support open, but it's deprecated.
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open

@lwm lwm referenced this issue Sep 17, 2017

Merged

Don't save Hackage credentials without asking first #3436

1 of 2 tasks complete
@lwm

This comment has been minimized.

Member

lwm commented Sep 17, 2017

Following #2159 (comment), I've opened up #3436 to kill the first two points. @paulrzcz, I know you mentioned writing a C function to solve the security issue - any progress there (open for anyone else who wishes to delve in there)?

@lwm lwm changed the title from stack upload should not save Hackage password in world-readable plain text to Hackage credentials file is world-readable when saved Dec 22, 2017

@lwm lwm assigned paulrzcz and unassigned lwm Dec 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment