classification
Title: os.makedirs(exist_ok=True) is not thread-safe: umask is set temporary to 0, serious security problem
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4, Python 3.3, Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, benjamin.peterson, christian.heimes, desrt, flox, georg.brandl, haypo, koobs, larry, matejcik, ned.deily, pitrou, python-dev, rpointel, serhiy.storchaka, terry.reedy
Priority: release blocker Keywords: patch, security_issue

Created on 2014-03-28 07:04 by desrt, last changed 2014-04-01 23:23 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
get_masked_mode.patch haypo, 2014-03-28 09:10 review
Messages (11)
msg215020 - (view) Author: Ryan Lortie (desrt) Date: 2014-03-28 07:04
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.
msg215022 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2014-03-28 08:19
The issue associated with the patch in question is Issue9299. Adding possibly affected releases and release managers for evaluation.
msg215024 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-03-28 08:54
yes, this seems bad enough for inclusion in security releases.
msg215025 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-03-28 09:02
> http://bugs.python.org/file19849/mkdirs.tr.diff

This patch comes from issue #9299: changeset 89cda0833ba6 made in Python 3.2 beta 1. The change was not backported to Python 2.7.
msg215026 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-03-28 09:10
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.
msg215027 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-03-28 09:47
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"
msg215028 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-03-28 09:48
(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)
msg215034 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-03-28 10:26
> 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 (*).

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:

if makedirs("a/b", mod=0o755, exists_ok=True):
  os.chmod("a", 0o755)
  os.chmod("a/b", 0o755)
# else a and b were created with the permission 0o755
msg215229 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-03-31 11:38
See also issue19930. 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.
msg215345 - (view) Author: Roundup Robot (python-dev) Date: 2014-04-01 23:22
New changeset 9186f4a18584 by Benjamin Peterson in branch '3.2':
remove directory mode check from makedirs (closes #21082)
http://hg.python.org/cpython/rev/9186f4a18584

New changeset 6370d44013f7 by Benjamin Peterson in branch '3.3':
merge 3.2 (#21082)
http://hg.python.org/cpython/rev/6370d44013f7

New changeset c24dd53ab4b9 by Benjamin Peterson in branch '3.4':
merge 3.3 (#21082)
http://hg.python.org/cpython/rev/c24dd53ab4b9

New changeset adfcdc831e98 by Benjamin Peterson in branch 'default':
merge 3.4 (#21082)
http://hg.python.org/cpython/rev/adfcdc831e98
msg215346 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2014-04-01 23:23
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.
History
Date User Action Args
2014-04-01 23:23:44benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg215346
2014-04-01 23:22:27python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg215345

resolution: fixed
stage: resolved
2014-03-31 15:06:01matejciksetnosy: + matejcik
2014-03-31 11:38:06serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg215229
2014-03-31 02:11:32rhettingersetnosy: + christian.heimes
2014-03-28 22:39:19floxsetnosy: + flox
2014-03-28 21:52:25koobssetnosy: + koobs
2014-03-28 18:02:17terry.reedysetnosy: + Arfrever
2014-03-28 16:58:45benjamin.petersonsetpriority: high -> release blocker
2014-03-28 14:14:08rpointelsetnosy: + rpointel
2014-03-28 10:26:39hayposetmessages: + msg215034
2014-03-28 09:48:45pitrousetmessages: + msg215028
2014-03-28 09:47:27pitrousetnosy: + terry.reedy, pitrou
messages: + msg215027
2014-03-28 09:10:53hayposetfiles: + get_masked_mode.patch
keywords: + patch
messages: + msg215026
2014-03-28 09:04:04hayposettitle: 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
2014-03-28 09:03:03hayposettitle: _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
2014-03-28 09:02:06hayposetmessages: + msg215025
2014-03-28 08:59:13hayposetnosy: + haypo
2014-03-28 08:54:24georg.brandlsetmessages: + msg215024
2014-03-28 08:19:54ned.deilysetpriority: normal -> high
versions: + Python 3.2, Python 3.4, Python 3.5
nosy: + georg.brandl, larry, ned.deily

messages: + msg215022

keywords: + security_issue
2014-03-28 07:04:06desrtcreate