Skip to content
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

Solves External IP issue for the monkey Island #510

Closed
wants to merge 20 commits into from

Conversation

shivank1234
Copy link
Contributor

What is this?

Fixes #467

Now , we can pass multiple External IP's of the Monkey Island Server to the monkey so that after breaching the host can connect back even if it is outside local LAN of the Monkey Island.

Checklist

  • 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?
  • Is the TravisCI build passing?

Proof that it works

If applicable, add screenshots or log transcripts of the feature working

Changes

Are the commit messages enough? If not, elaborate.

@@ -79,12 +79,24 @@ def initialize(self):
self._network = NetworkScanner()
self._dropper_path = sys.argv[0]

server_num=0
secondary_servers= len(self._args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not all command arguments are server ip's. Look above: there is -p for parrent argument, -t for tunnel argument and so on... I'm pretty sure this doesn't give you a number of servers passed with -s flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that.. Working on that ..
Thanks for your reviews .. :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not solve. Why is it called secondary_servers if it's argument list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected

Comment on lines 94 to 98
if self._args[server_num -1] not in WormConfiguration.command_servers:
LOG.debug("Added default server: %s" % self._args[server_num -1])
WormConfiguration.command_servers.insert(server_num, self._args[server_num-1])
else:
LOG.debug("Default server: %s is already in command servers list" % self._args[server_num-1])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. So you add server IPs to command servers list. Can you verify that monkey tries all IPs in that list when connecting to server?

Copy link
Contributor

@VakarisZ VakarisZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address my comments. Make sure to format code according to PEP8 (in pycharm alt+ctl+l). Lastly, make sure we can pass a list of servers trough configuration (UI)

@danielguardicore
Copy link
Contributor

@shivank1234 do you intend to fix this?

@shivank1234
Copy link
Contributor Author

Ya, Currently working on it..
Sorry ,A Noobie
It is taking more time to fix...

@ShayNehmad
Copy link
Contributor

Take your time @shivank1234. Feel free to reach out in Slack if you need advice 😇

@ShayNehmad ShayNehmad added Island Feature Issue that describes a new feature to be implemented. labels Jan 20, 2020
@@ -72,7 +72,7 @@ def initialize(self):
arg_parser = argparse.ArgumentParser()
arg_parser.add_argument('-p', '--parent')
arg_parser.add_argument('-t', '--tunnel')
arg_parser.add_argument('-s', '--server')
arg_parser.add_argument('-s', '--server',nargs = '+')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space after comma

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected

WormConfiguration.command_servers.insert(0, self._default_server)
else:
LOG.debug("Default server: %s is already in command servers list" % self._default_server)
if self._default_server[0] not in WormConfiguration.command_servers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable needs to be renamed to default_servers. Also, what does this if achieve?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't it break other fn which are using default_server as an attribute...?

LOG.debug("Default server: %s is already in command servers list" % self._default_server[0])
server_num=server_num+1

while server_num<(secondary_servers):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing spaces around <

server_num=server_num+1

while server_num<(secondary_servers):
if self._default_server[server_num ] not in WormConfiguration.command_servers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is different from if on line 93?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected ,Thx

@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #510 into develop will decrease coverage by 0.04%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #510      +/-   ##
===========================================
- Coverage    57.60%   57.56%   -0.05%     
===========================================
  Files          116      116              
  Lines         3892     3895       +3     
===========================================
  Hits          2242     2242              
- Misses        1650     1653       +3     
Impacted Files Coverage Δ
monkey/infection_monkey/config.py 63.63% <20.00%> (-1.59%) ⬇️
monkey/infection_monkey/model/host.py 45.71% <100.00%> (-0.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3ec436...322036f. Read the comment docs.

@@ -72,26 +72,31 @@ def initialize(self):
arg_parser = argparse.ArgumentParser()
arg_parser.add_argument('-p', '--parent')
arg_parser.add_argument('-t', '--tunnel')
arg_parser.add_argument('-s', '--server')
arg_parser.add_argument('-s', '--server', nargs = '+')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change --server to --servers. Same in dropper.py

@@ -9,7 +9,7 @@ def __init__(self, ip_addr, domain_name=''):
self.services = {}
self.monkey_exe = None
self.default_tunnel = None
self.default_server = None
self.server_list = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We renamed this field because we are going to set it to a list of servers instead of a single server. My two questions to you (which are related) is what VictimHost class has to do with our change and did you also rename the usages of this property?

@@ -86,12 +86,17 @@ def initialize(self):
self._network = NetworkScanner()
self._dropper_path = sys.argv[0]

server_num=0
server_count = len(self._default_server)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't make sense from readability perspective. If _default_server is in fact multiple servers we should rename it to _default_servers

Copy link
Contributor

@VakarisZ VakarisZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job but still not finished. You still need to edit build_monkey_commandline() and dropper.py


if self._default_servers:
while server_num < (server_count):
if self._default_servers[server_num ] not in WormConfiguration.command_servers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't WormConfiguration.command_servers empty/contains default values at this point? Why don't you use list comprehension then?

Copy link
Contributor

@VakarisZ VakarisZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, but we still could do some minor improvements

Comment on lines 89 to 90
def_server_list = [ def_server for def_server in WormConfiguration.command_servers ]
WormConfiguration.command_servers = [ server for server in self._default_servers ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use list comprehension instead of simple assignment. what's the difference between def_server_list = [ def_server for def_server in WormConfiguration.command_servers ] and def_server_list = WormConfiguration.command_servers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sry, Had a silly doubt,Corrected

@shivank1234
Copy link
Contributor Author

PR is merge ready and can be merged at maintainer's discretion.

@VakarisZ
Copy link
Contributor

We need to do same changes in our bootloader before merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue that describes a new feature to be implemented.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to add island's external IP
5 participants