Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os.makedirs(exist_ok=True) is not thread-safe: umask is set temporary to 0, serious security problem #65281

Closed
desrt mannequin opened this issue Mar 28, 2014 · 11 comments
Closed
Labels
release-blocker stdlib type-security

Comments

@desrt
Copy link
Mannequin

@desrt desrt mannequin commented Mar 28, 2014

BPO 21082
Nosy @birkenfeld
Files
  • get_masked_mode.patch
  • 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']

    @desrt
    Copy link
    Mannequin Author

    @desrt desrt mannequin commented 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.

    @desrt desrt mannequin added stdlib type-security labels Mar 28, 2014
    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Mar 28, 2014

    The issue associated with the patch in question is bpo-9299. Adding possibly affected releases and release managers for evaluation.

    @birkenfeld
    Copy link
    Member

    @birkenfeld birkenfeld commented Mar 28, 2014

    yes, this seems bad enough for inclusion in security releases.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Mar 28, 2014

    http://bugs.python.org/file19849/mkdirs.tr.diff

    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.

    @vstinner vstinner changed the title _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 Mar 28, 2014
    @vstinner vstinner changed the title 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 Mar 28, 2014
    @vstinner
    Copy link
    Member

    @vstinner vstinner commented 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
    Copy link
    Member

    @pitrou pitrou commented 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
    Copy link
    Member

    @pitrou pitrou commented 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
    Copy link
    Member

    @vstinner vstinner commented 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 (*).

    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

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented 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
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented 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

    @python-dev python-dev mannequin closed this as completed Apr 1, 2014
    @benjaminp
    Copy link
    Contributor

    @benjaminp benjaminp commented 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.

    @ahmedsayeed1982 ahmedsayeed1982 mannequin added expert-subinterpreters and removed stdlib labels Nov 4, 2021
    @eryksun eryksun added stdlib and removed expert-subinterpreters labels Nov 4, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    release-blocker stdlib type-security
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants