Cipher: IV must not be set in case ECB mode of operation is used

[mohamedasaker-arm]

In cipher.rs
psa_crypto_sys::psa_cipher_set_iv is called without checking if the operation requires setting IV or not.
In case ECB operations this function psa_crypto_sys::psa_cipher_set_iv should not be called.

Available approaches:

• Check operation mode and call the function if iv is required.
• Pass the IV as an Option and here the responsibility of the caller is to determine if the iv is required or not.
  if the user misused it then the function will propagate the error.

[ionut-arm]

See this conversation as well - it's somewhat frustrating because:

• there are 3 cases in relation to that input: 1) you pass an IV; 2) you don't pass an IV because it's not needed; 3) you don't pass an IV because you want one to be generated...
• we're trying to keep these operations #[no_std], so we can't just return a Vec for the new IV for case 3 above, and we can't easily combine the cases.

I think it's possible to just add Option around the IV and document that it's ONLY for case (2) above, but there's still risk for confusion. The other option - having the user pass some random reference to a buffer that we then ignore if it's ECB mode, seems kinda weird.

The other option that I could think of, similar to what I suggested in the conversation linked above, would be to add a new method, something like encrypt_no_iv, though that's confusing as well. It would be ideal in my perspective if we could have the IV as an Option, and if we get a None we either don't call any method (for ECB), or we generate and return the IV (for other modes), but I have no idea how we'd return the IV

[ionut-arm]

Perhaps an alternative would be that we implement an enum for those cases:

enum Iv {
    UserProvided(&[u8]),
    GeneratedByPsaCrypto(&mut [u8], &mut size),
    None,
}

(I know the variant names are fugly, couldn't think of something nicer off the top of my head)

[mohamedasaker-arm]

I think the last suggestion makes sense and covers the cases mentioned above.

[ionut-arm]

@egrimley-arm - as a follow-up from our conversation on your PR, would you find the option with the enum above reasonable?

[egrimley-arm]

@egrimley-arm - as a follow-up from our conversation on your PR, would you find the option with the enum above reasonable?

Yes, it seems reasonable, but I'd suggest these names for the enum: ProvidedByCaller, GeneratedByPsaCrypto, None.

(Does GeneratedByPsaCrypto work as follows? The caller provides a buffer and says how big it is. If it is not big enough then Error::BufferTooSmall is returned; otherwise the size is modified to tell the caller how much of the buffer was used. I worry a bit about how it would work if we had an operation that took serveral buffers: would the caller have to increase the size of all of the buffers before trying again? Or does the function modify size to say how much space it would need even when nothing has been written to the buffer? There are a number of possible protocols that could exist on top of the same function prototype, which is not ideal!)

[ionut-arm]

... ok, this sounds like something the Rust type system should really be able to help with, so we don't have to ever deal with the possibility of a wrong IV size in the functions. How about:

struct UnauthenticatedCipher {
    cipher: Cipher,
    iv: Some([u8; 32]), // or whatever the maximum possible size of the IV is
    iv_len: usize,
}

impl UnauthenticatedCipher {
    // checks the provided IV length against expected size
    pub fn new_with_iv(cipher: Cipher, iv: &[u8]) -> Result<Self> {...}

    // either sets the IV to `None`, or generates N random bytes of IV and uses that
    pub fn new_without_iv(cipher: Cipher) -> Result<Self> {...}
}

And in the functions we only take this new UnauthenticatedCipher instead (or even just a reference to it) of Cipher and iv separately.