1
\$\begingroup\$

I am creating a login for an encrypted chat application which retrieves login information from a MySQL database. I have got to the point where I feel pretty confident that (to the best of my knowledge) it is relatively secure. I am trying to learn so feel free to criticize!

import hashlib
import mysql.connector
from tkinter import *
from tkinter import messagebox
from cryptography.fernet import Fernet

chat = Tk() #Api I am using to create the GUI for the application

#Connect to MySQL database
try:
    loginFRetrieve = open("LK.bin", "rb") #Retrieving Encryption key from file
    retrivedKey =  loginFRetrieve.read()
    loginFRetrieve.close()

    loginFRetrieve = open("LC.bin", "rb") #Retrieving MySQL server login credentials
    retrivedLC = loginFRetrieve.read()
    loginFRetrieve.close()

    cipher = Fernet(retrivedKey)
    retrivedLC = cipher.decrypt(retrivedLC) #Decrypting server login data from file
    retrivedLC = retrivedLC.decode('utf-8')
    lC = retrivedLC.split()

    mydb = mysql.connector.connect(host=lC[0],user=lC[1],passwd=lC[2],database=lC[3])
    del(lC)
except mysql.connector.Error as err:
    chat.withdraw()
    messagebox.showerror("Database Error", "Failed to connect to database")
    exit()



mycursor = mydb.cursor()

#hashPass hashes and returns a string of characters using SHA-256 algorithm
def hashPass(hP):
    shaSignature = \
        hashlib.sha256(hP.encode()).hexdigest()
    return shaSignature

#userExists checks a database too see if username exists in the database
def userExists(userName):
    mycursor.execute("SELECT username FROM logins WHERE username = '%s'" % userName)
    userResult = mycursor.fetchall()
    if userResult:
        return True
    return False

#Creates a new user in the connected SQL database.
def newUser(nU, nP):
    if userExists(nU) == False:
        mycursor.execute("SELECT username FROM logins WHERE username = '%s'" % nU)
        mycursor.fetchall()
        r = hashPass(nP)
        sql = "INSERT INTO logins(username, passwordhash) VALUES(%s,%s)"
        val = (nU, r)
        mycursor.execute(sql, val)
        mydb.commit()
        chat.title(string="User created")
    else:
        messagebox.showwarning("User Creation Error", "User already exists")

#Checks the connected SQL database for an existing user.
def existingUser(uN, pW):
    if userN.get() != "":
        if userExists(uN) == True:
            encryptedPass = hashPass(pW)
            mycursor.execute("SELECT * FROM logins")
            passResult = mycursor.fetchall()
            for row in passResult:
                if row[1] == uN and row[2] == encryptedPass:
                    chat.title(string="Login Successful!")
                elif row[1] == uN and row[2] != encryptedPass:
                    messagebox.showerror("Login Error", "Password does not match our records")
        else:
            messagebox.showerror("Login Error", "User does not exist")
    else:
        messagebox.showwarning("Login Error", "Please enter a username")
\$\endgroup\$
  • \$\begingroup\$ Does this code work? What's userN? \$\endgroup\$ – 200_success Feb 19 at 7:24
  • \$\begingroup\$ @200_success I left a lot of the code for the GUI elements out. The userN is the text from the user name text box of the GUI. \$\endgroup\$ – Mr.clean Feb 19 at 13:15
2
\$\begingroup\$

Encryption isn't Hashing

encryptedPass = hashPass(pW)

You're not encrypting the password, you're hashing it.

For passwords you should not be hashing them with the SHA2 family. Instead, use bcrypt.

Sanitize input

From my limited knowledge of Python, it doesn't appear you're sanitizing your input on some functions, for example userExists() and the first query in newUser(). Instead, you're using simple string formatting to substitute values directly.

You should be passing the variables as arguments to execute() every time.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the input! What's your thoughts on the way I am retrieving the key from the bin file and decrypting the server login credentials? Is this a secure way to store connection strings? \$\endgroup\$ – Mr.clean Feb 19 at 1:44
  • \$\begingroup\$ @Mr.clean I am not very familiar with Python. It looks okay, but security is very nuanced and I can't say for sure. Perhaps, if variables persist beyond the try-catch scope, you could del other variables used in it. \$\endgroup\$ – esote Feb 19 at 1:59
  • \$\begingroup\$ Please note that while bcrypt is better than plain SHA256 hashing (as is done now), Argon2 would be a more modern choice. \$\endgroup\$ – SEJPM Feb 19 at 10:18
  • \$\begingroup\$ @SEJPM what’s your thoughts on my code from a security perspective? Also what makes sha256 insecure for password hashing? \$\endgroup\$ – Mr.clean Feb 19 at 13:12
  • \$\begingroup\$ @Mr.clean this answer should explain password-hashing. It's a bit dated, so doesn't talk about Argon2 yet, but is otherwise really good. The only other interesting thing I could spot was the code putting the credential's key unencrypted in a file right next to the ciphertext. \$\endgroup\$ – SEJPM Feb 19 at 14:53

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.