[PM-27115] Force icon uri checksum verification on user crypto v2 (#519)

## 🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-27115
https://bitwarden.atlassian.net/browse/VULN-185
https://bitwarden.atlassian.net/browse/PM-4185

## 📔 Objective

Forces the icon URI check for crypto v2 users. This adds some plumbing
through the key store, which now also includes other non-key
cryptographic state. The account security version is expected to be used
in many other places that have access to the key store and we do not
want to pass through an additional struct to all places.

Please see the tickets for context about what specifically this change
achieves.

Further, this fixes the icon uri check to be constant time.

## ⏰ Reminders before review

- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or
informed the documentation
  team

## 🦮 Reviewer guidelines

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- 👍 (`:+1:`) or similar for great changes
- 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info
- ❓ (`:question:`) for questions
- 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry
that's not quite a confirmed
  issue and could potentially benefit from discussion
- 🎨 (`:art:`) for suggestions / improvements
- ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or
concerns needing attention
- 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or
indications of technical debt
- ⛏ (`:pick:`) for minor or nitpick changes

Author: quexten

Cargo.lock

@@ -73,6 +73,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"] }

Cargo.toml

@@ -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)]

crates/bitwarden-core/src/client/encryption_settings.rs

@@ -28,7 +28,9 @@ pub(crate) use master_password::{MasterPasswordAuthenticationData, MasterPasswor
 #[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")]

crates/bitwarden-core/src/key_management/mod.rs

@@ -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#"

crates/bitwarden-core/src/key_management/security_state.rs

@@ -55,7 +55,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"

crates/bitwarden-crypto/Cargo.toml

@@ -82,6 +82,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, ()>)>,
 }
@@ -122,6 +124,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) {

crates/bitwarden-crypto/src/store/context.rs

@@ -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.
@@ -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,
             })),
         }
     }
@@ -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;
+    }
+
     /// 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.
@@ -170,11 +178,14 @@ 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");
+        let security_state_version = data.security_state_version;
         KeyStoreContext {
-            global_keys: GlobalKeys::ReadOnly(self.inner.read().expect("RwLock is poisoned")),
+            global_keys: GlobalKeys::ReadOnly(data),
             local_symmetric_keys: create_store(),
             local_asymmetric_keys: create_store(),
             local_signing_keys: create_store(),
+            security_state_version,
             _phantom: std::marker::PhantomData,
         }
     }
@@ -200,11 +211,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,
         }
     }

crates/bitwarden-crypto/src/store/mod.rs

@@ -50,6 +50,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 }

crates/bitwarden-vault/Cargo.toml

@@ -8,7 +8,7 @@ use bitwarden_api_api::{
 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::{
@@ -554,7 +554,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();
         }