Fix CVE-2015-4053 world-readable permissions on client.admin key #300
Conversation
Signed-off-by: Travis Rhoden <trhoden@redhat.com>
Signed-off-by: Travis Rhoden <trhoden@redhat.com>
No need to get to the shortname of the host -- it was never used and was also overwriting the variable used in the loop. Signed-off-by: Travis Rhoden <trhoden@redhat.com>
This is mostly for testing purposes. The method has no way to specify a directory independently from the file name, which it makes it difficult to use with automated tests. It will get refactored more later, but for now add a new kwarg that pass in a dir to write to. Since a lot of callers are passing in absolute paths (.e.g. "/etc/ceph/keyring"), we look for the beginning slash and chop it off so that we can use os.path.join() and write into something like /tmp/etc/ceph. Signed-off-by: Travis Rhoden <trhoden@redhat.com>
Signed-off-by: Travis Rhoden <trhoden@redhat.com>
|
Refer to this link for build results (access rights to CI server needed): |
|
Tests look good on my computer (http://fpaste.org/224724/) |
|
Here are tests on mine: http://fpaste.org/224725/43231057/ All pass. |
| with file(path, 'w') as f: | ||
| f.write(content) | ||
| if mode: | ||
| os.chmod(path, mode) |
There was a problem hiding this comment.
this isn't quite safe--an attacker could open the file after it is created but before it is chowned. Even chowning before write isn't quite right. You need to set the mode at create time to be totally safe.
There was a problem hiding this comment.
Thanks Sage. This thought crossed my mind, and I even looked for the right Python call to do this (open() and file() built-ins don't take a file permissions mode).
I'll have to drop to a lower-level os.open() to do it, I think. :)
You an now pass a file mode to write_file with "mode=" and it will set the file mode that after writing the file. Signed-off-by: Travis Rhoden <trhoden@redhat.com>
Signed-off-by: Travis Rhoden <trhoden@redhat.com>
codenrhoden
force-pushed
the
RM-11694-admin-perms
branch
from
0e09ef8 to
8ef6d41
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
I think this is ready for re-review. I changed the way that write_file() works such that the mode is set at file creation time. I also did some extra tests and verified that the file permissions are reset if they were previously incorrect. So this way, if someone re-runs I did a quick check of all the callers of write_file() to see if the default mode of 0644 made sense. It seemed to with one exception. Right now, all files created by write_file would have been 0644, so the default keeps that backwards compatible. However, write_file is called by write_monitor_keyring(), and I don't think the monitor keyring should be 0644, it should be 0400. However, setting that to 0400 in write_monitor_keyring() didn't work on first try. I need to check if the code path is different here between As you may gather, there is a lot of cleanup/improvement to be made here. I think if this looks good, though, it is mergeable, and I can address the monitor keyring separately. I want to see if you think differently though, and would rather see that addressed at the same time as this. |
|
Looks good to me! |
|
I'm somewhat surprised that fdopen(open()) is minimal, but I don't have a better answer either. |
|
@dmick Yeah, seriously. Though it's The downside is that it ties you to POSIX. So it's not as portable as open(). But, that's kind of the point for both existing... My references: It definitely works. But I do not know if there is a more elegant or pythonic way to do it. |
|
Yeah, no, in the little bit of research I did it does seem right. I think os is missing an fopen, personally, but...there you go. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Okay, with a couple of 👍's on this thread, I think I'm going to merge. |
Fix CVE-2015-4053 world-readable permissions on client.admin key Reviewed-by: Dan Mick <dmick@redhat.com> Reviewed-by: Sage Weil <sweil@redhat.com>
When using the
ceph-deploy admincommand, the client.admin key would get written with 0644 permissions, making it world readable within /etc/ceph.This PR makes sure that it is written with 0600 permissions, and adds tests for it as well.