mirror of
https://github.com/jmcnamara/libxlsxwriter.git
synced 2026-05-15 14:15:54 -06:00
[GH-ISSUE #312] heap-buffer-overflow in utility.c:461:12 #251
Labels
No labels
awaiting user feedback
bug
cmake
cmake
docs
feature request
in progress
long term
medium term
medium term
pull-request
question
question
ready to close
short term
under investigation
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: github-starred/libxlsxwriter#251
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @MrBeanc on GitHub (Nov 6, 2020).
Original GitHub issue: https://github.com/jmcnamara/libxlsxwriter/issues/312
The code:
Here is my test procedure
Adding libfuzzer function when compiling libxlsxwriter
Then compile my test file
Then you can see the crash
fuzz.zip
@MrBeanc commented on GitHub (Nov 7, 2020):
I think it will cause the pointer to cross the boundary here
@jmcnamara commented on GitHub (Mar 27, 2021):
Merged into #323
@ANaumann85 commented on GitHub (Apr 5, 2021):
This issue is a bit delicate and consists of two problems:
It seems, that you rely on the null terminator of a string. For example in the file worksheet.c is a check for the end of the string using
!*string. That works only, if your string is really terminated by a 0. But if you get a zero-length array without the null-terminator, then the test does not work.The function
lxw_utf8_strlenalso relies on the zero-terminated string. If your string is not zero-terminated, the loop will not stop at the end of the string, but some where else in memory.Both problems arise, if you got wrongly terminated strings. It is not clear to me, how one should handle these errors. In my point of view, these are user errors.
@jmcnamara commented on GitHub (Apr 6, 2021):
@ANaumann85 Thanks for the analysis. I think I could add a
strnlen()-like function that would be safer. I'll look into it.@ANaumann85 commented on GitHub (Apr 6, 2021):
That looks like a good idea. Is there any feasible maximum string length? Does the xlsx specification give any hint on that?
At the moment the user passes only the pointer to the buffer, but without a length argument. One might add the buffer size as an additional argument.
But be aware, to be specific with the meaning of the size at this point. In my impression, most users have no clue about multi character encodings. So the buffer size in terms of bytes seems to be the only reasonable value.
@jmcnamara commented on GitHub (Apr 7, 2021):
Fortunately, yes. :-) All of the Excel string parameters have a size of some sort. The most comment are 32, 256 and 32K (in most cases -1B) characters. So a size check would be feasible.
I'll put together a demo branch to try it out.
@jmcnamara commented on GitHub (Apr 7, 2021):
I got the example working on a Fedora VM and had a look at it in more detail.
The issues isn't that a non-null terminated string is passed to
worksheet_write_string(). As far as I can see, the issue is a single byte object is created on the heap and then passed toworksheet_write_string()as a char*. Since it isn't null terminated thelxw_utf8_strlen()function reads past the end of the single byte into a region it shouldn't be reading. This is flagged by AddressSanitizer since it has instrumented boundary checks into the executable. However, I don't think this could cause a core dump in a non-instrumented code.As such I think this is just a "garbage in garbage out" error and not exploitable code. So I'm going to close this issue (again). If anyone had a different opinion/analysis please let me know.