CVE-2018-7889: Deserialization of Untrusted Data (CWE-502)
value: gui2/viewer/bookmarkmanager.py in Calibre 3.18 calls cPickle.load on imported bookmark data, which allows remote attackers to execute arbitrary code via a crafted .pickle file, as demonstrated by Python code that contains an os.system call.
CWE description: The product deserializes untrusted data without sufficiently verifying that the resulting data will be valid.
CVSS v3 base score: 7.8
Published: 2018-03-08T21:29Z
Repository: https://github.com/kovidgoyal/calibre
Language: Python
Files changed: src/calibre/gui2/viewer/bookmarkmanager.py, src/calibre/srv/code.py, src/pyj/book_list/edit_metadata.pyj
Commit date: 2018-03-07 10:07:11 +0530
Commit message: E-book viewer: Change the file format used to import/export bookmarks to use JSON. This prevents malicious bookmarks files from causing code execution.

Also more work on the EM page for the server.

Diff: diff --git a/src/calibre/gui2/viewer/bookmarkmanager.py b/src/calibre/gui2/viewer/bookmarkmanager.py
index 1a1c60dc66..708976d619 100644
--- a/src/calibre/gui2/viewer/bookmarkmanager.py
+++ b/src/calibre/gui2/viewer/bookmarkmanager.py
@@ -4,11 +4,11 @@
                         print_function)
 
 __license__ = 'GPL v3'
 __copyright__ = '2013, Kovid Goyal <kovid at kovidgoyal.net>'
 
-import cPickle
+import json
 
 from PyQt5.Qt import (
     Qt, QListWidget, QListWidgetItem, QItemSelectionModel, QAction,
     QGridLayout, QPushButton, QIcon, QWidget, pyqtSignal, QLabel)
 
@@ -184,36 +184,36 @@ def pos_key(b):
         bm.sort(key=pos_key)
         self.set_bookmarks(bm)
         self.edited.emit(bm)
 
     def bm_to_item(self, bm):
-        return bytearray(cPickle.dumps(bm, -1))
+        return bm.copy()
 
     def item_to_bm(self, item):
-        return cPickle.loads(bytes(item.data(Qt.UserRole)))
+        return item.data(Qt.UserRole).copy()
 
     def get_bookmarks(self):
         return list(self)
 
     def export_bookmarks(self):
         filename = choose_save_file(
             self, 'export-viewer-bookmarks', _('Export bookmarks'),
-            filters=[(_('Saved bookmarks'), ['pickle'])], all_files=False, initial_filename='bookmarks.pickle')
+            filters=[(_('Saved bookmarks'), ['calibre-bookmarks'])], all_files=False, initial_filename='bookmarks.calibre-bookmarks')
         if filename:
-            with open(filename, 'wb') as fileobj:
-                cPickle.dump(self.get_bookmarks(), fileobj, -1)
+            with lopen(filename, 'wb') as fileobj:
+                fileobj.write(json.dumps(self.get_bookmarks(), indent=True))
 
     def import_bookmarks(self):
         files = choose_files(self, 'export-viewer-bookmarks', _('Import bookmarks'),
-            filters=[(_('Saved bookmarks'), ['pickle'])], all_files=False, select_only_single_file=True)
+            filters=[(_('Saved bookmarks'), ['calibre-bookmarks'])], all_files=False, select_only_single_file=True)
         if not files:
             return
         filename = files[0]
 
         imported = None
-        with open(filename, 'rb') as fileobj:
-            imported = cPickle.load(fileobj)
+        with lopen(filename, 'rb') as fileobj:
+            imported = json.load(fileobj)
 
         if imported is not None:
             bad = False
             try:
                 for bm in imported:
diff --git a/src/calibre/srv/code.py b/src/calibre/srv/code.py
index 4e585e24d1..96b29fec05 100644
--- a/src/calibre/srv/code.py
+++ b/src/calibre/srv/code.py
@@ -24,11 +24,11 @@
     book_as_json, categories_as_json, categories_settings, icon_map
 )
 from calibre.srv.routes import endpoint, json
 from calibre.srv.utils import get_library_data, get_use_roman
 from calibre.utils.config import prefs, tweaks
-from calibre.utils.icu import sort_key
+from calibre.utils.icu import sort_key, numeric_sort_key
 from calibre.utils.localization import get_lang
 from calibre.utils.search_query_parser import ParseException
 
 POSTABLE = frozenset({'GET', 'POST', 'HEAD'})
 
@@ -391,6 +391,6 @@ def field_names(ctx, rd, field):
     '''
     Get a list of all names for the specified field
     Optional: ?library_id=<default library>
     '''
     db, library_id = get_library_data(ctx, rd)[:2]
-    return tuple(db.all_field_names(field))
+    return tuple(sorted(db.all_field_names(field), key=numeric_sort_key))
diff --git a/src/pyj/book_list/edit_metadata.pyj b/src/pyj/book_list/edit_metadata.pyj
index 99498731b7..d4987d21a9 100644
--- a/src/pyj/book_list/edit_metadata.pyj
+++ b/src/pyj/book_list/edit_metadata.pyj
@@ -13,10 +13,11 @@
 from book_list.library_data import (
     book_metadata, current_library_id, field_names_for, library_data, load_status,
     loaded_book_ids, set_book_metadata
 )
 from book_list.router import back
