7
\$\begingroup\$

I have created a Python 3 password manager for API keys and social media passwords on local storage. The script is 500 lines and PEP8/PYLINT compliant, each definition contains a docstring and the code is well commented. It is purposefully in "procedural" instead of "object" style.

  • The cryptography definitions all begin with crypto
  • The main loop definitions begin with wallet
  • The menu option definitions begin with option

The only non-standard python library is here.

I am seeking review on the overall security of my cryptography implementation, as well as, any insight to back doors or other threats.

The end goal is you enter site and user such as:

input site:  stackexchange 
input user:  litepresence

Then your site password is transferred to the clipboard for 10 seconds for authentication to the site. Security is key, I want to be able to use and distribute this app for financial applications.

I am maintaining the code here.

enter image description here

Key Features:

"""
writes site login to clipboard w/ xclip; auto deletes in 10 seconds
reads/writes AES CBC encrypted password JSON to text file along with current salt
new salt generated after every successful login, password change, return to main menu, and exit
salt is 16 byte and generated in crypto secure manner via os.urandom
master password stretched to several hundred megabytes to prevent GPU attacks
master password hashed iteratively via traditional salted pbkdf sha512
master password rehashed iteratively via non traditional method (for fun learning experience mostly)
includes password suggestion utility
"""

References:

"""
https://stackoverflow.com/questions/43860227/python-getting-and-setting-clipboard-data-with-subprocesses
https://www.quickprogrammingtips.com/python/aes-256-encryption-and-decryption-in-python.html
https://datalocker.com/what-is-the-difference-between-ecb-mode-versus-cbc-mode-aes-encryption
https://www.dlitz.net/software/pycrypto/api/current/Crypto.Cipher.blockalgo-module.html
https://docs.oracle.com/cd/E11223_01/doc.910/e11197/app_special_char.htm
https://docs.python.org/3/library/hashlib.html
https://crackstation.net/hashing-security.htm
https://en.wikipedia.org/wiki/PBKDF2
https://www.owasp.org
"""

Code:

# STANDARD PYTHON MODULES
import os
import sys
import time
import struct
import traceback
from hashlib import sha512
from hashlib import blake2b as blake
from hashlib import sha3_512 as sha3
from hashlib import pbkdf2_hmac as pbkdf
from hashlib import shake_256 as shake256
from base64 import b64encode, b64decode
from json import loads as json_loads
from json import dumps as json_dumps
from subprocess import Popen, PIPE
from random import seed, randint
from binascii import hexlify
from getpass import getpass
from pprint import pprint

# THIRD PARTY MODULES
from Crypto import Random
from Crypto.Cipher import AES


# USER DEFINED SECURITY CONSTANTS
# WARNING: when you change these parameters your CypherVault will need to be deleted
MEGABYTES = 400
ITERATIONS = 1000000

# CURRENT RELEASE ID
VERSION = 0.00000003


def it(style, text):
    """
    colored text in terminal
    """
    emphasis = {
        "red": 91,
        "green": 92,
        "yellow": 93,
        "blue": 94,
        "purple": 95,
        "cyan": 96,
    }
    return ("\033[%sm" % emphasis[style]) + str(text) + "\033[0m"


def trace():
    """
    Stack trace report upon exception
    """
    return "\n\n" + str(time.ctime()) + "\n\n" + str(traceback.format_exc()) + "\n\n"


def doc_write(document, text):
    """
    write a dictionary to file
    """
    with open(document, "w+") as handle:
        handle.write(text)
        handle.close()


def doc_read(document):
    """
    read dictionary from file
    """
    with open(document, "r") as handle:
        text = handle.read()
        handle.close()
        return text


def clip_get():
    """
    read from clipboard
    """
    clip = Popen(["xclip", "-selection", "clipboard", "-o"], stdout=PIPE)
    clip.wait()
    return clip.stdout.read().decode()


def clip_set(data):
    """
    write to clipboard
    """
    clip = Popen(["xclip", "-selection", "clipboard"], stdin=PIPE)
    clip.stdin.write(data.encode())
    clip.stdin.close()
    clip.wait()


