Skip to content
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

added HCTR #55

Closed
wants to merge 4 commits into from
Closed

added HCTR #55

wants to merge 4 commits into from

Conversation

WildCryptoFox
Copy link

@WildCryptoFox WildCryptoFox commented Sep 16, 2019

Not quite ready for publishing. Ready for review!

  • Generic over the block cipher and AXU universal hash function.

  • Documentation.

  • Review.

  • Publish to crates.io under RustCrypto

Why is Aes + ctr about half the performance of aesni w\ the ctr feature?

Because the latter benefits from XMM registers for the counter, while the former is more general and not currently checking if it can use XMM registers to boost aes in this case.

@newpavlov
Copy link
Member

Thank you!

I am not familiar with this construct, so I will have to read about it first to review your PR.

Not all ADX universal hash functions are valid; there are at least two additional properties that must hold.

We probably should add marker trait(s) to describe those properties to the universal-hash crate. cc @tarcieri

Why is Aes + ctr about half the performance of aesni w\ the ctr feature?

Because aesni uses some platform-dependent optimizations to make code more SIMD friendly. Without it blocks have to be transferred back and forth between XMM and general purpose registers.

@WildCryptoFox
Copy link
Author

WildCryptoFox commented Sep 16, 2019

You're welcome and thank you for the quick response. I've been waiting for @tarcieri's polyval, which is complete and fills the needed AXU slot. HCTR as an SPRP is an MRAE when padded with zeros, otherwise length preserving. I'll be using it as my general MRAE, starting with a HORNET implementation.

Is there anything blocking the ctr crate from doing the same? I'm using the ctr crate at least because it is easier to reuse the same aes128 post-keying state over both the flat PRP and the CTR components to HCTR.

Aesni's Aes128Ctr takes a key and not an already instantiated Aes128. Adding that from_cipher fn would probably be sufficient, but then it'd be even more aes-specialized but I'd rather the HCTR impl be generic, not hardcoded.

@WildCryptoFox
Copy link
Author

Regarding the CI failure. Just missing flags for this test. RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=+sse2,+sse4.1,+aes,+ssse3" cargo bench

Should the crate use cfg-targets to automatically select the aes and polyval features?

@newpavlov
Copy link
Member

Is there anything blocking the ctr crate from doing the same?

Using the same approach may degrade performance for ciphers which do not use XMM registers for blocks processing and keep data in general purpose registers. Although it may be that bad, since those ciphers are probably will not be performant enough to begin with for several additional cycles to be noticeable (especially if users will enable SSE4.1 which has _mm_extract_epi64). I will need to do some experiments with it.

Should the crate use cfg-targets to automatically select the aes and polyval features?

For now I think you can simply disable benchmarks if the required target features are not enabled.

@WildCryptoFox
Copy link
Author

WildCryptoFox commented Sep 18, 2019

@newpavlov The blockcipher trait could have an associated boolean constant, defaulting to false. const USES_XMM_REGISTERS: bool = false; Edit: Confirmed to work in stable rust on the playground.

Disabling only the benchmark wouldn't be sufficient as the whole library needs to be built (or disabled). I have generalized it over BlockCipher, but until polyval implements the universal-hash trait; I can't have the library even build a generic implementation without adding polyval's insecure-soft feature, which could be a "default" but I really do not want to enable an insecure default.

@WildCryptoFox
Copy link
Author

Polyval's software implementation should be constant time eventually. This PR can block on the const-time polyval.

@tarcieri
Copy link
Member

It’s already constant time. I just need to do another release. But first I wanted to add runtime feature detection.

James McGlashan added 2 commits September 19, 2019 00:00
Not quite ready for publishing.

* [ ] Should be generic over the block cipher and AXU universal hash
function.

* [ ] Documentation... Not all AXU universal hash functions are valid;
there are at least two additional properties that must hold.

* [ ] Is anything else missing?

* [ ] Publish to crates.io under RustCrypto

Why is Aes + ctr about half the performance of aesni w\ the ctr feature?

Please confirm this crate belongs here.