+from book_list.theme import get_color
 from book_list.top_bar import create_top_bar, set_title
 from book_list.ui import set_panel_handler, show_panel
 from date import format_date
 from dom import add_extra_css, build_rule, clear, svgicon
 from modals import error_dialog
@@ -37,10 +38,15 @@
     style = basic_table_rules(sel)
     style += build_rule(sel + 'table.metadata', margin_left='1rem')
     style += build_rule(sel + 'table.metadata td', padding_bottom='0.5ex', padding_top='0.5ex', cursor='pointer')
     style += build_rule(sel + 'table.metadata tr:hover', color='red')
     style += build_rule(sel + 'table.metadata tr:active', transform='scale(1.5)')
+
+    style += build_rule(sel + '.completions', display='flex', flex_wrap='wrap', align_items='center')
+    style += build_rule(sel + '.completions > div', margin='0.5ex 0.5rem', margin_left='0', padding='0.5ex 0.5rem', border='solid 1px currentColor', border_radius='1ex', cursor='pointer')
+    style += build_rule(sel + '.completions > div:active', transform='scale(1.5)')
+    style += build_rule(sel + '.completions > div:hover', background=get_color('window-foreground'), color=get_color('window-background'))
     return style
 )
 
 
 def resolved_metadata(mi, field):
@@ -112,25 +118,87 @@ def simple_line_edit(container_id, book_id, field, fm, div, mi):
     le.focus(), le.select()
     value_to_json = def(x):
         return x
 
 
+def add_completion(container_id, name):
+    pass
+
+
+def show_completions(container_id, div, field, prefix, names):
+    clear(div)
+    completions = E.div(class_='completions')
+    div.appendChild(completions)
+    for i, name in enumerate(names):
+        completions.appendChild(E.div(name, onclick=add_completion.bind(None, container_id, name)))
+        if i >= 50:
+            break
+
+
+def update_completions(container_id, ok, field, names):
+    c = document.getElementById(container_id)
+    if not c:
+        return
+    d = c.querySelector('div[data-ctype="edit"]')
+    if not d or d.style.display is not 'block':
+        return
+    div = d.lastChild
+    clear(div)
+    if not ok:
+        err = E.div()
+        safe_set_inner_html(err, names)
+        div.appendChild(E.div(
+            _('Failed to download items for completion, with error:'), err
+        ))
+        return
+    val = d.querySelector('input').value or ''
+    val = value_to_json(val)
+    if jstype(val) is 'string':
+        prefix = val
+    else:
+        prefix = val[-1] if val.length else ''
+    if prefix is update_completions.prefix:
+        return
+    pl = prefix.toLowerCase().strip()
+    if pl:
+        if pl.startswith(update_completions.prefix.toLowerCase()):
+            matching_names = [x for x in update_completions.names if x.toLowerCase().startswith(pl)]
+        else:
+            matching_names = [x for x in names if x.toLowerCase().startswith(pl)]
+    else:
+        matching_names = []
+    update_completions.prefix = prefix
+    update_completions.names = matching_names
+    show_completions(container_id, div, field, prefix, matching_names)
+
+
+update_completions.ui_to_list = None
+update_completions.list_to_ui = None
+update_completions.names = v'[]'
+update_completions.prefix = ''
+
+
+def line_edit_updated(container_id, field):
+    field_names_for(field, update_completions.bind(None, container_id))
+
+
 def multiple_line_edit(list_to_ui, ui_to_list, container_id, book_id, field, fm, div, mi):
     nonlocal value_to_json
+    update_completions.ui_to_list = ui_to_list
+    update_completions.list_to_ui = list_to_ui
     name = fm.name or field
-    le = E.input(type='text', name=name.replace('#', '_c_'), autocomplete=True)
+    le = E.input(type='text', name=name.replace('#', '_c_'), autocomplete=True, oninput=line_edit_updated.bind(None, container_id, field))
     le.value = (resolved_metadata(mi, field) or v'[]').join(list_to_ui)
     form = create_form(le, line_edit_get_value, container_id, book_id, field)
     div.appendChild(E.div(style='margin: 0.5ex 1rem', _(
         'Edit the "{0}" below. Multiple items can be separated by {1}.').format(name, list_to_ui.strip())))
     div.appendChild(E.div(style='margin: 0.5ex 1rem', form))
-    div.appendChild(E.div(style='margin: 0.5ex 1rem'))
+    div.appendChild(E.div(E.span(_('Loading all {}...').format(name)), style='margin: 0.5ex 1rem'))
     le.focus(), le.select()
     value_to_json = def(x):
         return [a.strip() for a in x.split(ui_to_list) if a.strip()]
-    div.lastChild.appendChild(E.span(_('Loading all {}...').format(name)))
-    field_names_for(field, print)
+    field_names_for(field, update_completions.bind(None, container_id))
 
 
 def edit_field(container_id, book_id, field):
     nonlocal value_to_json
     fm = library_data.field_metadata[field]
