Create mongo key file securely #1232

shreyamalviya · 2021-06-14T18:40:27Z

What does this PR do?

Fixes #1195

PR Checklist

Have you added an explanation of what your changes do and why you'd like to include them?
Is the TravisCI build passing?
Was the CHANGELOG.md updated to reflect the changes?
Was the documentation framework updated to reflect the changes?

Testing Checklist

Added relevant unit tests?
Have you successfully tested your changes locally? Elaborate:

Tested by {Running the Monkey locally with relevant config/running Island/...}
If applicable, add screenshots or log transcripts of the feature working

Explain Changes

Are the commit messages enough? If not, elaborate.

…eate a file that exists

…file descriptor

…st_utils.py

monkey/monkey_island/cc/environment/utils.py

mssalvatore · 2021-06-14T18:52:47Z

monkey/monkey_island/cc/environment/utils.py

+        flags = (
+            os.O_RDWR | os.O_CREAT | os.O_EXCL
+        )  # read/write, create new, throw error if file exists
+        mode = 0o700  # read/write/execute permissions to owner


Maybe we should use stat.S_IRWUX here instead of a magic number.

mssalvatore · 2021-06-14T18:52:53Z

monkey/monkey_island/cc/environment/utils.py

@@ -29,11 +30,9 @@ def _create_secure_directory_linux(path: str):
        # Don't split directory creation and permission setting
        # because it will temporarily create an accessible directory which anyone can use.
        os.mkdir(path, mode=0o700)


Maybe we should use stat.S_IRWUX here instead of a magic number.

That "magic number" is the standard unix permission expression, when you see 700 it's clear that it's: owner (read + write + execute), group (none), public (none). What S_IRWUX means heaven only knows.

I mistyped it. It's S_IRWXU which is for Read Write eXecute User. It's very commonly seen in code written for Linux.

I'm fine with either, but let's be consistent. We're using the constatns from stats in a test, but the magic number elsewhere.

mssalvatore · 2021-06-14T18:55:36Z

monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py

+def test_create_secure_file__already_created(test_path):
+    os.close(os.open(test_path, os.O_CREAT, 0o700))
+    assert os.path.isfile(test_path)
+    create_secure_file(test_path)


This test isn't asserting anything after create_secure_file(), which makes it unclear what it's testing.

I think it's relying on the fact that if an exception is thrown, the test will fail. In essence, it's testing that no exceptions are thrown. Maybe a comment is in order. # test will fail if any exceptions are thrown or something similar that explains the intent of the test.

monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py

mssalvatore · 2021-06-14T19:03:17Z

monkey/monkey_island/cc/server_utils/encryptor.py

@@ -21,6 +23,7 @@ def __init__(self, password_file_dir):
        if os.path.exists(password_file):
            self._load_existing_key(password_file)
        else:
+            create_secure_file(path=password_file)


I believe if create_secure_file() returns the file handler without closing, you could do something like:

with open(create_secure_file(password_file), "wb"):

This would avoid the os.close(os.open(...)) ugliness in the create_secure_file() function. You should probably then rename it to open_new_secure_file() or something.

What purpose would that serve? We would just be doing something like

with open(create_secure_file(password_file), "wb"): pass

?

Plus, I don't mind the os.close(os.open(...)) ugliness if it means that we're maintaining consistency in utils.py (where we're simply creating secure files/directories, not opening them).

There are better ways to improve readability. Creating and opening and then closing the file is not a very intuitive workflow just for creating a file.

You wouldn't use open() and then pass. The code that used it would look like this:

diff --git a/monkey/monkey_island/cc/server_utils/encryptor.py b/monkey/monkey_island/cc/server_utils/encryptor.py index abeb34dc..9ec3dc37 100644 --- a/monkey/monkey_island/cc/server_utils/encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryptor.py @@ -6,7 +6,7 @@ import os from Crypto import Random # noqa: DUO133 # nosec: B413 from Crypto.Cipher import AES # noqa: DUO133 # nosec: B413 -from monkey_island.cc.server_utils.file_utils import create_secure_file +from monkey_island.cc.server_utils.file_utils import open_new_secure_file __author__ = "itay.mizeretz" @@ -23,12 +23,11 @@ class Encryptor: if os.path.exists(password_file): self._load_existing_key(password_file) else: - create_secure_file(path=password_file) self._init_key(password_file) def _init_key(self, password_file): self._cipher_key = Random.new().read(self._BLOCK_SIZE) - with open(password_file, "wb") as f: + with open(open_new_secure_file(password_file, "wb")) as f: f.write(self._cipher_key) def _load_existing_key(self, password_file):

Rather than close(open()) and then open() the file again later, you would just open the file and write to it. Open a file and write to it is the standard workflow for handling files. Create a directory and use it is the standard workflow for directories. There's not really an inconsistency.

You could consider names like get_file_descriptor_for_secure_file() instead of open_secure_file() or something else. The point is, we should just open a file and write to it.

…explaining when it fails

…e_utils.py

ghost · 2021-06-15T07:22:16Z

DeepCode's analysis on #44bdfa found:

⚠️ 1 warning, ℹ️ 1 minor issue. 👇

Top issues

Description	Example fixes
Use os.makedirs instead of os.mkdir because the given path may require creating the parent directories. Occurrences: file_utils.py:37	🔧 Example fixes
Use os.makedirs instead of os.mkdir because the given path may require creating the parent directories. Occurrences: test_file_utils.py:60	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

VakarisZ · 2021-06-15T07:31:17Z

monkey/monkey_island/cc/environment/utils.py

+            os.O_RDWR | os.O_CREAT | os.O_EXCL
+        )  # read/write, create new, throw error if file exists
+        mode = 0o700  # read/write/execute permissions to owner
+        os.close(os.open(path, flags, mode))


