serpent: mismatch between `KeySizeUser` impl and dynamically determined key size

[bleichenbacher-daniel]

I'm currently testing various ciphers with different encryption modes. I've been unsuccessful in using CBC (and CFB) together with
Serpent with 192-bit and 256-bit keys. I'm unsure if or how this could be achieved.

The crate serpent does support other key sizes. E.g. using Serpent as a block cipher to encrypt single blocks works find with all key sizes
(128 bit, 160 bit, 192 bit, 224 bit and 256 bit). Also the encryption mode XTS works with multiple key sizes.

Other ciphers have variants such as aes::{Aes128, Aes192, Aes256} to specify the expected key size. I have not found an equivalent for
Serpent.

pub fn encrypt(alg: &str, key: &[u8], iv: &[u8], inp: &[u8]) -> Vec<u8> {
  match alg {
    // Serpent with 128-bit keys is OK.
    "serpentcbc128_pkcs7" => {
        type Serpent128CbcEnc = cbc::Encryptor<serpent::Serpent>;
        let key_bytes: &[u8; 16] = key.try_into().unwrap();
        let iv_bytes: &[u8; 16] = iv.try_into().unwrap();
        let res = Serpent128CbcEnc::new(key_bytes.into(), iv_bytes.into())
            .encrypt_padded_vec_mut::<Pkcs7>(&inp);
        return res.to_vec();
    },
    // This does not compile since the compiler expects 128 bit keys.
    "serpentcbc192_pkcs7" => {
        type Serpent192CbcEnc = cbc::Encryptor<serpent::Serpent>;
        let key_bytes: &[u8; 24] = key.try_into().unwrap();
        let iv_bytes: &[u8; 16] = iv.try_into().unwrap();
        let res = Serpent192CbcEnc::new(key_bytes.into(), iv_bytes.into())
            .encrypt_padded_vec_mut::<Pkcs7>(&inp);
        return res.to_vec();
    }, 
    // This does not compile either since the compiler expects 128 bit keys.
    "serpentcbc256_pkcs7" => {
        type Serpent256CbcEnc = cbc::Encryptor<serpent::Serpent>;
        let key_bytes: &[u8; 32] = key.try_into().unwrap();
        let iv_bytes: &[u8; 16] = iv.try_into().unwrap();
        let res = Serpent256CbcEnc::new(key_bytes.into(), iv_bytes.into())
            .encrypt_padded_vec_mut::<Pkcs7>(&inp);
        return res.to_vec();
    }, 
    // This works.
    "serpentcfb128_128" => {
        type Serpent128CfbEnc = cfb_mode::Encryptor<serpent::Serpent>;
        let key_bytes: &[u8; 16] = key.try_into().unwrap();
        let iv_bytes: &[u8; 16] = iv.try_into().unwrap();
        let mut buf = inp.to_vec();
        Serpent128CfbEnc::new(key_bytes.into(), iv_bytes.into()).encrypt(&mut buf);
        return buf;
    },
    // This does not compile, since the expected key size is 128-bit.
    "serpentcfb128_192" => {
        type Serpent192CfbEnc = cfb_mode::Encryptor<serpent::Serpent>;
        let key_bytes: &[u8; 24] = key.try_into().unwrap();
        let iv_bytes: &[u8; 16] = iv.try_into().unwrap();
        let mut buf = inp.to_vec();
        Serpent192CfbEnc::new(key_bytes.into(), iv_bytes.into()).encrypt(&mut buf);
        return buf;
    },
    // XTS with two 128-bit keys works.
    "serpentxts_256"  => {
        let cipher_1 = serpent::Serpent::new_from_slice(&key[..16]).unwrap();
        let cipher_2 = serpent::Serpent::new_from_slice(&key[16..]).unwrap();
        let xts_cipher = xts_mode::Xts128::<serpent::Serpent>::new(cipher_1, cipher_2);
        let iv_bytes: &[u8; 16] = iv.try_into().unwrap();
        let mut buf = inp.to_vec();
        xts_cipher.encrypt_sector(&mut buf, *iv_bytes);
        return buf;
    },
    // XTS with two 256-bit keys works too (and also gives the correct result).
    "serpentxts_512"  => {
        let cipher_1 = serpent::Serpent::new_from_slice(&key[..32]).unwrap();
        let cipher_2 = serpent::Serpent::new_from_slice(&key[32..]).unwrap();
        let xts_cipher = xts_mode::Xts128::<serpent::Serpent>::new(cipher_1, cipher_2);
        let iv_bytes: &[u8; 16] = iv.try_into().unwrap();
        let mut buf = inp.to_vec();
        xts_cipher.encrypt_sector(&mut buf, *iv_bytes);
        return buf;
    },
    // ...
    _ => {panic!("Unsupported algorithm:{}", alg);}
  }
}

The versions I'm using are:

cbc = { version = "0.1.2", features = ["block-padding", "alloc"] }
cfb-mode = { version = "0.8.2", features = ["block-padding", "alloc"] }
chacha20poly1305 = "0.10.1"
cipher = { version = "0.4.4", features = ["alloc", "block-padding"] }
serpent = "0.5.1"
xts-mode = "0.5.1"

[tarcieri]

The crate serpent does support other key sizes. E.g. using Serpent as a block cipher to encrypt single blocks works find with all key sizes (128 bit, 160 bit, 192 bit, 224 bit and 256 bit).

It looks like the serpent crate is implemented a bit weirdly. The Serpent type is configured for a 128-bit key size:

block-ciphers/serpent/src/lib.rs

Lines 72 to 74 in 1f7c695

	impl KeySizeUser for Serpent {
	type KeySize = U16;
	}

However KeyInit::new_from_slice has been overridden to support other key sizes, dynamically determined by the size of the slice:

block-ciphers/serpent/src/lib.rs

Lines 81 to 123 in 1f7c695

	fn new_from_slice(key: &[u8]) -> Result<Self, InvalidLength> {
	if key.len() < 16 || key.len() > 32 {
	return Err(InvalidLength);
	}
	let key = expand_key(key, key.len() * 8);
	let mut words = [0u32; 140];
	for (src, dst) in key.chunks_exact(4).zip(words[..8].iter_mut()) {
	*dst = u32::from_le_bytes(src.try_into().unwrap());
	}
	for i in 0..132 {
	let slot = i + 8;
	words[slot] = (words[slot - 8]
	^ words[slot - 5]
	^ words[slot - 3]
	^ words[slot - 1]
	^ PHI
	^ i as u32)
	.rotate_left(11);
	}
	let r = ROUNDS + 1;
	let words = &words[8..];
	let mut k = [0u32; 132];
	for i in 0..r {
	let sbox_index = (ROUNDS + 3 - i) % ROUNDS;
	let [a, b, c, d]: [u32; 4] = words[4 * i..][..4].try_into().unwrap();
	// calculate keys in bitslicing mode
	let output = bitslice::apply_s(sbox_index, [a, b, c, d]);
	for l in 0..4 {
	k[4 * i + l] = output[l];
	}
	}
	let mut round_keys: RoundKeys = [[0; 4]; ROUNDS + 1];
	for (src, dst) in k.chunks_exact(4).zip(round_keys.iter_mut()) {
	dst.copy_from_slice(src);
	}
	Ok(Serpent { round_keys })
	}

If you do use any size other than 128-bits, there will be a mismatch between the KeySizeUser::KeySize and the dynamically determined key size.

It should probably be refactored so there are types like Serpent128, Serpent160, Serpent192, Serpent224, Serpent256 which implement the correct KeySizeUser::KeySize, but share a common core. If need be, we can preserve the existing Serpent type to allow dynamic key sizes, but it shouldn't impl traits like KeyInit and KeySizeUser since the key size isn't fixed at compile-time.

I can move this issue over to https://github.com/RustCrypto/block-ciphers since it's a problem with the serpent crate itself.

[newpavlov]

This works as intended. For algorithms which support variable key sizes, the KeySize associated type indicates the "default" (or "recommended") key size for the algorithm. If you want to use a block mode with variable key size, then you should use the KeyIvInit::new_from_slices method. It will use KeyInit::new_from_slice to initialize the block cipher.

It should probably be refactored so there are types like Serpent128, Serpent160, Serpent192, Serpent224, Serpent256 which implement the correct KeySizeUser::KeySize, but share a common core.

We could do it, but then we would use the ability to work with key sizes variable at runtime. Right now we introduce separate types if encryption/decryption is different for different key sizes (like in the case of AES) and use one type if encryption/decryption is the same and different key sizes only result in different cipher initialization.

[tarcieri]