@@ -140,10 +208,14 @@ def edit_field(container_id, book_id, field):
         return
     d = c.querySelector('div[data-ctype="edit"]')
     d.style.display = 'block'
     d.previousSibling.style.display = 'none'
     clear(d)
+    update_completions.ui_to_list = None
+    update_completions.list_to_ui = None
+    update_completions.names = v'[]'
+    update_completions.prefix = ''
     if field is 'authors':
         multiple_line_edit(' & ', '&', container_id, book_id, field, fm, d, mi)
     else:
         simple_line_edit(container_id, book_id, field, fm, d, mi)
     if field is 'title':

Vulnerable code: import cPickle
        return bytearray(cPickle.dumps(bm, -1))
        return cPickle.loads(bytes(item.data(Qt.UserRole)))
            filters=[(_('Saved bookmarks'), ['pickle'])], all_files=False, initial_filename='bookmarks.pickle')
            with open(filename, 'wb') as fileobj:
                cPickle.dump(self.get_bookmarks(), fileobj, -1)
            filters=[(_('Saved bookmarks'), ['pickle'])], all_files=False, select_only_single_file=True)
        with open(filename, 'rb') as fileobj:
            imported = cPickle.load(fileobj)
from calibre.utils.icu import sort_key
    return tuple(db.all_field_names(field))
    le = E.input(type='text', name=name.replace('#', '_c_'), autocomplete=True)
    div.appendChild(E.div(style='margin: 0.5ex 1rem'))
    div.lastChild.appendChild(E.span(_('Loading all {}...').format(name)))
    field_names_for(field, print)
Fixed code: import json
        return bm.copy()
        return item.data(Qt.UserRole).copy()
            filters=[(_('Saved bookmarks'), ['calibre-bookmarks'])], all_files=False, initial_filename='bookmarks.calibre-bookmarks')
            with lopen(filename, 'wb') as fileobj:
                fileobj.write(json.dumps(self.get_bookmarks(), indent=True))
            filters=[(_('Saved bookmarks'), ['calibre-bookmarks'])], all_files=False, select_only_single_file=True)
        with lopen(filename, 'rb') as fileobj:
            imported = json.load(fileobj)
from calibre.utils.icu import sort_key, numeric_sort_key
    return tuple(sorted(db.all_field_names(field), key=numeric_sort_key))
from book_list.theme import get_color

    style += build_rule(sel + '.completions', display='flex', flex_wrap='wrap', align_items='center')
    style += build_rule(sel + '.completions > div', margin='0.5ex 0.5rem', margin_left='0', padding='0.5ex 0.5rem', border='solid 1px currentColor', border_radius='1ex', cursor='pointer')
    style += build_rule(sel + '.completions > div:active', transform='scale(1.5)')
    style += build_rule(sel + '.completions > div:hover', background=get_color('window-foreground'), color=get_color('window-background'))
def add_completion(container_id, name):
    pass


def show_completions(container_id, div, field, prefix, names):
    clear(div)
    completions = E.div(class_='completions')
    div.appendChild(completions)
    for i, name in enumerate(names):
        completions.appendChild(E.div(name, onclick=add_completion.bind(None, container_id, name)))
        if i >= 50:
            break


def update_completions(container_id, ok, field, names):
    c = document.getElementById(container_id)
    if not c:
        return
    d = c.querySelector('div[data-ctype="edit"]')
    if not d or d.style.display is not 'block':
        return
    div = d.lastChild
    clear(div)
    if not ok:
        err = E.div()
        safe_set_inner_html(err, names)
        div.appendChild(E.div(
            _('Failed to download items for completion, with error:'), err
        ))
        return
    val = d.querySelector('input').value or ''
    val = value_to_json(val)
    if jstype(val) is 'string':
        prefix = val
    else:
        prefix = val[-1] if val.length else ''
    if prefix is update_completions.prefix:
        return
    pl = prefix.toLowerCase().strip()
    if pl:
        if pl.startswith(update_completions.prefix.toLowerCase()):
            matching_names = [x for x in update_completions.names if x.toLowerCase().startswith(pl)]
        else:
            matching_names = [x for x in names if x.toLowerCase().startswith(pl)]
    else:
        matching_names = []
    update_completions.prefix = prefix
    update_completions.names = matching_names
    show_completions(container_id, div, field, prefix, matching_names)


update_completions.ui_to_list = None
update_completions.list_to_ui = None
update_completions.names = v'[]'
update_completions.prefix = ''


def line_edit_updated(container_id, field):
    field_names_for(field, update_completions.bind(None, container_id))


    update_completions.ui_to_list = ui_to_list
    update_completions.list_to_ui = list_to_ui
    le = E.input(type='text', name=name.replace('#', '_c_'), autocomplete=True, oninput=line_edit_updated.bind(None, container_id, field))
    div.appendChild(E.div(E.span(_('Loading all {}...').format(name)), style='margin: 0.5ex 1rem'))
    field_names_for(field, update_completions.bind(None, container_id))
    update_completions.ui_to_list = None
    update_completions.list_to_ui = None
    update_completions.names = v'[]'
    update_completions.prefix = ''