Re: tcpdump-3.8.3

[Marcus Brinkmann <marcus.brinkmann@ruhr-uni-bochum.de>]

At Thu, 10 Feb 2005 20:57:06 -0800,
Barry deFreese wrote:
> +#if defined(__GNU__)
> +# define MAXHOSTNAMELEN 64
> +#endif

tcpdump.

print-atalk.c uses MAXHOSTNAMELEN this way:

        char nambuf[MAXHOSTNAMELEN + 20];


        ... getting lines from "/etc/atalk.names" into ...
        char line[256];

        if (sscanf(line, "%d.%d.%d %256s", &i1, &i2, &i3,
                                   nambuf) == 4)

If I read this correctly, this will happily overwrite anything after
nambuf[64+20] if the lines in /etc/atalk.names happen to exceed
%d.%d.%d + space + 84 characters.  A simple buffer overrun.  The
"%256s" should instead maybe be something like "%*s" and then "sizeof
(nambuf) - 1, nambuf" as arguments (is that portable?) and, in fact,
it should use LINE_MAX instead of "MAXHOSTNAMELEN + 20 (do we define
LINE_MAX?  If not, provide a default in tcpdump-stdinc.h of 2048).

The uses of MAXHOSTNAMELEN in print-bgp.c and print-icmp.c seem to be
properly protected by snprintf.  I think using longer hostnames will
just cause truncation (which could be considered a DoS attack if the
log is important to you).  Maybe just using a large MAXHOSTNAMELEN
will be ok here (ie, a default definition as your patch adds).  In
fact, _I_'d also just use some LINE_MAX here and be done with it.  I
think they just want something that is large enough for all hostnames
plus some things extra (the "100").

In fact, to properly deal with _arbitrary_ host name lengths, I'd want
the code to print the IP address first, then the hostname, truncated
to some arbitrary limit, and then the rest of the line.  This would
make sure that a static buffer is sufficient, without risk of losing
information.

Thanks,
Marcus