From c3361c31b370140f323e481dd455450b1e74c099 Mon Sep 17 00:00:00 2001
From: Florian Bruhin <git@the-compiler.org>
Date: Mon, 9 Jul 2018 23:38:47 +0200
Subject: CVE-2018-10895: Fix CSRF issues with qute://settings/set URL

In ffc29ee043ae7336d9b9dcc029a05bf7a3f994e8 (part of v1.0.0), a
qute://settings/set URL was added to change settings.

Contrary to what I apparently believed at the time, it *is* possible for
websites to access `qute://*` URLs (i.e., neither QtWebKit nor QtWebEngine
prohibit such requests, other than the usual cross-origin rules).

In other words, this means a website can e.g. have an `<img>` tag which loads a
`qute://settings/set` URL, which then sets `editor.command` to a bash script.
The result of that is arbitrary code execution.

Fixes #4060
See #2332

(cherry picked from commit 43e58ac865ff862c2008c510fc5f7627e10b4660)
---
 qutebrowser/browser/qutescheme.py                  | 34 +++++++++++--
 qutebrowser/browser/webengine/interceptor.py       | 18 ++++++-
 .../browser/webengine/webenginequtescheme.py       | 23 ++++++++-
 qutebrowser/browser/webkit/network/filescheme.py   |  4 +-
 .../browser/webkit/network/networkmanager.py       | 14 +++---
 .../browser/webkit/network/webkitqutescheme.py     | 29 +++++++++--
 qutebrowser/html/settings.html                     |  3 +-
 tests/end2end/data/misc/qutescheme_csrf.html       | 20 ++++++++
 tests/end2end/features/qutescheme.feature          | 57 ++++++++++++++++++++++
 tests/end2end/test_invocations.py                  |  6 ++-
 .../unit/browser/webkit/network/test_filescheme.py |  6 +--
 11 files changed, 189 insertions(+), 25 deletions(-)
 create mode 100644 tests/end2end/data/misc/qutescheme_csrf.html

diff --git a/qutebrowser/browser/qutescheme.py b/qutebrowser/browser/qutescheme.py
index 8866f1643..436a96e19 100644
--- a/qutebrowser/browser/qutescheme.py
+++ b/qutebrowser/browser/qutescheme.py
@@ -31,10 +31,18 @@ import textwrap
 import mimetypes
 import urllib
 import collections
+import base64
+
+try:
+    import secrets
+except ImportError:
+    # New in Python 3.6
+    secrets = None
 
 import pkg_resources
 import sip
 from PyQt5.QtCore import QUrlQuery, QUrl
+from PyQt5.QtNetwork import QNetworkReply
 
 import qutebrowser
 from qutebrowser.config import config, configdata, configexc, configdiff
@@ -45,6 +53,7 @@ from qutebrowser.misc import objects
 
 pyeval_output = ":pyeval was never called"
 spawn_output = ":spawn was never called"
+csrf_token = None
 
 
 _HANDLERS = {}
@@ -447,13 +456,30 @@ def _qute_settings_set(url):
 @add_handler('settings')
 def qute_settings(url):
     """Handler for qute://settings. View/change qute configuration."""
+    global csrf_token
+
     if url.path() == '/set':
+        if url.password() != csrf_token:
+            message.error("Invalid CSRF token for qute://settings!")
+            raise QuteSchemeError("Invalid CSRF token!",
+                                  QNetworkReply.ContentAccessDenied)
         return _qute_settings_set(url)
 
-    html = jinja.render('settings.html', title='settings',
-                        configdata=configdata,
-                        confget=config.instance.get_str)
-    return 'text/html', html
+    # Requests to qute://settings/set should only be allowed from
+    # qute://settings. As an additional security precaution, we generate a CSRF
+    # token to use here.
+    if secrets:
+        csrf_token = secrets.token_urlsafe()
+    else:
+        # On Python < 3.6, from secrets.py
+        token = base64.urlsafe_b64encode(os.urandom(32))
+        csrf_token = token.rstrip(b'=').decode('ascii')
+
+    src = jinja.render('settings.html', title='settings',
+                       configdata=configdata,
+                       confget=config.instance.get_str,
+                       csrf_token=csrf_token)
+    return 'text/html', src
 
 
 @add_handler('bindings')
