decrypt/encrypt for block ciphers cannot be reused

[prokls]

Hi,

I think the block cipher API has a flaw. The encrypt and decrypt operations use self mut as a parameter. However, this is not actually required (AFAICS). encrypt and decrypt call encrypt_blocks and decrypt_blocks respectively, but they only require some &self mut. The point is that ciphers are consumed and not borrowed. Thus reuse of a cipher for consecutive operations are not possible. Consider this example:

use block_modes::block_padding::ZeroPadding;
use block_modes::{BlockMode, Cbc};
use twofish::Twofish;

fn main() {
    let k = [0u8; 32];
    let iv = [0u8; 16];
    let mut block = [0u8; 16];

    let cbc_cipher = Cbc::<Twofish, ZeroPadding>::new_from_slices(&k, &iv).unwrap();

    cbc_cipher.decrypt(&mut block).unwrap();
    cbc_cipher.decrypt(&mut block).unwrap();
}

The second call to decrypt is not allowed since the first decrypt consumes it. What do you think about this flaw?

[newpavlov]

Note that BlockEncrypt is implemented for &T where T: BlockEncrypt. In other words, you can create the CBC mode by passing to the new method &Twofish instead of Twofish, thus in such scenario your block mode will have type Cbc<&Twofish, ZeroPadding>. For obvious reasons &Twofish does not implement NewBlockCipher, so you can't use the new_fix and new_from_slices methods for creating such block mode.

[prokls]

Thank you for the hint.

Sadly, I don't think it solves the underlying issue. I adjusted my snippet according to your suggestions. The same error occurs again, because self mut will still consume the Cbc instance.

One experiment can be to replace the Cbc instance with some &Cbc instance. This will also fail since BlockMode is implemented for Cbc, but not for &Cbc and thus auto-dereferencing kicks in.

I just recognized that I wrote

The point is that ciphers are consumed and not borrowed.

I am sorry. I meant, block modes not ciphers.

Cargo.toml:

[package]
name = "rustcryptotest"
version = "1.0.0"
edition = "2018"

[dependencies.block-modes]
version = "0.8.1"

[dependencies.twofish]
version = "0.6.0"

[dependencies.typenum]
version = "1.14.0"

main.rs:

use twofish::cipher::generic_array::GenericArray;
use twofish::cipher::NewBlockCipher;
use twofish::Twofish;
use block_modes::block_padding::ZeroPadding;
use block_modes::{BlockMode, Cbc};
use typenum::{U16, U32};

fn main() {
    let mut block = [0u8; 16];
    let key: &GenericArray<u8, U32> = GenericArray::from_slice(&[0u8; 32]);
    let iv: &GenericArray<u8, U16> = GenericArray::from_slice(&[0u8; 16]);

    let cipher: Twofish = Twofish::new(&key);
    let cbc_cipher: Cbc<&Twofish, ZeroPadding> = Cbc::<&Twofish, ZeroPadding>::new(&cipher, &iv);

    cbc_cipher.decrypt(&mut block).unwrap();
    cbc_cipher.decrypt(&mut block).unwrap();
}

error message:

error[E0382]: use of moved value: `cbc_cipher`
  --> src/main.rs:17:5
   |
14 |     let cbc_cipher: Cbc<&Twofish, ZeroPadding> = Cbc::<&Twofish, ZeroPadding>::new(&cipher, &iv);
   |         ---------- move occurs because `cbc_cipher` has type `Cbc<&Twofish, ZeroPadding>`, which does not implement the `Copy` trait
15 | 
16 |     cbc_cipher.decrypt(&mut block).unwrap();
   |                ------------------- `cbc_cipher` moved due to this method call
17 |     cbc_cipher.decrypt(&mut block).unwrap();
   |     ^^^^^^^^^^ value used here after move
   |
note: this function takes ownership of the receiver `self`, which moves `cbc_cipher`
  --> /home/lprokop/.cargo/registry/src/github.com-1ecc6299db9ec823/block-modes-0.8.1/src/traits.rs:68:20
   |
68 |     fn decrypt(mut self, buffer: &mut [u8]) -> Result<&[u8], BlockModeError> {
   |                    ^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0382`.

[newpavlov]

You should re-create the block mode from the cipher instance. This is a very cheap operation (for most modes it simply copies iv and cipher reference). We generally do not store initial IV in block modes, so we can not reset it back to its initial state inside methods. This is why the methods in question consume block mode instance, to specifically prevent errors like the one you were gonna to make.

[prokls]

Thanks!

Based on your comment, I can see that you assumed I want to use the same IV repeatedly. Actually, I want to initialize the CBC instance with one IV and then call encrypt or decrypt repeatedly to encrypt/decrypt block-by-block one after another. Apparently, this is not supported which goes against my intuition of block ciphers. Instead, you have to provide all blocks simultaneously and can only call the CBC instance's method once.

[newpavlov]

Your scenario is somewhat unusual. If you absolutely sure that it's indeed what you want, then you have to pad messages manually and use the encrypt_blocks/decrypt_blocks methods. The consuming API is intentional and we use a similar approach in some other crates (e.g. in hash functions). Usually you pad and encrypt message using a single key/IV pair, so block mode state after message was processed is viewed as a "garbage" state, which should not be used further.

[prokls]

Alright, I understand. Thank you! I consider this issue closed.

[DavidVentura]

hi!
i am having the same "issue". i am trying to work with some performance issues on the keepass-rs crate (i am new to rust)

re-creating the cipher is not so cheap, as per
https://github.com/sseemayer/keepass-rs/blob/master/src/crypt/kdf.rs#L27
this creation, when opening a keepass database is executed 40 million times, and the runtime for this function on a slow device (like a phone) is ~15 seconds.

by moving the cipher creation outside of the loop and calling clone on every iteration, the execution time is halved, but still too long at ~7s.

is there a way to avoid the creation/cloning from being necessary on every iteration of the encryption?

[newpavlov]

@DavidVentura
Please, re-read my first messages in this thread. You can create block modes using block cipher references, so no cloning is needed.

But even before that, your code is quite wasteful. My guess is that you get a bigger performance hit from allocations on each iteration, not from cipher cloning. I would suggest splitting your composite_key into two GenericArray<u8, U16> blocks and applying Aes256::encrypt_block on them directly in-place, something like this (it probably will not compile right away, but should convey the main idea):

let cipher = Aes256::new(&self.seed);
let mut block1 = GenericArray::clone_from_slice(&composite_key[..16]);
let mut block2 = GenericArray::clone_from_slice(&composite_key[16..]);
for _ in 0..self.rounds {
    cipher.encrypt_block(&mut block1);
    cipher.encrypt_block(&mut block2);
}

Then you can either join them back into GenericArray<u8, U32> or pass separately to Sha256::update.

[DavidVentura]

hey, it is not my crate, i am trying to use it.
the code you posted worked fine and it brought execution down 10x! it now takes 0.66s from 7.5 (and 14 in the original code)
thanks a lot