Skip to content

bpo-31940: Reject faulty lchmod implementations #4783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
asottile wants to merge 1 commit into from
Closed

bpo-31940: Reject faulty lchmod implementations #4783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 19 additions & 27 deletions Lib/shutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,8 @@ def _samefile(src, dst):
return (os.path.normcase(os.path.abspath(src)) ==
os.path.normcase(os.path.abspath(dst)))

def _stat(fn):
return fn.stat() if isinstance(fn, os.DirEntry) else os.stat(fn)
def _stat(fn, **kwargs):
return fn.stat(**kwargs) if isinstance(fn, os.DirEntry) else os.stat(fn, **kwargs)

def _islink(fn):
return fn.is_symlink() if isinstance(fn, os.DirEntry) else os.path.islink(fn)
Expand Down Expand Up @@ -283,19 +283,24 @@ def copymode(src, dst, *, follow_symlinks=True):
(e.g. Linux) this method does nothing.

"""
if not follow_symlinks and _islink(src) and os.path.islink(dst):
if hasattr(os, 'lchmod'):
stat_func, chmod_func = os.lstat, os.lchmod
else:
return
elif hasattr(os, 'chmod'):
stat_func, chmod_func = _stat, os.chmod
else:
if not follow_symlinks and not (_islink(src) and _islink(dst)):
follow_symlinks = True
st = _stat(src, follow_symlinks=follow_symlinks)
try:
os.chmod(dst, stat.S_IMODE(st.st_mode), follow_symlinks=follow_symlinks)
except NotImplementedError:
# if we got a NotImplementedError, it's because
# * follow_symlinks=False,
# * lchmod() is unavailable, and
# * either
# * fchmodnat() is unavailable or
# * fchmodat() doesn't implement AT_SYMLINK_NOFOLLOW.
# (it returned ENOSUP.)
# therefore we're out of options--we simply cannot chmod the
# symlink. give up, suppress the error.
# (which is what shutil always did in this circumstance.)
return

st = stat_func(src)
chmod_func(dst, stat.S_IMODE(st.st_mode))

if hasattr(os, 'listxattr'):
def _copyxattr(src, dst, *, follow_symlinks=True):
"""Copy extended filesystem attributes from `src` to `dst`.
Expand Down Expand Up @@ -359,20 +364,7 @@ def lookup(name):
mode = stat.S_IMODE(st.st_mode)
lookup("utime")(dst, ns=(st.st_atime_ns, st.st_mtime_ns),
follow_symlinks=follow)
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
copymode(src, dst, follow_symlinks=follow)
if hasattr(st, 'st_flags'):
try:
lookup("chflags")(dst, st.st_flags, follow_symlinks=follow)
Expand Down
45 changes: 34 additions & 11 deletions Lib/test/test_shutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,29 @@
except ImportError:
UID_GID_SUPPORT = False


def _has_functional_lchmod():
if not support.can_symlink() or not hasattr(os, 'lchmod'):
return False

fname = tempfile.mktemp()
Copy link
Copy Markdown

@TheKevJames TheKevJames Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be mkstemp? mktemp seems to be deprecated.

Copy link
Copy Markdown
Contributor Author

@asottile asottile Aug 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copied from adjacent test (in the original PR). It's not exactly that mktemp is deprecated, it's that it's insecure. However in this case, the same security problem would exist since it's essentially:

  1. mkstemp
  2. unlink the file made by mkstemp
  3. symlink to the location made by mkstemp

There's a race between 2 and 3 where something could create a file

Fortunately it doesn't really matter since it's just a test.

os.symlink(os.devnull, fname)
try:
os.lchmod(fname, 0o777)
except OSError as e:
if e.errno in {errno.ENOTSUP, errno.EOPNOTSUPP}:
return False
else:
raise
else:
return True
finally:
os.remove(fname)


HAS_FUNCTIONAL_LCHMOD = _has_functional_lchmod()


def _fake_rename(*args, **kwargs):
# Pretend the destination path is on a different filesystem.
raise OSError(getattr(errno, 'EXDEV', 18), "Invalid cross-device link")
Expand Down Expand Up @@ -358,7 +381,7 @@ def test_copymode_follow_symlinks(self):
shutil.copymode(src_link, dst_link)
self.assertEqual(os.stat(src).st_mode, os.stat(dst).st_mode)

