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

[44fe2b09-3dde-43f4-810c-7445bd934d7e]

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: ```python assignee = None closed_at = None created_at = labels = ['type-security', 'library'] title = 'shutil copy* unsafe on POSIX - they preserve setuid/setgit bits' updated_at = user = 'https://bugs.python.org/milkokrachounov' ``` bugs.python.org fields: ```python activity = actor = 'Michael.Felt' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = 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'] ```

[44fe2b09-3dde-43f4-810c-7445bd934d7e]

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 ofcp -p', and this is a non-obvious difference.

[tiran]

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]

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]

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]

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]

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]

Yeah, I’m thinking about backup scripts etc.

[tiran]

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]

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

[pitrou]

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

[79528080-9d85-4d18-8a2a-8b1f07640dd7]

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]

Not blocking 2.7.4 as discussed on mailing list.

[tiran]

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]

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]

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

[larryhastings]

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.

[74c4563b-ab1c-43d8-9219-30c4eca796bc]

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]

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]

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]

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 ofcp -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]

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]

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!