GNOME Bugzilla – Bug 705866
Segfault on converting a fuzzed xls file into .gnumeric
Last modified: 2013-08-14 23:50:58 UTC
Segfault on converting a fuzzed xls file into .gnumeric. Git versions of glib, goffice, gnumeric, libgsf and libxml2. Test case: http://jutaky.com/fuzzing/gnumeric_case_27556_52373.2gnm.xls The crash doesn't occur always. Also the crash doesn't seem to occur at all under gdb. So here is valgrind's opinion on "ssconvert gnumeric_case_27556_52373.2gnm.xls out.gnumeric": Using exporter Gnumeric_XmlIO:sax ==6073== Invalid read of size 1 ==6073== at 0x8FDA377: g_utf8_offset_to_pointer (gutf8.c:354) ==6073== by 0x18569EE5: ms_container_read_markup (ms-container.c:276) ==6073== by 0x185A0816: read_pre_biff8_read_text (ms-obj.c:578) ==6073== by 0x185A12B4: ms_obj_read_pre_biff8_obj (ms-obj.c:742) ==6073== by 0x185A2890: ms_read_OBJ (ms-obj.c:1279) ==6073== by 0x18583B58: excel_read_sheet (ms-excel-read.c:6659) ==6073== by 0x185849A4: excel_read_BOF (ms-excel-read.c:6995) ==6073== by 0x185850F3: excel_read_workbook (ms-excel-read.c:7085) ==6073== by 0x18564C46: excel_enc_file_open (boot.c:193) ==6073== by 0x18564EFE: excel_file_open (boot.c:250) ==6073== by 0x5451909: go_plugin_loader_module_func_file_open (go-plugin-loader-module.c:282) ==6073== by 0x5453810: go_plugin_file_opener_open (go-plugin-service.c:685) ==6073== Address 0x18052eaa is 0 bytes after a block of size 346 alloc'd ==6073== at 0x4C2C25E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==6073== by 0x8FAA246: g_realloc (gmem.c:169) ==6073== by 0x18571446: excel_get_chars (ms-excel-read.c:1045) ==6073== by 0x185A06A5: read_pre_biff8_read_text (ms-obj.c:552) ==6073== by 0x185A12B4: ms_obj_read_pre_biff8_obj (ms-obj.c:742) ==6073== by 0x185A2890: ms_read_OBJ (ms-obj.c:1279) ==6073== by 0x18583B58: excel_read_sheet (ms-excel-read.c:6659) ==6073== by 0x185849A4: excel_read_BOF (ms-excel-read.c:6995) ==6073== by 0x185850F3: excel_read_workbook (ms-excel-read.c:7085) ==6073== by 0x18564C46: excel_enc_file_open (boot.c:193) ==6073== by 0x18564EFE: excel_file_open (boot.c:250) ==6073== by 0x5451909: go_plugin_loader_module_func_file_open (go-plugin-loader-module.c:282) ==6073== ==6073== Use of uninitialised value of size 8 ==6073== at 0x8FDA380: g_utf8_offset_to_pointer (gutf8.c:354) ==6073== by 0x18569EE5: ms_container_read_markup (ms-container.c:276) ==6073== by 0x185A0816: read_pre_biff8_read_text (ms-obj.c:578) ==6073== by 0x185A12B4: ms_obj_read_pre_biff8_obj (ms-obj.c:742) ==6073== by 0x185A2890: ms_read_OBJ (ms-obj.c:1279) ==6073== by 0x18583B58: excel_read_sheet (ms-excel-read.c:6659) ==6073== by 0x185849A4: excel_read_BOF (ms-excel-read.c:6995) ==6073== by 0x185850F3: excel_read_workbook (ms-excel-read.c:7085) ==6073== by 0x18564C46: excel_enc_file_open (boot.c:193) ==6073== by 0x18564EFE: excel_file_open (boot.c:250) ==6073== by 0x5451909: go_plugin_loader_module_func_file_open (go-plugin-loader-module.c:282) ==6073== by 0x5453810: go_plugin_file_opener_open (go-plugin-service.c:685) ==6073== -- Juha Kylmänen Research Assistant, OUSPG
The documentation for g_utf8_offset_to_pointer says: "This function doesn't abort when reaching the end of str. Therefore you should be sure that offset is within string boundaries before calling that function. Call g_utf8_strlen() when unsure." For a fuzzed file we probably cannot be sure!
In the commit 1b40e7d046956ff8810bcb4b33aa21efc8053eaf I see the check I think we should have. In commit 2feb5a210205bb450ae65336a8e7e588842606d0 that test was (incompletely) removed. ('Incompletely' since we still retained the calculation of str_len.) Supposedly that was done to "Fix reading richtext in xls objects." Morten: please have a look at this!
Allegedly this was to fix bug 579835.
Created attachment 251667 [details] [review] Tentative patch This might work. Note, that this check is different from what used to be there. We used to think the second part (then "l", now "idx") was a length to be counted from "o".
Yes. In fact your patch is what I thought we should do, and when I was trying to figure out what we used to do with sdtr_len I didn't realize the difference between that and the deleted code.
This problem has been fixed in our software repository. The fix will go into the next software release. Thank you for your bug report.