6
\$\begingroup\$

I have the code bellow, which is supposed to be used inside a larger program. Please see notes about the requirements below the code.

from __future__ import print_function
from Crypto.Cipher import AES
from Crypto.Protocol.KDF import PBKDF2
import base64
import os
import sys
import binascii

if sys.version_info.major > 2:
    raw_input = input

EncodeAES = lambda c, s: base64.b64encode(c.encrypt(s))
DecodeAES = lambda c, e: c.decrypt(base64.b64decode(e)).rstrip()


def get_digest(password, salt):
    """
    Get a digest based on clear text password
    """
    iterations = 5000
    return PBKDF2(password, salt, dkLen=32, count=iterations)


def authenticate(password, salt, digest):
    """
    salt and digest are stored in a file or a database
    """
    dig = get_digest(password, salt)
    return binascii.hexlify(dig) == digest


def write_password():
    """
    Write a secret password as a hash and the salt used for this hash
    to a file
    """
    salt = base64.b64encode(os.urandom(32))
    passwd = raw_input("Please type in the secret key:")
    key = get_digest(passwd, salt)
    f = open('passwords.txt', 'wt')
    hpk = salt+'$6$'.encode('utf8')+binascii.hexlify(key)
    f.write(hpk.decode('utf-8'))
    f.close()

def get_digest_from_file(filename):
    """
    Read a digested password and salt from the file
    """
    f = open(filename, 'rt')
    sf = f.readline()
    f.seek(0)
    salt, digest = f.readline().split('$6$')
    return salt.encode('utf-8'), digest.encode('utf-8')


def get_cipher(password, salt):
    """
    Create a chiper object from a hashed password
    """
    iv = os.urandom(AES.block_size)
    dig = get_digest(password, salt)
    chiper = AES.new(dig, AES.MODE_ECB, iv)
    return chiper

def cli_auth():
    """
    Read password from the user, if the password is correct,
    finish the execution an return the password and salt which
    are read from the file.
    """
    salt, digest = get_digest_from_file('passwords.txt')
    while True:
        password = raw_input("Please type in your password:").encode('utf-8')
        if authenticate(password, salt, digest):
            return password, salt

def prepare_data(text, block_size):
    """
    prepare data before encryption so the lenght matches the expected
    lenght by the algorithm.
    """
    num_blocks = len(text)//block_size + 1
    newdatasize = block_size*num_blocks
    return text.ljust(newdatasize)

def save_a_secret_message():
    """
    PoC to show we can encrypt a message
    """
    secret_msg = """This is a very important message! Learn Cryptography!!!"""
    # the secret message will be encrypted with the secret password found
    # in the file
    passwd, salt = cli_auth()
    cipher = get_cipher(passwd, salt)
    # explictly destroy password, so now there is no clear text reference
    # to the input given by the user
    del(passwd)
    msg = EncodeAES(cipher, prepare_data(secret_msg, AES.block_size))
    with open('secret.enc','wt') as s:
        s.write(msg.decode())
    print("The cipher message is:", msg.decode())

def read_a_secret_message():
    """
    PoC to show we can decrypt a message
    """
    passwd, salt = cli_auth()
    cipher = get_cipher(passwd, salt)
    del(passwd)
    with open('secret.enc') as s:
        msg = s.readline()
        print("The decrypted secret message is:")
        decoded = DecodeAES(cipher, msg)
        print(decoded)

if __name__ == '__main__':
    if '-i' in sys.argv:
        write_password()
    save_a_secret_message()
    read_a_secret_message()
  • The user is supposed to enter is password only once.
  • but decryption and encryption take place many times while the program is running.
  • There should be a mechanism to save the password (or a digest) on the disk (not in clear text).
  • Should be Python2 and Python3 compatible.

So far, it seems to me, it all works OK. I tested python2 and python3. The cipher text can be decrypted with different Python versions too.

However, I am not a Crypto expert, and I would definitely like to here some criticism about this code.

\$\endgroup\$
2
  • \$\begingroup\$ "on the disk (not in clear text)." That only shifts the problem to where you'd store the key to encrypt the password. \$\endgroup\$ Aug 6, 2014 at 12:51
  • \$\begingroup\$ @CodesInChaos, can you elaborate? I don't get what you mean. \$\endgroup\$
    – oz123
    Aug 6, 2014 at 12:54

1 Answer 1

2
\$\begingroup\$

Be careful naming your functions

You declare a lambda function named EncodeAES, but this function itself doesn't know anything about AES. It accepts an object with an encrypt() member function which it calls, but that object could be anything.

The name should reflect what the function does. This one does two things: it base64-encodes, and it calls encrypt(). I think any descriptive name would be really awkward, maybe it is best not to use these lambda's at all, especially since they are only used once.

Don't modify the message before/after encryption

Your DecodeAES lambda calls rstrip() on the decrypted message. But what if the original text has whitespace of its own at the end of the message that is significant? Or what if the message is not a piece of text but a binary file? Then the output after decryption will be different from the message before encryption. It also allows for length extension attacks.

Don't use ECB mode

You should never use ECB mode to encrypt things, it has many issues such as revealing patterns in the plaintext, and allowing an attacker to flip bits in the plaintext at will.

Almost any other mode would be suitable to use, if you add a unique initialization vector (IV) to the ciphertext. Note that this IV should be different each time you write an encrypted file, otherwise it might make it easy for someone to decrypt parts of the message by comparing two ciphertexts.

Even better would be to use an authenticated encryption mode to ensure that if an attacker has tampered with the ciphertext, this is detected when trying to decrypt the message. If you don't provide any form of authentication of the ciphertext, it allows an attacker to modify it, and then the output after decryption will be modified as well, sometimes in ways that are not easy to spot.

Don't use home-grown crypto in production code

While it is perfectly fine to try to implement your own encryption protocols (it's a good way to start learning about it), it is very easy to make mistakes, and mistakes will be costly (since there probably is a good reason why you want to keep the data private).

Go to https://crypto.stackexchange.com/ and search there for how to safely encrypt files. You'll likely find many answers like this one that tell you to not try it yourself and rather use existing, well-established software to do encryption for you. If you don't want to become a crypto expert, then follow that advice. If you really do want to become better at crypto, then have a look at the well-established software and protocols out there, and try to learn how they work.

Use a higher-level cryptography library

The Crypto package is quite low-level. It provides you the basics of encryption, but already for properly encrypting a file you need to combine several techniques. It is better to use a library that provides higher level functions that take care of all the details for you. There are many of them out there. A popular one is PyNacl.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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