Comment 8 for bug 2024204

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thanks Seth for your recheck about what is left and should be addressed in the next version.
We have:

1. in uaclient/files/files.py UAFile::write():
 Problem: race of makedirs -> chmod
 Solution: setting the permissions right away in the mkdirs() call

2. in uaclient/system.py::write_file
 Problem: race of NamedTemporaryFile -> chmod

You said "This could be mitigated by setting the permissions when creating the file."
But I wondered why there is no way to set an initial mode to NamedTemporaryFile to do so.

Later I found that this is intentional and considered a security feature.
To do that NamedTemporaryFile hardcodes this to the minimal permission of 0600 here
https://hg.python.org/cpython/file/63bde882e311/Lib/tempfile.py#l235

Sadly, I didn't find a more official source than https://stackoverflow.com/a/10541972/6361589

Due to that it is private until we open it up with chmod.
And we might open it up, but we are never lowering the barrier and hence no one could benefit from the race window between the hidden open and chmod in this case.

Therefore I think #2 is not an issue and can stay as-is.

---

So overall the remaining fix could be like this I guess?

diff --git a/uaclient/files/files.py b/uaclient/files/files.py
index 821493f5..475a2d7a 100644
--- a/uaclient/files/files.py
+++ b/uaclient/files/files.py
@@ -42,9 +42,10 @@ class UAFile:
             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)
+ os.makedirs(self._directory)
+ else:
+ os.makedirs(self._directory, mode=0o700)
         system.write_file(self.path, content, file_mode)

     def read(self) -> Optional[str]:

Pro squad, WDYT?