classification
Title: Race conditions in shutil.copy, shutil.copy2 and shutil.copyfile
Type: security Stage: needs patch
Components: IO, Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, Jim.Jewett, flox, giampaolo.rodola, hynek, jm, loewis, neologix, pitrou, radoslaw.zarzynski
Priority: high Keywords:

Created on 2012-06-18 11:57 by radoslaw.zarzynski, last changed 2018-06-12 10:25 by giampaolo.rodola.

Files
File name Uploaded Description Edit
python_shutil_copyfile.diff radoslaw.zarzynski, 2012-06-18 11:57 A patch for copyfile procedure
python_shutil_copy_with_umask.diff radoslaw.zarzynski, 2012-06-18 12:03 A patch for the disclosure bug *only*
Messages (5)
msg163096 - (view) Author: Radoslaw A. Zarzynski (radoslaw.zarzynski) Date: 2012-06-18 11:57
shutil.copy and shutil.copy2 first copy a file content and afterwards
change permissions of a destination file. Unfortunately, the sequence isn't atomical and may lead to disclosure of matter of any file that is being duplicated.
            
Additionally, shutil.copyfile procedure seems to have a problem with symlinks that could result in the corruption of content of any file on filesystem (in favorable conditions).

Some functions from shutil module that depend on copy[2] (and thus copyfile) are vulnerable too.
For example, shutil.move is using copy2 when os.rename fails because of file transfer between filesystems.

I have attached listing from strace(1) system utility below that illustrates the disclosure problem.

# ls -l ./shutil_test
-r-------- 1 root root 10 06-18 11:42 shutil_test

# strace -- python -c "import shutil; shutil.move('./shutil_test', '/tmp')"
<many irrelevant lines>
open("./shutil_test", O_RDONLY)         = 3
fstat(3, {st_mode=S_IFREG|0400, st_size=10, ...}) = 0
open("/tmp/shutil_test", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 4
fstat(4, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
fstat(3, {st_mode=S_IFREG|0400, st_size=10, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fd82e13e000
read(3, "blablabla\n", 16384)           = 10
read(3, "", 12288)                      = 0
fstat(4, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fd82e13d000
read(3, "", 16384)                      = 0
write(4, "blablabla\n", 10)             = 10
close(4)                                = 0
munmap(0x7fd82e13d000, 4096)            = 0
close(3)                                = 0
munmap(0x7fd82e13e000, 4096)            = 0
stat("./shutil_test", {st_mode=S_IFREG|0400, st_size=10, ...}) = 0
utimes("/tmp/shutil_test", {{1340012952, 0}, {1340012539, 0}}) = 0
chmod("/tmp/shutil_test", 0400)         = 0

Quick fix for the first issue could rely on os.umask but much more elegant and composite solution might use combination of os.open, os.fchmod and os.fdopen instead of open(dst, 'wb') in shutil.copyfile procedure which additionally rectifies the problem with symlink attack.
However, I am not sure that the last one is portable and won't mess with another code.
I have prepared *untested* patches for both propositions.

Best regards,
Radoslaw A. Zarzynski
msg163160 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-06-19 13:17
It's not that simple as we currently use public functions for copying the files and the metadata. copyfile is explicitly _not_ supposed to copy any metadata so we can't patch it up to do so. Also that won't help for other stat & xattr data so this fix would be rather incomplete.

umask is ruled out too, because that isn't (if I'm not mistaken) thread safe thus wouldn't help against stat & xattr disclosures anyway.

Therefore we'll have to re-implement the whole metadata "stack" for copy and copy2 using fd-based functions. Taking into account #4489, I guess it's the best way (+ hoping someone implements safe versions for other platforms too).

The mode itself can also be copied pretty easily using a custom opener for open (os.open has a mode argument).

I doubt I will be able to do that till beta1 though. But I suppose we can commit this while in the betas?
msg185124 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-03-24 10:29
That shouldn't be too complicated, but does Windows have fcomod() & Co?
msg185125 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-03-24 10:34
Windows doesn't have fchmod(), but chmod() doesn't do much on it either:

“Although Windows supports chmod(), you can only set the file’s read-only flag with it (via the stat.S_IWRITE and stat.S_IREAD constants or a corresponding integer value). All other bits are ignored.”

(Windows has a sophisticated file permissions scheme, but you probably need to use native APIs to effect them)
msg213975 - (view) Author: Jim Jewett (Jim.Jewett) (Python triager) Date: 2014-03-18 14:42
Is this really only 3.4?  Since it is security-related, it seems like it should be at least considered for older versions as well.
History
Date User Action Args
2018-06-12 10:25:21giampaolo.rodolasetnosy: + giampaolo.rodola
2016-10-14 15:45:59jmsetnosy: + jm
2016-09-08 23:52:21christian.heimessetpriority: normal -> high
versions: + Python 2.7, Python 3.5, Python 3.6, Python 3.7, - Python 3.4
2014-03-18 14:42:15Jim.Jewettsetnosy: + Jim.Jewett
messages: + msg213975
2013-03-24 10:34:20pitrousetmessages: + msg185125
2013-03-24 10:29:46neologixsetmessages: + msg185124
2013-03-23 20:21:40pitrousetnosy: + neologix
2013-03-23 20:18:04Arfreversetnosy: + Arfrever
2012-06-24 07:29:49hyneksetversions: + Python 3.4, - Python 3.3
2012-06-19 13:17:10hyneksetversions: + Python 3.3, - Python 2.7, Python 3.2
nosy: + loewis, pitrou

messages: + msg163160

keywords: - patch
stage: needs patch
2012-06-18 12:34:10hyneksetnosy: + hynek
2012-06-18 12:10:47floxsetnosy: + flox
components: + IO
2012-06-18 12:03:49radoslaw.zarzynskisetfiles: + python_shutil_copy_with_umask.diff
2012-06-18 11:57:48radoslaw.zarzynskicreate