File fix-cve-2024-22231-and-cve-2024-22232-bsc-12194... of Package salt

Overview Repositories Revisions Requests Users Attributes Meta

File fix-cve-2024-22231-and-cve-2024-22232-bsc-1219430-bs.patch of Package salt (Revision 7706aaae92c91f30677dbf327a0ed84c)

Currently displaying revision 7706aaae92c91f30677dbf327a0ed84c , Show latest

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

Places

File fix-cve-2024-22231-and-cve-2024-22232-bsc-1219430-bs.patch of Package salt (Revision 7706aaae92c91f30677dbf327a0ed84c)

Places