Feature/secure island db #86

itaymmguardicore · 2018-02-13T14:35:20Z

Feature

DB now stores its credentials encrypted.
All credentials are encrypted in DB. Preventing simple non-targeted malware from stealing credentials from the DB.

Have you added an explanation of what your changes do and why you'd like to include them?
Have you successfully tested your changes locally?

danielguardicore · 2018-02-15T10:12:33Z

What happens if the configuration is not encrypted? If ConfigService should_encrypt is false, we should make sure not to encrypt when not needed, and more importantly, not decrypt encrypted data.

danielguardicore

I'm mostly worried about any changes requiring very careful attention to the actual API to avoid double encryption/decryption of data.

danielguardicore · 2018-03-06T14:57:41Z

monkey_island/cc/services/config.py

+        ConfigService._encrypt_config(config, False)
+
+    @staticmethod
+    def _encrypt_config(config, is_decrypt=False):


Is this really the best name for this function? Just to avoid code duplication

What would you suggest?

I think two functions is better. I just think this will end up confusing

maybe rename it do _encrypt_or_decrypt_config?
Longer name but I think it solves the confusion, and is only used from withing encrypt_config and decrypt_config

danielguardicore · 2018-03-06T15:09:50Z

monkey_island/cc/services/config.py

@@ -794,29 +797,42 @@
    }
 }

+ENCRYPTED_CONFIG_ARRAYS = \


This is duplicated here (with schema information) and in add_system_info_creds_to_config
Can we avoid this somehow?

I'm not sure I know what you're talking about

There are two seperate places in the code where what fields need to be encrypted are listed.

These 2 places are (sort of) unrelated. It's true that currently the credentials are the only config values being encrypted. Maybe in the future we'll have further secrets we'd want to encrypt but not use in add_system_info_creds_to_config.

Yeah, the problem is that keeping track of whats encrypted requires a double lookup. If we can't avoid it oh well

Can't avoid it right now

danielguardicore · 2018-03-06T15:14:14Z

monkey_island/cc/services/config.py

@@ -17,60 +20,60 @@
            "type": "string",


Can we move this entire schema to a seperate file? Out of 970 lines, 785 are the schema, making the file hard to read.

Yeah. But let's not do it in this branch. I've added as task to tasklist

danielguardicore · 2018-03-06T15:19:09Z

monkey_island/cc/resources/telemetry.py

@@ -124,6 +125,8 @@ def process_exploit_telemetry(telemetry_json):
        if new_exploit['result']:
            EdgeService.set_edge_exploited(edge)

+        Telemetry.encrypt_exploit_creds(telemetry_json)


At this point, what happened to the plain text creds in the telemetry_json which were written to the mongodb under new_exploit?

If you're asking what happened up to this point, nothing happened. They were sent plain text from the monkey (over HTTPS).

Meant the line where mongo.db.edge.update
With something that's a deep clone of telemetry_json['data']
which is what's being encrypted later.

We're encrypting the data when we add the creds but it seems the data is also saved without encryption under edges.

danielguardicore · 2018-03-06T15:25:55Z

monkey_island/cc/services/config.py

    def __init__(self):
        pass

    @staticmethod
-    def get_config(is_initial_config=False):
+    def get_config(is_initial_config=False, should_decrypt=True):


Please document when get_config and get_config_value should each be used

danielguardicore · 2018-03-06T15:29:57Z

monkey_island/cc/services/config.py

-    def get_config_value(config_key_as_arr, is_initial_config=False):
-        config_key = reduce(lambda x, y: x+'.'+y, config_key_as_arr)
+    def get_config_value(config_key_as_arr, is_initial_config=False, should_decrypt=True):
+        config_key = reduce(lambda x, y: x + '.' + y, config_key_as_arr)


I prefer you use functools.reduce

danielguardicore · 2018-03-06T15:36:11Z

monkey_island/cc/services/config.py

        return config

    @staticmethod
-    def get_config_value(config_key_as_arr, is_initial_config=False):
-        config_key = reduce(lambda x, y: x+'.'+y, config_key_as_arr)
+    def get_config_value(config_key_as_arr, is_initial_config=False, should_decrypt=True):


You're inconsistent on when are you explicit/implict on should_decrypt.
get_config, 2 implicit true, 1 explicit false
get_config_value - all explicitly true
get_flat_config - 1 implicit true

Why? I'm not sure it really matters except I'd rather be explicit where possible, so we know when are we passing plain credentials and we can verify it's always to the monkey.

danielguardicore · 2018-03-06T15:43:31Z

monkey_island/cc/services/config.py

+            ConfigService.default_config = config
+
+    @staticmethod
+    def get_default_config(should_decrypt=True):


Fun with naming
should_decrypt leads to encrypt_config, this does not make sense.

danielguardicore · 2018-03-08T11:07:28Z

monkey_island/requirements.txt

+enum34
+PyCrypto


Add a new line at end.

itaymmguardicore added 3 commits February 13, 2018 12:05

Add encryptor

0ed2f74

Add global encryptor

29e8510

encrypt credentials in config+telemetry

06a2e4f

itaymmguardicore requested a review from danielguardicore February 13, 2018 14:35

itaymmguardicore self-assigned this Feb 19, 2018

itaymmguardicore added island Feature Issue that describes a new feature to be implemented. labels Feb 19, 2018

Merge branch 'develop' into feature/secure-island-db

d8210bf

danielguardicore suggested changes Mar 6, 2018

View reviewed changes

danielguardicore reviewed Mar 8, 2018

View reviewed changes

itaymmguardicore added 2 commits March 8, 2018 14:17

Fix CR

795885b

fix CR

cf86294

itaymmguardicore merged commit 1a3ca06 into develop Mar 8, 2018

itaymmguardicore deleted the feature/secure-island-db branch March 8, 2018 18:03

snyk-bot mentioned this pull request Jul 18, 2020

[Snyk] Upgrade react-tooltip-lite from 1.11.2 to 1.12.0 #730

Merged

		enum34
		PyCrypto

Feature/secure island db #86

Feature/secure island db #86

Uh oh!

Conversation

itaymmguardicore commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Feature

Uh oh!

danielguardicore commented Feb 15, 2018

Uh oh!

danielguardicore left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

itaymmguardicore commented Feb 13, 2018 •

edited

Loading