Networked Chat App in Python

Question

I'm a highschool student with a passion for Python, and this is my first try at networking.

GitHub

server.py

import select
import socket
import sys
import time
from encrypt import AESCipher
server = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
server.setblocking(0)
server_addr = ('localhost',46643)
server.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
server.bind(server_addr)
passwd = input('You can now enter a password for your server, or press enter for none: ')
enc = AESCipher(passwd)
server.listen(12)
SOCKS = [server]
nicknames = {}
def chat_server():
 global SOCKS, nicknames, passwd
 while SOCKS:
 readable, writable, exception = select.select(SOCKS,[],[],0)
 for s in readable:
 if s == server:
 sock, addr = server.accept()
 SOCKS.append(sock)
 print('{} connected to server'.format(addr))
 nicknames[sock] = addr
 broadcast(s,'{} connected to server'.format(addr))
 else:
 try:
 data = s.recv(4096)
 except ConnectionResetError:
 broadcast(s,'{} killed the connection'.format(nicknames[s]))
 print('{} killed the connection'.format(nicknames[s]))
 if s in SOCKS:
 SOCKS.remove(s)
 s.close()
 continue
 try:
 plaintext = enc.decrypt(data)
 assert plaintext
 except:
 continue
 if data:
 if plaintext.startswith('/help'):
 s.send(enc.encrypt('/nick <nickname> - change nickname'))
 elif plaintext.startswith('/nick '):
 broadcast(s,'{} changed their nickname to {}'.format(nicknames[s],plaintext[6:]))
 nicknames[s] = plaintext[6:].strip()
 else:
 broadcast(s,'{}: {}'.format(nicknames[s],plaintext))
 else:
 broadcast(s,'{} killed the connection'.format(nicknames[s]))
 print('{} killed the connection\n'.format(nicknames[s]))
 if s in SOCKS:
 SOCKS.remove(s)
 s.close()
 for s in exception:
 broadcast(s,'{} killed the connection'.format(nicknames[s]))
 print('{} killed the connection'.format(nicknames[s]))
 if s in SOCKS:
 SOCKS.remove(s)
 s.close()
def broadcast(sock,message):
 global server, SOCKS
 for s in SOCKS:
 if s != server:
 try:
 s.send(enc.encrypt(message))
 except:
 s.close()
 if s in SOCKS:
 SOCKS.remove(s)
chat_server()

client.py

from kivy.app import App
from kivy.lang.builder import Builder
from kivy.uix.screenmanager import ScreenManager
from kivy.properties import ObjectProperty
from kivy.uix.widget import Widget
from kivy.uix.button import Button
from kivy.clock import Clock
import select
import socket
import sys
from encrypt import AESCipher
class Chat(Widget):
 tout = ObjectProperty(None)
 tin = ObjectProperty(None)
 send_btn = ObjectProperty(None)
 def __init__(self, *args, **kwargs):
 super(Chat, self).__init__(*args, **kwargs)
 self.client = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
 self.client.settimeout(2)
 self.enc = AESCipher('')
 self.connected = 0
 Clock.schedule_interval(self.chat_client, 0)
 def chat_client(self,e):
 self.tin.focus = True
 if self.connected:
 readable, writable, exception = select.select([self.client],[],[],0)
 if self.client in readable:
 data = self.client.recv(4096)
 if not data:
 self.tout.text = 'Server connection killed.'
 else:
 try:
 self.tout.text += '\n' + self.enc.decrypt(data)
 except:
 pass
 def send_msg(self):
 text = self.tin.text
 if text.startswith('#passwd '):
 self.enc = AESCipher(text[8:])
 elif text.startswith('#connect '):
 try:
 self.client.connect((text.split(' ')[1], int(text.split(' ')[2])))
 self.connected = 1
 except:
 self.tout.text += '\nConnection to {}:{} failed. If you\'ve already connected to a server, try restarting your client.'.format(text.split(' ')[1], text.split(' ')[2])
 self.connected = 0
 elif text == '#exit':
 sys.exit()
 elif self.connected:
 self.client.send(self.enc.encrypt(text))
 self.tin.text = ''
