Most of our chmod calls were too late. Fixed those and a couple related issues.
From: David Shea dshea@redhat.com
This uses the "opener" argument to open to pass extra arguments through to os.open. --- pyanaconda/iutil.py | 16 ++++++++++++++++ tests/pyanaconda_tests/iutil_test.py | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+)
diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index 84bc44a..858c9b1 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -1343,3 +1343,19 @@ def open(*args, **kwargs): # pylint: disable=redefined-builtin high-level languages handle this for you, like C's fopen. """ return eintr_retry_call(_open, *args, **kwargs) + +def open_with_perm(path, mode='r', perm=0o777, **kwargs): + """Open a file with the given permission bits. + + This is more or less the same as using os.open(path, flags, perm), but + with the builtin open() semantics and return type instead of a file + descriptor. + + :param str path: The path of the file to be opened + :param str mode: The same thing as the mode argument to open() + :param int perm: What permission bits to use if creating a new file + """ + def _opener(path, open_flags): + return eintr_retry_call(os.open, path, open_flags, perm) + + return open(path, mode, opener=_opener, **kwargs) diff --git a/tests/pyanaconda_tests/iutil_test.py b/tests/pyanaconda_tests/iutil_test.py index 090d84a..9f19455 100644 --- a/tests/pyanaconda_tests/iutil_test.py +++ b/tests/pyanaconda_tests/iutil_test.py @@ -771,3 +771,23 @@ def parent_dir_test(self):
for d, r in dirs: self.assertEquals(iutil.parent_dir(d), r) + + def open_with_perm_test(self): + """Test the open_with_perm function""" + # Create a directory for test files + test_dir = tempfile.mkdtemp() + try: + # Reset the umask + old_umask = os.umask(0) + try: + # Create a file with mode 0777 + iutil.open_with_perm('test1', 'w', 0o777) + self.assertEqual(os.stat('test1').st_mode & 0o777, 0o777) + + # Create a file with mode 0600 + iutil.open_with_perm('test2', 'w', 0o600) + self.assertEqual(os.stat('test2').st_mode & 0o777, 0o600) + finally: + os.umask(old_umask) + finally: + shutil.rmtree(test_dir)
From: David Shea dshea@redhat.com
When using permission bits to restrict access to the contents of a file, the permission bits must be set when the file is created, otherwise, depending on the current umask, there may be a period where any user can open the file, and the contents will be readable as long as this file descriptor is open.
Move setting permission bits to the calls to open(). --- pyanaconda/bootloader.py | 10 +++------- pyanaconda/install.py | 9 ++------- pyanaconda/iutil.py | 3 +-- pyanaconda/users.py | 5 ++--- utils/dd/rpmutils.c | 21 ++++++++++++++------- 5 files changed, 22 insertions(+), 26 deletions(-)
diff --git a/pyanaconda/bootloader.py b/pyanaconda/bootloader.py index 65eea56..2aac710 100644 --- a/pyanaconda/bootloader.py +++ b/pyanaconda/bootloader.py @@ -953,10 +953,7 @@ def write_config_images(self, config): raise NotImplementedError()
def write_config_post(self): - try: - iutil.eintr_retry_call(os.chmod, iutil.getSysroot() + self.config_file, self.config_file_mode) - except OSError as e: - log.error("failed to set config file permissions: %s", e) + pass
def write_config(self): """ Write the bootloader configuration. """ @@ -967,7 +964,7 @@ def write_config(self): if os.access(config_path, os.R_OK): os.rename(config_path, config_path + ".anacbak")
- config = open(config_path, "w") + config = iutil.open_with_perm(config_path, "w", self.config_file_mode) self.write_config_header(config) self.write_config_images(config) config.close() @@ -1559,7 +1556,7 @@ def write_password_config(self): return
users_file = iutil.getSysroot() + "/etc/grub.d/01_users" - header = open(users_file, "w") + header = iutil.open_with_perm(users_file, "w", 0o700) header.write("#!/bin/sh -e\n\n") header.write("cat << "EOF"\n") # XXX FIXME: document somewhere that the username is "root" @@ -1570,7 +1567,6 @@ def write_password_config(self): header.write("%s\n" % password_line) header.write("EOF\n") header.close() - iutil.eintr_retry_call(os.chmod, users_file, 0o700)
def write_config(self): self.write_config_console(None) diff --git a/pyanaconda/install.py b/pyanaconda/install.py index 305bb32..43655c9 100644 --- a/pyanaconda/install.py +++ b/pyanaconda/install.py @@ -28,7 +28,6 @@ from pyanaconda.users import createLuserConf, getPassAlgo, Users from pyanaconda import flags from pyanaconda import iutil -from pyanaconda.iutil import open # pylint: disable=redefined-builtin from pyanaconda import timezone from pyanaconda import network from pyanaconda.i18n import _ @@ -40,8 +39,6 @@ log = logging.getLogger("anaconda")
def _writeKS(ksdata): - import os - path = iutil.getSysroot() + "/root/anaconda-ks.cfg"
# Clear out certain sensitive information that kickstart doesn't have a @@ -50,11 +47,9 @@ def _writeKS(ksdata): ksdata.partition.dataList() + ksdata.raid.dataList(): obj.passphrase = ""
- with open(path, "w") as f: - f.write(str(ksdata)) - # Make it so only root can read - could have passwords - iutil.eintr_retry_call(os.chmod, path, 0o600) + with iutil.open_with_perm(path, "w", 0o600) as f: + f.write(str(ksdata))
def doConfiguration(storage, payload, ksdata, instClass): willWriteNetwork = not flags.flags.imageInstall and not flags.flags.dirInstall diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index 858c9b1..7a261a1 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -795,7 +795,7 @@ def dracut_eject(device): try: if not os.path.exists(DRACUT_SHUTDOWN_EJECT): mkdirChain(os.path.dirname(DRACUT_SHUTDOWN_EJECT)) - f = open(DRACUT_SHUTDOWN_EJECT, "w") + f = open_with_perm(DRACUT_SHUTDOWN_EJECT, "w", 0o755) f.write("#!/bin/sh\n") f.write("# Created by Anaconda\n") else: @@ -803,7 +803,6 @@ def dracut_eject(device):
f.write("eject %s\n" % (device,)) f.close() - eintr_retry_call(os.chmod, DRACUT_SHUTDOWN_EJECT, 0o755) log.info("Wrote dracut shutdown eject hook for %s", device) except (IOError, OSError) as e: log.error("Error writing dracut shutdown eject hook for %s: %s", device, e) diff --git a/pyanaconda/users.py b/pyanaconda/users.py index ce8e718..5eee4fc 100644 --- a/pyanaconda/users.py +++ b/pyanaconda/users.py @@ -452,12 +452,11 @@ def setUserSshKey(self, username, key, **kwargs):
authfile = os.path.join(sshdir, "authorized_keys") authfile_existed = os.path.exists(authfile) - with open(authfile, "a") as f: + with iutil.open_with_perm(authfile, "a", 0o600) as f: f.write(key + "\n")
- # Only change mode and ownership if we created it + # Only change ownership if we created it if not authfile_existed: - iutil.eintr_retry_call(os.chmod, authfile, 0o600) iutil.eintr_retry_call(os.chown, authfile, user.get(libuser.UIDNUMBER)[0], user.get(libuser.GIDNUMBER)[0]) iutil.execWithRedirect("restorecon", ["-r", sshdir]) os._exit(0) diff --git a/utils/dd/rpmutils.c b/utils/dd/rpmutils.c index 9f06790..fbbde06 100644 --- a/utils/dd/rpmutils.c +++ b/utils/dd/rpmutils.c @@ -28,6 +28,11 @@ #include <stdio.h> #include <string.h>
+#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h> + #include <rpm/rpmlib.h> /* rpmReadPackageFile .. */ #include <rpm/rpmtag.h> #include <rpm/rpmio.h> @@ -391,20 +396,22 @@ int explodeDDRPM(const char *source,
/* Regular file */ if (towrite>=2) { - FILE *fdout = fopen(filename+offset, "w"); + int fd = open(filename+offset, O_WRONLY, fstat->st_mode);
- if (fdout==NULL){ + if (fd==-1){ rc = 33; break; }
- rc = archive_read_data_into_fd(cpio, fileno(fdout)); - if (rc==ARCHIVE_OK) { - /* set permissions on the new file */ - chmod(filename+offset, fstat->st_mode); + /* call chmod to set any bits that were removed by umask during open */ + if (fchmod(fd, fstat->st_mode) != 0) { + rc = 33; + break; }
- fclose(fdout); + rc = archive_read_data_into_fd(cpio, fd); + + close(fd); }
/* symlink, we assume that the path contained in symlink
From: David Shea dshea@redhat.com
Instead of copying the file contents and permissions and then resetting the permissions, just copy the file contents. The selinux context will be correctly applied to new files and unchanged for existing files, just as with shutil.copy. --- pyanaconda/ntp.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/pyanaconda/ntp.py b/pyanaconda/ntp.py index 701fd8d..1dd5799 100644 --- a/pyanaconda/ntp.py +++ b/pyanaconda/ntp.py @@ -31,7 +31,6 @@ import socket
from pyanaconda import isys -from pyanaconda import iutil from pyanaconda.iutil import open # pylint: disable=redefined-builtin from pyanaconda.threads import threadMgr, AnacondaThread from pyanaconda.constants import THREAD_SYNC_TIME_BASENAME @@ -192,10 +191,8 @@ def save_servers_to_config(pools, servers, conf_file_path=NTP_CONFIG_FILE,
if not out_file_path: try: - stat = os.stat(conf_file_path) # Use copy rather then move to get the correct selinux context - shutil.copy(temp_path, conf_file_path) - iutil.eintr_retry_call(os.chmod, conf_file_path, stat.st_mode) + shutil.copyfile(temp_path, conf_file_path) os.unlink(temp_path)
except OSError as oserr:
From: David Shea dshea@redhat.com
Ensure that using a temporary file replaces the original file instead of overwriting it, and vice versa. --- tests/pyanaconda_tests/simpleconfig_test.py | 58 +++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
diff --git a/tests/pyanaconda_tests/simpleconfig_test.py b/tests/pyanaconda_tests/simpleconfig_test.py index c40b69e..4d1879a 100644 --- a/tests/pyanaconda_tests/simpleconfig_test.py +++ b/tests/pyanaconda_tests/simpleconfig_test.py @@ -131,3 +131,61 @@ def remove_key_test(self): scf.reset() scf.read(testconfig.name) self.assertEqual(scf.get("BOOT"), "") + + def use_tmp_test(self): + with tempfile.NamedTemporaryFile(mode="w+t") as testconfig: + # Write a starting file and keep the handle open + testconfig.write("KEY1=value1\n") + testconfig.flush() + testconfig.seek(0) + + # Overwrite the value and write a new file + scf = SimpleConfigFile() + scf.read(testconfig.name) + scf.set(('key1', 'value2')) + scf.write(testconfig.name, use_tmp=True) + + # Check the new contents + self.assertEqual(open(testconfig.name).read(), 'KEY1=value2\n') + + # Check that the original file handle still points to the old contents + self.assertEqual(testconfig.read(), 'KEY1=value1\n') + + def use_tmp_multifs_test(self): + # Open a file on a non-default filesystem + with tempfile.NamedTemporaryFile(dir='/dev/shm', mode='w+t') as testconfig: + # Write a starting file and keep the handle open + testconfig.write("KEY1=value1\n") + testconfig.flush() + testconfig.seek(0) + + # Overwrite the value and write a new file + scf = SimpleConfigFile() + scf.read(testconfig.name) + scf.set(('key1', 'value2')) + scf.write(testconfig.name, use_tmp=True) + + # Check the new contents + self.assertEqual(open(testconfig.name).read(), 'KEY1=value2\n') + + # Check that the original file handle still points to the old contents + self.assertEqual(testconfig.read(), 'KEY1=value1\n') + + def no_use_tmp_test(self): + with tempfile.NamedTemporaryFile(mode="w+t") as testconfig: + # Write a starting file and keep the handle open + testconfig.write("KEY1=value1\n") + testconfig.flush() + testconfig.seek(0) + + # Overwrite the value and write a new file + scf = SimpleConfigFile() + scf.read(testconfig.name) + scf.set(('key1', 'value2')) + scf.write(testconfig.name, use_tmp=False) + + # Check the new contents + self.assertEqual(open(testconfig.name).read(), 'KEY1=value2\n') + + # Check that the original file handle points to the replaced contents + self.assertEqual(testconfig.read(), 'KEY1=value2\n')
From: David Shea dshea@redhat.com
shutil.move is only an atomic rename if the files are on the same filesystem, and since /tmp is usually a tmpfs, the temporary file and the config file were usually not on the same filesystem. Create the temporary file as a hidden file in the target directory and use os.rename to move it into place. --- pyanaconda/simpleconfig.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/pyanaconda/simpleconfig.py b/pyanaconda/simpleconfig.py index 4becb09..f30258f 100644 --- a/pyanaconda/simpleconfig.py +++ b/pyanaconda/simpleconfig.py @@ -23,7 +23,6 @@ # Brian C. Lane bcl@redhat.com # import os -import shutil import shlex import string # pylint: disable=deprecated-module import tempfile @@ -112,19 +111,24 @@ def write(self, filename=None, use_tmp=True): return None
if use_tmp: - tmpf = tempfile.NamedTemporaryFile(mode="w", delete=False) + filename = os.path.realpath(filename) + + # Create a temporary in the same directory as the target file to ensure + # the new file is on the same filesystem + tmpf = tempfile.NamedTemporaryFile(mode="w", delete=False, + dir=os.path.dirname(filename), prefix="." + os.path.basename(filename)) tmpf.write(str(self)) tmpf.close()
- # Move the temporary file (with 0600 permissions) over the top of the - # original and preserve the original's permissions - filename = os.path.realpath(filename) + # Change the permissions (currently 0600) to match the original file if os.path.exists(filename): m = os.stat(filename).st_mode else: - m = int('0100644', 8) - shutil.move(tmpf.name, filename) + m = 0o0644 eintr_retry_call(os.chmod, filename, m) + + # Move the temporary file over the top of the original + os.rename(tmpf.name, filename) else: # write directly to the file with open(filename, "w") as fobj:
Added label: ACK.
Closed.
Pushed.
anaconda-patches@lists.fedorahosted.org