diff --git a/qutebrowser/browser/webengine/interceptor.py b/qutebrowser/browser/webengine/interceptor.py
index 480e8ee85..80563f172 100644
--- a/qutebrowser/browser/webengine/interceptor.py
+++ b/qutebrowser/browser/webengine/interceptor.py
@@ -19,7 +19,9 @@
 
 """A request interceptor taking care of adblocking and custom headers."""
 
-from PyQt5.QtWebEngineCore import QWebEngineUrlRequestInterceptor
+from PyQt5.QtCore import QUrl
+from PyQt5.QtWebEngineCore import (QWebEngineUrlRequestInterceptor,
+                                   QWebEngineUrlRequestInfo)
 
 from qutebrowser.config import config
 from qutebrowser.browser import shared
@@ -54,6 +56,20 @@ class RequestInterceptor(QWebEngineUrlRequestInterceptor):
         Args:
             info: QWebEngineUrlRequestInfo &info
         """
+        url = info.requestUrl()
+        firstparty = info.firstPartyUrl()
+
+        if ((url.scheme(), url.host(), url.path()) ==
+                ('qute', 'settings', '/set')):
+            if (firstparty != QUrl('qute://settings/') or
+                    info.resourceType() !=
+                    QWebEngineUrlRequestInfo.ResourceTypeXhr):
+                log.webview.warning("Blocking malicious request from {} to {}"
+                                    .format(firstparty.toDisplayString(),
+                                            url.toDisplayString()))
+                info.block(True)
+                return
+
         # FIXME:qtwebengine only block ads for NavigationTypeOther?
         if self._host_blocker.is_blocked(info.requestUrl()):
             log.webview.info("Request to {} blocked by host blocker.".format(
diff --git a/qutebrowser/browser/webengine/webenginequtescheme.py b/qutebrowser/browser/webengine/webenginequtescheme.py
index ac583a671..04a89e2e2 100644
--- a/qutebrowser/browser/webengine/webenginequtescheme.py
+++ b/qutebrowser/browser/webengine/webenginequtescheme.py
@@ -45,8 +45,29 @@ class QuteSchemeHandler(QWebEngineUrlSchemeHandler):
             job: QWebEngineUrlRequestJob
         """
         url = job.requestUrl()
-        assert job.requestMethod() == b'GET'
+
+        # Only the browser itself or qute:// pages should access any of those
+        # URLs.
+        # The request interceptor further locks down qute://settings/set.
+        try:
+            initiator = job.initiator()
+        except AttributeError:
+            # Added in Qt 5.11
+            pass
+        else:
+            if initiator.isValid() and initiator.scheme() != 'qute':
+                log.misc.warning("Blocking malicious request from {} to {}"
+                                 .format(initiator.toDisplayString(),
+                                         url.toDisplayString()))
+                job.fail(QWebEngineUrlRequestJob.RequestDenied)
+                return
+
+        if job.requestMethod() != b'GET':
+            job.fail(QWebEngineUrlRequestJob.RequestDenied)
+            return
+
         assert url.scheme() == 'qute'
+
         log.misc.debug("Got request for {}".format(url.toDisplayString()))
         try:
             mimetype, data = qutescheme.data_for_url(url)
diff --git a/qutebrowser/browser/webkit/network/filescheme.py b/qutebrowser/browser/webkit/network/filescheme.py
index 840ed6a4a..a29674e25 100644
--- a/qutebrowser/browser/webkit/network/filescheme.py
+++ b/qutebrowser/browser/webkit/network/filescheme.py
@@ -111,11 +111,13 @@ def dirbrowser_html(path):
     return html.encode('UTF-8', errors='xmlcharrefreplace')
 
 
