Potential for security issues with AESEncryptedField

[cym13]

Hi,

The AESEncryptField's encryption is initialized as such:

    def get_cipher(self, key, iv):
        if len(key) > 32:
            raise ValueError('Key length cannot exceed 32 bytes.')
        key = key + bytes(' ') * (32 - len(key))
        return AES.new(key, AES.MODE_CFB, iv)

It doesn'st use a key derivation algorithm such as PBKDF2 and padds the key with spaces. This is wrong on two levels:

• The key needs to be random. Using the password directly exposes it to frequency attacks.
• If the key isn't 32 chars long then you are guaranteed that the last byte of the key is a space. In the same way you are almost guaranteed that the byte before is a space. In fact, as people think that this is a password they're not likely to use much more than 12 characters, which means that half the key will often be spaces. This has of course dramatic consequences on the security of encrypted data.

Besides there have been attacks against CFB mode but it doesn't seem usable there. If you can I'd recommend using CTR instead.

EDIT: fixed misleading terminology

[coleifer]

@cym13 -- may I ask what you intend to use the AESEncryptedField for? Is this for an application you're working on?

[coleifer]

@cym13 -- may I ask what you intend to use the AESEncryptedField for? Is this for an application you're working on?

[coleifer]

Sorry, I should've added -- I definitely intend to address the issues you've raised. Thanks. I also edited the title of the comment to be a little more representative of the situation.

The code I'm using is mostly based on the example in pycrypto itself, you can see here:

>>> from Crypto.Cipher import AES
>>> obj = AES.new('This is a key123', AES.MODE_CBC, 'This is an IV456')
>>> message = "The answer is no"
>>> ciphertext = obj.encrypt(message)
>>> ciphertext
'\xd6\x83\x8dd!VT\x92\xaa`A\x05\xe0\x9b\x8b\xf1'
>>> obj2 = AES.new('This is a key123', AES.MODE_CBC, 'This is an IV456')
>>> obj2.decrypt(ciphertext)
'The answer is no'

Versus peewee:

        def get_cipher(self, key, iv):
            if len(key) > 32:
                raise ValueError('Key length cannot exceed 32 bytes.')
            key = key + bytes(' ') * (32 - len(key))
            return AES.new(key, AES.MODE_CFB, iv)

        def encrypt(self, value):
            iv = Random.get_random_bytes(AES.block_size)
            cipher = self.get_cipher(self.key, iv)
            return iv + cipher.encrypt(value)

        def decrypt(self, value):
            iv = value[:AES.block_size]
            cipher = self.get_cipher(self.key, iv)
            return cipher.decrypt(value[AES.block_size:])

[coleifer]

Sorry, I should've added -- I definitely intend to address the issues you've raised. Thanks. I also edited the title of the comment to be a little more representative of the situation.

The code I'm using is mostly based on the example in pycrypto itself, you can see here:

>>> from Crypto.Cipher import AES
>>> obj = AES.new('This is a key123', AES.MODE_CBC, 'This is an IV456')
>>> message = "The answer is no"
>>> ciphertext = obj.encrypt(message)
>>> ciphertext
'\xd6\x83\x8dd!VT\x92\xaa`A\x05\xe0\x9b\x8b\xf1'
>>> obj2 = AES.new('This is a key123', AES.MODE_CBC, 'This is an IV456')
>>> obj2.decrypt(ciphertext)
'The answer is no'

Versus peewee:

        def get_cipher(self, key, iv):
            if len(key) > 32:
                raise ValueError('Key length cannot exceed 32 bytes.')
            key = key + bytes(' ') * (32 - len(key))
            return AES.new(key, AES.MODE_CFB, iv)

        def encrypt(self, value):
            iv = Random.get_random_bytes(AES.block_size)
            cipher = self.get_cipher(self.key, iv)
            return iv + cipher.encrypt(value)

        def decrypt(self, value):
            iv = value[:AES.block_size]
            cipher = self.get_cipher(self.key, iv)
            return cipher.decrypt(value[AES.block_size:])

[cym13]

I don't use this field myself, someone else I know does. Maybe he'll come comment himself he he feels like so.

What PyCrypto does is completely different from what your code does and the PyCrypto snippet you choose shows nothing of its internal mechanics. I'm sorry, but there's no way arround it: AESEncryptedField is broken and endangers its users. Maybe I'll take some time this weekend to demonstrate actual decryption, the flaw is important enough that I have no doubt about the feasibility of it. It's not one of those theoretical issues.

As a side note I would recommend against taking PyCrypto as a reference when it comes to security. The project is very flawed itself with some vulnerabilities that aren't likely to be fixed ever (dlitz/pycrypto#176). The API makes it very easy to get things wrong too. Try https://pypi.python.org/pypi/cryptography or PyCryptodome which is a fork of PyCrypto if you need compatibility.

[cym13]

I don't use this field myself, someone else I know does. Maybe he'll come comment himself he he feels like so.

What PyCrypto does is completely different from what your code does and the PyCrypto snippet you choose shows nothing of its internal mechanics. I'm sorry, but there's no way arround it: AESEncryptedField is broken and endangers its users. Maybe I'll take some time this weekend to demonstrate actual decryption, the flaw is important enough that I have no doubt about the feasibility of it. It's not one of those theoretical issues.

As a side note I would recommend against taking PyCrypto as a reference when it comes to security. The project is very flawed itself with some vulnerabilities that aren't likely to be fixed ever (dlitz/pycrypto#176). The API makes it very easy to get things wrong too. Try https://pypi.python.org/pypi/cryptography or PyCryptodome which is a fork of PyCrypto if you need compatibility.

[coleifer]

Thanks for the explanation. I'll go ahead and remove the field, that seems like the best option.

[coleifer]

Thanks for the explanation. I'll go ahead and remove the field, that seems like the best option.

[coleifer]

Removed for 2.10.0: https://github.com/coleifer/peewee/releases/tag/2.10.0 -- thanks for bringing these problems to my attention.

[coleifer]

Removed for 2.10.0: https://github.com/coleifer/peewee/releases/tag/2.10.0 -- thanks for bringing these problems to my attention.