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.