From 57b61180ced7309b6b746f13035847cb6e24d6cb Mon Sep 17 00:00:00 2001 From: Shreya Date: Wed, 21 Apr 2021 18:22:24 +0530 Subject: [PATCH 01/14] Add dlint as an additional dependency for the flake8 pre-commit hook --- .pre-commit-config.yaml | 1 + 1 file changed, 1 insertion(+) 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: From d8c1bf5cbe57aa7da6614bcfc88985a286ac056a Mon Sep 17 00:00:00 2001 From: Shreya Date: Wed, 21 Apr 2021 18:37:40 +0530 Subject: [PATCH 02/14] Add dlint to Pipfile --- monkey/monkey_island/Pipfile | 1 + monkey/monkey_island/Pipfile.lock | 13 ++++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) 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", From a3fa4663cb795cebbe1a978bf11ac88ae1388026 Mon Sep 17 00:00:00 2001 From: Shreya Date: Wed, 21 Apr 2021 19:11:13 +0530 Subject: [PATCH 03/14] Fix DUO102 warnings Added comments to ignore some because: "Python uses the Mersenne Twister as the core generator. However, being completely deterministic, it is not suitable for all purposes, and is completely unsuitable for cryptographic purposes. Because the generator is deterministic this means attackers can predict future values given a sufficient amount of previous values. Normal random use is acceptable if the relevant code is not used for security or cryptographic purposes." --- monkey/infection_monkey/exploit/hadoop.py | 5 +++-- monkey/infection_monkey/exploit/shellshock.py | 5 +++-- monkey/infection_monkey/network/info.py | 2 +- monkey/infection_monkey/network/tcp_scanner.py | 2 +- .../post_breach/actions/communicate_as_new_user.py | 4 ++-- 5 files changed, 10 insertions(+), 8 deletions(-) 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..f83eb9a15 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 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/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()): From af381e062ff3d5f06a5fbbe80c4395a652a28178 Mon Sep 17 00:00:00 2001 From: Shreya Date: Wed, 21 Apr 2021 19:30:32 +0530 Subject: [PATCH 04/14] Fix DUO106 warnings (Introduces a DUO116 warning) --- monkey/infection_monkey/utils/linux/users.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/monkey/infection_monkey/utils/linux/users.py b/monkey/infection_monkey/utils/linux/users.py index fa91fced8..112de0655 100644 --- a/monkey/infection_monkey/utils/linux/users.py +++ b/monkey/infection_monkey/utils/linux/users.py @@ -1,6 +1,5 @@ import datetime import logging -import os import subprocess from infection_monkey.utils.auto_new_user import AutoNewUser @@ -54,7 +53,7 @@ class AutoNewLinuxUser(AutoNewUser): command_as_new_user = "sudo -u {username} {command}".format( username=self.username, command=command ) - return os.system(command_as_new_user) + return subprocess.call(command_as_new_user, shell=True) def __exit__(self, exc_type, exc_val, exc_tb): # delete the user. From b0be14193d556fab3b1158e64b9bcda051a572a6 Mon Sep 17 00:00:00 2001 From: Shreya Date: Thu, 22 Apr 2021 16:04:25 +0530 Subject: [PATCH 05/14] Fix DUO122 warnings --- monkey/infection_monkey/exploit/struts2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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") From c0fdc9561f22b6350b03e1775a4969d554ebb98b Mon Sep 17 00:00:00 2001 From: Shreya Date: Thu, 22 Apr 2021 16:39:19 +0530 Subject: [PATCH 06/14] Fix DUO123 warnings --- .../island_client/monkey_island_requests.py | 22 ++++---- monkey/infection_monkey/control.py | 54 +++++++++---------- monkey/infection_monkey/exploit/drupal.py | 12 ++--- monkey/infection_monkey/exploit/shellshock.py | 4 +- monkey/infection_monkey/exploit/weblogic.py | 8 +-- monkey/infection_monkey/transport/http.py | 2 +- .../cc/server_utils/bootloader_server.py | 4 +- 7 files changed, 52 insertions(+), 54 deletions(-) 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/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/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/shellshock.py b/monkey/infection_monkey/exploit/shellshock.py index f83eb9a15..7bca6b04b 100644 --- a/monkey/infection_monkey/exploit/shellshock.py +++ b/monkey/infection_monkey/exploit/shellshock.py @@ -243,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/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/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/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: From 6b467fd20b2578a3f0f0d044ef3af7d5b928ff63 Mon Sep 17 00:00:00 2001 From: Shreya Date: Thu, 22 Apr 2021 19:37:25 +0530 Subject: [PATCH 07/14] Fix DUO116 warnings in monkey/infection_monkey/utils/linux/users.py --- monkey/infection_monkey/utils/linux/users.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/monkey/infection_monkey/utils/linux/users.py b/monkey/infection_monkey/utils/linux/users.py index 112de0655..9bd3c2bf8 100644 --- a/monkey/infection_monkey/utils/linux/users.py +++ b/monkey/infection_monkey/utils/linux/users.py @@ -1,5 +1,6 @@ import datetime import logging +import shlex import subprocess from infection_monkey.utils.auto_new_user import AutoNewUser @@ -42,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 subprocess.call(command_as_new_user, shell=True) + return subprocess.call(command_as_new_user) def __exit__(self, exc_type, exc_val, exc_tb): # delete the user. @@ -63,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) From 4d88efdd843a02b7b2d255369306f779d4330e74 Mon Sep 17 00:00:00 2001 From: Shreya Date: Sat, 24 Apr 2021 14:04:57 +0530 Subject: [PATCH 08/14] Fix DUO116 warnings in post breach actions by ignoring them --- .../post_breach/actions/clear_command_history.py | 4 ++-- .../post_breach/actions/modify_shell_startup_files.py | 4 ++-- .../post_breach/actions/use_signed_scripts.py | 5 ++--- monkey/infection_monkey/post_breach/pba.py | 2 +- .../windows/shell_startup_files_modification.py | 6 ++++-- .../post_breach/signed_script_proxy/signed_script_proxy.py | 4 ++-- monkey/infection_monkey/utils/hidden_files.py | 4 ++-- 7 files changed, 15 insertions(+), 14 deletions(-) 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/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/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, ) From 294e8fe56adb2a2f356a07ab2c9d988014db88ea Mon Sep 17 00:00:00 2001 From: Shreya Date: Sat, 24 Apr 2021 21:08:59 +0530 Subject: [PATCH 09/14] Fix DU0116 warnings in blackbox tests by ignoring them --- .../blackbox/utils/gcp_machine_handlers.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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) From 410cbadbb3fab7303dbb1109008fd4356cea4a0f Mon Sep 17 00:00:00 2001 From: Shreya Date: Sun, 25 Apr 2021 00:25:13 +0530 Subject: [PATCH 10/14] Fix DUO116 warnings for: - monkey/infection_monkey/dropper.py - monkey/infection_monkey/system_info/windows_info_collector.py - monkey/infection_monkey/utils/windows/users.py - monkey/infection_monkey/windows_upgrader.py --- monkey/infection_monkey/dropper.py | 6 ++++-- .../system_info/windows_info_collector.py | 5 +++-- monkey/infection_monkey/utils/windows/users.py | 10 +++------- monkey/infection_monkey/windows_upgrader.py | 6 ++++-- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/monkey/infection_monkey/dropper.py b/monkey/infection_monkey/dropper.py index 3d34688ef..3b7bba818 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 @@ -164,9 +165,10 @@ class MonkeyDrops(object): "monkey_commandline": inner_monkey_cmdline, } + monkey_cmdline_split = shlex.split(monkey_cmdline) + monkey_process = subprocess.Popen( - monkey_cmdline, - shell=True, + monkey_cmdline_split, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, 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/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..8739ae556 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 @@ -50,9 +51,10 @@ class WindowsUpgrader(object): + monkey_options ) + monkey_cmdline_split = shlex.split(monkey_cmdline) + monkey_process = subprocess.Popen( - monkey_cmdline, - shell=True, + monkey_cmdline_split, stdin=None, stdout=None, stderr=None, From 9602a67d28995f8d57db77946715833950d6c17f Mon Sep 17 00:00:00 2001 From: Shreya Date: Sun, 25 Apr 2021 00:31:21 +0530 Subject: [PATCH 11/14] Modify unit tests: tests/infection_monkey/utils/linux/test_users.py --- monkey/tests/infection_monkey/utils/linux/test_users.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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) From b50faceba72d25c4e40cd045a335423bc1648dc6 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 26 Apr 2021 12:03:16 -0400 Subject: [PATCH 12/14] Add a changelog entry for dlint work --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) 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 From d4e277c70b24364e1ad7eaddeae3f6e9dba56032 Mon Sep 17 00:00:00 2001 From: Shreya Date: Wed, 28 Apr 2021 18:57:04 +0530 Subject: [PATCH 13/14] Modify what commands are passed to `subprocess.Popen` in the dropper and windows_upgrader --- monkey/infection_monkey/dropper.py | 50 +++++++++++---------- monkey/infection_monkey/windows_upgrader.py | 5 ++- 2 files changed, 31 insertions(+), 24 deletions(-) diff --git a/monkey/infection_monkey/dropper.py b/monkey/infection_monkey/dropper.py index 3b7bba818..ec0c5c03e 100644 --- a/monkey/infection_monkey/dropper.py +++ b/monkey/infection_monkey/dropper.py @@ -14,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 @@ -151,30 +147,38 @@ class MonkeyDrops(object): 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 + 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_cmdline_split = shlex.split(monkey_cmdline) - - monkey_process = subprocess.Popen( - monkey_cmdline_split, - 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/windows_upgrader.py b/monkey/infection_monkey/windows_upgrader.py index 8739ae556..db1446a45 100644 --- a/monkey/infection_monkey/windows_upgrader.py +++ b/monkey/infection_monkey/windows_upgrader.py @@ -51,7 +51,10 @@ class WindowsUpgrader(object): + monkey_options ) - monkey_cmdline_split = shlex.split(monkey_cmdline) + 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, From e5935e43c1eb2afb26036641efee76937f5d0cc3 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Wed, 28 Apr 2021 11:00:53 -0400 Subject: [PATCH 14/14] agent: Add TODOs regarding string templates. --- monkey/infection_monkey/dropper.py | 4 ++++ monkey/infection_monkey/windows_upgrader.py | 2 ++ 2 files changed, 6 insertions(+) diff --git a/monkey/infection_monkey/dropper.py b/monkey/infection_monkey/dropper.py index ec0c5c03e..902d30280 100644 --- a/monkey/infection_monkey/dropper.py +++ b/monkey/infection_monkey/dropper.py @@ -143,6 +143,8 @@ 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 @@ -164,6 +166,8 @@ class MonkeyDrops(object): dest_path = self._config["destination_path"] # 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 diff --git a/monkey/infection_monkey/windows_upgrader.py b/monkey/infection_monkey/windows_upgrader.py index db1446a45..d81b7dc52 100644 --- a/monkey/infection_monkey/windows_upgrader.py +++ b/monkey/infection_monkey/windows_upgrader.py @@ -46,6 +46,8 @@ 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