5
\$\begingroup\$

I am working on a server for a smart house. This part of code is responsible for communication with client (separate application), sending requested data and ensuring everything is safe.

I've decided to use multithreaded version of Python socketserver in order to avoid bottlenecks (I want it to be flexible enough to manage a something bigger than a flat). The server_forever() method is called from separate part of the program, not important for this workflow. The handle() method triggers actions specific to the type of order chosen by the user I've decided to use abstract factory for this task.

Everything afterwards is implemented separately for each order type. If the authentication fails the address is banned for a short time to prevent attacks (orders are sent by another application so any mistake can be expected to be caused by an attempt to gain unauthorized access).

Here are my questions:

  1. I needed lock and banned_addresses to be remembered but ConnectionHandler is created each time new connection is set up. Is it ok to store them in the ThreadedTCP class?
  2. I assume that i will need to lock sensitive data, which in this case is the list of banned addresses not to get a mess while removing ban. Is this mechanism implemented correctly? I haven't faced any problems while testing but I might have not noticed something.
  3. I want to extend it in future so that there would be multiple users with different privileges logging in. It would be done in __authorize_connection__() part after asking the user to send login and password and checking if they can perform a specific order. Do you have any ideas about any potential jeopardy?
  4. Any other ideas on how to improve security? What do you think about this generally?
  5. I am rather inexperienced when it comes to python programming so any suggestions on how to make this code more "pythonic" are welcome.

import socketserver
from datetime import datetime
from Networking import Errors, OrderFactory
import threading


def setup_connection_handler(host, port):
    return ThreadedTCP((host, port), ConnectionHandler)


class BannedAddresses:
    def __init__(self, address):
        self.address = address
        self.ban_timestamp = datetime.now()


class ThreadedTCP(socketserver.ThreadingMixIn, socketserver.TCPServer):
    banned_addresses = [BannedAddresses(None)]
    lock = threading.Lock()


class ConnectionHandler(socketserver.BaseRequestHandler):

    def handle(self):
        try:
            self.__authorize_connection__()
            self.__receive_data_from_network__()
            order_factory = OrderFactory.OrderFactory.create_factory(self.data)
            order = order_factory.create_order(self.request)
            order.perform_order()

        except Errors.UnknownOrderError:
            # If we receive wring order we can assume that unauthorized connection is attempted.
            # Therefore connection is being shut down.
            print('Wrong order! Shutting connection down.')
            self.request.close()
            self.__ban_client__()

        except Errors.AuthorizationError:
            self.request.close()

    def __receive_data_from_network__(self):
        self.data = self.request.recv(1024)
        self.data = self.data.decode('utf-8')

    # TODO: extend this, introduce password checking
    def __authorize_connection__(self):
        self.__unban_after_withdrawal_period__()
        if self.__is_banned__():
            raise Errors.AuthorizationError

    def __unban_after_withdrawal_period__(self):
        current_timestamp = datetime.now()
        ThreadedTCP.lock.acquire()
        to_delete = []
        for banned in ThreadedTCP.banned_addresses:
            elapsed = current_timestamp - banned.ban_timestamp
            if elapsed.days >= 1 or elapsed.seconds > 60:
                to_delete.append(banned)

        for td in to_delete:
            ThreadedTCP.banned_addresses.remove(td)
        ThreadedTCP.lock.release()

    def __is_banned__(self):
        banned_addresses = (l.address for l in ThreadedTCP.banned_addresses)
        if self.client_address[0] in banned_addresses:
            return True

    def __ban_client__(self):
        ThreadedTCP.banned_addresses.append(BannedAddresses(self.client_address[0]))
\$\endgroup\$
5
\$\begingroup\$

