While fuzzing irssi’s config file parser using afl, I encountered a bug that I reported and became issue #563 for irssi on GitHub. I am going to perform an analysis of the bug and my solution to the bug.
All I had to do to build irssi with afl was add CC=/path/to/afl/afl-gcc to the configure script invocation. After building irssi, I started running irssi --config file.cfg with afl, allowing afl to pass in the configuration file it generates in place of file.cfg. All that can be done by following a guide that covers how to use afl.
After running afl for a while, I ended up with a number of crashes. The input that caused the first crash minimized to the following:
0 0
chatnets{0""
Running that input through irssi outside of the context of afl yielded a coredump that I was able to give to gdb to get a backtrace.
I reported the bug, including the minimized input, the backtrace, and the commit I was fuzzing and it became issue #563.
After going through some more of the crashes I found, I ended up with two more minimized inputs that appeared to be caused by the same issue.
We will start with the backtrace:
(gdb) bt
#0 0x00007f2a7a92c693 in g_slist_last () from /usr/lib/libglib-2.0.so.0
#1 0x00007f2a7a92c6df in g_slist_append () from /usr/lib/libglib-2.0.so.0
#2 0x0000000000494d62 in config_node_set_str (rec=0x16f6790, parent=parent@entry=0x16f7ac0, key=key@entry=0x4a4100 "type", value=0x16ff490 "IRC") at set.c:119
#3 0x000000000047cf5e in chatnet_read (node=0x16f7ac0) at chatnets.c:146
#4 0x000000000047d14a in read_chatnets () at chatnets.c:174
#5 0x000000000048f083 in signal_emit_real (rec=rec@entry=0x16f84b0, params=params@entry=0, va=va@entry=0x7ffcaf583948, first_hook=<optimized out>) at signals.c:242
#6 0x000000000048f4ae in signal_emit (signal=signal@entry=0x4a617e "irssi init read settings", params=params@entry=0) at signals.c:286
#7 0x000000000043ef5f in fe_common_core_finish_init () at fe-common-core.c:426
#8 0x000000000042a29b in textui_finish_init () at irssi.c:197
#9 0x000000000042a497 in main (argc=<optimized out>, argv=<optimized out>) at irssi.c:314
Seeing that the last function called that is part of irssi itself is config_node_set_str, that looks like a good starting place.
Let’s see what that line looks like:
(gdb) list set.c:119
114 if (g_strcmp0(node->value, value) == 0)
115 return;
116 g_free(node->value);
117 } else {
118 node = g_new0(CONFIG_NODE, 1);
119 parent->value = g_slist_append(parent->value, node);
120
121 node->type = no_key ? NODE_TYPE_VALUE : NODE_TYPE_KEY;
122 node->key = no_key ? NULL : g_strdup(key);
123 }
We can see that the g_slist_append is taking in parameters of parent->value and node.
What if we print node?
(gdb) frame 2
#2 0x0000000000494d62 in config_node_set_str (rec=0x16f6790, parent=parent@entry=0x16f7ac0, key=key@entry=0x4a4100 "type", value=0x16ff490 "IRC") at set.c:119
119 parent->value = g_slist_append(parent->value, node);
(gdb) p node
$1 = (CONFIG_NODE *) 0x1795ff0
We see that node is a CONFIG_NODE pointer.
How about parent->value?
(gdb) p parent->value
$2 = (void *) 0x16f7bd0
We get a void pointer. We have no type info about the pointer. Looking at the GLib documentation, it seems that the first parameter to g_slist_append is expected to be a GSList pointer.
What if we interpret the void pointer as a GSList pointer?
(gdb) p (GSList *)(parent->value)
$3 = 0x16f7bd0 = {0x7f2a79f60030, 0x7f2a79f67c58 <main_arena+376>, 0x7f2a79f67c48 <main_arena+360>, 0x7f2a79f67c38 <main_arena+344>, 0x7f2a79f67c28 <main_arena+328>, 0x7f2a79f67c18 <main_arena+312>,
0x7f2a79f67c08 <main_arena+296>, 0x7f2a79f67bf8 <main_arena+280>, 0x7f2a79f67be8 <main_arena+264>, 0x7f2a79f67bd8 <main_arena+248>, 0x7f2a79f67bc8 <main_arena+232>, 0x7f2a79f67bb8 <main_arena+216>,
0x7f2a79f67ba8 <main_arena+200>, 0x7f2a79f67b98 <main_arena+184>, 0x7f2a79f67b88 <main_arena+168>, 0x7f2a79f67b78 <main_arena+152>, 0x7f2a79f67b68 <main_arena+136>, 0x7f2a79f67b58 <main_arena+120>,
0x7f2a79f67b48 <main_arena+104>, 0x1796120, 0x64, Cannot access memory at address 0x141
Hm, interesting. Now lets see what happens if we interpret the void pointer as a char pointer?
(gdb) p (char *)(parent->value)
$4 = 0x16f7bd0 "0"
Ah, so it seems like parent->value points to the character ‘0’ followed by a terminator.
So one solution is to make sure that parent is a list node before assuming that parent->value points to a GSList.
First we need to backtrack and see where parent is coming from:
(gdb) l chatnet_read
130 {
131 CHAT_PROTOCOL_REC *proto;
132 CHATNET_REC *rec;
133 char *type;
134
135 if (node == NULL || node->key == NULL)
136 return;
137
138 type = config_node_get_str(node, "type", NULL);
139 proto = type == NULL ? NULL : chat_protocol_find(type);
140 if (proto == NULL) {
141 proto = type == NULL ? chat_protocol_get_default() :
142 chat_protocol_get_unknown(type);
143 }
144
145 if (type == NULL)
146 iconfig_node_set_str(node, "type", proto->name);
147
148 rec = proto->create_chatnet();
149 rec->type = module_get_uniq_id("CHATNET", 0);
We see the beginning of the chatnet_read function. There is a call to iconfig_node_set_str on line 146, where node is passed as the first parameter, so it will be parent in the context of config_node_set_str. The iconfig_node_* functions just seem to actually be #defines to the corresponding config_node_* functions with the mainconfig passed in as the first parameter.
We see on line 135 a check to ensure that node != NULL and node->key != NULL before proceeding, but there isn’t a check to make sure node is a list node. So I made the following change:
diff --git a/src/core/chatnets.c b/src/core/chatnets.c
index dd4d94b..e0a7a8d 100644
--- a/src/core/chatnets.c
+++ b/src/core/chatnets.c
@@ -132,7 +132,7 @@ static void chatnet_read(CONFIG_NODE *node)
CHATNET_REC *rec;
char *type;
- if (node == NULL || node->key == NULL)
+ if (node == NULL || node->key == NULL || !is_node_list(node))
return;
type = config_node_get_str(node, "type", NULL);
That change is irssi PR #570.