cbc_decrypt() panics if data.is_empty()

[ADannadata]

The real panic happens in the unpad() function.

Also the unpad() never checks padding bytes. If they are wrong you could

• prevent unnecessary aes decryption
• return padding error

In the closed issueyou wrote that you didn't use Result because

even when the method returns Ok, it does not mean the msg decrypted correctly.

But this will happen almost never if you check padding bytes.

Imho you'll better return Option from the unpad() and pass it up to a cbc_decrypt()'s caller

[keepsimple1]

Thanks for reporting the issue! I thought about it and opted to keep the API interface unchanged, and fix the panic directly by adding the missing check. PR is opened with the diff.

Btw, the semantics is described such in the doc comments:

    /// If decrypt encountered unexpected errors, the output is an empty Vec. If the cipher has
    /// the wrong key, the output will either be a wrong plaintext or an empty Vec.

I'm wondering, what's the drawback of returning an empty Vec for error cases in practice?

[ADannadata]

Empty string could be a valid plaintext. How can users differ this case from encryption with a wrong key?

unpad() with the Option would report about wrong key with probability that differs from 1 approximately 2^-16. also this approach can cover wrong IV in cbc_decrypt() that will make possible to evade panic at all.

[keepsimple1]

Empty string could be a valid plaintext. How can users differ this case from encryption with a wrong key?

In this crate's API, an empty string returned from cbc_decrypt() indicates an error. If the user considers an empty input as valid, then they have to handle that in their application's logic.

unpad() with the Option would report about wrong key with probability that differs from 1 approximately 2-16. also this approach can cover wrong IV in cbc_decrypt() that will make possible to evade panic at all.

IMO, a security library API should not report about wrong key (or wrong IV) because the caller might be malicious.

[ADannadata]

an empty string returned from cbc_decrypt() indicates an error

which is definitely some encryption error that means that "wrong key (or wrong IV)" was used. So you report about "wrong key (or wrong IV)" just in the different way.

If the user considers an empty input as valid, then they have to handle that in their application's logic.

If a cipher text is short enough(1 block) there is no way to distinguish between a valid empty plaintext and an error.

[keepsimple1]

If a cipher text is short enough(1 block) there is no way to distinguish between a valid empty plaintext and an error.

That's correct, and it's consistent with my intention. This is a different case from "empty input for cbc_decrypt". However, this is the same case as using a wrong key:

/// If the cipher has the wrong key, the output will either be a wrong plaintext or an empty Vec.

When a caller gets an output from cbc_decrypt, regardless it's empty output or not, they cannot automatically know if they got the correct plaintext unless they know their key is correct. (Or they look at the output data and know if it makes sense to their application).

[keepsimple1]

Also, I think we could add a new API that allows the user to validate the length of the cipher text before calling decryption:

/// Returns false if the cipher data is not properly padded, e.g. corrupted.
/// If the cipher text is empty, this will return false.
fn cbc_cipher_len_check(data: &[u8]) -> bool

Although the user could do such checks themselves, this function provides some convenience and its optional. In this way, we don't need to introduce a Result or Error type and change the existing API.

What do you think?

[keepsimple1]

I've posted a new Release (0.6.5) with the current fix for the panic. But I'm open to enhancements for the future.

[ADannadata]

Thank you for the new release. It's a pity that I've failed to convince you to include "all possible" information into the single cbc_decrypt() method. I still prefer such approach and also still afraid of any panic in the library.

[keepsimple1]

I've opened a discussion poll to see what users think about the question of returning Result from API such as cbc_decrypt(). I am closing this issue itself as the original panic is fixed. If there are new comments related to this issue, please feel free to re-open it, or create a new issue.