3.7.1: memory leak: getname()

[guyharris]

Converted from SourceForge issue 604464, submitted by nobody

hi,

while observing millions of mostly broken udp, tcp and
icmp payloads inside mostly broken ipv4 packets,
tcpdump-3.7.1 kept allocating more and more memory
until it had occupied a huge 244MB memory space.

my analysis is that there is some memory forgotten to
free in case of broken packets.

libpcap version 0.7.

regards,
-tom

[guyharris]

Submitted by guy_harris

Logged In: YES
user_id=541179

What happens if you run with the "-S" flag?

The IP dissector allocates no memory except for, when
running on a platform that requires strict alignment, a
one-time allocation of a buffer for packets whose IP header
isn't aligned on a 4-byte boundary.

The TCP dissector, however, allocates, when printing
relative sequence numbers, a data structure for each TCP
connection it sees (to keep track of the starting sequence
numbers); bogus traffic could cause it to see lots and lots
of bogus connections.

"-S" causes absolute sequence numbers to be displayed; the
TCP dissector, when "-S" is specified, doesn't allocate
those per-connection data structures.

[guyharris]

Submitted by nobody

Logged In: NO

hi,

i was running it like tcpdump -nvvvei eth0 and now i'm
running it like tcpdump -nvvvSei eth0. it still seems to
grow even with the -S option.

i also tried a simple modification that limits the total
number of per-connection tcp data structures to 10. but it
still grows.

i bet there is no possibility for recursion that would eat
the memory?

-tom

[guyharris]

Submitted by nobody

Logged In: NO

hi,

i'm beginning to feel that this is not really tcpdump's
problem but more likely the problem is in some of the libraries.

also it might be that the key is not 'broken packets' but
'the number of packets'; there are zillions of them being
observed so memory consuption problems come to light more
easily than under normal circumstances.

libpcap is version 0.7.1 to be more precise.

-tom

[guyharris]

Submitted by tom_soderlund

Logged In: YES
user_id=606285

hi,

observing now the HEAD version of both tcpdump and libpcap.

i think i found it. the key was not only the number of
packets, but that the packets were from random addresses:
this causes the cache of intoa() return values to grow
address by address until it contains the string
representations of all possible ip-addresses.

i verified this by making getname() in addrtoname.c return
straightly the result of intoa(addr) without caching it
first.

maybe the cache should be limited in size somehow?

sorry for my rather misleading information and assumptions
in the original report.

-tom

[workerthread]

TCPDUMP Denial-of-Service vulnerability

Hello,
while investigating some performance issues with tcpdump in a high-traffic environment, we discovered a non-optimal implementation of the “getname” function in file “addrtoname.c”, which not only leads to poor performance, but also makes tcpdump vulnerable to a DoS type attack.

Before proceeding, please note that we are not regular contributors of the tcpdump project, so we are not experts about the rest of the codebase, and before submitting the patch and notify the vulnerability we would like to have your feedback.

From what we understood by analyzing the code of “addrtoname.c”, the program, in order to cache the IP Address – Hostname resolution, maintains an hashtable (hnametable), which holds lists of type “hnamemem”.

Each “hnamemem” element holds:

• The IP address in unsigned int format
• The char pointer to the string containing the hostname
• A pointer to the subsequent hnamemem element in the list

The function “getname” in “addrtoname.c” is responsible for populating and maintaining such data structure, however we noticed that new elements are added to the “cache” even if the hostname resolution is turned off (by using the “-n” flag); in that case, the hostname field of hnamemem is populated with the string representation of the IP address (xxx.xxx.xxx.xxx).

This leads to a performance degradation and increasing memory occupation in case tcpdump is deployed in an environment where it “sees” many different IP addresses (i.e.: on a LAN with few hundreds or thousands of hosts, it is difficult to notice any performance degradation or anomalous memory usage).

In addition, since this function is called each time an IP has to be printed (i.e.: it is triggered also in “non-verbose” mode), an attacker could easily exploit for a DoS-type attack it by sending packets that sweep the entire IP address space. Please note that since this function is used almost everywhere, the attacker is not limited to crafting source and/or destination IP, but he can play also with addresses embedded inside the packet (i.e.: RADIUS packets).

In this way, he would obtain the following results:

• Continuously increase the memory occupied by the tcpdump process, possibly causing it to crash, or fill the entire server memory (assuming a size of 24 bytes for each entry in the cache, sweeping the entire IPv4 address space would lead to approx. 96 GB memory size).
• Increase the tcpdump CPU usage; since each time an IP address has to be printed, the program has to lookup inside a very large data structure (we observed that even with a data structure of about 40-50 MB the impact is huge), this would cause tcpdump to start dropping packets, even with moderate traffic.

We also did a quick check on the “getname6” function, and it also seems to be vulnerable (and in that case, with IPv6, only the sky is the limit for possible memory occupation :) ).

We have also successfully developed a shell script (quick&dirty) that exploits the vulnerability: it simply sweeps the IPv4 address space by sending “arping” requests, causing tcpdump to steadily increase the memory usage, and thus CPU usage too.

What we suggest in order to fix this vulnerability is:

1. Avoid instantiating and using such data structure at all, if the “-n” flag is specified. The only advantage we see in using the cache even if the hostname resolution is turned off, is avoiding the conversion of the IP address into printable format, but the experimental results show that this becomes a penalty when the cache becomes large. We believe the cache should serve just to avoid the costly DNS reverse lookups.
2. The remediation listed above, would leave tcpdump vulnerable in case the hostname lookup is turned on; in order to fix also this situation, we suggest to populate the cache only when an hostname has been successfully resolved. In this way, an attacker would not have a direct control over the cache size.

We propose the following patch, that according to our tests resolves the issue (obviously, it has been developed in a quick&dirty way, so we may have missed something, and it has to be polished before submitting it to the repository).

We look forward to receiving your feedback.

Antonio Trapanese
Mattia Dossena

---

+++ addrtoname.c.patched        2014-05-16 15:31:19.000000000 +0200
@@ -224,18 +224,12 @@
 const char *
 getname(const u_char *ap)
 {
-       register struct hostent *hp;
+
        u_int32_t addr;
-       static struct hnamemem *p;              /* static for longjmp() */
+

        memcpy(&addr, ap, sizeof(addr));
-       p = &hnametable[addr & (HASHNAMESIZE-1)];
-       for (; p->nxt; p = p->nxt) {
-               if (p->addr == addr)
-                       return (p->name);
-       }
-       p->addr = addr;
-       p->nxt = newhnamemem();
+

        /*
         * Print names unless:
@@ -246,8 +240,21 @@
         */
        if (!nflag &&
            (addr & f_netmask) == f_localnet) {
+
+       register struct hostent *hp;
+       static struct hnamemem *p;              /* static for longjmp() */
+
+       p = &hnametable[addr & (HASHNAMESIZE-1)];
+       for (; p->nxt; p = p->nxt) {
+               if (p->addr == addr)
+                       return (p->name);
+       }
+
+
                hp = gethostbyaddr((char *)&addr, 4, AF_INET);
                if (hp) {
+                       p->addr = addr;
+                       p->nxt = newhnamemem();
                        char *dotp;

                        p->name = strdup(hp->h_name);
@@ -260,8 +267,8 @@
                        return (p->name);
                }
        }
-       p->name = strdup(intoa(addr));
-       return (p->name);
+
+       return (intoa(addr));
 }

[infrastation]

I acknowledge this long-standing issue, it needs to be fixed eventually (whether in the suggested way or not).