Permalink
Browse files

Revert 128016 - Make sandbox explicitly block opening broker and sand…

…boxed processes

BUG=117627
BUG=119150
TEST=sbox_validation_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=127795
Review URL: https://chromiumcodereview.appspot.com/9716027

TBR=jschuh@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9834065

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128583 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information...
1 parent 7d2e053 commit 0c942629623a288ea774f1b9e8512182b746bfab jschuh@chromium.org committed Mar 23, 2012
@@ -315,7 +315,10 @@ bool GpuChannel::ShouldPreferDiscreteGpu() const {
void GpuChannel::OnInitialize(base::ProcessHandle renderer_process) {
// Initialize should only happen once.
DCHECK(!renderer_process_);
- renderer_process_ = renderer_process;
+
+ // Verify that the renderer has passed its own process handle.
+ if (base::GetProcId(renderer_process) == renderer_pid_)
+ renderer_process_ = renderer_process;
}
void GpuChannel::OnCreateOffscreenCommandBuffer(
@@ -1,11 +1,9 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "sandbox/src/broker_services.h"
-#include <AclAPI.h>
-
#include "base/logging.h"
#include "base/threading/platform_thread.h"
#include "sandbox/src/sandbox_policy_base.h"
@@ -44,64 +42,27 @@ enum {
THREAD_CTRL_LAST
};
-// Adds deny ACEs to broker and returns the security descriptor so it can
-// be applied to target processes. The returned descriptor must be freed by
-// calling LocalFree.
-PSECURITY_DESCRIPTOR SetSecurityDescriptorForBroker() {
- static bool is_initialized = false;
- DWORD error = ERROR_SUCCESS;
- PSECURITY_DESCRIPTOR security_descriptor = NULL;
-
- if (!is_initialized) {
- error = sandbox::SetObjectDenyRestrictedAndNull(GetCurrentProcess(),
- SE_KERNEL_OBJECT);
- if (error) {
- ::SetLastError(error);
- return NULL;
- }
-
- is_initialized = true;
- }
-
- // Save off resulting security descriptor for spawning the targets.
- error = ::GetSecurityInfo(GetCurrentProcess(), SE_KERNEL_OBJECT,
- DACL_SECURITY_INFORMATION, NULL, NULL,
- NULL, NULL, &security_descriptor);
- if (error) {
- ::SetLastError(error);
- return NULL;
- }
-
- return security_descriptor;
-}
-
}
namespace sandbox {
BrokerServicesBase::BrokerServicesBase()
: thread_pool_(NULL), job_port_(NULL), no_targets_(NULL),
- security_descriptor_(NULL), job_thread_(NULL) {
+ job_thread_(NULL) {
}
// The broker uses a dedicated worker thread that services the job completion
// port to perform policy notifications and associated cleanup tasks.
ResultCode BrokerServicesBase::Init() {
- if ((NULL != job_port_) || (NULL != thread_pool_) ||
- (NULL != security_descriptor_)) {
+ if ((NULL != job_port_) || (NULL != thread_pool_))
return SBOX_ERROR_UNEXPECTED_CALL;
- }
::InitializeCriticalSection(&lock_);
job_port_ = ::CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 0);
if (NULL == job_port_)
return SBOX_ERROR_GENERIC;
- security_descriptor_ = SetSecurityDescriptorForBroker();
- if (NULL == security_descriptor_)
- return SBOX_ERROR_GENERIC;
-
no_targets_ = ::CreateEventW(NULL, TRUE, FALSE, NULL);
job_thread_ = ::CreateThread(NULL, 0, // Default security and stack.
@@ -143,10 +104,6 @@ BrokerServicesBase::~BrokerServicesBase() {
::CloseHandle(job_thread_);
delete thread_pool_;
::CloseHandle(no_targets_);
-
- if (security_descriptor_)
- ::LocalFree(security_descriptor_);
-
// If job_port_ isn't NULL, assumes that the lock has been initialized.
if (job_port_)
::DeleteCriticalSection(&lock_);
@@ -306,20 +263,13 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
// Create the TargetProces object and spawn the target suspended. Note that
// Brokerservices does not own the target object. It is owned by the Policy.
PROCESS_INFORMATION process_info = {0};
-
TargetProcess* target = new TargetProcess(initial_token, lockdown_token,
job, thread_pool_);
std::wstring desktop = policy_base->GetAlternateDesktop();
- // Set the security descriptor so the target picks up deny ACEs.
- SECURITY_ATTRIBUTES security_attributes = {sizeof(security_attributes),
- security_descriptor_,
- FALSE};
-
win_result = target->Create(exe_path, command_line,
desktop.empty() ? NULL : desktop.c_str(),
- &security_attributes,
&process_info);
if (ERROR_SUCCESS != win_result)
return SpawnCleanup(target, win_result);
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -74,9 +74,6 @@ class BrokerServicesBase : public BrokerServices,
// Handle to the worker thread that reacts to job notifications.
HANDLE job_thread_;
- // Copy of our security descriptor containing deny ACEs to block children.
- PSECURITY_DESCRIPTOR security_descriptor_;
-
// Lock used to protect the list of targets from being modified by 2
// threads at the same time.
CRITICAL_SECTION lock_;
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -340,40 +340,4 @@ DWORD SetProcessIntegrityLevel(IntegrityLevel integrity_level) {
return SetTokenIntegrityLevel(token.Get(), integrity_level);
}
-DWORD SetObjectDenyRestrictedAndNull(HANDLE handle, SE_OBJECT_TYPE type) {
- PSECURITY_DESCRIPTOR sec_desc = NULL;
- PACL old_dacl = NULL;
-
- DWORD error = ::GetSecurityInfo(handle, type, DACL_SECURITY_INFORMATION,
- NULL, NULL, &old_dacl, NULL, &sec_desc);
- if (!error) {
- Sid deny_sids[] = { Sid(WinNullSid), Sid(WinRestrictedCodeSid) };
- const int kDenySidsCount = sizeof(deny_sids) / sizeof(deny_sids[0]);
- EXPLICIT_ACCESS deny_aces[kDenySidsCount];
- ::ZeroMemory(deny_aces, sizeof(deny_aces));
-
- for (int i = 0; i < kDenySidsCount; ++i) {
- deny_aces[i].grfAccessMode = DENY_ACCESS;
- deny_aces[i].grfAccessPermissions = GENERIC_ALL;
- deny_aces[i].grfInheritance = NO_INHERITANCE;
- deny_aces[i].Trustee.TrusteeForm = TRUSTEE_IS_SID;
- deny_aces[i].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
- deny_aces[i].Trustee.ptstrName =
- reinterpret_cast<LPWSTR>(const_cast<SID*>(deny_sids[i].GetPSID()));
- }
-
- PACL new_dacl = NULL;
- error = ::SetEntriesInAcl(kDenySidsCount, deny_aces, old_dacl, &new_dacl);
- if (!error) {
- error = ::SetSecurityInfo(handle, type, DACL_SECURITY_INFORMATION,
- NULL, NULL, new_dacl, NULL);
- ::LocalFree(new_dacl);
- }
-
- ::LocalFree(sec_desc);
- }
-
- return error;
-}
-
} // namespace sandbox
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -78,12 +78,6 @@ DWORD SetTokenIntegrityLevel(HANDLE token, IntegrityLevel integrity_level);
// current integrity level, the function will fail.
DWORD SetProcessIntegrityLevel(IntegrityLevel integrity_level);
-// Adds deny ACEs on the supplied object for WinRestrictedCodeSid and
-// WinNullSid. This prevents the object from being accessible to sandboxed
-// processes. This prevents the object from being accessed by a sandboxed
-// process at USER_INTERACTIVE through USER_LOCKDOWN;
-DWORD SetObjectDenyRestrictedAndNull(HANDLE handle, SE_OBJECT_TYPE type);
-
} // namespace sandbox
#endif // SANDBOX_SRC_RESTRICTED_TOKEN_UTILS_H__
@@ -141,7 +141,6 @@ TargetProcess::~TargetProcess() {
DWORD TargetProcess::Create(const wchar_t* exe_path,
const wchar_t* command_line,
const wchar_t* desktop,
- PSECURITY_ATTRIBUTES security_attributes,
PROCESS_INFORMATION* target_info) {
exe_name_ = _wcsdup(exe_path);
@@ -163,7 +162,7 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
if (!::CreateProcessAsUserW(lockdown_token_,
exe_path,
cmd_line.get(),
- security_attributes,
+ NULL, // No security attribute.
NULL, // No thread attribute.
FALSE, // Do not inherit handles.
flags,
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -34,9 +34,7 @@ class TargetProcess {
// Creates the new target process. The process is created suspended.
DWORD Create(const wchar_t* exe_path, const wchar_t* command_line,
- const wchar_t* desktop,
- PSECURITY_ATTRIBUTES security_attributes,
- PROCESS_INFORMATION* target_info);
+ const wchar_t* desktop, PROCESS_INFORMATION* target_info);
// Destroys the target process.
void Terminate();
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-2010 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -105,19 +105,6 @@ TEST(ValidationSuite, TestProcess) {
EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(command));
}
-// Tests that sandboxed processes are explicitly denied access to the broker
-// (and, transitively, each other).
-TEST(ValidationSuite, TestProcessDenyAces) {
- TestRunner runner;
- wchar_t command[1024] = {0};
-
- runner.GetPolicy()->SetTokenLevel(USER_INTERACTIVE, USER_INTERACTIVE);
- runner.GetPolicy()->SetIntegrityLevel(INTEGRITY_LEVEL_MEDIUM);
-
- wsprintf(command, L"OpenProcessCmd %d", ::GetCurrentProcessId());
- EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(command));
-}
-
// Tests if the threads are correctly protected by the sandbox.
TEST(ValidationSuite, TestThread) {
TestRunner runner;

0 comments on commit 0c94262

Please sign in to comment.