@unittest.skipUnless(hasattr(os, 'lchmod'), 'requires os.lchmod')
@unittest.skipUnless(HAS_FUNCTIONAL_LCHMOD, 'requires os.lchmod')
@support.skip_unless_symlink
def test_copymode_symlink_to_symlink(self):
tmp_dir = self.mkdtemp()
Expand Down Expand Up @@ -388,7 +411,7 @@ def test_copymode_symlink_to_symlink(self):
shutil.copymode(src, dst_link, follow_symlinks=False)
self.assertEqual(os.stat(src).st_mode, os.stat(dst).st_mode)

@unittest.skipIf(hasattr(os, 'lchmod'), 'requires os.lchmod to be missing')
@unittest.skipIf(HAS_FUNCTIONAL_LCHMOD, 'requires os.lchmod to be missing')
@support.skip_unless_symlink
def test_copymode_symlink_to_symlink_wo_lchmod(self):
tmp_dir = self.mkdtemp()
Expand Down Expand Up @@ -417,13 +440,13 @@ def test_copystat_symlinks(self):
self.assertNotEqual(os.stat(src).st_mtime, os.stat(dst).st_mtime)
os.symlink(src, src_link)
os.symlink(dst, dst_link)
if hasattr(os, 'lchmod'):
if HAS_FUNCTIONAL_LCHMOD:
os.lchmod(src_link, stat.S_IRWXO)
if hasattr(os, 'lchflags') and hasattr(stat, 'UF_NODUMP'):
os.lchflags(src_link, stat.UF_NODUMP)
src_link_stat = os.lstat(src_link)
# follow
if hasattr(os, 'lchmod'):
if HAS_FUNCTIONAL_LCHMOD:
shutil.copystat(src_link, dst_link, follow_symlinks=True)
self.assertNotEqual(src_link_stat.st_mode, os.stat(dst).st_mode)
# don't follow
Expand All @@ -434,7 +457,7 @@ def test_copystat_symlinks(self):
# The modification times may be truncated in the new file.
self.assertLessEqual(getattr(src_link_stat, attr),
getattr(dst_link_stat, attr) + 1)
if hasattr(os, 'lchmod'):
if HAS_FUNCTIONAL_LCHMOD:
self.assertEqual(src_link_stat.st_mode, dst_link_stat.st_mode)
if hasattr(os, 'lchflags') and hasattr(src_link_stat, 'st_flags'):
self.assertEqual(src_link_stat.st_flags, dst_link_stat.st_flags)
Expand Down Expand Up @@ -561,7 +584,7 @@ def test_copy_symlinks(self):
src_link = os.path.join(tmp_dir, 'baz')
write_file(src, 'foo')
os.symlink(src, src_link)
if hasattr(os, 'lchmod'):
if HAS_FUNCTIONAL_LCHMOD:
os.lchmod(src_link, stat.S_IRWXU | stat.S_IRWXO)
# don't follow
shutil.copy(src_link, dst, follow_symlinks=True)
Expand All @@ -572,7 +595,7 @@ def test_copy_symlinks(self):
shutil.copy(src_link, dst, follow_symlinks=False)
self.assertTrue(os.path.islink(dst))
self.assertEqual(os.readlink(dst), os.readlink(src_link))
if hasattr(os, 'lchmod'):
if HAS_FUNCTIONAL_LCHMOD:
self.assertEqual(os.lstat(src_link).st_mode,
os.lstat(dst).st_mode)

