Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Null Pointer Dereference #53

Closed
mssalvatore opened this issue Jun 26, 2019 · 7 comments
Closed

Null Pointer Dereference #53

mssalvatore opened this issue Jun 26, 2019 · 7 comments

Comments

@mssalvatore
Copy link
Contributor

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
Copy link
Contributor

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

@kevinhendricks
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@kevinhendricks
Copy link
Contributor

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
Copy link

Ofirnir123 commented Jun 30, 2019

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 b4f4a70
Fixes the issue?

@kevinhendricks
Copy link
Contributor

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

c75c100

and

b4f4a70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants