classification
Title: shutil copy* unsafe on POSIX - they preserve setuid/setgit bits
Type: security Stage: patch review
Components: Library (Lib) Versions: Python 3.5, Python 3.2, Python 3.3, Python 3.4, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, benjamin.peterson, christian.heimes, georg.brandl, hynek, larry, milko.krachounov, neologix, pitrou, serhiy.storchaka, tarek
Priority: critical Keywords: patch

Created on 2013-02-11 09:10 by milko.krachounov, last changed 2014-08-24 08:42 by serhiy.storchaka.

Files
File name Uploaded Description Edit
17180.patch christian.heimes, 2013-02-13 12:00 review
17180_preserve_sbits.patch christian.heimes, 2013-02-13 13:04 review
17180_preserve_sbits2.patch christian.heimes, 2013-06-19 14:43 review
Messages (14)
msg181885 - (view) Author: Milko Krachounov (milko.krachounov) Date: 2013-02-11 09:10
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.
msg182020 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-13 10:59
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.
msg182021 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-13 11:10
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.
msg182022 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2013-02-13 11:49
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.
msg182023 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-13 12:00
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.
msg182024 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-13 12:05
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 ...
msg182025 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2013-02-13 12:07
Yeah, I’m thinking about backup scripts etc.
msg182029 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-13 13:04
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?
msg182031 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2013-02-13 13:09
SGTM. I’d like an explicit warning on the security implications in the docs though.
msg182062 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-02-13 21:19
Shouldn't you try to make the permission removal atomic? Otherwise there's a window of opportunity to exploit the suid bit.
msg182690 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-02-22 19:29
> 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.
msg185057 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-23 14:45
Not blocking 2.7.4 as discussed on mailing list.
msg191482 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-06-19 14:43
> 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.
msg225803 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-08-24 08:42
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.
History
Date User Action Args
2014-08-24 08:42:33serhiy.storchakasetnosy: + serhiy.storchaka

messages: + msg225803
versions: + Python 3.5
2013-06-19 14:43:59christian.heimessetfiles: + 17180_preserve_sbits2.patch

messages: + msg191482
2013-03-23 14:45:37benjamin.petersonsetpriority: release blocker -> critical

messages: + msg185057
2013-02-22 23:51:48Arfreversetnosy: + Arfrever
2013-02-22 19:29:42neologixsetnosy: + neologix
messages: + msg182690
2013-02-13 21:19:59pitrousetnosy: + pitrou
messages: + msg182062
2013-02-13 13:09:15hyneksetmessages: + msg182031
2013-02-13 13:04:01christian.heimessetfiles: + 17180_preserve_sbits.patch

messages: + msg182029
2013-02-13 12:07:01hyneksetmessages: + msg182025
2013-02-13 12:05:55christian.heimessetpriority: high -> release blocker

nosy: + larry, benjamin.peterson, georg.brandl
messages: + msg182024

stage: needs patch -> patch review
2013-02-13 12:00:47christian.heimessetfiles: + 17180.patch
keywords: + patch
2013-02-13 12:00:38christian.heimessetmessages: + msg182023
2013-02-13 11:49:27hyneksetmessages: + msg182022
2013-02-13 11:10:31christian.heimessetmessages: + msg182021
2013-02-13 10:59:26christian.heimessetmessages: + msg182020
stage: needs patch
2013-02-13 10:11:49pitrousetpriority: normal -> high
nosy: + christian.heimes, tarek, hynek

type: security
versions: + Python 3.3, Python 3.4, - Python 2.6, Python 3.1
2013-02-11 09:10:56milko.krachounovcreate