Expand All @@ -584,7 +607,7 @@ def test_copy2_symlinks(self):
src_link = os.path.join(tmp_dir, 'baz')
write_file(src, 'foo')
os.symlink(src, src_link)
if hasattr(os, 'lchmod'):
if HAS_FUNCTIONAL_LCHMOD:
os.lchmod(src_link, stat.S_IRWXU | stat.S_IRWXO)
if hasattr(os, 'lchflags') and hasattr(stat, 'UF_NODUMP'):
os.lchflags(src_link, stat.UF_NODUMP)
Expand All @@ -605,7 +628,7 @@ def test_copy2_symlinks(self):
# The modification times may be truncated in the new file.
self.assertLessEqual(getattr(src_link_stat, attr),
getattr(dst_stat, attr) + 1)
if hasattr(os, 'lchmod'):
if HAS_FUNCTIONAL_LCHMOD:
self.assertEqual(src_link_stat.st_mode, dst_stat.st_mode)
self.assertNotEqual(src_stat.st_mode, dst_stat.st_mode)
if hasattr(os, 'lchflags') and hasattr(src_link_stat, 'st_flags'):
Expand Down Expand Up @@ -704,7 +727,7 @@ def test_copytree_symlinks(self):
dst_link = os.path.join(dst_dir, 'sub/link')
os.symlink(os.path.join(src_dir, 'file.txt'),
src_link)
if hasattr(os, 'lchmod'):
if HAS_FUNCTIONAL_LCHMOD:
os.lchmod(src_link, stat.S_IRWXU | stat.S_IRWXO)
if hasattr(os, 'lchflags') and hasattr(stat, 'UF_NODUMP'):
os.lchflags(src_link, stat.UF_NODUMP)
Expand All @@ -714,7 +737,7 @@ def test_copytree_symlinks(self):
self.assertEqual(os.readlink(os.path.join(dst_dir, 'sub', 'link')),
os.path.join(src_dir, 'file.txt'))
dst_stat = os.lstat(dst_link)
if hasattr(os, 'lchmod'):
if HAS_FUNCTIONAL_LCHMOD:
self.assertEqual(dst_stat.st_mode, src_stat.st_mode)
if hasattr(os, 'lchflags'):
self.assertEqual(dst_stat.st_flags, src_stat.st_flags)
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2017-12-10-11-35-57.bpo-31940.Mhw94T.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Detect faulty ``lchmod`` implementation to fix ``shutil.copystat`` on musl
libc. Patch by Anthony Sottile.
19 changes: 18 additions & 1 deletion Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2814,6 +2814,9 @@ os_chmod_impl(PyObject *module, path_t *path, int mode, int dir_fd,
#ifdef HAVE_FCHMODAT
int fchmodat_nofollow_unsupported = 0;
#endif
#ifdef HAVE_LCHMOD
int lchmod_unsupported = 0;
#endif

#if !(defined(HAVE_FCHMODAT) || defined(HAVE_LCHMOD))
if (follow_symlinks_specified("chmod", follow_symlinks))
Expand Down Expand Up @@ -2845,8 +2848,16 @@ os_chmod_impl(PyObject *module, path_t *path, int mode, int dir_fd,
else
#endif
#ifdef HAVE_LCHMOD
if ((!follow_symlinks) && (dir_fd == DEFAULT_DIR_FD))
if ((!follow_symlinks) && (dir_fd == DEFAULT_DIR_FD)) {
result = lchmod(path->narrow, mode);
/*
* similar to the comment below about fchmodat(), some platforms
* (for instance musl libc) do not ship a functional lchmod().
*/
lchmod_unsupported =
result &&
((errno == ENOTSUP) || (errno == EOPNOTSUPP));
Copy link
Copy Markdown
Member

@vstinner vstinner Dec 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, there are two different error codes? Which one is raised on muslc?

Copy link
Copy Markdown
Contributor Author

@asottile asottile Dec 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

muslc raises EOPNOTSUPP (errno 95) though on my platform they were #defined to the same thing. I simply picked the same errnos as fchmodat catches below -- I suspect implementations use fchmodat to implement lchown (or the other way around).

}
else
#endif
#ifdef HAVE_FCHMODAT
Expand Down Expand Up @@ -2888,6 +2899,12 @@ os_chmod_impl(PyObject *module, path_t *path, int mode, int dir_fd,
return NULL;
}
else
#endif
#ifdef HAVE_LCHMOD
if (lchmod_unsupported) {
follow_symlinks_specified("chmod", follow_symlinks);
return NULL;
} else
#endif
return path_error(path);
}
Expand Down