From 27a7aa133d3608fe8c25ae39125d1012c333cbfa Mon Sep 17 00:00:00 2001 From: Antoine Nguyen Date: Fri, 20 Feb 2026 15:34:45 +0100 Subject: [PATCH 1/8] Prevent OS command injection using exec_cmd() Security issue. --- modoboa/admin/api/v2/serializers.py | 2 +- modoboa/admin/app_settings.py | 4 +- modoboa/admin/jobs.py | 2 +- .../commands/subcommands/_manage_dkim_keys.py | 8 ++- modoboa/admin/models/mailbox.py | 2 +- modoboa/amavis/lib.py | 49 +++++++++++++------ modoboa/core/password_hashers/utils.py | 4 +- modoboa/lib/sysutils.py | 15 ++++-- modoboa/maillog/graphics.py | 18 +++++-- modoboa/webmail/models.py | 25 ++++++++-- 10 files changed, 89 insertions(+), 40 deletions(-) diff --git a/modoboa/admin/api/v2/serializers.py b/modoboa/admin/api/v2/serializers.py index c45d22cb9..c165305d8 100644 --- a/modoboa/admin/api/v2/serializers.py +++ b/modoboa/admin/api/v2/serializers.py @@ -274,7 +274,7 @@ class AdminGlobalParametersSerializer(serializers.Serializer): def validate_dkim_keys_storage_dir(self, value): """Check that directory exists.""" if value: - code, output = exec_cmd("which openssl") + code, output = exec_cmd("which openssl", shell=True) if code: raise serializers.ValidationError( _("openssl not found, please make sure it is installed.") diff --git a/modoboa/admin/app_settings.py b/modoboa/admin/app_settings.py index fd40589c9..2f6e9153d 100644 --- a/modoboa/admin/app_settings.py +++ b/modoboa/admin/app_settings.py @@ -298,7 +298,7 @@ def init_structure() -> dict: structure = copy.deepcopy(GLOBAL_PARAMETERS_STRUCT) hide_fields = False dpath = None - code, output = exec_cmd("which dovecot") + code, output = exec_cmd("which dovecot", shell=True) if not code: dpath = force_str(output).strip() else: @@ -312,7 +312,7 @@ def init_structure() -> dict: dpath = fpath if dpath: try: - code, version = exec_cmd(f"{dpath} --version") + code, version = exec_cmd(f"{dpath} --version", shell=True) except OSError: hide_fields = True else: diff --git a/modoboa/admin/jobs.py b/modoboa/admin/jobs.py index 4080a0fb3..f67d16e16 100644 --- a/modoboa/admin/jobs.py +++ b/modoboa/admin/jobs.py @@ -35,7 +35,7 @@ def rename_mailbox(operation): f"renaming of {operation.argument} to {new_mail_home} failed (reason: {reason})" ) return - code, output = exec_cmd(f"mv {operation.argument} {new_mail_home}") + code, output = exec_cmd(["mv", operation.argument, new_mail_home]) if code: logger.critical(f"Renaming of {new_mail_home} failed (reason: {output})") return diff --git a/modoboa/admin/management/commands/subcommands/_manage_dkim_keys.py b/modoboa/admin/management/commands/subcommands/_manage_dkim_keys.py index d817f698d..4e58a66d1 100644 --- a/modoboa/admin/management/commands/subcommands/_manage_dkim_keys.py +++ b/modoboa/admin/management/commands/subcommands/_manage_dkim_keys.py @@ -44,7 +44,9 @@ class ManageDKIMKeys(BaseCommand): if domain.dkim_key_length else self.default_key_length ) - code, output = sysutils.exec_cmd(f"openssl genrsa -out {pkey_path} {key_size}") + code, output = sysutils.exec_cmd( + ["openssl", "genrsa", "-out", pkey_path, str(key_size)] + ) if code: print( f"Failed to generate DKIM private key for domain {domain.name}: {smart_str(output)}" @@ -54,7 +56,9 @@ class ManageDKIMKeys(BaseCommand): ) return domain.dkim_private_key_path = pkey_path - code, output = sysutils.exec_cmd(f"openssl rsa -in {pkey_path} -pubout") + code, output = sysutils.exec_cmd( + ["openssl", "rsa", "-in", pkey_path, "-pubout"] + ) if code: print( f"Failed to generate DKIM public key for domain {domain.name}: {smart_str(output)}" diff --git a/modoboa/admin/models/mailbox.py b/modoboa/admin/models/mailbox.py index 3d667b790..94debfe52 100644 --- a/modoboa/admin/models/mailbox.py +++ b/modoboa/admin/models/mailbox.py @@ -147,7 +147,7 @@ class Mailbox(mixins.MessageLimitMixin, AdminObject): if not admin_params.get("handle_mailboxes"): return None if self.__mail_home is None: - code, output = doveadm_cmd(f"user -f home {self.full_address}") + code, output = doveadm_cmd(["user", "-f", "home", self.full_address]) if code: raise lib_exceptions.InternalError( _("Failed to retrieve mailbox location (%s)") % output diff --git a/modoboa/amavis/lib.py b/modoboa/amavis/lib.py index 17be1960c..0f2905e83 100644 --- a/modoboa/amavis/lib.py +++ b/modoboa/amavis/lib.py @@ -92,37 +92,28 @@ class SpamassassinClient: def __init__(self, user, recipient_db): """Constructor.""" - conf = dict(param_tools.get_global_parameters("amavis")) - self._sa_is_local = conf["sa_is_local"] - self._default_username = conf["default_user"] + self.conf = dict(param_tools.get_global_parameters("amavis")) + self._sa_is_local = self.conf["sa_is_local"] + self._default_username = self.conf["default_user"] self._recipient_db = recipient_db self._setup_cache = {} self._username_cache = [] if user.role == "SimpleUsers": - if conf["user_level_learning"]: + if self.conf["user_level_learning"]: self._username = user.email else: self._username = None self.error = None if self._sa_is_local: - self._learn_cmd = self._find_binary("sa-learn") - self._learn_cmd += " --{0} --no-sync -u {1}" self._learn_cmd_kwargs = {} self._expected_exit_codes = [0] - self._sync_cmd = self._find_binary("sa-learn") - self._sync_cmd += " -u {0} --sync" else: - self._learn_cmd = self._find_binary("spamc") - self._learn_cmd += " -d {} -p {}".format( - conf["spamd_address"], conf["spamd_port"] - ) - self._learn_cmd += " -L {0} -u {1}" self._learn_cmd_kwargs = {} self._expected_exit_codes = [5, 6] def _find_binary(self, name): """Find path to binary.""" - code, output = exec_cmd(f"which {name}") + code, output = exec_cmd(f"which {name}", shell=True) if not code: return smart_str(output).strip() known_paths = getattr(settings, "SA_LOOKUP_PATH", ("/usr/bin",)) @@ -132,6 +123,32 @@ class SpamassassinClient: return bpath raise InternalError(_("Failed to find {} binary").format(name)) + def get_learn_cmd(self, mtype: str, username: str) -> list[str]: + result = [] + if self._sa_is_local: + result += [self._find_binary("sa-learn")] + result += [f"--{mtype}", "--no-sync", "-u", username] + else: + result += [self._find_binary("spamc")] + result += [ + "-d", + self.conf["spamd_address"], + "-p", + self.conf["spamd_port"], + "-L", + mtype, + "-u", + username, + ] + return result + + def get_sync_cmd(self, username: str) -> list[str]: + result = [] + if self._sa_is_local: + result += [self._find_binary("sa-learn")] + result += ["-u", username, "--sync"] + return result + def _get_mailbox_from_rcpt(self, rcpt): """Retrieve a mailbox from a recipient address.""" local_part, domname, extension = split_mailbox(rcpt, return_extension=True) @@ -199,7 +216,7 @@ class SpamassassinClient: self._setup_cache[username] = True if username not in self._username_cache: self._username_cache.append(username) - cmd = self._learn_cmd.format(mtype, username) + cmd = self.get_learn_cmd(mtype, username) code, output = exec_cmd(cmd, pinput=smart_bytes(msg), **self._learn_cmd_kwargs) if code in self._expected_exit_codes: return True @@ -218,7 +235,7 @@ class SpamassassinClient: """Call this method at the end of the processing.""" if self._sa_is_local: for username in self._username_cache: - cmd = self._sync_cmd.format(username) + cmd = self.get_sync_cmd(username) exec_cmd(cmd, **self._learn_cmd_kwargs) diff --git a/modoboa/core/password_hashers/utils.py b/modoboa/core/password_hashers/utils.py index a852f0cba..72416ba64 100644 --- a/modoboa/core/password_hashers/utils.py +++ b/modoboa/core/password_hashers/utils.py @@ -1,4 +1,4 @@ -""" Utils for password schemes caching/retrieval. """ +"""Utils for password schemes caching/retrieval.""" from django.conf import settings from django.core.cache import cache @@ -30,7 +30,7 @@ def get_dovecot_schemes(): if not schemes: try: - retcode, schemes = doveadm_cmd("pw -l") + retcode, schemes = doveadm_cmd(["pw", "-l"]) except OSError: schemes = default_schemes status = 2 diff --git a/modoboa/lib/sysutils.py b/modoboa/lib/sysutils.py index 281d00efa..2f63b0abc 100644 --- a/modoboa/lib/sysutils.py +++ b/modoboa/lib/sysutils.py @@ -13,7 +13,13 @@ from django.conf import settings from django.utils.encoding import force_str -def exec_cmd(cmd, sudo_user=None, pinput=None, capture_output=True, **kwargs): +def exec_cmd( + cmd: str | list[str], + sudo_user: str | None = None, + pinput: str | None = None, + capture_output: bool = True, + **kwargs, +): """Execute a shell command. Run a command using the current user. Set :keyword:`sudo_user` if @@ -28,7 +34,6 @@ def exec_cmd(cmd, sudo_user=None, pinput=None, capture_output=True, **kwargs): """ if sudo_user is not None: cmd = f"sudo -u {sudo_user} {cmd}" - kwargs["shell"] = True if pinput is not None: kwargs["stdin"] = subprocess.PIPE if capture_output: @@ -43,7 +48,7 @@ def exec_cmd(cmd, sudo_user=None, pinput=None, capture_output=True, **kwargs): return process.returncode, output -def doveadm_cmd(params: str, pinput=None, capture_output=True, **kwargs): +def doveadm_cmd(params: list[str], pinput=None, capture_output=True, **kwargs): """Execute doveadm command. Run doveadm command using the current user. Set :keyword:`sudo_user` if @@ -57,7 +62,7 @@ def doveadm_cmd(params: str, pinput=None, capture_output=True, **kwargs): :return: return code, command output """ dpath = None - code, output = exec_cmd("which doveadm") + code, output = exec_cmd("which doveadm", shell=True) if not code: dpath = force_str(output).strip() else: @@ -75,7 +80,7 @@ def doveadm_cmd(params: str, pinput=None, capture_output=True, **kwargs): sudo_user = dovecot_user if curuser != dovecot_user else None if dpath: return exec_cmd( - f"{dpath} {params}", + [dpath] + params, sudo_user=sudo_user, pinput=pinput, capture_output=capture_output, diff --git a/modoboa/maillog/graphics.py b/modoboa/maillog/graphics.py index 1a632e90a..e83b34979 100644 --- a/modoboa/maillog/graphics.py +++ b/modoboa/maillog/graphics.py @@ -7,7 +7,7 @@ from itertools import chain import os from django.conf import settings -from django.utils.encoding import smart_bytes, smart_str +from django.utils.encoding import smart_str from django.utils.translation import gettext as _, gettext_lazy from modoboa.admin import models as admin_models @@ -75,7 +75,7 @@ class Graphic: def rrdtool_binary(self): """Return path to rrdtool binary.""" dpath = None - code, output = exec_cmd("which rrdtool") + code, output = exec_cmd("which rrdtool", shell=True) if not code: dpath = output.strip() else: @@ -102,9 +102,17 @@ class Graphic: cmdargs += curve.to_rrd_command_args(rrdfile) code = 0 - cmd = f"{self.rrdtool_binary} xport --json -t --start {str(start)} --end {str(end)} " - cmd += " ".join(cmdargs) - code, output = exec_cmd(smart_bytes(cmd)) + cmd = [ + self.rrdtool_binary, + "xport", + "--json", + "-t", + "--start", + str(start), + "--end", + str(end), + ] + cmdargs + code, output = exec_cmd(cmd) if code: return [] diff --git a/modoboa/webmail/models.py b/modoboa/webmail/models.py index 434bcfaf4..5a05146db 100644 --- a/modoboa/webmail/models.py +++ b/modoboa/webmail/models.py @@ -52,9 +52,17 @@ class ScheduledMessage(models.Model): # TODO: use doveadm HTTP API when dovecot is not local sent_folder = self.account.parameters.get_value("sent_folder") code, output = doveadm_cmd( - f"move -u {self.account.email} {sent_folder} " - f"mailbox {constants.MAILBOX_NAME_SCHEDULED} " - f"header {constants.CUSTOM_HEADER_SCHEDULED_ID} {self.id}" + [ + "move", + "-u", + self.account.email, + sent_folder, + "mailbox", + constants.MAILBOX_NAME_SCHEDULED, + "header", + constants.CUSTOM_HEADER_SCHEDULED_ID, + self.id, + ] ) if code: self.status = constants.SchedulingState.MOVE_ERROR.value @@ -64,8 +72,15 @@ class ScheduledMessage(models.Model): # Try to delete mailbox when empty doveadm_cmd( - f"mailbox delete -u {self.account.email} -s -e " - f"{constants.MAILBOX_NAME_SCHEDULED}" + [ + "mailbox", + "delete", + "-u", + self.account.email, + "-s", + "-e", + constants.MAILBOX_NAME_SCHEDULED, + ] ) return True From 5effff7ddf03837f223c4c45dbc7ece6726dc4b2 Mon Sep 17 00:00:00 2001 From: Antoine Nguyen Date: Fri, 20 Feb 2026 15:39:50 +0100 Subject: [PATCH 2/8] Added shell=True arg --- tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests.py b/tests.py index 36a9512bb..6a9e1a9ea 100644 --- a/tests.py +++ b/tests.py @@ -30,7 +30,7 @@ class DeployTest(unittest.TestCase): f"modoboa-admin.py deploy --collectstatic " f"--dburl {dburl} --domain localhost --admin-username admin {self.projname}" ) - code, output = exec_cmd(cmd, cwd=self.workdir) + code, output = exec_cmd(cmd, cwd=self.workdir, shell=True) self.assertEqual(code, 0) From 0d8fc455cbcb562e7173921bcd9bb700debcd174 Mon Sep 17 00:00:00 2001 From: Antoine Nguyen Date: Fri, 20 Feb 2026 22:29:03 +0100 Subject: [PATCH 3/8] Make sure all args are strings --- modoboa/amavis/lib.py | 4 ++-- modoboa/webmail/models.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modoboa/amavis/lib.py b/modoboa/amavis/lib.py index 0f2905e83..d8196da0a 100644 --- a/modoboa/amavis/lib.py +++ b/modoboa/amavis/lib.py @@ -111,7 +111,7 @@ class SpamassassinClient: self._learn_cmd_kwargs = {} self._expected_exit_codes = [5, 6] - def _find_binary(self, name): + def _find_binary(self, name: str) -> str: """Find path to binary.""" code, output = exec_cmd(f"which {name}", shell=True) if not code: @@ -134,7 +134,7 @@ class SpamassassinClient: "-d", self.conf["spamd_address"], "-p", - self.conf["spamd_port"], + str(self.conf["spamd_port"]), "-L", mtype, "-u", diff --git a/modoboa/webmail/models.py b/modoboa/webmail/models.py index 5a05146db..9e9c9f31e 100644 --- a/modoboa/webmail/models.py +++ b/modoboa/webmail/models.py @@ -61,7 +61,7 @@ class ScheduledMessage(models.Model): constants.MAILBOX_NAME_SCHEDULED, "header", constants.CUSTOM_HEADER_SCHEDULED_ID, - self.id, + str(self.id), ] ) if code: From a2cf6e4cb8002c118aba83d910e337caa15b450d Mon Sep 17 00:00:00 2001 From: Antoine Nguyen Date: Sat, 21 Feb 2026 09:25:36 +0100 Subject: [PATCH 4/8] Removed all shell=True from command executions. --- modoboa/admin/api/v2/serializers.py | 2 +- modoboa/admin/app_settings.py | 4 ++-- modoboa/amavis/lib.py | 2 +- modoboa/lib/sysutils.py | 8 ++++++-- modoboa/maillog/graphics.py | 2 +- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/modoboa/admin/api/v2/serializers.py b/modoboa/admin/api/v2/serializers.py index c165305d8..180d412ab 100644 --- a/modoboa/admin/api/v2/serializers.py +++ b/modoboa/admin/api/v2/serializers.py @@ -274,7 +274,7 @@ class AdminGlobalParametersSerializer(serializers.Serializer): def validate_dkim_keys_storage_dir(self, value): """Check that directory exists.""" if value: - code, output = exec_cmd("which openssl", shell=True) + code, output = exec_cmd(["which", "openssl"]) if code: raise serializers.ValidationError( _("openssl not found, please make sure it is installed.") diff --git a/modoboa/admin/app_settings.py b/modoboa/admin/app_settings.py index 2f6e9153d..b541ad9c4 100644 --- a/modoboa/admin/app_settings.py +++ b/modoboa/admin/app_settings.py @@ -298,7 +298,7 @@ def init_structure() -> dict: structure = copy.deepcopy(GLOBAL_PARAMETERS_STRUCT) hide_fields = False dpath = None - code, output = exec_cmd("which dovecot", shell=True) + code, output = exec_cmd(["which", "dovecot"]) if not code: dpath = force_str(output).strip() else: @@ -312,7 +312,7 @@ def init_structure() -> dict: dpath = fpath if dpath: try: - code, version = exec_cmd(f"{dpath} --version", shell=True) + code, version = exec_cmd([dpath, "--version"]) except OSError: hide_fields = True else: diff --git a/modoboa/amavis/lib.py b/modoboa/amavis/lib.py index d8196da0a..c126e9b8f 100644 --- a/modoboa/amavis/lib.py +++ b/modoboa/amavis/lib.py @@ -113,7 +113,7 @@ class SpamassassinClient: def _find_binary(self, name: str) -> str: """Find path to binary.""" - code, output = exec_cmd(f"which {name}", shell=True) + code, output = exec_cmd(["which", name]) if not code: return smart_str(output).strip() known_paths = getattr(settings, "SA_LOOKUP_PATH", ("/usr/bin",)) diff --git a/modoboa/lib/sysutils.py b/modoboa/lib/sysutils.py index 2f63b0abc..7eb665493 100644 --- a/modoboa/lib/sysutils.py +++ b/modoboa/lib/sysutils.py @@ -33,7 +33,11 @@ def exec_cmd( :return: return code, command output """ if sudo_user is not None: - cmd = f"sudo -u {sudo_user} {cmd}" + if isinstance(cmd, list): + cmd = ["sudo", "-u", sudo_user] + cmd + else: + cmd = f"sudo -u {sudo_user} {cmd}" + if pinput is not None: kwargs["stdin"] = subprocess.PIPE if capture_output: @@ -62,7 +66,7 @@ def doveadm_cmd(params: list[str], pinput=None, capture_output=True, **kwargs): :return: return code, command output """ dpath = None - code, output = exec_cmd("which doveadm", shell=True) + code, output = exec_cmd(["which", "doveadm"]) if not code: dpath = force_str(output).strip() else: diff --git a/modoboa/maillog/graphics.py b/modoboa/maillog/graphics.py index e83b34979..4596a692b 100644 --- a/modoboa/maillog/graphics.py +++ b/modoboa/maillog/graphics.py @@ -75,7 +75,7 @@ class Graphic: def rrdtool_binary(self): """Return path to rrdtool binary.""" dpath = None - code, output = exec_cmd("which rrdtool", shell=True) + code, output = exec_cmd(["which", "rrdtool"]) if not code: dpath = output.strip() else: From 573021308314ba74899a20665f75441326ac2f3e Mon Sep 17 00:00:00 2001 From: Antoine Nguyen Date: Sat, 21 Feb 2026 09:39:30 +0100 Subject: [PATCH 5/8] Use list everywhere. --- modoboa/lib/sysutils.py | 8 ++------ tests.py | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/modoboa/lib/sysutils.py b/modoboa/lib/sysutils.py index 7eb665493..221ef2b09 100644 --- a/modoboa/lib/sysutils.py +++ b/modoboa/lib/sysutils.py @@ -14,7 +14,7 @@ from django.utils.encoding import force_str def exec_cmd( - cmd: str | list[str], + cmd: list[str], sudo_user: str | None = None, pinput: str | None = None, capture_output: bool = True, @@ -33,11 +33,7 @@ def exec_cmd( :return: return code, command output """ if sudo_user is not None: - if isinstance(cmd, list): - cmd = ["sudo", "-u", sudo_user] + cmd - else: - cmd = f"sudo -u {sudo_user} {cmd}" - + cmd = ["sudo", "-u", sudo_user] + cmd if pinput is not None: kwargs["stdin"] = subprocess.PIPE if capture_output: diff --git a/tests.py b/tests.py index 6a9e1a9ea..89b756b7d 100644 --- a/tests.py +++ b/tests.py @@ -26,10 +26,18 @@ class DeployTest(unittest.TestCase): def test_silent(self): dburl = f"default:{self.dbtype}://{self.dbuser}:{self.dbpassword}@{self.dbhost}:{self.dbport}/modoboa" - cmd = ( - f"modoboa-admin.py deploy --collectstatic " - f"--dburl {dburl} --domain localhost --admin-username admin {self.projname}" - ) + cmd = [ + "modoboa-admin.py", + "deploy", + "--collectstatic", + "--dburl", + dburl, + "--domain", + "localhost", + "--admin-username", + "admin", + self.projname, + ] code, output = exec_cmd(cmd, cwd=self.workdir, shell=True) self.assertEqual(code, 0) From 6208f82b855c203e270acbb6b3b62fb73e7baaa0 Mon Sep 17 00:00:00 2001 From: Antoine Nguyen Date: Sat, 21 Feb 2026 09:43:53 +0100 Subject: [PATCH 6/8] Added debug --- tests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests.py b/tests.py index 89b756b7d..e09a4efeb 100644 --- a/tests.py +++ b/tests.py @@ -39,6 +39,7 @@ class DeployTest(unittest.TestCase): self.projname, ] code, output = exec_cmd(cmd, cwd=self.workdir, shell=True) + print(output) self.assertEqual(code, 0) From 45caec192903c18de804ffc946564e3b6cf49401 Mon Sep 17 00:00:00 2001 From: Antoine Nguyen Date: Sat, 21 Feb 2026 09:57:06 +0100 Subject: [PATCH 7/8] Removed shell=True --- tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests.py b/tests.py index e09a4efeb..ba3c3fd0d 100644 --- a/tests.py +++ b/tests.py @@ -38,7 +38,7 @@ class DeployTest(unittest.TestCase): "admin", self.projname, ] - code, output = exec_cmd(cmd, cwd=self.workdir, shell=True) + code, output = exec_cmd(cmd, cwd=self.workdir) print(output) self.assertEqual(code, 0) From bf5310c13195e5e194758563e7fdbd6a8b2cda7d Mon Sep 17 00:00:00 2001 From: Antoine Nguyen Date: Sat, 21 Feb 2026 10:06:07 +0100 Subject: [PATCH 8/8] Removed debug --- tests.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests.py b/tests.py index ba3c3fd0d..480abaf84 100644 --- a/tests.py +++ b/tests.py @@ -39,7 +39,6 @@ class DeployTest(unittest.TestCase): self.projname, ] code, output = exec_cmd(cmd, cwd=self.workdir) - print(output) self.assertEqual(code, 0)