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

Add masscan support #92

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

svkirillov
Copy link
Contributor

Add initial masscan support

@manmolecular
Copy link
Collaborator

Thanks! Great job.
I will make a review soon and let you know.

@manmolecular manmolecular self-requested a review March 30, 2020 14:12
@manmolecular manmolecular added the enhancement New feature or request label Mar 30, 2020
@svkirillov svkirillov changed the title Add masscan support WIP: Add masscan support Mar 30, 2020
Copy link
Collaborator

@manmolecular manmolecular left a comment

Choose a reason for hiding this comment

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

Sorry, the previous note was out-of-scope of this PR. I removed this message and will fix it later, separately from these changes. Thanks for attention.

Copy link
Collaborator

@manmolecular manmolecular left a comment

Choose a reason for hiding this comment

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

Some changes are required. At the moment, the module does not save data and crashes at the end of the scan. Please, pay attention to the comments and tracebacks that I attached.

Python environment:

Python 3.7.7 (default, Mar 10 2020, 15:43:33)
[Clang 11.0.0 (clang-1100.0.33.17)] on darwin

System:

MacOS Catalina 10.15.4

grinder/errors.py Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/defaultvalues.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@manmolecular manmolecular left a comment

Choose a reason for hiding this comment

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

Some changes are required. We need more accurate testing.

  1. What happens if the network connection disappears during scanning because of overload?
  2. What happens if the network connection disappears between ranges? For example, we have 5 ranges for scanning, and internet connection disappears after second one? What happens with the last 3 ranges?
  3. What happens in case if we have a VPN connector enabled?
  4. What happens if masscan returns 0 results?

grinder/masscanconnector.py Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/defaultvalues.py Outdated Show resolved Hide resolved
@manmolecular
Copy link
Collaborator

Also, please update the Dockerfile with masscan package.

@manmolecular
Copy link
Collaborator

???

@manmolecular manmolecular marked this pull request as draft April 23, 2020 16:43
@manmolecular manmolecular added bug Something isn't working refactoring Something needs to be changed tests Core tests and removed tests Core tests labels May 26, 2020
@manmolecular
Copy link
Collaborator

Any news? @svkirillov

@svkirillov svkirillov marked this pull request as ready for review June 14, 2020 19:05
@svkirillov svkirillov changed the title WIP: Add masscan support Add masscan support Jun 18, 2020
@svkirillov
Copy link
Contributor Author

@manmolecular need review

grinder/core.py Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/errors.py Show resolved Hide resolved
@manmolecular
Copy link
Collaborator

Sorry, that is not all review! Github just finishes my review too early for some reason, idk why. I will continue looking and testing it.

Copy link
Collaborator

@manmolecular manmolecular left a comment

Choose a reason for hiding this comment

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

Bugs.

grinder/core.py Show resolved Hide resolved
grinder/top_ports.py Outdated Show resolved Hide resolved
grinder/top_ports.py Outdated Show resolved Hide resolved
grinder/top_ports.py Outdated Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/core.py Show resolved Hide resolved
Copy link
Collaborator

@manmolecular manmolecular left a comment

Choose a reason for hiding this comment

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

Little fixes required.

grinder/defaultvalues.py Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/core.py Outdated Show resolved Hide resolved
grinder/masscanconnector.py Show resolved Hide resolved
grinder/dbhandling.py Show resolved Hide resolved
@manmolecular manmolecular removed the bug Something isn't working label Jun 22, 2020
Copy link
Collaborator

@manmolecular manmolecular left a comment

Choose a reason for hiding this comment

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

Some bugs are still present.
Is it okay that we use the key port for multiple ports? It's better to turn:

{
...
"port": "80"
...
}

Into the:

{
...
"ports": ["80"]
...
}

Cause with the masscan module and based on all the features that you implement this transformation looks logical. And moreover, different logical parts like counters and so on now works strange.

BUT, this is it, and obviously we don't have time for this ⏰, so it seems that I need to fix it later somehow.
So, thanks for the job, but some changes are still required (just to support at least the basic logic).

:param host: ip of the host to scan
:param rate: packet rate argument for Masscan
:param arguments: arguments for Masscan
:param ports: ports to scan with Masscan
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you check the unique entities' counter? Run scan with the flag -cu to count unique entities, and you will see:

{
    "80": 18,
    "443": 10,
    "3389": 9,
    "22": 4,
    "111": 4,
    "25": 3,
    "21": 3,
    "139": 3,
    "53": 3,
    "443,22": 2,
    "80,443": 2,
    "80,111": 2,
    "110": 2,
    "1947,2040": 1,
    "23": 1,
    "443,1034,21,1088": 1,
    "443,53,139": 1,
    "25,443": 1,
    "636": 1,
    "3389,80": 1,
    "111,80": 1,
    "110,80": 1,
    "21,443": 1,
    "80,1935": 1,
    "3370,1947": 1,
    "3306,22": 1,
    "3306,873": 1,
    "465": 1,
    "111,443": 1,
    "80,139": 1,
    "80,143,995": 1,
    "2222,53,22": 1,
    "22,110,995,25,143": 1,
}

Something like "443,1034,21,1088" is not okay, we need to separate ports.

else:
self.masscan.scan(hosts=host, arguments="", sudo=sudo)

self.results = {host: self.masscan[host] for host in self.masscan.all_hosts}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you check the country.json file? Run scan with the flag -cu and you will see:

{
    "": 103
}

Because masscan scan results don't have any country-related data or latitude and longitude. We need to skip empty data from counting in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The same for the organization.json, proto.json

Copy link
Collaborator

Choose a reason for hiding this comment

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

map_feature_bug

And I don't even talk about this case. Did you check it?

By default, we put something like (0, 0) coordinates in case if the host doesn't have any coordinates, but, these cases are really rare (because Shodan/Censys usually provide us information about location). So, for 5-10 hosts in 1.000 it is okay to put (0, 0) coordinates, but if you have 1.000 hosts and all of them don't have coordinates it's not the good idea to put them on the map (imho).

@manmolecular manmolecular added the bug Something isn't working label Jun 24, 2020
@manmolecular
Copy link
Collaborator

How r u doin'? What about fixes? Will it take too long to fix it or maybe you need any help? Let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request refactoring Something needs to be changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants