cipher.encrypt requires an excessive buffer for PKCS#7 padding.

[fortanix]

cipher.encrypt requires an excessive buffer for PKCS#7 padding.

[kitanic]

I found one more erroneous behavior about PKCS#7.

The decrypted result with padding should always be smaller than the ciphertext, but the storage buffer requires the size of the ciphertext + 16bytes.
The following code will result in error.

fn decode_test(ciphertext: &[u8], key: &[u8], iv: &[u8]) {
  let mut plain = Vec::new();

  plain.resize(ciphertext.len(), 0); // MUST: plain < cipher

  let mut cipher = Cipher::<Decryption, Traditional>::new(CipherId::Aes, CipherMode::CBC, (key.len() as u32) * 8)?;
  cipher.set_padding(CipherPadding::Pkcs7)?;
  let cipher = cipher.set_key_iv(key, iv)?;

  cipher.decrypt(ciphertext, &mut plain)?; // An error occurred here.
}

[jethrogb]

This requirement comes from MbedTLS

output The buffer for the output data. This must be able to hold at least ilen + block_size. This must not be the same buffer as input.

https://tls.mbed.org/api/cipher_8h.html#a1c249f6ee1a0d2c906927c7790c41dc5

[kitanic]

No. It is incorrect.

In practice, it don't need more buffer than necessary, and it can encrypt without any problem if you have enough space for just blocksize boundary.
Perhaps the mbedtls documentation is wrong(!).

This is verification code.
check.c.txt

[jethrogb]

I understand that logically in some cases not the full output buffer is accessed, but for memory safety we must follow the documented API. We can relax the requirements in the bindings if MbedTLS does.

[kitanic]

I agree it.
So my suggestion is to not do a prior buffer size check in the rust side implementation. mbedtls should receive olen during encryption/decryption to check for buffer size and return MBEDTLS_ERR_CIPHER_FULL_BLOCK_EXPECTED if necessary. Therefore, there is no risk of crashing if you pass an undersized buffer.

Wrappers should not limit native behavior, and should follow its error codes.