Why do we even need the complexity of the flags? Why not simply Path(path).touch(0o700)?

Nice; changed it.

VakarisZ · 2021-06-15T07:36:54Z

monkey/monkey_island/cc/environment/utils.py

+        file_sharing = (
+            win32file.FILE_SHARE_READ
+        )  # subsequent open operations on the object will succeed only if read access is requested


Strange, why do we need () here?

That's how black formats it. Because it's in the next line.

That means the comment needs to be above the statement and the statement needs to be one like, like so: file_sharing = win32file.FILE_SHARE_READ

VakarisZ · 2021-06-15T08:00:06Z

monkey/monkey_island/cc/environment/utils.py

+                win32job.CreateJobObject(
+                    None, ""
+                ),  # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu  # noqa: E501


No clue about what win32job.CreateJobObject does, why do we call it here, what argument it specifies etc. The whole win32file.CreateFile function should be called with keyword arguments.

Generally, just looks like a dirty workaround, there should be an easier way to pass Null. Either way, this should be hidden in a clearly named function.

The whole win32file.CreateFile function should be called with keyword arguments.

Can't. It throws an error that it can't be called with keyword arguments.

Generally, just looks like a dirty workaround, there should be an easier way to pass Null.

I agree. Couldn't find an easier way, though. I'll try again, perhaps.

It's OK if that's the only way we can provide NULL argument, but you should create a method called get_null_value_for_win32 or something and call it instead. Then it would be clear that we actually provide NULL as the last argument and not some kind of created job.

Lastly, don't put comments that obviously violate line length inline, put them above the statement.

monkey/monkey_island/cc/environment/utils.py

VakarisZ · 2021-06-15T08:04:41Z

monkey/monkey_island/cc/server_utils/file_utils.py

+            windows_permissions.get_security_descriptor_for_owner_only_perms()
+        )
+        file_creation = win32file.CREATE_NEW  # fails if file exists
+        file_attributes = win32file.FILE_FLAG_BACKUP_SEMANTICS


why do we need this flag?

Using win32file.FILE_ATTRIBUTE_NORMAL throws an access denied error. win32file.FILE_FLAG_BACKUP_SEMANTICS doesn't probably because of:

The operating system ensures that the calling process overrides file security checks, provided it has the necessary permission to do so.