1. Review

  1. There are no docstrings. What do these functions and classes do? How do I use them?

  2. In Python, it's conventional to use a single underscore to indicate a method that's only intended for use internally to a class. Names starting and ending with two underscores should be reserved for the system. So __receive_data_from_network__ should be _receive_data_from_network and so on.

  3. Do you come from a Java background? In Java, all values have to be attributes of objects belonging to classes, so there is often some difficulty about where to put persistent data, leading to workarounds like the singleton pattern. But Python has global variables, so there is no difficulty here: just make banned_addresses and lock into global variables.

  4. You initialize banned_addresses with [BannedAddresses(None)], but the address None does not seem to serve any purpose. Better to initialize it to the empty list.

  5. The problem with calling lock.acquire and lock.release is that if an exception occurs between these calls, then the lock will not be released. Instead, use the lock as a context manager to ensure that it gets released on all paths through the code:

     with lock:
         # ... code using banned_addresses ...
    
  6. The banned_addresses data structure stores the time that the address was added to the cache. This means that it's necessary to do a subtraction when checking to see if it has expired. It would be simpler to store the expiry time instead. Then checking for expiry can be a simple comparison.

  7. The banned_addresses data structure needs to support the following operations: (i) look up an address to see if it is banned; (ii) add a new address; (iii) expire old addresses. Lists do not support efficient implementations of (i) — in order to determine if an item is in a list, it's necessary to iterate over the whole list.

    When writing a network server, it's important to pay attention to data structure efficiency, otherwise an attacker can exploit program inefficiency to cause denial of service.

2. Improved cache

A better data structure for representing the set of banned addresses would be a dictionary mapping an address to its expiry time. I would encapsulate this in a class that also performed the necessary locking:

from datetime import datetime, timedelta

class TimedCache:
    """A thread-safe cache whose items expire after a certain interval."""
    def __init__(self, interval):
        self._lock = threading.Lock()
        self._cache = {}
        self._interval = interval

    def add(self, key):
        with self._lock:
            self._cache[key] = datetime.now() + self._interval

    def remove(self, key):
        with self._lock:
            del self._cache[key]

    def __contains__(self, key):
        with self._lock:
            try:
                expiry = self._cache[key]
            except KeyError:
                return False
            if expiry <= datetime.now():
                del self._cache[key]
                return False
            else:
                return True

    def flush(self):
        """Flush expired keys from the cache."""
        with self._lock:
            now = datetime.now()
            expired = [key for key, e in self._cache.items() if e <= now]
            for key in expired:
                del self._cache[key]

banned_addresses = TimedCache(timedelta(seconds=60))

Notes:

  1. This class doesn't know anything about what kinds of keys it is storing. This separation of concerns makes it easier to understand, test, and reuse.

  2. The locking takes place within the methods — this reduces the risk that the caller will get it wrong.

  3. The class checks the expiry time for an address when looking it up: this avoids having to flush the cache every time you are about to look up an address. (You still need to flush the cache occasionally to prevent it growing without limit, but that can be done at your convenience.)

  4. This much simplifies the rest of the code, for example the __authorize_connection__ and __ban_client__ methods become:

    def _authorize_connection(self):
        if self.client_address[0] not in banned_addresses:
            raise Errors.AuthorizationError
    
    def _ban_client(self):
        banned_addresses.add(self.client_address[0])
    

    and the __unban_after_withdrawal_period__ and __is_banned__ methods can be dropped altogether.

3. Security

It's hard to comment on security without knowing the threat environment. But these concerns stand out to me:

  1. If you're going to authenticate with a password, then the connection needs to be encrypted, otherwise an eavesdropper can read the password off the network.

  2. Authenticating a connection doesn't help unless all communication over that connection is also authenticated. Otherwise an attacker can wait until a connection is established and then insert their own commands into the stream of packets.

  3. A client is banned for 60 seconds if it issues an incorrect command. It's not clear what threat this is intended to mitigate. A malicious client will surely arrange to send correct commands to avoid being banned. It's probably better to authenticate the client, and then just close the connection on an error, but not bother with banning the client as it doesn't gain anything.

So at a minimum, you should use the facilities in the ssl module to encrypt and authenticate the communication over the socket.

\$\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.