search

Sigil-Ebook / flightcrew

Automatically exported from code.google.com/p/flightcrew
GNU General Public License v3.0
34 stars 11 forks source link

Null Pointer Dereference #53

Closed mssalvatore closed 5 years ago

mssalvatore commented 5 years ago

Summary An issue was discovered in FlightCrew v0.9.2 and earlier. A NULL pointer dereference occurs in GetRelativePathToNcx() when a null pointer is passed to xc::XMLUri::isValidURI().

Details If the package.opf contains <item> elements without "href" attributes, then the variable "href" is set to an empty string on ValidateEpub.cpp:118. On ValidateEpub.cpp:121, "toX(href)" is called which returns a NULL pointer if href is empty. This pointer is then passed to xc::XMLUri::isValidURI() which dereferences it, causing a segmentation fault.

In the attached null_ptr.zip, you'll see that the href attributes in EPUB/package.opf have been replaced by the attribute "malformed".

Impact This vulnerability has very little security impact for Sigil users, but may be used as a Denial of Service attack against third-party software that uses FlightCrew as a library.

Steps to reproduce 1) Download the attached "null_ptr.zip" 2) On a linux system, process "null_ptr.zip" using flightcrew-cli. flightcrew-cli --input-file null_ptr.zip

At this point, flightcrew-cli will segfault.

null_ptr.zip

kevinhendricks commented 5 years ago

Thank you for your detailed bug report and explanation. We can detect this null and prevent the segfault.

kevinhendricks commented 5 years ago

The last commit should address this issue. So closing this as fixed in master. Please feel free to reopen the issue if you have any concerns.

mssalvatore commented 5 years ago

@kevinhendricks, the bug still persists. Previously the bug occurred in the call to GetRelativePathToNcx(), which would crash and terminate execution. Since GetRelativePathToNcx() does not crash the program after your fix, execution continues and GetRelativePathsToXhtmlDocuments() is called. This function has the same issue and crashes.

kevinhendricks commented 5 years ago

Thanks, I will look through the code to see where more fixes are needed.

kevinhendricks commented 5 years ago

Okay, added the new fix, used grep to look at toX() calls. After eliminating all calls with string literals, eliminating all calls already where null was already handled, and eliminating all calls which can accept toX() returning a null, these appear to be the only two cases.

This passes your null_ptr.zip testcase.

Ofirnir123 commented 5 years ago

Okay, added the new fix, used grep to look at toX() calls. After eliminating all calls with string literals, eliminating all calls already where null was already handled, and eliminating all calls which can accept toX() returning a null, these appear to be the only two cases.

This passes your null_ptr.zip testcase.

Hi @kevinhendricks! is this commit https://github.com/Sigil-Ebook/flightcrew/commit/b4f4a70f604ddcb4e8e343aa0e690764fc46d780 Fixes the issue?

kevinhendricks commented 5 years ago

The segfault fixes for missing required href attributes in the manifest are in two parts:

https://github.com/Sigil-Ebook/flightcrew/commit/c75c100218ed5c0e7652947051e28b54a75212ae

and

https://github.com/Sigil-Ebook/flightcrew/commit/b4f4a70f604ddcb4e8e343aa0e690764fc46d780