New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/detect cross segment traffic #120

Merged
merged 19 commits into from Sep 3, 2018

Conversation

Projects
None yet
2 participants
@itaymmguardicore
Contributor

itaymmguardicore commented Apr 30, 2018

Feature / Fixes

Adds ability to test for cross-segment traffic

  • 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?

Fixes #93

@danielguardicore

Will keep going over this, but would prefer a commit documenting the more complex functions, what they're going to do, less the how.

At least, _get_inaccessible_subnets_ips, get_cross_segment_issues_per_subnet_pair, get_cross_segment_issues_per_subnet_group and maybe get_cross_segment_ip

LOG.info("Base local networks to scan are: %r", self._ranges)
def _get_inaccessible_subnets_ips(self):
subnets_to_scan = []

This comment has been minimized.

@danielguardicore

danielguardicore May 5, 2018

Contributor

I'm going to need some documentation on what's going on here.

This comment has been minimized.

@itaymmguardicore
"minItems": 2,
"uniqueItems": True,
"description": "List of IPs/subnets."
" Examples: \"192.168.0.1\", \"192.168.0.5-192.168.0.20\","

This comment has been minimized.

@danielguardicore

danielguardicore May 5, 2018

Contributor

What you're providing is single items, not groups, not sure what the example is trying to say.

This comment has been minimized.

@itaymmguardicore

itaymmguardicore May 23, 2018

Contributor

This is how it looks like from the island. Is this unclear? What would you change?

image

This comment has been minimized.

@danielguardicore

danielguardicore May 23, 2018

Contributor

No. I, as a user, do not understand how this is different from the IPs to attack display.

We need to explain/show that each box needs needs to have a set of IP ranges. For example,
192.168.0.5-192.168.0.20, 201.40.0.5-201.40.1.20, 30.5.1.2/24
And explain it means all 3 shouldn't be accessible to each other.
Or am I myself not understanding the UI :)

This comment has been minimized.

@itaymmguardicore

itaymmguardicore May 23, 2018

Contributor

Fixed (until we rephrase)

@staticmethod
def get_issues():
issues = ReportService.get_exploits() + ReportService.get_tunnels() + ReportService.get_cross_segment_issues() + ReportService.get_azure_issues()
issues = ReportService.get_exploits() + ReportService.get_tunnels() \

This comment has been minimized.

@danielguardicore

danielguardicore May 5, 2018

Contributor

This is starting to get long, can we move this to yet another array and iterate?

This comment has been minimized.

@itaymmguardicore

itaymmguardicore May 23, 2018

Contributor

I'm not sure I understand what you're suggesting

This comment has been minimized.

@danielguardicore

danielguardicore May 23, 2018

Contributor

We're going to have more and more, do we want to keep adding functions or have some collection of analysis functions?

for subnet_str in subnet_group:
if NetworkScanner._is_any_ip_in_subnet([unicode(x) for x in self._ip_addresses], subnet_str):
# If machine has IPs from 2 different subnets in the same group, there's no point checking the other
# subnet.

This comment has been minimized.

@danielguardicore

danielguardicore May 5, 2018

Contributor

More conceptual, but if we have a machine in both subnets as a possible pinpoint, don't we want to alert on that?

This comment has been minimized.

@itaymmguardicore

itaymmguardicore May 23, 2018

Contributor

Interesting point. I fear we'll get a lot of false positives (in cases we'll have cross segmentation issues). Also unsure how strong this is. Feels awkward to add a section of machines which may be a gateway between the subnets. What do you think?

This comment has been minimized.

@itaymmguardicore

itaymmguardicore added some commits May 23, 2018

Make feature simpler
Change config value phrasing
Merge branch 'develop' into feature/detect-cross-segment-traffic
# Conflicts:
#	infection_monkey/config.py
#	infection_monkey/example.conf
#	monkey_island/cc/services/report.py
if NetworkScanner._is_any_ip_in_subnet([unicode(x) for x in self._ip_addresses], subnet_str):
# If machine has IPs from 2 different subnets in the same group, there's no point checking the other
# subnet.
for other_subnet_str in WormConfiguration.inaccessible_subnets:

This comment has been minimized.

@danielguardicore

danielguardicore Aug 30, 2018

Contributor

Inside _is_any_ip_in_subnet we generate new range objects for every other_subnet_str. In a machine with say, 3 IP addresses and 3 possible subnets to check, we have a very expensive lookup.
At minimum, can we cache the range objects by pregenerating them?

@@ -6,6 +6,11 @@
import logging
BASE_PATH = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
PARENT_PATH = os.path.dirname(BASE_PATH)

This comment has been minimized.

@danielguardicore

danielguardicore Aug 30, 2018

Contributor

What is this doing in this PR?
Is this related to the common folder?

This comment has been minimized.

@itaymmguardicore

itaymmguardicore Sep 2, 2018

Contributor

Yeah. I think this commit was before the common branch was even created 😅
I can remove this, fix other CR comments, backmerge from develop and then from support common.
Then I'll merge after support common is merged.
You have any other suggestion?

"""
if source_subnet == target_subnet:
return []
source_subnet_range = NetworkRange.get_range_obj(source_subnet)

This comment has been minimized.

@danielguardicore

danielguardicore Sep 1, 2018

Contributor

Similar comment to before, we're going to "reiterate" these objects again and again, is it possible to pregenerate them?

@itaymmguardicore itaymmguardicore merged commit 5ce902f into develop Sep 3, 2018

@itaymmguardicore itaymmguardicore deleted the feature/detect-cross-segment-traffic branch Sep 3, 2018

@itaymmguardicore itaymmguardicore self-assigned this Sep 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment