briansmith / ring

Safe, fast, small crypto using Rust
Other
3.71k stars 701 forks source link

Add AES 256 and AES128, both in CBC mode #588

Open P-E-Meunier opened 6 years ago

P-E-Meunier commented 6 years ago

I don't know anything about the security implications, but these are widely used ciphers for SSH private keys. I'm currently using OpenSSL in thrussh-keys, but this is obviously suboptimal, since I'd love this crate to work seamlessly on Windows.

briansmith commented 6 years ago

First, what's your goal in supporting these deprecated cipher suites? ~Nobody should be using them any more.

Second, in general, we only add ciphers in combination with an authentication mode. What is the authentication mode you want to use for each of AES-256 and AES-128? Presumably some HMAC. Which ones?

P-E-Meunier commented 6 years ago

Oops, sorry, I thought I had explained more, but I realise I didn't.

So, OpenSSH keys are still encrypted using these, even with the latest version of ssh-keygen. Many people seem to encrypt their keys with just AES256, CBC mode, and no authentication code.

tcsc commented 6 years ago

I have a similar issue - trying to decrypt legacy data encrypted with AES256-CBC. Specifically, a bunch of arq backups - https://www.arqbackup.com/arq_data_format.txt.

briansmith commented 6 years ago

@tcsc Your use case is significantly different than @P-E-Meunier's because your application us not just -AES256-CBC but AES256-CBC-HMAC-SHA256. AES256-CBC-HMAC-SHA256 fits perfectly into ring's AEAD API so it is easy to do.

AES-CBC without any authentication requires a new API that's not in line with ring's original design goals and we need to come up with a new way of thinking about ring's design and goals to accomodate things like that. Thus it's harder to do because it isn't just about code.

briansmith commented 6 years ago

Strawman proposal:

Let's create AES_128_CBC_HMAC_SHA256, AES_256_CBC_HMAC_SHA256, and AES_256_CBC_HMAC_SHA384 "AEADs" where the key is the concatenation of the AES key and the HMAC key. (The AEAD API only supports one key as input.) We could create a helper function that can construct the concatenated key from the two parts.

To address the unauthenticated AES-CBC use case: 1. For decryption, an external project could create a wrapper API around this AES-CBC-HMAC API that generates an HMAC key, calculates an HMAC over the input, appends the HMAC value to the input, and opens the modified input using the right CBC_HMAC AEAD result. 2. For encryption, they'd generate a HMAC key, encrypt and authenticate the input, and then strip off the HMAC at the end. It would be ugly but at least this ugliness could live outside of ring instead of inside it; at least it'd be pretty straightforward.

tcsc commented 6 years ago

In general architectural terms your proposal seem sound, especially as it keeps you from having to support an ugly solution into the future.

Unfortunately I can't offer any opinion worth listening to regarding its cryptological soundness.

Thanks for the quick response!

hwchen commented 6 years ago

Hello,

Would the proposed features allow me to use ring for this?

I'm currently using rust-crypto, and am hoping to move away. Some issues:

Thanks!

briansmith commented 6 years ago

@hwchen, Yes, if the encryption is done first, and then the HMAC is computed over the encrypted data, then this would work for that

@tcsc, do you know what padding form the arq_data_format is using? it isn't documented in the file. I'm guessing it's probably pkcs7 style padding like @hwchen's format.

dabo8008 commented 5 years ago

What is the current state of this issue? I would like to have AES_CBC_256_HMAC_SHA512

akhilles commented 5 years ago

@briansmith Is there agreement on whether the following authenticated ciphers should be added: AES_128_CBC_HMAC_SHA256, AES_256_CBC_HMAC_SHA256, and AES_256_CBC_HMAC_SHA384? If so, I'd like to take a crack at it.

adrian-budau commented 4 years ago

Also interested in the current state of this. Have you had any progress @akhilles ? I'd also like it if we could have constant time decrypting, i.e. protection from Lucky13, also taking into account LuckyMinus20.

I'd be willing to give it a try as well, if no one is working on this issue at the moment.

doy commented 4 years ago

i am also interested in AES_256_CBC_HMAC_SHA256 - is this still a thing that we would like to add?

unseddd commented 4 years ago

Also have a use-case for AES_CBC_HMAC_SHA256 in olm-rs. If @akhilles is still MIA, or not working on this, I can begin work on it.

LinusU commented 9 months ago

I have a use case for this which is to implement the RAOP (AirPlay) protocol. I currently have an implementation that uses OpenSSL, but would love to switch that out for Ring:

https://github.com/LinusU/rust-raop-player/blob/5c451804c2798644779742ad461337ee69fc9b9f/src/crypto.rs

However, I don't think that it uses any authentication on top of the encryption, so I'm not sure that it's in line with how the Ring apis are designed 🤔

briansmith commented 9 months ago

However, I don't think that it uses any authentication on top of the encryption, so I'm not sure that it's in line with how the Ring apis are designed

It is OK to expose unauthenticated AES-CBC from ring. I suggest we start out by doing it 100% in Rust on top of aead::aes. Once we have that then we can evaluate whether to bring in any of the optimized assembly for AES-CBC.

The bigger issue is that ring::aead::aes only has AES encryption, not decryption. That's because AES-GCM (and the upcoming AES-CCM) only requires encryption. So we need to first add AES decryption to ring::aead::aes. I will work on this.