PyCrypto AES-256 CTR wrapper secure for public use?

I have tried to write a more user-friendly AES CTR wrapper with PyCrypto, but I'm not sure if it's safe enough. I mean, by default there is IV=1 for CTR and in documentation it's said that IV is ignored anyway, so I'm not sure if I should use it or not (or if it's even necessary rather than overkill).

This code should be for files on a local disk, nothing to do with networking. The code saves initial part of a Counter i.e. secret.secret to a separate file together with the encrypted one (two files). Counter (or rather parts of it) in the script is incremented always by 1 and if it comes to 255 (which seems to throw some error), then it resets to 0 and starts again, so for each block for file stream there should be a unique counter (go print it in the while loop), therefore each block should be safe enough if only user knows the key and the counter incrementing isn't something obvious as +1, but rather something more complex such as working with % and remainders.

• Is my code safe to use in public? Of course this is only an example, counter incrementing is much more complex than +1.

• Is my code still safe if I reveal how Counter is incrementing and someone has access to .ctr file if he/she doesn't know the key(32)?

Note: the key(32) is generated with user's input combined with a machine's constant + salt and other stuff. User has to input password for each usage of the code and user's input isn't stored anywhere.

 

```
import os
import array
from Crypto.Cipher import AES

class Secret(object):
def __init__(self, secret=None):
if secret is None:
secret = os.urandom(16)
self.secret = secret
self.reset()

def counter(self):
for i, c in enumerate(self.current):
if c + 1 == 255:
self.current[i] = 0
else:
self.current[i] = c + 1
return self.current.tostring()

def reset(self):
self.current = array.array('B', self.secret)

clas

This is buggy and insecure due to the implementation of the Secret class.

1. Bug

If by chance one of the bytes returned by os.urandom should be 255, then the counter method will fail:

>>> [Secret().counter() for _ in range(100)]
Traceback (most recent call last):
  File "", line 1, in 
  File "cr131248.py", line 18, in counter
    self.current[i] = c + 1
OverflowError: unsigned byte integer is greater than maximum

This will happen for about 6% of Secret instances.

It looks as if you are confused about what's going on here. A byte array consists of bytes, each of which consists of exactly 8 bits. So a byte can only take values between 0 and 255 (inclusive). That's why you get an error if you try to assign a value outside that range.

1. Insecure

Quoting from Dworkin's "Recommendation for Block Cipher Modes of Operation":


The Counter (CTR) mode is a confidentiality mode that features the application of the forward cipher to a set of input blocks, called counters, to produce a sequence of output blocks that are exclusive-ORed with the plaintext to produce the ciphertext, and vice versa. The sequence of counters must have the property that each block in the sequence is different from every other block.

But the counter blocks generated by the Secret class repeat every 255 blocks:

>>> secret = Secret()
>>> ctr = [secret.counter() for _ in range(400)]
>>> ctr[255] == ctr[0]
True
>>> ctr[256] == ctr[1]
True

That's because the counter method adds 1 to every byte in the array, wrapping around every 255 additions. So the array only gets 255 different values before it starts to repeat.

This is disastrous for the security of the message because if the message is long enough (more than 255 blocks or 4080 bytes) then it will have been built using repeated counter blocks. So if an eavesdropper XORs blocks 0 and 255 of the ciphertext, the duplicate encrypted counter blocks will cancel, leaving the XOR of blocks 0 and 255 of the plaintext, and from the XOR of two parts of the plaintext it is possible to recover the plaintext (see this answer on crypto.stackexchange.com).

If you are really determined to implement your own counter blocks, then Appendix B.1 of "Recommendation for Block Cipher Modes of Operation" explains how to do it correctly.

But it would be much better to use Crypto.Util.Counter.

1. How to count

The problem with Secret.counter is that it increments every byte of the counter. So if counter.current starts like this:

[253, 208, 169, 133, 161, 177, 54, 116, 192, 104, 69, 248, 65, 60, 111, 115]

then after one call to counter it becomes:

[254, 209, 170, 134, 162, 178, 55, 117, 193, 105, 70, 249, 66, 61, 112, 116]

and this is what causes it to repeat after only 255 calls.

What you need to do instead is to increment only the first byte, but if that rolls around from 255 to 0, increment the second byte, and if that rolls around rom 255 to 0, increment the third byte, and so on. This procedure is just just like counting in decimal, where you increment the units digits, but if that rolls around from 9 to 0 then you increment the tens digit, and so on.

So in the above example the array should be incremented to:

[254, 208, 169, 133, 161, 177, 54, 116, 192, 104, 69, 248, 65, 60, 111, 115]

and then:

[255, 208, 169, 133, 161, 177, 54, 116, 192, 104, 69, 248, 65, 60, 111, 115]

and then 255 rolls over to 0, so you increment the second byte:

[0, 209, 169, 133, 161, 177, 54, 116, 192, 104, 69, 248, 65, 60, 111, 115]

and then:

[1, 209, 169, 133, 161, 177, 54, 116, 192, 104, 69, 248, 65, 60, 111, 115]

and so on.

If you look at what Crypto.Util.Counter does, you'll see that it works in much the same way:

>>> c = Counter.new(128, initial_value=int.from_bytes(os.urandom(16), 'big'), allow_wraparound=True)
>>> array.array('B', c())
array('B', [170, 36, 8, 107, 115, 112, 224, 212, 58, 251, 145, 1, 68, 57, 75, 254])
>>> array.array('B', c())
array('B', [170, 36, 8, 107, 115, 112, 224, 212, 58, 251, 145, 1, 68, 57, 75, 255])
>>> array.array('B', c())
array('B', [170, 36, 8, 107, 115, 112, 224, 212, 58, 251, 145, 1, 68, 57, 76, 0])
>>> array.array('B', c())
array('B', [170, 36, 8, 107, 115, 112, 224, 212, 58, 251, 145, 1, 68, 57, 76, 1])

except that it increments the last byte in the array and works backwards.

Notice that I passed a random initial_value argument to Counter.new. That's because the counter block need to be unique across all messages using the same key (otherwise an eavesdropper could find two messages that have coincident counter blocks and XOR them to recover the plaintext, just as if the two blocks had occurred in the same message). I also specified allow_wraparound=True so that there's no possibility of getting an OverflowError if we are very unlucky with the result of os.urandom.

StackExchange Code Review Q#131248, answer score: 3