Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions client/crashpad_client_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1205,21 +1205,19 @@ void CrashpadClient::SetFirstChanceExceptionHandler(
}

void CrashpadClient::AddAttachment(const base::FilePath& attachment) {
ClientToServerMessage message = {};
message.type = ClientToServerMessage::kAddAttachment;
swprintf_s(
message.attachment.path, MAX_PATH, L"%ls", attachment.value().c_str());
ServerToClientMessage response = {};
SendToCrashHandlerServer(ipc_pipe_, message, &response);
SendAttachmentToCrashHandlerServer(ipc_pipe_,
ClientToServerMessage::kAddAttachmentV2,
attachment.value(),
&response);
}

void CrashpadClient::RemoveAttachment(const base::FilePath& attachment) {
ClientToServerMessage message = {};
message.type = ClientToServerMessage::kRemoveAttachment;
swprintf_s(
message.attachment.path, MAX_PATH, L"%ls", attachment.value().c_str());
ServerToClientMessage response = {};
SendToCrashHandlerServer(ipc_pipe_, message, &response);
SendAttachmentToCrashHandlerServer(ipc_pipe_,
ClientToServerMessage::kRemoveAttachmentV2,
attachment.value(),
&response);
}

} // namespace crashpad
18 changes: 13 additions & 5 deletions util/misc/paths_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,25 @@ namespace crashpad {

// static
bool Paths::Executable(base::FilePath* path) {
wchar_t executable_path[_MAX_PATH];
unsigned int len = GetModuleFileName(
nullptr, executable_path, static_cast<DWORD>(std::size(executable_path)));
// follow the maximum path length documented here:
// https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
constexpr DWORD kMaxPathChars = 32768;
std::wstring executable_path(kMaxPathChars, L'\0');

const DWORD len =
GetModuleFileName(nullptr, &executable_path[0], kMaxPathChars);

if (len == 0) {
PLOG(ERROR) << "GetModuleFileName";
return false;
} else if (len >= std::size(executable_path)) {
LOG(ERROR) << "GetModuleFileName";
}

if (len >= kMaxPathChars) {
LOG(ERROR) << "GetModuleFileName: path exceeds maximum length";
return false;
}

executable_path.resize(len);
*path = base::FilePath(executable_path);
return true;
}
Expand Down
75 changes: 72 additions & 3 deletions util/win/exception_handler_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,14 @@ void ExceptionHandlerServer::InitializeWithInheritedDataForInitialClient(

first_pipe_instance_.reset(initial_client_data.first_pipe_instance());

// TODO(scottmg): Vista+. Might need to pass through or possibly find an Nt*.
auto data = base::HeapArray<uint8_t>::Uninit(sizeof(wchar_t) * _MAX_PATH +
sizeof(FILE_NAME_INFO));
// Allocate buffer for FILE_NAME_INFO with maximum pipe name length.
// According to Windows documentation, pipe name strings are limited to 256
// characters: https://learn.microsoft.com/en-us/windows/win32/ipc/pipe-names
// FILE_NAME_INFO has a flexible array member (FileName) and provides an
// explicit FileNameLength field, so no null terminator is needed.
constexpr size_t kMaxPipeNameChars = 256;
auto data = base::HeapArray<uint8_t>::Uninit(
sizeof(FILE_NAME_INFO) + sizeof(wchar_t) * kMaxPipeNameChars);
if (!GetFileInformationByHandleEx(first_pipe_instance_.get(),
FileNameInfo,
data.data(),
Expand Down Expand Up @@ -414,6 +419,60 @@ void ExceptionHandlerServer::Stop() {
PostQueuedCompletionStatus(port_.get(), 0, 0, nullptr);
}

static void HandleAddAttachmentV2(
const internal::PipeServiceContext& service_context,
const ClientToServerMessage& message) {
const uint32_t path_length_bytes = message.attachment_v2.path_length_bytes;

if (path_length_bytes == 0 || path_length_bytes > kMaxPathBytes) {
LOG(ERROR) << "Invalid path length: " << path_length_bytes;
return;
}

auto path_buffer =
base::HeapArray<wchar_t>::Uninit(path_length_bytes / sizeof(wchar_t));
Comment on lines +437 to +438

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential bug: The buffer allocation for path_buffer uses integer division on path_length_bytes without validating its divisibility by sizeof(wchar_t), potentially creating an undersized buffer.
  • Description: In HandleAddAttachmentV2 and HandleRemoveAttachmentV2, the buffer for an attachment path is allocated using path_length_bytes / sizeof(wchar_t). This calculation uses integer division, which truncates the result. If the server receives a path_length_bytes value from a client that is not evenly divisible by sizeof(wchar_t) (2 bytes), the allocated path_buffer will be smaller than path_length_bytes. The subsequent call to LoggingReadFileExactly attempts to read the full path_length_bytes into this undersized buffer, causing a heap buffer overflow. This can lead to memory corruption or a crash of the exception handler server.

  • Suggested fix: Before allocating the buffer in HandleAddAttachmentV2 and HandleRemoveAttachmentV2, add a check to validate that path_length_bytes is evenly divisible by sizeof(wchar_t). If the check fails, log an error and return early to prevent the allocation and subsequent buffer overflow.
    severity: 0.85, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we can be sure that our untampered client will send bytes that are multiples of wchar_t, since it is IPC, we should protect against malicious or corrupted input. I will add further validation of the incoming message.


if (!LoggingReadFileExactly(
service_context.pipe(), path_buffer.data(), path_length_bytes)) {
LOG(ERROR) << "Failed to read attachment path";
return;
}

path_buffer[path_buffer.size() - 1] = L'\0';

ServerToClientMessage response = {};
service_context.delegate()->ExceptionHandlerServerAttachmentAdded(
base::FilePath(std::wstring(path_buffer.data())));
LoggingWriteFile(service_context.pipe(), &response, sizeof(response));
}

static void HandleRemoveAttachmentV2(
const internal::PipeServiceContext& service_context,
const ClientToServerMessage& message) {
const uint32_t path_length_bytes = message.attachment_v2.path_length_bytes;

if (path_length_bytes == 0 || path_length_bytes > kMaxPathBytes) {
LOG(ERROR) << "Invalid path length: " << path_length_bytes;
return;
}

auto path_buffer =
base::HeapArray<wchar_t>::Uninit(path_length_bytes / sizeof(wchar_t));

if (!LoggingReadFileExactly(
service_context.pipe(), path_buffer.data(), path_length_bytes)) {
LOG(ERROR) << "Failed to read attachment path";
return;
}

path_buffer[path_buffer.size() - 1] = L'\0';

ServerToClientMessage response = {};
service_context.delegate()->ExceptionHandlerServerAttachmentRemoved(
base::FilePath(std::wstring(path_buffer.data())));
LoggingWriteFile(service_context.pipe(), &response, sizeof(response));
}

// This function must be called with service_context.pipe() already connected to
// a client pipe. It exchanges data with the client and adds a ClientData record
// to service_context->clients().
Expand Down Expand Up @@ -475,6 +534,16 @@ bool ExceptionHandlerServer::ServiceClientConnection(
return false;
}

case ClientToServerMessage::kAddAttachmentV2: {
HandleAddAttachmentV2(service_context, message);
return false;
}

case ClientToServerMessage::kRemoveAttachmentV2: {
HandleRemoveAttachmentV2(service_context, message);
return false;
}

default:
LOG(ERROR) << "unhandled message type: " << message.type;
return false;
Expand Down
75 changes: 75 additions & 0 deletions util/win/registration_protocol_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,81 @@ bool SendToCrashHandlerServer(const std::wstring& pipe_name,
}
}

bool SendAttachmentToCrashHandlerServer(
const std::wstring& pipe_name,
ClientToServerMessage::Type message_type,
const std::wstring& path,
ServerToClientMessage* response) {
if (message_type != ClientToServerMessage::kAddAttachmentV2 &&
message_type != ClientToServerMessage::kRemoveAttachmentV2) {
LOG(ERROR) << "Invalid message type for attachment: " << message_type;
return false;
}

const size_t path_length_bytes = (path.length() + 1) * sizeof(wchar_t);

if (path_length_bytes > kMaxPathBytes) {
LOG(ERROR) << "Path too long: " << path_length_bytes << " bytes";
return false;
}

// Build the message header.
ClientToServerMessage message = {};
message.type = message_type;
message.attachment_v2.path_length_bytes =
static_cast<uint32_t>(path_length_bytes);

// Retry CreateFile() in a loop (follows the logic in
// SendToCrashHandlerServer).
for (;;) {
ScopedFileHANDLE pipe(
CreateFile(pipe_name.c_str(),
GENERIC_READ | GENERIC_WRITE,
0,
nullptr,
OPEN_EXISTING,
SECURITY_SQOS_PRESENT | SECURITY_IDENTIFICATION,
nullptr));
if (!pipe.is_valid()) {
if (GetLastError() != ERROR_PIPE_BUSY) {
PLOG(ERROR) << "CreateFile";
return false;
}

if (!WaitNamedPipe(pipe_name.c_str(), NMPWAIT_WAIT_FOREVER)) {
PLOG(ERROR) << "WaitNamedPipe";
return false;
}

continue;
}

DWORD mode = PIPE_READMODE_MESSAGE;
if (!SetNamedPipeHandleState(pipe.get(), &mode, nullptr, nullptr)) {
PLOG(ERROR) << "SetNamedPipeHandleState";
return false;
}

if (!WriteFile(pipe.get(), &message, sizeof(message))) {
PLOG(ERROR) << "WriteFile (header)";
return false;
}

if (!WriteFile(
pipe.get(), path.c_str(), static_cast<DWORD>(path_length_bytes))) {
PLOG(ERROR) << "WriteFile (path)";
return false;
}

if (!ReadFile(pipe.get(), response, sizeof(*response))) {
PLOG(ERROR) << "ReadFile (response)";
return false;
}

return true;
}
}

HANDLE CreateNamedPipeInstance(const std::wstring& pipe_name,
bool first_instance) {
SECURITY_ATTRIBUTES security_attributes;
Expand Down
18 changes: 18 additions & 0 deletions util/win/registration_protocol_win.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,24 @@ bool SendToCrashHandlerServer(const std::wstring& pipe_name,
const ClientToServerMessage& message,
ServerToClientMessage* response);

//! \brief Connect over the given \a pipe_name, passing a variable-length
//! attachment message to the server.
//!
//! This is used for kAddAttachmentV2 and kRemoveAttachmentV2 message types
//! which support paths longer than MAX_PATH.
//!
//! \param[in] pipe_name The name of the pipe to connect to.
//! \param[in] message_type Either kAddAttachmentV2 or kRemoveAttachmentV2.
//! \param[in] path The attachment path to send.
//! \param[out] response The server's response.
//!
//! \return `true` on success, `false` on failure with a message logged.
bool SendAttachmentToCrashHandlerServer(
const std::wstring& pipe_name,
ClientToServerMessage::Type message_type,
const std::wstring& path,
ServerToClientMessage* response);

//! \brief Wraps CreateNamedPipe() to create a single named pipe instance.
//!
//! \param[in] pipe_name The name to use for the pipe.
Expand Down
27 changes: 24 additions & 3 deletions util/win/registration_protocol_win_structs.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,26 @@ struct ShutdownRequest {
uint64_t token;
};

//! \brief A file attachment request.
//! \brief A file attachment request (legacy, limited to MAX_PATH).
struct AttachmentRequest {
//! \brief The path of the attachment.
wchar_t path[MAX_PATH];
};

//! \brief A variable-length file attachment request header.
//!
//! For kAddAttachmentV2 and kRemoveAttachmentV2, the message consists of
//! a ClientToServerMessage with this header in the union, followed by
//! path_length_bytes of wchar_t data containing the null-terminated path.
struct AttachmentRequestV2 {
//! \brief Length of the path in bytes, including null terminator.
uint32_t path_length_bytes;
};

//! follow the maximum path length documented here:
//! https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
constexpr uint32_t kMaxPathBytes = 32768 * sizeof(wchar_t);

//! \brief The message passed from client to server by
//! SendToCrashHandlerServer().
struct ClientToServerMessage {
Expand All @@ -133,22 +147,29 @@ struct ClientToServerMessage {
//! \brief For ShutdownRequest.
kShutdown,

//! \brief For AttachmentRequest.
//! \brief For AttachmentRequest (legacy, MAX_PATH limited).
kAddAttachment,

//! \brief For AttachmentRequest.
//! \brief For AttachmentRequest (legacy, MAX_PATH limited).
kRemoveAttachment,

//! \brief An empty message sent by the initial client in asynchronous mode.
//! No data is required, this just confirms that the server is ready to
//! accept client registrations.
kPing,

//! \brief For AttachmentRequestV2 (variable-length paths).
kAddAttachmentV2,

//! \brief For AttachmentRequestV2 (variable-length paths).
kRemoveAttachmentV2,
} type;

union {
RegistrationRequest registration;
ShutdownRequest shutdown;
AttachmentRequest attachment;
AttachmentRequestV2 attachment_v2;
};
};

Expand Down