Skip to content

Commit

Permalink
CVE-2018-10895: Fix CSRF issues with qute://settings/set URL
Browse files Browse the repository at this point in the history
In ffc29ee (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
  • Loading branch information
The-Compiler committed Jul 11, 2018
1 parent 3b9b2bc commit 43e58ac
Show file tree
Hide file tree
Showing 11 changed files with 181 additions and 21 deletions.
28 changes: 27 additions & 1 deletion qutebrowser/browser/qutescheme.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,17 @@
import mimetypes
import urllib
import collections
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

This comment was marked as spam.

Copy link
@bushuevky

bushuevky Dec 4, 2019

Sudoreboot

from qutebrowser.config import config, configdata, configexc, configdiff
Expand All @@ -46,6 +54,7 @@

pyeval_output = ":pyeval was never called"
spawn_output = ":spawn was never called"
csrf_token = None


_HANDLERS = {}
Expand Down Expand Up @@ -449,12 +458,29 @@ 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)

# 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)
confget=config.instance.get_str,
csrf_token=csrf_token)
return 'text/html', src


Expand Down
13 changes: 13 additions & 0 deletions qutebrowser/browser/webengine/interceptor.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

"""A request interceptor taking care of adblocking and custom headers."""

from PyQt5.QtCore import QUrl
from PyQt5.QtWebEngineCore import (QWebEngineUrlRequestInterceptor,
QWebEngineUrlRequestInfo)

Expand Down Expand Up @@ -69,6 +70,18 @@ def interceptRequest(self, info):
resource_type, navigation_type))

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(url):
Expand Down
22 changes: 21 additions & 1 deletion qutebrowser/browser/webengine/webenginequtescheme.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,28 @@ def requestStarted(self, job):
job.fail(QWebEngineUrlRequestJob.UrlInvalid)
return

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)
Expand Down
4 changes: 3 additions & 1 deletion qutebrowser/browser/webkit/network/filescheme.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
14 changes: 7 additions & 7 deletions qutebrowser/browser/webkit/network/networkmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -373,13 +373,6 @@ def createRequest(self, op, req, outgoing_data):
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(url=req.url()):
req.setRawHeader(header, value)

Expand Down Expand Up @@ -416,5 +409,12 @@ def createRequest(self, op, req, outgoing_data):
req.url().toDisplayString(),
current_url.toDisplayString()))

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)
29 changes: 24 additions & 5 deletions qutebrowser/browser/webkit/network/webkitqutescheme.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
3 changes: 2 additions & 1 deletion qutebrowser/html/settings.html
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
20 changes: 20 additions & 0 deletions tests/end2end/data/misc/qutescheme_csrf.html
Original file line number Diff line number Diff line change
@@ -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>
57 changes: 57 additions & 0 deletions tests/end2end/features/qutescheme.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions tests/end2end/test_invocations.py
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,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')
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/browser/webkit/network/test_filescheme.py
Original file line number Diff line number Diff line change
Expand Up @@ -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, '/')
Expand All @@ -259,7 +259,7 @@ def test_file(self, tmpdir):
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):
Expand All @@ -269,5 +269,5 @@ def test_unicode_encode_error(self, mocker):
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

1 comment on commit 43e58ac

@The-Compiler

This comment was marked as off-topic.

Please sign in to comment.