class Manager(ScreenManager):
 pass
class Main(App):
 def build(self):
 Builder.load_string('''
<Chat>:
 tout: tout
 tin: tin
 send_btn: send_btn
 TextInput:
 id: tout
 width: root.width
 height: root.height * 0.94
 y: root.height * 0.06
 text: "Use #connect <ip> <port> to connect to a server. Use the command `/help` to view a server's commands. Use #passwd <password> if the server you're connected to has encryption."
 TextInput:
 id: tin
 focus: True
 multiline: False
 width: root.width * 0.9
 height: root.height * 0.06
 on_text_validate: root.send_msg()
 Button:
 id: send_btn
 width: root.width * 0.1
 height: root.height * 0.06
 x: root.width * 0.9
 text: 'Send'
 on_press: root.send_msg()
<Manager>:
 Screen:
 name: 'chat'
 Chat:
 id: chat''')
 return Manager()
Main().run()

encrypt.py (the encryption snippet was taken from elsewhere - I wanted to focus on networking rather than learning how to use PyCrypto):

from hashlib import md5
from base64 import b64decode
from base64 import b64encode
from Crypto import Random
from Crypto.Cipher import AES
# Padding for the input string --not
# related to encryption itself.
BLOCK_SIZE = 16 # Bytes
pad = lambda s: s + (BLOCK_SIZE - len(s) % BLOCK_SIZE) * \
 chr(BLOCK_SIZE - len(s) % BLOCK_SIZE)
unpad = lambda s: s[:-ord(s[len(s) - 1:])]
class AESCipher:
 def __init__(self, key):
 self.key = md5(key.encode('utf8')).hexdigest()
 def encrypt(self, raw):
 raw = pad(raw)
 iv = Random.new().read(AES.block_size)
 cipher = AES.new(self.key.encode(), AES.MODE_CBC, iv)
 return b64encode(iv + cipher.encrypt(raw.encode()))
 def decrypt(self, enc):
 enc = b64decode(enc)
 iv = enc[:16]
 cipher = AES.new(self.key.encode(), AES.MODE_CBC, iv)
 return unpad(cipher.decrypt(enc[16:])).decode('utf8')

What I've aimed for is a secure chat server/client, where a number of people can connect. I'd like to focus on looking at:

1. How secure is this? Could it easily be wiretapped and am I making some stupid mistakes?

2. Is my code styled ok? Are my variable names properly representative of what they are etc.?

3. And most of all, is the server/client relationship fairly responsible and not ridiculous?

Answer 1

This is pretty clean code, good job. I'll be reviewing each file in order, and then I'll you some extra advice at the end.

server.py

General

• If a statement consists of multiple names, like in a tuple, where the names are elements, or in a function call, where the names are arguments, the names should be separated by a comma and a single space. So:

  server_addr = ('localhost',46643)
  

  Becomes:

  server_addr = ('localhost', 46643)
  

  And similarly:

  readable, writable, exception = select.select(SOCKS,[],[],0)
  

  Becomes:

  readable, writable, exception = select.select(SOCKS, [], [], 0)
  

• In Python, we typically don't abbreviate names (passwd should be password, SOCKS should be SOCKETS).

• Each function or class definition should be separated by two blank lines, except if it is inside a class:

  def foo():
   print("foo!")
  def bar():
   print("bar!")
  class Spam:
   def __init__(self):
   print("Spam!")
   def eggs(self):
   print("eggs!")
  

  Remember, you don't have to follow these rules.

• In many programming languages (including in Python), constants follow the UPPERCASE_WITH_UNDERSCORES naming convention. SOCK is initially just the server socket, however, we're appending new sockets to it later, so it isn't a constant.

General

• In production level code, using assert is barely ever a good idea. Typically, we should be more specific about what went wrong. In case we expected x to be a Foo instance and it is a Bar instance, we raise a TypeError. If we expect x to be a positive integer, but it is -5, we raise a ValueError, etc. This makes debugging a lot easier.

