From 937dbac4d04c5d4af13a176ebe9888a3054c8530 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 10 Jun 2021 06:50:00 -0400 Subject: [PATCH 1/8] island: Remove SSL permissions checks These checks prevent the docker container from working properly, as the default SSL cert must have at least 444 permissions. --- monkey/common/utils/exceptions.py | 4 - .../cc/server_utils/file_utils.py | 52 ----------- .../setup/island_config_options_validator.py | 21 ----- .../cc/server_utils/test_file_utils.py | 35 -------- .../test_island_config_options_validator.py | 86 ++----------------- 5 files changed, 6 insertions(+), 192 deletions(-) diff --git a/monkey/common/utils/exceptions.py b/monkey/common/utils/exceptions.py index 632c08991..8396b423b 100644 --- a/monkey/common/utils/exceptions.py +++ b/monkey/common/utils/exceptions.py @@ -52,7 +52,3 @@ class FindingWithoutDetailsError(Exception): class DomainControllerNameFetchError(FailedExploitationError): """ Raise on failed attempt to extract domain controller's name """ - - -class InsecurePermissionsError(Exception): - """ Raise when a file does not have permissions that are secure enough """ diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 668aa4356..225fb8732 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -1,57 +1,5 @@ import os -from monkey_island.cc.environment.utils import is_windows_os - def expand_path(path: str) -> str: return os.path.expandvars(os.path.expanduser(path)) - - -def has_expected_permissions(path: str, expected_permissions: int) -> bool: - if is_windows_os(): - return _has_expected_windows_permissions(path, expected_permissions) - - return _has_expected_linux_permissions(path, expected_permissions) - - -def _has_expected_linux_permissions(path: str, expected_permissions: int) -> bool: - file_mode = os.stat(path).st_mode - file_permissions = file_mode & 0o777 - - return file_permissions == expected_permissions - - -def _has_expected_windows_permissions(path: str, expected_permissions: int) -> bool: - import win32api # noqa: E402 - import win32security # noqa: E402 - - FULL_CONTROL = 2032127 - ACE_TYPE_ALLOW = 0 - ACE_TYPE_DENY = 1 - - admins_sid, _, _ = win32security.LookupAccountName("", "Administrators") - user_sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) - - security_descriptor = win32security.GetNamedSecurityInfo( - path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION - ) - - acl = security_descriptor.GetSecurityDescriptorDacl() - - for i in range(acl.GetAceCount()): - ace = acl.GetAce(i) - ace_type, _ = ace[0] # 0 for allow, 1 for deny - permissions = ace[1] - sid = ace[-1] - - if sid == user_sid: - if not (permissions == expected_permissions and ace_type == ACE_TYPE_ALLOW): - return False - elif sid == admins_sid: - continue - # TODO: consider removing; so many system accounts/groups exist, it's likely to fail - else: - if not (permissions == FULL_CONTROL and ace_type == ACE_TYPE_DENY): - return False - - return True diff --git a/monkey/monkey_island/cc/setup/island_config_options_validator.py b/monkey/monkey_island/cc/setup/island_config_options_validator.py index 5febe16ab..032eeb2e2 100644 --- a/monkey/monkey_island/cc/setup/island_config_options_validator.py +++ b/monkey/monkey_island/cc/setup/island_config_options_validator.py @@ -1,34 +1,13 @@ import os -from common.utils.exceptions import InsecurePermissionsError -from monkey_island.cc.environment.utils import is_windows_os -from monkey_island.cc.server_utils.file_utils import has_expected_permissions from monkey_island.cc.setup.island_config_options import IslandConfigOptions def raise_on_invalid_options(options: IslandConfigOptions): - LINUX_READ_ONLY_BY_USER = 0o400 - WINDOWS_READ_ONLY = 1179817 - _raise_if_not_isfile(options.crt_path) - _raise_if_incorrect_permissions(options.crt_path, LINUX_READ_ONLY_BY_USER, WINDOWS_READ_ONLY) - _raise_if_not_isfile(options.key_path) - _raise_if_incorrect_permissions(options.key_path, LINUX_READ_ONLY_BY_USER, WINDOWS_READ_ONLY) def _raise_if_not_isfile(f: str): if not os.path.isfile(f): raise FileNotFoundError(f"{f} does not exist or is not a regular file.") - - -def _raise_if_incorrect_permissions( - f: str, linux_expected_permissions: int, windows_expected_permissions: int -): - expected_permissions = ( - windows_expected_permissions if is_windows_os() else linux_expected_permissions - ) - if not has_expected_permissions(f, expected_permissions): - raise InsecurePermissionsError( - f"The file {f} has incorrect permissions. Expected: {expected_permissions}" - ) diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/test_file_utils.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/test_file_utils.py index 6297aada9..cff716135 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/test_file_utils.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/test_file_utils.py @@ -1,7 +1,4 @@ import os -import subprocess - -import pytest from monkey_island.cc.server_utils import file_utils @@ -18,35 +15,3 @@ def test_expand_vars(patched_home_env): expected_path = os.path.join(patched_home_env, "test") assert file_utils.expand_path(input_path) == expected_path - - -@pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") -def test_has_expected_permissions_true_linux(tmpdir, create_empty_tmp_file): - file_name = create_empty_tmp_file("test") - os.chmod(file_name, 0o754) - - assert file_utils.has_expected_permissions(file_name, 0o754) - - -@pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") -def test_has_expected_permissions_false_linux(tmpdir, create_empty_tmp_file): - file_name = create_empty_tmp_file("test") - os.chmod(file_name, 0o755) - - assert not file_utils.has_expected_permissions(file_name, 0o700) - - -@pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") -def test_has_expected_permissions_true_windows(tmpdir, create_empty_tmp_file): - file_name = create_empty_tmp_file("test") - subprocess.run(f"echo y| cacls {file_name} /p %USERNAME%:F", shell=True) # noqa: DUO116 - - assert file_utils.has_expected_permissions(file_name, 2032127) - - -@pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") -def test_has_expected_permissions_false_windows(tmpdir, create_empty_tmp_file): - file_name = create_empty_tmp_file("test") - subprocess.run(f"echo y| cacls {file_name} /p %USERNAME%:R", shell=True) # noqa: DUO116 - - assert not file_utils.has_expected_permissions(file_name, 2032127) diff --git a/monkey/tests/unit_tests/monkey_island/cc/setup/test_island_config_options_validator.py b/monkey/tests/unit_tests/monkey_island/cc/setup/test_island_config_options_validator.py index b6d9eeb85..9fb132305 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/setup/test_island_config_options_validator.py +++ b/monkey/tests/unit_tests/monkey_island/cc/setup/test_island_config_options_validator.py @@ -1,16 +1,11 @@ import os -import subprocess from collections.abc import Callable import pytest -from common.utils.exceptions import InsecurePermissionsError from monkey_island.cc.setup.island_config_options import IslandConfigOptions from monkey_island.cc.setup.island_config_options_validator import raise_on_invalid_options -LINUX_READ_ONLY_BY_USER = 0o400 -LINUX_RWX_BY_ALL = 0o777 - def certificate_test_island_config_options(crt_file, key_file): return IslandConfigOptions( @@ -24,24 +19,13 @@ def certificate_test_island_config_options(crt_file, key_file): @pytest.fixture -def linux_island_config_options(create_read_only_linux_file: Callable): - crt_file = create_read_only_linux_file("test.crt") - key_file = create_read_only_linux_file("test.key") +def linux_island_config_options(create_empty_tmp_file: Callable): + crt_file = create_empty_tmp_file("test.crt") + key_file = create_empty_tmp_file("test.key") return certificate_test_island_config_options(crt_file, key_file) -@pytest.fixture -def create_read_only_linux_file(tmpdir: str, create_empty_tmp_file: Callable) -> Callable: - def inner(file_name: str) -> str: - full_file_path = create_empty_tmp_file(file_name) - os.chmod(full_file_path, LINUX_READ_ONLY_BY_USER) - - return full_file_path - - return inner - - @pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") def test_linux_valid_crt_and_key_paths(linux_island_config_options): try: @@ -59,14 +43,6 @@ def test_linux_crt_path_does_not_exist(linux_island_config_options): raise_on_invalid_options(linux_island_config_options) -@pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") -def test_linux_crt_path_insecure_permissions(linux_island_config_options): - os.chmod(linux_island_config_options.crt_path, LINUX_RWX_BY_ALL) - - with pytest.raises(InsecurePermissionsError): - raise_on_invalid_options(linux_island_config_options) - - @pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") def test_linux_key_path_does_not_exist(linux_island_config_options): os.remove(linux_island_config_options.key_path) @@ -75,42 +51,14 @@ def test_linux_key_path_does_not_exist(linux_island_config_options): raise_on_invalid_options(linux_island_config_options) -@pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") -def test_linux_key_path_insecure_permissions(linux_island_config_options): - os.chmod(linux_island_config_options.key_path, LINUX_RWX_BY_ALL) - - with pytest.raises(InsecurePermissionsError): - raise_on_invalid_options(linux_island_config_options) - - @pytest.fixture -def windows_island_config_options(tmpdir: str, create_read_only_windows_file: Callable): - crt_file = create_read_only_windows_file("test.crt") - key_file = create_read_only_windows_file("test.key") +def windows_island_config_options(tmpdir: str, create_empty_tmp_file: Callable): + crt_file = create_empty_tmp_file("test.crt") + key_file = create_empty_tmp_file("test.key") return certificate_test_island_config_options(crt_file, key_file) -@pytest.fixture -def create_read_only_windows_file(tmpdir: str, create_empty_tmp_file: Callable) -> Callable: - def inner(file_name: str) -> str: - full_file_path = create_empty_tmp_file(file_name) - cmd_to_change_permissions = get_windows_cmd_to_change_permissions(full_file_path, "R") - subprocess.run(cmd_to_change_permissions, shell=True) # noqa DUO116 - - return full_file_path - - return inner - - -def get_windows_cmd_to_change_permissions(file_name, permissions): - """ - :param file_name: name of file - :param permissions: can be: N (None), R (Read), W (Write), C (Change (write)), F (Full control) - """ - return f"echo y| cacls {file_name} /p %USERNAME%:{permissions}" - - @pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") def test_windows_valid_crt_and_key_paths(windows_island_config_options): try: @@ -128,31 +76,9 @@ def test_windows_crt_path_does_not_exist(windows_island_config_options): raise_on_invalid_options(windows_island_config_options) -@pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") -def test_windows_crt_path_insecure_permissions(windows_island_config_options): - cmd_to_change_permissions = get_windows_cmd_to_change_permissions( - windows_island_config_options.crt_path, "W" - ) - subprocess.run(cmd_to_change_permissions, shell=True) # noqa DUO116 - - with pytest.raises(InsecurePermissionsError): - raise_on_invalid_options(windows_island_config_options) - - @pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") def test_windows_key_path_does_not_exist(windows_island_config_options): os.remove(windows_island_config_options.key_path) with pytest.raises(FileNotFoundError): raise_on_invalid_options(windows_island_config_options) - - -@pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") -def test_windows_key_path_insecure_permissions(windows_island_config_options): - cmd_to_change_permissions = get_windows_cmd_to_change_permissions( - windows_island_config_options.key_path, "W" - ) - subprocess.run(cmd_to_change_permissions, shell=True) # noqa DUO116 - - with pytest.raises(InsecurePermissionsError): - raise_on_invalid_options(windows_island_config_options) From 118dfa53d4645c2cb6331b559803431584ca70c7 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 10 Jun 2021 07:13:47 -0400 Subject: [PATCH 2/8] docs: Loosen ssl permissions recommendation for appimage --- docs/content/setup/linux.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/content/setup/linux.md b/docs/content/setup/linux.md index 85e4a0f13..052da81d2 100644 --- a/docs/content/setup/linux.md +++ b/docs/content/setup/linux.md @@ -41,12 +41,11 @@ private certificate authority. 1. (Optional but recommended) Move your `.crt` and `.key` files to `$HOME/.monkey_island`. -1. Make sure that your `.crt` and `.key` files are read-only and readable only - by you. +1. Make sure that your `.crt` and `.key` files are readable only by you. ```bash - chmod 400 - chmod 400 + chmod 600 + chmod 600 ``` 1. Edit `$HOME/.monkey_island/server_config.json` to configure Monkey Island From 6a1a1721bdf9403f2b34573a9666104dbe88e577 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 10 Jun 2021 07:38:29 -0400 Subject: [PATCH 3/8] island: Loosen permissions on ssl cert in create_certificate.sh --- monkey/monkey_island/linux/create_certificate.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/linux/create_certificate.sh b/monkey/monkey_island/linux/create_certificate.sh index cbbe5261b..acdbd5b9d 100644 --- a/monkey/monkey_island/linux/create_certificate.sh +++ b/monkey/monkey_island/linux/create_certificate.sh @@ -21,15 +21,15 @@ umask 377 echo "Generating key in $server_root/server.key..." openssl genrsa -out "$server_root"/server.key 2048 -chmod 400 "$server_root"/server.key +chmod 600 "$server_root"/server.key echo "Generating csr in $server_root/server.csr..." openssl req -new -key "$server_root"/server.key -out "$server_root"/server.csr -subj "/C=GB/ST=London/L=London/O=Global Security/OU=Monkey Department/CN=monkey.com" -chmod 400 "$server_root"/server.csr +chmod 600 "$server_root"/server.csr echo "Generating certificate in $server_root/server.crt..." openssl x509 -req -days 366 -in "$server_root"/server.csr -signkey "$server_root"/server.key -out "$server_root"/server.crt -chmod 400 "$server_root"/server.crt +chmod 600 "$server_root"/server.crt # Shove some new random data into the file to override the original seed we put in. From 348118a5ed65792a81e821f1b24ef2d74f359de2 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 10 Jun 2021 10:50:59 -0400 Subject: [PATCH 4/8] docs: Minor corrections to docker certificate setup --- docs/content/setup/docker.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/content/setup/docker.md b/docs/content/setup/docker.md index 478cb6660..f9335dc44 100644 --- a/docs/content/setup/docker.md +++ b/docs/content/setup/docker.md @@ -82,13 +82,13 @@ been signed by a private certificate authority. guardicore/monkey-island:1.10.0 --setup-only ``` -1. (Optional but recommended) Move your `.crt` and `.key` files to `./monkey_island_data`. +1. Move your `.crt` and `.key` files to `./monkey_island_data`. -1. Make sure that your `.crt` and `.key` files are read-only and readable only by you. +1. Make sure that your `.crt` and `.key` files are readable only by you. ```bash - chmod 400 - chmod 400 + chmod 600 ./monkey_island_data/ + chmod 600 ./monkey_island_data/ ``` 1. Edit `./monkey_island_data/server_config.json` to configure Monkey Island @@ -106,8 +106,8 @@ been signed by a private certificate authority. "start_mongodb": false }, "ssl_certificate": { - "ssl_certificate_file": "", - "ssl_certificate_key_file": "", + "ssl_certificate_file": "/monkey_island_data/", + "ssl_certificate_key_file": "/monkey_island_data/" } } ``` From 9b7c9130079d2b310f0b907d5245e6d4c15abee9 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 10 Jun 2021 11:08:18 -0400 Subject: [PATCH 5/8] docs: Remove errant comma in linux setup --- docs/content/setup/linux.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/setup/linux.md b/docs/content/setup/linux.md index 052da81d2..2340907ba 100644 --- a/docs/content/setup/linux.md +++ b/docs/content/setup/linux.md @@ -64,7 +64,7 @@ private certificate authority. }, "ssl_certificate": { "ssl_certificate_file": "", - "ssl_certificate_key_file": "", + "ssl_certificate_key_file": "" } } ``` From 7b14b917cdc707f4b1414fd5f26a71f8df6e72a2 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 10 Jun 2021 11:33:05 -0400 Subject: [PATCH 6/8] docs: Tell the user to set secure permissions on /monkey_island_data --- docs/content/setup/docker.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/content/setup/docker.md b/docs/content/setup/docker.md index f9335dc44..06bab3e72 100644 --- a/docs/content/setup/docker.md +++ b/docs/content/setup/docker.md @@ -68,6 +68,7 @@ been signed by a private certificate authority. ```bash mkdir ./monkey_island_data + chmod 700 ./monkey_island_data ``` 1. Run Monkey Island with the `--setup-only` flag to populate the `./monkey_island_data` directory with a default `server_config.json` file. From ca5450b932b2c4909d5fa03917274c17788b895c Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 10 Jun 2021 11:34:08 -0400 Subject: [PATCH 7/8] island: Add quotes around --user argument in docker setup --- docs/content/setup/docker.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/content/setup/docker.md b/docs/content/setup/docker.md index 06bab3e72..18491504f 100644 --- a/docs/content/setup/docker.md +++ b/docs/content/setup/docker.md @@ -78,7 +78,7 @@ been signed by a private certificate authority. --rm \ --name monkey-island \ --network=host \ - --user $(id -u ${USER}):$(id -g ${USER}) \ + --user "$(id -u ${USER}):$(id -g ${USER})" \ --volume "$(realpath ./monkey_island_data)":/monkey_island_data \ guardicore/monkey-island:1.10.0 --setup-only ``` @@ -119,7 +119,7 @@ been signed by a private certificate authority. sudo docker run \ --name monkey-island \ --network=host \ - --user $(id -u ${USER}):$(id -g ${USER}) \ + --user "$(id -u ${USER}):$(id -g ${USER})" \ --volume "$(realpath ./monkey_island_data)":/monkey_island_data \ guardicore/monkey-island:1.10.0 ``` From 331d2aeb7e9c87cded32a57013923a7aaf1af95b Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 10 Jun 2021 13:08:02 -0400 Subject: [PATCH 8/8] docs: Add "writable" for more accurate description Co-authored-by: Shreya Malviya --- docs/content/setup/docker.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/setup/docker.md b/docs/content/setup/docker.md index 18491504f..fec2701d4 100644 --- a/docs/content/setup/docker.md +++ b/docs/content/setup/docker.md @@ -85,7 +85,7 @@ been signed by a private certificate authority. 1. Move your `.crt` and `.key` files to `./monkey_island_data`. -1. Make sure that your `.crt` and `.key` files are readable only by you. +1. Make sure that your `.crt` and `.key` files are readable and writeable only by you. ```bash chmod 600 ./monkey_island_data/