Skip to content

Conversation

@ismaelsadeeq
Copy link
Member

This work is an attempt to continue from #14 by implementing the solution mentioned in #14 (comment), using PKCS#7 padding.

Currently, the CBC mode of AES does not pad the plaintext before encrypting. As suggested in #14, the industry standard is to pad the plaintext, regardless of whether it is a multiple of the block size.

PKCS#7 ) is widely used in the industry and is considered secure, which is why I have implemented it here.

As mentioned in the issue:

I just realized that AES-CBC in practice also includes a padding scheme, and since that needs to be checked in constant time, it seems appropriate to have it implemented here.

Unfortunately, it doesn't quite match the API implemented here very well. The problem is that if the padding is incorrect, the decryption has to be rejected, which doesn't work well with a mutable state that allows you to decrypt initial blocks before checking the padding. In cases where the amount of data is large and you want to use it in a streaming fashion, there is little that can be done about that. However, I'm not sure there is a need for that. Instead, there could just be a function that takes a const AES<n>_ctx, an input, and an output byte array, and does the whole thing at once.

To approach this, I modified AES256_CBC_ctx to pass the ciphertext length. Then, the entire data is decrypted before checking whether the padding is correct.
The decryption function returns the plaintext including the padding, as well as the length of the plaintext with the padding removed, a boolean value whether the decryption was done correctly i.e padding is correct

This approach does not detect incorrect padding in constant time; the ciphertext has to be decrypted before checking the padding correctness, as if I understand correctly in CBC Mode you have to use the iv, the first block, to get the next iv, and do that continuously to reach the last block with the padding.

If there is a better approach, I would appreciate guidance on how to improve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant