Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request #2794 from cobbler/fix/file-rce
Fix multiple security vulnerabilites
  • Loading branch information
SchoolGuy committed Sep 20, 2021
2 parents 36c2ba1 + 452e471 commit d8f60bb
Show file tree
Hide file tree
Showing 10 changed files with 323 additions and 57 deletions.
2 changes: 1 addition & 1 deletion 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"
Expand Down
10 changes: 6 additions & 4 deletions cobbler/api.py
Expand Up @@ -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")
Expand Down Expand Up @@ -1317,13 +1318,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")
Expand Down
142 changes: 101 additions & 41 deletions cobbler/remote.py
Expand Up @@ -21,20 +21,26 @@
import base64
import errno
import fcntl
import keyword
import logging
import os
import random
import stat
import time
import re
import xmlrpc.server
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, 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, validate_obj_id, validate_obj_name

EVENT_TIMEOUT = 7 * 24 * 60 * 60 # 1 week
CACHE_TIMEOUT = 10 * 60 # 10 minutes
Expand Down Expand Up @@ -122,8 +128,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 = {}
Expand Down Expand Up @@ -545,31 +551,33 @@ 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, user=None, token=None, name=None, object_id=None, attribute=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.
:param attribute: Additional attributes should be supplied if known.
:param debug: If the message logged is a debug message.
:param error: If the message logged is an error message.
"""
if not all((isinstance(error, bool), isinstance(debug, bool), 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)
Expand All @@ -579,13 +587,19 @@ def _log(self, msg, user=None, token=None, name=None, object_id=None, attribute=
msg = "REMOTE %s; user(%s)" % (msg, m_user)

if name is not None:
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 (isinstance(attribute, str) and attribute.isidentifier()) or keyword.iskeyword(attribute):
return
msg = "%s; attribute(%s)" % (msg, attribute)

# log to the correct logger
Expand Down Expand Up @@ -1842,6 +1856,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
Expand Down Expand Up @@ -1880,7 +1898,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.
Expand Down Expand Up @@ -2276,17 +2294,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):
Expand Down Expand Up @@ -2627,20 +2648,22 @@ 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.
: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.
: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)

Expand All @@ -2650,59 +2673,82 @@ 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 __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.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
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, file, size, offset, data):
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
else:
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 | os.O_CLOEXEC, 0o644)
# log_error("fd=%r" %fd)
try:
if offset == 0 or (offset == -1 and size == len(contents)):
Expand Down Expand Up @@ -3163,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 36 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.
Expand Down
27 changes: 17 additions & 10 deletions cobbler/tftpgen.py
Expand Up @@ -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:
Expand Down Expand Up @@ -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():
Expand All @@ -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)

Expand Down

0 comments on commit d8f60bb

Please sign in to comment.