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

shutil copy* unsafe on POSIX - they preserve setuid/setgit bits #61382

Open
milkokrachounov mannequin opened this issue Feb 11, 2013 · 22 comments
Open

shutil copy* unsafe on POSIX - they preserve setuid/setgit bits #61382

milkokrachounov mannequin opened this issue Feb 11, 2013 · 22 comments
Labels
stdlib Python modules in the Lib dir type-security A security issue

Comments

@milkokrachounov
Copy link
Mannequin

milkokrachounov mannequin commented Feb 11, 2013

BPO 17180
Nosy @birkenfeld, @terryjreedy, @ronaldoussoren, @pitrou, @larryhastings, @giampaolo, @tiran, @benjaminp, @tarekziade, @hynek, @JimJJewett, @serhiy-storchaka, @aixtools
Files
  • 17180.patch
  • 17180_preserve_sbits.patch
  • 17180_preserve_sbits2.patch
  • pEpkey.asc
  • 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 = None
    created_at = <Date 2013-02-11.09:10:56.497>
    labels = ['type-security', 'library']
    title = 'shutil copy* unsafe on POSIX - they preserve setuid/setgit bits'
    updated_at = <Date 2018-08-16.19:33:06.896>
    user = 'https://bugs.python.org/milkokrachounov'

    bugs.python.org fields:

    activity = <Date 2018-08-16.19:33:06.896>
    actor = 'Michael.Felt'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2013-02-11.09:10:56.497>
    creator = 'milko.krachounov'
    dependencies = []
    files = ['29057', '29058', '30647', '47752']
    hgrepos = []
    issue_num = 17180
    keywords = ['patch']
    message_count = 22.0
    messages = ['181885', '182020', '182021', '182022', '182023', '182024', '182025', '182029', '182031', '182062', '182690', '185057', '191482', '225803', '321296', '321298', '323482', '323561', '323562', '323598', '323604', '323614']
    nosy_count = 16.0
    nosy_names = ['georg.brandl', 'terry.reedy', 'ronaldoussoren', 'pitrou', 'larry', 'giampaolo.rodola', 'christian.heimes', 'benjamin.peterson', 'tarek', 'Arfrever', 'milko.krachounov', 'neologix', 'hynek', 'Jim.Jewett', 'serhiy.storchaka', 'Michael.Felt']
    pr_nums = []
    priority = 'critical'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue17180'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @milkokrachounov
    Copy link
    Mannequin Author

    milkokrachounov mannequin commented Feb 11, 2013

    When copying the mode of a file with copy, copy2, copymode, copystat or copytree, all permission bits are copied (including setuid and setgit), but the owner of the file is not. This can be used for privilege escalation.

    An example:

    -rwSr--r-- 1 milko milko 0 фев 11 10:53 test1

    shutil.copy("test1", "test2")

    -rwSr--r-- 1 root root 0 фев 11 10:53 test2

    If test1 contained anything malicious, now the user milko can execute his malicious payload as root.

    Potential fixes:

    • Strip setuid/setgid bits.
    • Copy the owner on POSIX.
    • Perform a safety check on the owner.
    • Document the security risk.

    The behaviour of copymode/copystat in this case is the same as chmod --reference', and there can be some expectation of unsafety, but copy/copy2/copytree's behaviour differs from that of cp -p', and this is a non-obvious difference.

    @milkokrachounov milkokrachounov mannequin added the stdlib Python modules in the Lib dir label Feb 11, 2013
    @pitrou pitrou added the type-security A security issue label Feb 13, 2013
    @tiran
    Copy link
    Member

    tiran commented Feb 13, 2013

    Thanks for the report. I agree with your analysis. We should follow the behavior of cp and always strip off the suid/sgid bits in shutil.copy(). coreutil's cp removes the bits and doesn't handle source owner = destination owner special.

    There are other bits that may need special treatment, too. I'm going to check the sources of cp.

    @tiran
    Copy link
    Member

    tiran commented Feb 13, 2013

    cp removes three bits unless preserve ownership is enabled and some additional things are true.

    mode &= ~ (S_ISUID | S_ISGID | S_ISVTX)

    S_ISVTX is the sticky bit.

    @hynek
    Copy link
    Member

    hynek commented Feb 13, 2013

    While I agree that it’s a problem, I’m a bit uneasy about changing that back to 2.7. I’m pretty sure this would break numerous programs.

    @tiran
    Copy link
    Member

    tiran commented Feb 13, 2013

    Here is a patch for the issue with test and doc updates.

    I'm escalating the bug to release blocker to draw the attention of our RMs.

    @tiran
    Copy link
    Member

    tiran commented Feb 13, 2013

    Sorry for the extra noise. I got into a comment conflict with Hynek.

    Hynek,
    I don't think it's going to break lots of apps. setuid/setgid programs are rare these days. Most operating system ignore sticky bits on files, too.

    It may break system scripts that copy entire Unix systems with a recursive copytree(), though ...

    @hynek
    Copy link
    Member

    hynek commented Feb 13, 2013

    Yeah, I’m thinking about backup scripts etc.

    @tiran
    Copy link
    Member

    tiran commented Feb 13, 2013

    Here is a new patch with a new keyword argument preserve_sbits. Perhaps we use True as default for Python 2.6 to 3.3 and switch to False in Python 3.4?

    @hynek
    Copy link
    Member

    hynek commented Feb 13, 2013

    SGTM. I’d like an explicit warning on the security implications in the docs though.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 13, 2013

    Shouldn't you try to make the permission removal atomic? Otherwise there's a window of opportunity to exploit the suid bit.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Feb 22, 2013

    Shouldn't you try to make the permission removal atomic?
    Otherwise there's a window of opportunity to exploit the suid bit.

    Actually there's already a race even without setuid bit: http://bugs.python.org/issue15100

    All metadat should be set atomically.

    @benjaminp
    Copy link
    Contributor

    Not blocking 2.7.4 as discussed on mailing list.

    @tiran
    Copy link
    Member

    tiran commented Jun 19, 2013

    Shouldn't you try to make the permission removal atomic? Otherwise there's a window of opportunity to exploit the suid bit.

    Permissions bits are copied from the source file *after* all data has been copied to the destination file. copy() calls copyfile() followed by copymode()

    copyfile() doesn't create files with SUID. In fact it has 0666 & umask. In worst case the new file is readable and writable by every user. The new patch addresses the unlikely issue with os.open()ing the file with mask=0600.

    I could also add a create_mode argument to _io.FileIO() in order to make the permission bits of new files more flexible. Modules/_io/fileio.c hard codes mode as 0600.

    @serhiy-storchaka
    Copy link
    Member

    See also bpo-15795. It would be good to make shutil, zipfile and tarfile interfaces consistent.

    I think we need more graduated interface matching coretools.

    """
    --preserve[=ATTR_LIST]
    preserve the specified attributes (default: mode,ownership,timestamps), if possible additional attributes: context, links, xattr, all

       --no-preserve=ATTR_LIST
              don't preserve the specified attributes
    

    """

    This means that we should add preserve_mode, preserve_ownership, preserve_time, etc parameters. preserve_ownership should control also copying of suid/sgid/sticky bits. copy()'s defaults will be preserve_mode=True, preserve_ownership=False, preserve_time=False, copy2()'s defaults (corresponding to "cp -p" behavior) will be preserve_mode=True, preserve_ownership=True, preserve_time=True.

    @terryjreedy
    Copy link
    Member

    Should the patch be turned into a PR or should this be closed?

    @larryhastings
    Copy link
    Contributor

    I'll accept this into 3.4 and 3.5, if someone produces a PR and someone else reviews it. Given that the issue has already celebrated its fifth birthday I can't say I feel a lot of urgency about it.

    @jimjjewett
    Copy link
    Mannequin

    jimjjewett mannequin commented Aug 13, 2018

    My current UI shows this as relevant *only* to 3.4 and 3.5. If it really has been fixed in 3.6, and the fix can't be backported, I think the risk of breaking backup programs is enough to argue for doing nothing more than a doc change. Anyone still using 3.4 (or even 3.5) *and* able to install from source is likely to be more upset by unexpected (and possibly silent) breakage of an existing process than new exploits of a 6 year old bug.

    That said, I'm probably the wrong person to verify which versions are affected, so consider this as only soft support for Release Manager to do so if this continues to languish.

    @aixtools
    Copy link
    Contributor

    I am looking at this.

    Based on the comments from a historical perspective - copyfile() needs to be calling the copy_mode function before any copying actually occurs.

    As the dest is already open for writing it does not matter (on posix)
    what mode it has later on.

    Question: in copystat() there is a block that talks about chown() in the comments but in the code it only seems to be accessing chmod(). Should there (also) be a call to chown "if _SUPER"?

    _SUPER = _POSIX and os.geteuid() == 0

    basically - mode becomes:

        # remove setuid, setgid and sticky bits if _POSIX and not _SUPER
        mode = stat.S_IMODE(st.st_mode) & _REMOVE_HARMFUL_MASK if _POSIX and not _SUPER else stat.S_IMODE(st.st_mode)

    Comments?

    @aixtools
    Copy link
    Contributor

    my bad: forgot the snippet I mentioned in the previous post:

    try:
        lookup("chmod")(dst, mode, follow_symlinks=follow)
    except NotImplementedError:
        # if we got a NotImplementedError, it's because
        #   * follow_symlinks=False,
        #   * lchown() is unavailable, and
        #   * either
        #       * fchownat() is unavailable or
        #       * fchownat() doesn't implement AT_SYMLINK_NOFOLLOW.
        #         (it returned ENOSUP.)
        # therefore we're out of options--we simply cannot chown the
        # symlink.  give up, suppress the error.
        # (which is what shutil always did in this circumstance.)
        pass
    

    @aixtools
    Copy link
    Contributor

    I want to believe this can be resolved - without breakage on POSIX.

    Clarification: while Mac/OS falls under "posix" in python terms - maybe
    "breakage" will need to be accepted,
    or, for "back-ports" Mac/OS will be "as if root or super-user" and use
    an additional (optional) argument in 3.8 and beyond
    to keep backwards compatibility.

    Short text: to proceed I think we should start with getting some
    additional tests into test_shutil.py asap so that we can see how systems
    respond without any changes.

    My experience is that cp -r[pP] behaves the same as shutil.copy*() when
    the EUID==0, aka superuser,
    but strips special bits from files and cannot copy the UID/GID owner
    bits of the inode.

    I would appreciate someone helping me writing more extensive testing.

    We need to test:

    • root

    • not-root, but owner

    • not-root, not owner, but in group

    • not-root, "other", other "read" access exists

    • if the test does not already exist - also check behavior when directories
        have/do not have "search" (x-bit) enabled.

    I am working on a patch to address these different conditions.

    Ideally, the "not-owner" and "not-group" tests can be run
    by creating the "copy.me" area as root, setting perms, etc.
    and then using su -c to run the shutil.copy*() call
    and back as root make the verification.

    ±±± Perspective ±±±

    If this is too much discussion, please reply with suggestions - privately -
    on what I could do better to not waste your time.

    The issue seems unchanged since original posting.

    The original report states:
    hen copying the mode of a file with copy, copy2, copymode, copystat or
    copytree, all permission bits are copied (including setuid and setgit),
    but the owner of the file is not. This can be used for privilege escalation.

    ...snip...

    The behaviour of copymode/copystat in this case is the same as chmod --reference', and there can be some expectation of unsafety, but copy/copy2/copytree's behaviour differs from that of cp -p', and this
    is a non-obvious difference.

    For clarity: GNU chmod states:

    --reference=RFILE
        use RFILE's mode instead of MODE values

    Additionally, the chmod man page reminds us the "special bit" masking
    behavior is different for files and directries.
    Specifically, SUID, SGID and SVTX should not be cleared unless
    specifically requested by a chmod "u-s,g-s" specification.

    "... a directory's unmentioned set user and group ID bits are not affected"

    Additional comments discuss:
    short window of opportunity (files are copied first, then mode bits copied)
    breakage with the past (copytree used as "backup", regardless of version)

    And the comment/opinion that shutil.copy() should emulate cp (implies
    emulate "cp -r", so neither -p nor -P)

    it seems shutil.copy2() is adding the -p (or -P if follow_symlinks=false)

    There was a modification to test_shutil.py suggested as part of a patch.
    I added that to verify the issue is still current.

    ±±±
    diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py
    index 7e0a3292e0..7ceefd1ebc 100644
    --- a/Lib/test/test_shutil.py
    +++ b/Lib/test/test_shutil.py
    @@ -1471,6 +1471,24 @@ class TestShutil(unittest.TestCase):
             rv = shutil.copytree(src_dir, dst_dir)
             self.assertEqual(['foo'], os.listdir(rv))

    +    @unittest.skipUnless((os.name == "posix" and os.geteuid() != 0),
    "Requires POSIX compatible OS and non-root userid")
    +    def test_copy_remove_setuid(self):
    +        src_dir = self.mkdtemp()
    +        src_file = os.path.join(src_dir, 'foo')
    +        write_file(src_file, 'foo')
    +        dst_file = os.path.join(src_dir, 'bar')
    +        harmful_mode = stat.S_IRUSR | stat.S_IXUSR | stat.S_ISUID
    +        harmless_mode = stat.S_IRUSR | stat.S_IXUSR
    +
    +        # set mode and verify
    +        os.chmod(src_file, harmful_mode)
    +        mode = stat.S_IMODE(os.stat(src_file).st_mode)
    +        self.assertTrue(oct(mode), oct(harmful_mode))
    +
    +        # check that copy does not preserve harmful bits
    +        shutil.copy(src_file, dst_file)
    +        mode = stat.S_IMODE(os.stat(dst_file).st_mode)
    +        self.assertEqual(oct(mode), oct(harmless_mode))

     class TestWhich(unittest.TestCase):
    ±±±
    The result is:
    root@x066:[/data/prj/python/python3-3.8]./python -m test -v test_shutil
    == CPython 3.8.0a0 (heads/master:cca4eec3c0, Aug 13 2018, 04:53:15) [C]
    == AIX-1-00C291F54C00-powerpc-32bit big-endian
    == cwd: /data/prj/python/python3-3.8/build/test_python_10944516
    == CPU count: 8
    == encodings: locale=ISO8859-1, FS=iso8859-1
    Run tests sequentially
    ...
    test_copy_remove_setuid (test.test_shutil.TestShutil) ... FAIL
    ...
    ======================================================================
    FAIL: test_copy_remove_setuid (test.test_shutil.TestShutil)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/data/prj/python/git/python3-3.8/Lib/test/test_shutil.py", line
    1491, in test_copy_remove_setuid
        self.assertEqual(oct(mode), oct(harmless_mode))
    AssertionError: '0o4500' != '0o500'
    - 0o4500
    ?   -
    + 0o500

    On 8/15/2018 1:01 PM, Michael Felt wrote:

    Michael Felt <aixtools@felt.demon.nl> added the comment:

    I am looking at this.

    @ronaldoussoren
    Copy link
    Contributor

    I don't understand this clarification:

    Clarification: while Mac/OS falls under "posix" in python terms - maybe
    "breakage" will need to be accepted,
    or, for "back-ports" Mac/OS will be "as if root or super-user" and use
    an additional (optional) argument in 3.8 and beyond
    to keep backwards compatibility.

    AFAIK macOS should behave just like other posix-y platforms here. In particular, I've verified that cp(1) behaves the same as on other platforms: the SUID bit is stripped when copying a setuid file.

    Do you have a reason to assume that macOS is special here?

    P.S. macOS is spelled macOS, not Mac/OS

    @aixtools
    Copy link
    Contributor

    On 16/08/2018 17:34, Ronald Oussoren wrote:

    Ronald Oussoren <ronaldoussoren@mac.com> added the comment:

    I don't understand this clarification:

    > Clarification: while Mac/OS falls under "posix" in python terms - maybe
    > "breakage" will need to be accepted,
    > or, for "back-ports" Mac/OS will be "as if root or super-user" and use
    > an additional (optional) argument in 3.8 and beyond
    > to keep backwards compatibility.
    AFAIK macOS should behave just like other posix-y platforms here. In particular, I've verified that cp(1) behaves the same as on other platforms: the SUID bit is stripped when copying a setuid file.
    Glad to hear!

    Do you have a reason to assume that macOS is special here?
    No reason to assume that macOS is different. "They" are all called
    "posix" by python (Linux, macOS, AIX, FreeBSD, and I am sure there is
    something else I have forgotten). So, I was trying to neither assume
    that macOS is more "posix" or more "gnu". As the comments refer to gnu
    coreutils behavior, not "posix" behavior (chmod --reference...) I am
    looking for responses to be able to come up with better ideas for tests
    we need - and then define/design code that meets the demands of the tests.

    I was expecting or hoping macOS would behave as you describe but I was
    also trying to prepare myself for a discussion of macOS user experience
    being a discord.

    P.S. macOS is spelled macOS, not Mac/OS
    Should be clear I am not a macOS user. Corrected!

    @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

    9 participants