Add support for the AES256-GCM AEAD construction

[klittlepage]

This commit adds support for AES256-GCM. NB: libsodium limits its support
of AES256-GCM to x86/x86-64 processors with the AES-NI instruction set.
Compile time feature flags are used to hide GCM functionality for targets
lacking the requisite support. In the interest of maintaining parity with
the libsodium API an is_available function mapping to
crypto_aead_aes256gcm_is_available is unconditionally exported but
shouldn't be required given the compile-time check.

Signed-off-by: Kelly Littlepage kelly@onechronos.com

[coveralls]

Pull Request Test Coverage Report for Build 1383

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 6 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.2%) to 95.066%

---

Files with Coverage Reduction	New Missed Lines	%
crypto/aead/aead_macros.rs	6	96.08%

Totals
Change from base Build 1379:	-0.2%
Covered Lines:	3218
Relevant Lines:	3385

---

💛 - Coveralls

[klittlepage]

Hi, checking in on this. Is there any reason to not merge?

[kpp]

$ rg pclmulqdq -c /proc/cpuinfo
8
$ rg aes -c /proc/cpuinfo
8
$ cargo test aes
    Finished test [unoptimized + debuginfo] target(s) in 0.03s
     Running target/debug/deps/sodiumoxide-e35cd2b3f289092a

running 1 test
test crypto::aead::aes256gcm::test::test_is_available ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 237 filtered out

Which means I don't have access to aes256-gcm even if I have aes&pclmulqdq

[klittlepage]

RUSTFLAGS="-C target-cpu=native" cargo test aes

running 7 tests
test crypto::aead::aes256gcm::aes::test::test_vector_1 ... ok
test crypto::aead::aes256gcm::test::test_is_available ... ok
test crypto::aead::aes256gcm::aes::test_m::test_seal_open_detached_tamper ... ok
test crypto::aead::aes256gcm::aes::test_m::test_seal_open_tamper ... ok
test crypto::aead::aes256gcm::aes::test_m::test_seal_open_detached ... ok
test crypto::aead::aes256gcm::aes::test_m::test_seal_open ... ok
test crypto::aead::aes256gcm::aes::test_m::test_seal_open_detached_same ... ok

test result: ok. 7 passed; 0 failed; 0 ignored; 0 measured; 237 filtered out

Rust won't use specialized instructions in test profile builds unless you either target a specific CPU or test a release profile.

Those instructions exist in Westmere and beyond (~2009) so support on x86 is near universal.

[kpp]

