2
\$\begingroup\$

I wrote a simple algorithm to encrypt and decrypt files in Python using aes-256-cbc.

from Crypto import Random
from Crypto.Cipher import AES
import base64

def pad(s):
    return s + b"\0" * (AES.block_size - len(s) % AES.block_size)

def encrypt(message, key, key_size=256):
    message = pad(message)
    iv = Random.new().read(AES.block_size)
    cipher = AES.new(key, AES.MODE_CBC, iv)
    return iv + cipher.encrypt(message)

def decrypt(ciphertext, key):
    iv = ciphertext[:AES.block_size]
    cipher = AES.new(key, AES.MODE_CBC, iv)
    plaintext = cipher.decrypt(ciphertext[AES.block_size:])
    return plaintext.rstrip(b"\0")

def encrypt_file(file_name, key):
    with open(file_name, 'rb') as fo:
        plaintext = fo.read()
    plaintext = base64.b64encode(plaintext)
    print(plaintext)
    enc = encrypt(plaintext, key)
    with open("file.txt" + ".enc", 'wb') as fo:
        fo.write(enc)

def decrypt_file(file_name, key):
    with open(file_name, 'rb') as fo:
        ciphertext = fo.read()
    dec = decrypt(ciphertext, key)
    dec = base64.b64decode(dec)
    with open(file_name[:-4]+ ".dec", 'wb') as fo:
        fo.write(dec)
    return dec.decode()


key = b'1234sfsfdsafasdf'

encrypt_file('file.txt', key)
decrypt_file('file.txt.enc', key)

However, I am definitely not a cryptography expert and I suppose my code may be missing something.

Is it safe to use this code for encryption of my files and what could be improved in this algorithm?

Also what do you think about This Implementation, is it secure and could be used in production?

\$\endgroup\$
3
  • \$\begingroup\$ I wrote a simple algorithm to encrypt and decrypt files in Python using aes-256-cbc if you manage to decrypt AES without following it to the letter, you may have broken it. \$\endgroup\$ – greybeard Feb 9 '20 at 19:08
  • \$\begingroup\$ Why is key_size=256 a default parameter in encrypt when you don't even use it? \$\endgroup\$ – Linny Feb 9 '20 at 19:21
  • \$\begingroup\$ @Linny This parameter was used in example on PyCrypto wiki, so I thought I would leave it there, but it is true, it doesn't do much \$\endgroup\$ – Stageflix Feb 9 '20 at 19:29
3
\$\begingroup\$
def pad(s):
    return s + b"\0" * (AES.block_size - len(s) % AES.block_size)

Generally a good library should have you covered when it comes to padding and unpadding. Prefer the Python 3 bytearray types rather than the stringified b"\0" trick.

Worse, you're implementing Zero padding, which would strip off zero valued bytes from your text files. That may not harm text files much (well, unless UTF-16LE I suppose), but for binary files that's very very dangerous.

Everybody tends to use PKCS#7 compatible padding for CBC mode.

def encrypt(message, key, key_size=256):

The key size is usually required for C-style languages but not here. You're already giving it a key, and the key has a certain size. The only thing you can do is to test the key against the key size if you're (overly) afraid that you might pass a 128 bit / 16 byte key by accident. But in general you should not pass parameters that can be derived from other parameters anyway.

    message = pad(message)

This creates a copy of the message. Now that's all right for small messages, but for files over 4 GiB it becomes "a bit of a nuisance". Especially if you decide to use your logical cores to encrypt many files. In theory AES/CBC pads the messages, but in practice we only pad the last 16 byte block that we need to encrypt.

    iv = Random.new().read(AES.block_size)

It's not all gloom and doom, you used a cryptographic random number generator to generate an unpredictable IV and you used AES.block_size instead of 16. And then prefixed it to the ciphertext. Well done!

    return plaintext.rstrip(b"\0")

And there's the Zero padding problem being executed. If you have a file consisting of all zero's, it would empty entirely. Oops!

    plaintext = base64.b64encode(plaintext)

Ah yes, that saves us from the padding problem, as base 64 doesn't contain zero valued bytes. The disadvantage is of course that your ciphertext will now expand 33%, and that you create another copy in memory for that size. Sure, the raw plaintext can now be freed up, but your system will still not be happy with you.

    with open("file.txt" + ".enc", 'wb') as fo:

The "file.txt" name is a bit out of wag with the 'wb' flag, don't you thing? And besides, although you base 64 encoded your plaintext, you didn't do it with the ciphertext. And you should not need to, just keeping everything in binary is fine for files.

Besides that, you probably should do something with a filename parameter. At least create a note to fix things later when you're experimenting.

    with open(file_name[:-4]+ ".dec", 'wb') as fo:

Ah, we just replace the last 4 characters now, without checking if the filename even has an extension?

    return dec.decode()

What, why? And what are we decoding?

key = b'1234sfsfdsafasdf'

Strings are not keys. If you want to create a test key, create one by decoding hexadecimal encoded bytes. And those look like 16 characters which means AES-128 rather than 256. If you want to use a password, you need a password based key derivation function (to perform so called Password Based Encryption, see the PKCS#5 standard).


CBC mode is fine for keeping files confidential, but generally cryptographers prefer authenticated modes such as GCM or EAX nowadays. Your files are likely kept confidential, but there is no message integrity or authenticity implemented for the files. Note that CBC mode is entirely unsuitable to send unauthenticated messages over a network.

Files can be large, so generally we try and use an API that provides update and final methods or a streaming API (not to be confused with the update method specified for pycryptodome's AEAD ciphers). That way you can encrypt files piecemeal.

Quite often for files it makes sense to use a specific container format such as CMS or PGP. In that case most of your errors are avoided (although, truth to be said, many container formats are starting to show their age).

\$\endgroup\$
2
  • \$\begingroup\$ Thank you very much, that was really helphul. Since you mentioned GSM mode, what's your opinion about this implementation \$\endgroup\$ – Stageflix Feb 9 '20 at 20:21
  • \$\begingroup\$ GCM mode. Uh, it's not really an implementation but a demo. I don't see any particular errors in it. The HazMat guys did seem to know a bit about how to use cryptography - but I don't know if there are any implementation mistakes. Biggest issue with it is that I don't see any way of encrypting chunk-by-chunk through update / final mechanism, which makes it unsuitable at least for big files. \$\endgroup\$ – Maarten Bodewes Feb 9 '20 at 20:39

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.