diff --git a/changedetectionio/processors/__init__.py b/changedetectionio/processors/__init__.py index 7baf8708..ec8a3a28 100644 --- a/changedetectionio/processors/__init__.py +++ b/changedetectionio/processors/__init__.py @@ -5,6 +5,7 @@ import importlib import inspect import os import pkgutil +import re def find_sub_packages(package_name): """ @@ -456,6 +457,15 @@ def save_processor_config(datastore, watch_uuid, config_data): processor_name = watch.get('processor', 'text_json_diff') + # The processor name becomes a filename below, and it is not enum-validated on every + # write path (/imports/import accepts it verbatim), so treat it as untrusted: a value + # like '../../../../tmp/pwned' would otherwise escape the watch directory. + # update_extra_watch_config() also contains the path, this is the second layer. + if not re.fullmatch(r'[A-Za-z0-9_-]+', processor_name or ''): + logger.error(f"Refusing to save processor config: unsafe processor name {processor_name!r} " + f"on watch {watch_uuid}") + return False + # Create a processor instance to access config methods processor_instance = difference_detection_processor(datastore, watch_uuid) diff --git a/changedetectionio/processors/base.py b/changedetectionio/processors/base.py index f12ca64b..55f45bd9 100644 --- a/changedetectionio/processors/base.py +++ b/changedetectionio/processors/base.py @@ -300,6 +300,38 @@ class difference_detection_processor(): # After init, call run_changedetection() which will do the actual change-detection + @staticmethod + def _resolve_watch_config_path(data_dir, filename): + """Resolve `filename` inside `data_dir`, refusing anything that escapes it. + + Security: callers derive `filename` from watch['processor'] (see + processors/save_processor_config), and that value is not enum-validated on every + write path - so it must be treated as untrusted here. os.path.join() will happily + accept '../../../../tmp/pwned', which previously escaped the watch directory and + allowed an arbitrary-path JSON file write (and read) as the app user. + + Returns the absolute path, or None if it is not safely contained. + """ + import os + + if not filename or filename in ('.', '..'): + logger.error(f"Refusing unsafe watch config filename {filename!r}") + return None + + # Must be a bare filename - no directory component, no separator of either flavour + if filename != os.path.basename(filename) or '/' in filename or '\\' in filename: + logger.error(f"Refusing watch config filename with a path component: {filename!r}") + return None + + # realpath both sides so a symlink planted inside data_dir cannot redirect the write + base = os.path.realpath(data_dir) + filepath = os.path.realpath(os.path.join(base, filename)) + if os.path.dirname(filepath) != base: + logger.error(f"Refusing watch config path outside the watch directory: {filepath!r}") + return None + + return filepath + def get_extra_watch_config(self, filename): """ Read processor-specific JSON config file from watch data directory. @@ -319,7 +351,9 @@ class difference_detection_processor(): if not data_dir: return {} - filepath = os.path.join(data_dir, filename) + filepath = self._resolve_watch_config_path(data_dir, filename) + if not filepath: + return {} if not os.path.isfile(filepath): return {} @@ -353,7 +387,9 @@ class difference_detection_processor(): # Ensure directory exists watch.ensure_data_dir_exists() - filepath = os.path.join(data_dir, filename) + filepath = self._resolve_watch_config_path(data_dir, filename) + if not filepath: + return try: # If merge is enabled, read existing data first diff --git a/changedetectionio/store/__init__.py b/changedetectionio/store/__init__.py index 16031b09..4af86498 100644 --- a/changedetectionio/store/__init__.py +++ b/changedetectionio/store/__init__.py @@ -32,7 +32,7 @@ try: except ImportError: HAS_ORJSON = False -from ..processors import get_custom_watch_obj_for_processor +from ..processors import get_custom_watch_obj_for_processor, find_processors # Import the base class and helpers from .file_saving_datastore import FileSavingDataStore, load_all_watches, load_all_tags, save_json_atomic @@ -778,6 +778,19 @@ class ChangeDetectionStore(DatastoreUpdatesMixin, FileSavingDataStore): if apply_extras.get('tags'): apply_extras['tags'] = list(set(apply_extras.get('tags'))) + # 'processor' reaches here from callers that do NOT enum-validate it the way the API does: + # /imports/import passes request.values through verbatim, and the share-link path above + # takes it straight out of remote JSON. It later becomes a config filename + # (f'{processor}.json'), so an unknown value is both a data-integrity problem and how + # GHSA-mh42-m7cg-49fr escaped the watch directory. Drop it rather than store it; the + # write paths are contained too, this stops it being persisted at all. + if apply_extras.get('processor'): + known_processors = [name for _module, name in find_processors()] + if apply_extras['processor'] not in known_processors: + logger.error(f"Ignoring unknown processor {apply_extras['processor']!r} when adding " + f"'{url}' - falling back to the default. Known: {known_processors}") + del apply_extras['processor'] + # If the processor also has its own Watch implementation watch_class = get_custom_watch_obj_for_processor(apply_extras.get('processor')) new_watch = watch_class(datastore_path=self.datastore_path, __datastore=self.__data, url=url) diff --git a/changedetectionio/tests/unit/test_processor_config_path_traversal.py b/changedetectionio/tests/unit/test_processor_config_path_traversal.py new file mode 100644 index 00000000..dea83222 --- /dev/null +++ b/changedetectionio/tests/unit/test_processor_config_path_traversal.py @@ -0,0 +1,145 @@ +#!/usr/bin/env python3 + +# run from dir above changedetectionio/ dir +# python3 -m unittest changedetectionio.tests.unit.test_processor_config_path_traversal + +"""Regression tests for GHSA-mh42-m7cg-49fr - arbitrary file write via the watch `processor` field. + +A watch's `processor` is enum-validated by the API, but /imports/import puts +request.values.get('processor') into the watch verbatim. save_processor_config() then uses that +value as a filename (f'{processor}.json') and update_extra_watch_config() did +os.path.join(data_dir, filename) + open(filepath, 'w') with no containment - so a value like +'../../../../tmp/pwned' escaped the watch directory and wrote an attacker-named JSON file +anywhere the app user could reach. get_extra_watch_config() had the same traversal on read. + +Three layers are asserted here: path containment in base.py (the security boundary), the filename +sanity check in save_processor_config(), and add_watch() refusing to persist an unknown processor +at all - which also covers the share-link import path, where 'processor' arrives in JSON fetched +from a remote URL. +""" + +import os +import tempfile +import unittest + +from changedetectionio.processors.base import difference_detection_processor + + +class TestWatchConfigPathContainment(unittest.TestCase): + """The containment helper is the security boundary - every caller goes through it.""" + + def setUp(self): + self.base = tempfile.mkdtemp() + self.data_dir = os.path.join(self.base, 'datastore', 'some-watch-uuid') + os.makedirs(self.data_dir) + + def _resolve(self, filename): + return difference_detection_processor._resolve_watch_config_path(self.data_dir, filename) + + def test_normal_processor_filenames_are_allowed(self): + for filename in ('text_json_diff.json', 'restock_diff.json', 'visual_ssim_score.json'): + with self.subTest(filename=filename): + got = self._resolve(filename) + self.assertEqual(got, os.path.join(os.path.realpath(self.data_dir), filename)) + + def test_traversal_is_refused(self): + # The exact shape from the advisory PoC, plus the usual variants + attempts = ( + '../../../../../../tmp/pwned.json', + '../pwned.json', + '..', + '.', + '', + None, + '/etc/cron.d/pwned.json', + 'subdir/pwned.json', + '..\\..\\pwned.json', + ) + for filename in attempts: + with self.subTest(filename=filename): + self.assertIsNone(self._resolve(filename), + f"{filename!r} must not resolve to a writable path") + + def test_symlink_inside_the_watch_dir_cannot_redirect_the_write(self): + """A bare filename is not enough - it must still land inside the directory.""" + outside = os.path.join(self.base, 'outside.json') + link = os.path.join(self.data_dir, 'evil.json') + os.symlink(outside, link) + self.assertIsNone(self._resolve('evil.json')) + + +class TestSaveProcessorConfigRejectsUnsafeNames(unittest.TestCase): + """Second layer: the processor name is sanitised before it becomes a filename.""" + + def setUp(self): + from changedetectionio.store import ChangeDetectionStore + self.datastore_path = tempfile.mkdtemp() + self.store = ChangeDetectionStore(datastore_path=self.datastore_path, + include_default_watches=False) + + def tearDown(self): + self.store.stop_thread = True + + def test_traversing_processor_name_writes_nothing(self): + from changedetectionio.processors import save_processor_config + + uuid = self.store.add_watch(url='https://example.com/x') + + # What /imports/import allows through today (no enum check on that path) + evil_target = os.path.join(tempfile.mkdtemp(), 'pwned') + self.store.data['watching'][uuid]['processor'] = f'../../../..{evil_target}' + + ok = save_processor_config(self.store, uuid, {'marker': 'owned'}) + + # Assert the file write FIRST - that is the vulnerability itself, and on unfixed code + # this is the assertion that fires (naming the escaped path in the failure message). + self.assertFalse(os.path.exists(f'{evil_target}.json'), + f"GHSA-mh42-m7cg-49fr: wrote outside the datastore to {evil_target}.json") + self.assertFalse(ok, "save_processor_config must refuse an unsafe processor name") + + def test_legitimate_processor_name_still_saves(self): + from changedetectionio.processors import save_processor_config + + uuid = self.store.add_watch(url='https://example.com/x') + self.store.data['watching'][uuid]['processor'] = 'text_json_diff' + + self.assertTrue(save_processor_config(self.store, uuid, {'marker': 'fine'})) + + written = os.path.join(self.store.data['watching'][uuid].data_dir, 'text_json_diff.json') + self.assertTrue(os.path.isfile(written), "the legitimate config write must still happen") + + + + +class TestAddWatchRejectsUnknownProcessor(unittest.TestCase): + """Third layer: an unknown processor never gets persisted in the first place. + + add_watch() is the chokepoint for the two callers that don't enum-validate: /imports/import + (request.values verbatim) and the share-link path, which takes 'processor' out of JSON + fetched from a remote URL. + """ + + def setUp(self): + from changedetectionio.store import ChangeDetectionStore + self.datastore_path = tempfile.mkdtemp() + self.store = ChangeDetectionStore(datastore_path=self.datastore_path, + include_default_watches=False) + + def tearDown(self): + self.store.stop_thread = True + + def test_traversing_processor_is_not_stored(self): + uuid = self.store.add_watch(url='https://example.com/x', + extras={'processor': '../../../../tmp/pwned'}) + self.assertNotEqual(self.store.data['watching'][uuid].get('processor'), + '../../../../tmp/pwned', + "a path-traversing processor must not be persisted") + + def test_known_processor_is_kept(self): + uuid = self.store.add_watch(url='https://example.com/x', + extras={'processor': 'restock_diff'}) + self.assertEqual(self.store.data['watching'][uuid].get('processor'), 'restock_diff') + + +if __name__ == '__main__': + unittest.main()