AES CTR mode using pycrypto

I've implemented CTR mode by myself (only decryption for now), using only AES built-in functions from pycrypto. It means that I'm not supposed to use mode=AES.MODE_CTR. However, I know that using AES.MODE_CTR would be more simple, but I'm doing this as a learning experience.

Is it a safe AES CTR??

(non-parallel version)

```
from Crypto.Cipher import AES

from Crypto.Cipher import AES

ciphers = ["69dda8455c7dd4254bf353b773304eec0ec7702330098ce7f7520d1cbbb20fc3" + \
"88d1b0adb5054dbd7370849dbf0b88d393f252e764f1f5f7ad97ef79d59ce29f5f51eeca32eabedd9afa9329", \
"770b80259ec33beb2561358a9f2dc617e46218c0a53cbeca695ae45faa8952aa" + \
"0e311bde9d4e01726d3184c34451"]

key = "36f18357be4dbd77f050515c73fcf9f2"

class IVCounter(object):
def __init__(self, value):
self.value = value

def increment(self):
# Add the counter value to IV
newIV = hex(int(self.value.encode('hex'), 16) + 1)

# Cut the negligible part of the string
self.value = newIV[2:len(newIV) - 1].decode('hex') # for not L strings remove $ - 1 $
return self.value

def __repr__(self):
self.increment()
return self.value

def string(self):
return self.value

class CTR():
def __init__(self, k):
self.key = k.decode('hex')

def __strxor(self, a, b): # xor two strings of different lengths
if len(a) > len(b):
return "".join([chr(ord(x) ^ ord(y)) for (x, y) in zip(a[:len(b)], b)])
else:
return "".join([chr(ord(x) ^ ord(y)) for (x, y) in zip(a, b[:len(a)])])

def __split_len(self, seq, lenght):
return [seq[i:i+lenght] for i in range(0, len(seq), lenght)]

def __AESencryptor(self, cipher):
encryptor = AES.new(self.key, AES.MODE_ECB)
return encryptor.encrypt(cipher)

def decrypt(self, cipher):
# Split the CT into blocks of 16 bytes
blocks = self.__split_len(cipher.decode('hex'), 16)

# Takes the initi

I couldn't spot any problems with your implementations, but caveat lector: I'm not a cryptography expert, so take that with an appropriate pinch of salt.

I'm also concerned about the output:

$ python reinvent.py
msg = CTR mode lets yo▒B▒▒▒c▒?▒N▒▒w▒▒ap▒9▒uz▒▒▒3▒▒▒o▒V_M▒▒A
msg = Always avoid the▒?A▒▒r ▒ά


It looks as if the first block has been decrypted correctly, and then it all falls over. Probably a bug, but I didn't dig into what's causing it.

Edit: Turns out this was an indentation problem – the code as posted had lost some indentation, and I guessed wrong when I tried to add it back.

The biggest problem with this code is the lack of docstrings and comments. Especially when you're dealing with cryptography – it's important to explain why you wrote the code you did, and how it relates to the original problem. This makes code easier to read, review and maintain – and would probably make it much easier to spot the bug above.

A variety of comments below.

IVCounter class

-
There should be a docstring explaining what this class represents, and what sort of values it expects as input. I had lots of fun hassle trying different inputs and hitting a variety of errors before I got it working.

-
Your __repr__ method is mutating the internal state of the object – that seems like a really bad idea. The usual use case for repr() is as a debugging tool; changing the instance you're trying to debug will be a bundle of laughs.

Here's an alternative repr you could use:

def __repr__(self):
    return '%s(%r)' % (self.__class__.__name__, self.value)

This returns a string that could be eval'd to get something equivalent to the current object, which is a much more conventional use of this function.

-
I'm not sure why you have a string method. It would be better for callers to access self.value directly, and if you want a string representation of an object, you should define __str__ instead.

-
What happens when the counter overflows? You get a nasty TypeError:

>>> x = IVCounter('\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe')
>>> x.increment()
'\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff'
>>> x.increment()
Traceback (most recent call last):
File "", line 1, in
File "", line 8, in increment
File "/usr/lib/python2.7/encodings/hex_codec.py", line 42, in hex_decode
output = binascii.a2b_hex(input)
TypeError: Odd-length string


You should think about how you want to handle an overflow like this – at the very least, you should wrap the exception and provide a more intelligible error message.

CTR class.

• Should subclass from object.

• Avoid single letter variable names where possible; try to prefer descriptive and expressive names. For example, __init__(self, key) instead of __init__(self, key). Or for block in blocks.

• In your __strxor method, you have repeated code – the only thing that changes is the arguments to zip(). Would be good to cut down that repetition.

• The variable name lenght is misspelt in the definition of the __split_len method.

• When I hear the word cipher, I think of things like AES, whereas you seem to be using it to describe a message text. That's a little confusing – docstrings would help.

-
In the decrypt() method, rather than:

self.IV = IVCounter(blocks[0])
blocks.remove(blocks[0])

you could just use:

self.IV = IVCounter(blocks.pop(0))

which will remove the item and extract it.

Misc

• You appear to be importing Crypto.Cipher.AES twice. Why?

StackExchange Code Review Q#40393, answer score: 2