cipherdyne.org

30 January, 2007

I have started using the excellent Valgrind project to ensure the proper handling of heap allocated memory within the portions of psad that are written in C (kmsgsd, and psadwatchd). The following example is from psad-2.0.3, and please note that this bug is fixed in psad-2.0.4. For reference, kmsgsd is the psad daemon that is responsible for collecting iptables log messages from a named pipe that is written to by syslog.
Assuming that valgrind is installed, and the psad-2.0.3 sources are unpacked in the local directory, the first thing is to compile kmsgsd and psadwatchd in debug mode. This is just so that neither program calls fork() to become a daemon: $ cd psad-2.0.3
$ make debug
/usr/bin/gcc -Wall -g -DDEBUG kmsgsd.c psad_funcs.c strlcpy.c strlcat.c -o kmsgsd
/usr/bin/gcc -Wall -g -DDEBUG psadwatchd.c psad_funcs.c strlcpy.c strlcat.c -o psadwatchd
Now, let us run kmsgsd (as root) underneath valgrind (some output has been abbreviated): # valgrind --leak-check=full ./kmsgsd
==424== Memcheck, a memory error detector.
==424== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
==424== Using LibVEX rev 1715, library for dynamic binary translation.
==424== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
==424== Using valgrind-3.2.2, dynamic binary instrumentation framework
. ==424== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
==424== For more details, rerun with: -v
==424==
[+] Entering DEBUG mode
[+] Firewall messages will be written to both STDOUT _and_ to fwdata.

[+] parsing config_file: /etc/psad/kmsgsd.conf

==424== Invalid write of size 1
==424== at 0x804A165: strlcpy (strlcpy.c:52)
==424== by 0x8049763: parse_fw_search_file (kmsgsd.c:407)
==424== by 0x8048C53: main (kmsgsd.c:116)
==424== Address 0x415E350 is 0 bytes after a block of size 8 alloc'd
==424== at 0x401C5F1: malloc (vg_replace_malloc.c:149)
==424== by 0x8049736: parse_fw_search_file (kmsgsd.c:405)
==424== by 0x8048C53: main (kmsgsd.c:116) So, it looks like the call to strlcpy() at kmsgsd.c line 407 is causing the problem. Indeed this code is the culprit: fw_msg_search[num_fw_search_strings] = (char *) malloc(strlen(tmp_fw_search_buf));
strlcpy(fw_msg_search[num_fw_search_strings], tmp_fw_search_buf, MAX_GEN_LEN); The problem is that the strlcpy() function uses the size parameter (MAX_GEN_LEN in this case) as the size of the destination buffer. But, the above code shows that the size of the destination buffer is not MAX_GEN_LEN and is defined instead by a call to the strlen() function. Hence the fix is as follows (note the additional of the +1 as well to accomodate the ending NULL char and use of safe_malloc() which is a malloc() wrapper function - this is coming in psad-2.0.5): fw_msg_search[num_fw_search_strings] = (char *) safe_malloc(strlen(tmp_fw_search_buf)+1);
strlcpy(fw_msg_search[num_fw_search_strings], tmp_fw_search_buf, strlen(tmp_fw_search_buf)+1); Because (to my knowledge) there has never been a crash in kmsgsd as a result of the bug above, it would have been hard to track down without a tool like valgrind.