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.
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)
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. /hg.python. org/cpython/ file/63bde882e3 11/Lib/ tempfile. py#l235
To do that NamedTemporaryFile hardcodes this to the minimal permission of 0600 here
https:/
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 files/files. py files/files. py WORLD_READABLE_ MODE exists( self._directory ): self._directory ) basename( self._directory ) == defaults. PRIVATE_ SUBDIR: self._directory , 0o700) self._directory ) self._directory , mode=0o700)
system. write_file( self.path, content, file_mode)
index 821493f5..475a2d7a 100644
--- a/uaclient/
+++ b/uaclient/
@@ -42,9 +42,10 @@ class UAFile:
else defaults.
)
if not os.path.
- os.makedirs(
if os.path.
- os.chmod(
+ os.makedirs(
+ else:
+ os.makedirs(
def read(self) -> Optional[str]:
Pro squad, WDYT?