Start checking code with Bandit

.bandit

@@ -0,0 +1,3 @@
+[bandit]
+exclude: salt/ext/tornado/*,tests/minionswarm.py
+skip: B701

.pre-commit-config.yaml

@@ -1144,6 +1144,31 @@ repos:
         additional_dependencies: [black==19.10b0]
   # <---- Code Formatting --------------------------------------------------------------------------------------------
 
+  # ----- Security -------------------------------------------------------------------------------------------------->
+  - repo: https://github.com/PyCQA/bandit
+    rev: "1.7.0"
+    hooks:
+      - id: bandit
+        alias: bandit-salt
+        name: Run bandit against Salt
+        args: [--silent, -lll, --skip, B701]
+        exclude: >
+            (?x)^(
+                templates/.*|
+                salt/ext/.*|
+                tests/.*
+            )$
+  - repo: https://github.com/PyCQA/bandit
+    rev: "1.7.0"
+    hooks:
+      - id: bandit
+        alias: bandit-tests
+        name: Run bandit against the test suite
+        args: [--silent, -lll, B701]
+        files: ^tests/.*
+        exclude: ^tests/minionswarm\.py
+  # <---- Security ---------------------------------------------------------------------------------------------------
+
   # ----- Pre-Commit ------------------------------------------------------------------------------------------------>
   - repo: https://github.com/saltstack/mirrors-nox
     rev: v2020.8.22

salt/auth/pki.py

@@ -29,7 +29,7 @@
         try:
             from Cryptodome.Util import asn1
         except ImportError:
-            from Crypto.Util import asn1
+            from Crypto.Util import asn1  # nosec
         import OpenSSL
     HAS_DEPS = True
 except ImportError:

salt/client/ssh/ssh_py_shim.py

@@ -239,12 +239,14 @@ def get_executable():
         "python",
     )
     for py_cmd in pycmds:
-        cmd = (
-            py_cmd
-            + " -c  \"import sys; sys.stdout.write('%s:%s' % (sys.version_info[0], sys.version_info[1]))\""
-        )
         stdout, _ = subprocess.Popen(
-            cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True
+            [
+                py_cmd,
+                "-c"
+                "import sys; sys.stdout.write('%s:%s' % (sys.version_info[0], sys.version_info[1]))",
+            ],
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
         ).communicate()
         if sys.version_info[0] == 2 and sys.version_info[1] < 7:
             stdout = stdout.decode(get_system_encoding(), "replace").strip()

salt/cloud/__init__.py

@@ -38,7 +38,7 @@
     import Cryptodome.Random
 except ImportError:
     try:
-        import Crypto.Random
+        import Crypto.Random  # nosec
     except ImportError:
         pass  # pycrypto < 2.1
 

salt/cloud/clouds/joyent.py

@@ -83,8 +83,8 @@
         HAS_REQUIRED_CRYPTO = True
     except ImportError:
         try:
-            from Crypto.Hash import SHA256
-            from Crypto.Signature import PKCS1_v1_5
+            from Crypto.Hash import SHA256  # nosec
+            from Crypto.Signature import PKCS1_v1_5  # nosec
 
             HAS_REQUIRED_CRYPTO = True
         except ImportError:

salt/crypt.py

@@ -62,13 +62,17 @@
 
 if not HAS_M2 and not HAS_CRYPTO:
     try:
-        from Crypto.Cipher import AES, PKCS1_OAEP, PKCS1_v1_5 as PKCS1_v1_5_CIPHER
-        from Crypto.Hash import SHA
-        from Crypto.PublicKey import RSA
-        from Crypto.Signature import PKCS1_v1_5
+        from Crypto.Cipher import (  # nosec
+            AES,
+            PKCS1_OAEP,
+            PKCS1_v1_5 as PKCS1_v1_5_CIPHER,
+        )
+        from Crypto.Hash import SHA  # nosec
+        from Crypto.PublicKey import RSA  # nosec
+        from Crypto.Signature import PKCS1_v1_5  # nosec
 
         # let this be imported, if possible
-        from Crypto import Random
+        from Crypto import Random  # nosec
 
         HAS_CRYPTO = True
     except ImportError:

salt/fileclient.py

@@ -3,7 +3,7 @@
 """
 import contextlib
 import errno
-import ftplib
+import ftplib  # nosec
 import http.server
 import logging
 import os
@@ -577,7 +577,7 @@ def s3_opt(key, default=None):
                 )
         if url_data.scheme == "ftp":
             try:
-                ftp = ftplib.FTP()
+                ftp = ftplib.FTP()  # nosec
                 ftp_port = url_data.port
                 if not ftp_port:
                     ftp_port = 21

salt/modules/archive.py

@@ -203,12 +203,12 @@ def _list_tar(name, cached, decompress_cmd, failhard=False):
             else:
                 if not salt.utils.path.which("tar"):
                     raise CommandExecutionError("'tar' command not available")
-                if decompress_cmd is not None:
+                if decompress_cmd is not None and isinstance(decompress_cmd, str):
                     # Guard against shell injection
                     try:
-                        decompress_cmd = " ".join(
-                            [shlex.quote(x) for x in shlex.split(decompress_cmd)]
-                        )
+                        decompress_cmd = [
+                            shlex.quote(x) for x in shlex.split(decompress_cmd)
+                        ]
                     except AttributeError:
                         raise CommandExecutionError("Invalid CLI options")
                 else:
@@ -221,12 +221,11 @@ def _list_tar(name, cached, decompress_cmd, failhard=False):
                         )
                         == 0
                     ):
-                        decompress_cmd = "xz --decompress --stdout"
+                        decompress_cmd = ["xz", "--decompress", "--stdout"]
 
                 if decompress_cmd:
                     decompressed = subprocess.Popen(
-                        "{} {}".format(decompress_cmd, shlex.quote(cached)),
-                        shell=True,
+                        decompress_cmd + [shlex.quote(cached)],
                         stdout=subprocess.PIPE,
                         stderr=subprocess.PIPE,
                     )

salt/modules/btrfs.py

@@ -19,6 +19,7 @@
 import itertools
 import os
 import re
+import subprocess
 import uuid
 
 import salt.utils.fsutils
@@ -541,9 +542,12 @@ def convert(device, permanent=False, keeplf=False):
 
     if not permanent:
         ret["after"]["{}_image".format(orig_fstype)] = image_path
-        ret["after"]["{}_image_info".format(orig_fstype)] = (
-            os.popen("file {}/image".format(image_path)).read().strip()
+        image_info_proc = subprocess.run(
+            ["file", "{}/image".format(image_path)], check=True, stdout=subprocess.PIPE
         )
+        ret["after"][
+            "{}_image_info".format(orig_fstype)
+        ] = image_info_proc.stdout.strip()
     else:
         ret["after"]["{}_image".format(orig_fstype)] = "removed"
         ret["after"]["{}_image_info".format(orig_fstype)] = "N/A"

salt/modules/inspectlib/collector.py

@@ -492,14 +492,19 @@ def request_snapshot(self, mode, priority=19, **kwargs):
 
         self._prepare_full_scan(**kwargs)
 
-        os.system(
-            "nice -{} python {} {} {} {} & > /dev/null".format(
-                priority,
+        subprocess.run(
+            [
+                "nice",
+                "-{}".format(priority),
+                "python",
                 __file__,
                 os.path.dirname(self.pidfile),
                 os.path.dirname(self.dbfile),
                 mode,
-            )
+            ],
+            check=False,
+            stdout=subprocess.DEVNULL,
+            stderr=subprocess.DEVNULL,
         )
 
     def export(self, description, local=False, path="/tmp", format="qcow2"):

salt/modules/snapper.py

@@ -14,6 +14,7 @@
 import difflib
 import logging
 import os
+import subprocess
 import time
 
 import salt.utils.files
@@ -554,7 +555,12 @@ def _is_text_file(filename):
     """
     Checks if a file is a text file
     """
-    type_of_file = os.popen("file -bi {}".format(filename), "r").read()
+    type_of_file = subprocess.run(
+        ["file", "-bi", filename],
+        check=False,
+        stdout=subprocess.STDOUT,
+        universal_newlines=True,
+    ).stdout
     return type_of_file.startswith("text")
 
 

salt/modules/virt.py

@@ -656,19 +656,15 @@ def _libvirt_creds():
     """
     Returns the user and group that the disk images should be owned by
     """
-    g_cmd = "grep ^\\s*group /etc/libvirt/qemu.conf"
-    u_cmd = "grep ^\\s*user /etc/libvirt/qemu.conf"
+    g_cmd = ["grep", "^\\s*group", "/etc/libvirt/qemu.conf"]
+    u_cmd = ["grep", "^\\s*user", "/etc/libvirt/qemu.conf"]
     try:
-        stdout = subprocess.Popen(
-            g_cmd, shell=True, stdout=subprocess.PIPE
-        ).communicate()[0]
+        stdout = subprocess.Popen(g_cmd, stdout=subprocess.PIPE).communicate()[0]
         group = salt.utils.stringutils.to_str(stdout).split('"')[1]
     except IndexError:
         group = "root"
     try:
-        stdout = subprocess.Popen(
-            u_cmd, shell=True, stdout=subprocess.PIPE
-        ).communicate()[0]
+        stdout = subprocess.Popen(u_cmd, stdout=subprocess.PIPE).communicate()[0]
         user = salt.utils.stringutils.to_str(stdout).split('"')[1]
     except IndexError:
         user = "root"
@@ -5728,7 +5724,7 @@ def seed_non_shared_migrate(disks, force=False):
             # the target exists, check to see if it is compatible
             pre = salt.utils.yaml.safe_load(
                 subprocess.Popen(
-                    "qemu-img info arch", shell=True, stdout=subprocess.PIPE
+                    ["qemu-img", "info", "arch"], stdout=subprocess.PIPE
                 ).communicate()[0]
             )
             if (
@@ -5740,11 +5736,9 @@ def seed_non_shared_migrate(disks, force=False):
             os.makedirs(os.path.dirname(fn_))
         if os.path.isfile(fn_):
             os.remove(fn_)
-        cmd = "qemu-img create -f " + form + " " + fn_ + " " + size
-        subprocess.call(cmd, shell=True)
+        subprocess.call(["qemu-img", "create", "-f", form, fn_, size])
         creds = _libvirt_creds()
-        cmd = "chown " + creds["user"] + ":" + creds["group"] + " " + fn_
-        subprocess.call(cmd, shell=True)
+        subprocess.call(["chown", "{user}:{group}".format(**creds), fn_])
     return True
 
 
@@ -5979,7 +5973,7 @@ def _is_bhyve_hyper():
     vmm_enabled = False
     try:
         stdout = subprocess.Popen(
-            sysctl_cmd, shell=True, stdout=subprocess.PIPE
+            ["sysctl", "hw.vmm.create"], stdout=subprocess.PIPE
         ).communicate()[0]
         vmm_enabled = len(salt.utils.stringutils.to_str(stdout).split('"')[1]) != 0
     except IndexError:

salt/pillar/libvirt.py

@@ -71,13 +71,27 @@ def gen_hyper_keys(
         with salt.utils.files.fopen(cainfo, "w+") as fp_:
             fp_.write("cn = salted\nca\ncert_signing_key")
     if not os.path.isfile(cakey):
-        subprocess.call("certtool --generate-privkey > {}".format(cakey), shell=True)
+        proc = subprocess.run(
+            ["certtool", "--generate-privkey"],
+            stdout=subprocess.PIPE,
+            universal_newlines=True,
+            check=True,
+        )
+        with salt.utils.files.fopen(cakey, "w") as wfh:
+            wfh.write(proc.stdout)
     if not os.path.isfile(cacert):
-        cmd = (
-            "certtool --generate-self-signed --load-privkey {} "
-            "--template {} --outfile {}"
-        ).format(cakey, cainfo, cacert)
-        subprocess.call(cmd, shell=True)
+        subprocess.call(
+            [
+                "certtool",
+                "--generate-self-signed",
+                "--load-privkey",
+                cakey,
+                "--template",
+                cainfo,
+                "--outfile",
+                cacert,
+            ]
+        )
     sub_dir = os.path.join(key_dir, minion_id)
     if not os.path.isdir(sub_dir):
         os.makedirs(sub_dir)
@@ -98,14 +112,31 @@ def gen_hyper_keys(
             )
             fp_.write(infodat)
     if not os.path.isfile(priv):
-        subprocess.call("certtool --generate-privkey > {}".format(priv), shell=True)
+        proc = subprocess.run(
+            ["certtool", "--generate-privkey"],
+            stdout=subprocess.PIPE,
+            universal_newlines=True,
+            check=True,
+        )
+        with salt.utils.files.fopen(priv, "w") as wfh:
+            wfh.write(proc.stdout)
     if not os.path.isfile(cert):
-        cmd = (
-            "certtool --generate-certificate --load-privkey {} "
-            "--load-ca-certificate {} --load-ca-privkey {} "
-            "--template {} --outfile {}"
-        ).format(priv, cacert, cakey, srvinfo, cert)
-        subprocess.call(cmd, shell=True)
+        subprocess.call(
+            [
+                "certtool",
+                "--generate-certificate",
+                "--load-privkey",
+                priv,
+                "--load-ca-certificate",
+                cacert,
+                "--load-ca-privkey",
+                cakey,
+                "--template",
+                srvinfo,
+                "--outfile",
+                cert,
+            ]
+        )
     if not os.path.isfile(clientinfo):
         with salt.utils.files.fopen(clientinfo, "w+") as fp_:
             infodat = salt.utils.stringutils.to_str(
@@ -118,11 +149,28 @@ def gen_hyper_keys(
             )
             fp_.write(infodat)
     if not os.path.isfile(cpriv):
-        subprocess.call("certtool --generate-privkey > {}".format(cpriv), shell=True)
+        proc = subprocess.run(
+            ["certtool", "--generate-privkey"],
+            stdout=subprocess.PIPE,
+            universal_newlines=True,
+            check=True,
+        )
+        with salt.utils.files.fopen(cpriv, "w") as wfh:
+            wfh.write(proc.stdout)
     if not os.path.isfile(ccert):
-        cmd = (
-            "certtool --generate-certificate --load-privkey {} "
-            "--load-ca-certificate {} --load-ca-privkey {} "
-            "--template {} --outfile {}"
-        ).format(cpriv, cacert, cakey, clientinfo, ccert)
-        subprocess.call(cmd, shell=True)
+        subprocess.call(
+            [
+                "certtool",
+                "--generate-certificate",
+                "--load-privkey",
+                cpriv,
+                "--load-ca-certificate",
+                cacert,
+                "--load-ca-privkey",
+                cakey,
+                "--template",
+                clientinfo,
+                "--outfile",
+                ccert,
+            ]
+        )