Bug Fix 1857800

[Hpoonis]

Hi all,

Regarding the latest changelog:

***************

Viewer: Fix a bug that could allow maliciously crafted EPUB files to read data from files on the computer. Thanks to dozernz for discovering this attack vector.

Closes tickets: 1857800

***************

Can anyone clarify this? I was under the impression that EPUB, etc were just benign files with formatting tags. How could an EPUB (via a reader) begin scanning non-related files?

[Sarmat89]

Via external entities, but I fail to see what danger it can pose.

[Hpoonis]

Someone here suggested (quite rightly) that if there were links to 'bad actor' sites then it is these that pose the risk. Another suggested that EPUB files can contain javascript code and this might also be an issue, although I cannot find anything that hints at EPUB files containing such code.

[kovidgoyal]

The problem was in lxml https://bugs.launchpad.net/lxml/+bug/1742885

which calibre uses to parse EPUB files. It allows injection of arbitrary file content into the parsed EPUB. The parsed EPUB in turn can have javascript which is run in the viewer (in a sandbox), and that javascript can access the parsed content despite being sandoxed because it is part of the parsed book contents.

[KevinH]

Hi Kovid,

I understand the best way to workaround this with lxml is to always add the resolve_entities=False. I can do that in lxml use in Sigil and by the _lxml builder code in BS4.

Under html5, all named entities are illegal except for the basic xml ones so no issues but error out?

But under epub2 xhtml, what is the consensus best way to handle this:

1. Do we just NOT try and resolve the Entities and allow the problem case through?

or

2. Remove the custom entity definition?

If we decide to remove the custom entity definition itself, then how do we to best deal with the entity references to that custom entity?
- Should they be removed?
- Left in place but with no definition
- Should they be replaced by a placeholder string
- Should they be disarmed by removing their & and ;

Perhaps simply wrapping the custom entity declaration in an xhtml comment is enough, letting every thing else alone?

Thanks,

KevinH

[kovidgoyal]

Well, the proper solution is not resolve_entities=False but setting up a custom resolver as described here: https://lxml.de/resolvers.html

I didn't have time to do that for the previous release, something for my TODO list.

[KevinH]

Sigil has in the past had its own resolve routine in xhtml source for epub2 which assumed a simple macro-like substitution approach with no other expansion.

I will most probably handle this in epub2 xhtml by commenting out the custom entity definition and leaving the resulting named entities in place for the user to decide if they want to resolve it manually or not. Seems like the simplest and safest approach.
I have seen very very few epub2's with custom entity declarations outside of the dtd supplied ones in the wild.

Thanks

[kovidgoyal]

That should do it: https://github.com/kovidgoyal/calibr...e8b18a13187ded

[KevinH]

Thanks, that will help a lot.