Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ serde_json = ">=1.0.96, <2.0"
serde_qs = ">=0.12.0, <0.16"
serde_repr = ">=0.1.12, <0.2"
serde-wasm-bindgen = ">=0.6.0, <0.7"
subtle = ">=2.5.0, <3.0"
syn = ">=2.0.87, <3"
thiserror = ">=1.0.40, <3"
tokio = { version = "1.36.0", features = ["macros"] }
Expand Down
1 change: 1 addition & 0 deletions crates/bitwarden-core/src/client/encryption_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ impl EncryptionSettings {
let security_state: SecurityState = security_state
.verify_and_unwrap(&signing_key.to_verifying_key())
.map_err(|_| EncryptionSettingsError::InvalidSecurityState)?;
store.set_security_state_version(security_state.version());
*sdk_security_state.write().expect("RwLock not poisoned") = Some(security_state);

#[allow(deprecated)]
Expand Down
4 changes: 3 additions & 1 deletion crates/bitwarden-core/src/key_management/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ pub(crate) use non_generic_wrappers::*;
#[cfg(feature = "internal")]
mod security_state;
#[cfg(feature = "internal")]
pub use security_state::{SecurityState, SignedSecurityState};
pub use security_state::{
MINIMUM_ENFORCE_ICON_URI_HASH_VERSION, SecurityState, SignedSecurityState,
};
#[cfg(feature = "internal")]
mod user_decryption;
#[cfg(feature = "internal")]
Expand Down
3 changes: 3 additions & 0 deletions crates/bitwarden-core/src/key_management/security_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ use serde::{Deserialize, Serialize};

use crate::UserId;

/// Icon URI hashes are enforced starting with this security state version.
pub const MINIMUM_ENFORCE_ICON_URI_HASH_VERSION: u64 = 2;

#[cfg(feature = "wasm")]
#[wasm_bindgen::prelude::wasm_bindgen(typescript_custom_section)]
const TS_CUSTOM_TYPES: &'static str = r#"
Expand Down
2 changes: 1 addition & 1 deletion crates/bitwarden-crypto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ serde_bytes = { workspace = true }
serde_repr.workspace = true
sha1 = ">=0.10.5, <0.11"
sha2 = ">=0.10.6, <0.11"
subtle = ">=2.5.0, <3.0"
subtle = { workspace = true }
thiserror = { workspace = true }
tsify = { workspace = true, optional = true }
typenum = ">=1.18.0, <1.19.0"
Expand Down
9 changes: 9 additions & 0 deletions crates/bitwarden-crypto/src/store/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ pub struct KeyStoreContext<'a, Ids: KeyIds> {
pub(super) local_asymmetric_keys: Box<dyn StoreBackend<Ids::Asymmetric>>,
pub(super) local_signing_keys: Box<dyn StoreBackend<Ids::Signing>>,

pub(super) security_state_version: u64,

// Make sure the context is !Send & !Sync
pub(super) _phantom: std::marker::PhantomData<(Cell<()>, RwLockReadGuard<'static, ()>)>,
}
Expand Down Expand Up @@ -121,6 +123,13 @@ impl<Ids: KeyIds> KeyStoreContext<'_, Ids> {
self.local_signing_keys.clear();
}

/// Returns the version of the security state of the key context. This describes the user's
/// encryption version and can be used to disable certain old / dangerous format features
/// safely.
pub fn get_security_state_version(&self) -> u64 {
self.security_state_version
}

/// Remove all symmetric keys from the context for which the predicate returns false
/// This will also remove the keys from the global store if this context has write access
pub fn retain_symmetric_keys(&mut self, f: fn(Ids::Symmetric) -> bool) {
Expand Down
15 changes: 14 additions & 1 deletion crates/bitwarden-crypto/src/store/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ struct KeyStoreInner<Ids: KeyIds> {
symmetric_keys: Box<dyn StoreBackend<Ids::Symmetric>>,
asymmetric_keys: Box<dyn StoreBackend<Ids::Asymmetric>>,
signing_keys: Box<dyn StoreBackend<Ids::Signing>>,
security_state_version: u64,
}

/// Create a new key store with the best available implementation for the current platform.
Expand All @@ -123,6 +124,7 @@ impl<Ids: KeyIds> Default for KeyStore<Ids> {
symmetric_keys: create_store(),
asymmetric_keys: create_store(),
signing_keys: create_store(),
security_state_version: 1,
})),
}
}
Expand All @@ -138,6 +140,12 @@ impl<Ids: KeyIds> KeyStore<Ids> {
keys.signing_keys.clear();
}

