This change attempts to optimse TFTP perfomance.
It reads the next block immediately after sending a block,
in order to reduce latency between receiving an ACK and sending
the next block. Is also suppresses lseek() calls when reading
successive blocks.
This improves performance when doing transfers without windowing
and when only a single transfer is in progress at one time.
TFTP fails with a timeout when the transfer exceeds 128K blocks.
The block field in an ACK packet is 16 bits, so large transfers
need to deal with this field wrapping. Testing failure meant
that it worked for the first wrap, but not for subsequent ones.
Having fixed the block number problem, file size calculations
accidentally being done in 32 bits not 64 bits then broke
transfers larger than 2G or maybe 4G.
The first problem was probably introduced in commit
ebef27f321 and has not appeared
in a stable release. The second appears to have been there forever.
Clearly, nobody is mad enough to transfer multi-gigabyte files
over TFTP.
Thanks to Jean-François JUBLIN for spotting this and supplying
useful packet dumps that made it easy to diagnose.
The next() function is broken for any TFTP packet with padding
which doesn't end with a zero.
Rewrite to handle such packets.
Thanks to Helge Deller <deller@gmx.de> for persisting in finding the
actual problem and proposing a solution. This patch is modelled on his,
but rewritten for personal preference by Simon Kelley, who is
responsible for all bugs.
Timeouts for TCP connections to non-responive servers are very long.
This in not appropriate for DNS connections.
Set timeouts for connection setup, sending data and recieving data.
The timeouts for connection setup and sending data are set at 5 seconds.
For recieving the reply this is doubled, to take into account the
time for usptream to actually get the answer.
Thanks to Petr Menšík for pointing out this problem, and finding a better
and more portable solution than the one in place heretofore.
As defined in the C standard:
In all cases the argument is an int, the value of which shall
be representable as an unsigned char or shall equal the value
of the macro EOF. If the argument has any other value, the
behavior is undefined.
This is because they're designed to work with the int values returned
by getc or fgetc; they need extra work to handle a char value.
If EOF is -1 (as it almost always is), with 8-bit bytes, the allowed
inputs to the ctype(3) functions are:
{-1, 0, 1, 2, 3, ..., 255}.
However, on platforms where char is signed, such as x86 with the
usual ABI, code like
char *arg = ...;
... isspace(*arg) ...
may pass in values in the range:
{-128, -127, -126, ..., -2, -1, 0, 1, ..., 127}.
This has two problems:
1. Inputs in the set {-128, -127, -126, ..., -2} are forbidden.
2. The non-EOF byte 0xff is conflated with the value EOF = -1, so
even though the input is not forbidden, it may give the wrong
answer.
Casting char to int first before passing the result to ctype(3)
doesn't help: inputs like -128 are unchanged by this cast. It is
necessary to cast char inputs to unsigned char first; you can then
cast to int if you like but there's no need because the functions
will always convert the argument to int by definition. So the above
fragment needs to be:
char *arg = ...;
... isspace((unsigned char)*arg) ...
This patch inserts unsigned char casts where necessary, and changes
int casts to unsigned char casts where the input is char.
I left alone int casts where the input is unsigned char already --
they're not immediately harmful, although they would have the effect
of suppressing some compiler warnings if the input is ever changed to
be char instead of unsigned char, so it might be better to remove
those casts too.
I also left alone calls where the input is int to begin with because
it came from getc; casting to unsigned char here would be wrong, of
course.
Check sender of all received packets, as specified in RFC 1350 para 4.
My understanding of the example in the RFC is that it in fact only
applies to server-to-client packets, and packet loss or duplication
cannot result in a client sending from more than one port to a server.
This check is not, therefore, strictly needed on the server side.
It's still useful, and adds a little security against packet
spoofing. (though if you're running TFTP on a public network with
bad actors, nothing can really save you.)
CVE-2021-3448 applies.
It's possible to specify the source address or interface to be
used when contacting upstream nameservers: server=8.8.8.8@1.2.3.4
or server=8.8.8.8@1.2.3.4#66 or server=8.8.8.8@eth0, and all of
these have, until now, used a single socket, bound to a fixed
port. This was originally done to allow an error (non-existent
interface, or non-local address) to be detected at start-up. This
means that any upstream servers specified in such a way don't use
random source ports, and are more susceptible to cache-poisoning
attacks.
We now use random ports where possible, even when the
source is specified, so server=8.8.8.8@1.2.3.4 or
server=8.8.8.8@eth0 will use random source
ports. server=8.8.8.8@1.2.3.4#66 or any use of --query-port will
use the explicitly configured port, and should only be done with
understanding of the security implications.
Note that this change changes non-existing interface, or non-local
source address errors from fatal to run-time. The error will be
logged and communiction with the server not possible.
At least on Fedora 32 with GCC 10.2.1, dnsmasq compilation emits warning:
tftp.c: In function ‘tftp_request’:
tftp.c:754:3: warning: ‘strcpy’ source argument is the same as
destination [-Wrestrict]
754 | strcpy(daemon->namebuff, file);
And indeed it is the same source always on line 477, sometimes also on
571 in tftp.c
Attached patch fixes the warning and possible undefined behaviour on
tftp error.
Fail on overlarge files (block numbers are limited to 16 bits)
Honour tftp-max setting in single port mode.
Tweak timeouts, and fix logic which suppresses errors if the
last ACK is missing.
This was the source of a large number of #ifdefs, originally
included for use with old embedded libc versions. I'm
sure no-one wants or needs IPv6-free code these days, so this
is a move towards more maintainable code.
Adds option to delay replying to DHCP packets by one or more seconds.
This provides a workaround for a PXE boot firmware implementation
that has a bug causing it to fail if it receives a (proxy) DHCP
reply instantly.
On Linux it looks up the exact receive time of the UDP packet with
the SIOCGSTAMP ioctl to prevent multiple delays if multiple packets
come in around the same time.
It is currently only possible to let the TFTP server serve a different
folder depending on the client's IP address.
However it isn't always possible to predict what the client's
IP address will be, especially in situations in which we are not
responsible for handing them out (e.g. proxy dhcp setups).
Extend the current --tftp-unique-root parameter to support having a
separate folder per MAC address instead.
