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

Passive fingerprinting #16

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

Conversation

but-i-am-dominator
Copy link

Smudge V1.0

Copy link

@Zalgo2462 Zalgo2462 left a comment

Choose a reason for hiding this comment

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

When I saw this PR drop, I just had to take a look. I'm so excited to see this project developing!

I haven't historically been too involved with this project, so take everything I've said here with a heaping pinch of of salt. I more or less used this PR as an opportunity to learn about the codebase. Nonetheless, I think I've found a few comments/ names that ought to be updated.

PS: I really enjoyed reading the Smudge code. What a nice, clean approach!

Cheers,
LL

print(dframe)


def wraper_function(dframe, options):

Choose a reason for hiding this comment

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

Typo in wraper

Choose a reason for hiding this comment

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

Consider filter_by_options

@@ -0,0 +1,133 @@
import pandas as pd
from numpy import sum

Choose a reason for hiding this comment

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

Import appears to be unused. Pandas .sum should call out to numpy internally.

warnings = (df['description'].str.startswith('Warning')).sum()
suspicious = (df.State.values == 'suspicious').sum()
n = len(pd.unique(df['IPAddress']))
print(len(df), "records,", n, "distinct addresses,", op, "open ports", suspicious, "suspicious entries,", warnings,

Choose a reason for hiding this comment

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

Consider either returning these stats along with the dataframe or moving this analysis out to a separate function.

Choose a reason for hiding this comment

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

It looks like this logic is repeated in show()

reader = csv.reader(aih, delimiter='\t')
#Format: range_start range_end AS_number country_code AS_description
for first_ip, last_ip, as_num, country, as_description in reader:
if sys.version_info < (3, 0):

Choose a reason for hiding this comment

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

Consider using six to implement python version compatibility.
https://stackoverflow.com/a/29213992

pass
#elif as_num == '0' and as_description != 'Not routed':
# sys.stderr.write('as == 0, desc != not routed\n')
#elif as_num != '0' and as_description == 'Not routed':

Choose a reason for hiding this comment

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

Please either delete these comments or add some text comments explaining their usefulness.

except UnicodeDecodeError:
raw_addr_string = ''

#if Devel:

Choose a reason for hiding this comment

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

Debug comments.


sys.path.insert(0, '.') #Allows us to load from the current directory (There was one claim that we need to create an empty file __init__.py , but this does not appear to be required.)

Choose a reason for hiding this comment

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

Consider bundling ip2asn.py as a module as you have done with smudge. This would allow you to import the file without this work around. Modules can be run with python -m ./path/to/module. Alternatively, move ip2asn.py into a folder ip2asn, and make an empty __init__.py file in the folder. Then, you can import the file as ip2asn.ip2asn and still run the file from the cli.

pytz
scapy>=2.4.0
scapy>=2.4.0

Choose a reason for hiding this comment

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

Might consider adding pandas here or building out a separate requirements file for the analysis script.

parser.add_argument('-p', '--per_packet_timeout', help='Time to wait for a reply for a single packet, can be fractional (default: ' + str(per_packet_timeout_default) + ' ).', required=False, default=per_packet_timeout_default)
parser.add_argument('-f', '--forced_interface', help='Force packets through this interface (needed on macos, default: ' + str(forced_interface_default) + ' ).', required=False, default=forced_interface_default)
parser.add_argument('-t', '--ttl', help='Maximum number of hops to try (default: ' + str(ttl_default) + ')', required=False, default=ttl_default)
parser.add_argument('-c', '--cache_dir', help='Directory tree to hold cached traceroutes (default: ' + str(traceroute_cache_dir_default) + ' ). Use None to not cache results.', required=False, default=traceroute_cache_dir_default)

Choose a reason for hiding this comment

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

I don't believe that the string None passed on the cli will be properly cast to None in python. Instead, I believe it will come in as the string "None".

I haven't tested this, so please dismiss this if this is not an issue.

"""

import json
import urllib.request

Choose a reason for hiding this comment

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

Consider moving these imports to the top of the file and accessing the libraries as normal rather than through the class.

@william-stearns
Copy link
Collaborator

This code has been merged into the passer-ng branch. Once you're set with Zalgo's comments, dominator, please consider retracting this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants