Author Michael.Felt
Recipients Arfrever, Jim.Jewett, Michael.Felt, benjamin.peterson, christian.heimes, georg.brandl, giampaolo.rodola, hynek, larry, milko.krachounov, neologix, pitrou, serhiy.storchaka, tarek, terry.reedy
Date 2018-08-16.09:07:50
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <3836f08b-afe3-fa03-a73f-7bdf69f4fdf1@felt.demon.nl>
In-reply-to <1534330869.85.0.56676864532.issue17180@psf.upfronthosting.co.za>
Content
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.
Files
File name Uploaded
pEpkey.asc Michael.Felt, 2018-08-16.09:07:50
History
Date User Action Args
2018-08-16 09:07:50Michael.Feltsetrecipients: + Michael.Felt, georg.brandl, terry.reedy, pitrou, larry, giampaolo.rodola, christian.heimes, benjamin.peterson, tarek, Arfrever, milko.krachounov, neologix, hynek, Jim.Jewett, serhiy.storchaka
2018-08-16 09:07:50Michael.Feltlinkissue17180 messages
2018-08-16 09:07:50Michael.Feltcreate