xaes: initial implementation #612
Conversation
SergioBenitez
force-pushed
the
xaes
branch
3 times, most recently
from
90897dd
to
8c08e80
Compare
aes-gcm/src/xaes.rs
Outdated
| pub const P_MAX: u64 = 1 << 36; | ||
|
|
||
| /// Maximum length of associated data. | ||
| // pub const A_MAX: u64 = 1 << 61; |
There was a problem hiding this comment.
I left the code here to inspire this comment. :)
According to the C2SP spec, the associated data max size for XAES-256-GCM is 2EiB = 2^61 bytes. However the max size for AD for AES-GCM is 2^64 and yet it's been restricted to 2^36 by this crate, so specifying the "true" AD would have no affect. I didn't explore why the AD is limited in this manner, however, so I left the code there in hopes that that would be clarified.
aes-gcm/src/lib.rs
Outdated
| pub use aead::{self, AeadCore, AeadInPlace, Error, Key, KeyInit, KeySizeUser}; | ||
|
|
||
| #[cfg(feature = "aes")] | ||
| pub use aes; | ||
|
|
||
| #[cfg(feature = "aes")] |
There was a problem hiding this comment.
Maybe feature = "xaes" extending "aes" ?
aes-gcm/tests/xaes256gcm.rs
Outdated
| use common::TestVector; | ||
| use hex_literal::hex; | ||
|
|
||
| /// C2SP XAES-256-GCM test vectors |
There was a problem hiding this comment.
sidebar - would love if C2SP test vectors would be embedded in crate a-like of wycheproof-rs so test vectors can be updated independently to all associated impl's and no messing updating multiple places
aes-gcm/src/xaes.rs
Outdated
| // Kₓ = Kₘ || Kₙ = AES-256ₖ(M1 ⊕ K1) || AES-256ₖ(M2 ⊕ K1) | ||
| let mut key: Key<Aes256Gcm> = Array::default(); | ||
| let (km, kn) = key.split_ref_mut::<<KeySize as Div<U2>>::Output>(); | ||
| for i in 0..km.len() { |
There was a problem hiding this comment.
Would be good to comment why conditional km.len() is okay to potentially leak on all conditionals ?
There was a problem hiding this comment.
Not sure I follow. km.len() isn't a conditional. Perhaps I'm misunderstanding?
aes-gcm/src/xaes.rs
Outdated
|
|
||
| // If MSB₁(L) = 0 then K1 = L << 1 Else K1 = (L << 1) ⊕ 0¹²⁰10000111 | ||
| let mut msb = 0; | ||
| for i in (0..k1.len()).rev() { |
There was a problem hiding this comment.
Also probably good to document k1.len() conditional here
|
gave you few nits - take em if you like if you feel like it and if worried about non-ct anywhere: |
aes-gcm/src/xaes.rs
Outdated
| // M1 = 0x00 || 0x01 || X || 0x00 || N[:12] | ||
| let mut m1 = Block::default(); | ||
| m1[..4].copy_from_slice(&[0, 1, b'X', 0]); | ||
| m1[4..].copy_from_slice(n1); |
There was a problem hiding this comment.
conditional length on ? would be great to clarify maybe why perhaps
There was a problem hiding this comment.
I don't understand the comment. What is ??
aes-gcm/tests/common/mod.rs
Outdated
| @@ -4,7 +4,7 @@ | |||
| #[derive(Debug)] | |||
| pub struct TestVector<K: 'static> { | |||
| pub key: &'static K, | |||
| pub nonce: &'static [u8; 12], | |||
| pub nonce: &'static [u8], | |||
There was a problem hiding this comment.
Is there a reason to make this non-fixed length ?
There was a problem hiding this comment.
Now we have nonces of two different lengths, so this is needed to be able to reuse the structure.
aes-gcm/src/xaes.rs
Outdated
| let mut msb = 0; | ||
| for i in (0..k1.len()).rev() { | ||
| let new_msb = k1[i] >> 7; | ||
| k1[i] = (k1[i] << 1) | msb; |
There was a problem hiding this comment.
Should this be in subtle::Blackbox::new() ref https://github.com/dalek-cryptography/curve25519-dalek/pull/662/files
There was a problem hiding this comment.
I compiled this with various flags, and at least of the nightly as of 3 days, the assembly generated is identical and doesn't leak any secrets via timing side-channels. That may change with future compilers, of course.
Where precisely are you suggesting Blackbox be used? Could you provide a GitHub suggestion of the change, perhaps?
aes-gcm/src/xaes.rs
Outdated
| km[i] = m1[i] ^ self.k1[i]; | ||
| } | ||
| for i in 0..kn.len() { | ||
| kn[i] = m2[i] ^ self.k1[i]; |
There was a problem hiding this comment.
Also should these be in subtle::BlackBox
|
@newpavlov @tarcieri Any chance for a review here? |
|
@SergioBenitez sorry, I've been on vacation. I'll look at this soon. |
|
Checking in. Any chance to push this forward? |
|
@SergioBenitez haven't had a whole lot of free time lately for code review but I still hope to review it soon |
|
Sorry for the belated review. On #1 we had discussed an I am a bit wary including the construction in the However, I'd also note the construction in the spec is called |
SergioBenitez
force-pushed
the
xaes
branch
7 times, most recently
from
5908f6a
to
d3141c6
Compare
|
Sure! Went ahead and published a |
This is an initial implementation of XAES-256-GCM (re: #1) which passes the test vectors.
Would love a review, especially as it pertains to constant-time and zeroing (why isn't
zeroizeused to zero IVs?). I don't see an obvious constant-time byte-slice XOR in use elsewhere in Rust-Crypto, but please point to a canonical reference if possible. I also have not placed this behind any feature flags, yet. Finally, the primary structureXaesGcm256is not parameterized in any way. If it's desirable to parameterize it in a similar fashion toAesGcm, please let me know.