Pitfalls Using strcat

strcat is a C standard library call that concatenates strings. strncat is a similar call with a notion of bounds checking. Correct use of strcat and strncat can be problematic and it's easy for developers to use these APIs incorrectly. I'll outline some of the problems and show real code in Kali/Debian Linux that uses them incorrectly.

Introduction

Code review is necessary to aid secure development. Code review is also a primary tool used in vulnerability research. Although application development is popular today, systems languages like C are dominant in Operating Systems and in embedded devices.

The C standard library includes a number of string related APIs. Strings have long been a source of vulnerabilities in C and in this blog post, I'll highlight issues related to the strcat/strncat API.

The strcat/strncat API is defined in the Linux man pages as:

char *strcat(char *dest, const char *src);

char *strncat(char *dest, const char *src, size_t n);  

The strcat() function appends the src string to the dest string, overwriting the terminating null byte ('\0') at the end of dest, and then adds a terminating null byte. The strings may not overlap, and the dest string must have enough space for the result. If dest is not large enough, program behavior is unpredictable; buffer overruns are a favorite avenue for attacking secure programs.  

The strncat() function is similar, except that it will use at most n bytes from src; and src does not need to be null-terminated if it contains n or more bytes.

Bug Pattern 1

The simplest bug pattern is not using the bounds checked version of strcat.

Here is a current example from a Kali/Debian package tbb-2017_U7/examples/parallel_for/tachyon/src/main.cpp

static char *window_title_string (int argc, const char **argv)
{
    int i;
    char *name;

    name = (char *) malloc (8192);
    char *title = getenv ("TITLE");
    if( title ) strcpy( name, title );
    else {
        if(strrchr(argv[0], '\\')) strcpy (name, strrchr(argv[0], '\\')+1);
        else if(strrchr(argv[0], '/')) strcpy (name, strrchr(argv[0], '/')+1);
        else strcpy (name, *argv[0]?argv[0]:"Tachyon");
    }
    for (i = 1; i < argc; i++) {
        strcat (name, " ");
        strcat (name, argv[i]);
    }
#ifdef _DEBUG
    strcat (name, " (DEBUG BUILD)");
#endif
    return name;
}

argv is passed via command line arguments and if the length of the command line arguments exceeds 8192, then a buffer overflow will occur.

Note there is also another simple buffer overflow in the above code.

    name = (char *) malloc (8192);
    char *title = getenv ("TITLE");
    if( title ) strcpy( name, title )

This is a classic heap overflow based on a large environment variable. To trigger these types of environment variable overflows, from the shell, we can use this command before running our vulnerable program

$ export TITLE=$(python -c  'print "A" * 10000')
$ ./vulnerable_program

Bug Pattern 2

The simplest fix to the previous bug pattern is to use the bounds checked version of strcat. However, the size field in strncat can be confusing to developers. The size indicates how many characters from source string to write, not the total size of the destination string. As such, a common bug pattern is to use the destination size for the size field ignoring the fact that if the destination string is of non zero length, the strncat call will cause a buffer overflow.

Here is a current example in openbios-1.1.git20171015/fs/hfs/hfs_fs.c

