From ddfb327292ddf4d716ddf932abf685d1c6a8c8c4 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Fri, 15 Oct 2021 02:56:46 +0000 Subject: [PATCH 1/6] WIP --- Cargo.lock | 1 + taskchampion/Cargo.toml | 1 + taskchampion/src/server/remote/crypto.rs | 70 +++++++++++++++++++++++- 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 10f51edf9..9ac25caa0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2961,6 +2961,7 @@ name = "taskchampion" version = "0.4.1" dependencies = [ "anyhow", + "byteorder", "chrono", "flate2", "log", diff --git a/taskchampion/Cargo.toml b/taskchampion/Cargo.toml index c5da9898c..b48ed4491 100644 --- a/taskchampion/Cargo.toml +++ b/taskchampion/Cargo.toml @@ -24,6 +24,7 @@ rusqlite = { version = "0.25", features = ["bundled"] } strum = "0.21" strum_macros = "0.21" flate2 = "1" +byteorder = "1.0" [dev-dependencies] proptest = "^1.0.0" diff --git a/taskchampion/src/server/remote/crypto.rs b/taskchampion/src/server/remote/crypto.rs index 40103f422..65bdddbfa 100644 --- a/taskchampion/src/server/remote/crypto.rs +++ b/taskchampion/src/server/remote/crypto.rs @@ -1,5 +1,6 @@ use crate::server::HistorySegment; -use std::io::Read; +use byteorder::{NetworkEndian, ReadBytesExt, WriteBytesExt}; +use std::io::{Cursor, Read}; use tindercrypt::cryptors::RingCryptor; use uuid::Uuid; @@ -17,6 +18,60 @@ 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], +} + +impl Envelope { + const SIZE: usize = 4 + 4 + PBKDF2_SALT_SIZE + NONCE_SIZE; + + fn from_bytes(buf: &[u8]) -> anyhow::Result { + if buf.len() < 4 { + anyhow::bail!("envelope is too small"); + } + + let mut rdr = Cursor::new(buf); + let version = rdr.read_u32::().unwrap(); + 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 + } + + fn to_bytes(&self) -> Vec { + let mut buf = Vec::with_capacity(Envelope::SIZE); + + 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 + } +} + /// A cleartext payload with an attached version_id. The version_id is used to /// validate the context of the payload. pub(super) struct Cleartext { @@ -75,6 +130,19 @@ mod test { use super::*; use pretty_assertions::assert_eq; + #[test] + fn envelope_round_trip() { + let env = Envelope { + kdf_iterations: 100, + kdf_salt: [1; 32], + nonce: [2; 12], + }; + + let bytes = env.to_bytes(); + let env2 = Envelope::from_bytes(&bytes).unwrap(); + assert_eq!(env, env2); + } + #[test] fn round_trip() { let version_id = Uuid::new_v4(); From 0f39a3f3b247d2ce8a691a879e57c862e07e84b9 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sat, 16 Oct 2021 21:05:25 +0000 Subject: [PATCH 2/6] [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(|_| ())?) } } From 97d1366b660334c09118e06611813193af8c6e50 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sat, 16 Oct 2021 22:37:28 +0000 Subject: [PATCH 3/6] move taskchampion::server::remote::crypto to taskchampion::server::crypto --- taskchampion/src/server/{remote => }/crypto.rs | 0 taskchampion/src/server/mod.rs | 1 + taskchampion/src/server/remote/mod.rs | 3 +-- 3 files changed, 2 insertions(+), 2 deletions(-) rename taskchampion/src/server/{remote => }/crypto.rs (100%) diff --git a/taskchampion/src/server/remote/crypto.rs b/taskchampion/src/server/crypto.rs similarity index 100% rename from taskchampion/src/server/remote/crypto.rs rename to taskchampion/src/server/crypto.rs diff --git a/taskchampion/src/server/mod.rs b/taskchampion/src/server/mod.rs index 68b5a713f..eb77b9bd3 100644 --- a/taskchampion/src/server/mod.rs +++ b/taskchampion/src/server/mod.rs @@ -12,6 +12,7 @@ However, users who wish to implement their own server interfaces can implement t pub(crate) mod test; mod config; +mod crypto; mod local; mod remote; mod types; diff --git a/taskchampion/src/server/remote/mod.rs b/taskchampion/src/server/remote/mod.rs index e9d29b1ea..8ddfa52ff 100644 --- a/taskchampion/src/server/remote/mod.rs +++ b/taskchampion/src/server/remote/mod.rs @@ -5,8 +5,7 @@ use crate::server::{ use std::time::Duration; use uuid::Uuid; -mod crypto; -use crypto::{Cryptor, Sealed, Secret, Unsealed}; +use super::crypto::{Cryptor, Sealed, Secret, Unsealed}; pub struct RemoteServer { origin: String, From 4300f7bddaa51b159730036e3d67e796209d2fc2 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 17 Oct 2021 17:36:30 -0400 Subject: [PATCH 4/6] use CHACHA20_POLY1305 instead of AES_256_GCM --- docs/src/sync-protocol.md | 2 +- taskchampion/src/server/crypto.rs | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/src/sync-protocol.md b/docs/src/sync-protocol.md index 252b16bef..4d86f0af6 100644 --- a/docs/src/sync-protocol.md +++ b/docs/src/sync-protocol.md @@ -52,7 +52,7 @@ 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. +The client uses [AEAD](https://commondatastorage.googleapis.com/chromium-boringssl-docs/aead.h.html), with algorithm CHACHA20_POLY1305. 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. diff --git a/taskchampion/src/server/crypto.rs b/taskchampion/src/server/crypto.rs index 45e2adac8..fe47f65d5 100644 --- a/taskchampion/src/server/crypto.rs +++ b/taskchampion/src/server/crypto.rs @@ -28,7 +28,7 @@ impl Cryptor { 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()]; + let mut key_bytes = vec![0u8; aead::CHACHA20_POLY1305.key_len()]; pbkdf2::derive( pbkdf2::PBKDF2_HMAC_SHA256, std::num::NonZeroU32::new(PBKDF2_ITERATIONS).unwrap(), @@ -37,9 +37,9 @@ impl Cryptor { &mut key_bytes, ); - let unbound_key = ring::aead::UnboundKey::new(&ring::aead::AES_256_GCM, &key_bytes) + let unbound_key = aead::UnboundKey::new(&aead::CHACHA20_POLY1305, &key_bytes) .map_err(|_| anyhow::anyhow!("error while creating AEAD key"))?; - Ok(ring::aead::LessSafeKey::new(unbound_key)) + Ok(aead::LessSafeKey::new(unbound_key)) } /// Encrypt the given payload. @@ -53,9 +53,9 @@ impl Cryptor { 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 nonce = aead::Nonce::assume_unique_for_key(nonce_buf); - let aad = ring::aead::Aad::from(version_id.as_bytes()); + let aad = aead::Aad::from(version_id.as_bytes()); let tag = self .key @@ -85,8 +85,8 @@ impl Cryptor { 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 nonce = aead::Nonce::assume_unique_for_key(nonce); + let aad = aead::Aad::from(version_id.as_bytes()); let mut payload = env.payload.to_vec(); let plaintext = self From 17f5521ea4e0d77869d4e4be99b0cf307f7c45f7 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 17 Oct 2021 18:06:16 -0400 Subject: [PATCH 5/6] add an app_id to the encryption AAD --- docs/src/sync-protocol.md | 18 +++++++--- taskchampion/src/server/crypto.rs | 59 +++++++++++++++++++++++-------- 2 files changed, 58 insertions(+), 19 deletions(-) diff --git a/docs/src/sync-protocol.md b/docs/src/sync-protocol.md index 4d86f0af6..0bb588383 100644 --- a/docs/src/sync-protocol.md +++ b/docs/src/sync-protocol.md @@ -53,21 +53,29 @@ 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 CHACHA20_POLY1305. -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. +AEAD supports additional authenticated data (AAD) which must be provided for both open and seal operations. +In this protocol, the AAD is always 17 bytes of the form: + * `app_id` (byte) - always 1 + * `version_id` (16 bytes) - 16-byte form of the version ID associated with this data + * for versions (AddVersion, GetChildVersion), the _parent_ version_id + * for snapshots (AddSnapshot, GetSnapshot), the snapshot version_id + +The `app_id` field is for future expansion to handle other, non-task data using this protocol. +Including it in the AAD ensures that such data cannot be confused with task data. + 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. +The final byte-stream is comprised of the following structure: -* `version` (32-bit int) - format version (always 1) +* `version` (byte) - format version (always 1) * `nonce` (12 bytes) - encryption nonce * `ciphertext` (remaining bytes) - ciphertext from sealing operation -Future versions may have a completely different format. +The `version` field identifies this data format, and future formats will have a value other than 1 in this position. ### Version diff --git a/taskchampion/src/server/crypto.rs b/taskchampion/src/server/crypto.rs index fe47f65d5..b2728acad 100644 --- a/taskchampion/src/server/crypto.rs +++ b/taskchampion/src/server/crypto.rs @@ -1,12 +1,13 @@ /// 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 std::io::Read; use uuid::Uuid; const PBKDF2_ITERATIONS: u32 = 100000; -const ENVELOPE_VERSION: u32 = 1; +const ENVELOPE_VERSION: u8 = 1; +const AAD_LEN: usize = 17; +const TASK_APP_ID: u8 = 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 @@ -55,7 +56,7 @@ impl Cryptor { .map_err(|_| anyhow::anyhow!("error generating random nonce"))?; let nonce = aead::Nonce::assume_unique_for_key(nonce_buf); - let aad = aead::Aad::from(version_id.as_bytes()); + let aad = self.make_aad(version_id); let tag = self .key @@ -86,7 +87,7 @@ impl Cryptor { let mut nonce = [0u8; aead::NONCE_LEN]; nonce.copy_from_slice(env.nonce); let nonce = aead::Nonce::assume_unique_for_key(nonce); - let aad = aead::Aad::from(version_id.as_bytes()); + let aad = self.make_aad(version_id); let mut payload = env.payload.to_vec(); let plaintext = self @@ -99,6 +100,13 @@ impl Cryptor { payload: plaintext.to_vec(), }) } + + fn make_aad(&self, version_id: Uuid) -> aead::Aad<[u8; AAD_LEN]> { + let mut aad = [0u8; AAD_LEN]; + aad[0] = TASK_APP_ID; + aad[1..].copy_from_slice(version_id.as_bytes()); + aead::Aad::from(aad) + } } /// Secret represents a secret key as used for encryption and decryption. @@ -126,26 +134,25 @@ struct Envelope<'a> { impl<'a> Envelope<'a> { fn from_bytes(buf: &'a [u8]) -> anyhow::Result> { - if buf.len() <= 4 + aead::NONCE_LEN { + if buf.len() <= 1 + aead::NONCE_LEN { anyhow::bail!("envelope is too small"); } - let mut rdr = Cursor::new(buf); - let version = rdr.read_u32::().unwrap(); - if version != 1 { - anyhow::bail!("unrecognized envelope version {}", version); + let version = buf[0]; + if version != ENVELOPE_VERSION { + anyhow::bail!("unrecognized encryption envelope version {}", version); } Ok(Envelope { - nonce: &buf[4..4 + aead::NONCE_LEN], - payload: &buf[4 + aead::NONCE_LEN..], + nonce: &buf[1..1 + aead::NONCE_LEN], + payload: &buf[1 + aead::NONCE_LEN..], }) } fn to_bytes(&self) -> Vec { - let mut buf = Vec::with_capacity(4 + self.nonce.len() + self.payload.len()); + let mut buf = Vec::with_capacity(1 + self.nonce.len() + self.payload.len()); - buf.write_u32::(ENVELOPE_VERSION).unwrap(); + buf.push(ENVELOPE_VERSION); buf.extend_from_slice(self.nonce); buf.extend_from_slice(self.payload); buf @@ -210,6 +217,30 @@ mod test { assert_eq!(env, env2); } + #[test] + fn envelope_bad_version() { + let env = Envelope { + nonce: &[2; 12], + payload: b"HELLO", + }; + + let mut bytes = env.to_bytes(); + bytes[0] = 99; + assert!(Envelope::from_bytes(&bytes).is_err()); + } + + #[test] + fn envelope_too_short() { + let env = Envelope { + nonce: &[2; 12], + payload: b"HELLO", + }; + + let bytes = env.to_bytes(); + let bytes = &bytes[..10]; + assert!(Envelope::from_bytes(bytes).is_err()); + } + #[test] fn round_trip() { let version_id = Uuid::new_v4(); From 0af66fd6c8029390c53f0d4fa34b3e4fe168c16d Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Tue, 19 Oct 2021 18:31:30 -0400 Subject: [PATCH 6/6] Validate encryption using externally-generated data --- taskchampion/src/server/crypto.rs | 92 +++++++++++++++++++ taskchampion/src/server/generate-test-data.py | 77 ++++++++++++++++ taskchampion/src/server/test-bad-app-id.data | 2 + .../src/server/test-bad-client-key.data | 1 + taskchampion/src/server/test-bad-secret.data | 1 + .../src/server/test-bad-version-id.data | 1 + taskchampion/src/server/test-bad-version.data | 1 + .../src/server/test-bad-version_id.data | 2 + taskchampion/src/server/test-good.data | 1 + 9 files changed, 178 insertions(+) create mode 100644 taskchampion/src/server/generate-test-data.py create mode 100644 taskchampion/src/server/test-bad-app-id.data create mode 100644 taskchampion/src/server/test-bad-client-key.data create mode 100644 taskchampion/src/server/test-bad-secret.data create mode 100644 taskchampion/src/server/test-bad-version-id.data create mode 100644 taskchampion/src/server/test-bad-version.data create mode 100644 taskchampion/src/server/test-bad-version_id.data create mode 100644 taskchampion/src/server/test-good.data diff --git a/taskchampion/src/server/crypto.rs b/taskchampion/src/server/crypto.rs index b2728acad..978a38fd8 100644 --- a/taskchampion/src/server/crypto.rs +++ b/taskchampion/src/server/crypto.rs @@ -317,4 +317,96 @@ mod test { let cryptor = Cryptor::new(client_key, &secret).unwrap(); assert!(cryptor.unseal(sealed).is_err()); } + + mod externally_valid { + // validate data generated by generate-test-data.py. The intent is to + // validate that this format matches the specification by implementing + // the specification in a second language + use super::*; + use pretty_assertions::assert_eq; + + /// The values in generate-test-data.py + fn defaults() -> (Uuid, Uuid, Vec) { + ( + Uuid::parse_str("b0517957-f912-4d49-8330-f612e73030c4").unwrap(), + Uuid::parse_str("0666d464-418a-4a08-ad53-6f15c78270cd").unwrap(), + b"b4a4e6b7b811eda1dc1a2693ded".to_vec(), + ) + } + + #[test] + fn good() { + let (version_id, client_key, encryption_secret) = defaults(); + let sealed = Sealed { + version_id, + payload: include_bytes!("test-good.data").to_vec(), + }; + + let cryptor = Cryptor::new(client_key, &Secret(encryption_secret)).unwrap(); + let unsealed = cryptor.unseal(sealed).unwrap(); + + assert_eq!(unsealed.payload, b"SUCCESS"); + assert_eq!(unsealed.version_id, version_id); + } + + #[test] + fn bad_version_id() { + let (version_id, client_key, encryption_secret) = defaults(); + let sealed = Sealed { + version_id, + payload: include_bytes!("test-bad-version-id.data").to_vec(), + }; + + let cryptor = Cryptor::new(client_key, &Secret(encryption_secret)).unwrap(); + assert!(cryptor.unseal(sealed).is_err()); + } + + #[test] + fn bad_client_key() { + let (version_id, client_key, encryption_secret) = defaults(); + let sealed = Sealed { + version_id, + payload: include_bytes!("test-bad-client-key.data").to_vec(), + }; + + let cryptor = Cryptor::new(client_key, &Secret(encryption_secret)).unwrap(); + assert!(cryptor.unseal(sealed).is_err()); + } + + #[test] + fn bad_secret() { + let (version_id, client_key, encryption_secret) = defaults(); + let sealed = Sealed { + version_id, + payload: include_bytes!("test-bad-secret.data").to_vec(), + }; + + let cryptor = Cryptor::new(client_key, &Secret(encryption_secret)).unwrap(); + assert!(cryptor.unseal(sealed).is_err()); + } + + #[test] + fn bad_version() { + let (version_id, client_key, encryption_secret) = defaults(); + let sealed = Sealed { + version_id, + payload: include_bytes!("test-bad-version.data").to_vec(), + }; + + let cryptor = Cryptor::new(client_key, &Secret(encryption_secret)).unwrap(); + assert!(cryptor.unseal(sealed).is_err()); + } + + #[test] + fn bad_app_id() { + let (version_id, client_key, encryption_secret) = defaults(); + let sealed = Sealed { + version_id, + payload: include_bytes!("test-bad-app-id.data").to_vec(), + }; + + let cryptor = Cryptor::new(client_key, &Secret(encryption_secret)).unwrap(); + assert!(cryptor.unseal(sealed).is_err()); + } + } } diff --git a/taskchampion/src/server/generate-test-data.py b/taskchampion/src/server/generate-test-data.py new file mode 100644 index 000000000..366597813 --- /dev/null +++ b/taskchampion/src/server/generate-test-data.py @@ -0,0 +1,77 @@ +# This file generates test-encrypted.data. To run it: +# - pip install cryptography pbkdf2 +# - python taskchampion/src/server/generate-test-data.py taskchampion/src/server/ + +import os +import hashlib +import pbkdf2 +import secrets +import sys +import uuid + +from cryptography.hazmat.primitives.ciphers.aead import ChaCha20Poly1305 + +# these values match values used in the rust tests +client_key = "0666d464-418a-4a08-ad53-6f15c78270cd" +encryption_secret = b"b4a4e6b7b811eda1dc1a2693ded" +version_id = "b0517957-f912-4d49-8330-f612e73030c4" + +def gen( + version_id=version_id, client_key=client_key, encryption_secret=encryption_secret, + app_id=1, version=1): + # first, generate the encryption key + salt = hashlib.sha256(uuid.UUID(client_key).bytes).digest() + key = pbkdf2.PBKDF2( + encryption_secret, + salt, + digestmodule=hashlib.sha256, + iterations=100000, + ).read(32) + + # create a nonce + nonce = secrets.token_bytes(12) + + assert len(b"\x01") == 1 + # create the AAD + aad = b''.join([ + bytes([app_id]), + uuid.UUID(version_id).bytes, + ]) + + # encrypt using AEAD + chacha = ChaCha20Poly1305(key) + ciphertext = chacha.encrypt(nonce, b"SUCCESS", aad) + + # create the envelope + envelope = b''.join([ + bytes([version]), + nonce, + ciphertext, + ]) + + return envelope + + +def main(): + dir = sys.argv[1] + + with open(os.path.join(dir, 'test-good.data'), "wb") as f: + f.write(gen()) + + with open(os.path.join(dir, 'test-bad-version-id.data'), "wb") as f: + f.write(gen(version_id=uuid.uuid4().hex)) + + with open(os.path.join(dir, 'test-bad-client-key.data'), "wb") as f: + f.write(gen(client_key=uuid.uuid4().hex)) + + with open(os.path.join(dir, 'test-bad-secret.data'), "wb") as f: + f.write(gen(encryption_secret=b"xxxxxxxxxxxxxxxxxxxxx")) + + with open(os.path.join(dir, 'test-bad-version.data'), "wb") as f: + f.write(gen(version=99)) + + with open(os.path.join(dir, 'test-bad-app-id.data'), "wb") as f: + f.write(gen(app_id=99)) + + +main() diff --git a/taskchampion/src/server/test-bad-app-id.data b/taskchampion/src/server/test-bad-app-id.data new file mode 100644 index 000000000..a1c6832a6 --- /dev/null +++ b/taskchampion/src/server/test-bad-app-id.data @@ -0,0 +1,2 @@ +#$ +^~B>n)ji19|~ \ No newline at end of file diff --git a/taskchampion/src/server/test-bad-client-key.data b/taskchampion/src/server/test-bad-client-key.data new file mode 100644 index 000000000..bfdd9635e --- /dev/null +++ b/taskchampion/src/server/test-bad-client-key.data @@ -0,0 +1 @@ +A4 t; pϦx^reJԤ \ No newline at end of file diff --git a/taskchampion/src/server/test-bad-secret.data b/taskchampion/src/server/test-bad-secret.data new file mode 100644 index 000000000..696da066f --- /dev/null +++ b/taskchampion/src/server/test-bad-secret.data @@ -0,0 +1 @@ +/}d EdIcX-!V%4d]} \ No newline at end of file diff --git a/taskchampion/src/server/test-bad-version-id.data b/taskchampion/src/server/test-bad-version-id.data new file mode 100644 index 000000000..2ccd8c638 --- /dev/null +++ b/taskchampion/src/server/test-bad-version-id.data @@ -0,0 +1 @@ +la|@ύS_zV9qх)+ \ No newline at end of file diff --git a/taskchampion/src/server/test-bad-version.data b/taskchampion/src/server/test-bad-version.data new file mode 100644 index 000000000..20fed792e --- /dev/null +++ b/taskchampion/src/server/test-bad-version.data @@ -0,0 +1 @@ +cTHp>溦m4~10PIW \ No newline at end of file diff --git a/taskchampion/src/server/test-bad-version_id.data b/taskchampion/src/server/test-bad-version_id.data new file mode 100644 index 000000000..4a228ec4a --- /dev/null +++ b/taskchampion/src/server/test-bad-version_id.data @@ -0,0 +1,2 @@ +B +-3%j,*ߺ7꩖QKOFPZ \ No newline at end of file diff --git a/taskchampion/src/server/test-good.data b/taskchampion/src/server/test-good.data new file mode 100644 index 000000000..9efec7577 --- /dev/null +++ b/taskchampion/src/server/test-good.data @@ -0,0 +1 @@ +pѿҟVTo"}cTY7 @dLT` \ No newline at end of file