HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Python 3 NetScanner Daemon

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
netscannerpythondaemon

Problem

I’m pretty new to programming and python in particular. This is a simple ICMP/TCP scanner that dumps output to a terminal and/or csv file. My goal was to make a daemon that could do simple scanning without requiring any special imports. It is my first attempt at writing something that would be useful and that I might even cannibalize for other projects. I just finished the TCP scanning portion of it and have done some basic vetting.

Really what I’m looking for specific feedback on is:

Performance and Efficiency (I know these terms are ambiguous in code, so I’ll elaborate)

  • Do you see anything that is effectively a memory leak?



  • Do you see anything that can be done with less code and or is


repeated too many times?

  • Do you see anything that is clearly written badly and needs


attention (arg structure, looping, etc)

  • Do you see anything that will become less efficient as it loops? I


haven’t run my script for long durations yet, as most of my time has
been spent making sure it functions

Process

  • Is my code written well?



  • Should I rearrange anything?



  • Am I using the right loops for the right jobs?



  • Are my TCP sockets written well enough?



  • I borrowed the ICMP code, is there a better way to do that?



  • Do you find any of it particularly good/bad/dangerous?



Format

  • How does it look?



  • I know the RFC for python formatting, but haven’t read fully through


it yet. Is there anything that makes your eye twitch?

  • Does what I’ve written look like usable code to you when you look at


it? (I know this is objective and kind of random, as well as not
100% related to function, but I really do want this feedback from
someone who looks at code a lot, if you’re willing to give it).

  • What’s your first reaction when looking at this script?



Other than that I’ll take any feedback you have on it.

```
#!/usr/bin/env python3

'''
#==================================================================================#
# AUTHOR: Chris Gleason

Solution

Ok, so I'll try to comment on this code as much as I can, but due to the fact that you said:


Requires Python 3 and OSX to run.

Note that this could also be the reason some users don't review your code (I kinda like running the code that I edit, so that I can be sure it really brings some improvements)

I won't be able to run / test it. I'll start from the top to the bottom.

Formatting:

Imports

  • don't import modules if you're not using them (e.g: import threading )



Try to also put them in an alphabetically order:

import argparse
import csv
import ipaddress
import os
import random
import select
import smtplib
import socket
import subprocess
import sys
import time
from datetime import datetime


Punctuation

-in Python, we don't put a space before a comma:

parser.add_argument('-t', '--tcp' ,
        action='store_true' ,
        help='Use TCP SYN/ACK scanning for discovery')


should become:

parser.add_argument('-t', '--tcp',
                    action='store_true',
                    help='Use TCP SYN/ACK scanning for discovery')


Please also note the new alignment that I've made. This way, your code become more readable.

Comments

I personally don't like:

###########################################
# NON FUNCTION/CLASS SCRIPT RELATED STUFF #
###########################################


Because it's distracting me + you shouldn't make obvious comments. In this case, it's obviously what you're doing. However, if you want to be sure that every person who'll step into this code will understand it, just:

# Non function / class related stuff


More,


triple double-quoted strings should be used for docstrings (but that's debatable - just told you so you'll know).

This means that:

def output_title(title):

    '''
    Function to auto-generate output headers and titles

    output=string
    '''


might become:

def output_title(title):
    # Function to auto-generate output headers and titles(output=string)
    '''


Spacing

Between methods, you should have two blank lines, not one:

def output_title(title):
    ....

def get_tout(a):
    ....


I kinda' see a perlish thing when you:

payload = random.randrange(0, 65536).to_bytes(2, 'big') + b'\x01\x00'
packet  = b'\x08\x00' + b'\x00\x00' + payload
packet  = b'\x08\x00' + chk(packet) + payload


You have multiple spaces before operator, and that's not ok according to PEP8. So, this will become:

payload = random.randrange(0, 65536).to_bytes(2, 'big') + b'\x01\x00'
packet = b'\x08\x00' + b'\x00\x00' + payload
packet = b'\x08\x00' + chk(packet) + payload


Parentheses

You can remove redundant parentheses:

if not (args.tcp):
    ....


should be:

if not args.tcp:
    ....


To sum up all that I said so far, your code would look like this (if you want to respect PEP8):

```
import argparse
import csv
import ipaddress
import os
import random
import select
import smtplib
import socket
import subprocess
import sys
import time
from datetime import datetime

# Non function / class related stuff

if os.geteuid() != 0:
exit('''

This program creates and uses raw sockets which require root\n\
priviledges to run. Please run it as root in order to use it.

''')

if sys.platform != 'darwin':
print("This script was designed to run on OSX. Currently that is the only platform it will work on.")
exit(0)

parser = argparse.ArgumentParser(description='\
Network scanning daemon to check for node state changes via TCP/UDP/ICMP. \
Default (no arguments) will run in the foreground using ICMP and broadcast\
domain for discovery and will store state data in memory. Default (no arguments)\
uses true ICMP, so it\'s not usually routed. If you "ping scan" with NMAP that rides\
over TCP unless you specifically tell it to use the ICMP protocol, so if you are\
trying to scan a remote subnet, use the --tcp flag.')

parser.add_argument('-t', '--tcp',
action='store_true',
help='Use TCP SYN/ACK scanning for discovery')
parser.add_argument('-q', '--quiet',
action='store_true',
help='Use to demonize netscanner for background processing - NOT IMPLEMENTED YET')
parser.add_argument('-i', '--infile',
action='store_true',
help='Use an existing CSV file instead of scanning the network for initial discovery')
parser.add_argument('-o', '--outfile',
action='store_true',
help='Export stat data to a CSV file')
parser.add_argument('-c', '--cidr',
action='store_true',
help='Use a CIDR block to generate scan range instead of using the broadcast domain')
parser.add_argument('-H', '--host',
action='store_true',
help='Monitor the state of a single host')
parser.add_argument('-e', '--email',
action='store_true',

Code Snippets

import argparse
import csv
import ipaddress
import os
import random
import select
import smtplib
import socket
import subprocess
import sys
import time
from datetime import datetime
parser.add_argument('-t', '--tcp' ,
        action='store_true' ,
        help='Use TCP SYN/ACK scanning for discovery')
parser.add_argument('-t', '--tcp',
                    action='store_true',
                    help='Use TCP SYN/ACK scanning for discovery')
###########################################
# NON FUNCTION/CLASS SCRIPT RELATED STUFF #
###########################################
# Non function / class related stuff

Context

StackExchange Code Review Q#128090, answer score: 4

Revisions (0)

No revisions yet.