54d14ca1a

Denis Vlasenko

2007-03-15

FAILED Duration: 16s

Subject

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.

Binary

No binary (build failed)

Architecture & Compiler Flags

Architecture: x86_64-linux-gnu Compiler: gcc (Ubuntu 11.4.0-1ubuntu1~22.04.3) 11.4.0

—

Build Log

(log yok)

Commit Diff GitHub →

libbb/copy_file.c +50 -28

libbb/copy_file.c+50 -28

@@ -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;