Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
SUSE:SLE-15-SP5:GA
salt.34964
fix-cve-2024-22231-and-cve-2024-22232-bsc-12194...
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File fix-cve-2024-22231-and-cve-2024-22232-bsc-1219430-bs.patch of Package salt.34964
From 5710bc3ff3887762182f8326bd74f40d3872a69f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= <psuarezhernandez@suse.com> Date: Thu, 1 Feb 2024 11:50:16 +0000 Subject: [PATCH] Fix "CVE-2024-22231" and "CVE-2024-22232" (bsc#1219430, bsc#1219431) (#621) * Fix CVE-2024-22231 and CVE-2024-22232 * Add changelogs for CVE-2024-22231 and CVE-2024-22232 * Fix linter issue * Add credit * Fix wart in patch * Clean up test fixtures * Fix test on windows * Update changelog file name * Fix fileroots tests --------- Co-authored-by: Daniel A. Wozniak <dwozniak@vmware.com> --- changelog/565.security.md | 4 + salt/fileserver/__init__.py | 9 +- salt/fileserver/roots.py | 26 +++++ salt/master.py | 15 ++- tests/pytests/unit/fileserver/test_roots.py | 58 +++++++-- tests/pytests/unit/test_fileserver.py | 123 ++++++++++++++++++++ tests/pytests/unit/test_master.py | 33 ++++++ tests/unit/test_fileserver.py | 79 ------------- 8 files changed, 250 insertions(+), 97 deletions(-) create mode 100644 changelog/565.security.md create mode 100644 tests/pytests/unit/test_fileserver.py delete mode 100644 tests/unit/test_fileserver.py diff --git a/changelog/565.security.md b/changelog/565.security.md new file mode 100644 index 00000000000..5d7ec8202ba --- /dev/null +++ b/changelog/565.security.md @@ -0,0 +1,4 @@ +CVE-2024-22231 Prevent directory traversal when creating syndic cache directory on the master +CVE-2024-22232 Prevent directory traversal attacks in the master's serve_file method. +These vulerablities were discovered and reported by: +Yudi Zhao(Huawei Nebula Security Lab),Chenwei Jiang(Huawei Nebula Security Lab) diff --git a/salt/fileserver/__init__.py b/salt/fileserver/__init__.py index 99f12387f91..4eca98d14a4 100644 --- a/salt/fileserver/__init__.py +++ b/salt/fileserver/__init__.py @@ -568,11 +568,6 @@ class Fileserver: saltenv = salt.utils.stringutils.to_unicode(saltenv) back = self.backends(back) kwargs = {} - fnd = {"path": "", "rel": ""} - if os.path.isabs(path): - return fnd - if "../" in path: - return fnd if salt.utils.url.is_escaped(path): # don't attempt to find URL query arguments in the path path = salt.utils.url.unescape(path) @@ -588,6 +583,10 @@ class Fileserver: args = comp.split("=", 1) kwargs[args[0]] = args[1] + fnd = {"path": "", "rel": ""} + if os.path.isabs(path) or "../" in path: + return fnd + if "env" in kwargs: # "env" is not supported; Use "saltenv". kwargs.pop("env") diff --git a/salt/fileserver/roots.py b/salt/fileserver/roots.py index a02b597c6f8..e2ea92029c3 100644 --- a/salt/fileserver/roots.py +++ b/salt/fileserver/roots.py @@ -27,6 +27,7 @@ import salt.utils.hashutils import salt.utils.path import salt.utils.platform import salt.utils.stringutils +import salt.utils.verify import salt.utils.versions log = logging.getLogger(__name__) @@ -98,6 +99,11 @@ def find_file(path, saltenv="base", **kwargs): if saltenv == "__env__": root = root.replace("__env__", actual_saltenv) full = os.path.join(root, path) + + # Refuse to serve file that is not under the root. + if not salt.utils.verify.clean_path(root, full, subdir=True): + continue + if os.path.isfile(full) and not salt.fileserver.is_file_ignored(__opts__, full): fnd["path"] = full fnd["rel"] = path @@ -128,6 +134,26 @@ def serve_file(load, fnd): ret["dest"] = fnd["rel"] gzip = load.get("gzip", None) fpath = os.path.normpath(fnd["path"]) + + actual_saltenv = saltenv = load["saltenv"] + if saltenv not in __opts__["file_roots"]: + if "__env__" in __opts__["file_roots"]: + log.debug( + "salt environment '%s' maps to __env__ file_roots directory", saltenv + ) + saltenv = "__env__" + else: + return fnd + file_in_root = False + for root in __opts__["file_roots"][saltenv]: + if saltenv == "__env__": + root = root.replace("__env__", actual_saltenv) + # Refuse to serve file that is not under the root. + if salt.utils.verify.clean_path(root, fpath, subdir=True): + file_in_root = True + if not file_in_root: + return ret + with salt.utils.files.fopen(fpath, "rb") as fp_: fp_.seek(load["loc"]) data = fp_.read(__opts__["file_buffer_size"]) diff --git a/salt/master.py b/salt/master.py index 3d2ba1e29de..425b4121481 100644 --- a/salt/master.py +++ b/salt/master.py @@ -1038,7 +1038,10 @@ class MWorker(salt.utils.process.SignalHandlingProcess): """ key = payload["enc"] load = payload["load"] - ret = {"aes": self._handle_aes, "clear": self._handle_clear}[key](load) + if key == "aes": + ret = self._handle_aes(load) + else: + ret = self._handle_clear(load) raise salt.ext.tornado.gen.Return(ret) def _post_stats(self, start, cmd): @@ -1213,7 +1216,7 @@ class AESFuncs(TransportMethods): "_dir_list", "_symlink_list", "_file_envs", - "_ext_nodes", # To keep compatibility with old Salt minion versions + "_ext_nodes", # To keep compatibility with old Salt minion versions ) def __init__(self, opts, context=None): @@ -1746,10 +1749,16 @@ class AESFuncs(TransportMethods): self.mminion.returners[fstr](load["jid"], load["load"]) # Register the syndic + + # We are creating a path using user suplied input. Use the + # clean_path to prevent a directory traversal. + root = os.path.join(self.opts["cachedir"], "syndics") syndic_cache_path = os.path.join( self.opts["cachedir"], "syndics", load["id"] ) - if not os.path.exists(syndic_cache_path): + if salt.utils.verify.clean_path( + root, syndic_cache_path + ) and not os.path.exists(syndic_cache_path): path_name = os.path.split(syndic_cache_path)[0] if not os.path.exists(path_name): os.makedirs(path_name) diff --git a/tests/pytests/unit/fileserver/test_roots.py b/tests/pytests/unit/fileserver/test_roots.py index 96bceb0fd3d..c1660280bc5 100644 --- a/tests/pytests/unit/fileserver/test_roots.py +++ b/tests/pytests/unit/fileserver/test_roots.py @@ -5,6 +5,7 @@ import copy import pathlib import shutil +import sys import textwrap import pytest @@ -28,14 +29,14 @@ def unicode_dirname(): return "соль" -@pytest.fixture(autouse=True) +@pytest.fixture def testfile(tmp_path): fp = tmp_path / "testfile" fp.write_text("This is a testfile") return fp -@pytest.fixture(autouse=True) +@pytest.fixture def tmp_state_tree(tmp_path, testfile, unicode_filename, unicode_dirname): dirname = tmp_path / "roots_tmp_state_tree" dirname.mkdir(parents=True, exist_ok=True) @@ -54,11 +55,15 @@ def tmp_state_tree(tmp_path, testfile, unicode_filename, unicode_dirname): @pytest.fixture -def configure_loader_modules(tmp_state_tree, temp_salt_master): - opts = temp_salt_master.config.copy() +def testfilepath(tmp_state_tree, testfile): + return tmp_state_tree / testfile.name + + +@pytest.fixture +def configure_loader_modules(tmp_state_tree, master_opts): overrides = {"file_roots": {"base": [str(tmp_state_tree)]}} - opts.update(overrides) - return {roots: {"__opts__": opts}} + master_opts.update(overrides) + return {roots: {"__opts__": master_opts}} def test_file_list(unicode_filename): @@ -75,17 +80,17 @@ def test_find_file(tmp_state_tree): assert full_path_to_file == ret["path"] -def test_serve_file(testfile): +def test_serve_file(testfilepath): with patch.dict(roots.__opts__, {"file_buffer_size": 262144}): load = { "saltenv": "base", - "path": str(testfile), + "path": str(testfilepath), "loc": 0, } - fnd = {"path": str(testfile), "rel": "testfile"} + fnd = {"path": str(testfilepath), "rel": "testfile"} ret = roots.serve_file(load, fnd) - with salt.utils.files.fopen(str(testfile), "rb") as fp_: + with salt.utils.files.fopen(str(testfilepath), "rb") as fp_: data = fp_.read() assert ret == {"data": data, "dest": "testfile"} @@ -277,3 +282,36 @@ def test_update_mtime_map_unicode_error(tmp_path): }, "backend": "roots", } + + +def test_find_file_not_in_root(tmp_state_tree): + """ + Fileroots should never 'find' a file that is outside of it's root. + """ + badfile = pathlib.Path(tmp_state_tree).parent / "bar" + badfile.write_text("Bad file") + badpath = f"../bar" + ret = roots.find_file(badpath) + assert ret == {"path": "", "rel": ""} + badpath = f"{tmp_state_tree / '..' / 'bar'}" + ret = roots.find_file(badpath) + assert ret == {"path": "", "rel": ""} + + +def test_serve_file_not_in_root(tmp_state_tree): + """ + Fileroots should never 'serve' a file that is outside of it's root. + """ + badfile = pathlib.Path(tmp_state_tree).parent / "bar" + badfile.write_text("Bad file") + badpath = f"../bar" + load = {"path": "salt://|..\\bar", "saltenv": "base", "loc": 0} + fnd = { + "path": f"{tmp_state_tree / '..' / 'bar'}", + "rel": f"{pathlib.Path('..') / 'bar'}", + } + ret = roots.serve_file(load, fnd) + if "win" in sys.platform: + assert ret == {"data": "", "dest": "..\\bar"} + else: + assert ret == {"data": "", "dest": "../bar"} diff --git a/tests/pytests/unit/test_fileserver.py b/tests/pytests/unit/test_fileserver.py new file mode 100644 index 00000000000..8dd3ea0a27d --- /dev/null +++ b/tests/pytests/unit/test_fileserver.py @@ -0,0 +1,123 @@ +import datetime +import os +import time + +import salt.fileserver +import salt.utils.files + + +def test_diff_with_diffent_keys(): + """ + Test that different maps are indeed reported different + """ + map1 = {"file1": 1234} + map2 = {"file2": 1234} + assert salt.fileserver.diff_mtime_map(map1, map2) is True + + +def test_diff_with_diffent_values(): + """ + Test that different maps are indeed reported different + """ + map1 = {"file1": 12345} + map2 = {"file1": 1234} + assert salt.fileserver.diff_mtime_map(map1, map2) is True + + +def test_whitelist(): + opts = { + "fileserver_backend": ["roots", "git", "s3fs", "hgfs", "svn"], + "extension_modules": "", + } + fs = salt.fileserver.Fileserver(opts) + assert sorted(fs.servers.whitelist) == sorted( + ["git", "gitfs", "hg", "hgfs", "svn", "svnfs", "roots", "s3fs"] + ), fs.servers.whitelist + + +def test_future_file_list_cache_file_ignored(tmp_path): + opts = { + "fileserver_backend": ["roots"], + "cachedir": tmp_path, + "extension_modules": "", + } + + back_cachedir = os.path.join(tmp_path, "file_lists/roots") + os.makedirs(os.path.join(back_cachedir)) + + # Touch a couple files + for filename in ("base.p", "foo.txt"): + with salt.utils.files.fopen(os.path.join(back_cachedir, filename), "wb") as _f: + if filename == "base.p": + _f.write(b"\x80") + + # Set modification time to file list cache file to 1 year in the future + now = datetime.datetime.utcnow() + future = now + datetime.timedelta(days=365) + mod_time = time.mktime(future.timetuple()) + os.utime(os.path.join(back_cachedir, "base.p"), (mod_time, mod_time)) + + list_cache = os.path.join(back_cachedir, "base.p") + w_lock = os.path.join(back_cachedir, ".base.w") + ret = salt.fileserver.check_file_list_cache(opts, "files", list_cache, w_lock) + assert ( + ret[1] is True + ), "Cache file list cache file is not refreshed when future modification time" + + +def test_file_server_url_escape(tmp_path): + (tmp_path / "srv").mkdir() + (tmp_path / "srv" / "salt").mkdir() + (tmp_path / "foo").mkdir() + (tmp_path / "foo" / "bar").write_text("Bad file") + fileroot = str(tmp_path / "srv" / "salt") + badfile = str(tmp_path / "foo" / "bar") + opts = { + "fileserver_backend": ["roots"], + "extension_modules": "", + "optimization_order": [ + 0, + ], + "file_roots": { + "base": [fileroot], + }, + "file_ignore_regex": "", + "file_ignore_glob": "", + } + fs = salt.fileserver.Fileserver(opts) + ret = fs.find_file( + "salt://|..\\..\\..\\foo/bar", + "base", + ) + assert ret == {"path": "", "rel": ""} + + +def test_file_server_serve_url_escape(tmp_path): + (tmp_path / "srv").mkdir() + (tmp_path / "srv" / "salt").mkdir() + (tmp_path / "foo").mkdir() + (tmp_path / "foo" / "bar").write_text("Bad file") + fileroot = str(tmp_path / "srv" / "salt") + badfile = str(tmp_path / "foo" / "bar") + opts = { + "fileserver_backend": ["roots"], + "extension_modules": "", + "optimization_order": [ + 0, + ], + "file_roots": { + "base": [fileroot], + }, + "file_ignore_regex": "", + "file_ignore_glob": "", + "file_buffer_size": 2048, + } + fs = salt.fileserver.Fileserver(opts) + ret = fs.serve_file( + { + "path": "salt://|..\\..\\..\\foo/bar", + "saltenv": "base", + "loc": 0, + } + ) + assert ret == {"data": "", "dest": ""} diff --git a/tests/pytests/unit/test_master.py b/tests/pytests/unit/test_master.py index 98c796912aa..d338307d1f8 100644 --- a/tests/pytests/unit/test_master.py +++ b/tests/pytests/unit/test_master.py @@ -1,3 +1,4 @@ +import pathlib import time import pytest @@ -249,3 +250,35 @@ def test_mworker_pass_context(): loadler_pillars_mock.call_args_list[0][1].get("pack").get("__context__") == test_context ) + + +def test_syndic_return_cache_dir_creation(encrypted_requests): + """master's cachedir for a syndic will be created by AESFuncs._syndic_return method""" + cachedir = pathlib.Path(encrypted_requests.opts["cachedir"]) + assert not (cachedir / "syndics").exists() + encrypted_requests._syndic_return( + { + "id": "mamajama", + "jid": "", + "return": {}, + } + ) + assert (cachedir / "syndics").exists() + assert (cachedir / "syndics" / "mamajama").exists() + + +def test_syndic_return_cache_dir_creation_traversal(encrypted_requests): + """ + master's AESFuncs._syndic_return method cachdir creation is not vulnerable to a directory traversal + """ + cachedir = pathlib.Path(encrypted_requests.opts["cachedir"]) + assert not (cachedir / "syndics").exists() + encrypted_requests._syndic_return( + { + "id": "../mamajama", + "jid": "", + "return": {}, + } + ) + assert not (cachedir / "syndics").exists() + assert not (cachedir / "mamajama").exists() diff --git a/tests/unit/test_fileserver.py b/tests/unit/test_fileserver.py deleted file mode 100644 index c290b16b7e4..00000000000 --- a/tests/unit/test_fileserver.py +++ /dev/null @@ -1,79 +0,0 @@ -""" - :codeauthor: Joao Mesquita <jmesquita@sangoma.com> -""" - - -import datetime -import os -import time - -import salt.utils.files -from salt import fileserver -from tests.support.helpers import with_tempdir -from tests.support.mixins import LoaderModuleMockMixin -from tests.support.unit import TestCase - - -class MapDiffTestCase(TestCase): - def test_diff_with_diffent_keys(self): - """ - Test that different maps are indeed reported different - """ - map1 = {"file1": 1234} - map2 = {"file2": 1234} - assert fileserver.diff_mtime_map(map1, map2) is True - - def test_diff_with_diffent_values(self): - """ - Test that different maps are indeed reported different - """ - map1 = {"file1": 12345} - map2 = {"file1": 1234} - assert fileserver.diff_mtime_map(map1, map2) is True - - -class VCSBackendWhitelistCase(TestCase, LoaderModuleMockMixin): - def setup_loader_modules(self): - return {fileserver: {}} - - def test_whitelist(self): - opts = { - "fileserver_backend": ["roots", "git", "s3fs", "hgfs", "svn"], - "extension_modules": "", - } - fs = fileserver.Fileserver(opts) - assert sorted(fs.servers.whitelist) == sorted( - ["git", "gitfs", "hg", "hgfs", "svn", "svnfs", "roots", "s3fs"] - ), fs.servers.whitelist - - @with_tempdir() - def test_future_file_list_cache_file_ignored(self, cachedir): - opts = { - "fileserver_backend": ["roots"], - "cachedir": cachedir, - "extension_modules": "", - } - - back_cachedir = os.path.join(cachedir, "file_lists/roots") - os.makedirs(os.path.join(back_cachedir)) - - # Touch a couple files - for filename in ("base.p", "foo.txt"): - with salt.utils.files.fopen( - os.path.join(back_cachedir, filename), "wb" - ) as _f: - if filename == "base.p": - _f.write(b"\x80") - - # Set modification time to file list cache file to 1 year in the future - now = datetime.datetime.utcnow() - future = now + datetime.timedelta(days=365) - mod_time = time.mktime(future.timetuple()) - os.utime(os.path.join(back_cachedir, "base.p"), (mod_time, mod_time)) - - list_cache = os.path.join(back_cachedir, "base.p") - w_lock = os.path.join(back_cachedir, ".base.w") - ret = fileserver.check_file_list_cache(opts, "files", list_cache, w_lock) - assert ( - ret[1] is True - ), "Cache file list cache file is not refreshed when future modification time" -- 2.43.0
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