-def handler(request):
+def handler(request, _operation, _current_url):
     """Handler for a file:// URL.
 
     Args:
         request: QNetworkRequest to answer to.
+        _operation: The HTTP operation being done.
+        _current_url: The page we're on currently.
 
     Return:
         A QNetworkReply for directories, None for files.
diff --git a/qutebrowser/browser/webkit/network/networkmanager.py b/qutebrowser/browser/webkit/network/networkmanager.py
index 53508aaa6..a9a591b60 100644
--- a/qutebrowser/browser/webkit/network/networkmanager.py
+++ b/qutebrowser/browser/webkit/network/networkmanager.py
@@ -371,13 +371,6 @@ class NetworkManager(QNetworkAccessManager):
                     req, proxy_error, QNetworkReply.UnknownProxyError,
                     self)
 
-        scheme = req.url().scheme()
-        if scheme in self._scheme_handlers:
-            result = self._scheme_handlers[scheme](req)
-            if result is not None:
-                result.setParent(self)
-                return result
-
         for header, value in shared.custom_headers():
             req.setRawHeader(header, value)
 
@@ -406,5 +399,12 @@ class NetworkManager(QNetworkAccessManager):
                 # the webpage shutdown here.
                 current_url = QUrl()
 
+        scheme = req.url().scheme()
+        if scheme in self._scheme_handlers:
+            result = self._scheme_handlers[scheme](req, op, current_url)
+            if result is not None:
+                result.setParent(self)
+                return result
+
         self.set_referer(req, current_url)
         return super().createRequest(op, req, outgoing_data)
diff --git a/qutebrowser/browser/webkit/network/webkitqutescheme.py b/qutebrowser/browser/webkit/network/webkitqutescheme.py
index d732b6ab0..b6f99437a 100644
--- a/qutebrowser/browser/webkit/network/webkitqutescheme.py
+++ b/qutebrowser/browser/webkit/network/webkitqutescheme.py
@@ -21,27 +21,46 @@
 
 import mimetypes
 
-from PyQt5.QtNetwork import QNetworkReply
+from PyQt5.QtCore import QUrl
+from PyQt5.QtNetwork import QNetworkReply, QNetworkAccessManager
 
 from qutebrowser.browser import pdfjs, qutescheme
 from qutebrowser.browser.webkit.network import networkreply
 from qutebrowser.utils import log, usertypes, qtutils
 
 
-def handler(request):
+def handler(request, operation, current_url):
     """Scheme handler for qute:// URLs.
 
     Args:
         request: QNetworkRequest to answer to.
