bpo-38893: Add preserve_security_context to shutil

tiran · tiran · commit 559677f7d29f · 2020-12-09T16:32:41.000+01:00
Users typically don't have permission to modify the restricted
`security` namespace of extended attributes. An LSM prevents
root processes in confined contexts from changing `security`
attributes, too.

Any write attempt from a confined context is considered a security
violation and logged in the system's security audit log. Therefore we
cannot safely except and ignore exceptions without flooding the audit
system.

Linux coreutils has a similar workaround. Tools like `cp` skips
`security.selinux` when it detects that SELinux is enabled
(`preserve_security_context`). The (security) context is only copied
with non-default `--preserve=context` or `--preserve=all`.

`shutil.copy2()` now behaves mostly like `cp -p --preserve=xattr` by default.

Signed-off-by: Christian Heimes &lt;christian@python.org&gt;
diff --git a/Doc/library/shutil.rst b/Doc/library/shutil.rst
@@ -108,7 +108,8 @@ Directory and files operations
    .. versionchanged:: 3.3
       Added *follow_symlinks* argument.
 
-.. function:: copystat(src, dst, *, follow_symlinks=True)
+.. function:: copystat(src, dst, *, follow_symlinks=True, \
+                       preserve_security_context=False)
 
    Copy the permission bits, last access time, last modification time, and
    flags from *src* to *dst*.  On Linux, :func:`copystat` also copies the
@@ -123,6 +124,10 @@ Directory and files operations
    *src* symbolic link, and writing the information to the
    *dst* symbolic link.
 
+    If the optional flag *preserve_security_context* is set, extended
+    attributes in the security namespace are copied as well. By default,
+    attributes like ``security.selinux`` are ignored.
+
    .. note::
 
       Not all platforms provide the ability to examine and
@@ -155,6 +160,10 @@ Directory and files operations
    .. versionchanged:: 3.3
       Added *follow_symlinks* argument and support for Linux extended attributes.
 
+   .. versionchanged:: 3.10
+      Added *preserve_security_context* argument.
+      Skip xattrs in the security namespace by default.
+
 .. function:: copy(src, dst, *, follow_symlinks=True)
 
    Copies the file *src* to the file or directory *dst*.  *src* and *dst*
@@ -186,7 +195,8 @@ Directory and files operations
       copy the file more efficiently. See
       :ref:`shutil-platform-dependent-efficient-copy-operations` section.
 
-.. function:: copy2(src, dst, *, follow_symlinks=True)
+.. function:: copy2(src, dst, *, follow_symlinks=True, \
+                    preserve_security_context=False)
 
    Identical to :func:`~shutil.copy` except that :func:`copy2`
    also attempts to preserve file metadata.
@@ -218,6 +228,10 @@ Directory and files operations
       copy the file more efficiently. See
       :ref:`shutil-platform-dependent-efficient-copy-operations` section.
 
+   .. versionchanged:: 3.10
+      Added *preserve_security_context* argument.
+      Skip xattrs in the security namespace by default.
+
 .. function:: ignore_patterns(\*patterns)
 
    This factory function creates a function that can be used as a callable for
@@ -227,7 +241,7 @@ Directory and files operations
 
 .. function:: copytree(src, dst, symlinks=False, ignore=None, \
               copy_function=copy2, ignore_dangling_symlinks=False, \
-              dirs_exist_ok=False)
+              dirs_exist_ok=False, preserve_security_context=False)
 
    Recursively copy an entire directory tree rooted at *src* to a directory
    named *dst* and return the destination directory. *dirs_exist_ok* dictates
@@ -286,6 +300,10 @@ Directory and files operations
    .. versionadded:: 3.8
       The *dirs_exist_ok* parameter.
 
+   .. versionchanged:: 3.10
+      Added *preserve_security_context* argument.
+      Skip xattrs in the security namespace by default.
+
 .. function:: rmtree(path, ignore_errors=False, onerror=None)
 
    .. index:: single: directory; deleting
@@ -334,7 +352,8 @@ Directory and files operations
       .. versionadded:: 3.3
 
 
