[PM-8657] Handle attachments with cipher keys (#820)

## 📔 Objective - Added support for decrypting attachments without keys, for backwards compatibility - Updated `move_to_organization` and `generate_cipher_key` to reencrypt attachment keys when possible or error out when it's not. - Fix `generate_cipher_key` incorrectly encrypting the new cipher key with the old cipher key when the cipher already had a key. ## ⏰ 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  - 👍 (`:+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 --------- Co-authored-by: Hinton <[email protected]>
bitwarden · Jun 6, 2024 · 3c16656 · 3c16656
1 parent 2bd26f4
commit 3c16656
Show file tree

Hide file tree

Showing 4 changed files with 490 additions and 20 deletions.
diff --git a/crates/bitwarden/src/client/test_accounts.rs b/crates/bitwarden/src/client/test_accounts.rs
@@ -14,6 +14,11 @@ impl Client {
     pub async fn init_test_account(account: TestAccount) -> Self {
         let mut client = Client::new(None);
 
+        client.load_flags(HashMap::from([(
+            "enableCipherKeyEncryption".to_owned(),
+            true,
+        )]));
+
         initialize_user_crypto(&mut client, account.user)
             .await
             .unwrap();

diff --git a/crates/bitwarden/src/mobile/vault/client_ciphers.rs b/crates/bitwarden/src/mobile/vault/client_ciphers.rs
@@ -82,7 +82,7 @@ mod tests {
     use super::*;
     use crate::{
         client::test_accounts::test_bitwarden_com_account,
-        vault::{login::Login, CipherRepromptType, CipherType},
+        vault::{login::Login, Attachment, CipherRepromptType, CipherType},
     };
 
     #[tokio::test]
@@ -126,4 +126,221 @@ mod tests {
 
         assert_eq!(dec[0].name, "Test item");
     }
+
+    fn test_cipher() -> Cipher {
+        Cipher {
+    id: Some("358f2b2b-9326-4e5e-94a8-b18100bb0908".parse().unwrap()),
+            organization_id: None,
+            folder_id: None,
+            collection_ids: vec![],
+            key: None,
+            name: "2.+oPT8B4xJhyhQRe1VkIx0A==|PBtC/bZkggXR+fSnL/pG7g==|UkjRD0VpnUYkjRC/05ZLdEBAmRbr3qWRyJey2bUvR9w=".parse().unwrap(),
+            notes: None,
+            r#type: CipherType::Login,
+            login: Some(Login{
+                username: None,
+                password: None,
+                password_revision_date: None,
+                uris:None,
+                totp: None,
+                autofill_on_page_load: None,
+                fido2_credentials: None,
+            }),
+            identity: None,
+            card: None,
+            secure_note: None,
+            favorite: false,
+            reprompt: CipherRepromptType::None,
+            organization_use_totp: true,
+            edit: true,
+            view_password: true,
+            local_data: None,
+            attachments: None,
+            fields:  None,
+            password_history: None,
+            creation_date: "2024-05-31T11:20:58.4566667Z".parse().unwrap(),
+            deleted_date: None,
+            revision_date: "2024-05-31T11:20:58.4566667Z".parse().unwrap(),
+        }
+    }
+
+    fn test_attachment_legacy() -> Attachment {
+        Attachment {
+            id: Some("uf7bkexzag04d3cw04jsbqqkbpbwhxs0".to_string()),
+            url: Some("http://localhost:4000/attachments//358f2b2b-9326-4e5e-94a8-b18100bb0908/uf7bkexzag04d3cw04jsbqqkbpbwhxs0".to_string()),
+            file_name: Some("2.mV50WiLq6duhwGbhM1TO0A==|dTufWNH8YTPP0EMlNLIpFA==|QHp+7OM8xHtEmCfc9QPXJ0Ro2BeakzvLgxJZ7NdLuDc=".parse().unwrap()),
+            key: None,
+            size: Some("65".to_string()),
+            size_name: Some("65 Bytes".to_string()),
+        }
+    }
+
+    fn test_attachment_v2() -> Attachment {
+        Attachment {
+            id: Some("a77m56oerrz5b92jm05lq5qoyj1xh2t9".to_string()),
+            url: Some("http://localhost:4000/attachments//358f2b2b-9326-4e5e-94a8-b18100bb0908/uf7bkexzag04d3cw04jsbqqkbpbwhxs0".to_string()),
+            file_name: Some("2.GhazFdCYQcM5v+AtVwceQA==|98bMUToqC61VdVsSuXWRwA==|bsLByMht9Hy5QO9pPMRz0K4d0aqBiYnnROGM5YGbNu4=".parse().unwrap()),
+            key: Some("2.6TPEiYULFg/4+3CpDRwCqw==|6swweBHCJcd5CHdwBBWuRN33XRV22VoroDFDUmiM4OzjPEAhgZK57IZS1KkBlCcFvT+t+YbsmDcdv+Lqr+iJ3MmzfJ40MCB5TfYy+22HVRA=|rkgFDh2IWTfPC1Y66h68Diiab/deyi1p/X0Fwkva0NQ=".parse().unwrap()),
+            size: Some("65".to_string()),
+            size_name: Some("65 Bytes".to_string()),
+        }
+    }
+
+    #[tokio::test]
+    async fn test_move_user_cipher_with_attachment_without_key_to_org_fails() {
+        let mut client = Client::init_test_account(test_bitwarden_com_account()).await;
+
+        let mut cipher = test_cipher();
+        cipher.attachments = Some(vec![test_attachment_legacy()]);
+
+        let view = client
+            .vault()
+            .ciphers()
+            .decrypt(cipher.clone())
+            .await
+            .unwrap();
+
+        //  Move cipher to organization
+        let res = client
+            .vault()
+            .ciphers()
+            .move_to_organization(
+                view,
+                "1bc9ac1e-f5aa-45f2-94bf-b181009709b8".parse().unwrap(),
+            )
+            .await;
+
+        assert!(res.is_err());
+    }
+
+    #[tokio::test]
+    async fn test_encrypt_cipher_with_legacy_attachment_without_key() {
+        let mut client = Client::init_test_account(test_bitwarden_com_account()).await;
+
+        let mut cipher = test_cipher();
+        let attachment = test_attachment_legacy();
+        cipher.attachments = Some(vec![attachment.clone()]);
+
+        let view = client
+            .vault()
+            .ciphers()
+            .decrypt(cipher.clone())
+            .await
+            .unwrap();
+
+        assert!(cipher.key.is_none());
+
+        // Assert the cipher has a key, and the attachment is still readable
+        let new_cipher = client.vault().ciphers().encrypt(view).await.unwrap();
+        assert!(new_cipher.key.is_some());
+
+        let view = client.vault().ciphers().decrypt(new_cipher).await.unwrap();
+        let attachments = view.clone().attachments.unwrap();
+        let attachment_view = attachments.first().unwrap().clone();
+        assert!(attachment_view.key.is_none());
+
+        assert_eq!(attachment_view.file_name.unwrap(), "h.txt");
+
+        let buf = vec![
+            2, 100, 205, 148, 152, 77, 184, 77, 53, 80, 38, 240, 83, 217, 251, 118, 254, 27, 117,
+            41, 148, 244, 216, 110, 216, 255, 104, 215, 23, 15, 176, 239, 208, 114, 95, 159, 23,
+            211, 98, 24, 145, 166, 60, 197, 42, 204, 131, 144, 253, 204, 195, 154, 27, 201, 215,
+            43, 10, 244, 107, 226, 152, 85, 167, 66, 185,
+        ];
+
+        let content = client
+            .vault()
+            .attachments()
+            .decrypt_buffer(cipher, attachment, buf.as_slice())
+            .await
+            .unwrap();
+
+        assert_eq!(content, b"Hello");
+    }
+
+    #[tokio::test]
+    async fn test_encrypt_cipher_with_v1_attachment_without_key() {
+        let mut client = Client::init_test_account(test_bitwarden_com_account()).await;
+
+        let mut cipher = test_cipher();
+        let attachment = test_attachment_v2();
+        cipher.attachments = Some(vec![attachment.clone()]);
+
+        let view = client
+            .vault()
+            .ciphers()
+            .decrypt(cipher.clone())
+            .await
+            .unwrap();
+
+        assert!(cipher.key.is_none());
+
+        // Assert the cipher has a key, and the attachment is still readable
+        let new_cipher = client.vault().ciphers().encrypt(view).await.unwrap();
+        assert!(new_cipher.key.is_some());
+
+        let view = client.vault().ciphers().decrypt(new_cipher).await.unwrap();
+        let attachments = view.clone().attachments.unwrap();
+        let attachment_view = attachments.first().unwrap().clone();
+        assert!(attachment_view.key.is_some());
+
+        // Ensure attachment key is updated since it's now protected by the cipher key
+        assert_ne!(
+            attachment.clone().key.unwrap().to_string(),
+            attachment_view.clone().key.unwrap().to_string()
+        );
+
+        assert_eq!(attachment_view.file_name.unwrap(), "h.txt");
+
+        let buf = vec![
+            2, 114, 53, 72, 20, 82, 18, 46, 48, 137, 97, 1, 100, 142, 120, 187, 28, 36, 180, 46,
+            189, 254, 133, 23, 169, 58, 73, 212, 172, 116, 185, 127, 111, 92, 112, 145, 99, 28,
+            158, 198, 48, 241, 121, 218, 66, 37, 152, 197, 122, 241, 110, 82, 245, 72, 47, 230, 95,
+            188, 196, 170, 127, 67, 44, 129, 90,
+        ];
+
+        let content = client
+            .vault()
+            .attachments()
+            .decrypt_buffer(cipher, attachment, buf.as_slice())
+            .await
+            .unwrap();
+
+        assert_eq!(content, b"Hello");
+
+        // Move cipher to organization
+        let new_view = client
+            .vault()
+            .ciphers()
+            .move_to_organization(
+                view,
+                "1bc9ac1e-f5aa-45f2-94bf-b181009709b8".parse().unwrap(),
+            )
+            .await
+            .unwrap();
+        let new_cipher = client.vault().ciphers().encrypt(new_view).await.unwrap();
+
+        let attachment = new_cipher
+            .clone()
+            .attachments
+            .unwrap()
+            .first()
+            .unwrap()
+            .clone();
+
+        // Ensure attachment key is still the same since it's protected by the cipher key
+        assert_eq!(
+            attachment.clone().key.unwrap().to_string(),
+            attachment_view.key.unwrap().to_string()
+        );
+
+        let content = client
+            .vault()
+            .attachments()
+            .decrypt_buffer(new_cipher, attachment, buf.as_slice())
+            .await
+            .unwrap();
+
+        assert_eq!(content, b"Hello");
+    }
 }
diff --git a/crates/bitwarden/src/vault/cipher/attachment.rs b/crates/bitwarden/src/vault/cipher/attachment.rs
@@ -43,6 +43,11 @@ pub struct AttachmentEncryptResult {
 pub struct AttachmentFile {
     pub cipher: Cipher,
     pub attachment: Attachment,
+
+    /// There are three different ways attachments are encrypted.
+    /// 1. UserKey / OrgKey (Contents) - Legacy
+    /// 2. AttachmentKey(Contents) - Pre CipherKey
+    /// 3. CipherKey(AttachmentKey(Contents)) - Current
     pub contents: EncString,
 }
 
@@ -80,15 +85,16 @@ impl KeyDecryptable<SymmetricCryptoKey, Vec<u8>> for AttachmentFile {
         let ciphers_key = Cipher::get_cipher_key(key, &self.cipher.key)?;
         let ciphers_key = ciphers_key.as_ref().unwrap_or(key);
 
-        let mut attachment_key: Vec<u8> = self
-            .attachment
-            .key
-            .as_ref()
-            .ok_or(CryptoError::MissingKey)?
-            .decrypt_with_key(ciphers_key)?;
-        let attachment_key = SymmetricCryptoKey::try_from(attachment_key.as_mut_slice())?;
+        // Version 2 or 3, `AttachmentKey` or `CipherKey(AttachmentKey)`
+        if let Some(attachment_key) = &self.attachment.key {
+            let mut content_key: Vec<u8> = attachment_key.decrypt_with_key(ciphers_key)?;
+            let content_key = SymmetricCryptoKey::try_from(content_key.as_mut_slice())?;
 
-        self.contents.decrypt_with_key(&attachment_key)
+            self.contents.decrypt_with_key(&content_key)
+        } else {
+            // Legacy attachment version 1, use user/org key
+            self.contents.decrypt_with_key(key)
+        }
     }
 }
 
@@ -145,7 +151,7 @@ mod tests {
 
     #[test]
     fn test_attachment_key() {
-        let user_key : SymmetricCryptoKey = "w2LO+nwV4oxwswVYCxlOfRUseXfvU03VzvKQHrqeklPgiMZrspUe6sOBToCnDn9Ay0tuCBn8ykVVRb7PWhub2Q==".to_string().try_into().unwrap();
+        let user_key: SymmetricCryptoKey = "w2LO+nwV4oxwswVYCxlOfRUseXfvU03VzvKQHrqeklPgiMZrspUe6sOBToCnDn9Ay0tuCBn8ykVVRb7PWhub2Q==".to_string().try_into().unwrap();
 
         let attachment = Attachment {
             id: None,
@@ -196,4 +202,58 @@ mod tests {
 
         assert_eq!(dec, original);
     }
+
+    #[test]
+    fn test_attachment_without_key() {
+        let user_key: SymmetricCryptoKey = "w2LO+nwV4oxwswVYCxlOfRUseXfvU03VzvKQHrqeklPgiMZrspUe6sOBToCnDn9Ay0tuCBn8ykVVRb7PWhub2Q==".to_string().try_into().unwrap();
+
+        let attachment = Attachment {
+            id: None,
+            url: None,
+            size: Some("161".into()),
+            size_name: Some("161 Bytes".into()),
+            file_name: Some("2.qTJdrgQe+tLCHTlPHMJXLw==|0we9+uYJPEY3FU5SblX2hg==|+fL/wTF/WgpoV+BNzmsmi284O0QNdVBUYmfqqs0CG1E=".parse().unwrap()),
+            key: None,
+        };
+
+        let cipher  = Cipher {
+            id: None,
+            organization_id: None,
+            folder_id: None,
+            collection_ids: Vec::new(),
+            key: None,
+            name: "2.d24xECyEdMZ3MG9s6SrGNw==|XvJlTeu5KJ22M3jKosy6iw==|8xGiQty4X61cDMx6PVqkJfSQ0ZTdA/5L9TpG7QfovoM=".parse().unwrap(),
+            notes: None,
+            r#type: CipherType::Login,
+            login: None,
+            identity: None,
+            card: None,
+            secure_note: None,
+            favorite: false,
+            reprompt: CipherRepromptType::None,
+            organization_use_totp: false,
+            edit: true,
+            view_password: true,
+            local_data: None,
+            attachments: None,
+            fields: None,
+            password_history: None,
+            creation_date: "2023-07-24T12:05:09.466666700Z".parse().unwrap(),
+            deleted_date: None,
+            revision_date: "2023-07-27T19:28:05.240Z".parse().unwrap(),
+        };
+
+        let enc_file = STANDARD.decode(b"AsQLXOBHrJ8porroTUlPxeJOm9XID7LL9D2+KwYATXEpR1EFjLBpcCvMmnqcnYLXIEefe9TCeY4Us50ux43kRSpvdB7YkjxDKV0O1/y6tB7qC4vvv9J9+O/uDEnMx/9yXuEhAW/LA/TsU/WAgxkOM0uTvm8JdD9LUR1z9Ql7zOWycMVzkvGsk2KBNcqAdrotS5FlDftZOXyU8pWecNeyA/w=").unwrap();
+        let original = STANDARD.decode(b"rMweTemxOL9D0iWWfRxiY3enxiZ5IrwWD6ef2apGO6MvgdGhy2fpwmATmn7BpSj9lRumddLLXm7u8zSp6hnXt1hS71YDNh78LjGKGhGL4sbg8uNnpa/I6GK/83jzqGYN7+ESbg==").unwrap();
+
+        let dec = AttachmentFile {
+            cipher,
+            attachment,
+            contents: EncString::from_buffer(&enc_file).unwrap(),
+        }
+        .decrypt_with_key(&user_key)
+        .unwrap();
+
+        assert_eq!(dec, original);
+    }
 }