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
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-08-13 12:48 by Jim.Jewett.

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 (6)
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
read(3, "blablabla\n", 16384)           = 10
read(3, "", 12288)                      = 0
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.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 ( 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:


(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.
msg323481 - (view) Author: Jim Jewett (Jim.Jewett) (Python triager) Date: 2018-08-13 12:48
(Note:  I am talking only about the disclosure issue; file corruption would ideally be fixed as far back as possible, though I would be somewhat sympathetic to a "nah, that ain't security, too late" argument.)

My current UI shows this as relevant to every release *except* 3.4 and 3.8.  If it is really 3.4 only, I think it should be closed -- anyone still using 3.4 *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.  

If it really does apply to 3.5-3.7, then it would be good to do the same fix in all (and to match 3.8, which presumably is also affected, and simply wasn't available to check when the Versions were last set).

If, for some reason, the *right* fix on 3.8 (or at least 3.7 or 3.6) doesn't apply to earlier 3.x versions, I suggest closing it as won't-fix on those older versions.

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.
Date User Action Args
2018-08-13 12:48:25Jim.Jewettsetmessages: + msg323481
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