2
\$\begingroup\$

I wrote a simple python port scanner today and I would like to get some advice on how to improve on the code in different ways.

I am aware that I can implement threading to reduce runtime but I won't for now as it feels a bit advanced at this time. Rather I would like tips/opinions on how to improve the program in other ways.

You can find the code on my GitHub.

I am aware that the logging is a bit redundant as it doesn't log anything now, I simply forgot to remove it.

# Network port scanner
# Focus first will be on making the functionality of the software.
# Second focus will be on lowering the runtime of the software.


import socket
import logging
import time


class SConnect:

    def __init__(self, ip, port=None):
        self.ip = ip
        self.port = port
        self.address = (self.ip, self.port)
        self.s_connection = socket.socket(socket.AF_INET,       socket.SOCK_STREAM)
        self.s_connection.settimeout(0.3)

    def portscan(self):

        return self.s_connection.connect_ex(self.address)


def main():

    logging.basicConfig(filename="errlog.log", format="%(asctime)s : %(message)s")
    logging.info("Start")
    print("\nHello user and welcome to Network Port Scanner!")
    print("Please insert a IP address that you want to scan for open and     closed ports.")
    print("The range of ports scanned is 1-65535.")
    u_ip = input("\nTarget IP: ")

    open_pcounter = 0
    closed_pcounter = 0

    if u_ip is not None:
        for p in range(1, 65536):
            start_ptime = time.time()
            c = SConnect(u_ip, p)
            if c.portscan() == 0:
                print("Port {} is open".format(p))
                open_pcounter += 1
            else:
                print("Port {} is closed".format(p))
                closed_pcounter += 1
            print("--- %s seconds ---" % (time.time() - start_ptime))
    else:
        print("You failed, terminating.\n")

    print("Total open ports:%s".format(open_pcounter))
    print("Total closed ports:%s".format(closed_pcounter))
    logging.info("Finished")


if __name__ == '__main__':
    start_time = time.time()
    main()
    print("--- %s seconds ---" % (time.time() - start_time))
\$\endgroup\$
2
\$\begingroup\$

Connections

You don't close your sockets and connections. The best way to ensure you close them is with a context manager (with-statement)

You can also reuse the socket you make, and connect it to other ports. This would make a SocketConnection:

class SocketConnection:
    def init(self, ip, port=None):
        self.socket = None
        self.ip = ip    

    def __enter__(self):
        self.socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)

    def __exit__(self, type, value, traceback):
        self.socket.close()
        self.socket = None

    def portscan(self, port):
        if self.socket is None:
            raise ConnectionError('No socket open')
        try:
            return not self.socket.connect_ex((self.ip, port))
        finally:
            self.socket.detach()

This can be used like this:

def scan_ports(ip, port_range):
    """yields the open ports in `port_range` (port_range is half-open) """
    with SocketConnection(ip) as connection:
        for port in range(*ports):
            if connection.portscan(port):
                yield port

Validate input

You don't validate the input of your client. To do this, you can do something like this:

def validate_ip(ip):
    return True # needs implementation
def get_ip():
    while True:
        ip = input("Target IP: ")
        if validate_ip(ip):
            return ip

You can do ctrl+C to get out of this while True loop

and then the main() function:

def main():
    logging.basicConfig(filename="errlog.log", format="%(asctime)s : %(message)s")
    logging.info("Start")
    message = """
    Hello user and welcome to Network Port Scanner!
    Please insert a IP address that you want to scan for open and closed ports.
    The range of ports scanned is 1-65535.
    """
    print(message)
    ip = get_ip()
    open_ports = list(scan_ports(ip, (1, 65536)))

    print(
        f"""open ports: ({len(open_ports)})
        {open_ports}""")

Caveat: I don't know a lot about socket programming. I just used the python documentation, so there might be socket-related bugs

\$\endgroup\$
  • \$\begingroup\$ I like this __enter__ and __exit__! \$\endgroup\$ – Ludisposed Sep 12 '18 at 9:00
  • \$\begingroup\$ The reason that I did not use "with" statements is because after some googling I found that it increases the run time because it raises exceptions. Using "connect_ex" did not raise any exceptions and such is faster, however I should close the socket after using it tho. And you are right I really should validate the input. Thank you! \$\endgroup\$ – Jonas Landhage Sep 12 '18 at 11:11
  • 1
    \$\begingroup\$ using the with statement to close the socket has little to do with whether you use connect or connect_ex. From what I read, you should close the sockets, if only not to warn the other side of the connection that the socket gets closed, and that with-statement is the easiest way to make sure it is closed \$\endgroup\$ – Maarten Fabré Sep 12 '18 at 11:44
