From 0f39a3f3b247d2ce8a691a879e57c862e07e84b9 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sat, 16 Oct 2021 21:05:25 +0000 Subject: [PATCH] [BREAKING CHANGE] drop use of tindercrypt, and use ring directly --- .changelogs/2021-10-16-issue299.md | 1 + Cargo.lock | 20 +- docs/src/sync-protocol.md | 27 ++- taskchampion/Cargo.toml | 2 +- taskchampion/src/server/config.rs | 2 +- taskchampion/src/server/remote/crypto.rs | 263 +++++++++++++++-------- taskchampion/src/server/remote/mod.rs | 35 +-- 7 files changed, 228 insertions(+), 122 deletions(-) create mode 100644 .changelogs/2021-10-16-issue299.md diff --git a/.changelogs/2021-10-16-issue299.md b/.changelogs/2021-10-16-issue299.md new file mode 100644 index 000000000..a74af24c5 --- /dev/null +++ b/.changelogs/2021-10-16-issue299.md @@ -0,0 +1 @@ +- The encryption format used for synchronization has changed incompatibly diff --git a/Cargo.lock b/Cargo.lock index 9ac25caa0..523501bb6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2253,12 +2253,6 @@ dependencies = [ "tempfile", ] -[[package]] -name = "protobuf" -version = "2.24.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "db50e77ae196458ccd3dc58a31ea1a90b0698ab1b7928d89f644c25d72070267" - [[package]] name = "pulldown-cmark" version = "0.7.2" @@ -2967,6 +2961,7 @@ dependencies = [ "log", "pretty_assertions", "proptest", + "ring", "rstest", "rusqlite", "serde", @@ -2975,7 +2970,6 @@ dependencies = [ "strum_macros 0.21.1", "tempfile", "thiserror", - "tindercrypt", "ureq", "uuid", ] @@ -3184,18 +3178,6 @@ dependencies = [ "syn", ] -[[package]] -name = "tindercrypt" -version = "0.2.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f34fd5cc8db265f27abf29e8a3cec5cc643ae9f3f9ae39f08a77dc4add375b6d" -dependencies = [ - "protobuf", - "rand 0.7.3", - "ring", - "thiserror", -] - [[package]] name = "tinyvec" version = "1.2.0" diff --git a/docs/src/sync-protocol.md b/docs/src/sync-protocol.md index 1f111312c..252b16bef 100644 --- a/docs/src/sync-protocol.md +++ b/docs/src/sync-protocol.md @@ -42,7 +42,32 @@ The fourth invariant prevents the server from discarding versions newer than the ### Encryption -TBD (#299) +The client configuration includes an encryption secret of arbitrary length and a clientId to identify itself. +This section describes how that information is used to encrypt and decrypt data sent to the server (versions and snapshots). + +#### Key Derivation + +The client derives the 32-byte encryption key from the configured encryption secret using PBKDF2 with HMAC-SHA256 and 100,000 iterations. +The salt is the SHA256 hash of the 16-byte form of the client key. + +#### Encryption + +The client uses [AEAD](https://commondatastorage.googleapis.com/chromium-boringssl-docs/aead.h.html), with algorithm AES_256_GCM. +Each encrypted payload has an associated version ID. +The 16-byte form of this UUID is used as the associated data (AAD) with the AEAD algorithm. +The client should generate a random nonce, noting that AEAD is _not secure_ if a nonce is used repeatedly for the same key. + +Although the AEAD specification distinguishes ciphertext and tags, for purposes of this specification they are considered concatenated into a single bytestring as in BoringSSL's `EVP_AEAD_CTX_seal`. + +#### Representation + +The final byte-stream is comprised of the following structure, with integers represented in network-endian format. + +* `version` (32-bit int) - format version (always 1) +* `nonce` (12 bytes) - encryption nonce +* `ciphertext` (remaining bytes) - ciphertext from sealing operation + +Future versions may have a completely different format. ### Version diff --git a/taskchampion/Cargo.toml b/taskchampion/Cargo.toml index b48ed4491..18caeaadf 100644 --- a/taskchampion/Cargo.toml +++ b/taskchampion/Cargo.toml @@ -19,12 +19,12 @@ anyhow = "1.0" thiserror = "1.0" ureq = "^2.1.0" log = "^0.4.14" -tindercrypt = { version = "^0.2.2", default-features = false } rusqlite = { version = "0.25", features = ["bundled"] } strum = "0.21" strum_macros = "0.21" flate2 = "1" byteorder = "1.0" +ring = "0.16" [dev-dependencies] proptest = "^1.0.0" diff --git a/taskchampion/src/server/config.rs b/taskchampion/src/server/config.rs index efc444ed8..68ed00b68 100644 --- a/taskchampion/src/server/config.rs +++ b/taskchampion/src/server/config.rs @@ -33,7 +33,7 @@ impl ServerConfig { origin, client_key, encryption_secret, - } => Box::new(RemoteServer::new(origin, client_key, encryption_secret)), + } => Box::new(RemoteServer::new(origin, client_key, encryption_secret)?), }) } } diff --git a/taskchampion/src/server/remote/crypto.rs b/taskchampion/src/server/remote/crypto.rs index 65bdddbfa..45e2adac8 100644 --- a/taskchampion/src/server/remote/crypto.rs +++ b/taskchampion/src/server/remote/crypto.rs @@ -1,9 +1,107 @@ -use crate::server::HistorySegment; +/// This module implements the encryption specified in the sync-protocol +/// document. use byteorder::{NetworkEndian, ReadBytesExt, WriteBytesExt}; +use ring::{aead, digest, pbkdf2, rand, rand::SecureRandom}; use std::io::{Cursor, Read}; -use tindercrypt::cryptors::RingCryptor; use uuid::Uuid; +const PBKDF2_ITERATIONS: u32 = 100000; +const ENVELOPE_VERSION: u32 = 1; + +/// An Cryptor stores a secret and allows sealing and unsealing. It derives a key from the secret, +/// which takes a nontrivial amount of time, so it should be created once and re-used for the given +/// client_key. +pub(super) struct Cryptor { + key: aead::LessSafeKey, + rng: rand::SystemRandom, +} + +impl Cryptor { + pub(super) fn new(client_key: Uuid, secret: &Secret) -> anyhow::Result { + Ok(Cryptor { + key: Self::derive_key(client_key, secret)?, + rng: rand::SystemRandom::new(), + }) + } + + /// Derive a key as specified for version 1. Note that this may take 10s of ms. + fn derive_key(client_key: Uuid, secret: &Secret) -> anyhow::Result { + let salt = digest::digest(&digest::SHA256, client_key.as_bytes()); + + let mut key_bytes = vec![0u8; ring::aead::AES_256_GCM.key_len()]; + pbkdf2::derive( + pbkdf2::PBKDF2_HMAC_SHA256, + std::num::NonZeroU32::new(PBKDF2_ITERATIONS).unwrap(), + salt.as_ref(), + secret.as_ref(), + &mut key_bytes, + ); + + let unbound_key = ring::aead::UnboundKey::new(&ring::aead::AES_256_GCM, &key_bytes) + .map_err(|_| anyhow::anyhow!("error while creating AEAD key"))?; + Ok(ring::aead::LessSafeKey::new(unbound_key)) + } + + /// Encrypt the given payload. + pub(super) fn seal(&self, payload: Unsealed) -> anyhow::Result { + let Unsealed { + version_id, + mut payload, + } = payload; + + let mut nonce_buf = [0u8; aead::NONCE_LEN]; + self.rng + .fill(&mut nonce_buf) + .map_err(|_| anyhow::anyhow!("error generating random nonce"))?; + let nonce = ring::aead::Nonce::assume_unique_for_key(nonce_buf); + + let aad = ring::aead::Aad::from(version_id.as_bytes()); + + let tag = self + .key + .seal_in_place_separate_tag(nonce, aad, &mut payload) + .map_err(|_| anyhow::anyhow!("error while sealing"))?; + payload.extend_from_slice(tag.as_ref()); + + let env = Envelope { + nonce: &nonce_buf, + payload: payload.as_ref(), + }; + + Ok(Sealed { + version_id, + payload: env.to_bytes(), + }) + } + + /// Decrypt the given payload, verifying it was created for the given version_id + pub(super) fn unseal(&self, payload: Sealed) -> anyhow::Result { + let Sealed { + version_id, + payload, + } = payload; + + let env = Envelope::from_bytes(&payload)?; + + let mut nonce = [0u8; aead::NONCE_LEN]; + nonce.copy_from_slice(env.nonce); + let nonce = ring::aead::Nonce::assume_unique_for_key(nonce); + let aad = ring::aead::Aad::from(version_id.as_bytes()); + + let mut payload = env.payload.to_vec(); + let plaintext = self + .key + .open_in_place(nonce, aad, payload.as_mut()) + .map_err(|_| anyhow::anyhow!("error while creating AEAD key"))?; + + Ok(Unsealed { + version_id, + payload: plaintext.to_vec(), + }) + } +} + +/// Secret represents a secret key as used for encryption and decryption. pub(super) struct Secret(pub(super) Vec); impl From> for Secret { @@ -18,24 +116,17 @@ impl AsRef<[u8]> for Secret { } } -const PBKDF2_SALT_SIZE: usize = 32; -const NONCE_SIZE: usize = 12; -const ENVELOPE_VERSION: u32 = 1; - /// Envelope for the data stored on the server, containing the information /// required to decrypt. #[derive(Debug, PartialEq, Eq)] -struct Envelope { - kdf_iterations: u32, - kdf_salt: [u8; PBKDF2_SALT_SIZE], - nonce: [u8; NONCE_SIZE], +struct Envelope<'a> { + nonce: &'a [u8], + payload: &'a [u8], } -impl Envelope { - const SIZE: usize = 4 + 4 + PBKDF2_SALT_SIZE + NONCE_SIZE; - - fn from_bytes(buf: &[u8]) -> anyhow::Result { - if buf.len() < 4 { +impl<'a> Envelope<'a> { + fn from_bytes(buf: &'a [u8]) -> anyhow::Result> { + if buf.len() <= 4 + aead::NONCE_LEN { anyhow::bail!("envelope is too small"); } @@ -44,84 +135,61 @@ impl Envelope { if version != 1 { anyhow::bail!("unrecognized envelope version {}", version); } - if buf.len() != Envelope::SIZE { - anyhow::bail!("envelope size {} is not {}", buf.len(), Envelope::SIZE); - } - let kdf_iterations = rdr.read_u32::().unwrap(); - - let mut env = Envelope { - kdf_iterations, - kdf_salt: [0; PBKDF2_SALT_SIZE], - nonce: [0; NONCE_SIZE], - }; - env.kdf_salt.clone_from_slice(&buf[8..8 + PBKDF2_SALT_SIZE]); - env.nonce - .clone_from_slice(&buf[8 + PBKDF2_SALT_SIZE..8 + PBKDF2_SALT_SIZE + NONCE_SIZE]); - Ok(env) // TODO: test + Ok(Envelope { + nonce: &buf[4..4 + aead::NONCE_LEN], + payload: &buf[4 + aead::NONCE_LEN..], + }) } fn to_bytes(&self) -> Vec { - let mut buf = Vec::with_capacity(Envelope::SIZE); + let mut buf = Vec::with_capacity(4 + self.nonce.len() + self.payload.len()); buf.write_u32::(ENVELOPE_VERSION).unwrap(); - buf.write_u32::(self.kdf_iterations).unwrap(); - buf.extend_from_slice(&self.kdf_salt); - buf.extend_from_slice(&self.nonce); - buf // TODO: test + buf.extend_from_slice(self.nonce); + buf.extend_from_slice(self.payload); + buf } } -/// A cleartext payload with an attached version_id. The version_id is used to -/// validate the context of the payload. -pub(super) struct Cleartext { +/// A unsealed payload with an attached version_id. The version_id is used to +/// validate the context of the payload on unsealing. +pub(super) struct Unsealed { pub(super) version_id: Uuid, - pub(super) payload: HistorySegment, + pub(super) payload: Vec, } -impl Cleartext { - /// Seal the payload into its ciphertext - pub(super) fn seal(self, secret: &Secret) -> anyhow::Result { - let cryptor = RingCryptor::new().with_aad(self.version_id.as_bytes()); - let ciphertext = cryptor.seal_with_passphrase(secret.as_ref(), &self.payload)?; - Ok(Ciphertext(ciphertext)) - } +/// An encrypted payload +pub(super) struct Sealed { + pub(super) version_id: Uuid, + pub(super) payload: Vec, } -/// An ecrypted payload -pub(super) struct Ciphertext(pub(super) Vec); - -impl Ciphertext { +impl Sealed { pub(super) fn from_resp( resp: ureq::Response, + version_id: Uuid, content_type: &str, - ) -> Result { + ) -> Result { if resp.header("Content-Type") == Some(content_type) { let mut reader = resp.into_reader(); - let mut bytes = vec![]; - reader.read_to_end(&mut bytes)?; - Ok(Self(bytes)) + let mut payload = vec![]; + reader.read_to_end(&mut payload)?; + Ok(Self { + version_id, + payload, + }) } else { Err(anyhow::anyhow!( "Response did not have expected content-type" )) } } - - pub(super) fn open(self, secret: &Secret, version_id: Uuid) -> anyhow::Result { - let cryptor = RingCryptor::new().with_aad(version_id.as_bytes()); - let plaintext = cryptor.open(secret.as_ref(), &self.0)?; - - Ok(Cleartext { - version_id, - payload: plaintext, - }) - } } -impl AsRef<[u8]> for Ciphertext { +impl AsRef<[u8]> for Sealed { fn as_ref(&self) -> &[u8] { - self.0.as_ref() + self.payload.as_ref() } } @@ -133,9 +201,8 @@ mod test { #[test] fn envelope_round_trip() { let env = Envelope { - kdf_iterations: 100, - kdf_salt: [1; 32], - nonce: [2; 12], + nonce: &[2; 12], + payload: b"HELLO", }; let bytes = env.to_bytes(); @@ -147,48 +214,76 @@ mod test { fn round_trip() { let version_id = Uuid::new_v4(); let payload = b"HISTORY REPEATS ITSELF".to_vec(); - let secret = Secret(b"SEKRIT".to_vec()); - let cleartext = Cleartext { + let secret = Secret(b"SEKRIT".to_vec()); + let cryptor = Cryptor::new(Uuid::new_v4(), &secret).unwrap(); + + let unsealed = Unsealed { version_id, payload: payload.clone(), }; - let ciphertext = cleartext.seal(&secret).unwrap(); - let cleartext = ciphertext.open(&secret, version_id).unwrap(); + let sealed = cryptor.seal(unsealed).unwrap(); + let unsealed = cryptor.unseal(sealed).unwrap(); - assert_eq!(cleartext.payload, payload); - assert_eq!(cleartext.version_id, version_id); + assert_eq!(unsealed.payload, payload); + assert_eq!(unsealed.version_id, version_id); } #[test] fn round_trip_bad_key() { let version_id = Uuid::new_v4(); let payload = b"HISTORY REPEATS ITSELF".to_vec(); - let secret = Secret(b"SEKRIT".to_vec()); + let client_key = Uuid::new_v4(); - let cleartext = Cleartext { + let secret = Secret(b"SEKRIT".to_vec()); + let cryptor = Cryptor::new(client_key, &secret).unwrap(); + + let unsealed = Unsealed { version_id, payload: payload.clone(), }; - let ciphertext = cleartext.seal(&secret).unwrap(); + let sealed = cryptor.seal(unsealed).unwrap(); - let secret = Secret(b"BADSEKRIT".to_vec()); - assert!(ciphertext.open(&secret, version_id).is_err()); + let secret = Secret(b"DIFFERENT_SECRET".to_vec()); + let cryptor = Cryptor::new(client_key, &secret).unwrap(); + assert!(cryptor.unseal(sealed).is_err()); } #[test] fn round_trip_bad_version() { let version_id = Uuid::new_v4(); let payload = b"HISTORY REPEATS ITSELF".to_vec(); - let secret = Secret(b"SEKRIT".to_vec()); + let client_key = Uuid::new_v4(); - let cleartext = Cleartext { + let secret = Secret(b"SEKRIT".to_vec()); + let cryptor = Cryptor::new(client_key, &secret).unwrap(); + + let unsealed = Unsealed { version_id, payload: payload.clone(), }; - let ciphertext = cleartext.seal(&secret).unwrap(); + let mut sealed = cryptor.seal(unsealed).unwrap(); + sealed.version_id = Uuid::new_v4(); // change the version_id + assert!(cryptor.unseal(sealed).is_err()); + } - let bad_version_id = Uuid::new_v4(); - assert!(ciphertext.open(&secret, bad_version_id).is_err()); + #[test] + fn round_trip_bad_client_key() { + let version_id = Uuid::new_v4(); + let payload = b"HISTORY REPEATS ITSELF".to_vec(); + let client_key = Uuid::new_v4(); + + let secret = Secret(b"SEKRIT".to_vec()); + let cryptor = Cryptor::new(client_key, &secret).unwrap(); + + let unsealed = Unsealed { + version_id, + payload: payload.clone(), + }; + let sealed = cryptor.seal(unsealed).unwrap(); + + let client_key = Uuid::new_v4(); + let cryptor = Cryptor::new(client_key, &secret).unwrap(); + assert!(cryptor.unseal(sealed).is_err()); } } diff --git a/taskchampion/src/server/remote/mod.rs b/taskchampion/src/server/remote/mod.rs index 139f5dc9f..e9d29b1ea 100644 --- a/taskchampion/src/server/remote/mod.rs +++ b/taskchampion/src/server/remote/mod.rs @@ -6,12 +6,12 @@ use std::time::Duration; use uuid::Uuid; mod crypto; -use crypto::{Ciphertext, Cleartext, Secret}; +use crypto::{Cryptor, Sealed, Secret, Unsealed}; pub struct RemoteServer { origin: String, client_key: Uuid, - encryption_secret: Secret, + cryptor: Cryptor, agent: ureq::Agent, } @@ -28,16 +28,20 @@ impl RemoteServer { /// without a trailing slash, such as `https://tcsync.example.com`. Pass a client_key to /// identify this client to the server. Multiple replicas synchronizing the same task history /// should use the same client_key. - pub fn new(origin: String, client_key: Uuid, encryption_secret: Vec) -> RemoteServer { - RemoteServer { + pub fn new( + origin: String, + client_key: Uuid, + encryption_secret: Vec, + ) -> anyhow::Result { + Ok(RemoteServer { origin, client_key, - encryption_secret: encryption_secret.into(), + cryptor: Cryptor::new(client_key, &Secret(encryption_secret.to_vec()))?, agent: ureq::AgentBuilder::new() .timeout_connect(Duration::from_secs(10)) .timeout_read(Duration::from_secs(60)) .build(), - } + }) } } @@ -73,17 +77,17 @@ impl Server for RemoteServer { "{}/v1/client/add-version/{}", self.origin, parent_version_id ); - let cleartext = Cleartext { + let unsealed = Unsealed { version_id: parent_version_id, payload: history_segment, }; - let ciphertext = cleartext.seal(&self.encryption_secret)?; + let sealed = self.cryptor.seal(unsealed)?; match self .agent .post(&url) .set("Content-Type", HISTORY_SEGMENT_CONTENT_TYPE) .set("X-Client-Key", &self.client_key.to_string()) - .send_bytes(ciphertext.as_ref()) + .send_bytes(sealed.as_ref()) { Ok(resp) => { let version_id = get_uuid_header(&resp, "X-Version-Id")?; @@ -120,10 +124,9 @@ impl Server for RemoteServer { Ok(resp) => { let parent_version_id = get_uuid_header(&resp, "X-Parent-Version-Id")?; let version_id = get_uuid_header(&resp, "X-Version-Id")?; - let ciphertext = Ciphertext::from_resp(resp, HISTORY_SEGMENT_CONTENT_TYPE)?; - let history_segment = ciphertext - .open(&self.encryption_secret, parent_version_id)? - .payload; + let sealed = + Sealed::from_resp(resp, parent_version_id, HISTORY_SEGMENT_CONTENT_TYPE)?; + let history_segment = self.cryptor.unseal(sealed)?.payload; Ok(GetVersionResult::Version { version_id, parent_version_id, @@ -139,17 +142,17 @@ impl Server for RemoteServer { fn add_snapshot(&mut self, version_id: VersionId, snapshot: Snapshot) -> anyhow::Result<()> { let url = format!("{}/v1/client/add-snapshot/{}", self.origin, version_id); - let cleartext = Cleartext { + let unsealed = Unsealed { version_id, payload: snapshot, }; - let ciphertext = cleartext.seal(&self.encryption_secret)?; + let sealed = self.cryptor.seal(unsealed)?; Ok(self .agent .post(&url) .set("Content-Type", SNAPSHOT_CONTENT_TYPE) .set("X-Client-Key", &self.client_key.to_string()) - .send_bytes(ciphertext.as_ref()) + .send_bytes(sealed.as_ref()) .map(|_| ())?) } }