+        operation: The HTTP operation being done.
+        current_url: The page we're on currently.
 
     Return:
         A QNetworkReply.
     """
+    if operation != QNetworkAccessManager.GetOperation:
+        return networkreply.ErrorNetworkReply(
+            request, "Unsupported request type",
+            QNetworkReply.ContentOperationNotPermittedError)
+
+    url = request.url()
+
+    if ((url.scheme(), url.host(), url.path()) ==
+            ('qute', 'settings', '/set')):
+        if current_url != QUrl('qute://settings/'):
+            log.webview.warning("Blocking malicious request from {} to {}"
+                                .format(current_url.toDisplayString(),
+                                        url.toDisplayString()))
+            return networkreply.ErrorNetworkReply(
+                request, "Invalid qute://settings request",
+                QNetworkReply.ContentAccessDenied)
+
     try:
-        mimetype, data = qutescheme.data_for_url(request.url())
+        mimetype, data = qutescheme.data_for_url(url)
     except qutescheme.NoHandlerFound:
-        errorstr = "No handler found for {}!".format(
-            request.url().toDisplayString())
+        errorstr = "No handler found for {}!".format(url.toDisplayString())
         return networkreply.ErrorNetworkReply(
             request, errorstr, QNetworkReply.ContentNotFoundError)
     except qutescheme.QuteSchemeOSError as e:
diff --git a/qutebrowser/html/settings.html b/qutebrowser/html/settings.html
index 62b424a59..d4ff4ce34 100644
--- a/qutebrowser/html/settings.html
+++ b/qutebrowser/html/settings.html
@@ -3,7 +3,8 @@
 {% block script %}
 var cset = function(option, value) {
   // FIXME:conf we might want some error handling here?
-  var url = "qute://settings/set?option=" + encodeURIComponent(option);
+  var url = "qute://user:{{csrf_token}}@settings/set"
+  url += "?option=" + encodeURIComponent(option);
   url += "&value=" + encodeURIComponent(value);
   var xhr = new XMLHttpRequest();
   xhr.open("GET", url);
diff --git a/tests/end2end/data/misc/qutescheme_csrf.html b/tests/end2end/data/misc/qutescheme_csrf.html
new file mode 100644
index 000000000..66c8fe240
--- /dev/null
+++ b/tests/end2end/data/misc/qutescheme_csrf.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <meta charset="utf-8">
+        <title>CSRF issues with qute://settings</title>
+        <script type="text/javascript">
+            function add_img() {
+                const elem = document.createElement("img")
+                elem.src = "qute://settings/set?option=auto_save.interval&value=invalid";
+                document.body.appendChild(elem);
+            }
+        </script>
+    </head>
+    <body>
+        <form action="qute://settings/set?option=auto_save.interval&value=invalid" method="post"><button type="submit" id="via-form">Via form</button></form>
+        <input type="button" onclick="add_img()" value="Via img" id="via-img">
+        <a href="qute://settings/set?option=auto_save.interval&value=invalid" id="via-link">Via link</a>
+        <a href="/redirect-to?url=qute://settings/set%3Foption=auto_save.interval%26value=invalid" id="via-redirect">Via redirect</a>
+    </body>
+</html>
diff --git a/tests/end2end/features/qutescheme.feature b/tests/end2end/features/qutescheme.feature
index 1abaadd87..74b11b344 100644
--- a/tests/end2end/features/qutescheme.feature
+++ b/tests/end2end/features/qutescheme.feature
@@ -130,6 +130,63 @@ Feature: Special qute:// pages
         And I press the key "<Tab>"
         Then "Invalid value 'foo' *" should be logged
 
+    @qtwebkit_skip
+    Scenario: qute://settings CSRF via img (webengine)
+        When I open data/misc/qutescheme_csrf.html
+        And I run :click-element id via-img
+        Then "Blocking malicious request from http://localhost:*/data/misc/qutescheme_csrf.html to qute://settings/set?*" should be logged
+
+    @qtwebkit_skip
+    Scenario: qute://settings CSRF via link (webengine)
+        When I open data/misc/qutescheme_csrf.html
+        And I run :click-element id via-link
+        Then "Blocking malicious request from qute://settings/set?* to qute://settings/set?*" should be logged
+
+    @qtwebkit_skip
+    Scenario: qute://settings CSRF via redirect (webengine)
+        When I open data/misc/qutescheme_csrf.html
+        And I run :click-element id via-redirect
+        Then "Blocking malicious request from qute://settings/set?* to qute://settings/set?*" should be logged
+
+    @qtwebkit_skip
+    Scenario: qute://settings CSRF via form (webengine)
+        When I open data/misc/qutescheme_csrf.html
+        And I run :click-element id via-form
+        Then "Blocking malicious request from qute://settings/set?* to qute://settings/set?*" should be logged
+
+    @qtwebkit_skip
+    Scenario: qute://settings CSRF token (webengine)
+        When I open qute://settings
+        And I run :jseval const xhr = new XMLHttpRequest(); xhr.open("GET", "qute://settings/set"); xhr.send()
+        Then "Error while handling qute://* URL" should be logged
+        And the error "Invalid CSRF token for qute://settings!" should be shown
+
+    @qtwebengine_skip
+    Scenario: qute://settings CSRF via img (webkit)
+        When I open data/misc/qutescheme_csrf.html
+        And I run :click-element id via-img
+        Then "Blocking malicious request from http://localhost:*/data/misc/qutescheme_csrf.html to qute://settings/set?*" should be logged
+
+    @qtwebengine_skip
+    Scenario: qute://settings CSRF via link (webkit)
+        When I open data/misc/qutescheme_csrf.html
+        And I run :click-element id via-link
+        Then "Blocking malicious request from http://localhost:*/data/misc/qutescheme_csrf.html to qute://settings/set?*" should be logged
+        And "Error while loading qute://settings/set?*: Invalid qute://settings request" should be logged
+
+    @qtwebengine_skip
+    Scenario: qute://settings CSRF via redirect (webkit)
+        When I open data/misc/qutescheme_csrf.html
+        And I run :click-element id via-redirect
+        Then "Blocking malicious request from http://localhost:*/data/misc/qutescheme_csrf.html to qute://settings/set?*" should be logged
+        And "Error while loading qute://settings/set?*: Invalid qute://settings request" should be logged
+
+    @qtwebengine_skip
+    Scenario: qute://settings CSRF via form (webkit)
+        When I open data/misc/qutescheme_csrf.html
+        And I run :click-element id via-form
+        Then "Error while loading qute://settings/set?*: Unsupported request type" should be logged
+
     # pdfjs support
 
     @qtwebengine_skip: pdfjs is not implemented yet
diff --git a/tests/end2end/test_invocations.py b/tests/end2end/test_invocations.py
index d6b4b1300..4ee0f36de 100644
--- a/tests/end2end/test_invocations.py
+++ b/tests/end2end/test_invocations.py
@@ -366,8 +366,10 @@ def test_qute_settings_persistence(short_tmpdir, request, quteproc_new):
     """Make sure settings from qute://settings are persistent."""
     args = _base_args(request.config) + ['--basedir', str(short_tmpdir)]
     quteproc_new.start(args)
