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

[Michael Felt]

Michael Felt  added the comment:

On 16/08/2018 17:34, Ronald Oussoren wrote:
> Ronald Oussoren  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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ronald Oussoren]

Ronald Oussoren  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.

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


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

--
nosy: +ronaldoussoren

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Michael Felt]

Michael Felt  added the comment:

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'
- 

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

[Michael Felt]

Michael Felt  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Michael Felt]

Michael Felt  added the comment:

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?

--
nosy: +Michael.Felt

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Jim Jewett]

Jim Jewett  added the comment:

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.

--
nosy: +Jim.Jewett

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Giampaolo Rodola']

Change by Giampaolo Rodola' :


--
nosy: +giampaolo.rodola

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Larry Hastings]

Larry Hastings  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Terry J. Reedy]

Terry J. Reedy  added the comment:

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

--
nosy: +terry.reedy
versions:  -Python 3.2, Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

See also issue15795. 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.

--
nosy: +serhiy.storchaka
versions: +Python 3.5

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Christian Heimes]

Christian Heimes added the comment:

 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.

--
Added file: http://bugs.python.org/file30647/17180_preserve_sbits2.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Benjamin Peterson]

Benjamin Peterson added the comment:

Not blocking 2.7.4 as discussed on mailing list.

--
priority: release blocker - critical

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Charles-François Natali]

Charles-François Natali added the comment:

 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.

--
nosy: +neologix

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Arfrever Frehtes Taifersar Arahesis]

Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com:


--
nosy: +Arfrever

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Antoine Pitrou]

Changes by Antoine Pitrou pit...@free.fr:


--
nosy: +christian.heimes, hynek, tarek
priority: normal - high
type:  - security
versions: +Python 3.3, Python 3.4 -Python 2.6, Python 3.1

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Christian Heimes]

Christian Heimes added the comment:

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.

--
stage:  - needs patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Christian Heimes]

Christian Heimes added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Hynek Schlawack]

Hynek Schlawack added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Christian Heimes]

Christian Heimes added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Christian Heimes]

Changes by Christian Heimes li...@cheimes.de:


--
keywords: +patch
Added file: http://bugs.python.org/file29057/17180.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Christian Heimes]

Christian Heimes added the comment:

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 ...

--
nosy: +benjamin.peterson, georg.brandl, larry
priority: high - release blocker
stage: needs patch - patch review

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Hynek Schlawack]

Hynek Schlawack added the comment:

Yeah, I’m thinking about backup scripts etc.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Christian Heimes]

Christian Heimes added the comment:

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?

--
Added file: http://bugs.python.org/file29058/17180_preserve_sbits.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Hynek Schlawack]

Hynek Schlawack added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Antoine Pitrou]

Antoine Pitrou added the comment:

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

--
nosy: +pitrou

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Milko Krachounov]

New submission from Milko Krachounov:

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 milko0 фев 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.

--
components: Library (Lib)
messages: 181885
nosy: milko.krachounov
priority: normal
severity: normal
status: open
title: shutil copy* unsafe on POSIX - they preserve setuid/setgit bits
versions: Python 2.6, Python 2.7, Python 3.1, Python 3.2

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com