taviso

linux, programming and security

Auditing puzzle

, ,

Here is an interesting auditing question, try to make the following short C program crash.

#include <string.h>
#include <stdio.h>
#include <locale.h>
#include <ctype.h>

int main(int argc, char **argv)
{
    char buf[128];
    int len;

    /* initialise the buffer */
    memset(buf, '\0', sizeof(buf));

    /* setup locale for toupper() */
    setlocale(LC_ALL, "");

    /* check an argument was specified */
    if (argc >= 2) {
        /* get the first 10 characters */
        len = snprintf(buf, sizeof(buf), "you entered: %.10s", argv[1]);

        /* check for non-printing characters */
        while (len--)
            buf[len] = isprint(buf[len]) ? toupper(buf[len]) : '?';

        /* output string */
        fprintf(stdout, "%s\n", buf);
    }

    return 0;
}

Scroll down for discussion. .
.
.
.
.
.
.
.
.
.
.
.
.
.
.

snprintf() wont overrun the `buf` buffer, so specifying too much data wont cause a crash

$ ./a.out `perl -e 'print "A"x"1024"'`; echo $?
YOU ENTERED: AAAAAAAAAA
0

However, the return code from snprintf() isnt checked, and it can return -1 for a number of reasons, including malloc() failing, or invalid multibyte characters in the string. If any of the users of this application are using a utf8 locale, they could be vulnerable to a security issue.

$ LC_ALL=en_GB.utf8 ./a.out `printf "\x80foobar"`; echo $?
Segmentation fault
139

Its worth remembering that a number of common libraries will call setlocale() for you in their constructors, so even if you didnt intend to, it could be called for you. Using the return code from snprintf() or sprintf() without testing for failure could result in multiple security vulnerabilities, so its always worth checking even if you dont modify the locale.

New scanmem releaseDisputed Security Issue in Glibc

Comments

Unregistered user Sunday, June 3, 2007 12:03:02 PM

Jason writes: That's pretty interesting. I wouldn't have thought of that, although I suspect most people don't bother to check the return value of s(n)printf in any case. Just for fun, you can look on Google Codesearch: http://www.google.com/codesearch?q=%3D%5C+*snprintf

Unregistered user Thursday, July 19, 2007 1:46:34 AM

Anonymous writes: please add more security auditing puzzle to your blog. Interesting to read and learn !!

Unregistered user Monday, August 27, 2007 10:57:00 PM

Anonymous writes: The conversion specifier is "s". There is no length modifier present (eg. "l"). Thus, at least according to the Single Unix Specification, Version 3, this snprintf() call has nothing to do with the current locale. http://www.unix.org/version3/online.html The ERRORS section allows for EILSEQ, EINVAL and EOVERFLOW, in case of sprintf(). We can exclude EILSEQ (see above). EINVAL is out of question (sufficient arguments are present). EOVERFLOW is not possible here, since INT_MAX is at least 2^31 - 1. See also http://www.gnu.org/software/libc/manual/html_node/Other-Output-Conversions.html#Other-Output-Conversions (snprintf() is not a wide stream function, since it is not even a stream function (no FILE* argument)). http://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html#Formatted-Output-Functions No mention of negative return values at all (at least for non-obsolete glibc's). The only documented error is space shortage, which the code in question makes impossible. Long story short, *according to the documentation* available, the snprintf() call above *should* succeed unconditionally. It does not (I compiled the example and it crashed, indeed), which is a bug either in the implementation or in the documentation of glibc.

Unregistered user Wednesday, June 25, 2008 1:22:10 PM

fiction writes: I am interested in what libraries call setlocale() in their default constructor. I guess that instead of the setlocale(LC_ALL, ""); call in the code it is then enough to LD_PRELOAD such a .so.

Unregistered user Wednesday, October 20, 2010 4:39:45 PM

Anonymous writes: how about this: ./a.out `printf "\x80\x31\x45\xbf"`

Unregistered user Wednesday, January 12, 2011 6:14:59 AM

Анонімний writes: This is cool that people can get the credit loans and that opens up completely new opportunities.

Unregistered user Friday, September 30, 2011 1:13:43 AM

caf writes: There's another possible crash bug in that code - isprint() and toupper() are specified to take an argument that is either in the range of unsigned char or EOF, and anything else is undefined behaviour. In practice many C libraries use the provided value as an index into an array, and can crash if given a negative argument that isn't EOF. If char is signed on the platform in question, this is quite possible.

Write a comment

New comments have been disabled for this post.