-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deoxys: rewrite the internals to use inout #666
deoxys: rewrite the internals to use inout #666
Conversation
@@ -20,6 +20,7 @@ rust-version = "1.85" | |||
[dependencies] | |||
aead = { version = "0.6.0-rc.0", default-features = false } | |||
aes = { version = "=0.9.0-pre.3", features = ["hazmat"], default-features = false } | |||
inout = { version = "0.2.0-rc.4", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually this should drop and use aead::inout
70de0a6
to
787a1d3
Compare
@baloo can you add one or more tests that actually exercise |
c517aa5
to
3396c54
Compare
Yeah, found a misuse of |
3396c54
to
3549927
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR includes a fair amount of changes which are not directly relevant to the inout
support. Ideally, I would prefer to have them in a separate PR, but it's fine to merge it as-is, if you don't wan to bother with the split.
3549927
to
29fbddb
Compare
d6a9bd2
to
c7bc9a5
Compare
a79861d
to
c9c5606
Compare
c9c5606
to
9be4164
Compare
I think this one is ready to merge, unless you have anything other to say. |
block[0..data.len()].copy_from_slice(data); | ||
let mut data = tail; | ||
let index = data_blocks_len; | ||
if !data.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buffer_len % 16 == 0
is always true when !data.is_empty()
is false. I think it would be better to write this part like this:
if tail.is_empty() {
// Without incomplete last block
...
} else {
// With incomplete last block
...
}
Same for decrypt_inout
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the best I can do will look this:
diff --git a/deoxys/src/modes.rs b/deoxys/src/modes.rs
index 0f91ec36e0..e6ed2cd000 100644
--- a/deoxys/src/modes.rs
+++ b/deoxys/src/modes.rs
@@ -111,12 +111,11 @@
tweak[8] = nonce[7] << 4;
// Message authentication and encryption
- if !buffer.is_empty() {
+ let (data_blocks, mut tail) = buffer.into_chunks();
+ let data_blocks_len = data_blocks.len();
+ if !data_blocks.is_empty() || !tail.is_empty() {
tweak[0] = (tweak[0] & 0xf) | TWEAK_M;
- let (data_blocks, tail) = buffer.into_chunks();
- let data_blocks_len = data_blocks.len();
-
for (index, data) in data_blocks.into_iter().enumerate() {
// Copy block number
let tmp = tweak[8] & 0xf0;
@@ -130,9 +129,8 @@
B::encrypt_inout(data, &tweak, subkeys);
}
- let mut data = tail;
let index = data_blocks_len;
- if !data.is_empty() {
+ if !tail.is_empty() {
// Last block, incomplete
// Copy block number
@@ -144,9 +142,9 @@
tweak[0] = (tweak[0] & 0xf) | TWEAK_M_LAST;
let mut block = Block::default();
- block[0..data.len()].copy_from_slice(data.get_in());
+ block[0..tail.len()].copy_from_slice(tail.get_in());
- block[data.len()] = 0x80;
+ block[tail.len()] = 0x80;
for (c, d) in checksum.iter_mut().zip(block.iter()) {
*c ^= d;
@@ -157,24 +155,23 @@
// Last block encryption
B::encrypt_inout((&mut block).into(), &tweak, subkeys);
- data.xor_in2out((block[..data.len()]).into());
-
- // Tag computing.
- tweak[0] = (tweak[0] & 0xf) | TWEAK_CHKSUM;
-
- let tmp = tweak[8] & 0xf0;
- tweak[8..].copy_from_slice(&((index + 1) as u64).to_be_bytes());
- tweak[8] = (tweak[8] & 0xf) | tmp;
-
- B::encrypt_inout((&mut checksum).into(), tweak.as_ref(), subkeys);
-
- for (t, c) in tag.iter_mut().zip(checksum.iter()) {
- *t ^= c;
- }
+ tail.xor_in2out((block[..tail.len()]).into());
}
}
- if buffer_len % 16 == 0 {
+ if !tail.is_empty() {
+ tweak[0] = (tweak[0] & 0xf) | TWEAK_CHKSUM;
+
+ let tmp = tweak[8] & 0xf0;
+ tweak[8..].copy_from_slice(&((data_blocks_len + 1) as u64).to_be_bytes());
+ tweak[8] = (tweak[8] & 0xf) | tmp;
+
+ B::encrypt_inout((&mut checksum).into(), tweak.as_ref(), subkeys);
+
+ for (t, c) in tag.iter_mut().zip(checksum.iter()) {
+ *t ^= c;
+ }
+ } else {
// Tag computing without last block
tweak[0] = (tweak[0] & 0xf) | TWEAK_TAG;
That makes the whole patch series a lot less readable in my opinion (even when ignoring whitespace).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we can do it in a different PR then.
This is to prepare the migration to
AeadInOut
, following RustCrypto/traits#1793Note: there is a strong chance this could actually use the StreamCipherCore api, but I couldn't not make it fit.