-.. function:: move(src, dst, copy_function=copy2)
+.. function:: move(src, dst, copy_function=copy2, \
+                   preserve_security_context=False)
 
    Recursively move a file or directory (*src*) to another location (*dst*)
    and return the destination.
@@ -374,6 +393,10 @@ Directory and files operations
    .. versionchanged:: 3.9
       Accepts a :term:`path-like object` for both *src* and *dst*.
 
+   .. versionchanged:: 3.10
+      Added *preserve_security_context* argument.
+      Skip xattrs in the security namespace by default.
+
 .. function:: disk_usage(path)
 
    Return disk usage statistics about the given path as a :term:`named tuple`
diff --git a/Lib/shutil.py b/Lib/shutil.py
@@ -308,21 +308,25 @@ def copymode(src, dst, *, follow_symlinks=True):
     chmod_func(dst, stat.S_IMODE(st.st_mode))
 
 if hasattr(os, 'listxattr'):
-    def _copyxattr(src, dst, *, follow_symlinks=True):
+    def _copyxattr(src, dst, *, follow_symlinks=True, names_filter=None):
         """Copy extended filesystem attributes from `src` to `dst`.
 
         Overwrite existing attributes.
 
         If `follow_symlinks` is false, symlinks won't be followed.
 
+        `names_filter` is a callback to filter xattr names.
         """
-
         try:
             names = os.listxattr(src, follow_symlinks=follow_symlinks)
         except OSError as e:
             if e.errno not in (errno.ENOTSUP, errno.ENODATA, errno.EINVAL):
                 raise
             return
+        if not names:
+            return
+        if names_filter is not None:
+            names = names_filter(names)
         for name in names:
             try:
                 value = os.getxattr(src, name, follow_symlinks=follow_symlinks)
@@ -335,7 +339,8 @@ def _copyxattr(src, dst, *, follow_symlinks=True):
     def _copyxattr(*args, **kwargs):
         pass
 
-def copystat(src, dst, *, follow_symlinks=True):
+def copystat(src, dst, *, follow_symlinks=True,
+             preserve_security_context=False):
     """Copy file metadata
 
     Copy the permission bits, last access time, last modification time, and
@@ -346,6 +351,14 @@ def copystat(src, dst, *, follow_symlinks=True):
 
     If the optional flag `follow_symlinks` is not set, symlinks aren't
     followed if and only if both `src` and `dst` are symlinks.
+
+    If the optional flag `preserve_security_context` is set, extended
+    attributes in the security namespace are copied as well. By default,
+    attributes like `security.selinux` are ignored.
+
+    `preserve_security_context=False` is equivalent to
+    `cp -p --preserve=xattr`. `preserve_security_context=True` behaves like
+    `cp -p --preserve=xattr,context`.
     """
     sys.audit("shutil.copystat", src, dst)
 
@@ -367,6 +380,16 @@ def lookup(name):
                 return fn
             return _nop
 
+    # don't copy security xattr by default
+    # see coreutils src/copy.c:check_selinux_attr()
+    if not preserve_security_context:
+        def names_filter(names):
+            return [
+                name for name in names if not name.startswith("security.")
+            ]
+    else:
+        names_filter = None
+
     if isinstance(src, os.DirEntry):
         st = src.stat(follow_symlinks=follow)
     else:
@@ -376,7 +399,7 @@ def lookup(name):
         follow_symlinks=follow)
     # We must copy extended attributes before the file is (potentially)
     # chmod()'ed read-only, otherwise setxattr() will error with -EACCES.
-    _copyxattr(src, dst, follow_symlinks=follow)
+    _copyxattr(src, dst, follow_symlinks=follow, names_filter=names_filter)
     try:
         lookup("chmod")(dst, mode, follow_symlinks=follow)
     except NotImplementedError:
@@ -419,7 +442,7 @@ def copy(src, dst, *, follow_symlinks=True):
     copymode(src, dst, follow_symlinks=follow_symlinks)
     return dst
 
