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

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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ similar to Rails' cookie jar.
[features]
default = ["secure"]
serialize-rustc = ["rustc-serialize", "time/rustc-serialize"]
secure = ["openssl", "rustc-serialize"]
secure = ["ring", "rustc-serialize"]
serialize-serde = ["serde"]

[dependencies]
url = "1.0"
time = "0.1"
rustc-serialize = { version = "0.3", optional = true }
openssl = { version = "0.9.0", optional = true }
ring = { version = "0.6", optional = true }
serde = { version = "0.8", optional = true }

[dev-dependencies]
Expand Down
263 changes: 149 additions & 114 deletions src/jar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ use Cookie;
/// # Example
///
/// ```
/// # #![allow(unstable)]
/// use cookie::{Cookie, CookieJar};
///
/// let c = CookieJar::new(b"f8f9eaf1ecdedff5e5b749c58115441e");
Expand Down Expand Up @@ -54,31 +53,17 @@ type Read = fn(&Root, Cookie) -> Option<Cookie>;
type Write = fn(&Root, Cookie) -> Cookie;

#[cfg(feature = "secure")]
type SigningKey = Vec<u8>;
#[cfg(not(feature = "secure"))]
type SigningKey = ();

#[cfg(feature = "secure")]
fn prepare_key(key: &[u8]) -> Vec<u8> {
if key.len() >= secure::MIN_KEY_LEN {
key.to_vec()
} else {
// Using a SHA-256 hash to normalize key as Rails suggests.
// See https://github.com/rails/rails/blob/master/activesupport/lib/active_support/message_encryptor.rb
secure::prepare_key(key)
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need two keys of different lengths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

key256: [u8; 256 / 8],
key512: [u8; 512 / 8],
}

struct Root {
map: RefCell<HashMap<String, Cookie>>,
new_cookies: RefCell<HashSet<String>>,
removed_cookies: RefCell<HashSet<String>>,
_key: SigningKey,
#[cfg(feature = "secure")]
keys: SecureKeys,
}

/// Iterator over the cookies in a cookie jar
Expand All @@ -88,16 +73,36 @@ pub struct Iter<'a> {
}

impl<'a> CookieJar<'a> {
/// Creates a new empty cookie jar with the given signing key.
/// Creates a new empty cookie jar with the given secret.
///
/// The given key is used to sign cookies in the signed cookie jar.
pub fn new(key: &[u8]) -> CookieJar<'static> {
/// The given secret is used to generate keys which are used to sign
/// cookies in the signed cookie jar.
pub fn new(secret: &[u8]) -> CookieJar<'static> {
CookieJar::_new(secret)
}

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

@SergioBenitez SergioBenitez Jan 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CookieJar {
flavor: Flavor::Root(Root {
map: RefCell::new(HashMap::new()),
new_cookies: RefCell::new(HashSet::new()),
removed_cookies: RefCell::new(HashSet::new()),
keys: SecureKeys {
key256: key256,
key512: key512,
},
})
}
}
#[cfg(not(feature = "secure"))]
fn _new(_secret: &[u8]) -> CookieJar<'static> {
CookieJar {
flavor: Flavor::Root(Root {
map: RefCell::new(HashMap::new()),
new_cookies: RefCell::new(HashSet::new()),
removed_cookies: RefCell::new(HashSet::new()),
_key: prepare_key(key),
})
}
}
Expand Down Expand Up @@ -209,10 +214,10 @@ impl<'a> CookieJar<'a> {
};

fn design(root: &Root, cookie: Cookie) -> Option<Cookie> {
secure::design(&root._key, cookie)
secure::design(&root.keys.key512, cookie)
}
fn sign(root: &Root, cookie: Cookie) -> Cookie {
secure::sign(&root._key, cookie)
secure::sign(&root.keys.key512, cookie)
}
}

Expand Down Expand Up @@ -246,10 +251,10 @@ impl<'a> CookieJar<'a> {
})
};
fn read(root: &Root, cookie: Cookie) -> Option<Cookie> {
secure::design_and_decrypt(&root._key, cookie)
secure::design_and_decrypt(&root.keys.key256, cookie).ok()
}
fn write(root: &Root, cookie: Cookie) -> Cookie {
secure::encrypt_and_sign(&root._key, cookie)
secure::encrypt_and_sign(&root.keys.key256, cookie)
}
}