It would be useful to add these env vars into CI to build your code. Also we can add rustflags to package metadata(https://docs.rs/about) so that docs.rs could render aes module in the docs instead of any(doc, isa_check).

Also I am curious whether we should add docs into README how to turn on aes. Should we point that this module is turned on only on AES-NI ISA.

[kpp]

Wouldn't -C target-cpu=native be enough?

[klittlepage]

To your earlier point doing it this way ensures that the docs will always be “expanded” regardless of what platform they’re built on. I could arbitrarily pick a target with AES/CMUL support but doing it this way documents intent. There’s an open discussion about how rust docs should handle conditional compilation (feature detection or otherwise) but this seems like the lesser of evils for now.

[kpp]

LGTM. Does anybody want to review this PR?

[adamreichold]

Maybe prefer the link that includes the navigation for context, e.g. https://doc.libsodium.org/secret-key_cryptography/aead/aes-256-gcm#limitations

[adamreichold]

I don't think this adds up, a rustup target, i.e. something listed by rustup target list, is a host tuple that is passed via --target to rustc whereas the possible options for the target CPU are given by rustc --print target-cpus. (It is also not a target in itself but just an alias for a collection of target features AFAIK.)

[klittlepage]

Yup, I didn't document that correctly. The new PR clarifies.

[adamreichold]

Do we need the any(doc, ...) arm here now that the flags for docs.rs are adjusted? (Running cargo doc should produce whatever your compiler flags will produce for the build, shouldn't it?)

[klittlepage]

Unfortunately we do still need it until https://github.com/rust-lang/cargo/issues/7677 is resolved.

The docs won't build without it.

[adamreichold]

I would suggest to avoid advocating use of the RUSTFLAGS environment variable here as in a production setting this should often be done via Cargo configuration instead. Maybe talk about rustc flags in a generic manner?

[adamreichold]

Compile time feature flags are used to hide GCM functionality for targets
lacking the requisite support.

@klittlepage One thing I do not fully understand is the interaction of rustc target features with how libsodium is compiled/does implementation selection here, i.e. doesn't libsodium do CPU feature detection at runtime, c.f. https://github.com/jedisct1/libsodium/blob/master/src/libsodium/sodium/runtime.c, and hence could have a hardware-backed AES implementation available even if the Rust code was not compiled to take advantage of these instructions? Meaning that I am not really convinced that hiding that API based on the compile-time target features is helpful as they have nothing to do with whether these instructions will be available at runtime and these bindings should probably be as flexible as the underlying implementation of libsodium is.

[adamreichold]

Is "rustoxide" maybe meant to be "sodiumoxide"?

[klittlepage]

Oops...

[klittlepage]

Compile time feature flags are used to hide GCM functionality for targets
lacking the requisite support.

@klittlepage One thing I do not fully understand is the interaction of rustc target features with how libsodium is compiled/does implementation selection here, i.e. doesn't libsodium do CPU feature detection at runtime, c.f. https://github.com/jedisct1/libsodium/blob/master/src/libsodium/sodium/runtime.c, and hence could have a hardware-backed AES implementation available even if Rust code was not compiled to take advantage of these instructions?

Yes, that's true. How the rust side of things is compiled has no impact on the compilation of libsodium and the libsodium API exports AES unconditionally.

Meaning that I am not really convinced that hiding that API based on the compile-time target features is helpful as they have nothing to do with whether these instructions will be available at runtime and these bindings should probably be as flexible as the underlying implementation of libsodium is.

Yes, and if the goal is to mirror libsodium that makes sense. However, there are plenty of examples where sodiumoxide improves on the safety of the libsodium API e.g. the use of strongly typed keys, nonces, and tags in AEAD. These approaches aren't as "flexible" as the underlying implementation of libsodium - and that's a good thing.

Putting AES functionality behind a compile time feature check limits the possibility of a foot gun (using AES on an architecture that doesn't support it) so I'm in favor of making the behavior opt-in. I added a feature flag "aes" for users who have read the caveats and want to force an unconditional build.

[kpcyrd]

I don't think testing cpu features at compile time is a good idea, instead this should be done at runtime. Binary distribution is very common and the cpu of the build system is almost guaranteed to be different from the system that's actually going to run it. A binary compiled on a cpu with support for aes instructions should be able to run on cpus without this instruction and vice versa. I've accidentally shipped broken updates to users in the past because the build system was feature-tested and the final binary assumed the instructions would be present without further runtime checks, causing SIGILL for some users.

Not testing this at compile-time is also important for reproducible builds, which relies on the build to be deterministic to allow independent verification of the binary on a system that isn't 100% identical.

I'm surprised libsodium doesn't have a fallback, does anybody know why? @jedisct1

[klittlepage]

I've accidentally shipped broken updates to users in the past because the build system was feature-tested and the final binary assumed the instructions would be present without further runtime checks, causing SIGILL for some users.

The assumption that a runtime check resolves that issue hinges on correct API usage. Libsodium doesn't gate the internals of AES with a runtime feature check - it relies on the caller calling init and crypto_aead_aes256gcm_is_available. The user has to know to do that. Moving the check into the relevant functions could make for significant overhead and doesn't really get you much aside from an unrecoverable error instead of a signal.

Here are the compile and run scenarios for conditional compilation:

1. runtime: has AES, target: emit AES => ✅
2. runtime: has AES, target: do not emit AES => no aead_aes256gcm module exported => user forced to read docs and resolve => ✅
3. runtime: does not have AES, target: do not emit AES => ✅
4. runtime: does not have AES, target: emit AES => problem that the user explicitly opted into when cross compiling incorrectly

Without conditional compilation 3) turns into 4) and is much more likely to happen on accident. At worst 2) forces the user to read the docs. That being viewed as a bad thing really doesn't bode well for correct use of crypto_aead_aes256gcm_is_available.

Not testing this at compile-time is also important for reproducible builds, which relies on the build to be deterministic to allow independent verification of the binary on a system that isn't 100% identical.

That's why - as @adamreichold noted - using rust's configuration system is the way to go. Reproducibility is always modulo the target/config. Said differently it's about purity - maintaining a consistent mapping of inputs to outputs. It's not about creating the same binary irrespective of the inputs. Your point that you still want to allow for verification of a binary on a non-identical system is valid and completely compatible with this model. Rust has a strong story around reproducibility specifically because the configuration system works to capture inputs that might otherwise be implicit or forgotten; we might as well use that system to build safer APIs.

I'm surprised libsodium doesn't have a fallback, does anybody know why? @jedisct1

AES in GCM is notoriously prone to timing side-channels in the computation of the Galois hash unless you have access to hardware primitives for constant time carry-less multiplication. There are software mitigants that are slower and more complex; the latter point is the real concern.

[kpcyrd]

The assumption that a runtime check resolves that issue hinges on correct API usage. Libsodium doesn't gate the internals of AES with a runtime feature check - it relies on the caller calling init and crypto_aead_aes256gcm_is_available. The user has to know to do that. Moving the check into the relevant functions could make for significant overhead and doesn't really get you much aside from an unrecoverable error instead of a signal.

Compile-time checks don't solve this. I'd argue there's (unfortunately) no way to expose crypto_aead_aes256gcm in a safe and performant interface due to the lack of a fallback and the only reasonable option is delegating the responsibility to check crypto_aead_aes256gcm_is_available to the developer consuming the sodiumoxide api.

