Skip to content

Commit

Permalink
Modify AllocAndCopyName to return smart pointer
Browse files Browse the repository at this point in the history 
R=wfh@chromium.org

Change-Id: I88b212bc3723e344fcd6b8e3827693e006e4808f
Reviewed-on: https://chromium-review.googlesource.com/c/1264197
Commit-Queue: Will Harris <wfh@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598354}
  • Loading branch information
@TheEragon
TheEragon authored and Commit Bot committed Oct 10, 2018
1 parent 6f098c8 commit 415957a
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 63 deletions.
55 changes: 23 additions & 32 deletions sandbox/win/src/filesystem_interception.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ NTSTATUS WINAPI TargetNtCreateFile(NtCreateFileFunction orig_CreateFile,
if (!SandboxFactory::GetTargetServices()->GetState()->InitCalled())
return status;

wchar_t* name = nullptr;
do {
if (!ValidParameter(file, sizeof(HANDLE), WRITE))
break;
Expand All @@ -52,6 +51,7 @@ NTSTATUS WINAPI TargetNtCreateFile(NtCreateFileFunction orig_CreateFile,
if (!memory)
break;

std::unique_ptr<wchar_t, NtAllocDeleter> name;
uint32_t attributes = 0;
NTSTATUS ret =
AllocAndCopyName(object_attributes, &name, &attributes, nullptr);
Expand All @@ -63,7 +63,8 @@ NTSTATUS WINAPI TargetNtCreateFile(NtCreateFileFunction orig_CreateFile,
uint32_t disposition_uint32 = disposition;
uint32_t broker = BROKER_FALSE;
CountedParameterSet<OpenFile> params;
params[OpenFile::NAME] = ParamPickerMake(name);
const wchar_t* name_ptr = name.get();
params[OpenFile::NAME] = ParamPickerMake(name_ptr);
params[OpenFile::ACCESS] = ParamPickerMake(desired_access_uint32);
params[OpenFile::DISPOSITION] = ParamPickerMake(disposition_uint32);
params[OpenFile::OPTIONS] = ParamPickerMake(options_uint32);
Expand All @@ -76,9 +77,10 @@ NTSTATUS WINAPI TargetNtCreateFile(NtCreateFileFunction orig_CreateFile,
CrossCallReturn answer = {0};
// The following call must match in the parameters with
// FilesystemDispatcher::ProcessNtCreateFile.
ResultCode code = CrossCall(ipc, IPC_NTCREATEFILE_TAG, name, attributes,
desired_access_uint32, file_attributes, sharing,
disposition, options_uint32, &answer);
ResultCode code =
CrossCall(ipc, IPC_NTCREATEFILE_TAG, name.get(), attributes,
desired_access_uint32, file_attributes, sharing, disposition,
options_uint32, &answer);
if (SBOX_ALL_OK != code)
break;

Expand All @@ -96,9 +98,6 @@ NTSTATUS WINAPI TargetNtCreateFile(NtCreateFileFunction orig_CreateFile,
}
} while (false);

if (name)
operator delete(name, NT_ALLOC);

return status;
}

Expand All @@ -119,7 +118,6 @@ NTSTATUS WINAPI TargetNtOpenFile(NtOpenFileFunction orig_OpenFile,
if (!SandboxFactory::GetTargetServices()->GetState()->InitCalled())
return status;

wchar_t* name = nullptr;
do {
if (!ValidParameter(file, sizeof(HANDLE), WRITE))
break;
Expand All @@ -130,6 +128,7 @@ NTSTATUS WINAPI TargetNtOpenFile(NtOpenFileFunction orig_OpenFile,
if (!memory)
break;

std::unique_ptr<wchar_t, NtAllocDeleter> name;
uint32_t attributes;
NTSTATUS ret =
AllocAndCopyName(object_attributes, &name, &attributes, nullptr);
Expand All @@ -140,8 +139,9 @@ NTSTATUS WINAPI TargetNtOpenFile(NtOpenFileFunction orig_OpenFile,
uint32_t options_uint32 = options;
uint32_t disposition_uint32 = FILE_OPEN;
uint32_t broker = BROKER_FALSE;
const wchar_t* name_ptr = name.get();
CountedParameterSet<OpenFile> params;
params[OpenFile::NAME] = ParamPickerMake(name);
params[OpenFile::NAME] = ParamPickerMake(name_ptr);
params[OpenFile::ACCESS] = ParamPickerMake(desired_access_uint32);
params[OpenFile::DISPOSITION] = ParamPickerMake(disposition_uint32);
params[OpenFile::OPTIONS] = ParamPickerMake(options_uint32);
Expand All @@ -153,7 +153,7 @@ NTSTATUS WINAPI TargetNtOpenFile(NtOpenFileFunction orig_OpenFile,
SharedMemIPCClient ipc(memory);
CrossCallReturn answer = {0};
ResultCode code =
CrossCall(ipc, IPC_NTOPENFILE_TAG, name, attributes,
CrossCall(ipc, IPC_NTOPENFILE_TAG, name.get(), attributes,
desired_access_uint32, sharing, options_uint32, &answer);
if (SBOX_ALL_OK != code)
break;
Expand All @@ -172,9 +172,6 @@ NTSTATUS WINAPI TargetNtOpenFile(NtOpenFileFunction orig_OpenFile,
}
} while (false);

if (name)
operator delete(name, NT_ALLOC);

return status;
}

Expand All @@ -191,7 +188,6 @@ TargetNtQueryAttributesFile(NtQueryAttributesFileFunction orig_QueryAttributes,
if (!SandboxFactory::GetTargetServices()->GetState()->InitCalled())
return status;

wchar_t* name = nullptr;
do {
if (!ValidParameter(file_attributes, sizeof(FILE_BASIC_INFORMATION), WRITE))
break;
Expand All @@ -200,6 +196,7 @@ TargetNtQueryAttributesFile(NtQueryAttributesFileFunction orig_QueryAttributes,
if (!memory)
break;

std::unique_ptr<wchar_t, NtAllocDeleter> name;
uint32_t attributes = 0;
NTSTATUS ret =
AllocAndCopyName(object_attributes, &name, &attributes, nullptr);
Expand All @@ -211,15 +208,16 @@ TargetNtQueryAttributesFile(NtQueryAttributesFileFunction orig_QueryAttributes,

uint32_t broker = BROKER_FALSE;
CountedParameterSet<FileName> params;
params[FileName::NAME] = ParamPickerMake(name);
const wchar_t* name_ptr = name.get();
params[FileName::NAME] = ParamPickerMake(name_ptr);
params[FileName::BROKER] = ParamPickerMake(broker);

if (!QueryBroker(IPC_NTQUERYATTRIBUTESFILE_TAG, params.GetBase()))
break;

SharedMemIPCClient ipc(memory);
CrossCallReturn answer = {0};
ResultCode code = CrossCall(ipc, IPC_NTQUERYATTRIBUTESFILE_TAG, name,
ResultCode code = CrossCall(ipc, IPC_NTQUERYATTRIBUTESFILE_TAG, name.get(),
attributes, file_info, &answer);

if (SBOX_ALL_OK != code)
Expand All @@ -229,9 +227,6 @@ TargetNtQueryAttributesFile(NtQueryAttributesFileFunction orig_QueryAttributes,

} while (false);

if (name)
operator delete(name, NT_ALLOC);

return status;
}

Expand All @@ -249,7 +244,6 @@ NTSTATUS WINAPI TargetNtQueryFullAttributesFile(
if (!SandboxFactory::GetTargetServices()->GetState()->InitCalled())
return status;

wchar_t* name = nullptr;
do {
if (!ValidParameter(file_attributes, sizeof(FILE_NETWORK_OPEN_INFORMATION),
WRITE))
Expand All @@ -259,6 +253,7 @@ NTSTATUS WINAPI TargetNtQueryFullAttributesFile(
if (!memory)
break;

std::unique_ptr<wchar_t, NtAllocDeleter> name;
uint32_t attributes = 0;
NTSTATUS ret =
AllocAndCopyName(object_attributes, &name, &attributes, nullptr);
Expand All @@ -270,26 +265,24 @@ NTSTATUS WINAPI TargetNtQueryFullAttributesFile(

uint32_t broker = BROKER_FALSE;
CountedParameterSet<FileName> params;
params[FileName::NAME] = ParamPickerMake(name);
const wchar_t* name_ptr = name.get();
params[FileName::NAME] = ParamPickerMake(name_ptr);
params[FileName::BROKER] = ParamPickerMake(broker);

if (!QueryBroker(IPC_NTQUERYFULLATTRIBUTESFILE_TAG, params.GetBase()))
break;

SharedMemIPCClient ipc(memory);
CrossCallReturn answer = {0};
ResultCode code = CrossCall(ipc, IPC_NTQUERYFULLATTRIBUTESFILE_TAG, name,
attributes, file_info, &answer);
ResultCode code = CrossCall(ipc, IPC_NTQUERYFULLATTRIBUTESFILE_TAG,
name.get(), attributes, file_info, &answer);

if (SBOX_ALL_OK != code)
break;

status = answer.nt_status;
} while (false);

if (name)
operator delete(name, NT_ALLOC);

return status;
}

Expand All @@ -310,7 +303,6 @@ TargetNtSetInformationFile(NtSetInformationFileFunction orig_SetInformationFile,
if (!SandboxFactory::GetTargetServices()->GetState()->InitCalled())
return status;

wchar_t* name = nullptr;
do {
void* memory = GetGlobalIPCMemory();
if (!memory)
Expand Down Expand Up @@ -341,14 +333,16 @@ TargetNtSetInformationFile(NtSetInformationFileFunction orig_SetInformationFile,
break;
}

std::unique_ptr<wchar_t, NtAllocDeleter> name;
NTSTATUS ret =
AllocAndCopyName(&object_attributes, &name, nullptr, nullptr);
if (!NT_SUCCESS(ret) || !name)
break;

uint32_t broker = BROKER_FALSE;
CountedParameterSet<FileName> params;
params[FileName::NAME] = ParamPickerMake(name);
const wchar_t* name_ptr = name.get();
params[FileName::NAME] = ParamPickerMake(name_ptr);
params[FileName::BROKER] = ParamPickerMake(broker);

if (!QueryBroker(IPC_NTSETINFO_RENAME_TAG, params.GetBase()))
Expand All @@ -371,9 +365,6 @@ TargetNtSetInformationFile(NtSetInformationFileFunction orig_SetInformationFile,
status = answer.nt_status;
} while (false);

if (name)
operator delete(name, NT_ALLOC);

return status;
}

Expand Down
28 changes: 14 additions & 14 deletions sandbox/win/src/registry_interception.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ NTSTATUS WINAPI TargetNtCreateKey(NtCreateKeyFunction orig_CreateKey,
if (!memory)
break;

wchar_t* name;
std::unique_ptr<wchar_t, NtAllocDeleter> name;
uint32_t attributes = 0;
HANDLE root_directory = 0;
NTSTATUS ret = AllocAndCopyName(object_attributes, &name, &attributes,
Expand All @@ -68,14 +68,16 @@ NTSTATUS WINAPI TargetNtCreateKey(NtCreateKeyFunction orig_CreateKey,
params[OpenKey::ACCESS] = ParamPickerMake(desired_access_uint32);

wchar_t* full_name = nullptr;
const wchar_t* name_ptr = name.get();

if (root_directory) {
ret = sandbox::AllocAndGetFullPath(root_directory, name, &full_name);
ret =
sandbox::AllocAndGetFullPath(root_directory, name.get(), &full_name);
if (!NT_SUCCESS(ret) || !full_name)
break;
params[OpenKey::NAME] = ParamPickerMake(full_name);
} else {
params[OpenKey::NAME] = ParamPickerMake(name);
params[OpenKey::NAME] = ParamPickerMake(name_ptr);
}

bool query_broker = QueryBroker(IPC_NTCREATEKEY_TAG, params.GetBase());
Expand All @@ -89,11 +91,9 @@ NTSTATUS WINAPI TargetNtCreateKey(NtCreateKeyFunction orig_CreateKey,
SharedMemIPCClient ipc(memory);
CrossCallReturn answer = {0};

ResultCode code =
CrossCall(ipc, IPC_NTCREATEKEY_TAG, name, attributes, root_directory,
desired_access, title_index, create_options, &answer);

operator delete(name, NT_ALLOC);
ResultCode code = CrossCall(ipc, IPC_NTCREATEKEY_TAG, name.get(),
attributes, root_directory, desired_access,
title_index, create_options, &answer);

if (SBOX_ALL_OK != code)
break;
Expand Down Expand Up @@ -138,7 +138,7 @@ NTSTATUS WINAPI CommonNtOpenKey(NTSTATUS status,
if (!memory)
break;

wchar_t* name;
std::unique_ptr<wchar_t, NtAllocDeleter> name;
uint32_t attributes;
HANDLE root_directory;
NTSTATUS ret = AllocAndCopyName(object_attributes, &name, &attributes,
Expand All @@ -151,14 +151,16 @@ NTSTATUS WINAPI CommonNtOpenKey(NTSTATUS status,
params[OpenKey::ACCESS] = ParamPickerMake(desired_access_uint32);

wchar_t* full_name = nullptr;
const wchar_t* name_ptr = name.get();

if (root_directory) {
ret = sandbox::AllocAndGetFullPath(root_directory, name, &full_name);
ret =
sandbox::AllocAndGetFullPath(root_directory, name.get(), &full_name);
if (!NT_SUCCESS(ret) || !full_name)
break;
params[OpenKey::NAME] = ParamPickerMake(full_name);
} else {
params[OpenKey::NAME] = ParamPickerMake(name);
params[OpenKey::NAME] = ParamPickerMake(name_ptr);
}

bool query_broker = QueryBroker(IPC_NTOPENKEY_TAG, params.GetBase());
Expand All @@ -171,11 +173,9 @@ NTSTATUS WINAPI CommonNtOpenKey(NTSTATUS status,

SharedMemIPCClient ipc(memory);
CrossCallReturn answer = {0};
ResultCode code = CrossCall(ipc, IPC_NTOPENKEY_TAG, name, attributes,
ResultCode code = CrossCall(ipc, IPC_NTOPENKEY_TAG, name.get(), attributes,
root_directory, desired_access, &answer);

operator delete(name, NT_ALLOC);

if (SBOX_ALL_OK != code)
break;

Expand Down
15 changes: 6 additions & 9 deletions sandbox/win/src/sandbox_nt_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,14 +300,13 @@ NTSTATUS AllocAndGetFullPath(HANDLE root, wchar_t* path, wchar_t** full_path) {

// Hacky code... replace with AllocAndCopyObjectAttributes.
NTSTATUS AllocAndCopyName(const OBJECT_ATTRIBUTES* in_object,
wchar_t** out_name,
std::unique_ptr<wchar_t, NtAllocDeleter>* out_name,
uint32_t* attributes,
HANDLE* root) {
if (!InitHeap())
return STATUS_NO_MEMORY;

DCHECK_NT(out_name);
*out_name = nullptr;
NTSTATUS ret = STATUS_UNSUCCESSFUL;
__try {
do {
Expand All @@ -319,16 +318,16 @@ NTSTATUS AllocAndCopyName(const OBJECT_ATTRIBUTES* in_object,
break;

size_t size = in_object->ObjectName->Length + sizeof(wchar_t);
*out_name = new (NT_ALLOC) wchar_t[size / sizeof(wchar_t)];
out_name->reset(new (NT_ALLOC) wchar_t[size / sizeof(wchar_t)]);
if (!*out_name)
break;

ret = CopyData(*out_name, in_object->ObjectName->Buffer,
ret = CopyData(out_name->get(), in_object->ObjectName->Buffer,
size - sizeof(wchar_t));
if (!NT_SUCCESS(ret))
break;

(*out_name)[size / sizeof(wchar_t) - 1] = L'\0';
out_name->get()[size / sizeof(wchar_t) - 1] = L'\0';

if (attributes)
*attributes = in_object->Attributes;
Expand All @@ -341,10 +340,8 @@ NTSTATUS AllocAndCopyName(const OBJECT_ATTRIBUTES* in_object,
ret = GetExceptionCode();
}

if (!NT_SUCCESS(ret) && *out_name) {
operator delete(*out_name, NT_ALLOC);
*out_name = nullptr;
}
if (!NT_SUCCESS(ret) && *out_name)
out_name->reset(nullptr);

return ret;
}
Expand Down
9 changes: 8 additions & 1 deletion sandbox/win/src/sandbox_nt_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <intrin.h>
#include <stddef.h>
#include <stdint.h>
#include <memory>

#include "base/macros.h"
#include "sandbox/win/src/nt_internals.h"
Expand Down Expand Up @@ -89,6 +90,12 @@ __forceinline void* _InterlockedCompareExchangePointer(

#endif

struct NtAllocDeleter {
inline void operator()(void* ptr) const {
operator delete(ptr, AllocationType::NT_ALLOC);
}
};

// Returns a pointer to the IPC shared memory.
void* GetGlobalIPCMemory();

Expand All @@ -107,7 +114,7 @@ NTSTATUS CopyData(void* destination, const void* source, size_t bytes);

// Copies the name from an object attributes.
NTSTATUS AllocAndCopyName(const OBJECT_ATTRIBUTES* in_object,
wchar_t** out_name,
std::unique_ptr<wchar_t, NtAllocDeleter>* out_name,
uint32_t* attributes,
HANDLE* root);

Expand Down
Loading

0 comments on commit 415957a

Please sign in to comment.