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
Nico Kadel-Garcia
2022-Feb-20 18:30 UTC
[PATCH] add sftp-server option to force temp files
On Sun, Feb 20, 2022 at 11:08 AM Nathan Wagner <nw at hydaspes.if.org> wrote:> > 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).Wouldn't rsync over SSH be better for this sort of feature aggregation? The potential chroot caged setups for sftp may have their uses, but the more complex you make this sort of behavior, the more vulnerable you become to alarming failures such as leaving behind temporary file debris as the artifact of a failed transfer, especially in edge cases like transferring large files and the transmission being interrupted or running out of disk space.
On Sun, 20 Feb 2022, Nathan Wagner wrote:> 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).IMO sftp-server is the wrong place to do this - as you probably observed while implementing this, the SFTP protocol is agnostic to the concept of uploads, instead operating more at the level of the Unix syscall level (i.e. exposing read/write/stat/open/close operations). Adding temporary file support to the server breaks this model and will break any use of sftp that doesn't adhere to the expected sequence of operations. E.g.> - fd = open(name, flags, mode); > + if (forcetemp) { > + tmpname = xstrdup(forcetemp); > + fd = mkstemp(tmpname); > + fchmod(fd, mode); > + } else { > + fd = open(name, flags, mode); > + }will AFAIK break downloads of files, since the interposition of the temporary name is performed regardless of whether the file was opened for reading or writing. That particular case could be fixed, but it would also break resumed uploads via common commandline tools as well as sshfs. I don't think these could be fixed. Implementing uploads that go via a temporary file in the client seems much more feasible as it would be subject to these considerations. -d