3party/crashpad
0
0
Fork 0
mirror of https://github.com/chromium/crashpad.git synced 2025-03-09 14:06:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

[Windows] Add AppContainer SID to Named Pipe DACL.

Browse Source 
This CL modifies the creation of the Named Pipe Security Descriptor to
allow access from AppContainer processes. The DACL only allows access for
the current user and SYSTEM which matches up with the auto-assigned DACL
used previously (the read-only logon SID ACE has been removed). As this
new code uses APIs from ADVAPI32 a check is made to ensure it's not being
called while the loader lock is held to avoid hitting previous similar
issues.

Bug: crashpad: 318
Change-Id: I3f9cf5c788dbadacad21c8a2d57a0188f690ac32
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/1955982
Commit-Queue: James Forshaw <forshaw@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
This commit is contained in:
James Forshaw 2019-12-10 08:51:20 -08:00 committed by Commit Bot
parent 3e4d6a9b7f
commit c6153f0b6e
3 changed files with 192 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

81
util/win/registration_protocol_win.cc
View File

@ -14,16 +14,63 @@
#include "util/win/registration_protocol_win.h"
#include <stddef.h>
#include <windows.h>
#include <aclapi.h>
#include <sddl.h>
#include <stddef.h>
#include "base/logging.h"
#include "base/stl_util.h"
#include "util/win/exception_handler_server.h"
#include "util/win/loader_lock.h"
#include "util/win/scoped_handle.h"
#include "util/win/scoped_local_alloc.h"
namespace crashpad {
namespace {
void* GetSecurityDescriptorWithUser(const base::char16* sddl_string,
size_t* size) {
if (size)
*size = 0;
PSECURITY_DESCRIPTOR base_sec_desc;
if (!ConvertStringSecurityDescriptorToSecurityDescriptor(
sddl_string, SDDL_REVISION_1, &base_sec_desc, nullptr)) {
PLOG(ERROR) << "ConvertStringSecurityDescriptorToSecurityDescriptor";
return nullptr;
}
ScopedLocalAlloc base_sec_desc_owner(base_sec_desc);
EXPLICIT_ACCESS access;
wchar_t username[] = L"CURRENT_USER";
BuildExplicitAccessWithName(
&access, username, GENERIC_ALL, GRANT_ACCESS, NO_INHERITANCE);
PSECURITY_DESCRIPTOR user_sec_desc;
ULONG user_sec_desc_size;
DWORD error = BuildSecurityDescriptor(nullptr,
nullptr,
1,
&access,
0,
nullptr,
base_sec_desc,
&user_sec_desc_size,
&user_sec_desc);
if (error != ERROR_SUCCESS) {
SetLastError(error);
PLOG(ERROR) << "BuildSecurityDescriptor";
return nullptr;
}
*size = user_sec_desc_size;
return user_sec_desc;
}
} // namespace
bool SendToCrashHandlerServer(const base::string16& pipe_name,
const ClientToServerMessage& message,
ServerToClientMessage* response) {
@ -124,16 +171,11 @@ HANDLE CreateNamedPipeInstance(const std::wstring& pipe_name,
security_attributes_pointer);
}
const void* GetSecurityDescriptorForNamedPipeInstance(size_t* size) {
const void* GetFallbackSecurityDescriptorForNamedPipeInstance(size_t* size) {
// Mandatory Label, no ACE flags, no ObjectType, integrity level untrusted is
// "S:(ML;;;;;S-1-16-0)". Typically
// ConvertStringSecurityDescriptorToSecurityDescriptor() would be used to
// convert from a string representation. However, that function cannot be used
// because it is in advapi32.dll and CreateNamedPipeInstance() is called from
// within DllMain() where the loader lock is held. advapi32.dll is delay
// loaded in chrome_elf.dll because it must avoid loading user32.dll. If an
// advapi32.dll function were used, it would cause a load of the DLL, which
// would in turn cause deadlock.
// "S:(ML;;;;;S-1-16-0)". This static security descriptor is used as a
// fallback if GetSecurityDescriptorWithUser fails, to avoid losing crashes
// from non-AppContainer sandboxed applications.
#pragma pack(push, 1)
static constexpr struct SecurityDescriptorBlob {
@ -207,4 +249,23 @@ const void* GetSecurityDescriptorForNamedPipeInstance(size_t* size) {
return reinterpret_cast<const void*>(&kSecDescBlob);
}
const void* GetSecurityDescriptorForNamedPipeInstance(size_t* size) {
CHECK(!IsThreadInLoaderLock());
// Get a security descriptor which grants the current user and SYSTEM full
// access to the named pipe. Also grant AppContainer RW access through the ALL
// APPLICATION PACKAGES SID (S-1-15-2-1). Finally add an Untrusted Mandatory
// Label for non-AppContainer sandboxed users.
static size_t sd_size;
static void* sec_desc = GetSecurityDescriptorWithUser(
L"D:(A;;GA;;;SY)(A;;GWGR;;;S-1-15-2-1)S:(ML;;;;;S-1-16-0)", &sd_size);
if (!sec_desc)
return GetFallbackSecurityDescriptorForNamedPipeInstance(size);
if (size)
*size = sd_size;
return sec_desc;
}
} // namespace crashpad

17
util/win/registration_protocol_win.h
View File

@ -145,10 +145,10 @@ bool SendToCrashHandlerServer(const base::string16& pipe_name,
HANDLE CreateNamedPipeInstance(const std::wstring& pipe_name,
bool first_instance);
//! \brief Returns the SECURITY_DESCRIPTOR blob that will be used for creating
//! \brief Returns the `SECURITY_DESCRIPTOR` blob that will be used for creating
//! the connection pipe in CreateNamedPipeInstance().
//!
//! This function is exposed for only for testing.
//! This function is only exposed for testing.
//!
//! \param[out] size The size of the returned blob. May be `nullptr` if not
//! required.
@ -157,6 +157,19 @@ HANDLE CreateNamedPipeInstance(const std::wstring& pipe_name,
//! transferred to the caller.
const void* GetSecurityDescriptorForNamedPipeInstance(size_t* size);
//! \brief Returns the `SECURITY_DESCRIPTOR` blob that will be used for creating
//! the connection pipe in CreateNamedPipeInstance() if the full descriptor
//! can't be created.
//!
//! This function is only exposed for testing.
//!
//! \param[out] size The size of the returned blob. May be `nullptr` if not
//! required.
//!
//! \return A pointer to a self-relative `SECURITY_DESCRIPTOR`. Ownership is not
//! transferred to the caller.
const void* GetFallbackSecurityDescriptorForNamedPipeInstance(size_t* size);
} // namespace crashpad
#endif // CRASHPAD_UTIL_WIN_REGISTRATION_PROTOCOL_WIN_H_

108
util/win/registration_protocol_win_test.cc
View File

@ -14,18 +14,122 @@
#include "util/win/registration_protocol_win.h"
#include <windows.h>
#include <aclapi.h>
#include <sddl.h>
#include <string.h>
#include <windows.h>
#include <vector>
#include "base/logging.h"
#include "base/strings/string16.h"
#include "gtest/gtest.h"
#include "test/errors.h"
#include "util/win/scoped_handle.h"
#include "util/win/scoped_local_alloc.h"
namespace crashpad {
namespace test {
namespace {
base::string16 GetStringFromSid(PSID sid) {
LPWSTR sid_str;
if (!ConvertSidToStringSid(sid, &sid_str)) {
PLOG(ERROR) << "ConvertSidToStringSid";
return base::string16();
}
ScopedLocalAlloc sid_str_ptr(sid_str);
return sid_str;
}
base::string16 GetUserSidString() {
HANDLE token_handle;
if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &token_handle)) {
PLOG(ERROR) << "OpenProcessToken";
return base::string16();
}
ScopedKernelHANDLE token(token_handle);
DWORD user_size = 0;
GetTokenInformation(token.get(), TokenUser, nullptr, 0, &user_size);
if (user_size == 0) {
PLOG(ERROR) << "GetTokenInformation Size";
return base::string16();
}
std::vector<char> user(user_size);
if (!GetTokenInformation(
token.get(), TokenUser, user.data(), user_size, &user_size)) {
PLOG(ERROR) << "GetTokenInformation";
return base::string16();
}
TOKEN_USER* user_ptr = reinterpret_cast<TOKEN_USER*>(user.data());
return GetStringFromSid(user_ptr->User.Sid);
}
void CheckAce(PACL acl,
DWORD index,
BYTE check_ace_type,
ACCESS_MASK check_mask,
const base::string16& check_sid) {
ASSERT_FALSE(check_sid.empty());
void* ace_ptr;
ASSERT_TRUE(GetAce(acl, index, &ace_ptr));
ACE_HEADER* header = static_cast<ACE_HEADER*>(ace_ptr);
ASSERT_EQ(check_ace_type, header->AceType);
ASSERT_EQ(0, header->AceFlags);
PSID sid = nullptr;
ACCESS_MASK mask = 0;
switch (header->AceType) {
case ACCESS_ALLOWED_ACE_TYPE: {
ACCESS_ALLOWED_ACE* allowed_ace =
static_cast<ACCESS_ALLOWED_ACE*>(ace_ptr);
sid = &allowed_ace->SidStart;
mask = allowed_ace->Mask;
} break;
case SYSTEM_MANDATORY_LABEL_ACE_TYPE: {
SYSTEM_MANDATORY_LABEL_ACE* label_ace =
static_cast<SYSTEM_MANDATORY_LABEL_ACE*>(ace_ptr);
sid = &label_ace->SidStart;
mask = label_ace->Mask;
} break;
default:
NOTREACHED();
break;
}
ASSERT_EQ(check_mask, mask);
ASSERT_EQ(check_sid, GetStringFromSid(sid));
}
TEST(SecurityDescriptor, NamedPipeDefault) {
const void* sec_desc = GetSecurityDescriptorForNamedPipeInstance(nullptr);
PACL acl;
BOOL acl_present;
BOOL acl_defaulted;
ASSERT_TRUE(GetSecurityDescriptorDacl(
const_cast<void*>(sec_desc), &acl_present, &acl, &acl_defaulted));
ASSERT_EQ(3, acl->AceCount);
CheckAce(acl, 0, ACCESS_ALLOWED_ACE_TYPE, GENERIC_ALL, GetUserSidString());
// Check SYSTEM user SID.
CheckAce(acl, 1, ACCESS_ALLOWED_ACE_TYPE, GENERIC_ALL, L"S-1-5-18");
// Check ALL APPLICATION PACKAGES group SID.
CheckAce(acl,
2,
ACCESS_ALLOWED_ACE_TYPE,
GENERIC_READ | GENERIC_WRITE,
L"S-1-15-2-1");
ASSERT_TRUE(GetSecurityDescriptorSacl(
const_cast<void*>(sec_desc), &acl_present, &acl, &acl_defaulted));
ASSERT_EQ(1, acl->AceCount);
CheckAce(acl, 0, SYSTEM_MANDATORY_LABEL_ACE_TYPE, 0, L"S-1-16-0");
}
TEST(SecurityDescriptor, MatchesAdvapi32) {
// This security descriptor is built manually in the connection code to avoid
// calling the advapi32 functions. Verify that it returns the same thing as
@ -43,7 +147,7 @@ TEST(SecurityDescriptor, MatchesAdvapi32) {
size_t created_len;
const void* const created =
GetSecurityDescriptorForNamedPipeInstance(&created_len);
GetFallbackSecurityDescriptorForNamedPipeInstance(&created_len);
ASSERT_EQ(created_len, sec_desc_len);
EXPECT_EQ(memcmp(sec_desc, created, sec_desc_len), 0);
}