Repository navigation
os.makedirs(exist_ok=True) is not thread-safe: umask is set temporary to 0, serious security problem #65281
Description
| BPO | 21082 |
|---|---|
| Nosy | @birkenfeld |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2014-04-01.23:22:27.183>
created_at = <Date 2014-03-28.07:04:06.306>
labels = ['type-security', 'library', 'release-blocker']
title = 'os.makedirs(exist_ok=True) is not thread-safe: umask is set temporary to 0, serious security problem'
updated_at = <Date 2021-11-04.14:29:38.918>
user = 'https://bugs.python.org/desrt'bugs.python.org fields:
activity = <Date 2021-11-04.14:29:38.918>
actor = 'eryksun'
assignee = 'none'
closed = True
closed_date = <Date 2014-04-01.23:22:27.183>
closer = 'python-dev'
components = ['Library (Lib)']
creation = <Date 2014-03-28.07:04:06.306>
creator = 'desrt'
dependencies = []
files = ['34649']
hgrepos = []
issue_num = 21082
keywords = ['patch', 'security_issue']
message_count = 11.0
messages = ['215020', '215022', '215024', '215025', '215026', '215027', '215028', '215034', '215229', '215345', '215346']
nosy_count = 1.0
nosy_names = ['georg.brandl']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue21082'
versions = ['Python 3.2']
Activity
desrt commentedon Mar 28, 2014
http://bugs.python.org/file19849/mkdirs.tr.diff introduced a patch with this code in it:
+def _get_masked_mode(mode):
+ mask = umask(0)
+ umask(mask)
+ return mode & ~mask
This changes the umask of the entire process. If another thread manages to create a file between those two calls then it will be world read/writable, regardless of the original umask of the process.
This is not theoretical: I discovered this bug by observing it happen.
ned-deily commentedon Mar 28, 2014
The issue associated with the patch in question is bpo-9299. Adding possibly affected releases and release managers for evaluation.
birkenfeld commentedon Mar 28, 2014
yes, this seems bad enough for inclusion in security releases.
vstinner commentedon Mar 28, 2014
This patch comes from issue bpo-9299: changeset 89cda0833ba6 made in Python 3.2 beta 1. The change was not backported to Python 2.7.
[-]_get_masked_mode in os.makedirs() is a serious security problem[/-][+]os.makedirs() is not thread-safe: umask is set temporary to 0, serious security problem[/+][-]os.makedirs() is not thread-safe: umask is set temporary to 0, serious security problem[/-][+]os.makedirs(exist_ok=True) is not thread-safe: umask is set temporary to 0, serious security problem[/+]vstinner commentedon Mar 28, 2014
The shell command "umask" calls umask(022) to get the current umask, and then call umask() with result of the first call.
022 is the default umask, it's probably safer to call umask(0o22) in _get_masked_mode() instead of umask(0).
Attached patch makes this change.
If you change something, it should be backported to 3.2, 3.3 and 3.4, because I agree that it affects the security.
pitrou commentedon Mar 28, 2014
I think the behaviour that an error is raised if the permissions are not the same is a nuisance that does not correspond to actual use cases (*). People who care about permissions so much that they expect an error can do the check themselves, or call chmod().
(*) and I got similar errors several times when running setup.py, only I didn't know it was because of that "feature"
pitrou commentedon Mar 28, 2014
(note that Victor's patch is of course not an actual fix, only a mitigation; if someone is relying on a stricter umask they will still be vulnerable to this)
vstinner commentedon Mar 28, 2014
I was also surprised that makedirs() checks for the exact permission.
We can probably document that makedirs(exists_ok=True) leaves the
directory permission unchanged if the directory already exist, and
that an explicit chmod() may be needed to ensure that permissions are
the expected permissions.
If the check on permissions is removed, an enhancement would be to
return a flag to indicate if at least one directory of the path
already existed. So the caller can avoid calling chmod() if all
directories of the path had to be created.
Something like:
serhiy-storchaka commentedon Mar 31, 2014
See also bpo-19930. Behaviors of os.makedirs() and pathlib.Path.mkdir() are different. pathlib.Path.mkdir() (as the mkdir command) creates parent directories with default mode, and os.makedirs() - with specified mode.
python-dev commentedon Apr 1, 2014
New changeset 9186f4a18584 by Benjamin Peterson in branch '3.2':
remove directory mode check from makedirs (closes bpo-21082)
http://hg.python.org/cpython/rev/9186f4a18584
New changeset 6370d44013f7 by Benjamin Peterson in branch '3.3':
merge 3.2 (bpo-21082)
http://hg.python.org/cpython/rev/6370d44013f7
New changeset c24dd53ab4b9 by Benjamin Peterson in branch '3.4':
merge 3.3 (bpo-21082)
http://hg.python.org/cpython/rev/c24dd53ab4b9
New changeset adfcdc831e98 by Benjamin Peterson in branch 'default':
merge 3.4 (bpo-21082)
http://hg.python.org/cpython/rev/adfcdc831e98
benjaminp commentedon Apr 1, 2014
I've now removed the offending behavior. exist_ok is still racy because it uses path.isdir() in the exceptional case, but fixing that can be an enhancement elsewhere.