svn commit: trunk/busybox/libbb
vda at busybox.net
vda at busybox.net
Thu Mar 15 13:33:37 UTC 2007
Author: vda
Date: 2007-03-15 06:33:37 -0700 (Thu, 15 Mar 2007)
New Revision: 18119
Log:
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.
Modified:
trunk/busybox/libbb/copy_file.c
Changeset:
Modified: trunk/busybox/libbb/copy_file.c
===================================================================
--- trunk/busybox/libbb/copy_file.c 2007-03-15 13:28:46 UTC (rev 18118)
+++ trunk/busybox/libbb/copy_file.c 2007-03-15 13:33:37 UTC (rev 18119)
@@ -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 @@
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 @@
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 @@
// (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 @@
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 @@
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 @@
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 @@
#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 @@
) {
// 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 @@
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;
More information about the busybox-cvs
mailing list