Firefox · Toolkit
CVE-2026-92047
Logic Error in Toolkit
Overview
Moderate
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
Files Changed
toolkit/crashreporter/crash_helper_client/src/lib.rstoolkit/crashreporter/crash_helper_client/src/platform/android.rstoolkit/crashreporter/crash_helper_client/src/platform/unix.rstoolkit/crashreporter/crash_helper_client/src/platform/windows.rstoolkit/crashreporter/crash_helper_common/build.rstoolkit/crashreporter/crash_helper_common/src/breakpad/windows_strings.rstoolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rstoolkit/crashreporter/crash_helper_common/src/platform/windows.rstoolkit/crashreporter/crash_helper_server/src/ipc_server.rs
Patch
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…
References
On This Page