Comment 2 for bug 2024204

Revision history for this message
Seth Arnold (seth-arnold) wrote :

I think there's multiple races here:

livepatch_support_cache = DataObjectFile(
    LivepatchSupportCacheData,
    UAFile(
        "livepatch-kernel-support-cache.json",
        directory=defaults.UAC_TMP_PATH,
        private=False,
    ),
    file_format=DataObjectFileFormat.JSON,
)

...

class UAFile:
...
    def write(self, content: str):
        file_mode = (
            defaults.ROOT_READABLE_MODE
            if self.is_private
            else defaults.WORLD_READABLE_MODE
        )
        if not os.path.exists(self._directory):
            os.makedirs(self._directory)
            if os.path.basename(self._directory) == defaults.PRIVATE_SUBDIR:
                os.chmod(self._directory, 0o700)
        system.write_file(self.path, content, file_mode)

Setting the permissions after it is created leaves open a small window for another process to race between the mkdir() and chmod() call. Setting the permissions correctly in the mkdir() call would mitigate this problem.

def write_file(filename: str, content: str, mode: int = 0o644) -> None:
    """Write content to the provided filename encoding it if necessary.

    @param filename: The full path of the file to write.
    @param content: The content to write to the file.
    @param mode: The filesystem mode to set on the file.
    """
    tmpf = None
    try:
        os.makedirs(os.path.dirname(filename), exist_ok=True)
        tmpf = tempfile.NamedTemporaryFile(
            mode="wb", delete=False, dir=os.path.dirname(filename)
        )
        logging.debug(
            "Writing file %s atomically via tempfile %s", filename, tmpf.name
        )
        tmpf.write(content.encode("utf-8"))
        tmpf.flush()
        tmpf.close()
        os.chmod(tmpf.name, mode)
        os.rename(tmpf.name, filename)
    except Exception as e:
        if tmpf is not None:
            os.unlink(tmpf.name)
        raise e

There's another race here between an open() hidden in the NamedTemporaryFile() function and the os.chmod() function here; this race is a lot longer than the previous one. If the permissions are being reduced, there's a window where another process could open the file and hold on to a file descriptor. This wouldn't be revoked when the permissions are changed. This could be mitigated by setting the permissions when creating the file.

If the directory is also group- or world- writable without being sticky, another process could also swap in a new file for the chmod() and rename() operations.

I can't immediately think of a path to use these for exploitation, but maybe I'm just not inventive enough.

Thanks