[ENG-358] Malicious client protection for the KM (#562)

* add `queue_check`, `mount_check` and `unlock_check`

* make `unlock_check` marginally more readable

* clippy

* clean up & rename functions

* remove unlock check on `keys.list`

* use american spelling of `favorite`

* use `then_some` (thanks clippy)

* more american spelling

core/src/api/keys.rs

@@ -57,7 +57,7 @@ pub(crate) fn mount() -> RouterBuilder {
 		})
 		// do not unlock the key manager until this route returns true
 		.library_query("isUnlocked", |t| {
-			t(|_, _: (), library| async move { Ok(library.key_manager.is_unlocked().await?) })
+			t(|_, _: (), library| async move { Ok(library.key_manager.is_unlocked().await) })
 		})
 		// this is so we can show the key as mounted in the UI
 		.library_query("listMounted", |t| {
@@ -121,7 +121,7 @@ pub(crate) fn mount() -> RouterBuilder {
 		})
 		.library_mutation("syncKeyToLibrary", |t| {
 			t(|_, key_uuid: Uuid, library| async move {
-				let key = library.key_manager.sync_to_database(key_uuid)?;
+				let key = library.key_manager.sync_to_database(key_uuid).await?;
 
 				// does not check that the key doesn't exist before writing
 				write_storedkey_to_db(&library.db, &key).await?;
@@ -132,10 +132,11 @@ pub(crate) fn mount() -> RouterBuilder {
 		})
 		.library_mutation("updateAutomountStatus", |t| {
 			t(|_, args: AutomountUpdateArgs, library| async move {
-				if !library.key_manager.is_memory_only(args.uuid)? {
+				if !library.key_manager.is_memory_only(args.uuid).await? {
 					library
 						.key_manager
-						.change_automount_status(args.uuid, args.status)?;
+						.change_automount_status(args.uuid, args.status)
+						.await?;
 
 					library
 						.db
@@ -155,7 +156,7 @@ pub(crate) fn mount() -> RouterBuilder {
 		})
 		.library_mutation("deleteFromLibrary", |t| {
 			t(|_, key_uuid: Uuid, library| async move {
-				if !library.key_manager.is_memory_only(key_uuid)? {
+				if !library.key_manager.is_memory_only(key_uuid).await? {
 					library
 						.db
 						.key()
@@ -266,8 +267,11 @@ pub(crate) fn mount() -> RouterBuilder {
 					.await?;
 
 				if args.library_sync {
-					write_storedkey_to_db(&library.db, &library.key_manager.access_keystore(uuid)?)
-						.await?;
+					write_storedkey_to_db(
+						&library.db,
+						&library.key_manager.access_keystore(uuid).await?,
+					)
+					.await?;
 
 					if args.automount {
 						library

core/src/object/fs/encrypt.rs

@@ -38,7 +38,7 @@ pub struct Metadata {
 	pub path_id: i32,
 	pub name: String,
 	pub hidden: bool,
-	pub favourite: bool,
+	pub favorite: bool,
 	pub important: bool,
 	pub note: Option<String>,
 	pub date_created: chrono::DateTime<FixedOffset>,
@@ -87,9 +87,12 @@ impl StatefulJob for FileEncryptorJob {
 		if !info.path_data.is_dir {
 			// handle overwriting checks, and making sure there's enough available space
 
-			let user_key = key_manager.access_keymount(state.init.key_uuid)?.hashed_key;
+			let user_key = key_manager
+				.access_keymount(state.init.key_uuid)
+				.await?
+				.hashed_key;
 
-			let user_key_details = key_manager.access_keystore(state.init.key_uuid)?;
+			let user_key_details = key_manager.access_keystore(state.init.key_uuid).await?;
 
 			let output_path = state.init.output_path.clone().map_or_else(
 				|| {
@@ -155,7 +158,7 @@ impl StatefulJob for FileEncryptorJob {
 							path_id: state.init.path_id,
 							name: info.path_data.materialized_path.clone(),
 							hidden: object.hidden,
-							favourite: object.favorite,
+							favorite: object.favorite,
 							important: object.important,
 							note: object.note,
 							date_created: object.date_created,

crates/crypto/src/error.rs

@@ -16,36 +16,29 @@ pub type Result<T> = std::result::Result<T, Error>;
 /// This enum defines all possible errors that this crate can give
 #[derive(Error, Debug)]
 pub enum Error {
+	// crypto primitive errors (STREAM, hashing)
 	#[error("there was an error while password hashing")]
 	PasswordHash,
-	#[error("I/O error: {0}")]
-	Io(#[from] std::io::Error),
 	#[error("error while encrypting")]
 	Encrypt,
 	#[error("error while decrypting")]
 	Decrypt,
 	#[error("nonce length mismatch")]
 	NonceLengthMismatch,
-	#[error("invalid file header")]
-	FileHeader,
 	#[error("error initialising stream encryption/decryption")]
 	StreamModeInit,
-	#[error("wrong password provided")]
-	IncorrectPassword,
+
+	// header errors
 	#[error("no keyslots available")]
 	NoKeyslots,
-	#[error("mismatched data length while converting vec to array")]
-	VecArrSizeMismatch,
-	#[error("error while parsing preview media length")]
-	MediaLengthParse,
 	#[error("no preview media found")]
 	NoPreviewMedia,
-	#[error("error while serializing/deserializing an item")]
-	Serialization,
 	#[error("no metadata found")]
 	NoMetadata,
 	#[error("tried adding too many keyslots to a header")]
 	TooManyKeyslots,
+
+	// key manager
 	#[error("requested key wasn't found in the key manager")]
 	KeyNotFound,
 	#[error("key is already mounted")]
@@ -58,38 +51,34 @@ pub enum Error {
 	KeyAlreadyQueued,
 	#[error("no default key has been set")]
 	NoDefaultKeySet,
-	#[error("no master password has been provided to the keymanager")]
-	NoMasterPassword,
-	#[error("mismatch between supplied keys and the keystore")]
-	KeystoreMismatch,
-	#[error("mutex lock error")]
-	MutexLock,
+	#[error("keymanager is not unlocked")]
+	NotUnlocked,
 	#[error("no verification key")]
 	NoVerificationKey,
 	#[error("key isn't flagged as memory only")]
 	KeyNotMemoryOnly,
-	#[error("wrong information provided to the key manager")]
-	IncorrectKeymanagerDetails,
+
+	// general errors
+	#[error("I/O error: {0}")]
+	Io(#[from] std::io::Error),
+	#[error("mismatched data length while converting vec to array")]
+	VecArrSizeMismatch,
+	#[error("incorrect password/details were provided")]
+	IncorrectPassword,
+	#[error("error while serializing/deserializing an item")]
+	Serialization,
 	#[error("string parse error")]
 	StringParse(#[from] FromUtf8Error),
 
+	// keyring
 	#[cfg(target_os = "linux")]
 	#[error("error with the linux keyring: {0}")]
 	LinuxKeyringError(#[from] secret_service::Error),
-
 	#[cfg(any(target_os = "macos", target_os = "ios"))]
 	#[error("error with the apple keyring: {0}")]
 	AppleKeyringError(#[from] security_framework::base::Error),
-
 	#[error("generic keyring error")]
 	KeyringError,
-
 	#[error("keyring not available on this platform")]
 	KeyringNotSupported,
 }
-
-impl<T> From<std::sync::PoisonError<T>> for Error {
-	fn from(_: std::sync::PoisonError<T>) -> Self {
-		Self::MutexLock
-	}
-}

crates/crypto/src/header/file.rs

@@ -256,7 +256,7 @@ impl FileHeader {
 		reader.read_exact(&mut magic_bytes).await?;
 
 		if magic_bytes != MAGIC_BYTES {
-			return Err(Error::FileHeader);
+			return Err(Error::Serialization);
 		}
 
 		let mut version = [0u8; 2];

crates/crypto/src/keys/keymanager.rs

@@ -152,6 +152,28 @@ impl KeyManager {
 		Ok(())
 	}
 
+	// This verifies that the key manager is unlocked before continuing the calling function.
+	pub async fn ensure_unlocked(&self) -> Result<()> {
+		self.is_unlocked()
+			.await
+			.then_some(())
+			.ok_or(Error::NotUnlocked)
+	}
+
+	// This verifies that the target key is not already queued before continuing the operation.
+	pub fn ensure_not_queued(&self, uuid: Uuid) -> Result<()> {
+		(!self.is_queued(uuid))
+			.then_some(())
+			.ok_or(Error::KeyAlreadyMounted)
+	}
+
+	// This verifies that the target key is not already mounted before continuing the operation.
+	pub fn ensure_not_mounted(&self, uuid: Uuid) -> Result<()> {
+		(!self.keymount.contains_key(&uuid))
+			.then_some(())
+			.ok_or(Error::KeyAlreadyMounted)
+	}
+
 	pub async fn keyring_retrieve(
 		&self,
 		library_uuid: Uuid,
@@ -319,6 +341,8 @@ impl KeyManager {
 
 	/// This function removes a key from the keystore, the keymount and it's unset as the default.
 	pub async fn remove_key(&self, uuid: Uuid) -> Result<()> {
+		self.ensure_unlocked().await?;
+
 		if self.keystore.contains_key(&uuid) {
 			// if key is default, clear it
 			// do this manually to prevent deadlocks
@@ -348,6 +372,8 @@ impl KeyManager {
 		hashing_algorithm: HashingAlgorithm,
 		library_uuid: Uuid,
 	) -> Result<StoredKey> {
+		self.ensure_unlocked().await?;
+
 		let secret_key = SecretKey::generate();
 		let content_salt = Salt::generate();
 
@@ -429,6 +455,8 @@ impl KeyManager {
 		secret_key: SecretKeyString,        // at the time of the backup
 		stored_keys: &[StoredKey],          // from the backup
 	) -> Result<Vec<StoredKey>> {
+		self.ensure_unlocked().await?;
+
 		// this backup should contain a verification key, which will tell us the algorithm+hashing algorithm
 		let secret_key = secret_key.into();
 
@@ -541,7 +569,7 @@ impl KeyManager {
 	///
 	/// Only provide the secret key if it should not/can not be sourced from an OS keychain (e.g. web, OS keychains not enabled/available, etc).
 	///
-	/// This minimises the risk of an attacker obtaining the master password, as both of these are required to unlock the vault (and both should be stored separately).
+	/// This minimizes the risk of an attacker obtaining the master password, as both of these are required to unlock the vault (and both should be stored separately).
 	///
 	/// Both values need to be correct, otherwise this function will return a generic error.
 	///
@@ -563,11 +591,7 @@ impl KeyManager {
 			.as_ref()
 			.map_or(Err(Error::NoVerificationKey), |k| Ok(k.clone()))?;
 
-		if self.is_unlocked().await? {
-			return Err(Error::KeyAlreadyMounted);
-		} else if self.is_queued(verification_key.uuid) {
-			return Err(Error::KeyAlreadyQueued);
-		}
+		self.ensure_not_queued(verification_key.uuid)?;
 
 		let secret_key = if let Some(secret_key) = provided_secret_key.clone() {
 			secret_key.into()
@@ -615,7 +639,7 @@ impl KeyManager {
 				.await
 				.map_err(|_| {
 					self.remove_from_queue(verification_key.uuid).ok();
-					Error::IncorrectKeymanagerDetails
+					Error::IncorrectPassword
 				})?;
 
 				*self.root_key.lock().await = Some(
@@ -663,11 +687,9 @@ impl KeyManager {
 	///
 	/// We could add a log to this, so that the user can view mounts
 	pub async fn mount(&self, uuid: Uuid) -> Result<()> {
-		if self.keymount.get(&uuid).is_some() {
-			return Err(Error::KeyAlreadyMounted);
-		} else if self.is_queued(uuid) {
-			return Err(Error::KeyAlreadyQueued);
-		}
+		self.ensure_unlocked().await?;
+		self.ensure_not_mounted(uuid)?;
+		self.ensure_not_queued(uuid)?;
 
 		if let Some(stored_key) = self.keystore.get(&uuid) {
 			match stored_key.version {
@@ -738,6 +760,8 @@ impl KeyManager {
 	///
 	/// The master password/salt needs to be present, so we are able to decrypt the key itself from the stored key.
 	pub async fn get_key(&self, uuid: Uuid) -> Result<Password> {
+		self.ensure_unlocked().await?;
+
 		if let Some(stored_key) = self.keystore.get(&uuid) {
 			let master_key = StreamDecryption::decrypt_bytes(
 				Key::derive(
@@ -790,6 +814,8 @@ impl KeyManager {
 		automount: bool,
 		content_salt: Option<Salt>,
 	) -> Result<Uuid> {
+		self.ensure_unlocked().await?;
+
 		let uuid = Uuid::new_v4();
 
 		// Generate items we'll need for encryption
@@ -851,14 +877,18 @@ impl KeyManager {
 	/// This function is for accessing the internal keymount.
 	///
 	/// We could add a log to this, so that the user can view accesses
-	pub fn access_keymount(&self, uuid: Uuid) -> Result<MountedKey> {
+	pub async fn access_keymount(&self, uuid: Uuid) -> Result<MountedKey> {
+		self.ensure_unlocked().await?;
+
 		self.keymount
 			.get(&uuid)
 			.map_or(Err(Error::KeyNotFound), |v| Ok(v.clone()))
 	}
 
 	/// This function is for accessing a `StoredKey`.
-	pub fn access_keystore(&self, uuid: Uuid) -> Result<StoredKey> {
+	pub async fn access_keystore(&self, uuid: Uuid) -> Result<StoredKey> {
+		self.ensure_unlocked().await?;
+
 		self.keystore
 			.get(&uuid)
 			.map_or(Err(Error::KeyNotFound), |v| Ok(v.clone()))
@@ -866,6 +896,8 @@ impl KeyManager {
 
 	/// This allows you to set the default key
 	pub async fn set_default(&self, uuid: Uuid) -> Result<()> {
+		self.ensure_unlocked().await?;
+
 		if self.keystore.contains_key(&uuid) {
 			*self.default.lock().await = Some(uuid);
 			Ok(())
@@ -876,16 +908,14 @@ impl KeyManager {
 
 	/// This allows you to get the default key's ID
 	pub async fn get_default(&self) -> Result<Uuid> {
+		self.ensure_unlocked().await?;
+
 		self.default.lock().await.ok_or(Error::NoDefaultKeySet)
 	}
 
 	/// This should ONLY be used internally.
 	async fn get_root_key(&self) -> Result<Key> {
-		self.root_key
-			.lock()
-			.await
-			.clone()
-			.ok_or(Error::NoMasterPassword)
+		self.root_key.lock().await.clone().ok_or(Error::NotUnlocked)
 	}
 
 	pub async fn get_verification_key(&self) -> Result<StoredKey> {
@@ -896,13 +926,17 @@ impl KeyManager {
 			.ok_or(Error::NoVerificationKey)
 	}
 
-	pub fn is_memory_only(&self, uuid: Uuid) -> Result<bool> {
+	pub async fn is_memory_only(&self, uuid: Uuid) -> Result<bool> {
+		self.ensure_unlocked().await?;
+
 		self.keystore
 			.get(&uuid)
 			.map_or(Err(Error::KeyNotFound), |v| Ok(v.memory_only))
 	}
 
-	pub fn change_automount_status(&self, uuid: Uuid, status: bool) -> Result<()> {
+	pub async fn change_automount_status(&self, uuid: Uuid, status: bool) -> Result<()> {
+		self.ensure_unlocked().await?;
+
 		let updated_key = self
 			.keystore
 			.get(&uuid)
@@ -933,8 +967,8 @@ impl KeyManager {
 	/// This function is for converting a memory-only key to a saved key which syncs to the library.
 	///
 	/// The returned value needs to be written to the database.
-	pub fn sync_to_database(&self, uuid: Uuid) -> Result<StoredKey> {
-		if !self.is_memory_only(uuid)? {
+	pub async fn sync_to_database(&self, uuid: Uuid) -> Result<StoredKey> {
+		if !self.is_memory_only(uuid).await? {
 			return Err(Error::KeyNotMemoryOnly);
 		}
 
@@ -961,8 +995,8 @@ impl KeyManager {
 	}
 
 	/// This function is used for checking if the key manager is unlocked.
-	pub async fn is_unlocked(&self) -> Result<bool> {
-		Ok(self.root_key.lock().await.is_some())
+	pub async fn is_unlocked(&self) -> bool {
+		self.root_key.lock().await.is_some()
 	}
 
 	/// This function is used for unmounting all keys at once.
@@ -985,12 +1019,10 @@ impl KeyManager {
 	/// This function returns a Vec of `StoredKey`s, so you can write them somewhere/update the database with them/etc
 	///
 	/// The database and keystore should be in sync at ALL times (unless the user chose an in-memory only key)
-	#[must_use]
 	pub fn dump_keystore(&self) -> Vec<StoredKey> {
 		self.keystore.iter().map(|key| key.clone()).collect()
 	}
 
-	#[must_use]
 	pub fn get_mounted_uuids(&self) -> Vec<Uuid> {
 		self.keymount.iter().map(|key| key.uuid).collect()
 	}

packages/interface/src/components/explorer/ExplorerContextMenu.tsx

@@ -348,7 +348,7 @@ export function FileItemContextMenu({ data, ...props }: FileItemContextMenuProps
 								dialogManager.create((dp) => (
 									<DecryptFileDialog
 										{...dp}
-										location_id={useExplorerStore().locationId!}
+										location_id={store.locationId!}
 										path_id={data.item.id}
 									/>
 								));

packages/interface/src/components/key/KeyManager.tsx

@@ -140,9 +140,11 @@ export function KeyManager(props: KeyManagerProps) {
 							</ButtonLink>
 						</Tabs.List>
 					</div>
-					<Tabs.Content value="keys">
-						<KeyList />
-					</Tabs.Content>
+					{isUnlocked && (
+						<Tabs.Content value="keys">
+							<KeyList />
+						</Tabs.Content>
+					)}
 					<Tabs.Content value="mount">
 						<KeyMounter />
 					</Tabs.Content>

packages/interface/src/screens/settings/library/KeysSetting.tsx

@@ -210,9 +210,11 @@ export default function KeysSettings() {
 						}
 					/>
 
-					<div className="grid space-y-2">
-						<ListOfKeys />
-					</div>
+					{isUnlocked && (
+						<div className="grid space-y-2">
+							<ListOfKeys />
+						</div>
+					)}
 
 					{keyringSk?.data && (
 						<>