static int
_search( hfsvol *vol, const char *path, const char *sname, hfsfile **ret_fd )
{
        hfsdir *dir;
        hfsdirent ent;
        int topdir=0, status = 1;
        char *p, buf[256];

        strncpy( buf, path, sizeof(buf) );
        if( buf[strlen(buf)-1] != ':' )
                strncat( buf, ":", sizeof(buf) );
        buf[sizeof(buf)-1] = 0;

The above code is also interesting that strncpy() has a bug in it too. From the strncpy man page:

The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null-terminated.

The above code does not guarantee NUL termination before the strncat and it tries to explicitly NUL terminate too late. This might result in the appearance of a large string depending on the state of the uninitialised stack and therefore might lead to another buffer overflow.

Bug Pattern 3

A simple fix for bug pattern 2 is to take into account the length of the destination string. However, it's quite easy to introduce an off-by-1 and write past the defined buffer by 1 NUL byte.

Here is a current example in wmi-1.3.16/Samba/source/client/tree.c

  /* 
   * Got a list of comps now, should check that we did not hit a workgroup
   * when we got other things as well ... Later
   *
   * Now, build the path
   */

  snprintf(path_string, sizeof(path_string), "smb:/");

  for (j = i - 1; j >= 0; j--) {

    strncat(path_string, "/", sizeof(path_string) - strlen(path_string));
    strncat(path_string, comps[j], sizeof(path_string) - strlen(path_string));

  }
  
  fprintf(stdout, "Path string = %s\n", path_string);

  return path_string;

Correct Code

Let's look at some correct code in android-tools_5.1.1_r38/system/core/adb/commandline.c:

    snprintf(buf, sizeof(buf), "backup");
    for (argc--, argv++; argc; argc--, argv++) {
        strncat(buf, ":", sizeof(buf) - strlen(buf) - 1);
        strncat(buf, argv[0], sizeof(buf) - strlen(buf) - 1);
    }

Correct Code?

We'll keep on looking at the previous code. Are they being consistent in other uses of strncat?

static int logcat(transport_type transport, char* serial, int argc, char **argv)
{
    char buf[4096];

    char *log_tags;
    char *quoted;

    log_tags = getenv("ANDROID_LOG_TAGS");
    quoted = escape_arg(log_tags == NULL ? "" : log_tags);
    snprintf(buf, sizeof(buf),
            "shell:export ANDROID_LOG_TAGS=\"%s\"; exec logcat", quoted);
    free(quoted);

    if (!strcmp(argv[0], "longcat")) {
        strncat(buf, " -v long", sizeof(buf) - 1);
    }

    argc -= 1;
    argv += 1;
    while(argc-- > 0) {
        quoted = escape_arg(*argv++);
        strncat(buf, " ", sizeof(buf) - 1);
        strncat(buf, quoted, sizeof(buf) - 1);
        free(quoted);
    }

    send_shellcommand(transport, serial, buf);
    return 0;
}

It's back to simple strncat misuse leading to a buffer overflow. It shows you that even within a single source file, consistency and security are not always evident. It probably means that multiple developers worked on this code.

Conclusion

strcat and strncat are simple APIs that are often misused. It is prudent to be aware of the misuse patterns to identify vulnerabilities.

Memory Bugs in Multiple Linux Kernel Drivers using DebugFS

Multiple drivers in the Linux Kernel using the DebugFS follow a bug pattern enabling memory disclosure and corruption of heap allocated memory. Generally, kernel memory is copied back into user space using an incorrect length field leading to memory disclosure.

There's no need for concern. These bugs are almost completely mitigated by the kernel configuration CONFIG_HARDENED_USERCOPY, which is set as a default on many Linux distributions. In the code review below, only 1 bug out of 4 is not mitigated by this configuration. Additionally, these bugs have low impact because DebugFS is normally only enabled by kernel developers. It is unlikely for production kernels to enable this feature.

Introduction

The Linux kernel is the heart of the operating system and controls things from device interfacing to process scheduling. DebugFS is a filesystem used internally by kernel developers to provide additional debugging information undesirable in production.

From Wikipedia,

debugfs is a special file system available in the Linux kernel since version 2.6.10-rc3.^[1] It was written by Greg Kroah-Hartman.^[2]debugfs is a simple-to-use RAM-based file system specially designed for debugging purposes. It exists as a simple way for kernel developers to make information available to user space.^[3] Unlike /proc, which is only meant for information about a process, or sysfs, which has strict one-value-per-file rules, debugfs has no rules at all. Developers can put any information they want there.^[4]

In the Linux kernel, debugfs is enabled using the DEBUGFS configuration variable.

Formatted String APIs in the Kernel

Before we look at the bugs, let's look at some kernel APIs which are used in triggering the bugs.

Most people are probably familiar with snprintf(). snprintf will write a formatted string and place it into a buffer. It performs bounds checking so will prevent simple buffer overflows.

snprintf — Format a string and place it in a buffer

int snprintf (	char * buf,
	size_t size,
	const char * fmt,
	...);

The return value is the number of characters which would be generated for the given input, excluding the trailing null, as per ISO C99. If the return is greater than or equal to size, the resulting string is truncated.

The return value is the number of bytes it would have written if bounds checking was ignored. The return value allows you to determine the number of characters needed in the buffer, so the correct size buffer may be allocated before trying to fully construct the output.

In the Linux kernel, an additional API is provided which slightly modifies the semantics of snprintf. In this case, the return value is altered.

scnprintf — Format a string and place it in a buffer

int scnprintf (	char * buf,
	size_t size,
	const char * fmt,
	...);

The return value is the number of characters written into buf not including the trailing '\0'. If size is == 0 the function returns 0. 

This is interesting. And if people aren't aware of the differences between scnprintf and snprintf, there is an opportunity for vulnerabilities.

Userspace Copies

Memory gets copied back and forth between userspace and kernelspace. This is a mechanism that could allow either memory corruption if too much memory was copied into kernelspace, or memory disclosure if too much memory was copied into userspace.

One API of interest is simple_copy_from_buffer.

ssize_t simple_read_from_buffer(void __user * to, size_t count, loff_t * ppos, const void * from, size_t available) 

copy data from the buffer to user space

The simple_read_from_buffer() function reads up to count bytes from the buffer from at offset ppos into the user space address starting at to.

On success, the number of bytes read is returned and the offset ppos is advanced by this number, or negative value is returned on error.

Hardened Usercopies

simple_read_from_buffer uses the copy_to_user API internally. Furthmore, copy_to_user is normally protected by the HARDENED_USERCOPY mitigation.

From https://lwn.net/Articles/727322/

Much of the heap memory used within the kernel is obtained from the slab allocator. The hardened usercopy patch set, merged for the 4.8 kernel, attempts to limit the impact of erroneous copy operations by ensuring that no single operation can cross the boundary between one slab-allocated object and the next. But the kernel gets a lot of large memory objects from the slab allocator, and it is often not necessary to copy the entire object between the kernel and user space. In cases where only part of an object needs to be copied, it would be useful to prevent a rogue copy operation from copying to or from parts of the structure that do not need to be exposed in this way. 

Ultimately, this means that if simple_copy_to_user copies heap allocated memory, then this mitigation will prevent a memory disclosure.

DebugFS Drivers Done the Right Way

Here is a kernel driver writing data to a buffer that will be copied back into userspace. Note that it uses scnprintf.

        buf = kmalloc(buf_size, GFP_KERNEL);

        if (!buf)
                return -ENOMEM;

        off = 0;
        off += scnprintf(buf + off, buf_size - off,
                        "Mp2 Device Information:\n");

        off += scnprintf(buf + off, buf_size - off,
                        "========================\n");
        off += scnprintf(buf + off, buf_size - off,
                        "\tMP2 C2P Message Register Dump:\n\n");
        u.v32 = readl(privdata->mmio + AMD_C2P_MSG0);
        off += scnprintf(buf + off, buf_size - off,
                        "AMD_C2P_MSG0 -\t\t\t%#06x\n", u.v32);

...

        u.v32 = readl(privdata->mmio + AMD_P2C_MSG_INTSTS);
        off += scnprintf(buf + off, buf_size - off,
                        "AMD_P2C_MSG_INTSTS -\t\t%#06x\n", u.v32);

        ret = simple_read_from_buffer(ubuf, count, offp, buf, off);
        kfree(buf);
        return ret;
}