Your argument dismisses the fact that binary distributions like Debian and Arch Linux exist, "the user" might not be the person writing the code, and the person compiling it might be yet another person. In the opensource world the binaries are deployed on a much broader set of cpus than in controlled, proprietary deployments, there is no concept of a "target" more specific than amd64/x86_64.

It's also going to cause reproducible build issues for real packages that depend on sodiumoxide today and are currently considered reproducible according to reproducible-builds.org standards, the build machine cpu is not considered a build input.

My best guess is having sodiumoxide::init return an Err(_) if the aes feature is enabled and crypto_aead_aes256gcm_is_available returns false at runtime. Calling sodiumoxide::init isn't mandatory right now though.

[adamreichold]

Yes, and if the goal is to mirror libsodium that makes sense. However, there are plenty of examples where sodiumoxide improves on the safety of the libsodium API e.g. the use of strongly typed keys, nonces, and tags in AEAD. These approaches aren't as "flexible" as the underlying implementation of libsodium - and that's a good thing.

I am very much on board with these bindings improving the safety beyond what libsodium itself offers, I just do not really see the target features helping here as:

• If we enable the target features and run on a CPU without them, we get SIGILL.
• If we rely purely on runtime checks and fail to perform them before calling the AES functions, we get SIGILL.

Considering that the person building the software is often not the same person that is running the software, the outcome of an incorrect assumption embedded in the build config and an incorrect/missing runtime check is the same: the program will crash.

Moving the check into the relevant functions could make for significant overhead and doesn't really get you much aside from an unrecoverable error instead of a signal.

Compile-time checks don't solve this. I'd argue there's (unfortunately) no way to expose crypto_aead_aes256gcm in a safe and performant interface due to the lack of a fallback and the only reasonable option is delegating the responsibility to check crypto_aead_aes256gcm_is_available to the developer consuming the sodiumoxide api.

Did we actually measure the overhead of e.g. using std::sync::Once to check crypto_aead_aes256gcm_is_available the first time before one of the crypto_aead_aes256gcm_* functions is called? (I think this should be safe even if init was not called as no CPU feature will have been detected and crypto_aead_aes256gcm_is_available should always return false.)

[adamreichold]

Did we actually measure the overhead of e.g. using std::sync::Once to check crypto_aead_aes256gcm_is_available the first time before one of the crypto_aead_aes256gcm_* functions is called? (I think this should be safe even if init was not called as no CPU feature will have been detected and crypto_aead_aes256gcm_is_available should always return false.)

@klittlepage I think we can have our cake and it eat it too: Instead of exposing free functions as an API, we could add a unit struct like pub struct Aes256Gcm; with a pub fn new() -> Self method which calls crypto_aead_aes256gcm_is_available (either panicking or making new return a Result instead). The actual API functions then become methods of this struct which does not impose any overhead with the zero-sized type only providing a type-level proof that the runtime check was performed.

Of course this could imply avoiding the aead_module! macro and rests on crypto_aead_aes256gcm_is_available returning false if init was never called, but it should provide a way to safely expose these functions without prohibitive runtime overhead.

EDIT: Following the signature of init something like

pub struct Aes256Gcm;

impl Aes256Gcm {
  pub fn new() -> Result<Self, ()> {
    if unsafe { ffi::crypto_aead_aes256gcm_is_available() } == 1 {
      Ok(Self)
    } else {
      Err(())
    }
  }
}

should work.

[adamreichold]

Minor nit on the trait impls of the unit struct, otherwise this looks good to me now!

[klittlepage]

@klittlepage I think we can have our cake and it eat it too: Instead of exposing free functions as an API, we could add a unit struct like pub struct Aes256Gcm; with a pub fn new() -> Self method which calls crypto_aead_aes256gcm_is_available (either panicking or making new return a Result instead).

If folks are open to AES having a slightly different API, this is by far the best way to do it. My earlier commit reflected an assumption that the API needed to remain consistent. I've updated the implementation and docs.

@kpcyrd to your earlier point that "Compile-time checks don't solve this" - just to clarify - I never said that they did. The argument for using them was simply that it requires someone (either the person building the package) or consuming the API to explicitly opt in to the behavior having read the docs. Your point that the value of that is lessened when different people build/package/consume the library is completely true, so the AES specific API really does seem like the best solution.

I ran a Criterion test across all of the AEAD primitives (violin plot attached). AES enjoys a slight performance lead at every payload size and a more significant reduction in variance; there's not a compelling performance reason to favor it over the stream ciphers.

[klittlepage]

@kpcyrd is anything else needed on my end?

[kpcyrd]

Great work!

[kpp]

bors: r+

[kpp]

@klittlepage rebase plz

[klittlepage]

@kpp done - thank you!

[kpp]

bors: r+

[bors]

Build succeeded:

• continuous-integration/appveyor/branch
• continuous-integration/travis-ci/push