-def copy2(src, dst, *, follow_symlinks=True):
+def copy2(src, dst, *, follow_symlinks=True, preserve_security_context=False):
     """Copy data and metadata. Return the file's destination.
 
     Metadata is copied with copystat(). Please see the copystat function
@@ -429,11 +452,16 @@ def copy2(src, dst, *, follow_symlinks=True):
 
     If follow_symlinks is false, symlinks won't be followed. This
     resembles GNU's "cp -P src dst".
+
+    If the optional flag `preserve_security_context` is set, extended
+    attributes in the security namespace are copied as well. By default,
+    attributes like `security.selinux` are ignored.
     """
     if os.path.isdir(dst):
         dst = os.path.join(dst, os.path.basename(src))
     copyfile(src, dst, follow_symlinks=follow_symlinks)
-    copystat(src, dst, follow_symlinks=follow_symlinks)
+    copystat(src, dst, follow_symlinks=follow_symlinks,
+             preserve_security_context=preserve_security_context)
     return dst
 
 def ignore_patterns(*patterns):
@@ -449,7 +477,8 @@ def _ignore_patterns(path, names):
     return _ignore_patterns
 
 def _copytree(entries, src, dst, symlinks, ignore, copy_function,
-              ignore_dangling_symlinks, dirs_exist_ok=False):
+              ignore_dangling_symlinks, dirs_exist_ok=False,
+              preserve_security_context=False):
     if ignore is not None:
         ignored_names = ignore(os.fspath(src), [x.name for x in entries])
     else:
@@ -480,7 +509,8 @@ def _copytree(entries, src, dst, symlinks, ignore, copy_function,
                     # code with a custom `copy_function` may rely on copytree
                     # doing the right thing.
                     os.symlink(linkto, dstname)
-                    copystat(srcobj, dstname, follow_symlinks=not symlinks)
+                    copystat(srcobj, dstname, follow_symlinks=not symlinks,
+                             preserve_security_context=preserve_security_context)
                 else:
                     # ignore dangling symlink if the flag is on
                     if not os.path.exists(linkto) and ignore_dangling_symlinks:
@@ -493,7 +523,8 @@ def _copytree(entries, src, dst, symlinks, ignore, copy_function,
                         copy_function(srcobj, dstname)
             elif srcentry.is_dir():
                 copytree(srcobj, dstname, symlinks, ignore, copy_function,
-                         dirs_exist_ok=dirs_exist_ok)
+                         dirs_exist_ok=dirs_exist_ok,
+                         preserve_security_context=preserve_security_context)
             else:
                 # Will raise a SpecialFileError for unsupported file types
                 copy_function(srcobj, dstname)
@@ -504,7 +535,7 @@ def _copytree(entries, src, dst, symlinks, ignore, copy_function,
         except OSError as why:
             errors.append((srcname, dstname, str(why)))
     try:
-        copystat(src, dst)
+        copystat(src, dst, preserve_security_context=preserve_security_context)
     except OSError as why:
         # Copying file access times may fail on Windows
         if getattr(why, 'winerror', None) is None:
@@ -514,7 +545,8 @@ def _copytree(entries, src, dst, symlinks, ignore, copy_function,
     return dst
 
 def copytree(src, dst, symlinks=False, ignore=None, copy_function=copy2,
-             ignore_dangling_symlinks=False, dirs_exist_ok=False):
+             ignore_dangling_symlinks=False, dirs_exist_ok=False,
+             preserve_security_context=False):
     """Recursively copy a directory tree and return the destination directory.
 
     dirs_exist_ok dictates whether to raise an exception in case dst or any
@@ -550,14 +582,18 @@ def copytree(src, dst, symlinks=False, ignore=None, copy_function=copy2,
     destination path as arguments. By default, copy2() is used, but any
     function that supports the same signature (like copy()) can be used.
 
+    If the optional flag `preserve_security_context` is set, extended
+    attributes in the security namespace are copied as well. By default,
+    attributes like `security.selinux` are ignored.
     """
     sys.audit("shutil.copytree", src, dst)
     with os.scandir(src) as itr:
         entries = list(itr)
     return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
                      ignore=ignore, copy_function=copy_function,
                      ignore_dangling_symlinks=ignore_dangling_symlinks,
-                     dirs_exist_ok=dirs_exist_ok)
+                     dirs_exist_ok=dirs_exist_ok,
+                     preserve_security_context=preserve_security_context)
 
 if hasattr(os.stat_result, 'st_file_attributes'):
     # Special handling for directory junctions to make them behave like