There are a couple of interesting points here. Let's outline the algorithm.

1. Allocate a kernel buffer
2. Write formatted content to the buffer.
1. Ensure the buffer does not overflow by using the appropriate size value in scnprintf
2. Track how much data is actually written by summing the return values of scnprintf
3. Copy buffer back to userspace

The code above requires scnprintf to work.

DebugFS Almost Done the Right Way

Let's look at a partially correct driver using snprintf.

        struct eg20t_port *priv = file->private_data;
        char *buf;
        u32 len = 0;
        ssize_t ret;
        unsigned char lcr;

        buf = kzalloc(PCH_REGS_BUFSIZE, GFP_KERNEL);
        if (!buf)
                return 0;

        len += snprintf(buf + len, PCH_REGS_BUFSIZE - len,
                        "PCH EG20T port[%d] regs:\n", priv->port.line);

        len += snprintf(buf + len, PCH_REGS_BUFSIZE - len,
                        "=================================\n");
        len += snprintf(buf + len, PCH_REGS_BUFSIZE - len,
                        "IER: \t0x%02x\n", ioread8(priv->membase + UART_IER));

...

        if (len > PCH_REGS_BUFSIZE)
                len = PCH_REGS_BUFSIZE;

        ret =  simple_read_from_buffer(user_buf, count, ppos, buf, len);
        kfree(buf);
        return ret;
}

