transfer.c: Prevent direct mutation of /home directory.

Users should not be able to mutate the /home directory
itself (e.g. create directories or files in it,
delete the directory itself, etc.), so add explicit
logic to ensure this.

bbs/transfer.c

@@ -76,6 +76,8 @@ size_t bbs_transfer_max_upload_size(void)
 	return (size_t) max_upload_size;
 }
 
+#define PATH_STARTS_WITH(p, s) (!strncmp(p, s, STRLEN(s)))
+
 int bbs_transfer_operation_allowed(struct bbs_node *node, int operation, const char *diskpath)
 {
 	int required_priv;
@@ -93,6 +95,18 @@ int bbs_transfer_operation_allowed(struct bbs_node *node, int operation, const c
 			bbs_debug(6, "Operation implicitly authorized since it's in the user's home directory\n");
 			return 1;
 		}
+		len = snprintf(homedir, sizeof(homedir), "%s/home", bbs_transfer_rootdir());
+		if ((operation == TRANSFER_UPLOAD || operation == TRANSFER_NEWDIR || operation == TRANSFER_DESTRUCTIVE) && !strncmp(diskpath, homedir, (size_t) len)) {
+			/* If not inside the user's home directory, reject the request.
+			 * Logic elsewhere prevents accesing other users' home directories,
+			 * but this is needed here to ensure that:
+			 * - Users cannot delete other home directories
+			 * - Users cannot delete the root /home/ directory (and everyone's home directories!)
+			 * - Users cannot create directories inside the root /home/ directory
+			 */
+			bbs_debug(2, "Operation rejected for '%s', since it modifies /home (root home directory)\n", diskpath);
+			return 0;
+		}
 	}
 
 	required_priv = privs[operation];
@@ -169,7 +183,9 @@ static int recursive_copy(const char *srcfiles, const char *dest)
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wdiscarded-qualifiers"
 #pragma GCC diagnostic ignored "-Wcast-qual"
-	char *const argv[] = { "cp", "-r", (char*) srcfiles, (char*) dest, NULL };
+	/* no clobber, just in case it already existed (which it shouldn't, but just supposing),
+	 * we don't want to overwrite all the user's existing files. */
+	char *const argv[] = { "cp", "-r", "-n", (char*) srcfiles, (char*) dest, NULL };
 #pragma GCC diagnostic pop
 	return bbs_execvp(NULL, argv[0], argv);
 }
@@ -181,7 +197,7 @@ int bbs_transfer_home_dir(unsigned int userid, char *buf, size_t len)
 		return -1;
 	}
 	snprintf(buf, len, "%s/home/%d", rootdir, userid);
-	if (eaccess(buf, X_OK)) {
+	if (eaccess(buf, R_OK | X_OK)) {
 		char srcfiles[258];
 		if (bbs_ensure_directory_exists(buf)) { /* Autocreate directory structure */
 			return -1;
@@ -265,8 +281,6 @@ int bbs_transfer_home_config_file(unsigned int userid, const char *name, char *b
 	return !bbs_file_exists(buf);
 }
 
-#define PATH_STARTS_WITH(p, s) (!strncmp(p, s, STRLEN(s)))
-
 int bbs_transfer_get_user_path(struct bbs_node *node, const char *diskpath, char *buf, size_t len)
 {
 	const char *userpath = diskpath + rootlen;

include/transfer.h

@@ -16,6 +16,7 @@
 /* Forward declarations */
 struct bbs_node;
 
+/* These cannot be bits, since they are used as array indices */
 #define TRANSFER_ACCESS 0
 #define TRANSFER_DOWNLOAD 1
 #define TRANSFER_UPLOAD 2

nets/net_ssh.c

@@ -1296,17 +1296,31 @@ static void sftp_server_free(sftp_session sftp)
 }
 #endif
 
+#define CANONICALIZE_PATHS() \
+	/* Clients are supposed to call REALPATH so that the server can canonicalize the actual path on disk.
+	 * We just assume that's been done afterwards... */ \
+	if (!realpath(mypath, buf)) { /* returns NULL on failure */ \
+		bbs_debug(5, "Path '%s' not found: %s\n", mypath, strerror(errno)); \
+		handle_errno(msg); \
+		break; \
+	} \
+	bbs_debug(3, "realpath(%s) -> %s\n", mypath, buf); \
+	safe_strncpy(mypath, buf, sizeof(mypath)); /* Replace mypath so we can use either */ \
+	bbs_transfer_get_user_path(node, buf, userpath, sizeof(userpath)); \
+
 #define SFTP_MAKE_PATH() \
 	if (bbs_transfer_set_disk_path_absolute(node, msg->filename, mypath, sizeof(mypath))) { \
 		handle_errno(msg); \
 		break; \
-	}
+	} \
+	CANONICALIZE_PATHS();
 
 #define SFTP_MAKE_PATH_NOCHECK() \
 	if (bbs_transfer_set_disk_path_absolute_nocheck(node, msg->filename, mypath, sizeof(mypath))) { \
 		handle_errno(msg); \
 		break; \
-	}
+	} \
+	CANONICALIZE_PATHS();
 
 static int do_sftp(struct bbs_node *node, ssh_session session, ssh_channel channel)
 {
@@ -1357,16 +1371,7 @@ static int do_sftp(struct bbs_node *node, ssh_session session, ssh_channel chann
 		switch (msg->type) {
 			case SFTP_REALPATH:
 				SFTP_MAKE_PATH();
-				/* Clients are supposed to call REALPATH so that the server can canonicalize the actual path on disk.
-				 * We just assume that's been done afterwards... */
-				if (!realpath(mypath, buf)) { /* returns NULL on failure */
-					bbs_debug(5, "Path '%s' not found: %s\n", mypath, strerror(errno));
-					handle_errno(msg);
-				} else {
-					bbs_debug(3, "realpath(%s) -> %s\n", mypath, buf);
-					bbs_transfer_get_user_path(node, buf, userpath, sizeof(userpath));
-					sftp_reply_name(msg, userpath, NULL); /* Skip root dir */
-				}
+				sftp_reply_name(msg, userpath, NULL); /* Skip root dir */
 				break;
 			case SFTP_OPENDIR:
 				SFTP_MAKE_PATH();