Migrate from rust-openssl to ring for 'secure' functionality. #68

frewsxcv · 2017-01-08T22:43:25Z

This also changes the encrypted' strategy from AES 256 CBC with an HMAC to AEAD ChaCha20 Poly1305.

[breaking-change]

frewsxcv · 2017-01-08T22:45:47Z

Philipp91 · 2017-01-08T22:52:51Z

src/jar.rs

-        let mut signer = Signer::new(MessageDigest::sha1(), &pkey).unwrap();
-        signer.update(val.as_bytes()).unwrap();
-        signer.finish().unwrap()
+    fn dosign(key: &[u8], val: &str) -> digest::Digest {


nit: If you eliminate dosign(), you can replace the second of its only two usages with verify() instead of another sign()
https://briansmith.org/rustdoc/ring/hmac/fn.verify.html

Great idea! Implemented in cae8d23

Philipp91 · 2017-01-08T22:54:52Z

src/jar.rs

-    fn encrypt_data(key: &[u8], val: &str) -> String {
-        let iv = random_iv();
-        let iv_str = iv.to_base64(STANDARD);
+        assert_eq!(key.len(), ENCRYPTION_KEY_LEN);


I think ring could benefit from a helper function that handles lines 443-464 in a single call.

For reference, here are the lines @Philipp91 mentioned.

cc @briansmith in case you find this helpful.

https://github.com/alexcrichton/cookie-rs/pull/68/files#r95091104

…an make my own decisions

SergioBenitez · 2017-01-09T16:05:32Z

src/jar.rs

+
+    #[cfg(feature = "secure")]
+    fn _new(secret: &[u8]) -> CookieJar<'static> {
+        let (key256, key512) = secure::generate_keys(secret);


This generate_keys call does two 10k PBKDF2 each time a cookie jar is created. A cookie jar is typically created once per request based on the incoming headers. As such, this is an unacceptable performance regression.

Why do we need to generate keys in this manner? The typical way to do this is to use SHA256/512 to pad the randomness coming in, or demand randomness of a certain length so that it suffices for use as the key. For instance, you could require 512 bits as an input.

If secret is a random value already then HKDF (ring::hkdf) should be used to derive the two keys if you insist on still using HMAC for the authentication-only case.

If secret is a password then something like PBKDF2 needs to be used to stretch the key material once. Then use HKDF to derive the two keys.

Regarding the expensive operation, @sfackler had a suggestion to design a struct Secret and have it be constructed once where it does the PBKDF2 during construction. Then that structure can be passed into CookieJar whenever secrets are needed.

To be honest, PBKDF2 doesn't make much sense in this application. PBKDF2 is for converting a password into a key. But nobody should be using a password for this stuff. instead, they should be randomly generating a key.

Once you have one key, you can derive multiple keys from it using a KDF such as ring::hkdf. This is much faster than PBKDF2 but still not so fast that you want to do it more than necessary. Thus, it is good to create an object for holding the outputs of HKDF.

However, if you go with my suggestion to just use a nonce-misuse-resistant AEAD for both encrypt-and-authenticate and for authenticate-only, then you only need one key, and you can skip using a KDF completely, if you are given a key.

And, again, don't support the mode where one is given a password instead of a key, if you can avoid it.

SergioBenitez · 2017-01-09T16:05:56Z

src/jar.rs

-#[cfg(not(feature = "secure"))]
-fn prepare_key(_key: &[u8]) -> () {
-    ()
+struct SecureKeys {


Why do we need two keys of different lengths?

The key size for AEAD ChaCha20 Poly1305 is 256 bits. The key size for HMAC SHA1 is 512 bits.

Sure, but why not have a 512-bit key and truncate to 256-bits for ChaCha? We're not gaining anything by having two keys here, as far as I can tell; they're both derived from one key.

I wasn't sure if truncating a 512-bit KDF-derived key would be secure, but people online are telling me it's fine. I'll do that then.

SergioBenitez · 2017-01-09T16:11:32Z

src/jar.rs

-            return None
+        let verification_key =
+            hmac::VerificationKey::new(SIGNING_ALGORITHM, key);
+        let is_valid_signature = hmac::verify(


It looks like this line and the following few could be chained into something like .ok().map().

It's possible I could do that, but I think the way I wrote it is easier to follow.

I disagree, especially because .ok().map() is very idiomatic.

Can you share a concrete example of what you're proposing?

hmac::verify(&verification_key, text.as_bytes(), &signature).ok() .map(move |_| { cookie.value = text.to_owned(); cookie });

The following also works:

if let Ok(_) = hmac::verify(&verification_key, text.as_bytes(), &signature) { cookie.value = text.to_owned(); Some(cookie) } else { None }

or even:

match hmac::verify(&verification_key, text.as_bytes(), &signature) { Ok(_) => { cookie.value = text.to_owned(); Some(cookie) } Err(_) => None }

None of those look marginally better to me, but I'll do it anyways.

SergioBenitez · 2017-01-09T16:13:15Z

src/jar.rs

+        // Prepare bytes to be sealed
+        let in_out_len = cookie.value.as_bytes().len() + overhead_len;
+        let mut in_out = vec![0; in_out_len];
+        in_out[..value_len].clone_from_slice(cookie.value.as_bytes());


Why is this clone and not copy?

Accident. Fixed in e60534d.

rwf2#68 (comment)

alexcrichton · 2017-01-10T07:38:45Z

Could the crypto stay the same? This is all modeled after Rails is which quite tried and true and I'd prefer to stick to that if possible.

dreid · 2017-01-10T16:22:41Z

@alexcrichton Are you actually concerned about compatibility with existing encrypted cookies? Or do you just want a more familiar set of algorithms?

sfackler · 2017-01-10T17:36:48Z

@alexcrichton I believe the main motivation here is the switch from OpenSSL to ring, which is a less painful dependency to deal with particularly on Windows.

However, ring doesn't support AES-CBC, but only AES-GCM and ChaCha20-Poly1305, so we'd need to switch to one of those. One nice property of both of these is that they're AEAD ciphers, meaning that they self-authenticate so we don't have to separately HMAC. They're both well regarded, and are actually the preferred ciphers in Mozilla's Server Side TLS recommendations: https://wiki.mozilla.org/Security/Server_Side_TLS.

@frewsxcv I think AES-256-GCM will be faster than ChaCha20 on hardware with AES-NI. I wonder if it'd make more sense to use that since ~all server hardware will support those?

briansmith

Note that I didn't look at this very carefully. My main advice is to wait until we have nonce-misuse-resistant encryption in ring before doing this.

In particular, I recommend using AES-GCM-SIV instead of either ChaCha20-Poly1305 and HMAC, once it's available.

briansmith · 2017-01-10T17:30:30Z

src/jar.rs

+        Ok(cookie)
+    }
+
+    pub fn generate_keys(secret: &[u8]) -> ([u8; 32], [u8; 64]) {


It would be better to return (aead::SIVSealingKey, hmac::SigningKey). Then you could use aead::seal_in_place()/hmac::sign() and open_in_place_with_own_key()/verify_with_own_key() for the verification.

briansmith · 2017-01-10T17:33:54Z

src/jar.rs

+
+    /// Algorithm used to encrypt the cookie value
+    static ENCRYPTION_ALGORITHM: &'static aead::Algorithm =
+        &aead::CHACHA20_POLY1305;


ChaCha20-Poly1305 isn't an encryption algorithm, it is an AEAD algorithm. In particular, it does encryption AND authentication.

ChaCha20-Poly1305 isn't the best choice for this application because 96-bit random nonces aren't a good choice. It would be better to use the upcoming SIV mode AEADs in ring or XChaCha20 (if we decide to add that). The SIV stuff isn't open source in ring yet, but AES-GCM-SIV probably will become open source "soon."

briansmith · 2017-01-10T17:35:49Z

src/jar.rs

    use self::rustc_serialize::base64::{ToBase64, FromBase64, STANDARD};

-    pub const MIN_KEY_LEN: usize = 32;
+    /// Algorithm used to sign the cookie value
+    static SIGNING_ALGORITHM: &'static digest::Algorithm = &digest::SHA1;


SHA-1 Isn't a signing/authentication algorithm. HMAC-SHA1 is.

It is not necessary to use HMAC for the authentication-without-encryption case. You can use the AEAD for authentication without encryption by passing [] as the plaintext/ciphertext and passing the data to be signed/authenticated in the ad parameter. For ChaCha20-Poly1305, this would result in authentication using Poly1305, for example.

briansmith · 2017-01-10T17:40:21Z

src/jar.rs

-
-        let actual = match val.from_base64() {
-            Ok(actual) => actual, Err(_) => return None
+    pub fn design_and_decrypt(key: &[u8], mut cookie: Cookie)


It took me a bit to realize that "design" means "de-sign." The English word for what this does is "authenticate." Also, "open" == "authenticate and decrypt" and "seal" == "encrypt and sign."

briansmith · 2017-01-10T17:43:16Z

src/jar.rs

+    /// Separator between sealed cookie value and nonce
+    static SEALED_NONCE_SEPARATOR: &'static str = "--";
+    /// Key length (in bytes) used for encryption
+    const ENCRYPTION_KEY_LEN: usize = 256 / 8;


You could use ENCRYPTION_ALGORITHM.key_len() for this.

briansmith · 2017-01-10T17:48:28Z

src/jar.rs

+
+    #[cfg(feature = "secure")]
+    fn _new(secret: &[u8]) -> CookieJar<'static> {
+        let (key256, key512) = secure::generate_keys(secret);


If secret is a random value already then HKDF (ring::hkdf) should be used to derive the two keys if you insist on still using HMAC for the authentication-only case.

If secret is a password then something like PBKDF2 needs to be used to stretch the key material once. Then use HKDF to derive the two keys.

alexcrichton · 2017-01-10T18:44:47Z

@dreid oh nah I'm not worried about backcompat here, just the crypto!

@sfackler yeah I'm all for moving to ring as the OpenSSL dependency is a bummer.

My main concern here is that I'm not a cryptographer by any means and I'm hesitant to change from the "accepted practice" of what Rails does which presumably has been tried, true, and vetted for quite awhile. That being said, though, the crypto here is relatively simple IIRC and I'd be open to changing of course!

Thanks for the review help @sfackler and @briansmith!

mjc-gh · 2017-01-11T15:41:24Z

Hello, I just wanted to pipe in let everyone here know that I wrote a crate called message_verifier which providers two main structs, Verifier and Encryptor, which aim to be compatible with ActiveSupport::MessageVerifier and ActiveSupport::MessageEncryptor.

My next step was to build a small crate for parsing Rails' session cookies. I plan on writing some blog posts about building Rust micro web services that work with Rails applications. Perhaps message_verifier can be integrated with this crate to provide both signed and/or encrypted cookies? By default, Rails' session cookies are both signed and encrypted.

EDIT: Here is a link to the message_verifier repo: https://github.com/mikeycgto/message_verifier

frewsxcv · 2017-01-12T02:46:15Z

Thank you everyone for your feedback ❤️

I'm a bit busy for a few days, but I'll try to get around to addressing all the comments this weekend

briansmith · 2017-01-13T10:40:56Z

My main advice is to wait until we have nonce-misuse-resistant encryption in ring before doing this.

See in particular briansmith/ring#411, briansmith/ring#412, and briansmith/ring#413.

alexcrichton · 2017-01-13T18:03:48Z

Thanks @briansmith!

frewsxcv · 2017-01-16T18:24:05Z

My main advice is to wait until we have nonce-misuse-resistant encryption in ring before doing this.

See in particular briansmith/ring#411, briansmith/ring#412, and briansmith/ring#413.

I'm still interested in seeing the ring migration happen, but considering these comments and my lack of free time right now, I'm not planning on making any changes to this PR for the foreseeable future.

Migrate from rust-openssl to *ring* for 'secure' functionality.

d22fdaf

Philipp91 reviewed Jan 8, 2017

View reviewed changes

fixup! fix compile errors

6b0794a

frewsxcv mentioned this pull request Jan 8, 2017

Use an AEAD cipher instead of AES-CBC + HMAC-SHA1 #50

Closed

frewsxcv added 3 commits January 8, 2017 18:13

fixup! rm dosign, use hmac::verify

cae8d23

https://github.com/alexcrichton/cookie-rs/pull/68/files#r95091104

fixup! remove unnecessary helper functions because i'm an adult and c…

4b74a14

…an make my own decisions

fixup! whitespace

40ebcab

SergioBenitez reviewed Jan 9, 2017

View reviewed changes

frewsxcv added 2 commits January 9, 2017 14:05

fixup! clone -> copy

e60534d

rwf2#68 (comment)

fixup! write the same thing in a different way

8754414

SergioBenitez mentioned this pull request Jan 9, 2017

Encrypt and sign session cookies rwf2/Rocket#20

Closed

briansmith reviewed Jan 10, 2017

View reviewed changes

SergioBenitez mentioned this pull request Feb 15, 2017

Overhaul CookieJar: more efficient, more secure, more docs. #76

Merged

SergioBenitez mentioned this pull request Feb 25, 2017

Discussion: Could parsing and signing be split into two separate crates? #81

Closed

alexcrichton closed this in #76 Mar 1, 2017

frewsxcv deleted the rm-openssl branch March 1, 2017 17:31

Migrate from rust-openssl to *ring* for 'secure' functionality. #68

Migrate from rust-openssl to *ring* for 'secure' functionality. #68

Conversation

frewsxcv commented Jan 8, 2017 • edited Loading

frewsxcv commented Jan 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jan 10, 2017

dreid commented Jan 10, 2017

sfackler commented Jan 10, 2017 • edited Loading

briansmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jan 10, 2017

mjc-gh commented Jan 11, 2017 • edited Loading

frewsxcv commented Jan 12, 2017

briansmith commented Jan 13, 2017

alexcrichton commented Jan 13, 2017

frewsxcv commented Jan 16, 2017

Migrate from rust-openssl to ring for 'secure' functionality. #68

Migrate from rust-openssl to ring for 'secure' functionality. #68

frewsxcv commented Jan 8, 2017 •

edited

Loading

SergioBenitez Jan 9, 2017 •

edited

Loading

SergioBenitez Jan 9, 2017 •

edited

Loading

SergioBenitez Jan 9, 2017 •

edited

Loading

SergioBenitez Jan 9, 2017 •

edited

Loading

sfackler commented Jan 10, 2017 •

edited

Loading

briansmith Jan 10, 2017 •

edited

Loading

mjc-gh commented Jan 11, 2017 •

edited

Loading