copy_file: comment out one condition which is always false.

Add comment explaining POSIX rules for cp - and why
these rules are dangerous. Provide conditionally compiled code
for both POSIX and safe behaviors, select safe for now.
Code shrunk by ~80 bytes.

diff --git a/libbb/copy_file.c b/libbb/copy_file.c
index 636fbdc1d2712315de464aceb3e76b67f9108b6d..58d657b02910f014ce41bc8a122602a1c06f95a4 100644 (file)
--- a/libbb/copy_file.c
+++ b/libbb/copy_file.c
@@ -11,13 +11,37 @@
 
 #include "libbb.h"
 
-static int retry_overwrite(const char *dest, int flags)
+// POSIX: if exists and -i, ask (w/o -i assume yes).
+// Then open w/o EXCL (yes, not unlink!).
+// If open still fails and -f, try unlink, then try open again.
+// Result: a mess:
+// If dest is a softlink, we overwrite softlink's destination!
+// (or fail, if it points to dir/nonexistent location/etc).
+// This is strange, but POSIX-correct.
+// coreutils cp has --remove-destination to override this...
+
+#define DO_POSIX_CP 0  /* 1 - POSIX behavior, 0 - safe behavior */
+
+
+static int ask_and_unlink(const char *dest, int flags)
 {
+       // If !DO_POSIX_CP, act as if -f is always in effect - we don't want
+       // "'file' exists" msg, we want unlink to be done (silently unless -i
+       // is also in effect).
+       // This prevents safe way from asking more questions than POSIX does.
+#if DO_POSIX_CP
        if (!(flags & (FILEUTILS_FORCE|FILEUTILS_INTERACTIVE))) {
                fprintf(stderr, "'%s' exists\n", dest);
                return -1;
        }
+#endif
+
+       // TODO: maybe we should do it only if ctty is present?
        if (flags & FILEUTILS_INTERACTIVE) {
+               // We would not do POSIX insanity. -i asks,
+               // then _unlinks_ the offender. Presto.
+               // (No opening without O_EXCL, no unlinks only if -f)
+               // Or else we will end up having 3 open()s!
                fprintf(stderr, "%s: overwrite '%s'? ", applet_name, dest);
                if (!bb_ask_confirmation())
                        return 0; // not allowed to overwrite
@@ -29,6 +53,11 @@ static int retry_overwrite(const char *dest, int flags)
        return 1; // ok (to try again)
 }
 
+/* Return:
+ * -1 error, copy not made
+ *  0 copy is made or user answered "no" in interactive mode
+ *    (failures to preserve mode/owner/times are not reported in exit code)
+ */
 int copy_file(const char *source, const char *dest, int flags)
 {
        struct stat source_stat;
@@ -72,13 +101,11 @@ int copy_file(const char *source, const char *dest, int flags)
                                freecon(con);
                                return -1;
                        }
+               } else if (errno == ENOTSUP || errno == ENODATA) {
+                       setfscreatecon_or_die(NULL);
                } else {
-                       if (errno == ENOTSUP || errno == ENODATA) {
-                               setfscreatecon_or_die(NULL);
-                       } else {
-                               bb_perror_msg("cannot lgetfilecon %s", source);
-                               return -1;
-                       }
+                       bb_perror_msg("cannot lgetfilecon %s", source);
+                       return -1;
                }
        }
 #endif
