Moderate firefox Logic Error 🔧 Commit mapped

Overview

Moderate
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
Impactmoderate
DescriptionPrivilege escalation in the Crash Reporting component
ComponentToolkit
Bug ClassLogic Error
Tracker2059021
Fix commit6479dff71825 (firefox) +64/-57
CISA KEVNot listed
CreditedMozilla
Disclosed2026-09-15

Files Changed

  • toolkit/crashreporter/crash_helper_client/src/lib.rs
  • toolkit/crashreporter/crash_helper_client/src/platform/android.rs
  • toolkit/crashreporter/crash_helper_client/src/platform/unix.rs
  • toolkit/crashreporter/crash_helper_client/src/platform/windows.rs
  • toolkit/crashreporter/crash_helper_common/build.rs
  • toolkit/crashreporter/crash_helper_common/src/breakpad/windows_strings.rs
  • toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs
  • toolkit/crashreporter/crash_helper_common/src/platform/windows.rs
  • toolkit/crashreporter/crash_helper_server/src/ipc_server.rs
diff --git a/toolkit/crashreporter/crash_helper_client/src/lib.rs b/toolkit/crashreporter/crash_helper_client/src/lib.rs
index 8cd94b3eb11..0137218da1d 100644
--- a/toolkit/crashreporter/crash_helper_client/src/lib.rs
+++ b/toolkit/crashreporter/crash_helper_client/src/lib.rs
@@ -5,8 +5,7 @@
 use anyhow::{bail, Result};
 use crash_helper_common::{
     messages::{self},
-    ApplicationInfo, BreakpadString, GeckoChildId, IPCClientChannel, IPCConnector, ProcessHandle,
-    RawIPCConnector,
+    ApplicationInfo, BreakpadString, GeckoChildId, IPCClientChannel, IPCConnector, RawIPCConnector,
 };
 #[cfg(any(target_os = "android", target_os = "linux"))]
 use minidump_writer::minidump_writer::{AuxvType, DirectAuxvDumpInfo};
@@ -23,7 +22,6 @@ use std::{
         atomic::{AtomicBool, Ordering},
         OnceLock,
     },
