[PATCH] sftp: preserve permissions of existing local directories

[Muhammad Bilal]

download_dir_internal() unconditionally applied the remote directory mode with chmod(2), including when the local destination directory already existed and -p was not requested. A remote SFTP server could therefore change the permissions of an existing local directory during a recursive download.
Track whether mkdir(2) created the destination and only apply the remote mode for newly-created directories or when permission preservation was requested. This matches the existing guards in upload_dir_internal() and crossload_dir_internal().

Add a regression test that checks recursive downloads do not change the mode of an existing local directory.


Verified with:
  make -s sftp sftp-server

  TEST_SSH_SFTP=./sftp TEST_SSH_SFTPSERVER=./sftp-server \
    sh regress/test-exec.sh ./regress ./regress/sftp-cmds.sh


The regression passes with the fix and fails against the unmodified source.
---
 regress/sftp-cmds.sh | 13 +++++++++++--
 sftp-client.c        | 17 ++++++++++-------
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/regress/sftp-cmds.sh b/regress/sftp-cmds.sh
index a03959d60..94b886b1b 100644
--- a/regress/sftp-cmds.sh
+++ b/regress/sftp-cmds.sh
@@ -161,6 +161,17 @@ printf "cd ${COPY}.dd/b\n lcd ${COPY}.dd2\n get -R ..\n" | sftpserver || \
 	fail "get failed"
 diff ${DIFFOPT} ${COPY}.dd ${COPY}.dd2 || fail "corrupted copy"
 
+forest
+chmod 0555 ${COPY}.dd
+mkdir ${COPY}.dd2
+chmod 0700 ${COPY}.dd2
+verbose "$tid: get recursive into existing directory preserves permissions"
+printf "get -R ${COPY}.dd ${COPY}.dd2\n" | sftpserver || \
+	fail "get failed"
+test "`(stat -c %a ${COPY}.dd2 2>/dev/null || stat -f %Lp ${COPY}.dd2)`" = \
+	700 || fail "get changed permissions of existing directory"
+chmod -R u+w ${COPY}.dd ${COPY}.dd2
+
 rm -f ${COPY}
 verbose "$tid: put"
 echo "put $DATA $COPY" | sftpserver || fail "put failed"
@@ -273,5 +284,3 @@ echo "lchdir ${COPY}.dd" | sftpserver || fail "lchdir failed"
 
 rm -rf ${COPY} ${COPY}.1 ${COPY}.2 ${COPY}.dd ${COPY}.dd2
 rm -rf ${QUOTECOPY} "$SPACECOPY" "$GLOBMETACOPY"
-
-
diff --git a/sftp-client.c b/sftp-client.c
index 1f031128e..16885b5d0 100644
--- a/sftp-client.c
+++ b/sftp-client.c
@@ -1882,7 +1882,7 @@ download_dir_internal(struct sftp_conn *conn, const char *src, const char *dst,
     int depth, Attrib *dirattrib, int preserve_flag, int print_flag,
     int resume_flag, int fsync_flag, int follow_link_flag, int inplace_flag)
 {
-	int i, ret = 0;
+	int i, ret = 0, created = 0;
 	SFTP_DIRENT **dir_entries;
 	char *filename, *new_src = NULL, *new_dst = NULL;
 	mode_t mode = 0777, tmpmode = mode;
@@ -1917,10 +1917,13 @@ download_dir_internal(struct sftp_conn *conn, const char *src, const char *dst,
 		    "did not send permissions", dst);
 	}
 
-	if (mkdir(dst, tmpmode) == -1 && errno != EEXIST) {
-		error("mkdir %s: %s", dst, strerror(errno));
-		return -1;
-	}
+	if (mkdir(dst, tmpmode) == -1) {
+		if (errno != EEXIST) {
+			error("mkdir %s: %s", dst, strerror(errno));
+			return -1;
+		}
+	} else
+		created = 1;
 
 	if (sftp_readdir(conn, src, &dir_entries) == -1) {
 		error("remote readdir \"%s\" failed", src);
@@ -1989,7 +1992,8 @@ download_dir_internal(struct sftp_conn *conn, const char *src, const char *dst,
 			    "\"%s\"", dst);
 	}
 
-	if (mode != tmpmode && chmod(dst, mode) == -1)
+	if ((created || preserve_flag) && mode != tmpmode &&
+	    chmod(dst, mode) == -1)
 		error("local chmod directory \"%s\": %s", dst,
 		    strerror(errno));
 
@@ -3010,4 +3014,3 @@ sftp_globpath_is_dir(const char *pathname)
 
 	return l > 0 && pathname[l - 1] == '/';
 }
-