From f8e587c415046616a53b3fa39eaaf6837bc29cc4 Mon Sep 17 00:00:00 2001 From: dgtlmoon Date: Sun, 29 Jan 2023 11:12:06 +0100 Subject: [PATCH] Security - Possible stored XSS in watch list - Only permit HTTP/HTTP/FTP by default - override with env var `SAFE_PROTOCOL_REGEX` (#1359) --- changedetectionio/__init__.py | 27 +++++++------- changedetectionio/api/api_v1.py | 7 +++- changedetectionio/forms.py | 5 +++ changedetectionio/model/Watch.py | 25 +++++++++++++ changedetectionio/store.py | 21 ++++++----- changedetectionio/tests/test_security.py | 47 +++++++++++++++++++----- 6 files changed, 99 insertions(+), 33 deletions(-) diff --git a/changedetectionio/__init__.py b/changedetectionio/__init__.py index d60ca816..57ecc460 100644 --- a/changedetectionio/__init__.py +++ b/changedetectionio/__init__.py @@ -74,7 +74,6 @@ app.config['TEMPLATES_AUTO_RELOAD'] = True app.jinja_env.add_extension('jinja2.ext.loopcontrols') csrf = CSRFProtect() csrf.init_app(app) - notification_debug_log=[] watch_api = Api(app, decorators=[csrf.exempt]) @@ -586,6 +585,7 @@ def changedetection_app(config=None, datastore_o=None): if request.method == 'POST' and form.validate(): + extra_update_obj = {} if request.args.get('unpause_on_save'): @@ -1133,7 +1133,8 @@ def changedetection_app(config=None, datastore_o=None): form = forms.quickWatchForm(request.form) if not form.validate(): - flash("Error") + for widget, l in form.errors.items(): + flash(','.join(l), 'error') return redirect(url_for('index')) url = request.form.get('url').strip() @@ -1144,15 +1145,14 @@ def changedetection_app(config=None, datastore_o=None): add_paused = request.form.get('edit_and_watch_submit_button') != None new_uuid = datastore.add_watch(url=url, tag=request.form.get('tag').strip(), extras={'paused': add_paused}) - - if not add_paused and new_uuid: - # Straight into the queue. - update_q.put(queuedWatchMetaData.PrioritizedItem(priority=1, item={'uuid': new_uuid})) - flash("Watch added.") - - if add_paused: - flash('Watch added in Paused state, saving will unpause.') - return redirect(url_for('edit_page', uuid=new_uuid, unpause_on_save=1)) + if new_uuid: + if add_paused: + flash('Watch added in Paused state, saving will unpause.') + return redirect(url_for('edit_page', uuid=new_uuid, unpause_on_save=1)) + else: + # Straight into the queue. + update_q.put(queuedWatchMetaData.PrioritizedItem(priority=1, item={'uuid': new_uuid})) + flash("Watch added.") return redirect(url_for('index')) @@ -1184,8 +1184,9 @@ def changedetection_app(config=None, datastore_o=None): uuid = list(datastore.data['watching'].keys()).pop() new_uuid = datastore.clone(uuid) - update_q.put(queuedWatchMetaData.PrioritizedItem(priority=5, item={'uuid': new_uuid, 'skip_when_checksum_same': True})) - flash('Cloned.') + if new_uuid: + update_q.put(queuedWatchMetaData.PrioritizedItem(priority=5, item={'uuid': new_uuid, 'skip_when_checksum_same': True})) + flash('Cloned.') return redirect(url_for('index')) diff --git a/changedetectionio/api/api_v1.py b/changedetectionio/api/api_v1.py index b43e97e9..27eb8e04 100644 --- a/changedetectionio/api/api_v1.py +++ b/changedetectionio/api/api_v1.py @@ -202,8 +202,11 @@ class CreateWatch(Resource): del extras['url'] new_uuid = self.datastore.add_watch(url=url, extras=extras) - self.update_q.put(queuedWatchMetaData.PrioritizedItem(priority=1, item={'uuid': new_uuid, 'skip_when_checksum_same': True})) - return {'uuid': new_uuid}, 201 + if new_uuid: + self.update_q.put(queuedWatchMetaData.PrioritizedItem(priority=1, item={'uuid': new_uuid, 'skip_when_checksum_same': True})) + return {'uuid': new_uuid}, 201 + else: + return "Invalid or unsupported URL", 400 @auth.check_token def get(self): diff --git a/changedetectionio/forms.py b/changedetectionio/forms.py index 8ef2112f..77223fad 100644 --- a/changedetectionio/forms.py +++ b/changedetectionio/forms.py @@ -232,12 +232,17 @@ class validateURL(object): def __call__(self, form, field): import validators + try: validators.url(field.data.strip()) except validators.ValidationFailure: message = field.gettext('\'%s\' is not a valid URL.' % (field.data.strip())) raise ValidationError(message) + from .model.Watch import is_safe_url + if not is_safe_url(field.data): + raise ValidationError('Watch protocol is not permitted by SAFE_PROTOCOL_REGEX') + class ValidateListRegex(object): """ diff --git a/changedetectionio/model/Watch.py b/changedetectionio/model/Watch.py index a8ec6fb4..f2210dc2 100644 --- a/changedetectionio/model/Watch.py +++ b/changedetectionio/model/Watch.py @@ -1,9 +1,14 @@ from distutils.util import strtobool import logging import os +import re import time import uuid +# Allowable protocols, protects against javascript: etc +# file:// is further checked by ALLOW_FILE_URI +SAFE_PROTOCOL_REGEX='^(http|https|ftp|file):' + minimum_seconds_recheck_time = int(os.getenv('MINIMUM_SECONDS_RECHECK_TIME', 60)) mtable = {'seconds': 1, 'minutes': 60, 'hours': 3600, 'days': 86400, 'weeks': 86400 * 7} @@ -55,6 +60,22 @@ base_config = { 'webdriver_js_execute_code': None, # Run before change-detection } + +def is_safe_url(test_url): + # See https://github.com/dgtlmoon/changedetection.io/issues/1358 + + # Remove 'source:' prefix so we dont get 'source:javascript:' etc + # 'source:' is a valid way to tell us to return the source + + r = re.compile(re.escape('source:'), re.IGNORECASE) + test_url = r.sub('', test_url) + + pattern = re.compile(os.getenv('SAFE_PROTOCOL_REGEX', SAFE_PROTOCOL_REGEX), re.IGNORECASE) + if not pattern.match(test_url.strip()): + return False + + return True + class model(dict): __newest_history_key = None __history_n = 0 @@ -93,7 +114,11 @@ class model(dict): @property def link(self): + url = self.get('url', '') + if not is_safe_url(url): + return 'DISABLED' + ready_url = url if '{%' in url or '{{' in url: from jinja2 import Environment diff --git a/changedetectionio/store.py b/changedetectionio/store.py index 820b5422..84e97395 100644 --- a/changedetectionio/store.py +++ b/changedetectionio/store.py @@ -1,20 +1,20 @@ from flask import ( flash ) -import json -import logging -import os -import threading -import time -import uuid as uuid_builder + +from . model import App, Watch from copy import deepcopy from os import path, unlink from threading import Lock +import json +import logging +import os import re import requests import secrets - -from . model import App, Watch +import threading +import time +import uuid as uuid_builder # Is there an existing library to ensure some data store (JSON etc) is in sync with CRUD methods? # Open a github issue if you know something :) @@ -309,9 +309,12 @@ class ChangeDetectionStore: logging.error("Error fetching metadata for shared watch link", url, str(e)) flash("Error fetching metadata for {}".format(url), 'error') return False + from .model.Watch import is_safe_url + if not is_safe_url(url): + flash('Watch protocol is not permitted by SAFE_PROTOCOL_REGEX', 'error') + return None with self.lock: - # #Re 569 new_watch = Watch.model(datastore_path=self.datastore_path, default={ 'url': url, diff --git a/changedetectionio/tests/test_security.py b/changedetectionio/tests/test_security.py index b912da04..7d0d3667 100644 --- a/changedetectionio/tests/test_security.py +++ b/changedetectionio/tests/test_security.py @@ -2,11 +2,9 @@ from flask import url_for from . util import set_original_response, set_modified_response, live_server_setup import time -def test_setup(live_server): - live_server_setup(live_server) - -def test_file_access(client, live_server): +def test_bad_access(client, live_server): + live_server_setup(live_server) res = client.post( url_for("import_page"), data={"urls": 'https://localhost'}, @@ -19,18 +17,49 @@ def test_file_access(client, live_server): res = client.post( url_for("edit_page", uuid="first"), data={ - "url": 'file:///etc/passwd', + "url": 'javascript:alert(document.domain)', "tag": "", "method": "GET", "fetch_backend": "html_requests", "body": ""}, follow_redirects=True ) - time.sleep(3) - res = client.get( - url_for("index", uuid="first"), + assert b'Watch protocol is not permitted by SAFE_PROTOCOL_REGEX' in res.data + + res = client.post( + url_for("form_quick_watch_add"), + data={"url": ' javascript:alert(123)', "tag": ''}, + follow_redirects=True + ) + + assert b'Watch protocol is not permitted by SAFE_PROTOCOL_REGEX' in res.data + + res = client.post( + url_for("form_quick_watch_add"), + data={"url": '%20%20%20javascript:alert(123)%20%20', "tag": ''}, + follow_redirects=True + ) + + assert b'Watch protocol is not permitted by SAFE_PROTOCOL_REGEX' in res.data + + + res = client.post( + url_for("form_quick_watch_add"), + data={"url": ' source:javascript:alert(document.domain)', "tag": ''}, + follow_redirects=True + ) + + assert b'Watch protocol is not permitted by SAFE_PROTOCOL_REGEX' in res.data + + # file:// is permitted by default, but it will be caught by ALLOW_FILE_URI + + client.post( + url_for("form_quick_watch_add"), + data={"url": 'file:///tasty/disk/drive', "tag": ''}, follow_redirects=True ) + time.sleep(1) + res = client.get(url_for("index")) - assert b'denied for security reasons' in res.data + assert b'file:// type access is denied for security reasons.' in res.data \ No newline at end of file