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

tarfile extractall() allows local attacker to overwrite files while extracting #46288

Closed
mebrown mannequin opened this issue Feb 3, 2008 · 9 comments
Closed

tarfile extractall() allows local attacker to overwrite files while extracting #46288

mebrown mannequin opened this issue Feb 3, 2008 · 9 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-security A security issue

Comments

@mebrown
Copy link
Mannequin

mebrown mannequin commented Feb 3, 2008

BPO 2004
Nosy @loewis, @birkenfeld, @gustaebel
Files
  • zipfile-dirperm.diff
  • tarfile-diffs.tar.gz
  • 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 = 'https://github.com/gustaebel'
    closed_at = <Date 2008-02-05.12:07:42.806>
    created_at = <Date 2008-02-03.03:59:12.562>
    labels = ['type-security', 'library']
    title = 'tarfile extractall() allows local attacker to overwrite files while extracting'
    updated_at = <Date 2008-02-05.12:07:42.761>
    user = 'https://bugs.python.org/mebrown'

    bugs.python.org fields:

    activity = <Date 2008-02-05.12:07:42.761>
    actor = 'georg.brandl'
    assignee = 'lars.gustaebel'
    closed = True
    closed_date = <Date 2008-02-05.12:07:42.806>
    closer = 'georg.brandl'
    components = ['Library (Lib)']
    creation = <Date 2008-02-03.03:59:12.562>
    creator = 'mebrown'
    dependencies = []
    files = ['9351', '9353']
    hgrepos = []
    issue_num = 2004
    keywords = []
    message_count = 9.0
    messages = ['62016', '62017', '62018', '62021', '62039', '62050', '62064', '62069', '62070']
    nosy_count = 5.0
    nosy_names = ['loewis', 'georg.brandl', 'alanmcintyre', 'lars.gustaebel', 'mebrown']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue2004'
    versions = ['Python 2.6', 'Python 2.5', 'Python 3.0']

    @mebrown
    Copy link
    Mannequin Author

    mebrown mannequin commented Feb 3, 2008

    python 2.5.1
    tarfile.py line 1516 in extractall()

    sets directories created to world-writeable while extracting which means
    an attacker can change/modify files before perms are fixed. Suggest 770
    while extracting to fix.

    @mebrown mebrown mannequin added stdlib Python modules in the Lib dir type-security A security issue labels Feb 3, 2008
    @mebrown
    Copy link
    Mannequin Author

    mebrown mannequin commented Feb 3, 2008

    I can confirm that this issue has been addressed in trunk tarfile.py.

    @alanmcintyre
    Copy link
    Mannequin

    alanmcintyre mannequin commented Feb 3, 2008

    I noticed that in the trunk, ZipFile._extract_member, at line 865, still
    uses 777 (the default of os.makedirs) to create directories. I attached
    a patch for it.

    A quick grep shows that tarfile still uses the default permissions for
    os.makedirs and mkdir. Should these all be changed to 700?

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 3, 2008

    Lars, can you take a look?

    @loewis loewis mannequin assigned gustaebel Feb 3, 2008
    @gustaebel
    Copy link
    Mannequin

    gustaebel mannequin commented Feb 4, 2008

    This was fixed in the trunk in r53526 about a year ago following
    bpo-1507247. I did not backport it to 2.5 at that time, because it
    would have changed existing behaviour. If there are no objections I
    could do this now.

    The os.mkdir() call in TarFile.makedir() uses the default mode, but the
    real mode from the TarInfo object is applied two instructions later in
    TarFile._extract_member(). I have nothing against using 0700 in
    TarFile.makedir().

    The os.makedirs() call in _extract_member() (trunk) is fine. It creates
    missing directories that are not part of the archive with default
    permissions, that is mode 0777 with the current umask masked out.

    I attached a patchset against the release25-maint branch and the trunk
    that is supposed to fix the issue and harmonizes the code between the
    two versions.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 4, 2008

    Even though it does change existing behaviour, can anybody imagine a
    case where extracting a tarfile now fails when it previously succeeds?

    If not, I would consider this a security-relevant fix, and thus a
    candidate for a backport. Perhaps this should be raised with
    security@python.org.

    @gustaebel
    Copy link
    Mannequin

    gustaebel mannequin commented Feb 5, 2008

    os.mkdir() and os.makedirs() always apply the current umask to the mode.
    We cannot be responsible for poorly chosen umasks. In general, tarfile
    and zipfile create directories with reasonable modes. So, IMO the
    zipfile-dirperm.diff is not needed and Michael's problem depends on the
    current umask.

    The only exception is in TarFile._extract_member() in Python <= 2.5.x
    that creates missing directories that are not part of the archive(!) and
    uses os.chmod() to force a 0777 mode. That problem was addressed in
    bpo-1507247 but only for Python 2.6 and should be backported. Although
    this would change behaviour it would not cause failures.

    @gustaebel
    Copy link
    Mannequin

    gustaebel mannequin commented Feb 5, 2008

    I took the liberty of applying my patches to the trunk (r60588) and the
    release25-maint branch (r60589).

    @birkenfeld
    Copy link
    Member

    Closing as fixed.

    @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
    stdlib Python modules in the Lib dir type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant