From 3e26ace4189749aba49a6f65d3f76577d9b179a2 Mon Sep 17 00:00:00 2001 From: Dmitry Yakimenko Date: Sat, 25 Oct 2025 23:43:56 +0200 Subject: [PATCH] Remove sensitive info from debug! and wrap debug! in a new macro that is controlled by a compile time const to make sure nothing sensitive is dumped in release --- .../bitwarden_chromium_importer/src/abe.rs | 8 +- .../bin/bitwarden_chromium_import_helper.rs | 76 +++++++++++-------- 2 files changed, 48 insertions(+), 36 deletions(-) diff --git a/apps/desktop/desktop_native/bitwarden_chromium_importer/src/abe.rs b/apps/desktop/desktop_native/bitwarden_chromium_importer/src/abe.rs index 9508e0bb776..9081a86188e 100644 --- a/apps/desktop/desktop_native/bitwarden_chromium_importer/src/abe.rs +++ b/apps/desktop/desktop_native/bitwarden_chromium_importer/src/abe.rs @@ -88,11 +88,13 @@ where } Ok(bytes_read) => { let message = String::from_utf8_lossy(&buffer[..bytes_read]); - - debug!("Received from client: '{}' ({} bytes)", message, bytes_read); + let preview = message.chars().take(16).collect::(); + debug!( + "Received from client: '{}...' ({} bytes)", + preview, bytes_read, + ); let response = process_message(&message); - match client.write_all(response.as_bytes()).await { Ok(_) => { debug!("Sent response to client ({} bytes)", response.len()); diff --git a/apps/desktop/desktop_native/bitwarden_chromium_importer/src/bin/bitwarden_chromium_import_helper.rs b/apps/desktop/desktop_native/bitwarden_chromium_importer/src/bin/bitwarden_chromium_import_helper.rs index dc8b4ef5fd2..26d317e52c0 100644 --- a/apps/desktop/desktop_native/bitwarden_chromium_importer/src/bin/bitwarden_chromium_import_helper.rs +++ b/apps/desktop/desktop_native/bitwarden_chromium_importer/src/bin/bitwarden_chromium_import_helper.rs @@ -72,19 +72,28 @@ mod windows_binary { // List of SYSTEM process names to try to impersonate const SYSTEM_PROCESS_NAMES: [&str; 2] = ["services.exe", "winlogon.exe"]; + // Macro wrapper around debug! that compiles to no-op when NEED_LOGGING is false + macro_rules! dbg_log { + ($($arg:tt)*) => { + if NEED_LOGGING { + debug!($($arg)*); + } + }; + } + async fn open_pipe_client(pipe_name: &'static str) -> Result { // TODO: Don't loop forever, but retry a few times let client = loop { match ClientOptions::new().open(pipe_name) { Ok(client) => { - debug!("Successfully connected to the pipe!"); + dbg_log!("Successfully connected to the pipe!"); break client; } Err(e) if e.raw_os_error() == Some(ERROR_PIPE_BUSY.0 as i32) => { - debug!("Pipe is busy, retrying in 50ms..."); + dbg_log!("Pipe is busy, retrying in 50ms..."); } Err(e) => { - debug!("Failed to connect to pipe: {}", &e); + dbg_log!("Failed to connect to pipe: {}", &e); return Err(e.into()); } } @@ -123,15 +132,15 @@ mod windows_binary { } fn resolve_process_executable_path(pid: u32) -> Result { - debug!("Resolving process executable path for PID {}", pid); + dbg_log!("Resolving process executable path for PID {}", pid); // Open the process handle let hprocess = unsafe { OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, false, pid) }?; - debug!("Opened process handle for PID {}", pid); + dbg_log!("Opened process handle for PID {}", pid); let _guard = guard(hprocess, |_| unsafe { - debug!("Closing process handle for PID {}", pid); + dbg_log!("Closing process handle for PID {}", pid); _ = CloseHandle(hprocess); }); @@ -145,7 +154,7 @@ mod windows_binary { &mut size, ) }?; - debug!("QueryFullProcessImageNameW returned {} bytes", size); + dbg_log!("QueryFullProcessImageNameW returned {} bytes", size); wide.truncate(size as usize); Ok(PathBuf::from(OsString::from_wide(&wide))) @@ -165,10 +174,10 @@ mod windows_binary { } fn decrypt_data_base64(data_base64: &str, expect_appb: bool) -> Result { - debug!("Decrypting data base64: {}", data_base64); + dbg_log!("Decrypting data base64: {}", data_base64); let data = general_purpose::STANDARD.decode(data_base64).map_err(|e| { - debug!("Failed to decode base64: {} APPB: {}", e, expect_appb); + dbg_log!("Failed to decode base64: {} APPB: {}", e, expect_appb); e })?; @@ -180,7 +189,7 @@ mod windows_binary { fn decrypt_data(data: &[u8], expect_appb: bool) -> Result> { if expect_appb && !data.starts_with(b"APPB") { - debug!("Decoded data does not start with 'APPB'"); + dbg_log!("Decoded data does not start with 'APPB'"); return Err(anyhow!("Decoded data does not start with 'APPB'")); } @@ -218,7 +227,7 @@ mod windows_binary { Ok(decrypted) } else { - debug!("CryptUnprotectData failed"); + dbg_log!("CryptUnprotectData failed"); Err(anyhow!("CryptUnprotectData failed")) } } @@ -250,7 +259,7 @@ mod windows_binary { unsafe { ImpersonateLoggedOnUser(sys_token)?; }; - debug!("Impersonating system process '{}' (PID: {})", name, pid); + dbg_log!("Impersonating system process '{}' (PID: {})", name, pid); Ok(Self { sys_token_handle: sys_token, @@ -278,13 +287,13 @@ mod windows_binary { fn enable_privilege() -> Result<()> { let mut previous_value = BOOL(0); let status = unsafe { - debug!("Setting SE_DEBUG_PRIVILEGE to 1 via RtlAdjustPrivilege"); + dbg_log!("Setting SE_DEBUG_PRIVILEGE to 1 via RtlAdjustPrivilege"); RtlAdjustPrivilege(SE_DEBUG_PRIVILEGE, BOOL(1), BOOL(0), &mut previous_value) }; if status != STATUS_SUCCESS { return Err(anyhow!("Failed to adjust privilege")); } - debug!( + dbg_log!( "SE_DEBUG_PRIVILEGE set to 1, was {} before", previous_value.0 ); @@ -297,9 +306,10 @@ mod windows_binary { for (pid, name) in pids { match Self::get_system_token_from_pid(pid) { Err(_) => { - debug!( + dbg_log!( "Failed to open process handle '{}' (PID: {}), skipping", - name, pid + name, + pid ); continue; } @@ -373,12 +383,12 @@ mod windows_binary { if NEED_SERVER_SIGNATURE_VALIDATION { let server_pid = get_named_pipe_server_pid(&client)?; - debug!("Connected to pipe server PID {}", server_pid); + dbg_log!("Connected to pipe server PID {}", server_pid); // Validate the server end process signature let exe_path = resolve_process_executable_path(server_pid)?; - debug!("Pipe server executable path: {}", exe_path.display()); + dbg_log!("Pipe server executable path: {}", exe_path.display()); let verifier = CodeSignVerifier::for_file(exe_path.as_path()).map_err(|e| { anyhow!("verifysign init failed for {}: {:?}", exe_path.display(), e) @@ -392,28 +402,28 @@ mod windows_binary { ) })?; - debug!("Pipe server executable path: {}", exe_path.display()); + dbg_log!("Pipe server executable path: {}", exe_path.display()); // Dump signature fields for debugging/inspection - debug!("Signature fields:"); - debug!(" Subject Name: {:?}", signature.subject_name()); - debug!(" Issuer Name: {:?}", signature.issuer_name()); - debug!(" SHA1 Thumbprint: {:?}", signature.sha1_thumbprint()); - debug!(" SHA256 Thumbprint: {:?}", signature.sha256_thumbprint()); - debug!(" Serial Number: {:?}", signature.serial()); + dbg_log!("Signature fields:"); + dbg_log!(" Subject Name: {:?}", signature.subject_name()); + dbg_log!(" Issuer Name: {:?}", signature.issuer_name()); + dbg_log!(" SHA1 Thumbprint: {:?}", signature.sha1_thumbprint()); + dbg_log!(" SHA256 Thumbprint: {:?}", signature.sha256_thumbprint()); + dbg_log!(" Serial Number: {:?}", signature.serial()); if signature.sha256_thumbprint() != EXPECTED_SERVER_SIGNATURE_SHA256_THUMBPRINT { return Err(anyhow!("Pipe server signature is not valid")); } - debug!("Pipe server signature verified for PID {}", server_pid); + dbg_log!("Pipe server signature verified for PID {}", server_pid); } Ok(client) } fn run() -> Result { - debug!("Starting bitwarden_chromium_import_helper.exe"); + dbg_log!("Starting bitwarden_chromium_import_helper.exe"); let args = Args::try_parse()?; @@ -421,13 +431,13 @@ mod windows_binary { return Err(anyhow!("Expected to run with admin privileges")); } - debug!("Running as ADMINISTRATOR"); + dbg_log!("Running as ADMINISTRATOR"); // Impersonate a SYSTEM process to be able to decrypt data encrypted for the machine let system_decrypted_base64 = { let _guard = ImpersonateGuard::start()?; let system_decrypted_base64 = decrypt_data_base64(&args.encrypted, true)?; - debug!("Decrypted data with system"); + dbg_log!("Decrypted data with system"); system_decrypted_base64 }; @@ -438,11 +448,11 @@ mod windows_binary { // We don't send this result back since the library will decrypt again at USER level. _ = decrypt_data_base64(&system_decrypted_base64, false).map_err(|e| { - debug!("User level decryption check failed: {}", e); + dbg_log!("User level decryption check failed: {}", e); e })?; - debug!("User level decryption check passed"); + dbg_log!("User level decryption check passed"); Ok(system_decrypted_base64) } @@ -476,11 +486,11 @@ mod windows_binary { match run() { Ok(system_decrypted_base64) => { - debug!("Sending response back to user"); + dbg_log!("Sending response back to user"); let _ = send_to_user(&mut client, &system_decrypted_base64).await; } Err(e) => { - debug!("Error: {}", e); + dbg_log!("Error: {}", e); send_error_to_user(&mut client, &format!("{}", e)).await; } }