On Sat, Jun 17, 2023 at 12:51:59AM -0000, Seth Arnold wrote:
> Is this a cause for concern?
> 
> tools/ua-dev-cloud-config.yaml

I got the impression that this is used in developer tooling and maybe CI
only, and doesn't end up in the built deb and thus doesn't end up in
production use of this package.

On Sat, Jun 17, 2023 at 01:10:56AM -0000, 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.

It is a race, but if we only put a sensitive file in there after the
chmod, then I think it's safe? The only thing an attacker would be able
to do is view an empty directory (which is useless). Am I missing
something?

> 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.

I don't think the permissions are being reduced. NamedTemporaryFile says
it does what mkstemp would do, and that says "The file is readable and
writable only by the creating user ID.". So the permissions are only
widened, not narrowed.