• a is b checks if a and b reference the exact same underlying object in memory, while a == b returns a.__eq__(b). In the case of the select.select call, using the former makes more sense:

  if s is server:
  

• data = s.recv(4096): 4096 is a magic number. We can fix this by declaring BUFFER_SIZE = 4096. There's a couple of reasons to do this:

  • It improves readability;

  • If we want to change the buffer size later, we don't have to go around replacing every 4096 with our new size, but we just change BUFFER_SIZE once. This may not seem like such a big problem right now, but it's a pain to have to do this in big libraries.

• Global state in a program is almost never a good thing, and there are most certainly solutions to it in this case. Perhaps we can change chat_server's signature to accept SOCKS, nicknames and password.

Security

• Using input to get a password from a user is potentially insecure, as it is echoed back to the screen while typing, which allows for shoulder surfing. Luckily, the standard library includes getpass which solves this problem perfectly:

  >>> from getpass import getpass
  >>> getpass("Enter your password: ")
  Enter your password:
  'spam'
  

client.py

Disclaimer: I don't know Kivy, so I can't tell if you're following Kivy best practices.

General

• It's probably best to be explicit about Chat.connected representing a boolean value, and switch between True / False.

Other concerns

• TCP is not a message-based protocol, which means that there's no guarantee that a send call on one machine will match a recv call on the other. This is very important, because it means naive implementations will break if lots of data is being sent simultaneously. There's no way to tell if data that we receive is a fragment of a previous message, the start of a new one, or a combination of both. To fix this, we have to implement our own 'message' protocol. There's three well-known ways to do this:

  • Messages are fixed-length. This works if we can split up each message into chunks of the same size, but we can't use this in a chat, because it'll interrupt the natural flow of text.

  • Messages are delimited. This is a possibility, but it's easy to mess up if you forget escaping user's messages.

  • Messages are prefixed by their size. This is, in my opinion, the way to go.

• Although PyCrypto is the de facto cryptography library for Python, I think it's time to switch to cryptography, because it's more modern, easier to work with, and harder to mess up with. It features Fernet, a high-level wrapper around AES implementing AES 128-CBC (as well as HMAC-SHA256).

• The snippet implementing AES is treating MD5 as a key derivation function, which it definitely is not! I strongly suggest using PBKDF2 instead. cryptography implementation here

• By sending the password in plaintext to the server, anyone listening in on the network can intercept it and decrypt all future messages, which is why it's essential to use an asymmetric encryption algorithm, like RSA. Unlike symmetric encryption, asymmetric encryption involves a (private_key, public_key) keypair. Let's say Alice creates a keypair. Bob wants to talk to Alice, but he can't have anyone listening in, so he asks Alice for her public key. Alice sends it to Bob and Bob encrypts his message with it. When Alice receives the encrypted message, she can decrypt it using her private key, which she keeps all to herself. The way in which the public key & private key are involved in encryption / decryption is really complicated, so I won't bore you with the details. (If you do want to know more, this is a nice demo.)

• user153161: Thank you so much! This is really helpful advice. Although I'm unsure about some of the concerns, I will research them further to gain a greater understanding. And, for the encryption bit, I don't think the password is ever sent to the server- just used to decrypt? I'm sorry if I'm incorrect in my understanding. There are some great videos on encryption over at khan academy, which I'll go look at to refresh my memory again. Thank you once again, I will keep all this in mind :)
• Daniel: You're welcome! And indeed, regarding the password being sent over the network, that was my mistake for mixing the tin, tout 'streams' =)

Answer 2

A few minor things:

• Indentation matter in Python, and PEP8 says to use indentation of 4 spaces. It will make your code more readable
• Remove unused modules, I dont think you use md5 anywhere, secondly md5 is not consided secure. Better to use a better hashing algorithm like Bcrypt
• You could use a if __name__ == '__main__' block
• I'm guessing this is run from the command line, you might want to consider using argparse instead of the input('An input:') you have.

Security

How secure is this? Could it easily be wiretapped?

• Yes, it could be easily tapped. You can assume any localhost connection can be tapped (With for instance a MiTM).
• The only way to negate this is to ensure the data is being encrypted before sending it.