/// Sets the security state version for this store.
pub fn set_security_state_version(&self, version: u64) {
let mut data = self.inner.write().expect("RwLock is poisoned");
data.security_state_version = version;
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking question: Would we want to add some rollback protection here in the future? Something like:

if version < data.security_state_version {
    return Err(...);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! However, I do think this should be considered when sync and at least the crypto state is managed in SDK.

}

/// Initiate an encryption/decryption context. This context will have read only access to the
/// global keys, and will have its own local key stores with read/write access. This
/// context-local store will be cleared when the context is dropped.
Expand Down Expand Up @@ -170,11 +178,13 @@ impl<Ids: KeyIds> KeyStore<Ids> {
/// - [KeyStoreContext::encapsulate_key_unsigned]
/// - [KeyStoreContext::derive_shareable_key]
pub fn context(&'_ self) -> KeyStoreContext<'_, Ids> {
let data = self.inner.read().expect("RwLock is poisoned");
KeyStoreContext {
global_keys: GlobalKeys::ReadOnly(self.inner.read().expect("RwLock is poisoned")),
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: You can probably restructure this in the same way of context_mut to avoid calling inner.read() twice. it won't ever deadlock so this isn't a blocking problem, but it's mildly wasteful to do it twice if not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 178fe43

local_symmetric_keys: create_store(),
local_asymmetric_keys: create_store(),
local_signing_keys: create_store(),
security_state_version: data.security_state_version,
_phantom: std::marker::PhantomData,
}
}
Expand All @@ -200,11 +210,14 @@ impl<Ids: KeyIds> KeyStore<Ids> {
/// you're not holding references to the context across await points, as that would cause the
/// future to also not be [Send].
pub fn context_mut(&'_ self) -> KeyStoreContext<'_, Ids> {
let inner = self.inner.write().expect("RwLock is poisoned");
let security_state_version = inner.security_state_version;
KeyStoreContext {
global_keys: GlobalKeys::ReadWrite(self.inner.write().expect("RwLock is poisoned")),
global_keys: GlobalKeys::ReadWrite(inner),
local_symmetric_keys: create_store(),
local_asymmetric_keys: create_store(),
local_signing_keys: create_store(),
security_state_version,
_phantom: std::marker::PhantomData,
}
}
Expand Down
1 change: 1 addition & 0 deletions crates/bitwarden-vault/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ serde_json = { workspace = true }
serde_repr = { workspace = true }
sha1 = ">=0.10.5, <0.11"
sha2 = ">=0.10.6, <0.11"
subtle = { workspace = true }
thiserror = { workspace = true }
tsify = { workspace = true, optional = true }
uniffi = { workspace = true, optional = true }
Expand Down
7 changes: 5 additions & 2 deletions crates/bitwarden-vault/src/cipher/cipher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use bitwarden_api_api::models::{CipherDetailsResponseModel, CipherResponseModel}
use bitwarden_collections::collection::CollectionId;
use bitwarden_core::{
MissingFieldError, OrganizationId, UserId,
key_management::{KeyIds, SymmetricKeyId},
key_management::{KeyIds, MINIMUM_ENFORCE_ICON_URI_HASH_VERSION, SymmetricKeyId},
require,
};
use bitwarden_crypto::{
Expand Down Expand Up @@ -389,7 +389,10 @@ impl Decryptable<KeyIds, SymmetricKeyId, CipherView> for Cipher {
};

// For compatibility we only remove URLs with invalid checksums if the cipher has a key
if cipher.key.is_some() {
// or the user is on Crypto V2
if cipher.key.is_some()
|| ctx.get_security_state_version() >= MINIMUM_ENFORCE_ICON_URI_HASH_VERSION
{
cipher.remove_invalid_checksums();
}

Expand Down
3 changes: 2 additions & 1 deletion crates/bitwarden-vault/src/cipher/login.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use bitwarden_encoding::B64;
use chrono::{DateTime, Utc};
use serde::{Deserialize, Serialize};
use serde_repr::{Deserialize_repr, Serialize_repr};
use subtle::ConstantTimeEq;
#[cfg(feature = "wasm")]
use tsify::Tsify;
#[cfg(feature = "wasm")]
Expand Down Expand Up @@ -70,7 +71,7 @@ impl LoginUriView {
use sha2::Digest;
let uri_hash = sha2::Sha256::new().chain_update(uri.as_bytes()).finalize();

uri_hash.as_slice() == cs.as_bytes()
uri_hash.as_slice().ct_eq(cs.as_bytes()).into()
}

pub(crate) fn generate_checksum(&mut self) {
Expand Down
Loading