(Source: http://timgolden.me.uk/pywin32-docs/win32file_FILE_FLAG_BACKUP_SEMANTICS.html)

VakarisZ

Address comments

…ating a file on Linux

mssalvatore · 2021-06-15T11:24:42Z

The permissions the mongo file should be 600. We don't need the execute bit set on this file.

…_file() get_file_descriptor_for_new_secure_file() should return a file descriptor. If the file already exists, this function would return nothing, potentially causing issues with whatever relies on this function's output.

create_secure_file() was previously renamed to get_file_descriptor_for_new_secure_file().

shreyamalviya added 15 commits June 14, 2021 13:21

island: Create mongo key file securely before using it

26ae50f

island: Modify log message when creating secure directory on Windows

5d7d86a

island: Add functions to create a file securely on Linux and Windows

ff85360

island: Use 'x' instead of '_' when creating a secure file

8dd4bb5

island: Add os.O_EXCL flag so that an error is thrown if trying to cr…

8b932e1

…eate a file that exists

island: Can't use with with os.open(), use os.close() to close …

5fe0c80

…file descriptor

tests: Add unit tests for securly creating a file

248d577

tests: Fix assertion in test_create_secure_directory__perm_linux()

6d360ef

tests: Extract duplicate code in Windows tests in test_utils

c0d9489

island: Fix secure file creation on Windows

37eda4e

island: Use win32file.CloseHandle() to close file descriptor on Windows

1467a53

tests: Fix file creation unit tests in test_utils.py

7ddb986

island: Fix Windows' secure file creation by using a different file flag

1170b17

tests: Remove accidental code in _get_acl_and_sid_from_path() in te…

443b66e

…st_utils.py

island: Format cc/environment/utils.py with black

5ea046e

shreyamalviya requested review from mssalvatore and VakarisZ June 14, 2021 18:40

mssalvatore requested changes Jun 14, 2021

View reviewed changes

shreyamalviya added 5 commits June 15, 2021 12:23

island: Use stat.S_IRWXU in place of 0o700 in cc/environment/utils.py

d7565fc

tests: Add comment to test_create_secure_directory__perm_windows() …

9187334

…explaining when it fails

island: Move code from cc/environment/utils.py to cc/server_utils/fil…

b5f092a

…e_utils.py

tests: Move tests from test_utils.py to test_file_utils.py

5abcadc

island, tests: Run isort and black on previously changed files

e011654

shreyamalviya requested a review from mssalvatore June 15, 2021 07:23

VakarisZ reviewed Jun 15, 2021

View reviewed changes

monkey/monkey_island/cc/environment/utils.py Outdated Show resolved Hide resolved

VakarisZ reviewed Jun 15, 2021

View reviewed changes

VakarisZ requested changes Jun 15, 2021

View reviewed changes

island: Use Path().touch() instead of os.open() when securely cre…

e90bf52

…ating a file on Linux

mssalvatore and others added 8 commits June 15, 2021 09:29

island: Remove execute bit from "secure" file creation

8b2c3ef

island: use constants for permissions mode in test_file_utils.py

6b4a090

island: Return file descriptor when creating secure file

14371f3

island: Fix comment and statement formatting in file_utils.py

b648452

island: Extract code to _get_null_value_for_win32() in file_utils.py

37889d0

tests: Fix secure file creation tests as per latest changes

22c3c5a

island: Add type hinting in file_utils.py

64ac1fe

island: Specify mode to open new secure file in, in encryptor.py

80bfd90

shreyamalviya requested a review from VakarisZ June 15, 2021 14:02

mssalvatore added 2 commits June 15, 2021 13:12

island: Rename create_secure_file tests

44bdfa5

create_secure_file() was previously renamed to get_file_descriptor_for_new_secure_file().

mssalvatore approved these changes Jun 15, 2021

View reviewed changes

mssalvatore merged commit 78e9b8c into develop Jun 15, 2021

mssalvatore deleted the secure-mongo-key-file branch June 15, 2021 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create mongo key file securely #1232

Create mongo key file securely #1232

shreyamalviya commented Jun 14, 2021

mssalvatore Jun 14, 2021

mssalvatore Jun 14, 2021

VakarisZ Jun 15, 2021 •

edited

Loading

mssalvatore Jun 15, 2021

mssalvatore Jun 14, 2021

mssalvatore Jun 14, 2021

shreyamalviya Jun 15, 2021

VakarisZ Jun 15, 2021

mssalvatore Jun 15, 2021 •

edited

Loading

ghost commented Jun 15, 2021 •

edited by ghost

Loading

VakarisZ Jun 15, 2021

shreyamalviya Jun 15, 2021

VakarisZ Jun 15, 2021

shreyamalviya Jun 15, 2021

VakarisZ Jun 15, 2021

VakarisZ Jun 15, 2021

VakarisZ Jun 15, 2021

shreyamalviya Jun 15, 2021

VakarisZ Jun 15, 2021

VakarisZ Jun 15, 2021

shreyamalviya Jun 15, 2021

VakarisZ left a comment

mssalvatore commented Jun 15, 2021

Create mongo key file securely #1232

Create mongo key file securely #1232

Conversation

shreyamalviya commented Jun 14, 2021

What does this PR do?

PR Checklist

Testing Checklist

Explain Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VakarisZ Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssalvatore Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

ghost commented Jun 15, 2021 • edited by ghost Loading

DeepCode's analysis on #44bdfa found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VakarisZ left a comment

Choose a reason for hiding this comment

mssalvatore commented Jun 15, 2021

VakarisZ Jun 15, 2021 •

edited

Loading

mssalvatore Jun 15, 2021 •

edited

Loading

ghost commented Jun 15, 2021 •

edited by ghost

Loading