def crypto_pad(msg):
    """
    pad if length is not a multiple of 128 else unpad as required
    """
    return (
        (msg + (128 - len(msg) % 128) * chr(128 - len(msg) % 128))
        if len(msg) % 128
        else msg[0 : -msg[-1]]
    )


def crypto_100():
    """
    cryptographically secure 100 digit string formatted integer generator
    """
    str_random = ""
    while len(str_random) != 100:
        set_random = struct.unpack("QQQQQQ", os.urandom(48))
        str_random = ""
        for integer in set_random:
            str_random += str(integer)
        str_random = str(int(str_random[-100:]))
    return str_random


def crypto_wacky_digest(msg_digest, salt):
    """
    never roll your own... except as a backup plan and fun learning experience!
    random amount of multiple hashing types to impose novelty restraint
    """
    shaken_salt = shake256(salt.encode()).digest(16)
    # randomized iteration count
    for _ in range(int(salt[-4:])):
        # for each up to 10 iterations; 1 in 10 no skip
        for _ in range(int(salt[-1])):
            # salted 512 chacha stream cipher with permuted input block copy
            msg_digest = hexlify(blake(msg_digest, salt=shaken_salt).digest())
        for _ in range(int(salt[-2])):
            # keccak 512 sponge construction
            msg_digest = sha3(msg_digest).digest()
        for _ in range(int(salt[-3])):
            # standard sha512
            msg_digest = sha512(msg_digest).digest()

    return msg_digest


def crypto_digest(password):
    """
    iterative rounds of hashing to impose time restraint
    expanded password length to impose memory restraint
    """
    # any scheme which uses more than a few hundred MB of RAM
    # is almost certainly inefficient for GPU or FPGA implementations
    password *= max(1, int(MEGABYTES * 10 ** 6 / len(password)))
    # shake and digest the salt to 16 bytes
    salt = doc_read("cyphervault.txt").split("$", 1)[0]
    shaken_salt = shake256(salt.encode()).digest(16)
    # many iterations of salted 512 password based key derivation function (PBKDF)
    msg_digest = hexlify(pbkdf("sha512", password.encode(), shaken_salt, ITERATIONS))
    # multiple types of hashing to impose novelty restraint
    msg_digest = crypto_wacky_digest(msg_digest, salt)
    # final format to sha256 for 32 byte output
    return shake256(msg_digest).digest(32)


def crypto_cypher(vector, password):
    """
    AES encryption method in CBC mode
    """
    return AES.new(crypto_digest(password), AES.MODE_CBC, vector, segment_size=256)


def crypto_encrypt(message, password):
    """
    encryption routine
    """
    vector = Random.new().read(AES.block_size)
    cypher = crypto_cypher(vector, password)
    recursion = cypher.encrypt(crypto_pad(message))
    return b64encode(vector + recursion)


def crypto_decrypt(cyphertext, password):
    """
    decryption routine
    """
    vector = b64decode(cyphertext)
    cypher = crypto_cypher(vector[:16], password)
    recursion = cypher.decrypt(vector[16:])
    return crypto_pad(recursion).decode()


def crypto_indite(passwords):
    """
    \nencrypting the CypherVault...
    """
    print(it("purple", crypto_indite.__doc__))
    cyphersalt = crypto_100()
    cyphervault = doc_read("cyphervault.txt").split("$", 1)[1]
    doc_write(
        "cyphervault.txt", cyphersalt + "$" + cyphervault,
    )
    cyphervault = crypto_encrypt(
        json_dumps(passwords), passwords["master"]["master"]
    ).decode()
    doc_write(
        "cyphervault.txt", cyphersalt + "$" + cyphervault,
    )