Expand Down Expand Up @@ -369,32 +374,42 @@ impl<'a> Iterator for Iter<'a> {

#[cfg(feature = "secure")]
mod secure {
extern crate openssl;
extern crate ring;
extern crate rustc_serialize;

use Cookie;
use self::openssl::{hash, memcmp, symm};
use self::openssl::pkey::PKey;
use self::openssl::sign::Signer;
use self::openssl::hash::MessageDigest;
use self::ring::{aead, digest, hmac, rand, pbkdf2};
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  2. 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.

/// Separator between cookie value and signature
static SIGNATURE_SEPARATOR: &'static str = "--";
/// Key length (in bytes) used for signing
const SIGNING_KEY_LEN: usize = 512 / 8;

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

@briansmith briansmith Jan 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  2. 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."

/// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use ENCRYPTION_ALGORITHM.key_len() for this.


/// Number of iterations for PBKDF2 when deriving keys
const PBKDF2_ITERATIONS: usize = 10_000;

// If a SHA1 HMAC is good enough for rails, it's probably good enough
// for us as well:
//
// https://github.com/rails/rails/blob/master/activesupport/lib
// /active_support/message_verifier.rb#L70
pub fn sign(key: &[u8], mut cookie: Cookie) -> Cookie {
let signature = dosign(key, &cookie.value);
cookie.value.push_str("--");
cookie.value.push_str(&signature.to_base64(STANDARD));
assert_eq!(key.len(), SIGNING_KEY_LEN);
let signing_key = hmac::SigningKey::new(SIGNING_ALGORITHM, key);
let signature = hmac::sign(&signing_key, cookie.value.as_bytes());
cookie.value.push_str(SIGNATURE_SEPARATOR);
cookie.value.push_str(&signature.as_ref().to_base64(STANDARD));
cookie
}

fn split_value(val: &str) -> Option<(&str, Vec<u8>)> {
let parts = val.split("--");
let parts = val.split(SIGNATURE_SEPARATOR);
let ext = match parts.last() {
Some(ext) => ext,
_ => return None,
Expand All @@ -410,96 +425,104 @@ mod secure {
}

pub fn design(key: &[u8], mut cookie: Cookie) -> Option<Cookie> {
assert_eq!(key.len(), SIGNING_KEY_LEN);
let signed_value = cookie.value;
let (text, signature) = match split_value(&signed_value) {
Some(pair) => pair, None => return None
};
cookie.value = text.to_owned();

let expected = dosign(key, text);
if expected.len() != signature.len() ||
!memcmp::eq(&expected, &signature) {
return None
let verification_key =
hmac::VerificationKey::new(SIGNING_ALGORITHM, key);
match hmac::verify(&verification_key, text.as_bytes(), &signature) {
Ok(_) => {
cookie.value = text.to_owned();
Some(cookie)
}
Err(_) => None
}
Some(cookie)
}

fn dosign(key: &[u8], val: &str) -> Vec<u8> {
let pkey = PKey::hmac(key).unwrap();
let mut signer = Signer::new(MessageDigest::sha1(), &pkey).unwrap();
signer.update(val.as_bytes()).unwrap();
signer.finish().unwrap()
}

// Implementation details were taken from Rails. See
// https://github.com/rails/rails/blob/master/activesupport/lib/active_support/message_encryptor.rb#L57
pub fn encrypt_and_sign(key: &[u8], mut cookie: Cookie) -> Cookie {
let encrypted_data = encrypt_data(key, &cookie.value);
cookie.value = encrypted_data;
sign(key, cookie)
}
assert_eq!(key.len(), ENCRYPTION_KEY_LEN);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, here are the lines @Philipp91 mentioned.

cc @briansmith in case you find this helpful.

let sealing_key = aead::SealingKey::new(ENCRYPTION_ALGORITHM, key)
.expect("could not create aead sealing key");
let value_len = cookie.value.as_bytes().len();
let overhead_len = ENCRYPTION_ALGORITHM.max_overhead_len();

// 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].copy_from_slice(cookie.value.as_bytes());

// Initialize nonce
let mut nonce = vec![0; ENCRYPTION_ALGORITHM.nonce_len()];
let system_random = rand::SystemRandom::new();
system_random.fill(&mut nonce)
.expect("could not generate random nonce");

// Seal the plaintext cookie value
let out_len = aead::seal_in_place(
&sealing_key, &nonce, &mut in_out, overhead_len, &[])
.expect("could not seal");
let sealed = &in_out[..out_len];

// Build the final cookie value, combining sealed and nonce
let mut encrypted = sealed.to_base64(STANDARD);
encrypted.push_str(SEALED_NONCE_SEPARATOR);
encrypted.push_str(&nonce.to_base64(STANDARD));
cookie.value = encrypted;

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

let mut encrypted_data = symm::encrypt(symm::Cipher::aes_256_cbc(),
&key[..MIN_KEY_LEN],
Some(&iv),
val.as_bytes()).unwrap()
.to_base64(STANDARD);

encrypted_data.push_str("--");
encrypted_data.push_str(&iv_str);
encrypted_data
}

pub fn design_and_decrypt(key: &[u8], cookie: Cookie) -> Option<Cookie> {
let mut cookie = match design(key, cookie) {
Some(cookie) => cookie,
None => return None
};

let decrypted_data = decrypt_data(key, &cookie.value)
.and_then(|data| String::from_utf8(data).ok());
match decrypted_data {
Some(val) => { cookie.value = val; Some(cookie) }
None => None
}
cookie
}

fn decrypt_data(key: &[u8], val: &str) -> Option<Vec<u8>> {
let (val, iv) = match split_value(val) {
Some(pair) => pair, None => return None
};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

-> Result<Cookie, ()>
{
assert_eq!(key.len(), ENCRYPTION_KEY_LEN);
let (mut in_out, nonce) = {
let mut parts =
cookie.value.splitn(2, SEALED_NONCE_SEPARATOR)
.filter_map(|n| n.from_base64().ok());
match (parts.next(), parts.next()) {
(Some(in_out), Some(nonce)) => (in_out, nonce),
(_, _) => return Err(()),
}
};

Some(symm::decrypt(symm::Cipher::aes_256_cbc(),
&key[..MIN_KEY_LEN],
Some(&iv),
&actual).unwrap())
}

fn random_iv() -> Vec<u8> {
let mut ret = vec![0; 16];
openssl::rand::rand_bytes(&mut ret).unwrap();
return ret
}

pub fn prepare_key(key: &[u8]) -> Vec<u8> {
hash::hash(MessageDigest::sha256(), key).unwrap()
let opening_key = aead::OpeningKey::new(ENCRYPTION_ALGORITHM, key)
.expect("could not create aead opening key");
let out_len = try!(
aead::open_in_place(&opening_key, &nonce, 0, &mut in_out, &[])
.map_err(|_| ()));
let decrypted = try!(
String::from_utf8(in_out[..out_len].into()).map_err(|_| ()));
cookie.value = decrypted;
Ok(cookie)
}

pub fn generate_keys(secret: &[u8]) -> ([u8; 32], [u8; 64]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let mut key256 = [0; 256 / 8];
let mut key512 = [0; 512 / 8];
pbkdf2::derive(
&pbkdf2::HMAC_SHA256, PBKDF2_ITERATIONS, &[], secret, &mut key256);
pbkdf2::derive(
&pbkdf2::HMAC_SHA512, PBKDF2_ITERATIONS, &[], secret, &mut key512);
(key256, key512)
}
}

#[cfg(test)]
mod test {
use {Cookie, CookieJar};

#[cfg(feature = "secure")]
const SHORT_KEY: &'static [u8] = b"foo";

const KEY: &'static [u8] = b"f8f9eaf1ecdedff5e5b749c58115441e";

#[cfg(feature = "secure")]
const LONG_KEY: &'static [u8] =
b"ff8f9eaf1ecdedff5e5b749c58115441ef8f9eaf1ecdedff5e5b749c58115441ef\
9eaf1ecdedff5e5b749c58115441e8f9eaf1ecdedff5e5b749c58115441eef8f9a";

#[test]
fn short_key() {
CookieJar::new(b"foo");
Expand Down Expand Up @@ -546,14 +569,26 @@ mod test {
#[test]
fn signed() {
let c = CookieJar::new(KEY);
secure_behaviour!(c, signed)
secure_behaviour!(c, signed);

let c = CookieJar::new(SHORT_KEY);
secure_behaviour!(c, signed);

let c = CookieJar::new(LONG_KEY);
secure_behaviour!(c, signed);
}

#[cfg(feature = "secure")]
#[test]
fn encrypted() {
let c = CookieJar::new(KEY);
secure_behaviour!(c, encrypted)
secure_behaviour!(c, encrypted);

let c = CookieJar::new(SHORT_KEY);
secure_behaviour!(c, encrypted);

let c = CookieJar::new(LONG_KEY);
secure_behaviour!(c, encrypted);
}

#[test]
Expand Down