@@ -761,7 +797,7 @@ def _basename(path):
     sep = os.path.sep + (os.path.altsep or '')
     return os.path.basename(path.rstrip(sep))
 
-def move(src, dst, copy_function=copy2):
+def move(src, dst, copy_function=copy2, preserve_security_context=False):
     """Recursively move a file or directory to another location. This is
     similar to the Unix "mv" command. Return the file or directory's
     destination.
@@ -814,10 +850,17 @@ def move(src, dst, copy_function=copy2):
                 raise Error("Cannot move a directory '%s' into itself"
                             " '%s'." % (src, dst))
             copytree(src, real_dst, copy_function=copy_function,
-                     symlinks=True)
+                     symlinks=True,
+                     preserve_security_context=preserve_security_context)
             rmtree(src)
         else:
-            copy_function(src, real_dst)
+            if preserve_security_context:
+                copy_function(
+                    src, real_dst,
+                    preserve_security_context=preserve_security_context
+                )
+            else:
+                copy_function(src, real_dst)
             os.unlink(src)
     return real_dst
 
diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py
@@ -1027,6 +1027,52 @@ def test_copyxattr_symlinks(self):
         shutil._copyxattr(src_link, dst, follow_symlinks=False)
         self.assertEqual(os.getxattr(dst, 'trusted.foo'), b'43')
 
+    @unittest.mock.patch('os.setxattr')
+    @unittest.mock.patch('os.getxattr')
+    @unittest.mock.patch('os.listxattr')
+    def test_copyxattr_preserve_security(
+            self, m_listxattr, m_getxattr, m_setxattr
+    ):
+        tmp_dir = self.mkdtemp()
+        src = os.path.join(tmp_dir, 'foo')
+        write_file(src, 'foo')
+        dst = os.path.join(tmp_dir, 'bar')
+        write_file(dst, 'bar')
+
+        m_listxattr.return_value = ["security.selinux", "user.python"]
+        m_getxattr.return_value = "value"
+
+        # by default do not copy xattr in security namespace
+        shutil.copystat(src, dst)
+        m_getxattr.assert_any_call(
+            src, "user.python", follow_symlinks=True
+        )
+        m_setxattr.assert_any_call(
+            dst, "user.python", "value", follow_symlinks=True
+        )
+        self.assertEqual(m_getxattr.call_count, 1)
+        self.assertEqual(m_setxattr.call_count, 1)
+
+        m_getxattr.reset_mock()
+        m_setxattr.reset_mock()
+
+        # preserve_security_context=True copies all attributes
+        shutil.copystat(src, dst, preserve_security_context=True)
+        m_getxattr.assert_any_call(
+            src, "user.python", follow_symlinks=True
+        )
+        m_setxattr.assert_any_call(
+            dst, "user.python", "value", follow_symlinks=True
+        )
+        m_getxattr.assert_any_call(
+            src, "security.selinux", follow_symlinks=True
+        )
+        m_setxattr.assert_any_call(
+            dst, "security.selinux", "value", follow_symlinks=True
+        )
+        self.assertEqual(m_getxattr.call_count, 2)
+        self.assertEqual(m_setxattr.call_count, 2)
+
     ### shutil.copy
 
     def _copy_file(self, method):
diff --git a/Misc/NEWS.d/next/Library/2020-12-09-15-58-26.bpo-38893.CCAmMb.rst b/Misc/NEWS.d/next/Library/2020-12-09-15-58-26.bpo-38893.CCAmMb.rst
@@ -0,0 +1,3 @@
+:mod:`shutil` functions no longer copy extended file attributes in the
+`security` namespace by default unless new *preserve_security_context*
+argument is given.

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	+:mod:`shutil` functions no longer copy extended file attributes in the
	`2`	+`security` namespace by default unless new preserve_security_context
	`3`	`+argument is given.`