GNOME Bugzilla – Bug 505330
Gnumeric crashes on opening Excel 97 file
Last modified: 2008-04-19 01:07:42 UTC
I got this. I will attach the file and the debug log. $ gnumeric G02hist.xls Reading file:///home/vinci/Desktop/G02hist.xls Excel 97 + warning: Can't read pathname for load map: Input/output error. warning: Can't read pathname for load map: Input/output error. Cannot access memory at address 0xcc0f4c60 Cannot access memory at address 0xcc0f4c60 (bug-buddy:3198): Gtk-CRITICAL **: gtk_text_buffer_emit_insert: assertion `g_utf8_validate (text, len, NULL)' failed
Created attachment 101525 [details] the file that crashes for me.
Created attachment 101526 [details] the debug log. hope this helps
I see the problem differently:
+ Trace 182858
Created attachment 101528 [details] [review] Patch Our sanity checks needed a little update. With the attached patch I get... (lt-gnumeric:3127): gnumeric:read-WARNING **: File is most likely corrupted. (Requested 2045430240 items of size 2, but only 144 bytes left in record. The problem occurred in excel_read_HLINK. and the file loads.
Patch needs review for 1.8.1
Rather than a macro, we can make it a function, and pass in some context so that we can generate decent warnings.
We could make it a macro, but we would have to pass in more that just context. We would need the number of bytes left too.
I made as much of it as possible a function, but I did not bother changing the error message at this point. This problem has been fixed in our software repository. The fix will go into the next software release. Thank you for your bug report. This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Created attachment 104258 [details] [review] PATCH: fixing some missing / incomplete overflow checks Hi, Short intro I'm the Fedora gnumeric package maintainer. While reviewing the patch for this currently in svn, I noticed that one check (in both the old and new form) was checking for less bytes then actually used. Up on further inspection I also found 2 missing checks, where read_utf16_str() got called without first checking if that many bytes were in the buffer. This patch fixes both these issues. Regards, Hans
Fixed, thanks. Note, that you cannot do things like XL_NEED_BYTES (len + 16 + 12 + 4); because the addition might overflow. Btw., the past week has seen a *pile* of fixes for this kind of issues. * Fix corrupted-xls-file problems. [#512984] [#513005] [#513313] [#513317] [#513361] [#513364] [#513551] [#513605] [#513608] [#513790] [#513787] [#513835] * Band-aid evaluation problem with broken xls. [#513559]
It appears that someone has reported this as a security bug. http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2008-0668 We are on the read side here. One of these things can happen: * The record size unexpectedly is zero. That generally leads to a null pointer reference which, while irritating, does not create a security risk on any platform I can imagine. * The record size is smaller than it should be, but not zero. In this case we may read bytes we should not. The result will be nonsense, but no security risk in sight. This much is true for all the bug numbers listed above. For HLINK there is an extra issue: it might be possible to send a very large length to read_utf16_str and have the *2 there overflow. If so, we will be storing string data in unpleasant places. That indeed does smell like a security problem.
The fix for this issue has been backported to gnumeric 1.6 by the Debian security team as Debian's stable release ("etch") includes gnumeric 1.6. I've taken the liberty of committing it as http://svn.gnome.org/viewvc/gnumeric?view=revision&revision=16509
Great -- a volunteer for maintaining 1.6! :-) Seriously, most of the patch you committed is actually not security issues. The added safety comes from the overflow checks, not from checking that the file has enough data -- it's no big deal if we read some random bytes from memory.
The patch for 1.6 has a minor error in the data counting. The switch statement was changed to post-increment data, so the default handler should not still have "data++" in it, I think.
Probably, but harmless. Line 3462 on http://svn.gnome.org/viewvc/gnumeric/branches/gnumeric-1-6/plugins/excel/ms-excel-read.c?r1=16509&r2=16508&pathrev=16509