HCTR is an SPRP but it builds on a block cipher and an AXU universal
hash function, it is a construction or cipher family; not a cipher
itself.
hctr/Cargo.toml Outdated Show resolved Hide resolved
@WildCryptoFox
Copy link
Author

@newpavlov The CI failure is in lazy_static only for the older Rust compiler. The CI is happy with my code.

The second commit generalizes over BlockCipher and UniversalHash. The API is marked as unstable until we have a TweakableBlockCipher trait. Once @tarcieri 's polyval PR is accepted, I'll patch the Cargo.toml to use the published polyval crate, then I'll mark this PR as "ready for review".

@WildCryptoFox WildCryptoFox marked this pull request as ready for review September 19, 2019 16:55
@WildCryptoFox
Copy link
Author

@tarcieri published polyval 0.1.0 which uses the UniversalHash trait and has no insecure-soft mode. @newpavlov HCTR version 0.0.0 is ready for crates.io if you're happy with it. The API will remain unstable until the TweakableBlockCipher trait is ready.

@WildCryptoFox WildCryptoFox changed the title [WIP] added HCTR added HCTR Sep 20, 2019
@WildCryptoFox
Copy link
Author

WildCryptoFox commented Sep 20, 2019

@newpavlov Re ctr performance. I've started a Ctr<C> which generalizes over the key size, attempts to improve on the counter and buffering logic, and accepts Borrow<C> such that Ctr<Aes128> doesn't need to clone the shareable BlockCipher.

@WildCryptoFox
Copy link
Author

WildCryptoFox commented Sep 21, 2019

Interleaving the second AXU with CTR doesn't appear to improve performance.

Interleaving two AXU passes (for sphinx) benefits both small and large messages.

Interleaving all three yields worse performance than just interleaving the hashes.

For the Sphinx client use case, the benefit is to interleave the AXUs across instances.

test hash + (ctr + hash) hash + ctr + hash
1K 2.46 us 2.94 us
16K 37.25 us 37.20 us
test hash + ctr + (hash + hash) hash + ctr + hash + hash
1K 2.97 us 3.38 us
16K 44.47 us 51.99 us
test hash + (ctr + hash + hash) hash + ctr + hash + hash
1K 3.13 us 3.35 us
16K 48.34 us 60.82 us

Disclaimer: Besides having >90% of my CPU available for the benchmarks the cpu frequency was unstable - I was re-running the benches until they were all consistently at 2500MHz. Future benchmarks may be more consistently measured at a much lower 790MHz. Do either of you have any benchmarking tips? Better ways to measure cpb? Edit: I've added cpb to criterion, though more stable it still shows noise.

@WildCryptoFox
Copy link
Author

hctr

@WildCryptoFox
Copy link
Author

@newpavlov Last commit makes this free of git dependencies. All depends are on crates.io. Just waiting on your review.

@tarcieri
Copy link
Member

@WildCryptoFox can you please rebase this? thank you!

@WildCryptoFox
Copy link
Author

WildCryptoFox commented May 25, 2020

@tarcieri I can do this, yes. However I'm still not sure if HCTR belongs in "block ciphers" or "aeads". It is a construction a la SIV but not restricted to AES or POLYVAL. They're merely recommended choices. A block cipher and AXU* like POLYVAL in; wide SPRP out. Padding alone implies an MRAE.

I can adjust the implementation to use the AEAD traits and keep the layering specialization separate.

* The AXU must also meet two properties defined in the paper for the security proofs (Section 3.3.)

Edit: Aead::TagSize would be variable; depending on a type parameter for the zero padding length. Edit 2: The documentation on TagSize should read "the length of the authentication tag".

@tarcieri
Copy link
Member

tarcieri commented May 26, 2020

Yes, I'd suggest reopening this PR in the AEADs repo.

It sounds like you'd want to have several generic parameters including cipher and tag size. Several of the AEAD ciphers already do this, as it were. See AesGcm as an example (which supports a generic nonce size).

@tarcieri tarcieri closed this May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants