Revert 128016 - Make sandbox explicitly block opening broker and sandboxed 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

7 files changed, 10 insertions, 121 deletions

diff --git a/sandbox/src/broker_services.cc b/sandbox/src/broker_services.cc
index 0b1fbe0..f6a0577 100644
--- a/sandbox/src/broker_services.cc
+++ b/sandbox/src/broker_services.cc
@@ -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,53 +42,20 @@ 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_);
 
@@ -98,10 +63,6 @@ ResultCode BrokerServicesBase::Init() {
   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);
diff --git a/sandbox/src/broker_services.h b/sandbox/src/broker_services.h
index adf2681..3e57dd2 100644
--- a/sandbox/src/broker_services.h
+++ b/sandbox/src/broker_services.h
@@ -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_;
diff --git a/sandbox/src/restricted_token_utils.cc b/sandbox/src/restricted_token_utils.cc
index c789471..b036e51 100644
--- a/sandbox/src/restricted_token_utils.cc
+++ b/sandbox/src/restricted_token_utils.cc
@@ -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
diff --git a/sandbox/src/restricted_token_utils.h b/sandbox/src/restricted_token_utils.h
index dd7b55c..0aade8b 100644
--- a/sandbox/src/restricted_token_utils.h
+++ b/sandbox/src/restricted_token_utils.h
@@ -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__
diff --git a/sandbox/src/target_process.cc b/sandbox/src/target_process.cc
index 4efbc18c..2710dc0 100644
--- a/sandbox/src/target_process.cc
+++ b/sandbox/src/target_process.cc
@@ -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,
diff --git a/sandbox/src/target_process.h b/sandbox/src/target_process.h
index 22fe08c..f20935d 100644
--- a/sandbox/src/target_process.h
+++ b/sandbox/src/target_process.h
@@ -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();
diff --git a/sandbox/tests/validation_tests/suite.cc b/sandbox/tests/validation_tests/suite.cc
index 7c3b1d7..a5886cd 100644
--- a/sandbox/tests/validation_tests/suite.cc
+++ b/sandbox/tests/validation_tests/suite.cc
@@ -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;