We can see that an extra length check must be made if snprintf is used. If this check was not in place, then it would be possible to leak memory back into user space (providing debugfs was enabled and hardened usercopy was not).

First Bug

Lets look at a code snippet from the snprintf code I showed earlier.

        len += snprintf(buf + len, PCH_REGS_BUFSIZE - len,
                        "=================================\n");

What happens when len is greater than PCH_REGS_BUFSIZE? This could occur if we ended up writing a larger than expected size to the buffer. This is seemingly possible, otherwise, why do we need all the extra code for bounds checking? The code tries to mitigate against larger than expected results by using bounds checking in snprintf.

Lets assume len can be greater than PCH_REGS_BUFSIZE.

snprintf will protect the driver right?

When len exceeds the expected bounds, PCH_REGS_BUFSIZE - len becomes negative. And this value is passed to snprintf. That's not good.

int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
{
        unsigned long long num;
        char *str, *end;
        struct printf_spec spec = {0};

        /* Reject out-of-range values early.  Large positive sizes are
           used for unknown buffer sizes. */
        if (WARN_ON_ONCE(size > INT_MAX))
                return 0;

So size is passed as a negative as I discussed earlier. This  makes size > INT_MAX. We don't get memory corruption, but there will be a warning.

So this almost correct driver will warn, but there won't be any information disclosure or memory corruption.

Memory Corruption (1)

Here is a driver in drivers/misc/lkdtm_core.c

static ssize_t lkdtm_debugfs_read(struct file *f, char __user *user_buf, size_t count, loff_t *off) { char *buf; int i, n, out; buf = (char *)__get_free_page(GFP_KERNEL); if (buf == NULL) return -ENOMEM; n = snprintf(buf, PAGE_SIZE, "Available crash types:\n"); for (i = 0; i < ARRAY_SIZE(crashtypes); i++) { n += snprintf(buf + n, PAGE_SIZE - n, "%s\n", crashtypes[i].name); } buf[n] = '\0'; out = simple_read_from_buffer(user_buf, count, off, buf, n); free_page((unsigned long) buf); return out; }

We know that snprintf does bounds checking and if the bounds are exceeded (n is greater PAGE_SIZE), we'll get a negative size for snprintf. The kernel will issue a warning because it is greater than INT_MAX.

The allocated buffer is manually NUL terminated using buf[n]. If n is greater than PAGE_SIZE, which is a possibility, then we'll have a NUL byte written out of bounds. We achieved memory corruption that will work even without HARDENED_USERCOPY.

Memory Disclosure (2)

Here is a vulnerable driver in drivers/spi/spi-dw.c

#ifdef CONFIG_DEBUG_FS
#define SPI_REGS_BUFSIZE        1024
static ssize_t dw_spi_show_regs(struct file *file, char __user *user_buf,
                size_t count, loff_t *ppos)
{
        struct dw_spi *dws = file->private_data;
        char *buf;
        u32 len = 0;
        ssize_t ret;

        buf = kzalloc(SPI_REGS_BUFSIZE, GFP_KERNEL);
        if (!buf)
                return 0;

        len += snprintf(buf + len, SPI_REGS_BUFSIZE - len,
                        "%s registers:\n", dev_name(&dws->master->dev));
        len += snprintf(buf + len, SPI_REGS_BUFSIZE - len,
                        "=================================\n");
        len += snprintf(buf + len, SPI_REGS_BUFSIZE - len,
...
        len += snprintf(buf + len, SPI_REGS_BUFSIZE - len,
                        "DMARDLR: \t0x%08x\n", dw_readl(dws, DW_SPI_DMARDLR));
        len += snprintf(buf + len, SPI_REGS_BUFSIZE - len,
                        "=================================\n");

        ret = simple_read_from_buffer(user_buf, count, ppos, buf, len);
        kfree(buf);
        return ret;

In this case, len might be large and lead to a userspace copy larger than the kernel buffer.

Memory Disclosure (3)

Let's look at drivers/char/virtio_console.c

static ssize_t debugfs_read(struct file *filp, char __user *ubuf,
                            size_t count, loff_t *offp)
{
        struct port *port;
        char *buf;
        ssize_t ret, out_offset, out_count;

        out_count = 1024;
        buf = kmalloc(out_count, GFP_KERNEL);
        if (!buf)
                return -ENOMEM;

        port = filp->private_data;
        out_offset = 0;
        out_offset += snprintf(buf + out_offset, out_count,
                               "name: %s\n", port->name ? port->name : "");
        out_offset += snprintf(buf + out_offset, out_count - out_offset,
                               "guest_connected: %d\n", port->guest_connected);
...
        out_offset += snprintf(buf + out_offset, out_count - out_offset,
                               "is_console: %s\n",
                               is_console_port(port) ? "yes" : "no");
        out_offset += snprintf(buf + out_offset, out_count - out_offset,
                               "console_vtermno: %u\n", port->cons.vtermno);

        ret = simple_read_from_buffer(ubuf, count, offp, buf, out_offset);
        kfree(buf);
        return ret;
}

Another memory disclosure following the same bug pattern.

Potential Memory Disclosures (4)

Let's look at drivers/net/wireless/marvell/libertas/debugfs.c

static ssize_t lbs_dev_info(struct file *file, char __user *userbuf,
                                  size_t count, loff_t *ppos)
{
        struct lbs_private *priv = file->private_data;
        size_t pos = 0;
        unsigned long addr = get_zeroed_page(GFP_KERNEL);
        char *buf = (char *)addr;
        ssize_t res;
        if (!buf)
                return -ENOMEM;

        pos += snprintf(buf+pos, len-pos, "state = %s\n",
                                szStates[priv->connect_status]);
        pos += snprintf(buf+pos, len-pos, "region_code = %02x\n",
                                (u32) priv->regioncode);

        res = simple_read_from_buffer(userbuf, count, ppos, buf, pos);

        free_page(addr);
        return res;
}

This might not be possible to trigger if the string length is short, but it still presents a buggy pattern . This driver has a number of cases using this pattern however. For example:

        for (i = 0; i < num_of_items; i++) {
                if (d[i].size == 1)
                        val = *((u8 *) d[i].addr);
                else if (d[i].size == 2)
                        val = *((u16 *) d[i].addr);
                else if (d[i].size == 4)
                        val = *((u32 *) d[i].addr);
                else if (d[i].size == 8)
                        val = *((u64 *) d[i].addr);

                pos += sprintf(p + pos, "%s=%d\n", d[i].name, val);
        }

        res = simple_read_from_buffer(userbuf, count, ppos, p, pos);

        free_page(addr);
        return res;
}

Other Bugs

There a few other places following similar patterns, but they are unlikely to be triggered.

Recommendations

The solution is quite easy. Replace snprintf with scnprintf.

Conclusion

This is an interesting look into bug patterns. There is almost certainly going to be a history of cut and pasting insecure code between drivers that leads to problems. It also highlights the problems of duplicate code leading to bugs and vulnerabilities - when code is manually duplicated instead of being centralised, bugs appear.

Overall, these bugs are extremely low impact. Highlighting them is more an exercise into the code review process and the nature of bugs. Still, they are security bugs and should be fixed.