@@ -153,7 +180,7 @@ int copy_file(const char *source, const char *dest, int flags)
                // (but realpath returns NULL on dangling symlinks...)
                lf = (flags & FILEUTILS_MAKE_SOFTLINK) ? symlink : link;
                if (lf(source, dest) < 0) {
-                       ovr = retry_overwrite(dest, flags);
+                       ovr = ask_and_unlink(dest, flags);
                        if (ovr <= 0)
                                return ovr;
                        if (lf(source, dest) < 0) {
@@ -164,8 +191,8 @@ int copy_file(const char *source, const char *dest, int flags)
                return 0;
 
        } else if (S_ISREG(source_stat.st_mode)
-       // Huh? DEREF uses stat, which never returns links IIRC...
-        || (FLAGS_DEREF && S_ISLNK(source_stat.st_mode))
+        /* Huh? DEREF uses stat, which never returns links! */
+        /* || (FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) */
        ) {
                int src_fd;
                int dst_fd;
@@ -176,7 +203,7 @@ int copy_file(const char *source, const char *dest, int flags)
                                link_name = is_in_ino_dev_hashtable(&source_stat);
                                if (link_name) {
                                        if (link(link_name, dest) < 0) {
-                                               ovr = retry_overwrite(dest, flags);
+                                               ovr = ask_and_unlink(dest, flags);
                                                if (ovr <= 0)
                                                        return ovr;
                                                if (link(link_name, dest) < 0) {
@@ -196,27 +223,21 @@ int copy_file(const char *source, const char *dest, int flags)
                        return -1;
                }
 
-               // POSIX: if exists and -i, ask (w/o -i assume yes).
-               // Then open w/o EXCL.
-               // If open still fails and -f, try unlink, then try open again.
-               // Result: a mess:
-               // If dest is a softlink, we overwrite softlink's destination!
-               // (or fail, if it points to dir/nonexistent location/etc).
-               // This is strange, but POSIX-correct.
-               // coreutils cp has --remove-destination to override this...
+#if DO_POSIX_CP  /* POSIX way (a security problem versus symlink attacks!): */
                dst_fd = open(dest, (flags & FILEUTILS_INTERACTIVE)
-                               ? O_WRONLY|O_CREAT|O_TRUNC|O_EXCL
+                               ? O_WRONLY|O_CREAT|O_EXCL
                                : O_WRONLY|O_CREAT|O_TRUNC, source_stat.st_mode);
+#else  /* safe way: */
+               dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, source_stat.st_mode);
+#endif
                if (dst_fd == -1) {
-                       // We would not do POSIX insanity. -i asks,
-                       // then _unlinks_ the offender. Presto.
-                       // Or else we will end up having 3 open()s!
-                       ovr = retry_overwrite(dest, flags);
+                       ovr = ask_and_unlink(dest, flags);
                        if (ovr <= 0) {
                                close(src_fd);
                                return ovr;
                        }
-                       dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, source_stat.st_mode);
+                       /* It shouldn't exist. If it exists, do not open (symlink attack?) */
+                       dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, source_stat.st_mode);
                        if (dst_fd == -1) {
                                bb_perror_msg("cannot open '%s'", dest);
                                close(src_fd);
@@ -227,7 +248,8 @@ int copy_file(const char *source, const char *dest, int flags)
 #if ENABLE_SELINUX
                if (((flags & FILEUTILS_PRESERVE_SECURITY_CONTEXT)
                    || (flags & FILEUTILS_SET_SECURITY_CONTEXT))
-                && is_selinux_enabled() > 0) {
+                && is_selinux_enabled() > 0
+               ) {
                        security_context_t con;  
                        if (getfscreatecon(&con) == -1) {
                                bb_perror_msg("getfscreatecon");
@@ -260,7 +282,7 @@ int copy_file(const char *source, const char *dest, int flags)
        ) {
                // We are lazy here, a bit lax with races...
                if (dest_exists) {
-                       ovr = retry_overwrite(dest, flags);
+                       ovr = ask_and_unlink(dest, flags);
                        if (ovr <= 0)
                                return ovr;
                }
@@ -273,7 +295,7 @@ int copy_file(const char *source, const char *dest, int flags)
                        char *lpath;
 
                        lpath = xmalloc_readlink_or_warn(source);
-                       if (symlink(lpath, dest) < 0) {
+                       if (lpath && symlink(lpath, dest) < 0) {
                                bb_perror_msg("cannot create symlink '%s'", dest);
                                free(lpath);
                                return -1;