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.

Popular posts from this blog

Empowering Women in Cybersecurity: InfoSect's 2024 Training Initiative

 InfoSect has always been committed to fostering diversity and inclusion within the cybersecurity industry, with a special focus on encouraging those who identify as women to excel in the technical streams. We're excited to announce that in 2024, we're taking our commitment to the next level. Following the resounding success of our 2022 initiative, we are proud to double our efforts and expand our training opportunities for women in cybersecurity. HackerChix: Uniting Women in Cybersecurity In 2017, InfoSect introduced HackerChix, a vibrant community that provides a platform for women to connect, support, and inspire one another in the world of cybersecurity. This initiative has been a constant presence at BSides Canberra every year.   Doubling Down on Education To empower more women with the knowledge and skills needed to excel in cybersecurity, InfoSect is thrilled to announce a significant expansion of its training offerings in 2024. Thanks to the generous sponsorshi
Read more

C++ Memory Corruption (std::string) - part 4

  Summary This is the next part of the C++ memory corruption series*. In this post, we'll look at corrupting the std:string object in Linux and see what exploitation primitives we can gain. * https://blog.infosectcbr.com.au/2020/08/c-memory-corruption-part-1.html * https://blog.infosectcbr.com.au/2022/01/c-memory-corruption-stdvector-part-2.html  *  https://blog.infosectcbr.com.au/2022/03/c-memory-corruption-stdlist-part-3.html Author: Dr Silvio Cesare Introduction C++ is a common language for memory corruption. However, there is much more literature on exploiting C programs and little on C++ programs. C++ presents new classes, objects, and data structures which can all be effectively used for building exploitation primitives. In this post, we'll look at corrupting the std::string class and see what specific primitives we can obtain. std::string We note that the object stored in memory for a basic string consists firstly of the backing pointer to the string contents. Se
Read more

Pointer Compression in V8

Image
Syed Faraz Abrar @farazsth98   Summary In this blog post, I will provide some details on how the Chromium developers implemented pointer compression in V8. I will also talk about what this means from an exploit development perspective. Introduction I’ve been an intern at InfoSect for the past couple of weeks now, and in this time, I’ve had to do a bunch of security related research into both V8 and Spidermonkey. One of the things that I spent a short amount of my time on was pointer compression in V8. I hadn’t heard of the term at all until Bruno Keith ( @bkth_ ) mentioned it on twitter some time in December last year. The V8 developers also made a blog post to celebrate V8 v8.0 where they mentioned that implementing pointer compression had allowed them to save up to 40% in usage of heap memory! That’s a big improvement, so let’s take a look at what pointer compression is and what it means from an exploit developer’s perspective. Disclaimer This is not intended to
Read more