Re: calibre / CVE-2018-7889
CCed to security team. Hello Security Team, Upstream feel that the fix for this is to provide a warning to the user. See: https://lists.debian.org/debian-lts/2018/04/msg00098.html (actually I can't find this warning in the code... but only a quick search so far...) However I don't think this is a real solution to the security problem. As a result I am prone to mark this no-dsa in wheezy. What do you think? -- Brian May
Re: calibre / CVE-2018-7889
Antoine Beaupré writes: > So I am wondering whether simply changing the serialization format is > the right approach after all: exported data still has executable code > and the patch is not sufficient to make arbitrary imports safe. It sounds like this patch will be some benefit, but very possibly insignificant for anybody using calibre normally. It does solve the problem for bookmarks, but not for data. > Maybe we should just make sure that imports have a popup warning as they > now do in the latest upstream. Of course this leads to warning fatigue > and is not a proper security policy, but it's upstream's choice at this > stage. > > Do we have such a warning in wheezy already? I don't think there is any warning. I am not convinced adding a warning would help either. An attacker would leave instructions with his mallacious bookmark files saying "please bypass the warnings" when installing this. No doubt some users probably would bypass the warnings without understanding the issues, as per the instructions. I believe the intention of this mechanism is to allow you to backup/restore your own data. In which case it is 100% fine. The problem occurs when using it to transfer data to/from other users (does this actually happen?) or if a user is tricked into doing so (I think I could think of easier ways of tricking users into running an exploit). My feeling is that the root cause of the problem is having the data files contain plugins. It sounds like this is the code for the plugins, not just a reference. However changing this would require upstream accept the change, and I am somewhat skeptical this is going to happen. -- Brian May
Re: calibre / CVE-2018-7889
Reviewing the upstream issue a little more, I stumbled upon this comment from upstream: https://bugs.launchpad.net/calibre/+bug/1753870/comments/7 Quote: > For export data, it is pointless, since, as I said export data > contains the entire calibre config, which in turn contains lots of > executable code including plugins. Therefore, changing the > conversion_options to not use pickle, does not actually achieve > anything, since a malicious actor can simply modify some of the other > executable code in the export instead. In other words, even if we fix bookmarks import to not use .pickle file format, the import will still load configuration files, which are Python executables. This means that the vulnerability is still present, in a different form: instead of injecting arbitrary code in bookmark exports, an attacker would simply modify the configuration files to execute arbitrary files. Upstream's response to this is to alert the user: > This has been mitigated by displaying a warning to the user informing > them of that when importing exported data. There isn't anything more > that can be done there, since exported data will always contain > executable code, so if it is tampered with, it is game over. So I am wondering whether simply changing the serialization format is the right approach after all: exported data still has executable code and the patch is not sufficient to make arbitrary imports safe. Maybe we should just make sure that imports have a popup warning as they now do in the latest upstream. Of course this leads to warning fatigue and is not a proper security policy, but it's upstream's choice at this stage. Do we have such a warning in wheezy already? A. -- We will create a civilization of the Mind in Cyberspace. May it be more humane and fair than the world your governments have made before. - John Perry Barlow
Re: calibre / CVE-2018-7889
On 2018-04-18 17:14:33, Brian May wrote: > I have a version available for testing: > https://people.debian.org/~bam/debian/pool/main/c/calibre/ > > I tried to test it myself, but I couldn't find how to start the export > bookmarks or import bookmarks functions from the UI in the short time I > had available. I'm not sure where that happens. The only thing I could find in a recent Calibre install is in "Books -> Export/import all Calibre data". Presumably that also imports bookmarks and so on. The only problem I can see with this patch is if someone made a backup of their Calibre metadata in .pickle and are trying to import it back again as JSON: this will fail, obviously. So there's clearly a backwards-compatibility concern here. Maybe we should state explicitly in the advisory that people need to re-export their backups before doing this upgrade? Otherwise the patch looks sound. A. -- Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it. - Brian W. Kernighan
Re: calibre / CVE-2018-7889
I have a version available for testing:
https://people.debian.org/~bam/debian/pool/main/c/calibre/
I tried to test it myself, but I couldn't find how to start the export
bookmarks or import bookmarks functions from the UI in the short time I
had available.
Attached is the debdiff patch.
--
Brian May
diff -Nru calibre-0.8.51+dfsg1/debian/changelog calibre-0.8.51+dfsg1/debian/changelog
--- calibre-0.8.51+dfsg1/debian/changelog 2017-03-14 17:36:04.0 +1100
+++ calibre-0.8.51+dfsg1/debian/changelog 2018-04-16 17:39:37.0 +1000
@@ -1,3 +1,9 @@
+calibre (0.8.51+dfsg1-0.1+deb7u2) wheezy-security; urgency=high
+
+ * Non-maintainer upload by the LTS Team.
+
+ -- Brian May Mon, 16 Apr 2018 17:39:37 +1000
+
calibre (0.8.51+dfsg1-0.1+deb7u1) wheezy-security; urgency=high
* Non-maintainer upload by the LTS Team.
diff -Nru calibre-0.8.51+dfsg1/debian/patches/0006-CVE-2018-7889.patch calibre-0.8.51+dfsg1/debian/patches/0006-CVE-2018-7889.patch
--- calibre-0.8.51+dfsg1/debian/patches/0006-CVE-2018-7889.patch 1970-01-01 10:00:00.0 +1000
+++ calibre-0.8.51+dfsg1/debian/patches/0006-CVE-2018-7889.patch 2018-04-16 17:38:28.0 +1000
@@ -0,0 +1,50 @@
+From: Brian May
+Date: Mon, 16 Apr 2018 17:33:39 +1000
+Subject: CVE-2018-7889
+
+Origin: https://github.com/kovidgoyal/calibre/commit/aeb5b036a0bf657951756688b3c72bd68b6e4a7d
+---
+ src/calibre/gui2/viewer/bookmarkmanager.py | 12 ++--
+ 1 file changed, 6 insertions(+), 6 deletions(-)
+
+diff --git a/src/calibre/gui2/viewer/bookmarkmanager.py b/src/calibre/gui2/viewer/bookmarkmanager.py
+index c3686bd..ce7c98a 100644
+--- a/src/calibre/gui2/viewer/bookmarkmanager.py
b/src/calibre/gui2/viewer/bookmarkmanager.py
+@@ -3,7 +3,7 @@ from __future__ import with_statement
+ __license__ = 'GPL v3'
+ __copyright__ = '2009, John Schember '
+
+-import cPickle, os
++import json, os
+
+ from PyQt4.Qt import Qt, QDialog, QAbstractTableModel, QVariant, SIGNAL, \
+ QModelIndex, QInputDialog, QLineEdit, QFileDialog
+@@ -51,22 +51,22 @@ class BookmarkManager(QDialog, Ui_BookmarkManager):
+
+ def export_bookmarks(self):
+ filename = QFileDialog.getSaveFileName(self, _("Export Bookmarks"),
+-'%s%suntitled.pickle' % (os.getcwdu(), os.sep),
+-_("Saved Bookmarks (*.pickle)"))
++'%s%suntitled.calibre-bookmarks' % (os.getcwdu(), os.sep),
++_("Saved Bookmarks (*.calibre-bookmarks)"))
+ if filename == '':
+ return
+
+ with open(filename, 'w') as fileobj:
+-cPickle.dump(self._model.bookmarks, fileobj)
++fileobj.write(json.dumps(self._model.bookmarks, indent=True))
+
+ def import_bookmarks(self):
+-filename = QFileDialog.getOpenFileName(self, _("Import Bookmarks"), '%s' % os.getcwdu(), _("Pickled Bookmarks (*.pickle)"))
++filename = QFileDialog.getOpenFileName(self, _("Import Bookmarks"), '%s' % os.getcwdu(), _("Pickled Bookmarks (*.calibre-bookmarks)"))
+ if filename == '':
+ return
+
+ imported = None
+ with open(filename, 'r') as fileobj:
+-imported = cPickle.load(fileobj)
++imported = json.load(fileobj)
+
+ if imported != None:
+ bad = False
diff -Nru calibre-0.8.51+dfsg1/debian/patches/series calibre-0.8.51+dfsg1/debian/patches/series
--- calibre-0.8.51+dfsg1/debian/patches/series 2017-03-14 17:29:33.0 +1100
+++ calibre-0.8.51+dfsg1/debian/patches/series 2018-04-16 17:39:31.0 +1000
@@ -8,3 +8,4 @@
manpages-installation.patch
disable_plugins.py
use-system-feedparser.patch
+0006-CVE-2018-7889.patch
Re: calibre / CVE-2018-7889
On 2018-04-16 17:12:03, Brian May wrote: > Antoine Beaupré writes: > >> But you're right, maybe we can just patch that out for now. It just >> seems the version in calibre is really, really old and I doubt anyone is >> actually using it. But I could be wrong! > > I am looking at this now. > https://github.com/kovidgoyal/calibre/commit/aeb5b036a0bf657951756688b3c72bd68b6e4a7d > > Antoine: Do you think any of the changes to > src/pyj/book_list/edit_metadata.pyj are required for the security fix? I > am struggling to understand how these changes relate to the rest of the > pull request. [...] > Also not really conviced the change to code.py, which appears to sort > something, is really required. > > Neither code.py or edit_metadata.pyj exist in wheezy. Those do look completely unrelated... On 2018-04-16 17:30:51, Brian May wrote: > Brian May writes: > >> The remaining file, bookmarkmanager.py fails to apply any hunks, but >> might be possible to apply some bits manually. > > Also noted that the change replaces the "open" calls with "lopen" calls, > which is just an alias for a locally defined "local_open" call (as far > as I can see). > > Guessin I probably should skip this change too... For what it's worth, this is "local_open": https://github.com/kovidgoyal/calibre/blob/565073136200e05e65e785af478339e40c2bb3eb/src/calibre/startup.py#L112 Extract: # local_open() opens a file that wont be inherited by child processes I don't see how that would change anything in this context especially since `lopen` is in a context manager, which means the file is closed before any child process could possibly be executed. A.
Re: calibre / CVE-2018-7889
Brian May writes: > The remaining file, bookmarkmanager.py fails to apply any hunks, but > might be possible to apply some bits manually. Also noted that the change replaces the "open" calls with "lopen" calls, which is just an alias for a locally defined "local_open" call (as far as I can see). Guessin I probably should skip this change too... -- Brian May
Re: calibre / CVE-2018-7889
Brian May writes: > Antoine: Do you think any of the changes to > src/pyj/book_list/edit_metadata.pyj are required for the security fix? I > am struggling to understand how these changes relate to the rest of the > pull request. Also not really conviced the change to code.py, which appears to sort something, is really required. Neither code.py or edit_metadata.pyj exist in wheezy. The remaining file, bookmarkmanager.py fails to apply any hunks, but might be possible to apply some bits manually. -- Brian May
Re: calibre / CVE-2018-7889
Antoine Beaupré writes: > But you're right, maybe we can just patch that out for now. It just > seems the version in calibre is really, really old and I doubt anyone is > actually using it. But I could be wrong! I am looking at this now. https://github.com/kovidgoyal/calibre/commit/aeb5b036a0bf657951756688b3c72bd68b6e4a7d Antoine: Do you think any of the changes to src/pyj/book_list/edit_metadata.pyj are required for the security fix? I am struggling to understand how these changes relate to the rest of the pull request. -- Brian May
Re: calibre / CVE-2018-7889
On 2018-04-12 10:17:25, Raphael Hertzog wrote: > Hi, > > On Wed, 11 Apr 2018, Antoine Beaupré wrote: >> 1. removing the package from dla-needed.txt >> 2. adding the package as unsupported in debian-security-support >> 3. (do we send end-of-life announcements to debian-lts-announce when we >> do that?) > > It's easy to mark packages as unsupported and to find many reasons to > justify this choice but we should refrain from doing so. This is > a last-resort decision and I don't think it's really warranted here. > > We have a patch for the bookmark handling, let's just apply it. The > other issue about metadata can probably be ignored because it requires > the user to upload a malicious files as metadata associated to one > of its book... and it's not like the malicious file could be hidden > as a modified cover picture or something like this. > > We're not short on time, let's handle it properly. While our sponsors > are mainly companies interested in server software, we strive to support > all packages and I have heard multiple times stories of desktop users who > are happy to continue to run what's in their old release. My concern is that this also applies to the on-disk metadata and that no one actually dared to look into that specifically. Of course, all this implies the user loads malicious metadata from the network, which is not the typical use case. But then this begs the question of what happens when you load an actual ebook from the network, and I shiver to think I'm doing that fairly often. ;) But you're right, maybe we can just patch that out for now. It just seems the version in calibre is really, really old and I doubt anyone is actually using it. But I could be wrong! A. -- Never underestimate the bandwidth of a station wagon full of tapes hurtling down the highway. - Andrew S. Tanenbaum, "Computer Networks"
Re: calibre / CVE-2018-7889
Hi, On Wed, 11 Apr 2018, Antoine Beaupré wrote: > 1. removing the package from dla-needed.txt > 2. adding the package as unsupported in debian-security-support > 3. (do we send end-of-life announcements to debian-lts-announce when we > do that?) It's easy to mark packages as unsupported and to find many reasons to justify this choice but we should refrain from doing so. This is a last-resort decision and I don't think it's really warranted here. We have a patch for the bookmark handling, let's just apply it. The other issue about metadata can probably be ignored because it requires the user to upload a malicious files as metadata associated to one of its book... and it's not like the malicious file could be hidden as a modified cover picture or something like this. We're not short on time, let's handle it properly. While our sponsors are mainly companies interested in server software, we strive to support all packages and I have heard multiple times stories of desktop users who are happy to continue to run what's in their old release. Cheers, -- Raphaël Hertzog ◈ Debian Developer Support Debian LTS: https://www.freexian.com/services/debian-lts.html Learn to master Debian: https://debian-handbook.info/get/
Re: calibre / CVE-2018-7889
Antoine Beaupré writes: > I would personnally suggest removing calibre from LTS-supported packages > completely. I'm an occasional Calibre user and I almost exclusively rely > on backports to do anything. I would assume that most people use Calibre > to talk to ebook readers (although that might not be a fair assumption), > which are frequently updated, even in older devices. Even in stretch > right now I built an unpublished backport from testing to get it talk > with my Kobo. > > So long story short, the package is not requested by sponsors and I > would be very surprised if anyone was running the actual version that is > in wheezy (0.8.51!). If anything, people on wheezy are more likely to > run the version from wheezy-backports which is also seriously outdated > (1.22, not present in any other suite). My personal (shortsighted?) view is I tend to think of packages like calibre as desktop applications, and desktop users are probably not really the target for LTS. > So I would propose: > > 1. removing the package from dla-needed.txt > > 2. adding the package as unsupported in debian-security-support > > 3. (do we send end-of-life announcements to debian-lts-announce when we > do that?) Sounds good to me. Anyone have any objections? I don't think we do 3, at least I don't remember ever seeing anything like that. > That said, I haven't looked at the details of the patch, but metadata > information is constantly rewritten by calibre. I've always considered > it was disposable data that Calibre regenerates on a whim. Ok, this eases some of my concerns. > Besides, my feeling with Calibre is that it is a security liability: it > has a fairly "interesting" history, shipping a suid helper that (if i > remember correctly) could be abused for local arbitrary code execution, > for example. I would be weary of any untrusted data input into Calibre, > in general. I'm personally looking for alternatives to manage my media > library at this point. I wasn't aware it needed a suid helper. In any case, sounds like a good argument for moving calibre to the not supported list. (if you do find a good alternative, I would be interested as well...) -- Brian May
Re: calibre / CVE-2018-7889
On 2018-04-10 17:28:26, Brian May wrote: > If I understand the upstream patch correctly, this replaces pickle with > json for bookmarks and metadata information. It looks like this patch > was applied to sid. > > Won't this break existing installs by making existing data inaccessible? > > Maybe we don't have much choice in the matter however. Any automatic > conversion tool is likely to have the same vulnerability we are > attempting to resolve. I would personnally suggest removing calibre from LTS-supported packages completely. I'm an occasional Calibre user and I almost exclusively rely on backports to do anything. I would assume that most people use Calibre to talk to ebook readers (although that might not be a fair assumption), which are frequently updated, even in older devices. Even in stretch right now I built an unpublished backport from testing to get it talk with my Kobo. So long story short, the package is not requested by sponsors and I would be very surprised if anyone was running the actual version that is in wheezy (0.8.51!). If anything, people on wheezy are more likely to run the version from wheezy-backports which is also seriously outdated (1.22, not present in any other suite). So I would propose: 1. removing the package from dla-needed.txt 2. adding the package as unsupported in debian-security-support 3. (do we send end-of-life announcements to debian-lts-announce when we do that?) That said, I haven't looked at the details of the patch, but metadata information is constantly rewritten by calibre. I've always considered it was disposable data that Calibre regenerates on a whim. Besides, my feeling with Calibre is that it is a security liability: it has a fairly "interesting" history, shipping a suid helper that (if i remember correctly) could be abused for local arbitrary code execution, for example. I would be weary of any untrusted data input into Calibre, in general. I'm personally looking for alternatives to manage my media library at this point. A. -- We build our computer (systems) the way we build our cities: over time, without a plan, on top of ruins. - Ellen Ullman
Re: calibre / CVE-2018-7889
Brian May writes: > As far as I can tell, the upstream patch for CVE-2018-7889 has changes > that aren't related to the security issue. Or it could be a fix for the > metadata.db issue, but if so I am completely confused because it doesn't > actually appear to touch the vulnerable call to cPickle. It looks like this is the fix for the metada.db issue (and other cPickle stuff removed): https://github.com/kovidgoyal/calibre/commit/9adc3b0ffb76092682bb05c8785889520ab83f22 https://github.com/kovidgoyal/calibre/commit/690698170297b7f9a0b3b515ff506605e53e3fb9 -- Brian May
Re: calibre / CVE-2018-7889
Brian May writes: > Won't this break existing installs by making existing data inaccessible? Maybe not. If I am reading the code correctly, for bookmarks this only affects imports/exports. Not the datastore for bookmarks. Possibly the same for the metadata.db data too, although as far as I can tell, CVE-2018-7889 doesn't actually cover this vulnerability. Not sure there is a CVE for this however. As far as I can tell, the upstream patch for CVE-2018-7889 has changes that aren't related to the security issue. Or it could be a fix for the metadata.db issue, but if so I am completely confused because it doesn't actually appear to touch the vulnerable call to cPickle. https://bugs.launchpad.net/calibre/+bug/1753870 https://github.com/kovidgoyal/calibre/commit/aeb5b036a0bf657951756688b3c72bd68b6e4a7d -- Brian May
