strncpys in libpcap should be strlcp

[guyharris]

Converted from SourceForge issue 599847, submitted by donhatch

All strncpys in libpcap should be changed to strlcpy.
Also, there is this strange thing in pcap-snit.c:
strncpy(ifr.ifr_name, device,
sizeof(ifr.ifr_name));
ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = ' ';
I don't understand why the space character is used
here; it seems to me
that it should be '\0'
(which will of course be unecessary when this strncpy
is changed to strlcpy).

[infrastation]

Commit bf2270d addressed NULL termination part of this report in 2002.

[infrastation]

strlcpy(), which boils to strncpy() with NULL termination enforced, isn't portable. For example, my Linux host doesn't include it. There is an article explaining that just having strlcpy() in the code doesn't result in guaranteed security: http://lwn.net/Articles/507319/

A sufficient scope of this request would be to check that all strncpy() instances handle NULL termination.

[guyharris]

There were already some strlcpy()s in pcap-linux.c; I converted the remainder of them to fix some Coverity issues. We have a fallback version of snprintf(); we should perhaps have a fallback version of strlcpy() as well, just as tcpdump does.

The strncpy()s and strcpy()s I converted are for ioctls on the interface; we should probably check, in the create routine on Linux, whether the name passed in is too big for those ioctls and, if so, return "no such device" (because the Linux kernel presumably won't create network interfaces whose names won't fit in those ioctls - if it does, that's a kernel bug).

[guyharris]

we should probably check, in the create routine on Linux, whether the name passed in is too big for those ioctls and, if so, return "no such device"

Or in the activate routine; I've added that check in 1099050.

[guyharris]

pcap-int.h has

#ifndef HAVE_STRLCPY
#define strlcpy(x, y, z) \
        (strncpy((x), (y), (z)), \
         ((z) <= 0 ? 0 : ((x)[(z) - 1] = '\0')), \
         strlen((y)))
#endif

and the libpcap configure script checks for strlcpy(), so if it's missing, we have a macro that does the right thing using strncpy().

[guyharris]

we should probably check, in the create routine on Linux, whether the name passed in is too big for those ioctls and, if so, return "no such device"

Or in the activate routine; I've added that check in 1099050.

Done in pcap-bpf.c as well, with various commits.

I'll look at cleaning up the (ancient) remaining code that's using strncpy().

[infrastation]

This issue was opened on SourceForge in 2002. Resolving it properly would take to convert the remaining 10 instances of strncpy() to pcapint_strlcpy(), correct?

[guyharris]

This issue was opened on SourceForge in 2002. Resolving it properly would take to convert the remaining 10 instances of strncpy() to pcapint_strlcpy(), correct?

Yes.

Just for the lulz, I took a look at various bits of network-interface-ioctl code in the Illumos kernel, treating it as a possible proxy for the Solaris kernel, and it's... inconsistent in how it handles the name field in network interface ioctl structures.

My inclination would be to null-terminate them by using pcapint_strlcpy() until somebody complains about getting errors because an interface has a 16-character or 32-character name (not counting the terminating '\0').

[infrastation]

Would reporting the inconsistencies you found make things simpler? Some of the bugs I reported about illumos got fixed, or at least acknowledged.

[infrastation]

In the illumos-gate repository ip_extract_lifreq() has been doing the following since 2005:

       if (ipip->ipi_cmd_type == IF_CMD) {
                /* This a old style SIOC[GS]IF* command */
                ifr = (struct ifreq *)mp1->b_rptr;
                /*
                 * Null terminate the string to protect against buffer
                 * overrun. String was generated by user code and may not
                 * be trusted.
                 */
                ifr->ifr_name[IFNAMSIZ - 1] = '\0';
                name = ifr->ifr_name;
                ci->ci_sin = (sin_t *)&ifr->ifr_addr;
                ci->ci_sin6 = NULL;
                ci->ci_lifr = (struct lifreq *)ifr;
        } else {
                /* This a new style SIOC[GS]LIF* command */
                ASSERT(ipip->ipi_cmd_type == LIF_CMD);
                lifr = (struct lifreq *)mp1->b_rptr;
                /*
                 * Null terminate the string to protect against buffer
                 * overrun. String was generated by user code and may not
                 * be trusted.
                 */
                lifr->lifr_name[LIFNAMSIZ - 1] = '\0';
                name = lifr->lifr_name;
                ci->ci_sin = (sin_t *)&lifr->lifr_addr;
                ci->ci_sin6 = (sin6_t *)&lifr->lifr_addr;
                ci->ci_lifr = lifr;
        }

If this sanitizes every relevant ioctl() request, converting the remaining instances in fad-gifc.c and fad-glifc.c would not break anything, as far as I understand.

[infrastation]

Furthermore, ioctl() inconsistencies (if present) do not apply to illumos, which uses fad-getad.c, so the potential breakage of enforcing the NUL termination concerns only Solaris 10, AIX and HP-UX.

Commit 6b6d692 removes fad-glifc.c from the problem space (#1425 is a by-product of this), so only fad-gifc.c remains.

[infrastation]

Commit c3e5d0c removes fad-gifc.c from the problem space and resolves this request completely: the source code does not use strncpy() at all. Closing.