From ff686ff7f395d83e5ac48507ecfae0b0e97a61ef 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   |  3 +-
 .../browser/webkit/network/networkmanager.py       | 14 +++---
 .../browser/webkit/network/webkitqutescheme.py     | 30 +++++++++---
 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, 188 insertions(+), 26 deletions(-)
 create mode 100644 tests/end2end/data/misc/qutescheme_csrf.html

diff --git a/qutebrowser/browser/qutescheme.py b/qutebrowser/browser/qutescheme.py
index 8bcb7ff37..d2387822f 100644
--- a/qutebrowser/browser/qutescheme.py
+++ b/qutebrowser/browser/qutescheme.py
@@ -30,9 +30,17 @@ import time
 import textwrap
 import mimetypes
 import urllib
+import base64
+
+try:
+    import secrets
+except ImportError:
+    # New in Python 3.6
+    secrets = None
 
 import pkg_resources
 from PyQt5.QtCore import QUrlQuery, QUrl
+from PyQt5.QtNetwork import QNetworkReply
 
 import qutebrowser
 from qutebrowser.config import config, configdata, configexc, configdiff
@@ -43,6 +51,7 @@ from qutebrowser.misc import objects
 
 pyeval_output = ":pyeval was never called"
 spawn_output = ":spawn was never called"
+csrf_token = None
 
 
 _HANDLERS = {}
@@ -426,13 +435,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('back')
diff --git a/qutebrowser/browser/webengine/interceptor.py b/qutebrowser/browser/webengine/interceptor.py
index e34718791..df5f32a9d 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 2e9aedd3e..03f595af1 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 a971e3257..890177808 100644
--- a/qutebrowser/browser/webkit/network/filescheme.py
+++ b/qutebrowser/browser/webkit/network/filescheme.py
@@ -115,13 +115,14 @@ class FileSchemeHandler(schemehandler.SchemeHandler):
 
     """Scheme handler for file: URLs."""
 
-    def createRequest(self, _op, request, _outgoing_data):
+    def createRequest(self, _op, request, _outgoing_data, _current_url):
         """Create a new request.
 
         Args:
              request: const QNetworkRequest & req
              _op: Operation op
              _outgoing_data: QIODevice * outgoingData
+             _current_url: The URL currently being visited.
 
         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 a19687eb1..64debed3f 100644
--- a/qutebrowser/browser/webkit/network/networkmanager.py
+++ b/qutebrowser/browser/webkit/network/networkmanager.py
@@ -384,13 +384,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].createRequest(
-                op, req, outgoing_data)
-            if result is not None:
-                return result
-
         for header, value in shared.custom_headers():
             req.setRawHeader(header, value)
 
@@ -418,5 +411,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].createRequest(
+                op, req, outgoing_data, current_url)
+            if result is not None:
+                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 5413a4a8d..236ebcc0c 100644
--- a/qutebrowser/browser/webkit/network/webkitqutescheme.py
+++ b/qutebrowser/browser/webkit/network/webkitqutescheme.py
@@ -21,7 +21,8 @@
 
 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 schemehandler, networkreply
@@ -32,22 +33,39 @@ class QuteSchemeHandler(schemehandler.SchemeHandler):
 
     """Scheme handler for qute:// URLs."""
 
-    def createRequest(self, _op, request, _outgoing_data):
+    def createRequest(self, op, request, _outgoing_data, current_url):
         """Create a new request.
 
         Args:
              request: const QNetworkRequest & req
-             _op: Operation op
+             op: Operation op
              _outgoing_data: QIODevice * outgoingData
+             current_url: The page we're on currently.
 
         Return:
             A QNetworkReply.
         """
+        if op != 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,
                 self.parent())
diff --git a/qutebrowser/html/settings.html b/qutebrowser/html/settings.html
index b370c0d91..e44cf9a7f 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 755c103e7..76b943271 100644
--- a/tests/end2end/features/qutescheme.feature
+++ b/tests/end2end/features/qutescheme.feature
@@ -128,6 +128,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 ef4808718..69a41c1a1 100644
--- a/tests/end2end/test_invocations.py
+++ b/tests/end2end/test_invocations.py
@@ -358,8 +358,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 a3f546628..659d00d4d 100644
--- a/tests/unit/browser/webkit/network/test_filescheme.py
+++ b/tests/unit/browser/webkit/network/test_filescheme.py
@@ -249,7 +249,7 @@ class TestFileSchemeHandler:
         url = QUrl.fromLocalFile(str(tmpdir))
         req = QNetworkRequest(url)
         handler = filescheme.FileSchemeHandler(win_id=0)
-        reply = handler.createRequest(None, req, None)
+        reply = handler.createRequest(None, req, None, None)
         # The URL will always use /, even on Windows - so we force this here
         # too.
         tmpdir_path = str(tmpdir).replace(os.sep, '/')
@@ -261,7 +261,7 @@ class TestFileSchemeHandler:
         url = QUrl.fromLocalFile(str(filename))
         req = QNetworkRequest(url)
         handler = filescheme.FileSchemeHandler(win_id=0)
-        reply = handler.createRequest(None, req, None)
+        reply = handler.createRequest(None, req, None, None)
         assert reply is None
 
     def test_unicode_encode_error(self, mocker):
@@ -272,5 +272,5 @@ class TestFileSchemeHandler:
         err = UnicodeEncodeError('ascii', '', 0, 2, 'foo')
         mocker.patch('os.path.isdir', side_effect=err)
 
-        reply = handler.createRequest(None, req, None)
+        reply = handler.createRequest(None, req, None, None)
         assert reply is None
-- 
cgit v1.2.3-54-g00ecf