def wallet_main(master=None, passwords=None):
    """
    ******************************************
    *** Welcome to CypherVault v"""
    msg = "***\n    ******************************************\n"
    print("\033c", it("green", wallet_main.__doc__ + ("%.8f " % VERSION) + msg))
    if passwords is None:
        passwords = wallet_initialize(master)
    crypto_indite(passwords)
    choice = wallet_choices(passwords["master"]["master"], passwords)
    if choice == 0:
        option_get(passwords)
    elif choice == 1:
        option_post(passwords)
    elif choice == 2:
        option_delete(passwords)
    elif choice == 3:
        option_suggest(passwords)
    elif choice == 4:
        option_print(passwords)
    elif choice == 5:
        option_print_full(passwords)
    elif choice == 6:
        crypto_indite(passwords)
        sys.exit()


def wallet_initialize(master):
    """
    initialize password dictionary and prompt for master password
    """
    # read password dictionary if none exists create a new encrypted cyphervault
    # all passwords are in format >>> passwords[site][user]
    try:
        cyphervault = doc_read("cyphervault.txt").split("$", 1)[1]
        assert len(cyphervault) > 0
    except Exception:
        print("\nCypherVault not found, intitializing new...")
        doc_write("cyphervault.txt", (crypto_100() + "$n"))
        passwords = {"master": {"master": "password"}}
        crypto_indite(passwords)
        cyphervault = doc_read("cyphervault.txt").split("$", 1)[1]
        print(it("purple", "\nyour default password is has been set to:"))
        print(it("green", "\npassword\n"))
    if master is None:
        master = getpass("Enter your master password:  ")
    # attempt to decrypt the cyphervault with the supplied password
    decrypted = False
    try:
        passwords = json_loads(crypto_decrypt(cyphervault, master))
        decrypted = True
    except Exception:
        trace()
        print(it("green", "\ninvalid master password, press Enter to try again..."))
        input("\npress Enter to return to main menu")
        wallet_main()  # recursion
    if decrypted:
        # after every successful login create a new salt
        crypto_indite(passwords)
        print(it("green", "\n    login successful!"))
        # warn if password is default
        if master == "password":
            print(it("purple", "\nyou should change default password immediately!"))
            print("\nyour master password is: ", it("green", master))
        # perform some tests on the password
        audit(master)

        return passwords


def wallet_choices(master, passwords):
    """
    1: ENTER A NEW PASSWORD OR EDIT A PASSWORD
    2: DELETE A PASSWORD
    3: SUGGEST A PASSWORD
    4: PRINT SITE/USER LIST
    5: PRINT SITE/USER/PASSWORD LIST
    6: EXIT
    """
    print(it("green", wallet_choices.__doc__))
    choice = input("input choice or press Enter to GET A PASSWORD: ")
    if not choice:
        choice = 0
    try:
        choice = int(choice)
        assert 0 <= choice <= 6
    except Exception:
        print("\033cinvalid choice (", choice, ") try again")
        time.sleep(2)
        wallet_main(master, passwords)
    return choice


def option_get(passwords):
    """
    the password has been copied to the clipboard
    \nyou only have 10 seconds to paste it via ctrl+V
    """
    site, user = input_site_user()
    found = False
    if site in passwords.keys():
        if user in passwords[site].keys():
            found = True
            clip_set(passwords[site][user])
            print(it("purple", option_get.__doc__))
            time.sleep(10)
            clip_set("")
            print("clipboard has been cleared")
            time.sleep(2)
    if not found:
        print("\nsite/user not found in wallet")
        response = input("\nwould you like to add this site/user? (y/n):  ")
        if response in ["", "y", "Y"]:
            option_post(passwords, site, user)
    input("\npress Enter to return to main menu")
    wallet_main(passwords["master"]["master"], passwords)


