[PATCH] add sftp-server option to force temp files

[Nathan Wagner]

The following patch will add a -T option to sftp-server.c that forces
use of a temp file for uploads to the server.  It takes an argument that
has 'XXXXXX' added to the end and used as a template string for
mkstemp(3).

The motivation for the patch is that I have a scanner that will upload
to an sftp server, but it doesn't use a temp file and then do a rename
into place.  I want to then have a program on the server process the
incoming files.  However, since the files are uploaded in place, I don't
have any way to know if the file transfer is complete or just paused.  I
also don't have any control over the sftp client the scanner uses.

So, what this patch does is allow an option to use a temp file and then
rename into the requested place at file close.  Then the file never is
partial.  This makes it simpler and more robust to write a file
processing program on the server side.

I have a slightly earlier version of this patch applied to my sshd
program and it seems to work fine.  I also ran a 'make tests' and
all tests passed.

Some issues:

The temp file and the final path have to be on the same filesystem.  The
sysadmin will have to ensure this.

The temp file, if relative, is relative to whereever the servers cwd is.
I could process the target filename and make a relative temp file
relative to the target file directory.

There could be some race conditions as to the directory the file ends up
in if there are renames going on during the upload.  I don't know that
this matters.  We could use openat() and similar functions, but we'd
have to keep a target directory file handle around.

The handle is slightly larger as we need to keep the tempfile name
around to be able to the rename into place.  There is an additional
malloc/free involved.

I malloc the temp file string template from the command line options and
append 'XXXXXX'.  This doesn't get freed anywhere, though since it lasts
and is fixed for the time of the process, I don't see anywhere to free
it.  Using atexit() seems like overkill.  If this code were turned into
a library, there might need to be some sort of teardown, but that may be
true of a number of file scoped variables.

mkstemp() opens the file in a restricted mode.  I immediately do
an fchmod() to set the mode to the same as it would have been opened
with via open().  In theory this opens up the file before it's
complete, but this was true already, so the behavior isn't worse.
There will be a slight period of time where the file is open
with different permissions than it would have been using open
directly.

There may be umask implications by using fchmod.  Specifically
I don't think that the umask will be applied to the mode for fchmod,
whereas it would have been with open.  I may need to either apply
the umask directly or otherwise ensure the mode is correct.

I haven't updated the sftp-server man page.

---
 sftp-server.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/sftp-server.c b/sftp-server.c
index d4c6a3b4..1c5dd8d9 100644
--- a/sftp-server.c
+++ b/sftp-server.c
@@ -80,6 +80,9 @@ static int init_done;
 /* Disable writes */
 static int readonly;
 
+/* force temp file pattern */
+static char *forcetemp;
+
 /* Requests that are allowed/denied */
 static char *request_allowlist, *request_denylist;
 
@@ -302,7 +305,7 @@ struct Handle {
 	DIR *dirp;
 	int fd;
 	int flags;
-	char *name;
+	char *name, *tmpname;
 	u_int64_t bytes_read, bytes_write;
 	int next_unused;
 };
@@ -345,6 +348,7 @@ handle_new(int use, const char *name, int fd, int flags, DIR *dirp)
 	handles[i].fd = fd;
 	handles[i].flags = flags;
 	handles[i].name = xstrdup(name);
+	handles[i].tmpname = 0;
 	handles[i].bytes_read = handles[i].bytes_write = 0;
 
 	return i;
@@ -451,11 +455,17 @@ handle_close(int handle)
 
 	if (handle_is_ok(handle, HANDLE_FILE)) {
 		ret = close(handles[handle].fd);
+		if (handles[handle].tmpname) {
+			rename(handles[handle].tmpname, handles[handle].name);
+		}
 		free(handles[handle].name);
+		free(handles[handle].tmpname);
+		handles[handle].tmpname = 0;
 		handle_unused(handle);
 	} else if (handle_is_ok(handle, HANDLE_DIR)) {
 		ret = closedir(handles[handle].dirp);
 		free(handles[handle].name);
+		free(handles[handle].tmpname);
 		handle_unused(handle);
 	} else {
 		errno = ENOENT;
@@ -730,7 +740,7 @@ process_open(u_int32_t id)
 {
 	u_int32_t pflags;
 	Attrib a;
-	char *name;
+	char *name, *tmpname = 0;
 	int r, handle, fd, flags, mode, status = SSH2_FX_FAILURE;
 
 	if ((r = sshbuf_get_cstring(iqueue, &name, NULL)) != 0 ||
@@ -749,7 +759,14 @@ process_open(u_int32_t id)
 		verbose("Refusing open request in read-only mode");
 		status = SSH2_FX_PERMISSION_DENIED;
 	} else {
-		fd = open(name, flags, mode);
+		if (forcetemp) {
+			tmpname = xstrdup(forcetemp);
+			fd = mkstemp(tmpname);
+			fchmod(fd, mode);
+		} else {
+			fd = open(name, flags, mode);
+		}
+
 		if (fd == -1) {
 			status = errno_to_portable(errno);
 		} else {
@@ -757,6 +774,7 @@ process_open(u_int32_t id)
 			if (handle < 0) {
 				close(fd);
 			} else {
+				handles[handle].tmpname = tmpname;
 				send_handle(id, handle);
 				status = SSH2_FX_OK;
 			}
@@ -1711,7 +1729,8 @@ sftp_server_usage(void)
 	    "usage: %s [-ehR] [-d start_directory] [-f log_facility] "
 	    "[-l log_level]\n\t[-P denied_requests] "
 	    "[-p allowed_requests] [-u umask]\n"
-	    "       %s -Q protocol_feature\n",
+	    "       %s -Q protocol_feature\n"
+	    "[-T tmpname]\n",
 	    __progname, __progname);
 	exit(1);
 }
@@ -1734,7 +1753,7 @@ sftp_server_main(int argc, char **argv, struct passwd *user_pw)
 	pw = pwcopy(user_pw);
 
 	while (!skipargs && (ch = getopt(argc, argv,
-	    "d:f:l:P:p:Q:u:cehR")) != -1) {
+	    "d:f:l:P:p:Q:u:cehRT:")) != -1) {
 		switch (ch) {
 		case 'Q':
 			if (strcasecmp(optarg, "requests") != 0) {
@@ -1750,6 +1769,10 @@ sftp_server_main(int argc, char **argv, struct passwd *user_pw)
 		case 'R':
 			readonly = 1;
 			break;
+		case 'T':
+			forcetemp = xmalloc(strlen(optarg) + 7);
+			sprintf(forcetemp, "%sXXXXXX", optarg);
+			break;
 		case 'c':
 			/*
 			 * Ignore all arguments if we are invoked as a

-- 
nw