Package: tftpd-hpa
Version: 5.2+20150808-1.2
We have discovered that tftpd-hpa does not detect a partial upload nor
does it remove the file in case of an interrupted upload. The attached
patch fixes both of these issues. Specifically, it does the following:
1. If the client sent the upload tsize, then verify that it matches
the final file size on disk.
2. And, if the sizes do not match or any other error was encountered
during upload, log a warning and remove the file.
We have tested the patch both manually with curl to confirm the above
error cases are indeed handled as expected, and on our CI system (where
tftpd-hpa is used to communicate result from the VM to the host) for
any regressions.
I am also happy to answer any questions about the issues or the patch.
--- tftp-hpa-5.2+20150808.orig/tftpd/tftpd.c 2015-08-08 06:24:45.000000000 +0200
+++ tftp-hpa-5.2+20150808/tftpd/tftpd.c 2020-09-30 16:00:50.512458328 +0200
@@ -151,13 +151,13 @@
exit_signal = sig;
}
-/* Handle timeout signal or timeout event */
+/* Handle timeout event */
void timer(int sig)
{
(void)sig; /* Suppress unused warning */
timeout <<= 1;
if (timeout >= maxtimeout || timeout_quit)
- exit(0);
+ return;
siglongjmp(timeoutbuf, 1);
}
@@ -264,7 +264,8 @@
} while (rv == -1 && err == EINTR);
if (rv == 0) {
- timer(0); /* Should not return */
+ timer(0); /* If returns, fail. */
+ errno = ETIMEDOUT;
return -1;
}
@@ -1245,8 +1246,20 @@
if (!tsize_ok)
return 0;
+ if (tsize_ok == 1) { /* RRQ */
if (sz == 0)
sz = tsize;
+ }
+ else { /* WRQ */
+ /* Note that a client may send 0 tsize both when the size is unknown
+ and when uploading 0 bytes (at least this is what curl does). But
+ this ambiguity doesn't matter in our case since 0 bytes cannot be
+ truncated to anything less. So we treat both cases as unknown. */
+ if (sz != 0) {
+ tsize = sz;
+ tsize_ok = 1; /* ok to get */
+ }
+ }
*vp = sz;
return 1;
@@ -1418,6 +1431,7 @@
#endif
static FILE *file;
+static const char *filename;
/*
* Validate file access. Since we
* have no uid or gid, for now require
@@ -1429,18 +1443,19 @@
* Note also, full path name must be
* given as we have no login directory.
*/
-static int validate_access(char *filename, int mode,
+static int validate_access(char *fname, int mode,
const struct formats *pf, const char **errmsg)
{
struct stat stbuf;
int i, len;
int fd, wmode, rmode;
- char *cp;
+ const char *cp;
const char **dirp;
char stdio_mode[3];
tsize_ok = 0;
*errmsg = NULL;
+ filename = fname;
if (!secure) {
if (*filename != '/') {
@@ -1510,7 +1525,7 @@
}
tsize = stbuf.st_size;
/* We don't know the tsize if conversion is needed */
- tsize_ok = !pf->f_convert;
+ tsize_ok = pf->f_convert ? 0 : 1 /* ok to get */;
} else {
if (!unixperms) {
if ((stbuf.st_mode & (S_IWRITE >> 6)) == 0) {
@@ -1527,7 +1542,7 @@
}
#endif
tsize = 0;
- tsize_ok = 1;
+ tsize_ok = 2; /* ok to set */
}
stdio_mode[0] = (mode == RRQ) ? 'r' : 'w';
@@ -1652,6 +1667,7 @@
static int acksize;
u_short dp_opcode, dp_block;
unsigned long r_timeout;
+ struct stat stbuf;
dp = w_init();
do {
@@ -1711,6 +1727,17 @@
}
} while (size == segsize);
write_behind(file, pf->f_convert);
+
+ /* Verify the file size if it was sent by the client. */
+ if (tsize_ok == 1) {
+ (void)fflush(file);
+ if (fstat(fileno(file), &stbuf) < 0)
+ exit(EX_OSERR); /* This shouldn't happen */
+ if (stbuf.st_size != tsize) {
+ goto abort;
+ }
+ }
+
(void)fclose(file); /* close data file */
ap->th_opcode = htons((u_short) ACK); /* send the "final" ack */
@@ -1726,7 +1753,11 @@
block == dp_block) { /* then my last ack was lost */
(void)send(peer, ackbuf, 4, 0); /* resend final ack */
}
+ return;
abort:
+ /* Remove the file in case of a partial upload. */
+ syslog(LOG_WARNING, "Partial/interrupted upload, removing %s", filename);
+ (void)unlink(filename);
return;
}