def option_post(passwords, site=None, user=None):
    """
    post a new or updated password to the cyphervault
    """

    def update(passwords, site, user):

        print(it("purple", "\nsite/user:"), site, user, "\n")
        # double Enter the new password
        new_pass = getpass("\ninput password: ")
        if new_pass == getpass("\ninput new password again: "):
            audit(new_pass)
            if site not in passwords.keys():
                passwords[site] = {}
            passwords[site][user] = new_pass
            crypto_indite(passwords)
            print(it("green", "\nCypherVault has been updated"))
        else:  # recursion
            print(it("purple", "\npasswords do not match, try again..."))
            time.sleep(2)
            update(passwords, site, user)

    if site is None:
        site, user = input_site_user()
    create_new = True
    # update a password if it already exists
    if site in passwords.keys():
        if user in passwords[site].keys():
            print("\nsite:", site, "\nuser:", user)
            print(it("purple", "\nWARN: site/user already exists"))
            response = input("\nwould you like to overwrite this site/user? (y/n):")
            if response not in ["", "y", "Y"]:
                create_new = False
    if create_new:
        update(passwords, site, user)
    # return to main menu
    input("\npress Enter to return to main menu")
    wallet_main(passwords["master"]["master"], passwords)


def option_delete(passwords):
    """
    remove a user from the cyphervault
    """
    print("\nEnter the site and user you would like to delete")
    site, user = input_site_user()
    if site != "master":
        found = False
        # check if the site/user exists in the passwords
        if site in passwords.keys():
            if user in passwords[site].keys():
                found = True
        if found:
            # remove the user
            del passwords[site][user]
            if passwords[site] == {}:
                # if there are no other users at that site, remove the site as well
                del passwords[site]
                crypto_indite(passwords)
                print(it("purple", "\nsite/user has been deleted"))
                time.sleep(2)
        else:
            # return to the main menu
            print(it("purple", "\nsite/user was not found"))

    else:
        print(it("purple", "you cannot delete the master password!"))
    input("\npress Enter to return to main menu")
    wallet_main(passwords["master"]["master"], passwords)


def option_print(passwords):
    """
    print all site/user combinations in the cyphervault
    """
    print("")
    for site, logins in passwords.items():
        for user, _ in logins.items():
            print(it("green", site + " : " + user))
    input("\npress Enter to return to main menu")
    wallet_main(passwords["master"]["master"], passwords)


def option_print_full(passwords):
    """
    \n\n
    WARNING: YOU ARE ABOUT TO EXPOSE YOUR UNENCRYPTED CYPHERVAULT
    Make sure you are in a private location!\n
    WARNING: DO NOT PRINT THIS TO PAPER
    CypherVault contents will be exposed on your printer hard drive\n
    WARNING: DO NOT SAVE THIS TO FILE
    CypherVault contents will be exposed on your local hard drive\n
    USE THIS FUNCTION ONLY TO INSPECT OR COPY - BY HAND - TO PAPER\n\n
    """
    print("\033c", it("purple", option_print_full.__doc__))
    msg = "\nare you sure you want to print your unencrypted CyperVault (y/n):  "
    response = input(it("green", msg))
    if response in ["y", "Y"]:
        msg = "\npress Enter to expose the CypherVault\npress Enter again to exit\n"
        print(it("green", msg))
        input()
        print("\033c\n\n\n")
        pprint(passwords)
    input(it("green", "\npress Enter to return to main menu"))
    wallet_main(passwords["master"]["master"], passwords)


def option_suggest(passwords, length=10):
    """
    press Enter to suggest another secure random password
    or any number 10 to 500, then Enter to change the length
    or any other key, then Enter to return to main menu\n
    """
    response = ""
    while not response:
        chars = "0123456789"
        chars += "abcdefghijklmnopqrstuvwxyz"
        chars += "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
        chars += "?%^*+~-=[]{}:,.#_"  # Oracle approved symbols
        legit = False
        while not legit:
            seed(int(crypto_100()))
            password = ""
            for _ in range(length):
                password += str(chars[randint(0, len(chars) - 1)])
            legit = audit(password, display=False)
        print("\033c\n   ", it("green", password), "\n")
        response = input(option_suggest.__doc__)
        print("")
        if response.isdigit():
            response = int(response)
            if response < 10:
                input("minimum suggested length is 10, press Enter to continue")
                length = 10
            if response >500:
                input("maximum supported length is 500, press Enter to continue")
                length = 500
            else:
                length = response
            response = ""
        elif response:
            wallet_main(passwords["master"]["master"], passwords)


