Encrypting binary strings of 32 bits does not work
AlexTMjugador opened this issue · 2 comments
Hi, thanks for your work on this crate! I've found it very useful, and I appreciate that the latest release improves performance and updates dependencies.
While updating fpe from v0.5.1 to v0.6.0 in one of my open-source projects, which uses it to encrypt 32-bit binary strings, I discovered thanks to unit tests that decrypting the 32-bit ciphertext with equal parameters did no longer yield the same plaintext back. In particular, I've found this code snippet to reproduce the problem, which was adapted from the docs.rs crate usage example:
use aes::Aes256;
use fpe::ff1::{BinaryNumeralString, FF1};
let key = [0; 32];
let radix = 2;
let pt = [0xab, 0xcd, 0xef, 0xaa]; // Add a extra byte
let ff = FF1::<Aes256>::new(&key, radix).unwrap();
let ct = ff.encrypt(&[], &BinaryNumeralString::from_bytes_le(&pt)).unwrap();
let p2 = ff.decrypt(&[], &ct).unwrap();
assert_eq!(p2.to_bytes_le(), pt); // The assertion failsRunning git bisect between HEAD (known "bad" commit, c4fb7b5) and the v0.5.1 release (known "good" commit, 92e5193) showed that the commit that introduced this regression was 4c3a5ab (made on PR #34). Previous commits of fpe made after v0.5.1 pass the test.
Obviously, reverting that commit is a fix for the regression, but doing that would also revert the performance improvements. Therefore, the ideal solution is to narrow down and fix what part of that rewrite is causing problems. I read the diff and tried to spot the mistake to fix it, but I couldn't do so in the time I allotted to the task, so I'm opening this issue instead.
I've managed to recreate this using proptest, and it turns out that the problem is specifically with even-length bytestrings. Amusingly, that was supposed to be the easy-to-handle case compared to odd-length 😂 (as the latter mean you have to handle nibbles).