This works as intended. For algorithms which support variable key sizes, the KeySize associated type indicates the "default" (or "recommended") key size for the algorithm

That seems confusing for the very reasons this issue was opened in the first place.

We could do it, but then we would use the ability to work with key sizes variable at runtime

You could keep the existing Serpent type but remove the KeyInit and KeySizeUser impls, moving those to ones which are typed with the appropriate size.

[bleichenbacher-daniel]

First, thanks a lot for the help. My tests are now running and give the desired results.

A unified interface for unauthenticated ciphers would indeed be quite useful, but the main issue was just to get things to compile and run.

[newpavlov]

That seems confusing for the very reasons this issue was opened in the first place.

We should document it better, but I think the current solution works well enough. Enforcing compile-time key sizes would mean genericity over key size (e.g. blowfish supports key sizes from 4 to 56 bytes), which would mean a more complex user-facing API for very little benefit. It would be even worse if we are to introduce separate runtime-variable KeyInit/KeyIvInit traits with associated blanket impls to bridge with compile time size traits.

Overall, it looks like the issue is resolved and we can close it. It's better to discuss potential trait improvements in a separate traits issue.

[tarcieri]

I don't think the issue was addressed? Serpent seems to be unusable with block modes for key sizes other than 128-bits

[newpavlov]

Serpent seems to be unusable with block modes for key sizes other than 128-bits

No, it can be used with alternative key sizes. You just have to use KeyIvInit::new_from_slices instead of KeyIvInit::new.

Note that @bleichenbacher-daniel has wrote the following:

My tests are now running and give the desired results.

[tarcieri]

You just have to use KeyIvInit::new_from_slices instead of KeyIvInit::new.

Or it could just work. I feel like the current approach is breaking the type safety these traits are supposed to provide. Instead the code examples above fail at runtime.

[newpavlov]

Again, the current implementation works as intended. The block modes implementations were written while keeping in mind block ciphers which support variable key sizes. If you have a proposal to introduce separate traits for runtime-variable key/IV size initialization, we should discuss it in a separate traits issue.

[tarcieri]

Again, the current implementation works as intended.

KeyIvInit::new is not working as intended in this case. If things were working correctly, the issue would never have been opened in the first place.

It would work if we had types like Serpent128 and Serpent256 at the very least. Or we could make it generic around the allowed range of key sizes, and use that as the key size for the KeySizeUser impl.

If you have a proposal to introduce separate traits for runtime-variable key/IV size initialization, we should discuss it in a separate traits issue.

I want the opposite of that: compile-time type safety for key sizes.

[newpavlov]

KeyIvInit::new is not working as intended in this case. If things were working correctly, the issue would never have been opened in the first place.

I disagree. It's just an API misunderstanding in the context of block ciphers which support a range of key sizes. The new_from_slice(s) methods were specifically introduced to support such cases (otherwise we could've just forced users to handle conversion into arrays in their code). I am telling you as the one who introduced them (i.e. the intent behind the methods was mine). If we had better docs about this, this issue may have not been open.

As I wrote above, we could make the implementations generic over key size (with appropriate trait bounds), but it would mean a more complex API and loss of support for runtime-variable key sizes. You could argue that the latter is not important and I am open to considering it, but you should argue the case in a separate issue, not here. If we are to follow this path, then we also should consider removing the new_from_slice(s) methods as a form of syntactic salt.

[bleichenbacher-daniel]

Another place where I'm having problems are AEAD modes. I'm trying to write a generic method as follows

fn encrypt<C: Aead + AeadCore + KeyInit>(
    key: &[u8],
    nonce: &[u8],
    aad: &[u8],
    msg: &[u8]) -> Result<Vec<u8>, Error> {
  let cipher = C::new_from_slice(&key).expect("Could not initialize cipher");
  let nonce =  Nonce::<C>::from_slice(&nonce);
  let payload = Payload {msg:msg, aad:aad};
  return cipher.encrypt(&nonce, payload);
}

For most block ciphers and encryption mode this works fine. I.e. it is possible to make calls like

encrypt::<Ocb3::<Camellia128, U15, U16>>(...)

Here the algorithm parameters of the cipher are fully specified, since U15 and U16 describe the IV size and tag size. When I try the same with Serpent (or Twofish) then I don't see a way to specify the key sizes and as a consequence I can only use the default key sizes.