Subject: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

From: Pádraig Brady <[email-masked]>
Date: Thu, 16 Feb 2023 21:32:01 +0000

On 15/02/2023 10:56, Paul Eggert wrote:
> Attached is an updated proposal for the fclonefileat patch.
> 
> CVE-2021-30995 confirmed my suspicion that coreutils 9.1 and the current
> bleeding-edge coreutils on Savannah both have an exploitable security
> bug on macOS. Although I hope this patch fixes the bug, it could use
> more pairs of eyes, given that Apple had so many problems fixing its own
> security bug involving fclonefileat, and given that the coreutils code
> has so many flags and conditions.

The logic looks good to me, and all tests pass here on macOS 13.2

I see the added `if (! (cloned_mode & ~desired_mode))` restriction I presume
for security reasons, so that we only _add_ permission bits to those copied by fclonefileat(),
and never create an overly accessible file and then remove permission bits.
That wasn't obvious to me at least so I'd suggest adding comments from the diff below.

For the record this restriction will mean we can't reflink in some cases.
For example the following would fail with ENOSUP on macOS:

   touch file;
   chmod g+w file
   umask 0022
   cp --reflink=always file file.copy

Using -p in the above cp command would work though.

thanks,
Pádraig.

diff --git a/src/copy.c b/src/copy.c
index c5f2cc186..7c866e4fb 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1284,6 +1284,7 @@ copy_reg (char const *src_name, char const *dst_name,
                 : x->set_mode ? x->mode
                 : ((x->explicit_no_preserve_mode ? MODE_RW_UGO : dst_mode)
                    & ~ cached_umask ()));
+          /* If fclonefileat() would not set undesired extra permissions.  */
            if (! (cloned_mode & ~desired_mode))
              {
                int fc_flags
@@ -1318,7 +1319,9 @@ copy_reg (char const *src_name, char const *dst_name,
                          }
                      }

+                  /* any desired permissions that weren't cloned.  */
                    extra_permissions = desired_mode & ~cloned_mode;
+
                    if (!extra_permissions
                        && (!x->preserve_mode || (fc_flags & CLONE_ACL)
                            || !fd_has_acl (source_desc)))