CBC mode refactor/improvements #124
Conversation
The previous implementation of CBC (not yet removed) takes as input a keysize and an encrypt/decrypt function, and hardcodes the IV and block size to 128 bits. Instead, we'd like to make use of CipherInterface, which makes it easier to instantiate CBC with different block ciphers (and without any hardcoded assumptions on the block size). Removal of the old implementation and updating module-level comments is reserved for future commits
Further tests the new CipherInterface implementation of CBC by instantiating it with AES128. The instantiation can be imported from: Primitive::Symmetric::Cipher::Block::Instantiations::AES128_CBC
Now that we've established Modes/CBC.cry, Instantiations/AES128_CBC.cry, and Tests/TestAES_CBC.cry as the CBC implementation, instantiation, and testing, we can remove the old implementations.
Comment updated to follow format set throughout cryptol-specs.
| /* | ||
| * Cipher Block Chaining mode of operation, as defined in [NIST-SP-800-38A], Section 6.2. | ||
| * | ||
| * ⚠️ Warning ⚠️: CBC mode requires that the initialization vector (IV) is generated "unpredictably". |
There was a problem hiding this comment.
CTR has a warning about not duplicating counters, so I figured I'd leave a warning about having unpredictable IVs here. Is that a good place to document such requirements?
There was a problem hiding this comment.
Yeah, right now I'm in "throw warnings everywhere" mode, since there's no canonical approach. At the top of the file I like to list any differences we have with the spec (e.g. things it requires and we don't do, assumptions it makes that we can't satisfy, methods we've skipped for some reason etc.) with a flag for any that are security-critical, like this. I also like having them on the functions itself, like you did below.
staslyakhov
force-pushed
the
cbc-cipher-interface
branch
from
44a35c8
to
2f56503
Compare
There was a problem hiding this comment.
This looks very nice, thanks Stan!
My only blocking comment is to look at the naming and capitalization in encrypt/decrypt. Everything else is suggestions and context for ya.
Another resource you may be interested in is the reviewing guide I recently added to the wiki. Didn't think to share it with you before, but I suppose things that reviewers should look for are also things that contributors should keep in mind.
| /* | ||
| * Cipher Block Chaining mode of operation, as defined in [NIST-SP-800-38A], Section 6.2. | ||
| * | ||
| * ⚠️ Warning ⚠️: CBC mode requires that the initialization vector (IV) is generated "unpredictably". |
There was a problem hiding this comment.
Yeah, right now I'm in "throw warnings everywhere" mode, since there's no canonical approach. At the top of the file I like to list any differences we have with the spec (e.g. things it requires and we don't do, assumptions it makes that we can't satisfy, methods we've skipped for some reason etc.) with a flag for any that are security-critical, like this. I also like having them on the functions itself, like you did below.
* Mention Appendix D that discusses what can go wrong if the IV is predictable. * Emphasize that cryptol cannot check that IVs were generated correctly.
* Match variable capitalization
* Use C_j_1 instead of c' to refer to $C_{j-1}$
|
Oh, one final request: please update #80 to
|
Thanks for the review! I updated the issue to separate the tasks for different modes and mark the CBC refactor as completed. |
Addresses, but does not complete, #80 : CBC refactor and generalization.
Follow the structure established in #83 for CTR, and applies it to CBC mode.
CipherInterface)