Auditing puzzle
Monday, 28. May 2007, 22:13:21
#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.









Anonymous # 3. June 2007, 12:03
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
Anonymous # 19. July 2007, 01:46
please add more security auditing puzzle to your blog. Interesting to read and learn !!
Anonymous # 27. August 2007, 22:57
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.
Anonymous # 25. June 2008, 13:22
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.