Mailing List Archive

[PATCH] add sftp-server option to force temp files
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
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] add sftp-server option to force temp files [ In reply to ]
On Sun, Feb 20, 2022 at 11:08 AM Nathan Wagner <nw@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.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] add sftp-server option to force temp files [ In reply to ]
On 2/20/22 13:30, Nico Kadel-Garcia wrote:
> On Sun, Feb 20, 2022 at 11:08 AM Nathan Wagner <nw@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 Linux, one can use O_TMPFILE to ensure that the file is created
atomically. That isn’t portable, though.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Re: [PATCH] add sftp-server option to force temp files [ In reply to ]
On Sun, Feb 20, 2022 at 01:30:57PM -0500, Nico Kadel-Garcia wrote:
> On Sun, Feb 20, 2022 at 11:08 AM Nathan Wagner <nw@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?

I don't have any control over the client software. The scanner runs
whatever the manufacturer installs.

> 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,

I'd rather leave behind a temp file than a partial file with the
intended name. The bad temp file is a lot easier to identify and
remove.

--
nw
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] add sftp-server option to force temp files [ In reply to ]
On Sun, 20 Feb 2022, Nathan Wagner wrote:

> intended name. The bad temp file is a lot easier to identify and
> remove.

Maybe make it possible to add a ~ after the XXXXXXXXXXes?
mkstemps(3), that is.

bye,
//mirabilos
--
Infrastrukturexperte • tarent solutions GmbH
Am Dickobskreuz 10, D-53121 Bonn • http://www.tarent.de/
Telephon +49 228 54881-393 • Fax: +49 228 54881-235
HRB AG Bonn 5168 • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

****************************************************
/?\ The UTF-8 Ribbon
? ? Campaign against Mit dem tarent-Newsletter nichts mehr verpassen:
 ?  HTML eMail! Also, https://www.tarent.de/newsletter
? ? header encryption!
****************************************************
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] add sftp-server option to force temp files [ In reply to ]
On Sun, Feb 20, 2022 at 11:27:26PM +0100, Thorsten Glaser wrote:
> On Sun, 20 Feb 2022, Nathan Wagner wrote:
>
> > intended name. The bad temp file is a lot easier to identify and
> > remove.
>
> Maybe make it possible to add a ~ after the XXXXXXXXXXes?
> mkstemps(3), that is.

The mkstemp(3) interface requires that the last six characters of the
passed template string be 'X'. There are non-posix functions that allow
for a suffix, but I wanted to keep the code as portable as seemed
reasonable.

--
nw
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] add sftp-server option to force temp files [ In reply to ]
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
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] add sftp-server option to force temp files [ In reply to ]
On 23.02.22 02:37, Damien Miller wrote:
> Implementing uploads that go via a temporary file in the client seems
> much more feasible as it would be subject to these considerations.

Oh I'd *wish* ... the latest supplier we added to our SFTP server and
urged to do "upload temp file, then rename" got as close as doing PUT +
COPY + DELETE, so we're now seriously having inotify watch for the
DELETE to trigger the postprocessing of the beforeCOPYed file ... :-/

(And then there are those like the one whose client s/w throws in the
towel if our server does as much as *offer* keypair auth in addition to
passwords ... *so* glad that PasswordAuthentication and
AuthenticationMethods can be used in Match blocks ...)

Kind regards,
--
Jochen Bern
Systemingenieur

Binect GmbH