def input_site_user():
    """
    routine to input site and user name
    """
    site = input("\nEnter site name:  ")
    if site == "master":
        user = "master"
    else:
        user = input("\nEnter user name:  ")
    print("")
    return site, user


def audit(password, display=True):
    """
    \nyour password is weak, you should change it!
    \naim for length greater or equal to 10
    \nand 2 each unique uppers, lowers, digits, symbols\n
    """
    uppers = []
    lowers = []
    digits = []
    others = []
    for item in [c for c in password]:
        if item.isupper():
            uppers.append(item)
        elif item.islower():
            lowers.append(item)
        elif item.isdigit():
            digits.append(item)
        else:
            others.append(item)
    length = len(password)
    uppers = len(list(set(uppers)))
    lowers = len(list(set(lowers)))
    digits = len(list(set(digits)))
    others = len(list(set(others)))
    review = {
        "length": length,
        "unique uppers": uppers,
        "unique lowers": lowers,
        "unique digits": digits,
        "unique symbols": others,
    }
    legit = True
    if not ((length >= 10) and (min(uppers, lowers, digits, others) >= 2)):
        legit = False
        if display:
            print(it("purple", audit.__doc__), it("green", "audit:"), review)
    return legit


if __name__ == "__main__":

    wallet_main()
\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the question after it has been answered, especially the code. See codereview.stackexchange.com/help/someone-answers. \$\endgroup\$ – pacmaninbw Dec 28 '19 at 15:06
  • \$\begingroup\$ I made no changes to the original code; I get the moving target concept. I "added" an update. \$\endgroup\$ – litepresence Dec 28 '19 at 15:08
  • \$\begingroup\$ Yes, which is why I didn't roll the question back to the previous version and up voted the question. \$\endgroup\$ – pacmaninbw Dec 28 '19 at 15:10
  • \$\begingroup\$ can we agree it is ok if I continue to "add updates" on a definition by definition basis as long as I do not "revise" anything in the original? I'm want to keep this conversation as interactive as possible without losing history. \$\endgroup\$ – litepresence Dec 28 '19 at 15:14
  • 1
    \$\begingroup\$ @litepresence No. We can't, since every reviewer has to see the same version of the code or the thread won't make sense. Consider asking a follow-up question linking back to this one instead. Feel free to add a link to this question to your next one. All that is ok. What's not ok, is updating the code. That creates a mess. \$\endgroup\$ – Mast Dec 29 '19 at 17:38
5
\$\begingroup\$

I know very little about cryptography, so I'm not even going to attempt to comment on that aspect of it.

I do see the odd thing that can be improved though.


if choice == 0:
    option_get(passwords)

elif choice == 1:
    option_post(passwords)
. . .

This has a lot of redundancy. The common pattern is you're dispatching on choice, then passing passwords to the given function (with the unfortunate exception of exit). This can be made more streamline either by using a list, or dictionary. Since you're using numbers 0 to 6 as "keys", this is arguably more appropriate as a list, but a dictionary would be more explicit:

MENU_DISPATCH = \
    {0: option_get,
     1: option_post
     2: option_delete,
     . . .
     6: lambda _: sys.exit}

or

MENU_DISPATCH = \
    [option_get,
     option_post,
     option_delete,
     . . .,
     lambda _: sys.exit]

Then, in either case:

f = MENU_DISPATCH[choice]
f(passwords)

The dictionary has the benefit that you can easily do error checking:

f = MENU_DISPATCH.get(choice, None)  # None on a bad option

if f:
    f(passwords)

else:
    print("Bad option")

I find it odd that wallet_main and wallet_choices are mutually recursive. Not only does this open you up to RecursionErrors (if somehow they enter bad input ~1000 times), it makes the code harder to follow because execution is jumping back and forth between the functions. Do you actually need the if passwords is None: and the rest of that code to run every time they make an error?

I'd make wallet_choices handle input completely. I'd also introduce a parse_int function so you don't need to wrap calls to int with a try, and get rid of the catching of the AssertionError. I think catching an assert error like you are here is an abuse of assert.

from typing import Optional