1
\$\begingroup\$

I would move the first conditional in main into a guard statement and lower the flow depth.

if u_ip is None:
     print("You failed, terminating.\n")
     return

Also, if instead of incrementing a counter if you had a dictionary of port to open/closed mapping, you could have the option of doing more with it at the end of the run, such as outputting to a file, or returning it to another function as it grows. This would also work with an async model as the dictionary is a hash and won't store a key more than once.

Other wise it looks solid. Nice work.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you, especially for the last comment :) Regarding flow depth I totally agree. When it comes to dict I understand what you mean but for now I won't use the data in anyway, but if I in the future will improve upon the program and then use the data in some way I will definitely take this into consideration. I could definitely see how a dict would be more useful tho. Once again, thank you !:) \$\endgroup\$ – Jonas Landhage Sep 12 '18 at 8:09
  • \$\begingroup\$ Happy to help. Actually instead of a dict, I just realized, a set of open ports would work, then you could infer the closed ones. You could also do that with the increment. Just subtract open count from 65536. Anyway what you have is fine and I'm nitpicking for nitpicking's sake. Good luck! \$\endgroup\$ – mutant_city Sep 12 '18 at 8:15
1
\$\begingroup\$

Review

  • Check validity of input

    You should validate the ip if the connection is reachable, else it will error.

  • This only checks for TCP ports not UDP

    You could add a (socket.AF_INET, socket.SOCK_DGRAM) connection for checking UDP ports

  • Don't use %s but {} for string formatting

  • Close the socket after usage

  • Use argparse or sys.argv for parsing user input arguments instead of input

    This makes it easier to rerun scripts

  • You could add a range of ports to the port scanner instead of creating a SConnect for every port

  • Use generators

Alternative code

import sys
import socket

class PortScanner:
    def __init__(self, ip, ports):
        self.ip = ip
        self.ports = ports

    def scan_tcp_port(self, port):
        connection = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        connection.settimeout(0.3)
        yield connection.connect_ex((self.ip, port)), port
        connection.close()

    def scan_ports(self):
        for port in self.ports:
            yield from self.scan_tcp_port(port)

    def host_up(self):
        try:
            connection = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
            connection.connect_ex((self.ip, 80))
            return True
        except socket.timeout:
            return True
        except socket.error:
            return False


def main(ip, ports=range(1, 65536)):
    scanner = PortScanner(ip, ports)
    if not scanner.host_up():
        print("Host is down")
        return

    for connection, port in scanner.scan_ports():
        connection = "UP" if connection == 0 else "DOWN"
        print(f"Port {port} is {connection}") # Log result

if __name__ == '__main__':
    if len(sys.argv) == 2:
        ip = sys.argv[1]
        main(ip)
\$\endgroup\$
  • \$\begingroup\$ If the connection.connect_ex raises an exception, or the rest of the pogram raises an exception while scan_tcp_port waits for a return of the control back, the connection will not get closed \$\endgroup\$ – Maarten Fabré Sep 12 '18 at 8:57
  • \$\begingroup\$ You brought up some interesting points and I appreciate it being in a dotted list making it easier to read. What is the difference in using "%s" and "{}"? Furthermore I only used "%s" because thats how the information I found (about logging for an example) was coded. I will take this list of points and add it as a to-do list, thank you!:D \$\endgroup\$ – Jonas Landhage Sep 12 '18 at 11:13
  • \$\begingroup\$ The logging is correct, but you do a "%s".format() which seems weird cuz it combines old formatting with new formatting. Instead replace that with {} or f"{string} \$\endgroup\$ – Ludisposed Sep 12 '18 at 11:18
1
\$\begingroup\$

One weakness of this approach is the sequential nature - by trying ports consecutively, we have to wait for a connect() to fail before beginning the next one. This can take a while, particularly for hosts with "blackhole" firewall rules (that drop incoming packets rather than replying with a connection-refused status).

To open TCP connections asynchronously, you'll want to read the Creating connections section of the asyncio documentation. Don't expect to be able to begin the opening of all ports at once, because most OSes have a limit on the number of open socket descriptors - start with a hundred or so, then throttle so that you then only open a new one when you get a result back and close a candidate.

Also, suitably defensive hosts will firewall you out when they observe a sequential port scan. Consider shuffling the port number range, or at least XOR with a random (per run) salt, to give a less predictable sequence of ports to try.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.