Segfault caused by config file #563

Closed
josephbisch opened this Issue Oct 22, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@josephbisch
Member

josephbisch commented Oct 22, 2016

When fuzzing irssi --config @@ (where @@ represents some config file) with afl, I found a config file that causes a segfault.

The following config file is the minimized config file that afl-tmin finds.

0 0
chatnets{0""

The following is the segfault coredump backtrace from gdb.

#0  0x00007f98ec9c5903 in g_slist_last () from /usr/lib/libglib-2.0.so.0
#1  0x00007f98ec9c594f in g_slist_append () from /usr/lib/libglib-2.0.so.0
#2  0x00000000006407f5 in config_node_set_str (rec=0xe1e7d0, parent=0xe1fd50, key=0x6681a0 "type", value=0xe278a0 "IRC") at set.c:119
#3  0x00000000005cb4d0 in chatnet_read (node=0xe1fd50) at chatnets.c:146
#4  read_chatnets () at chatnets.c:174
#5  0x0000000000621c42 in signal_emit_real (rec=rec@entry=0xe20790, params=params@entry=0, va=va@entry=0x7ffcdeece208, first_hook=<optimized out>) at signals.c:242
#6  0x0000000000623fce in signal_emit (signal=signal@entry=0x66a198 "irssi init read settings", params=params@entry=0) at signals.c:286
#7  0x00000000004cd32e in fe_common_core_finish_init () at fe-common-core.c:426
#8  0x00000000004179fd in textui_finish_init () at irssi.c:197
#9  main (argc=<optimized out>, argv=<optimized out>) at irssi.c:314

The bug appears to be a duplicate of this bug, but I am reporting here, since that website seems obsolete.

I was fuzzing fb78787.

@josephbisch

This comment has been minimized.

Show comment
Hide comment
@josephbisch

josephbisch Oct 23, 2016

Member

Similarly the following triggers this.

0 0
chatnets{""0

and

0 0
chatnets{0 0
Member

josephbisch commented Oct 23, 2016

Similarly the following triggers this.

0 0
chatnets{""0

and

0 0
chatnets{0 0
@josephbisch

This comment has been minimized.

Show comment
Hide comment
@josephbisch

josephbisch Oct 25, 2016

Member

The problem appears to be caused by passing a string to g_slist_append as the first parameter instead of a GSList pointer. That leads to valgrind reporting read/write of 8 bytes when the string is only 2 bytes (it is "0").

The line that checks to see if the node is a node list fails, but just returns null and seems to just continue on without doing anything to correct the issue.

Eventually in chatnet_read, iconfig_node_set_str is called. Then in config_node_set_str, irssi tries to append to parent->value assuming it is a GSList pointer. But parent key and value are both "0".

I'm not sure how this should be handled. Obviously the format is not how the configuration file is expected to be. I guess it has to be caught at some point before the append is attempted.

Also, in my manual testing, the first line in all the above testcases (0 0) seem unnecessary to reproduce the issue.

Member

josephbisch commented Oct 25, 2016

The problem appears to be caused by passing a string to g_slist_append as the first parameter instead of a GSList pointer. That leads to valgrind reporting read/write of 8 bytes when the string is only 2 bytes (it is "0").

The line that checks to see if the node is a node list fails, but just returns null and seems to just continue on without doing anything to correct the issue.

Eventually in chatnet_read, iconfig_node_set_str is called. Then in config_node_set_str, irssi tries to append to parent->value assuming it is a GSList pointer. But parent key and value are both "0".

I'm not sure how this should be handled. Obviously the format is not how the configuration file is expected to be. I guess it has to be caught at some point before the append is attempted.

Also, in my manual testing, the first line in all the above testcases (0 0) seem unnecessary to reproduce the issue.

@ailin-nemui

This comment has been minimized.

Show comment
Hide comment
@ailin-nemui

ailin-nemui Oct 31, 2016

Contributor

Thanks for the thorough analysis, I believe we should have a g_critical in config_node_set_str similar to the existing one that ensures the correct node type of parent

Contributor

ailin-nemui commented Oct 31, 2016

Thanks for the thorough analysis, I believe we should have a g_critical in config_node_set_str similar to the existing one that ensures the correct node type of parent

@ailin-nemui ailin-nemui added the bug label Nov 1, 2016

@ailin-nemui ailin-nemui closed this in #570 Dec 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment