[PM-27897] Fix release before use in chromium importer (#17276)

We ran into some weird issues where the memory was corrupted on certain architectures. It turns out we free'd memory before using it.

This ensures we make a copy of the data before freeing it, and extracts a common function for both crates to use.

apps/desktop/desktop_native/bitwarden_chromium_import_helper/src/windows/crypto.rs

@@ -2,17 +2,14 @@ use aes_gcm::{aead::Aead, Aes256Gcm, Key, KeyInit};
 use anyhow::{anyhow, Result};
 use base64::{engine::general_purpose, Engine as _};
 use chacha20poly1305::ChaCha20Poly1305;
+use chromium_importer::chromium::crypt_unprotect_data;
 use scopeguard::defer;
 use tracing::debug;
 use windows::{
     core::w,
-    Win32::{
-        Foundation::{LocalFree, HLOCAL},
-        Security::Cryptography::{
-            self, CryptUnprotectData, NCryptOpenKey, NCryptOpenStorageProvider, CERT_KEY_SPEC,
-            CRYPTPROTECT_UI_FORBIDDEN, CRYPT_INTEGER_BLOB, NCRYPT_FLAGS, NCRYPT_KEY_HANDLE,
-            NCRYPT_PROV_HANDLE, NCRYPT_SILENT_FLAG,
-        },
+    Win32::Security::Cryptography::{
+        self, NCryptOpenKey, NCryptOpenStorageProvider, CERT_KEY_SPEC, CRYPTPROTECT_UI_FORBIDDEN,
+        NCRYPT_FLAGS, NCRYPT_KEY_HANDLE, NCRYPT_PROV_HANDLE, NCRYPT_SILENT_FLAG,
     },
 };
 
@@ -71,38 +68,7 @@ fn decrypt_with_dpapi(data: &[u8], expect_appb: bool) -> Result<Vec<u8>> {
 
     let data = if expect_appb { &data[4..] } else { data };
 
-    let in_blob = CRYPT_INTEGER_BLOB {
-        cbData: data.len() as u32,
-        pbData: data.as_ptr() as *mut u8,
-    };
-
-    let mut out_blob = CRYPT_INTEGER_BLOB::default();
-
-    let result = unsafe {
-        CryptUnprotectData(
-            &in_blob,
-            None,
-            None,
-            None,
-            None,
-            CRYPTPROTECT_UI_FORBIDDEN,
-            &mut out_blob,
-        )
-    };
-
-    if result.is_ok() && !out_blob.pbData.is_null() && out_blob.cbData > 0 {
-        let decrypted = unsafe {
-            std::slice::from_raw_parts(out_blob.pbData, out_blob.cbData as usize).to_vec()
-        };
-
-        // Free the memory allocated by CryptUnprotectData
-        unsafe { LocalFree(Some(HLOCAL(out_blob.pbData as *mut _))) };
-
-        Ok(decrypted)
-    } else {
-        debug!("CryptUnprotectData failed");
-        Err(anyhow!("CryptUnprotectData failed"))
-    }
+    crypt_unprotect_data(data, CRYPTPROTECT_UI_FORBIDDEN)
 }
 
 //

apps/desktop/desktop_native/chromium_importer/src/chromium/mod.rs

@@ -10,7 +10,7 @@ use rusqlite::{params, Connection};
 mod platform;
 
 #[cfg(target_os = "windows")]
-pub use platform::{verify_signature, ADMIN_TO_USER_PIPE_NAME};
+pub use platform::*;
 
 pub(crate) use platform::SUPPORTED_BROWSERS as PLATFORM_SUPPORTED_BROWSERS;
 

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/windows/crypto.rs

@@ -0,0 +1,54 @@
+use anyhow::{anyhow, Result};
+use windows::Win32::{
+    Foundation::{LocalFree, HLOCAL},
+    Security::Cryptography::{CryptUnprotectData, CRYPT_INTEGER_BLOB},
+};
+
+/// Rust friendly wrapper around CryptUnprotectData
+///
+/// Decrypts the data passed in using the `CryptUnprotectData` api.
+pub fn crypt_unprotect_data(data: &[u8], flags: u32) -> Result<Vec<u8>> {
+    if data.is_empty() {
+        return Ok(Vec::new());
+    }
+
+    let data_in = CRYPT_INTEGER_BLOB {
+        cbData: data.len() as u32,
+        pbData: data.as_ptr() as *mut u8,
+    };
+
+    let mut data_out = CRYPT_INTEGER_BLOB::default();
+
+    let result = unsafe {
+        CryptUnprotectData(
+            &data_in,
+            None,  // ppszDataDescr: Option<*mut PWSTR>
+            None,  // pOptionalEntropy: Option<*const CRYPT_INTEGER_BLOB>
+            None,  // pvReserved: Option<*const std::ffi::c_void>
+            None,  // pPromptStruct: Option<*const CRYPTPROTECT_PROMPTSTRUCT>
+            flags, // dwFlags: u32
+            &mut data_out,
+        )
+    };
+
+    if result.is_err() {
+        return Err(anyhow!("CryptUnprotectData failed"));
+    }
+
+    if data_out.pbData.is_null() || data_out.cbData == 0 {
+        return Ok(Vec::new());
+    }
+
+    let output_slice =
+        unsafe { std::slice::from_raw_parts(data_out.pbData, data_out.cbData as usize) };
+
+    // SAFETY: Must copy data before calling LocalFree() below.
+    // Calling to_vec() after LocalFree() causes use-after-free bugs.
+    let output = output_slice.to_vec();
+
+    unsafe {
+        LocalFree(Some(HLOCAL(data_out.pbData as *mut _)));
+    }
+
+    Ok(output)
+}

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/windows/mod.rs

@@ -3,18 +3,16 @@ use anyhow::{anyhow, Result};
 use async_trait::async_trait;
 use base64::{engine::general_purpose::STANDARD as BASE64_STANDARD, Engine as _};
 use std::path::{Path, PathBuf};
-use windows::Win32::{
-    Foundation::{LocalFree, HLOCAL},
-    Security::Cryptography::{CryptUnprotectData, CRYPT_INTEGER_BLOB},
-};
 
 use crate::chromium::{BrowserConfig, CryptoService, LocalState};
 use crate::util;
 mod abe;
 mod abe_config;
+mod crypto;
 mod signature;
 
 pub use abe_config::ADMIN_TO_USER_PIPE_NAME;
+pub use crypto::*;
 pub use signature::*;
 
 //
@@ -166,7 +164,7 @@ impl WindowsCryptoService {
             return Err(anyhow!("Encrypted master key is not encrypted with DPAPI"));
         }
 
-        let key = unprotect_data_win(&key_bytes[5..])
+        let key = crypt_unprotect_data(&key_bytes[5..], 0)
             .map_err(|e| anyhow!("Failed to unprotect the master key: {}", e))?;
 
         Ok(key)
@@ -209,53 +207,6 @@ impl WindowsCryptoService {
     }
 }
 
-fn unprotect_data_win(data: &[u8]) -> Result<Vec<u8>> {
-    if data.is_empty() {
-        return Ok(Vec::new());
-    }
-
-    let data_in = CRYPT_INTEGER_BLOB {
-        cbData: data.len() as u32,
-        pbData: data.as_ptr() as *mut u8,
-    };
-
-    let mut data_out = CRYPT_INTEGER_BLOB {
-        cbData: 0,
-        pbData: std::ptr::null_mut(),
-    };
-
-    let result = unsafe {
-        CryptUnprotectData(
-            &data_in,
-            None, // ppszDataDescr: Option<*mut PWSTR>
-            None, // pOptionalEntropy: Option<*const CRYPT_INTEGER_BLOB>
-            None, // pvReserved: Option<*const std::ffi::c_void>
-            None, // pPromptStruct: Option<*const CRYPTPROTECT_PROMPTSTRUCT>
-            0,    // dwFlags: u32
-            &mut data_out,
-        )
-    };
-
-    if result.is_err() {
-        return Err(anyhow!("CryptUnprotectData failed"));
-    }
-
-    if data_out.pbData.is_null() || data_out.cbData == 0 {
-        return Ok(Vec::new());
-    }
-
-    let output_slice =
-        unsafe { std::slice::from_raw_parts(data_out.pbData, data_out.cbData as usize) };
-
-    unsafe {
-        if !data_out.pbData.is_null() {
-            LocalFree(Some(HLOCAL(data_out.pbData as *mut std::ffi::c_void)));
-        }
-    }
-
-    Ok(output_slice.to_vec())
-}
-
 fn get_admin_exe_path() -> Result<PathBuf> {
     let current_exe_full_path = std::env::current_exe()
         .map_err(|e| anyhow!("Failed to get current executable path: {}", e))?;