-    quteproc_new.open_path(
-        'qute://settings/set?option=search.ignore_case&value=always')
+    quteproc_new.open_path('qute://settings/')
+    quteproc_new.send_cmd(':jseval --world main '
+                          'cset("search.ignore_case", "always")')
+
     assert quteproc_new.get_setting('search.ignore_case') == 'always'
 
     quteproc_new.send_cmd(':quit')
diff --git a/tests/unit/browser/webkit/network/test_filescheme.py b/tests/unit/browser/webkit/network/test_filescheme.py
index 5bdbb47cc..2654097ea 100644
--- a/tests/unit/browser/webkit/network/test_filescheme.py
+++ b/tests/unit/browser/webkit/network/test_filescheme.py
@@ -248,7 +248,7 @@ class TestFileSchemeHandler:
     def test_dir(self, tmpdir):
         url = QUrl.fromLocalFile(str(tmpdir))
         req = QNetworkRequest(url)
-        reply = filescheme.handler(req)
+        reply = filescheme.handler(req, None, None)
         # The URL will always use /, even on Windows - so we force this here
         # too.
         tmpdir_path = str(tmpdir).replace(os.sep, '/')
@@ -259,7 +259,7 @@ class TestFileSchemeHandler:
         filename.ensure()
         url = QUrl.fromLocalFile(str(filename))
         req = QNetworkRequest(url)
-        reply = filescheme.handler(req)
+        reply = filescheme.handler(req, None, None)
         assert reply is None
 
     def test_unicode_encode_error(self, mocker):
@@ -269,5 +269,5 @@ class TestFileSchemeHandler:
         err = UnicodeEncodeError('ascii', '', 0, 2, 'foo')
         mocker.patch('os.path.isdir', side_effect=err)
 
-        reply = filescheme.handler(req)
+        reply = filescheme.handler(req, None, None)
         assert reply is None
-- 
cgit v1.2.3-54-g00ecf