diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index eab23793c..af30837fe 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -19,6 +19,7 @@ repos: rev: 3.9.1 hooks: - id: flake8 + additional_dependencies: [dlint] - repo: https://github.com/pre-commit/pre-commit-hooks rev: v3.4.0 hooks: diff --git a/CHANGELOG.md b/CHANGELOG.md index 022fba9e4..cc8ee9c96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,3 +25,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Fixed - Attempted to delete a directory when monkey config reset was called. #1054 + +### Security +- Address minor issues discovered by Dlint. #1075 diff --git a/envs/monkey_zoo/blackbox/island_client/monkey_island_requests.py b/envs/monkey_zoo/blackbox/island_client/monkey_island_requests.py index 4575f465e..fcea862e2 100644 --- a/envs/monkey_zoo/blackbox/island_client/monkey_island_requests.py +++ b/envs/monkey_zoo/blackbox/island_client/monkey_island_requests.py @@ -66,8 +66,8 @@ class MonkeyIslandRequests(object): return request_function_wrapper def get_jwt_from_server(self): - resp = requests.post( - self.addr + "api/auth", # noqa: DUO123 + resp = requests.post( # noqa: DUO123 + self.addr + "api/auth", json={"username": NO_AUTH_CREDS, "password": NO_AUTH_CREDS}, verify=False, ) @@ -75,8 +75,8 @@ class MonkeyIslandRequests(object): @_Decorators.refresh_jwt_token def get(self, url, data=None): - return requests.get( - self.addr + url, # noqa: DUO123 + return requests.get( # noqa: DUO123 + self.addr + url, headers=self.get_jwt_header(), params=data, verify=False, @@ -84,25 +84,25 @@ class MonkeyIslandRequests(object): @_Decorators.refresh_jwt_token def post(self, url, data): - return requests.post( - self.addr + url, data=data, headers=self.get_jwt_header(), verify=False # noqa: DUO123 + return requests.post( # noqa: DUO123 + self.addr + url, data=data, headers=self.get_jwt_header(), verify=False ) @_Decorators.refresh_jwt_token def post_json(self, url, data: Dict): - return requests.post( - self.addr + url, json=data, headers=self.get_jwt_header(), verify=False # noqa: DUO123 + return requests.post( # noqa: DUO123 + self.addr + url, json=data, headers=self.get_jwt_header(), verify=False ) @_Decorators.refresh_jwt_token def patch(self, url, data: Dict): - return requests.patch( - self.addr + url, data=data, headers=self.get_jwt_header(), verify=False # noqa: DUO123 + return requests.patch( # noqa: DUO123 + self.addr + url, data=data, headers=self.get_jwt_header(), verify=False ) @_Decorators.refresh_jwt_token def delete(self, url): - return requests.delete( # noqa: DOU123 + return requests.delete( # noqa: DUO123 self.addr + url, headers=self.get_jwt_header(), verify=False ) diff --git a/envs/monkey_zoo/blackbox/utils/gcp_machine_handlers.py b/envs/monkey_zoo/blackbox/utils/gcp_machine_handlers.py index 00279ea8b..147958fe2 100644 --- a/envs/monkey_zoo/blackbox/utils/gcp_machine_handlers.py +++ b/envs/monkey_zoo/blackbox/utils/gcp_machine_handlers.py @@ -23,9 +23,9 @@ class GCPHandler(object): subprocess.call(GCPHandler.get_auth_command(key_path), shell=True) # noqa: DUO116 LOGGER.info("GCP Handler passed key") # set project - subprocess.call( + subprocess.call( # noqa: DUO116 GCPHandler.get_set_project_command(project_id), shell=True - ) # noqa: DUO116 + ) LOGGER.info("GCP Handler set project") LOGGER.info("GCP Handler initialized successfully") except Exception as e: @@ -39,18 +39,18 @@ class GCPHandler(object): """ LOGGER.info("Setting up all GCP machines...") try: - subprocess.call( + subprocess.call( # noqa: DUO116 (GCPHandler.MACHINE_STARTING_COMMAND % (machine_list, self.zone)), shell=True - ) # noqa: DUO116 + ) LOGGER.info("GCP machines successfully started.") except Exception as e: LOGGER.error("GCP Handler failed to start GCP machines: %s" % e) def stop_machines(self, machine_list): try: - subprocess.call( + subprocess.call( # noqa: DUO116 (GCPHandler.MACHINE_STOPPING_COMMAND % (machine_list, self.zone)), shell=True - ) # noqa: DUO116 + ) LOGGER.info("GCP machines stopped successfully.") except Exception as e: LOGGER.error("GCP Handler failed to stop network machines: %s" % e) diff --git a/monkey/infection_monkey/control.py b/monkey/infection_monkey/control.py index 0df989d99..6fdd585b2 100644 --- a/monkey/infection_monkey/control.py +++ b/monkey/infection_monkey/control.py @@ -64,8 +64,8 @@ class ControlClient(object): if ControlClient.proxies: monkey["tunnel"] = ControlClient.proxies.get("https") - requests.post( - "https://%s/api/monkey" % (WormConfiguration.current_server,), # noqa: DUO123 + requests.post( # noqa: DUO123 + "https://%s/api/monkey" % (WormConfiguration.current_server,), data=json.dumps(monkey), headers={"content-type": "application/json"}, verify=False, @@ -92,8 +92,8 @@ class ControlClient(object): if ControlClient.proxies: debug_message += " through proxies: %s" % ControlClient.proxies LOG.debug(debug_message) - requests.get( - f"https://{server}/api?action=is-up", # noqa: DUO123 + requests.get( # noqa: DUO123 + f"https://{server}/api?action=is-up", verify=False, proxies=ControlClient.proxies, timeout=TIMEOUT_IN_SECONDS, @@ -130,9 +130,8 @@ class ControlClient(object): monkey = {} if ControlClient.proxies: monkey["tunnel"] = ControlClient.proxies.get("https") - requests.patch( - "https://%s/api/monkey/%s" - % (WormConfiguration.current_server, GUID), # noqa: DUO123 + requests.patch( # noqa: DUO123 + "https://%s/api/monkey/%s" % (WormConfiguration.current_server, GUID), data=json.dumps(monkey), headers={"content-type": "application/json"}, verify=False, @@ -155,8 +154,8 @@ class ControlClient(object): return try: telemetry = {"monkey_guid": GUID, "telem_category": telem_category, "data": json_data} - requests.post( - "https://%s/api/telemetry" % (WormConfiguration.current_server,), # noqa: DUO123 + requests.post( # noqa: DUO123 + "https://%s/api/telemetry" % (WormConfiguration.current_server,), data=json.dumps(telemetry), headers={"content-type": "application/json"}, verify=False, @@ -174,8 +173,8 @@ class ControlClient(object): return try: telemetry = {"monkey_guid": GUID, "log": json.dumps(log)} - requests.post( - "https://%s/api/log" % (WormConfiguration.current_server,), # noqa: DUO123 + requests.post( # noqa: DUO123 + "https://%s/api/log" % (WormConfiguration.current_server,), data=json.dumps(telemetry), headers={"content-type": "application/json"}, verify=False, @@ -192,9 +191,8 @@ class ControlClient(object): if not WormConfiguration.current_server: return try: - reply = requests.get( - "https://%s/api/monkey/%s" - % (WormConfiguration.current_server, GUID), # noqa: DUO123 + reply = requests.get( # noqa: DUO123 + "https://%s/api/monkey/%s" % (WormConfiguration.current_server, GUID), verify=False, proxies=ControlClient.proxies, timeout=MEDIUM_REQUEST_TIMEOUT, @@ -230,9 +228,8 @@ class ControlClient(object): if not WormConfiguration.current_server: return try: - requests.patch( - "https://%s/api/monkey/%s" - % (WormConfiguration.current_server, GUID), # noqa: DUO123 + requests.patch( # noqa: DUO123 + "https://%s/api/monkey/%s" % (WormConfiguration.current_server, GUID), data=json.dumps({"config_error": True}), headers={"content-type": "application/json"}, verify=False, @@ -292,9 +289,9 @@ class ControlClient(object): if (monkeyfs.isfile(dest_file)) and (size == monkeyfs.getsize(dest_file)): return dest_file else: - download = requests.get( + download = requests.get( # noqa: DUO123 "https://%s/api/monkey/download/%s" - % (WormConfiguration.current_server, filename), # noqa: DUO123 + % (WormConfiguration.current_server, filename), verify=False, proxies=ControlClient.proxies, timeout=MEDIUM_REQUEST_TIMEOUT, @@ -322,9 +319,8 @@ class ControlClient(object): if not WormConfiguration.current_server: return None, None try: - reply = requests.post( - "https://%s/api/monkey/download" - % (WormConfiguration.current_server,), # noqa: DUO123 + reply = requests.post( # noqa: DUO123 + "https://%s/api/monkey/download" % (WormConfiguration.current_server,), data=json.dumps(host_dict), headers={"content-type": "application/json"}, verify=False, @@ -370,8 +366,8 @@ class ControlClient(object): @staticmethod def get_pba_file(filename): try: - return requests.get( - PBA_FILE_DOWNLOAD % (WormConfiguration.current_server, filename), # noqa: DUO123 + return requests.get( # noqa: DUO123 + PBA_FILE_DOWNLOAD % (WormConfiguration.current_server, filename), verify=False, proxies=ControlClient.proxies, timeout=LONG_REQUEST_TIMEOUT, @@ -382,9 +378,9 @@ class ControlClient(object): @staticmethod def get_T1216_pba_file(): try: - return requests.get( + return requests.get( # noqa: DUO123 urljoin( - f"https://{WormConfiguration.current_server}/", # noqa: DUO123 + f"https://{WormConfiguration.current_server}/", T1216_PBA_FILE_DOWNLOAD_PATH, ), verify=False, @@ -416,7 +412,9 @@ class ControlClient(object): f"https://{WormConfiguration.current_server}/api/monkey_control" f"/check_remote_port/{port}" ) - response = requests.get(url, verify=False, timeout=SHORT_REQUEST_TIMEOUT) + response = requests.get( # noqa: DUO123 + url, verify=False, timeout=SHORT_REQUEST_TIMEOUT + ) response = json.loads(response.content.decode()) return response["status"] == "port_visible" except requests.exceptions.RequestException: @@ -424,7 +422,7 @@ class ControlClient(object): @staticmethod def report_start_on_island(): - requests.post( + requests.post( # noqa: DUO123 f"https://{WormConfiguration.current_server}/api/monkey_control/started_on_island", data=json.dumps({"started_on_island": True}), verify=False, diff --git a/monkey/infection_monkey/dropper.py b/monkey/infection_monkey/dropper.py index 3d34688ef..902d30280 100644 --- a/monkey/infection_monkey/dropper.py +++ b/monkey/infection_monkey/dropper.py @@ -4,6 +4,7 @@ import filecmp import logging import os import pprint +import shlex import shutil import subprocess import sys @@ -13,11 +14,7 @@ from ctypes import c_char_p from common.utils.attack_utils import ScanStatus, UsageEnum from infection_monkey.config import WormConfiguration from infection_monkey.exploit.tools.helpers import build_monkey_commandline_explicitly -from infection_monkey.model import ( - GENERAL_CMDLINE_LINUX, - MONKEY_CMDLINE_LINUX, - MONKEY_CMDLINE_WINDOWS, -) +from infection_monkey.model import MONKEY_CMDLINE_LINUX, MONKEY_CMDLINE_WINDOWS from infection_monkey.system_info import OperatingSystem, SystemInfoCollector from infection_monkey.telemetry.attack.t1106_telem import T1106Telem @@ -146,33 +143,46 @@ class MonkeyDrops(object): ) if OperatingSystem.Windows == SystemInfoCollector.get_os(): + # TODO: Replace all of this string templating with a function that accepts + # the necessary parameters and returns a list of arguments. monkey_cmdline = ( MONKEY_CMDLINE_WINDOWS % {"monkey_path": self._config["destination_path"]} + monkey_options ) + monkey_cmdline_split = shlex.split( + monkey_cmdline, + posix=False, # won't try resolving "\" in paths as part of escape sequences + ) + + monkey_process = subprocess.Popen( + monkey_cmdline_split, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + close_fds=True, + creationflags=DETACHED_PROCESS, + ) else: dest_path = self._config["destination_path"] - # In linux we have a more complex commandline. There's a general outer one, - # and the inner one which actually - # runs the monkey - inner_monkey_cmdline = ( + # In Linux, we need to change the directory first, which is done + # using thw `cwd` argument in `subprocess.Popen` below + # TODO: Replace all of this string templating with a function that accepts + # the necessary parameters and returns a list of arguments. + monkey_cmdline = ( MONKEY_CMDLINE_LINUX % {"monkey_filename": dest_path.split("/")[-1]} + monkey_options ) - monkey_cmdline = GENERAL_CMDLINE_LINUX % { - "monkey_directory": dest_path[0 : dest_path.rfind("/")], - "monkey_commandline": inner_monkey_cmdline, - } + monkey_cmdline_split = shlex.split(monkey_cmdline) - monkey_process = subprocess.Popen( - monkey_cmdline, - shell=True, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - close_fds=True, - creationflags=DETACHED_PROCESS, - ) + monkey_process = subprocess.Popen( + monkey_cmdline_split, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + close_fds=True, + cwd="/".join(dest_path.split("/")[0:-1]), + creationflags=DETACHED_PROCESS, + ) LOG.info( "Executed monkey process (PID=%d) with command line: %s", diff --git a/monkey/infection_monkey/exploit/drupal.py b/monkey/infection_monkey/exploit/drupal.py index 50594e656..d1f49432c 100644 --- a/monkey/infection_monkey/exploit/drupal.py +++ b/monkey/infection_monkey/exploit/drupal.py @@ -82,8 +82,8 @@ class DrupalExploiter(WebRCE): """ payload = build_exploitability_check_payload(url) - response = requests.get( - f"{url}?_format=hal_json", # noqa: DUO123 + response = requests.get( # noqa: DUO123 + f"{url}?_format=hal_json", json=payload, headers={"Content-Type": "application/hal+json"}, verify=False, @@ -102,8 +102,8 @@ class DrupalExploiter(WebRCE): base = remove_port(url) payload = build_cmd_execution_payload(base, cmd) - r = requests.get( - f"{url}?_format=hal_json", # noqa: DUO123 + r = requests.get( # noqa: DUO123 + f"{url}?_format=hal_json", json=payload, headers={"Content-Type": "application/hal+json"}, verify=False, @@ -157,9 +157,9 @@ def find_exploitbale_article_ids(base_url: str, lower: int = 1, upper: int = 100 articles = set() while lower < upper: node_url = urljoin(base_url, str(lower)) - response = requests.get( + response = requests.get( # noqa: DUO123 node_url, verify=False, timeout=LONG_REQUEST_TIMEOUT - ) # noqa: DUO123 + ) if response.status_code == 200: if is_response_cached(response): LOG.info(f"Found a cached article at: {node_url}, skipping") diff --git a/monkey/infection_monkey/exploit/hadoop.py b/monkey/infection_monkey/exploit/hadoop.py index a30112cce..7a0264380 100644 --- a/monkey/infection_monkey/exploit/hadoop.py +++ b/monkey/infection_monkey/exploit/hadoop.py @@ -7,8 +7,8 @@ import json import logging import posixpath -import random import string +from random import SystemRandom import requests @@ -69,8 +69,9 @@ class HadoopExploiter(WebRCE): resp = json.loads(resp.content) app_id = resp["application-id"] # Create a random name for our application in YARN + safe_random = SystemRandom() rand_name = ID_STRING + "".join( - [random.choice(string.ascii_lowercase) for _ in range(self.RAN_STR_LEN)] + [safe_random.choice(string.ascii_lowercase) for _ in range(self.RAN_STR_LEN)] ) payload = self.build_payload(app_id, rand_name, command) resp = requests.post( diff --git a/monkey/infection_monkey/exploit/shellshock.py b/monkey/infection_monkey/exploit/shellshock.py index 7854483a0..7bca6b04b 100644 --- a/monkey/infection_monkey/exploit/shellshock.py +++ b/monkey/infection_monkey/exploit/shellshock.py @@ -3,7 +3,7 @@ import logging import string -from random import choice +from random import SystemRandom import requests @@ -37,8 +37,9 @@ class ShellShockExploiter(HostExploiter): def __init__(self, host): super(ShellShockExploiter, self).__init__(host) self.HTTP = [str(port) for port in self._config.HTTP_PORTS] + safe_random = SystemRandom() self.success_flag = "".join( - choice(string.ascii_uppercase + string.digits) for _ in range(20) + safe_random.choice(string.ascii_uppercase + string.digits) for _ in range(20) ) self.skip_exist = self._config.skip_exploit_if_file_exist @@ -242,9 +243,9 @@ class ShellShockExploiter(HostExploiter): try: LOG.debug("Header is: %s" % header) LOG.debug("Attack is: %s" % attack) - r = requests.get( + r = requests.get( # noqa: DUO123 url, headers={header: attack}, verify=False, timeout=TIMEOUT - ) # noqa: DUO123 + ) result = r.content.decode() return result except requests.exceptions.RequestException as exc: diff --git a/monkey/infection_monkey/exploit/struts2.py b/monkey/infection_monkey/exploit/struts2.py index ff5b9887b..f29f6acac 100644 --- a/monkey/infection_monkey/exploit/struts2.py +++ b/monkey/infection_monkey/exploit/struts2.py @@ -52,7 +52,7 @@ class Struts2Exploiter(WebRCE): request = urllib.request.Request(url, headers=headers) try: return urllib.request.urlopen( - request, context=ssl._create_unverified_context() + request, context=ssl._create_unverified_context() # noqa: DUO122 ).geturl() except urllib.error.URLError: LOG.error("Can't reach struts2 server") diff --git a/monkey/infection_monkey/exploit/weblogic.py b/monkey/infection_monkey/exploit/weblogic.py index 017050346..9b158709f 100644 --- a/monkey/infection_monkey/exploit/weblogic.py +++ b/monkey/infection_monkey/exploit/weblogic.py @@ -83,9 +83,9 @@ class WebLogic201710271(WebRCE): else: payload = self.get_exploit_payload("cmd", "/c", command + " 1> NUL 2> NUL") try: - post( + post( # noqa: DUO123 url, data=payload, headers=HEADERS, timeout=EXECUTION_TIMEOUT, verify=False - ) # noqa: DUO123 + ) except Exception as e: LOG.error("Connection error: %s" % e) return False @@ -121,9 +121,9 @@ class WebLogic201710271(WebRCE): def check_if_exploitable_weblogic(self, url, httpd): payload = self.get_test_payload(ip=httpd.local_ip, port=httpd.local_port) try: - post( + post( # noqa: DUO123 url, data=payload, headers=HEADERS, timeout=REQUEST_DELAY, verify=False - ) # noqa: DUO123 + ) except exceptions.ReadTimeout: # Our request will not get response thus we get ReadTimeout error pass diff --git a/monkey/infection_monkey/network/info.py b/monkey/infection_monkey/network/info.py index c30f3d436..5ada2e29f 100644 --- a/monkey/infection_monkey/network/info.py +++ b/monkey/infection_monkey/network/info.py @@ -1,7 +1,7 @@ import itertools import socket import struct -from random import randint +from random import randint # noqa: DUO102 from subprocess import check_output import netifaces diff --git a/monkey/infection_monkey/network/tcp_scanner.py b/monkey/infection_monkey/network/tcp_scanner.py index 1260a590d..93e30ccad 100644 --- a/monkey/infection_monkey/network/tcp_scanner.py +++ b/monkey/infection_monkey/network/tcp_scanner.py @@ -1,5 +1,5 @@ from itertools import zip_longest -from random import shuffle +from random import shuffle # noqa: DUO102 import infection_monkey.config from infection_monkey.network.HostFinger import HostFinger diff --git a/monkey/infection_monkey/post_breach/actions/clear_command_history.py b/monkey/infection_monkey/post_breach/actions/clear_command_history.py index 412229ee5..f4aa5ad7b 100644 --- a/monkey/infection_monkey/post_breach/actions/clear_command_history.py +++ b/monkey/infection_monkey/post_breach/actions/clear_command_history.py @@ -46,8 +46,8 @@ class ClearCommandHistory(PBA): def run(self): if self.command: try: - output = subprocess.check_output( - self.command, stderr=subprocess.STDOUT, shell=True # noqa: DUO116 + output = subprocess.check_output( # noqa: DUO116 + self.command, stderr=subprocess.STDOUT, shell=True ).decode() return output, True except subprocess.CalledProcessError as e: diff --git a/monkey/infection_monkey/post_breach/actions/communicate_as_new_user.py b/monkey/infection_monkey/post_breach/actions/communicate_as_new_user.py index ecbebd4d0..d82b412d3 100644 --- a/monkey/infection_monkey/post_breach/actions/communicate_as_new_user.py +++ b/monkey/infection_monkey/post_breach/actions/communicate_as_new_user.py @@ -52,8 +52,8 @@ class CommunicateAsNewUser(PBA): @staticmethod def get_random_new_user_name(): return USERNAME_PREFIX + "".join( - random.choice(string.ascii_lowercase) for _ in range(5) - ) # noqa: DUO102 + random.choice(string.ascii_lowercase) for _ in range(5) # noqa: DUO102 + ) @staticmethod def get_commandline_for_http_request(url, is_windows=is_windows_os()): diff --git a/monkey/infection_monkey/post_breach/actions/modify_shell_startup_files.py b/monkey/infection_monkey/post_breach/actions/modify_shell_startup_files.py index eea61ed2f..18990ab11 100644 --- a/monkey/infection_monkey/post_breach/actions/modify_shell_startup_files.py +++ b/monkey/infection_monkey/post_breach/actions/modify_shell_startup_files.py @@ -58,8 +58,8 @@ class ModifyShellStartupFiles(PBA): def run(self): if self.command: try: - output = subprocess.check_output( - self.command, stderr=subprocess.STDOUT, shell=True # noqa: DUO116 + output = subprocess.check_output( # noqa: DUO116 + self.command, stderr=subprocess.STDOUT, shell=True ).decode() return output, True except subprocess.CalledProcessError as e: diff --git a/monkey/infection_monkey/post_breach/actions/use_signed_scripts.py b/monkey/infection_monkey/post_breach/actions/use_signed_scripts.py index 782f85d13..ed9b1dc21 100644 --- a/monkey/infection_monkey/post_breach/actions/use_signed_scripts.py +++ b/monkey/infection_monkey/post_breach/actions/use_signed_scripts.py @@ -21,10 +21,9 @@ class SignedScriptProxyExecution(PBA): try: original_comspec = "" if is_windows_os(): - original_comspec = subprocess.check_output( + original_comspec = subprocess.check_output( # noqa: DUO116 "if defined COMSPEC echo %COMSPEC%", shell=True - ).decode() # noqa: DUO116 - + ).decode() super().run() except Exception as e: LOG.warning( diff --git a/monkey/infection_monkey/post_breach/pba.py b/monkey/infection_monkey/post_breach/pba.py index a87cfec48..bf0e66ed4 100644 --- a/monkey/infection_monkey/post_breach/pba.py +++ b/monkey/infection_monkey/post_breach/pba.py @@ -90,7 +90,7 @@ class PBA(Plugin): :return: Tuple of command's output string and boolean, indicating if it succeeded """ try: - output = subprocess.check_output( + output = subprocess.check_output( # noqa: DUO116 self.command, stderr=subprocess.STDOUT, shell=True ).decode() return output, True diff --git a/monkey/infection_monkey/post_breach/shell_startup_files/windows/shell_startup_files_modification.py b/monkey/infection_monkey/post_breach/shell_startup_files/windows/shell_startup_files_modification.py index e65893095..62fd9425e 100644 --- a/monkey/infection_monkey/post_breach/shell_startup_files/windows/shell_startup_files_modification.py +++ b/monkey/infection_monkey/post_breach/shell_startup_files/windows/shell_startup_files_modification.py @@ -13,8 +13,10 @@ def get_windows_commands_to_modify_shell_startup_files(): # get list of usernames USERS = ( - subprocess.check_output("dir C:\\Users /b", shell=True).decode().split("\r\n")[:-1] - ) # noqa: DUO116 + subprocess.check_output("dir C:\\Users /b", shell=True) # noqa: DUO116 + .decode() + .split("\r\n")[:-1] + ) USERS.remove("Public") STARTUP_FILES_PER_USER = [ diff --git a/monkey/infection_monkey/post_breach/signed_script_proxy/signed_script_proxy.py b/monkey/infection_monkey/post_breach/signed_script_proxy/signed_script_proxy.py index cfabaafec..12343d8cf 100644 --- a/monkey/infection_monkey/post_breach/signed_script_proxy/signed_script_proxy.py +++ b/monkey/infection_monkey/post_breach/signed_script_proxy/signed_script_proxy.py @@ -15,7 +15,7 @@ def get_commands_to_proxy_execution_using_signed_script(): def cleanup_changes(original_comspec): if is_windows_os(): - subprocess.run( + subprocess.run( # noqa: DUO116 get_windows_commands_to_reset_comspec(original_comspec), shell=True - ) # noqa: DUO116 + ) subprocess.run(get_windows_commands_to_delete_temp_comspec(), shell=True) # noqa: DUO116 diff --git a/monkey/infection_monkey/system_info/windows_info_collector.py b/monkey/infection_monkey/system_info/windows_info_collector.py index f978a9942..cc46aae93 100644 --- a/monkey/infection_monkey/system_info/windows_info_collector.py +++ b/monkey/infection_monkey/system_info/windows_info_collector.py @@ -1,4 +1,5 @@ import logging +import shlex import subprocess import sys @@ -51,10 +52,10 @@ class WindowsInfoCollector(InfoCollector): def get_installed_packages(self): LOG.info("Getting installed packages") - packages = subprocess.check_output("dism /online /get-packages", shell=True) + packages = subprocess.check_output(shlex.split("dism /online /get-packages")) self.info["installed_packages"] = packages.decode("utf-8", errors="ignore") - features = subprocess.check_output("dism /online /get-features", shell=True) + features = subprocess.check_output(shlex.split("dism /online /get-features")) self.info["installed_features"] = features.decode("utf-8", errors="ignore") LOG.debug("Got installed packages") diff --git a/monkey/infection_monkey/transport/http.py b/monkey/infection_monkey/transport/http.py index e2b3a69da..fbddce109 100644 --- a/monkey/infection_monkey/transport/http.py +++ b/monkey/infection_monkey/transport/http.py @@ -126,7 +126,7 @@ class HTTPConnectProxyHandler(http.server.BaseHTTPRequestHandler): LOG.info("Received bootloader's request: {}".format(post_data)) try: dest_path = self.path - r = requests.post( + r = requests.post( # noqa: DUO123 url=dest_path, data=post_data, verify=False, diff --git a/monkey/infection_monkey/utils/hidden_files.py b/monkey/infection_monkey/utils/hidden_files.py index cc973cc5e..92391ecc9 100644 --- a/monkey/infection_monkey/utils/hidden_files.py +++ b/monkey/infection_monkey/utils/hidden_files.py @@ -26,9 +26,9 @@ def get_commands_to_hide_folders(): def cleanup_hidden_files(is_windows=is_windows_os()): - subprocess.run( + subprocess.run( # noqa: DUO116 get_windows_commands_to_delete() - if is_windows # noqa: DUO116 + if is_windows else " ".join(get_linux_commands_to_delete()), shell=True, ) diff --git a/monkey/infection_monkey/utils/linux/users.py b/monkey/infection_monkey/utils/linux/users.py index fa91fced8..9bd3c2bf8 100644 --- a/monkey/infection_monkey/utils/linux/users.py +++ b/monkey/infection_monkey/utils/linux/users.py @@ -1,6 +1,6 @@ import datetime import logging -import os +import shlex import subprocess from infection_monkey.utils.auto_new_user import AutoNewUser @@ -43,18 +43,16 @@ class AutoNewLinuxUser(AutoNewUser): logger.debug( "Trying to add {} with commands {}".format(self.username, str(commands_to_add_user)) ) - _ = subprocess.check_output( - " ".join(commands_to_add_user), stderr=subprocess.STDOUT, shell=True - ) + _ = subprocess.check_output(commands_to_add_user, stderr=subprocess.STDOUT) def __enter__(self): return self # No initialization/logging on needed in Linux def run_as(self, command): - command_as_new_user = "sudo -u {username} {command}".format( - username=self.username, command=command + command_as_new_user = shlex.split( + "sudo -u {username} {command}".format(username=self.username, command=command) ) - return os.system(command_as_new_user) + return subprocess.call(command_as_new_user) def __exit__(self, exc_type, exc_val, exc_tb): # delete the user. @@ -64,6 +62,4 @@ class AutoNewLinuxUser(AutoNewUser): self.username, str(commands_to_delete_user) ) ) - _ = subprocess.check_output( - " ".join(commands_to_delete_user), stderr=subprocess.STDOUT, shell=True - ) + _ = subprocess.check_output(commands_to_delete_user, stderr=subprocess.STDOUT) diff --git a/monkey/infection_monkey/utils/windows/users.py b/monkey/infection_monkey/utils/windows/users.py index d27b74547..06e626783 100644 --- a/monkey/infection_monkey/utils/windows/users.py +++ b/monkey/infection_monkey/utils/windows/users.py @@ -39,7 +39,7 @@ class AutoNewWindowsUser(AutoNewUser): windows_cmds = get_windows_commands_to_add_user(self.username, self.password, True) logger.debug("Trying to add {} with commands {}".format(self.username, str(windows_cmds))) - _ = subprocess.check_output(windows_cmds, stderr=subprocess.STDOUT, shell=True) + _ = subprocess.check_output(windows_cmds, stderr=subprocess.STDOUT) def __enter__(self): # Importing these only on windows, as they won't exist on linux. @@ -127,9 +127,7 @@ class AutoNewWindowsUser(AutoNewUser): self.username, str(commands_to_deactivate_user) ) ) - _ = subprocess.check_output( - commands_to_deactivate_user, stderr=subprocess.STDOUT, shell=True - ) + _ = subprocess.check_output(commands_to_deactivate_user, stderr=subprocess.STDOUT) except Exception as err: raise NewUserError("Can't deactivate user {}. Info: {}".format(self.username, err)) @@ -141,8 +139,6 @@ class AutoNewWindowsUser(AutoNewUser): self.username, str(commands_to_delete_user) ) ) - _ = subprocess.check_output( - commands_to_delete_user, stderr=subprocess.STDOUT, shell=True - ) + _ = subprocess.check_output(commands_to_delete_user, stderr=subprocess.STDOUT) except Exception as err: raise NewUserError("Can't delete user {}. Info: {}".format(self.username, err)) diff --git a/monkey/infection_monkey/windows_upgrader.py b/monkey/infection_monkey/windows_upgrader.py index cea71a326..d81b7dc52 100644 --- a/monkey/infection_monkey/windows_upgrader.py +++ b/monkey/infection_monkey/windows_upgrader.py @@ -1,4 +1,5 @@ import logging +import shlex import shutil import subprocess import sys @@ -45,14 +46,20 @@ class WindowsUpgrader(object): opts.parent, opts.tunnel, opts.server, opts.depth ) + # TODO: Replace all of this string templating with a function that accepts + # the necessary parameters and returns a list of arguments. monkey_cmdline = ( MONKEY_CMDLINE_WINDOWS % {"monkey_path": WormConfiguration.dropper_target_path_win_64} + monkey_options ) - monkey_process = subprocess.Popen( + monkey_cmdline_split = shlex.split( monkey_cmdline, - shell=True, + posix=False, # won't try resolving "\" in paths as part of escape sequences + ) + + monkey_process = subprocess.Popen( + monkey_cmdline_split, stdin=None, stdout=None, stderr=None, diff --git a/monkey/monkey_island/Pipfile b/monkey/monkey_island/Pipfile index 46c36c7de..70c0f304c 100644 --- a/monkey/monkey_island/Pipfile +++ b/monkey/monkey_island/Pipfile @@ -36,6 +36,7 @@ mongomock = "==3.19.0" pytest = ">=5.4" requests-mock = "==1.8.0" black = "==20.8b1" +dlint = "==0.11.0" flake8 = "==3.9.0" pytest-cov = "*" isort = "==5.8.0" diff --git a/monkey/monkey_island/Pipfile.lock b/monkey/monkey_island/Pipfile.lock index 704ef3bc9..55f5e4502 100644 --- a/monkey/monkey_island/Pipfile.lock +++ b/monkey/monkey_island/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "9d575bcb35dba0389e8a68dd285f9ed7015fbcc4a42cffff5301777497337616" + "sha256": "4fdfe90af14139cf855a0363ad0acbe7fb307b35b038e2c099c4d1227322a13b" }, "pipfile-spec": 6, "requires": { @@ -171,7 +171,7 @@ "sha256:7d73d2a99753107a36ac6b455ee49046802e59d9d076ef8e47b61499fa29afff", "sha256:e96da0d330793e2cb9485e9ddfd918d456036c7149416295932478192f4436a1" ], - "markers": "python_version != '3.4' and sys_platform == 'win32'", + "markers": "python_version != '3.4'", "version": "==0.4.3" }, "coloredlogs": { @@ -1076,7 +1076,7 @@ "sha256:7d73d2a99753107a36ac6b455ee49046802e59d9d076ef8e47b61499fa29afff", "sha256:e96da0d330793e2cb9485e9ddfd918d456036c7149416295932478192f4436a1" ], - "markers": "python_version != '3.4' and sys_platform == 'win32'", + "markers": "python_version != '3.4'", "version": "==0.4.3" }, "coverage": { @@ -1144,6 +1144,13 @@ ], "version": "==0.3.1" }, + "dlint": { + "hashes": [ + "sha256:e7297325f57e6b5318d88fba2497f9fea6830458cd5aecb36150856db010f409" + ], + "index": "pypi", + "version": "==0.11.0" + }, "filelock": { "hashes": [ "sha256:18d82244ee114f543149c66a6e0c14e9c4f8a1044b5cdaadd0f82159d6a6ff59", diff --git a/monkey/monkey_island/cc/server_utils/bootloader_server.py b/monkey/monkey_island/cc/server_utils/bootloader_server.py index 1532f1a8d..cc2742991 100644 --- a/monkey/monkey_island/cc/server_utils/bootloader_server.py +++ b/monkey/monkey_island/cc/server_utils/bootloader_server.py @@ -33,9 +33,9 @@ class BootloaderHTTPRequestHandler(BaseHTTPRequestHandler): # The island server doesn't always have a correct SSL cert installed # (By default it comes with a self signed one), # that's why we're not verifying the cert in this request. - r = requests.post( + r = requests.post( # noqa: DUO123 url=island_server_path, data=post_data, verify=False, timeout=SHORT_REQUEST_TIMEOUT - ) # noqa: DUO123 + ) try: if r.status_code != 200: diff --git a/monkey/tests/infection_monkey/utils/linux/test_users.py b/monkey/tests/infection_monkey/utils/linux/test_users.py index 67a3a35d4..8b0408c0a 100644 --- a/monkey/tests/infection_monkey/utils/linux/test_users.py +++ b/monkey/tests/infection_monkey/utils/linux/test_users.py @@ -9,7 +9,7 @@ TEST_USER = "test_user" @pytest.fixture def subprocess_check_output_spy(monkeypatch): - def mock_check_output(command, stderr, shell): + def mock_check_output(command, stderr): mock_check_output.command = command mock_check_output.command = "" @@ -21,11 +21,11 @@ def subprocess_check_output_spy(monkeypatch): def test_new_user_expires(subprocess_check_output_spy): with (AutoNewLinuxUser(TEST_USER, "password")): - assert "--expiredate" in subprocess_check_output_spy.command - assert "--inactive 0" in subprocess_check_output_spy.command + assert "--expiredate" in " ".join(subprocess_check_output_spy.command) + assert "--inactive 0" in " ".join(subprocess_check_output_spy.command) def test_new_user_try_delete(subprocess_check_output_spy): with (AutoNewLinuxUser(TEST_USER, "password")): pass - assert f"deluser {TEST_USER}" in subprocess_check_output_spy.command + assert f"deluser {TEST_USER}" in " ".join(subprocess_check_output_spy.command)