| 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) *  |
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) * |
Date: 2014-03-28 08:54 |
| yes, this seems bad enough for inclusion in security releases. |
|
|
| msg215025 - (view) |
Author: STINNER Victor (vstinner) * |
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 (vstinner) * |
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) * |
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) * |
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 (vstinner) * |
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) * |
Date: 2014-03-31 11:38 |
| See also . 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) * |
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. |
|
|