def parse_int(text_num: str) -> Optional[int]:
    """Returns the parsed number, or None if the parse failed."""
    try:
        return int(text_num)

    except ValueError:
        return None

def wallet_choices(master, passwords):
    """
    1: ENTER A NEW PASSWORD OR EDIT A PASSWORD
    2: DELETE A PASSWORD
    3: SUGGEST A PASSWORD
    4: PRINT SITE/USER LIST
    5: PRINT SITE/USER/PASSWORD LIST
    6: EXIT
    """
    print(it("green", wallet_choices.__doc__))

    while True:  # Imperative loop instead of recursion
        choice = input("input choice or press Enter to GET A PASSWORD: ")
        choice = choice or "0"  # That check can be reduced down by taking advantage of `or`

        choice = parse_int(choice)

        if choice is not None and 0 <= choice <= 6:  # Just use if instead of assert to jump to error handling
            return choice

        else:
            print("\033cinvalid choice (", choice, ") try again")
            time.sleep(2)
            # Will loop back to the top from here

Just a heads up, you can use Colorama instead of your it function.


In doc_write, you have:

with open(document, "w+") as handle:
    handle.write(text)
    handle.close()

This defeats the purpose of using with though. handle is automatically closed when with is exited and the TextIOWrapper's __exit__ method is called. You just need:

with open(document, "w+") as handle:
    handle.write(text)

And then the same with doc_read.


In trace, you have:

"\n\n" + str(time.ctime()) + "\n\n" + str(traceback.format_exc()) + "\n\n"

This can be neatened up using f-strings:

f"\n\n{time.ctime()}\n\n{traceback.format_exc()}\n\n"

Or maybe using join if you want to reduce the duplicated double-newlines:

"\n\n".join([time.ctime(), traceback.format_exc()])

And then similarly in it:

"\033[%sm" % emphasis[style]) + str(text) + "\033[0m"

This can be:

f"\033[{emphasis[style]}m{text}\033[0m"

f-strings are often preferable over format or %.


Instead of:

for user, _ in logins.items():

You can just do:

for user in logins.keys():

chars = "0123456789"
chars += "abcdefghijklmnopqrstuvwxyz"
chars += "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
chars += "?%^*+~-=[]{}:,.#_"

This whole bit could be neatened up using string.printable, or another member from string. That module contains strings to be used for purposes like this:

import string

>>> string.printable
'0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}~ \t\n\r\x0b\x0c'

If there's stuff in there you don't want, you could either filter it, or just use a more narrow set of constants from that module:

>>> string.ascii_letters + string.digits + "?%^*+~-=[]{}:,.#_"
'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789?%^*+~-=[]{}:,.#_'

Either way, that's far less verbose

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ re: colorama, want to avoid 3rd party utilities; security, re: double recursion & passwords=None and choices/main... yes I do think those two definitions need to be combined; it just broke every time I've tried so far (only day 3), will put on todo list. re: string.ascii_letters cute! will do!; re f-string... hesitant as many users still <3.6, but yes cleaner; re: handle.close() I've actually seen that fail in race conditions without; so just needless habit here&now; re: login.keys(), yes!; re: dict(MENU), nice!; re: assert will look into cleaning that up. Thanks! v4.0 coming soon. \$\endgroup\$ – litepresence Dec 28 '19 at 1:22
  • \$\begingroup\$ all of your suggestions (except colorama) have been implemented. Thanks again! \$\endgroup\$ – litepresence Dec 28 '19 at 15:02
  • \$\begingroup\$ @litepresence No problem. Glad to help. \$\endgroup\$ – Carcigenicate Dec 28 '19 at 15:08
  • \$\begingroup\$ btw... TIL definition values can be functions! I had never considered such a thing. \$\endgroup\$ – litepresence Dec 28 '19 at 15:19
  • \$\begingroup\$ @litepresence What do you mean? That functions can be put into a dictionary? Functions are first class citizens in Python. They are objects that can be passed around and stored like other objects. \$\endgroup\$ – Carcigenicate Dec 28 '19 at 15:25

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.