File fix-cve-2024-22231-and-cve-2024-22232-bsc-1219430-bs.patch of Package venv-salt-minion
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