-    thread::JoinHandle,
 };
 #[cfg(target_os = "windows")]
 use windows_sys::Win32::{
@@ -39,7 +37,6 @@ mod platform;
 
 pub struct CrashHelperClient {
     connector: IPCConnector,
-    spawner_thread: Option<JoinHandle<Result<ProcessHandle>>>,
     #[allow(unused)]
     pid: Pid,
 }
@@ -54,19 +51,6 @@ impl CrashHelperClient {
     fn register_child_process(&mut self, id: GeckoChildId) -> Result<IPCConnector> {
         let ipc_channel = IPCClientChannel::new()?;
         let (server_endpoint, client_endpoint) = ipc_channel.deconstruct();
-
-        if let Some(join_handle) = self.spawner_thread.take() {
-            let Ok(process_handle) = join_handle.join() else {
-                bail!("The spawner thread failed to execute");
-            };
-
-            let Ok(process_handle) = process_handle else {
-                bail!("The crash helper process failed to launch");
-            };
-
-            self.connector.set_process(process_handle);
-        }
-
         let message = messages::RegisterChildProcess::new(id, server_endpoint.into_ancillary());
         self.connector.send_message(message)?;
 
diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/android.rs b/toolkit/crashreporter/crash_helper_client/src/platform/android.rs
index 72d89ee32cb..f5326f7c217 100644
--- a/toolkit/crashreporter/crash_helper_client/src/platform/android.rs
+++ b/toolkit/crashreporter/crash_helper_client/src/platform/android.rs
@@ -21,7 +21,6 @@ impl CrashHelperClient {
 
         Ok(CrashHelperClient {
             connector,
-            spawner_thread: None,
             pid: 0, // Unused on Android
         })
     }
diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs b/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs
index 3c2ac4aeb4b..fee268ad1de 100644
--- a/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs
+++ b/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs
@@ -60,7 +60,6 @@ impl CrashHelperClient {
 
         Ok(CrashHelperClient {
             connector: client_endpoint,
-            spawner_thread: None,
             pid,
         })
     }
diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs b/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs
index 11ecefc799c..06b27698008 100644
--- a/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs
+++ b/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs
@@ -55,7 +55,7 @@ impl CrashHelperClient {
         let channel = IPCChannel::new()?;
         let (listener, server_endpoint, client_endpoint) = channel.deconstruct();
 
-        let spawner_thread = std::thread::spawn(move || {
+        let _spawner_thread = std::thread::spawn(move || {
             CrashHelperClient::spawn_crash_helper(
                 program,
                 breakpad_data,
@@ -68,7 +68,6 @@ impl CrashHelperClient {
 
         Ok(CrashHelperClient {
             connector: client_endpoint,
-            spawner_thread: Some(spawner_thread),
             pid: 0, // Unused on Windows
         })
     }
diff --git a/toolkit/crashreporter/crash_helper_common/build.rs b/toolkit/crashreporter/crash_helper_common/build.rs
index 7e291e16fff..0c9795d9655 100644
--- a/toolkit/crashreporter/crash_helper_common/build.rs
+++ b/toolkit/crashreporter/crash_helper_common/build.rs
@@ -120,7 +120,7 @@ fn generate_annotation_skiplist(annotations: &[Annotation]) -> Result<String, Er
         if let Some(skip_if) = &annotation.skip_if {
             let entry = format!(
                 "    CrashAnnotationSkipValue {{ annotation: CrashAnnotation::{}, value: b\"{}\" }},\n",
-                &annotation.name, skip_if
+                annotation.name, skip_if
             );
             skiplist.push_str(&entry);
         }
diff --git a/toolkit/crashreporter/crash_helper_common/src/breakpad/windows_strings.rs b/toolkit/crashreporter/crash_helper_common/src/breakpad/windows_strings.rs
index 153f26248ee..6362391461e 100644
--- a/toolkit/crashreporter/crash_helper_common/src/breakpad/windows_strings.rs
+++ b/toolkit/crashreporter/crash_helper_common/src/breakpad/windows_strings.rs
@@ -20,7 +20,7 @@ impl BreakpadString for OsString {
     }
 
     fn deserialize(bytes: Vec<u8>) -> Result<OsString, MessageError> {
-        if (bytes.len() % 2) != 0 {
+        if !bytes.len().is_multiple_of(2) {
             return Err(MessageError::InvalidData);
         }
 
diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs
index 828a6e1aaab..cc83d7de6ca 100644
--- a/toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs
+++ b/toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs
@@ -47,27 +47,11 @@ const INVALID_ANCILLARY_DATA: usize = 0;
 const HANDLE_SIZE: usize = size_of::<usize>();
 const MAX_HANDLES_PER_MESSAGE: usize = 2;
 
-// We encode handles at the beginning of every transmitted message. This
-// function extracts the handle (if present) and returns it together with
-// the rest of the buffer.
-fn extract_buffer_and_handle(buffer: Vec<u8>) -> Result<(Vec<u8>, Vec<OwnedHandle>), IPCError> {
-    let mut handles = Vec::<OwnedHandle>::new();
-    for i in 0..MAX_HANDLES_PER_MESSAGE {
-        let offset = i * HANDLE_SIZE;
-        let handle_bytes = &buffer[offset..offset + HANDLE_SIZE];
-        let handle_bytes: Result<[u8; HANDLE_SIZE], _> = handle_bytes.try_into();
-        let Ok(handle_bytes) = handle_bytes else {
-            return Err(IPCError::InvalidAncillary);
-        };
-        match usize::from_ne_bytes(handle_bytes) {
-            INVALID_ANCILLARY_DATA => {}
-            handle => handles.push(unsafe { OwnedHandle::from_raw_handle(handle as RawHandle) }),
-        };
-    }
-
-    let data = &buffer[MAX_HANDLES_PER_MESSAGE * HANDLE_SIZE..];
+fn is_pseudo_handle(handle: HANDLE) -> bool {
+    let signed_handle = handle as isize;
 
-    Ok((data.to_vec(), handles))
+    // Pseudo-handles fall between -1 (current process) and -12 (undocumented).
+    (-12..=-1).contains(&signed_handle)
 }
 
 pub type IPCConnectorKey = usize;
@@ -289,7 +273,8 @@ impl IPCConnector {
             BytesMut::with_capacity((MAX_HANDLES_PER_MESSAGE * HANDLE_SIZE) + payload.len());
 
         for handle in ancillary_data.into_iter() {
-            let handle = self.clone_handle(handle)?;
+            // Leak the handles, they'll be closed by the receiving process
+            let handle = handle.into_raw_handle();
             buffer.put_slice(&(handle as usize).to_ne_bytes());
         }
         for _i in handles_len..MAX_HANDLES_PER_MESSAGE {
@@ -330,27 +315,64 @@ impl IPCConnector {
         let buffer = self
             .recv_buffer((MAX_HANDLES_PER_MESSAGE * HANDLE_SIZE) + expected_size)
             .map_err(IPCError::ReceptionFailure)?;
-        extract_buffer_and_handle(buffer)
+        self.extract_buffer_and_handle(buffer)
     }
 
     fn recv_buffer(&self, expected_size: usize) -> Result<Vec<u8>, PlatformError> {
         OverlappedOperation::recv(&self.handle, self.event.as_handle(), expected_size)
     }
 
-    /// Clone a handle in the destination process, this is required to
-    /// transfer handles over this connector. Note that this consumes the
-    /// incoming handle because we want it to be closed after it's been cloned
-    /// over to the other process.
-    fn clone_handle(&self, handle: OwnedHandle) -> Result<HANDLE, PlatformError> {
-        let Some(dst_process) = self.process.as_ref() else {
+    // We encode handles at the beginning of every transmitted message. This
+    // function extracts the handles (if present) and returns them together
+    // with the rest of the buffer.
+    fn extract_buffer_and_handle(
+        &self,
+        buffer: Vec<u8>,
+    ) -> Result<(Vec<u8>, Vec<OwnedHandle>), IPCError> {
+        let mut handles = Vec::<OwnedHandle>::new();
+        for i in 0..MAX_HANDLES_PER_MESSAGE {
+            let offset = i * HANDLE_SIZE;
+            let handle_bytes = &buffer[offset..offset + HANDLE_SIZE];
+            let handle_bytes: Result<[u8; HANDLE_SIZE], _> = handle_bytes.try_into();
+            let Ok(handle_bytes) = handle_bytes else {
+                return Err(IPCError::InvalidAncillary);
+            };
+            match usize::from_ne_bytes(handle_bytes) {
+                INVALID_ANCILLARY_DATA => {}
+                handle => handles.push(
+                    self.clone_handle(handle as HANDLE)
+                        .map_err(IPCError::ReceptionFailure)?,
+                ),
+            };
+        }
+
+        let data = &buffer[MAX_HANDLES_PER_MESSAGE * HANDLE_SIZE..];
+
+        Ok((data.to_vec(), handles))
+    }
+
+    /// Clone a remote handle into the current process, this is required to
+    /// transfer handles over this connector. Note that the incoming handle
+    /// is closed in the source process after it's been cloned in the current
+    /// one.
+    fn clone_handle(&self, handle: HANDLE) -> Result<OwnedHandle, PlatformError> {
+        let Some(src_process) = self.process.as_ref() else {
             return Err(PlatformError::MissingProcessHandle);
         };
+
+        if is_pseudo_handle(handle) {
+            return Err(PlatformError::DuplicatePseudoHandle);
+        }
+
         let mut dst_handle: HANDLE = null_mut();
+
+        // SAFETY: Calling `DuplicateHandle()` is always safe once we've
+        // verified that the source handle is not a pseudo-handle.
         let res = unsafe {
             DuplicateHandle(
+                src_process.as_raw_handle(),
+                handle,
                 GetCurrentProcess(),
-                handle.into_raw_handle() as HANDLE,
-                dst_process.as_raw_handle() as HANDLE,
                 &mut dst_handle,
                 /* dwDesiredAccess */ 0,
                 /* bInheritHandle */ FALSE,
@@ -362,7 +384,8 @@ impl IPCConnector {
             return Err(PlatformError::DuplicateHandleFailed(get_last_error()));
         }
 
-        Ok(dst_handle)
+        // SAFETY: We just checked the `dst_handle` is a valid handle.
+        Ok(unsafe { OwnedHandle::from_raw_handle(dst_handle) })
     }
 }
 
diff --git a/toolkit/crashreporter/crash_helper_common/src/platform/windows.rs b/toolkit/crashreporter/crash_helper_common/src/platform/windows.rs
index 50e872fb1d6..dcc9d00f6b7 100644
--- a/toolkit/crashreporter/crash_helper_common/src/platform/windows.rs
+++ b/toolkit/crashreporter/crash_helper_common/src/platform/windows.rs
@@ -71,14 +71,16 @@ pub enum PlatformError {
     AcceptFailed(WIN32_ERROR),
     #[error("Broken pipe")]
     BrokenPipe,
-    #[error("Failed to duplicate handle: {0}")]
-    DuplicateHandleFailed(WIN32_ERROR),
     #[error("Could not create event: {0}")]
     CreateEventFailed(WIN32_ERROR),
     #[error("Could not create or add an I/O completion port: {0}")]
     CreateIoCompletionPortFailed(WIN32_ERROR),
     #[error("Could not create a pipe: {0}")]
     CreatePipeFailure(WIN32_ERROR),
+    #[error("Failed to duplicate handle: {0}")]
+    DuplicateHandleFailed(WIN32_ERROR),
+    #[error("Attempted to duplicate a pseudo-handle")]
+    DuplicatePseudoHandle,
     #[error("Malformed string cannot be converted")]
     InvalidString,
     #[error("I/O error: {0}")]
diff --git a/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs b/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs
index 3ec83cbdca3..65537cec97d 100644
--- a/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs
+++ b/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs
@@ -78,7 +78,7 @@ impl IPCServer {
         client_pid: Pid,
         client_handle: Option<ProcessHandle>,
         listener: IPCListener,
-        connector: IPCConnector,
+        mut connector: IPCConnector,
         breakpad_data: BreakpadData,
         minidump_path: OsString,
         build_id: String,
@@ -92,6 +92,7 @@ impl IPCServer {
                 .context("Client failed to rendez-vous")?
                 .get_process_handle(),
         };
+        connector.set_process(client_handle.clone());
 
         let crash_generator = Box::new(Mutex::new(CrashGenerator::new(
             client_handle.clone(),
Loading diff…