From 82a4ec9251c8eb4dff5d16771b296fafa87407ae Mon Sep 17 00:00:00 2001 From: Xiretza Date: Tue, 26 Apr 2022 22:06:15 +0200 Subject: [PATCH 01/11] feat(utils): add randombytes_array() function --- src/utils.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/utils.rs b/src/utils.rs index ecbc820..d14a44c 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,6 +1,8 @@ // SPDX-FileCopyrightText: © 2020 Etebase Authors // SPDX-License-Identifier: LGPL-2.1-only +use std::convert::TryInto; + use sodiumoxide::{ base64, padding::{pad, unpad}, @@ -44,6 +46,25 @@ pub fn randombytes(size: usize) -> Vec { sodiumoxide::randombytes::randombytes(size) } +/// A version of [`randombytes`] that returns a fixed-size array instead of a Vec. +/// +/// # Examples +/// +/// ``` +/// use etebase::utils::randombytes_array; +/// +/// // Explicitly specifying the length as a type generic +/// let a = randombytes_array::<5>(); +/// +/// // Letting the length be inferred from the result type +/// let b: [u8; 10] = randombytes_array(); +/// ``` +pub fn randombytes_array() -> [u8; N] { + sodiumoxide::randombytes::randombytes(N) + .try_into() + .expect("randombytes() returned a Vec with wrong size") +} + /// Return a buffer filled with deterministically cryptographically random bytes /// /// This function is similar to [`randombytes`] but always returns the same data for the same seed. From 1114244db0516b08be711d22d3232daf848b522d Mon Sep 17 00:00:00 2001 From: Xiretza Date: Wed, 27 Apr 2022 10:01:23 +0200 Subject: [PATCH 02/11] fix(Account): use slice instead of Vec for salt in signup_common --- src/service.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/service.rs b/src/service.rs index b874384..30474d6 100644 --- a/src/service.rs +++ b/src/service.rs @@ -111,7 +111,7 @@ impl Account { let salt = randombytes(32); let main_key = derive_key(&salt, password)?; - Self::signup_common(client, user, main_key, salt) + Self::signup_common(client, user, main_key, &salt) } /// Creates a new user on the server and returns a handle to it. The user is authenticated @@ -128,14 +128,14 @@ impl Account { let salt = randombytes(32); let main_key = main_key.to_vec(); - Self::signup_common(client, user, main_key, salt) + Self::signup_common(client, user, main_key, &salt) } fn signup_common( mut client: Client, user: &User, main_key: Vec, - salt: Vec, + salt: &[u8], ) -> Result { let authenticator = Authenticator::new(&client); let version = super::CURRENT_VERSION; @@ -151,7 +151,7 @@ impl Account { let login_response = authenticator.signup( user, - &salt, + salt, login_crypto_manager.pubkey(), identity_crypto_manager.pubkey(), &encrypted_content, From 705c1b6071689b0e2d6854624cb37c230632b275 Mon Sep 17 00:00:00 2001 From: Xiretza Date: Wed, 27 Apr 2022 10:43:36 +0200 Subject: [PATCH 03/11] fix(crypto): use array instead of Vec for main_key --- src/crypto.rs | 14 +++++++++----- src/service.rs | 47 ++++++++++++++++++++--------------------------- 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/src/crypto.rs b/src/crypto.rs index a463d61..abaeb1e 100644 --- a/src/crypto.rs +++ b/src/crypto.rs @@ -8,6 +8,8 @@ use sodiumoxide::crypto::{ sign, }; +use crate::utils::SYMMETRIC_KEY_SIZE; + use super::error::{Error, Result}; macro_rules! to_enc_error { @@ -31,21 +33,23 @@ pub fn init() -> Result<()> { to_enc_error!(sodiumoxide::init(), "Failed initialising libsodium") } -pub fn derive_key(salt: &[u8], password: &str) -> Result> { - let mut key = vec![0; 32]; +pub fn derive_key(salt: &[u8], password: &str) -> Result<[u8; SYMMETRIC_KEY_SIZE]> { + let mut key = [0; SYMMETRIC_KEY_SIZE]; let salt = &salt[..argon2id13::SALTBYTES]; let salt: &[u8; argon2id13::SALTBYTES] = to_enc_error!(salt.try_into(), "Expect salt to be at least 16 bytes")?; let password = password.as_bytes(); - let ret = argon2id13::derive_key( + argon2id13::derive_key( &mut key, password, &argon2id13::Salt(*salt), argon2id13::OPSLIMIT_SENSITIVE, argon2id13::MEMLIMIT_MODERATE, - ); - Ok(to_enc_error!(ret, "pwhash failed")?.as_ref().to_vec()) + ) + .map_err(|_| Error::Encryption("pwhash failed"))?; + + Ok(key) } pub struct CryptoManager { diff --git a/src/service.rs b/src/service.rs index 30474d6..9b44d65 100644 --- a/src/service.rs +++ b/src/service.rs @@ -86,7 +86,7 @@ pub struct AccountDataStored<'a> { /// The main object for all user interactions and data manipulation, representing an authenticated /// user account. pub struct Account { - main_key: Vec, + main_key: [u8; SYMMETRIC_KEY_SIZE], version: u8, pub user: LoginResponseUser, client: Arc, @@ -119,14 +119,12 @@ impl Account { pub fn signup_key(client: Client, user: &User, main_key: &[u8]) -> Result { super::init()?; - if main_key.len() != SYMMETRIC_KEY_SIZE { - return Err(Error::ProgrammingError( - "Key should be exactly 32 bytes long.", - )); - } + // TODO: change argument type to [u8; 32] + let main_key: [u8; 32] = main_key + .try_into() + .map_err(|_| Error::ProgrammingError("Key should be exactly 32 bytes long."))?; let salt = randombytes(32); - let main_key = main_key.to_vec(); Self::signup_common(client, user, main_key, &salt) } @@ -134,13 +132,13 @@ impl Account { fn signup_common( mut client: Client, user: &User, - main_key: Vec, + main_key: [u8; SYMMETRIC_KEY_SIZE], salt: &[u8], ) -> Result { let authenticator = Authenticator::new(&client); let version = super::CURRENT_VERSION; - let main_crypto_manager = MainCryptoManager::new(try_into!(&main_key[..])?, version)?; + let main_crypto_manager = MainCryptoManager::new(&main_key, version)?; let login_crypto_manager = main_crypto_manager.login_crypto_manager()?; let identity_crypto_manager = BoxCryptoManager::keygen(None)?; @@ -201,11 +199,10 @@ impl Account { pub fn login_key(client: Client, username: &str, main_key: &[u8]) -> Result { super::init()?; - if main_key.len() < SYMMETRIC_KEY_SIZE { - return Err(Error::ProgrammingError( - "Key should be at least 32 bytes long.", - )); - } + // TODO: change argument type to [u8; 32] + let main_key: [u8; 32] = main_key + .try_into() + .map_err(|_| Error::ProgrammingError("Key should be exactly 32 bytes long."))?; let authenticator = Authenticator::new(&client); let login_challenge = match authenticator.get_login_challenge(username) { @@ -213,7 +210,7 @@ impl Account { // FIXME: fragile, we should have a proper error value or actually use codes if s == "User not properly init" { let user = User::new(username, "init@localhost"); - return Self::signup_key(client, &user, main_key); + return Self::signup_key(client, &user, &main_key[..]); } else { return Err(Error::Unauthorized(s)); } @@ -221,23 +218,20 @@ impl Account { rest => rest?, }; - let main_key = main_key.to_vec(); - Self::login_common(client, username, main_key, login_challenge) } fn login_common( mut client: Client, username: &str, - main_key: Vec, + main_key: [u8; SYMMETRIC_KEY_SIZE], login_challenge: LoginChallange, ) -> Result { let authenticator = Authenticator::new(&client); let version = login_challenge.version; - let main_key = main_key.to_vec(); - let main_crypto_manager = MainCryptoManager::new(try_into!(&main_key[..])?, version)?; + let main_crypto_manager = MainCryptoManager::new(&main_key, version)?; let login_crypto_manager = main_crypto_manager.login_crypto_manager()?; let response_struct = LoginBodyResponse { @@ -285,8 +279,7 @@ impl Account { let version = self.version; let username = &self.user.username; - let main_key = &self.main_key; - let main_crypto_manager = MainCryptoManager::new(try_into!(&main_key[..])?, version)?; + let main_crypto_manager = MainCryptoManager::new(&self.main_key, version)?; let login_crypto_manager = main_crypto_manager.login_crypto_manager()?; let response_struct = LoginBodyResponse { @@ -331,14 +324,14 @@ impl Account { let main_key = &self.main_key; let login_challenge = authenticator.get_login_challenge(username)?; - let old_main_crypto_manager = MainCryptoManager::new(try_into!(&main_key[..])?, version)?; + let old_main_crypto_manager = MainCryptoManager::new(main_key, version)?; let content = old_main_crypto_manager .0 .decrypt(&self.user.encrypted_content, None)?; let old_login_crypto_manager = old_main_crypto_manager.login_crypto_manager()?; let main_key = derive_key(&login_challenge.salt, new_password)?; - let main_crypto_manager = MainCryptoManager::new(try_into!(&main_key[..])?, version)?; + let main_crypto_manager = MainCryptoManager::new(&main_key, version)?; let login_crypto_manager = main_crypto_manager.login_crypto_manager()?; let encrypted_content = main_crypto_manager.0.encrypt(&content, None)?; @@ -450,8 +443,9 @@ impl Account { client.set_server_url(account_data.server_url)?; let main_key = crypto_manager.0.decrypt(account_data.key, None)?; + let main_key = try_into!(&main_key[..])?; - let main_crypto_manager = MainCryptoManager::new(try_into!(&main_key[..])?, version)?; + let main_crypto_manager = MainCryptoManager::new(&main_key, version)?; let content = main_crypto_manager .0 .decrypt(&account_data.user.encrypted_content, None)?; @@ -487,8 +481,7 @@ impl Account { fn main_crypto_manager(&self) -> Result { let version = self.version; - let main_key = &self.main_key; - MainCryptoManager::new(try_into!(&main_key[..])?, version) + MainCryptoManager::new(&self.main_key, version) } fn identity_crypto_manager(&self) -> Result { From 93ce8885e61a54e1628bf74142719c2f84155be2 Mon Sep 17 00:00:00 2001 From: Xiretza Date: Wed, 27 Apr 2022 10:57:04 +0200 Subject: [PATCH 04/11] feat(crypto): move length requirement of password salt to type level --- src/crypto.rs | 7 ++----- src/service.rs | 37 ++++++++++++++++++++++++++++++------- src/utils.rs | 2 ++ tests/crypto.rs | 6 +++++- 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/crypto.rs b/src/crypto.rs index abaeb1e..d5f4f48 100644 --- a/src/crypto.rs +++ b/src/crypto.rs @@ -8,7 +8,7 @@ use sodiumoxide::crypto::{ sign, }; -use crate::utils::SYMMETRIC_KEY_SIZE; +use crate::utils::{SALT_LENGTH, SYMMETRIC_KEY_SIZE}; use super::error::{Error, Result}; @@ -33,11 +33,8 @@ pub fn init() -> Result<()> { to_enc_error!(sodiumoxide::init(), "Failed initialising libsodium") } -pub fn derive_key(salt: &[u8], password: &str) -> Result<[u8; SYMMETRIC_KEY_SIZE]> { +pub fn derive_key(salt: &[u8; SALT_LENGTH], password: &str) -> Result<[u8; SYMMETRIC_KEY_SIZE]> { let mut key = [0; SYMMETRIC_KEY_SIZE]; - let salt = &salt[..argon2id13::SALTBYTES]; - let salt: &[u8; argon2id13::SALTBYTES] = - to_enc_error!(salt.try_into(), "Expect salt to be at least 16 bytes")?; let password = password.as_bytes(); argon2id13::derive_key( diff --git a/src/service.rs b/src/service.rs index 9b44d65..f1ea2fe 100644 --- a/src/service.rs +++ b/src/service.rs @@ -7,6 +7,7 @@ use std::convert::TryInto; use std::iter; use std::sync::Arc; +use crate::utils::SALT_LENGTH; use serde::{Deserialize, Serialize}; use super::{ @@ -25,8 +26,8 @@ use super::{ }, try_into, utils::{ - buffer_unpad, from_base64, randombytes, to_base64, MsgPackSerilization, StrBase64, - SYMMETRIC_KEY_SIZE, + buffer_unpad, from_base64, randombytes, randombytes_array, to_base64, MsgPackSerilization, + StrBase64, SYMMETRIC_KEY_SIZE, }, }; @@ -108,8 +109,10 @@ impl Account { pub fn signup(client: Client, user: &User, password: &str) -> Result { super::init()?; - let salt = randombytes(32); - let main_key = derive_key(&salt, password)?; + // only the first 16 bytes of the salt are used for key generation, but previous + // implementations have always generated 32 bytes regardless. + let salt = randombytes_array::<32>(); + let main_key = derive_key(&salt[..16].try_into().unwrap(), password)?; Self::signup_common(client, user, main_key, &salt) } @@ -124,7 +127,9 @@ impl Account { .try_into() .map_err(|_| Error::ProgrammingError("Key should be exactly 32 bytes long."))?; - let salt = randombytes(32); + // Since the key is provided as-is instead of being generated from a password+hash, this is + // not actually used for anything; generate it anyway for consistency. + let salt = randombytes_array::<32>(); Self::signup_common(client, user, main_key, &salt) } @@ -189,7 +194,17 @@ impl Account { rest => rest?, }; - let main_key = derive_key(&login_challenge.salt, password)?; + // A 32-byte value is generated during signup, but only first 16 bytes are used for key + // generation. + let salt = login_challenge + .salt + .get(..SALT_LENGTH) + .ok_or(Error::Encryption( + "Salt obtained from login challenge too short: expected at least 16 bytes", + ))? + .try_into() + .unwrap(); + let main_key = derive_key(&salt, password)?; Self::login_common(client, username, main_key, login_challenge) } @@ -330,7 +345,15 @@ impl Account { .decrypt(&self.user.encrypted_content, None)?; let old_login_crypto_manager = old_main_crypto_manager.login_crypto_manager()?; - let main_key = derive_key(&login_challenge.salt, new_password)?; + let salt = login_challenge + .salt + .get(..SALT_LENGTH) + .ok_or(Error::Encryption( + "Salt obtained from login challenge too short: expected at least 16 bytes", + ))? + .try_into() + .unwrap(); + let main_key = derive_key(&salt, new_password)?; let main_crypto_manager = MainCryptoManager::new(&main_key, version)?; let login_crypto_manager = main_crypto_manager.login_crypto_manager()?; diff --git a/src/utils.rs b/src/utils.rs index d14a44c..dc9da4b 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -22,6 +22,8 @@ macro_rules! try_into { pub type StrBase64 = str; pub type StringBase64 = String; +/// The size of the salt added to the password to derive the symmetric encryption key +pub const SALT_LENGTH: usize = 16; // sodium.crypto_pwhash_argon2id_SALTBYTES /// The size of a symmetric encryption key pub const SYMMETRIC_KEY_SIZE: usize = 32; // sodium.crypto_aead_xchacha20poly1305_ietf_KEYBYTES; /// The size of a symmetric encryption tag diff --git a/tests/crypto.rs b/tests/crypto.rs index ce1dad7..71c206d 100644 --- a/tests/crypto.rs +++ b/tests/crypto.rs @@ -13,7 +13,11 @@ use std::convert::TryInto; fn derive_key() { etebase::init().unwrap(); - let derived = crypto::derive_key(&from_base64(USER.salt).unwrap(), USER.password).unwrap(); + let derived = crypto::derive_key( + from_base64(USER.salt).unwrap()[..16].try_into().unwrap(), + USER.password, + ) + .unwrap(); let expected = from_base64(USER.key).unwrap(); assert_eq!(&derived[..], &expected[..]); } From e21e5bbeda9e74f74d33673f7a13f4c2e1a42df1 Mon Sep 17 00:00:00 2001 From: Xiretza Date: Mon, 9 May 2022 10:40:37 +0200 Subject: [PATCH 05/11] fix: use arrays instead of Vecs internally --- src/crypto.rs | 11 ++++++----- src/encrypted_models.rs | 17 ++++++++++++----- src/service.rs | 4 ++++ tests/crypto.rs | 6 +++--- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/crypto.rs b/src/crypto.rs index d5f4f48..c3b1e62 100644 --- a/src/crypto.rs +++ b/src/crypto.rs @@ -18,7 +18,7 @@ macro_rules! to_enc_error { }; } -fn generichash_quick(msg: &[u8], key: Option<&[u8]>) -> Result> { +fn generichash_quick(msg: &[u8], key: Option<&[u8]>) -> Result<[u8; 32]> { let mut state = to_enc_error!( generichash::State::new(Some(32), key), "Failed to init hash" @@ -26,7 +26,8 @@ fn generichash_quick(msg: &[u8], key: Option<&[u8]>) -> Result> { to_enc_error!(state.update(msg), "Failed to update hash")?; Ok(to_enc_error!(state.finalize(), "Failed to finalize hash")? .as_ref() - .to_vec()) + .try_into() + .expect("generichash returned result of wrong size")) } pub fn init() -> Result<()> { @@ -219,7 +220,7 @@ impl CryptoManager { )?) } - pub fn derive_subkey(&self, salt: &[u8]) -> Result> { + pub fn derive_subkey(&self, salt: &[u8]) -> Result<[u8; 32]> { generichash_quick(&self.sub_derivation_key, Some(salt)) } @@ -227,11 +228,11 @@ impl CryptoManager { CryptoMac::new(Some(&self.mac_key)) } - pub fn calculate_mac(&self, msg: &[u8]) -> Result> { + pub fn calculate_mac(&self, msg: &[u8]) -> Result<[u8; 32]> { generichash_quick(msg, Some(&self.mac_key)) } - pub fn calculate_hash(&self, msg: &[u8]) -> Result> { + pub fn calculate_hash(&self, msg: &[u8]) -> Result<[u8; 32]> { generichash_quick(msg, None) } } diff --git a/src/encrypted_models.rs b/src/encrypted_models.rs index bad3502..a5141d8 100644 --- a/src/encrypted_models.rs +++ b/src/encrypted_models.rs @@ -10,6 +10,7 @@ use std::collections::HashMap; use serde::{Deserialize, Serialize}; use serde_repr::{Deserialize_repr, Serialize_repr}; +use sodiumoxide::crypto::sign; use super::{ chunker::Rollsum, @@ -475,7 +476,7 @@ impl EncryptedCollection { account_crypto_manager: &AccountCryptoManager, identity_crypto_manager: &BoxCryptoManager, username: &str, - pubkey: &[u8], + pubkey: &[u8; sign::PUBLICKEYBYTES], access_level: CollectionAccessLevel, ) -> Result { let uid = to_base64(&randombytes(32))?; @@ -486,8 +487,8 @@ impl EncryptedCollection { collection_type, }; let raw_content = rmp_serde::to_vec_named(&content)?; - let signed_encryption_key = identity_crypto_manager - .encrypt(&buffer_pad_small(&raw_content)?, try_into!(pubkey)?)?; + let signed_encryption_key = + identity_crypto_manager.encrypt(&buffer_pad_small(&raw_content)?, pubkey)?; Ok(SignedInvitation { uid, version: CURRENT_VERSION, @@ -978,11 +979,17 @@ impl EncryptedItem { encryption_key: Option<&[u8]>, ) -> Result { let encryption_key = match encryption_key { - Some(encryption_key) => parent_crypto_manager.0.decrypt(encryption_key, None)?, + Some(encryption_key) => parent_crypto_manager + .0 + .decrypt(encryption_key, None)? + .try_into() + .map_err(|_| { + Error::ProgrammingError("Decrypted encryption key has wrong length") + })?, None => parent_crypto_manager.0.derive_subkey(uid.as_bytes())?, }; - ItemCryptoManager::new(try_into!(&encryption_key[..])?, version) + ItemCryptoManager::new(&encryption_key, version) } pub fn crypto_manager( diff --git a/src/service.rs b/src/service.rs index f1ea2fe..4540a44 100644 --- a/src/service.rs +++ b/src/service.rs @@ -1144,6 +1144,10 @@ impl CollectionInvitationManager { pubkey: &[u8], access_level: CollectionAccessLevel, ) -> Result<()> { + // TODO: change argument type to &[u8; 32] + let pubkey: &[u8; 32] = pubkey + .try_into() + .map_err(|_| Error::ProgrammingError("Public key should be exactly 32 bytes long"))?; let invitation = collection.col.create_invitation( &self.account_crypto_manager, &self.identity_crypto_manager, diff --git a/tests/crypto.rs b/tests/crypto.rs index 71c206d..b47248d 100644 --- a/tests/crypto.rs +++ b/tests/crypto.rs @@ -37,19 +37,19 @@ fn crypto_manager() { let subkey = crypto_manager.derive_subkey(&[0; 32]).unwrap(); assert_eq!( - subkey, + &subkey[..], from_base64("4w-VCSTETv26JjVlVlD2VaACcb6aQSD2JbF-e89xnaA").unwrap() ); let hash = crypto_manager.calculate_mac(&[0; 32]).unwrap(); assert_eq!( - hash, + &hash[..], from_base64("bz6eMZdAkIuiLUuFDiVwuH3IFs4hYkRfhzang_JzHr8").unwrap() ); let hash = crypto_manager.calculate_hash(&[0; 32]).unwrap(); assert_eq!( - hash, + &hash[..], from_base64("iesNaoppHa4s0V7QNpkxzgqUnsr6XD-T-BIYM2RuFcM").unwrap() ); From 1640a690276873fb360d8a718618e254f0c68b9a Mon Sep 17 00:00:00 2001 From: Xiretza Date: Wed, 27 Apr 2022 10:51:49 +0200 Subject: [PATCH 06/11] fix: improve error messages for some try_into calls --- src/encrypted_models.rs | 52 +++++++++++++++++------------- src/service.rs | 70 ++++++++++++++++++++++++++++++++--------- src/utils.rs | 2 ++ 3 files changed, 88 insertions(+), 36 deletions(-) diff --git a/src/encrypted_models.rs b/src/encrypted_models.rs index a5141d8..21dac49 100644 --- a/src/encrypted_models.rs +++ b/src/encrypted_models.rs @@ -16,7 +16,6 @@ use super::{ chunker::Rollsum, crypto::{BoxCryptoManager, CryptoMac, CryptoManager}, error::{Error, Result}, - try_into, utils::{ buffer_pad, buffer_pad_fixed, buffer_pad_small, buffer_unpad, buffer_unpad_fixed, from_base64, memcmp, randombytes, shuffle, to_base64, MsgPackSerilization, StringBase64, @@ -266,15 +265,17 @@ impl SignedInvitation { &self, identity_crypto_manager: &BoxCryptoManager, ) -> Result> { - let from_pubkey = match self.from_pubkey.as_deref() { - Some(from_pubkey) => from_pubkey, - None => { - return Err(Error::ProgrammingError( - "Missing invitation encryption key.", - )) - } - }; - identity_crypto_manager.decrypt(&self.signed_encryption_key, try_into!(from_pubkey)?) + let from_pubkey = self + .from_pubkey + .as_deref() + .ok_or(Error::ProgrammingError( + "Missing invitation encryption key.", + ))? + .try_into() + .map_err(|_| { + Error::Encryption("Received invitation encryption key has wrong length") + })?; + identity_crypto_manager.decrypt(&self.signed_encryption_key, from_pubkey) } } @@ -527,9 +528,12 @@ impl EncryptedCollection { collection_type: Option<&[u8]>, ) -> Result { let encryption_key = - Self::collection_key_static(parent_crypto_manager, encryption_key, collection_type)?; + Self::collection_key_static(parent_crypto_manager, encryption_key, collection_type)? + .as_slice() + .try_into() + .map_err(|_| Error::Encryption("Collection encryption key has wrong length"))?; - CollectionCryptoManager::new(try_into!(&encryption_key[..])?, version) + CollectionCryptoManager::new(&encryption_key, version) } pub fn crypto_manager( @@ -610,12 +614,13 @@ impl EncryptedRevision { crypto_manager: &ItemCryptoManager, additional_data: &[u8], ) -> Result { - let mac = from_base64(&self.uid)?; + let mac = from_base64(&self.uid)? + .as_slice() + .try_into() + .map_err(|_| Error::Encryption("Collection MAC has wrong length"))?; let ad_hash = self.calculate_hash(crypto_manager, additional_data)?; - crypto_manager - .0 - .verify(&self.meta, try_into!(&mac[..])?, Some(&ad_hash)) + crypto_manager.0.verify(&self.meta, &mac, Some(&ad_hash)) } pub fn set_meta( @@ -640,14 +645,17 @@ impl EncryptedRevision { crypto_manager: &ItemCryptoManager, additional_data: &[u8], ) -> Result> { - let mac = from_base64(&self.uid)?; + let mac = from_base64(&self.uid)? + .as_slice() + .try_into() + .map_err(|_| Error::Encryption("Collection MAC has wrong length"))?; let ad_hash = self.calculate_hash(crypto_manager, additional_data)?; - buffer_unpad(&crypto_manager.0.decrypt_detached( - &self.meta, - try_into!(&mac[..])?, - Some(&ad_hash), - )?) + buffer_unpad( + &crypto_manager + .0 + .decrypt_detached(&self.meta, &mac, Some(&ad_hash))?, + ) } pub fn set_content( diff --git a/src/service.rs b/src/service.rs index 4540a44..ed041be 100644 --- a/src/service.rs +++ b/src/service.rs @@ -7,7 +7,7 @@ use std::convert::TryInto; use std::iter; use std::sync::Arc; -use crate::utils::SALT_LENGTH; +use crate::utils::{PRIVATE_KEY_SIZE, SALT_LENGTH}; use serde::{Deserialize, Serialize}; use super::{ @@ -269,9 +269,16 @@ impl Account { let content = main_crypto_manager .0 .decrypt(&login_response.user.encrypted_content, None)?; - let account_key = &content[..SYMMETRIC_KEY_SIZE]; - let account_crypto_manager = - main_crypto_manager.account_crypto_manager(try_into!(account_key)?)?; + + // The content is the concatenation of the account key and the private key + let account_key = content + .get(..SYMMETRIC_KEY_SIZE) + .ok_or(Error::Encryption( + "Server's login response too short to contain account key", + ))? + .try_into() + .unwrap(); + let account_crypto_manager = main_crypto_manager.account_crypto_manager(account_key)?; let ret = Self { main_key, @@ -417,8 +424,17 @@ impl Account { /// The data should be encrypted using a 32-byte `encryption_key` for added security. pub fn save(&self, encryption_key: Option<&[u8]>) -> Result { let version = super::CURRENT_VERSION; - let encryption_key = encryption_key.unwrap_or(&[0; 32]); - let crypto_manager = StorageCryptoManager::new(try_into!(encryption_key)?, version)?; + + let encryption_key = if let Some(encryption_key) = encryption_key { + // TODO: change argument type to Option<&[u8; 32]> + encryption_key.try_into().map_err(|_| { + Error::ProgrammingError("Encryption key must be exactly 32 bytes long") + })? + } else { + &[0; 32] + }; + + let crypto_manager = StorageCryptoManager::new(encryption_key, version)?; let account_data = AccountData { user: self.user.clone(), version, @@ -450,13 +466,21 @@ impl Account { account_data_stored: &str, encryption_key: Option<&[u8]>, ) -> Result { - let encryption_key = encryption_key.unwrap_or(&[0; 32]); + let encryption_key = if let Some(encryption_key) = encryption_key { + // TODO: change argument type to Option<&[u8; 32]> + encryption_key.try_into().map_err(|_| { + Error::ProgrammingError("Encryption key must be exactly 32 bytes long") + })? + } else { + &[0; 32] + }; + let account_data_stored = from_base64(account_data_stored)?; let account_data_stored: AccountDataStored = rmp_serde::from_read_ref(&account_data_stored)?; let version = account_data_stored.version; - let crypto_manager = StorageCryptoManager::new(try_into!(encryption_key)?, version)?; + let crypto_manager = StorageCryptoManager::new(encryption_key, version)?; let decrypted = crypto_manager .0 .decrypt(account_data_stored.encrypted_data, Some(&[version]))?; @@ -466,15 +490,25 @@ impl Account { client.set_server_url(account_data.server_url)?; let main_key = crypto_manager.0.decrypt(account_data.key, None)?; - let main_key = try_into!(&main_key[..])?; + let main_key = main_key + .as_slice() + .try_into() + .map_err(|_| Error::Encryption("Restored main key has wrong size"))?; let main_crypto_manager = MainCryptoManager::new(&main_key, version)?; let content = main_crypto_manager .0 .decrypt(&account_data.user.encrypted_content, None)?; - let account_key = &content[..SYMMETRIC_KEY_SIZE]; - let account_crypto_manager = - main_crypto_manager.account_crypto_manager(try_into!(account_key)?)?; + + // The content is the concatenation of the account key and the private key + let account_key = content + .get(..SYMMETRIC_KEY_SIZE) + .ok_or(Error::Encryption( + "Server's login response too short to contain account key", + ))? + .try_into() + .unwrap(); + let account_crypto_manager = main_crypto_manager.account_crypto_manager(account_key)?; Ok(Self { user: account_data.user, @@ -512,8 +546,16 @@ impl Account { let content = main_crypto_manager .0 .decrypt(&self.user.encrypted_content, None)?; - let privkey = &content[SYMMETRIC_KEY_SIZE..]; - main_crypto_manager.identity_crypto_manager(try_into!(privkey)?) + + // The content is the concatenation of the account key and the private key + let privkey = content + .get(SYMMETRIC_KEY_SIZE..(SYMMETRIC_KEY_SIZE + PRIVATE_KEY_SIZE)) + .ok_or(Error::Encryption( + "Server's login response too short to contain private key", + ))? + .try_into() + .unwrap(); + main_crypto_manager.identity_crypto_manager(privkey) } } diff --git a/src/utils.rs b/src/utils.rs index dc9da4b..aab24ac 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -24,6 +24,8 @@ pub type StringBase64 = String; /// The size of the salt added to the password to derive the symmetric encryption key pub const SALT_LENGTH: usize = 16; // sodium.crypto_pwhash_argon2id_SALTBYTES +/// The size of the private encryption key +pub const PRIVATE_KEY_SIZE: usize = 32; // sodium.crypto_box_curve25519xsalsa20poly1305_SECRETKEYBYTES; /// The size of a symmetric encryption key pub const SYMMETRIC_KEY_SIZE: usize = 32; // sodium.crypto_aead_xchacha20poly1305_ietf_KEYBYTES; /// The size of a symmetric encryption tag From 9a41038f77695ee35e4ddb1423ba5b5a0eaa9d04 Mon Sep 17 00:00:00 2001 From: Xiretza Date: Wed, 18 May 2022 20:34:53 +0200 Subject: [PATCH 07/11] fix: remove some unnecessary try_into calls --- src/crypto.rs | 10 +++++----- src/service.rs | 10 ++++------ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/crypto.rs b/src/crypto.rs index c3b1e62..feb88db 100644 --- a/src/crypto.rs +++ b/src/crypto.rs @@ -301,14 +301,14 @@ impl BoxCryptoManager { let privkey_scalar = scalarmult::Scalar(*privkey); let privkey = box_::SecretKey(*privkey); let pubkey_scalar = scalarmult::scalarmult_base(&privkey_scalar); - let pubkey = box_::PublicKey(pubkey_scalar[..].try_into().unwrap()); + let pubkey = box_::PublicKey(pubkey_scalar.0); Ok(BoxCryptoManager { privkey, pubkey }) } pub fn encrypt(&self, msg: &[u8], pubkey: &[u8; box_::PUBLICKEYBYTES]) -> Result> { - let pubkey = box_::PublicKey(pubkey[..].try_into().unwrap()); - let privkey = box_::SecretKey(self.privkey[..].try_into().unwrap()); + let pubkey = box_::PublicKey(*pubkey); + let privkey = box_::SecretKey(self.privkey.0); let nonce = box_::gen_nonce(); let encrypted = box_::seal(msg, &nonce, &pubkey, &privkey); let ret = [nonce.as_ref(), &encrypted].concat(); @@ -317,8 +317,8 @@ impl BoxCryptoManager { } pub fn decrypt(&self, cipher: &[u8], pubkey: &[u8; sign::PUBLICKEYBYTES]) -> Result> { - let pubkey = box_::PublicKey(pubkey[..].try_into().unwrap()); - let privkey = box_::SecretKey(self.privkey[..].try_into().unwrap()); + let pubkey = box_::PublicKey(*pubkey); + let privkey = box_::SecretKey(self.privkey.0); let nonce = &cipher[..box_::NONCEBYTES]; let nonce: &[u8; box_::NONCEBYTES] = to_enc_error!(nonce.try_into(), "Got a nonce of a wrong size")?; diff --git a/src/service.rs b/src/service.rs index ed041be..b10e343 100644 --- a/src/service.rs +++ b/src/service.rs @@ -24,10 +24,9 @@ use super::{ ItemListResponse, ItemManagerOnline, IteratorListResponse, LoginBodyResponse, LoginChallange, LoginResponseUser, User, UserProfile, }, - try_into, utils::{ - buffer_unpad, from_base64, randombytes, randombytes_array, to_base64, MsgPackSerilization, - StrBase64, SYMMETRIC_KEY_SIZE, + buffer_unpad, from_base64, randombytes_array, to_base64, MsgPackSerilization, StrBase64, + SYMMETRIC_KEY_SIZE, }, }; @@ -148,7 +147,7 @@ impl Account { let identity_crypto_manager = BoxCryptoManager::keygen(None)?; - let account_key = randombytes(SYMMETRIC_KEY_SIZE); + let account_key = randombytes_array(); let content = [&account_key, identity_crypto_manager.privkey()].concat(); let encrypted_content = main_crypto_manager.0.encrypt(&content, None)?; @@ -162,8 +161,7 @@ impl Account { client.set_token(Some(&login_response.token)); - let account_crypto_manager = - main_crypto_manager.account_crypto_manager(try_into!(&account_key[..])?)?; + let account_crypto_manager = main_crypto_manager.account_crypto_manager(&account_key)?; let ret = Self { main_key, From 28ea090d0591c954e4bd431c8190f96fb0533b15 Mon Sep 17 00:00:00 2001 From: Xiretza Date: Sun, 22 May 2022 17:15:08 +0200 Subject: [PATCH 08/11] chore: remove unused try_into! macro --- src/utils.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/utils.rs b/src/utils.rs index aab24ac..7e6a89f 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -10,15 +10,6 @@ use sodiumoxide::{ use super::error::{Error, Result}; -#[doc(hidden)] -#[macro_export] -macro_rules! try_into { - ($x:expr) => { - ($x).try_into() - .or(Err(Error::ProgrammingError("Try into failed"))) - }; -} - pub type StrBase64 = str; pub type StringBase64 = String; From 35a94926aa71ee7f3e8107a15a9ea6bcd29949e6 Mon Sep 17 00:00:00 2001 From: Xiretza Date: Sun, 22 May 2022 21:03:35 +0200 Subject: [PATCH 09/11] chore: update changelog --- ChangeLog.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog.md b/ChangeLog.md index 90b3997..f507f30 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -1,6 +1,9 @@ # Changelog ## Unreleased +### Features +* Added `utils::randombytes_array()` function to create fixed-sized arrays of random numbers + ### Fixes * Loosened the argument types for `ItemMetadata::set_*()` methods to allow passing `String`s directly * Loosened the argument types for several methods to take any `IntoIterator`, not just `Iterator` @@ -10,6 +13,7 @@ * Renamed `SignedInvitation::from_*()` methods to `sender_*()` to avoid confusing them with constructors. * Renamed the `Account::is_etebase_server()` function to a method - `Client::is_server_valid()`. * `Error` now implements `Eq`. +* Made some error messages more specific ## Version 0.5.3 * Upgrade dependencies - ignore package lock. Upgrade to absolute latest. From 25de264812c76c5a04ec7d6798dc9b1c1ba62605 Mon Sep 17 00:00:00 2001 From: Xiretza Date: Tue, 21 Jun 2022 17:30:43 +0200 Subject: [PATCH 10/11] fix: rename SALT_LENGTH to SALT_SIZE for consistency --- src/crypto.rs | 4 ++-- src/service.rs | 6 +++--- src/utils.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/crypto.rs b/src/crypto.rs index feb88db..f9c4620 100644 --- a/src/crypto.rs +++ b/src/crypto.rs @@ -8,7 +8,7 @@ use sodiumoxide::crypto::{ sign, }; -use crate::utils::{SALT_LENGTH, SYMMETRIC_KEY_SIZE}; +use crate::utils::{SALT_SIZE, SYMMETRIC_KEY_SIZE}; use super::error::{Error, Result}; @@ -34,7 +34,7 @@ pub fn init() -> Result<()> { to_enc_error!(sodiumoxide::init(), "Failed initialising libsodium") } -pub fn derive_key(salt: &[u8; SALT_LENGTH], password: &str) -> Result<[u8; SYMMETRIC_KEY_SIZE]> { +pub fn derive_key(salt: &[u8; SALT_SIZE], password: &str) -> Result<[u8; SYMMETRIC_KEY_SIZE]> { let mut key = [0; SYMMETRIC_KEY_SIZE]; let password = password.as_bytes(); diff --git a/src/service.rs b/src/service.rs index b10e343..2cf34fd 100644 --- a/src/service.rs +++ b/src/service.rs @@ -7,7 +7,7 @@ use std::convert::TryInto; use std::iter; use std::sync::Arc; -use crate::utils::{PRIVATE_KEY_SIZE, SALT_LENGTH}; +use crate::utils::{PRIVATE_KEY_SIZE, SALT_SIZE}; use serde::{Deserialize, Serialize}; use super::{ @@ -196,7 +196,7 @@ impl Account { // generation. let salt = login_challenge .salt - .get(..SALT_LENGTH) + .get(..SALT_SIZE) .ok_or(Error::Encryption( "Salt obtained from login challenge too short: expected at least 16 bytes", ))? @@ -352,7 +352,7 @@ impl Account { let salt = login_challenge .salt - .get(..SALT_LENGTH) + .get(..SALT_SIZE) .ok_or(Error::Encryption( "Salt obtained from login challenge too short: expected at least 16 bytes", ))? diff --git a/src/utils.rs b/src/utils.rs index 7e6a89f..a1a79de 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -14,7 +14,7 @@ pub type StrBase64 = str; pub type StringBase64 = String; /// The size of the salt added to the password to derive the symmetric encryption key -pub const SALT_LENGTH: usize = 16; // sodium.crypto_pwhash_argon2id_SALTBYTES +pub const SALT_SIZE: usize = 16; // sodium.crypto_pwhash_argon2id_SALTBYTES /// The size of the private encryption key pub const PRIVATE_KEY_SIZE: usize = 32; // sodium.crypto_box_curve25519xsalsa20poly1305_SECRETKEYBYTES; /// The size of a symmetric encryption key From 6753479e850bc927461c3bb6930b2f57482f1e6f Mon Sep 17 00:00:00 2001 From: Xiretza Date: Tue, 21 Jun 2022 20:16:21 +0200 Subject: [PATCH 11/11] Add TODO.md file --- TODO.md | 9 +++++++++ src/service.rs | 5 ----- 2 files changed, 9 insertions(+), 5 deletions(-) create mode 100644 TODO.md diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..d67b776 --- /dev/null +++ b/TODO.md @@ -0,0 +1,9 @@ +# TODO + +This file tracks planned changes to the project, especially breaking changes that have to wait +until the next major release. + +## Planned changes for next major release + +- Change argument types in `src/service.rs` for public and private keys to use `[u8; 32]` instead + of `[u8]`. This should remove all the `ProgrammingError`s resulting from failed `try_into()` calls. diff --git a/src/service.rs b/src/service.rs index 2cf34fd..26eb6ff 100644 --- a/src/service.rs +++ b/src/service.rs @@ -121,7 +121,6 @@ impl Account { pub fn signup_key(client: Client, user: &User, main_key: &[u8]) -> Result { super::init()?; - // TODO: change argument type to [u8; 32] let main_key: [u8; 32] = main_key .try_into() .map_err(|_| Error::ProgrammingError("Key should be exactly 32 bytes long."))?; @@ -212,7 +211,6 @@ impl Account { pub fn login_key(client: Client, username: &str, main_key: &[u8]) -> Result { super::init()?; - // TODO: change argument type to [u8; 32] let main_key: [u8; 32] = main_key .try_into() .map_err(|_| Error::ProgrammingError("Key should be exactly 32 bytes long."))?; @@ -424,7 +422,6 @@ impl Account { let version = super::CURRENT_VERSION; let encryption_key = if let Some(encryption_key) = encryption_key { - // TODO: change argument type to Option<&[u8; 32]> encryption_key.try_into().map_err(|_| { Error::ProgrammingError("Encryption key must be exactly 32 bytes long") })? @@ -465,7 +462,6 @@ impl Account { encryption_key: Option<&[u8]>, ) -> Result { let encryption_key = if let Some(encryption_key) = encryption_key { - // TODO: change argument type to Option<&[u8; 32]> encryption_key.try_into().map_err(|_| { Error::ProgrammingError("Encryption key must be exactly 32 bytes long") })? @@ -1184,7 +1180,6 @@ impl CollectionInvitationManager { pubkey: &[u8], access_level: CollectionAccessLevel, ) -> Result<()> { - // TODO: change argument type to &[u8; 32] let pubkey: &[u8; 32] = pubkey .try_into() .map_err(|_| Error::ProgrammingError("Public key should be exactly 32 bytes long"))?;