Skip to content

Commit

Permalink
Revert 253415 "Reduce sandbox permissions granted to alternate d..."
Browse files Browse the repository at this point in the history 
This is being reverted due to ValidationSuite.TestAlternateDesktop failures
on the Vista and XP bots.

> Reduce sandbox permissions granted to alternate desktop
> 
> This pass adds the first round of deny ACEs for the Winstation and
> Desktop objects. Assuming these stick, I'll get more aggressive in
> a follow-up.
> 
> BUG=346586
> 
> Review URL: https://codereview.chromium.org/178423005

TBR=jschuh@chromium.org

Review URL: https://codereview.chromium.org/180243011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@253438 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
robertphillips@google.com committed Feb 26, 2014
1 parent be13a8d commit e4c85c3
Show file tree
Hide file tree
Showing 7 changed files with 26 additions and 129 deletions.
7 changes: 3 additions & 4 deletions sandbox/win/sandbox_poc/main_ui_window.cc
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2014 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.

Expand Down Expand Up @@ -553,9 +553,8 @@ bool MainUIWindow::SpawnTarget() {
if (INVALID_HANDLE_VALUE == pipe_handle_)
AddDebugMessage(L"Failed to create pipe. Error %d", ::GetLastError());

if (!sandbox::AddKnownSidToObject(pipe_handle_, SE_KERNEL_OBJECT,
WinWorldSid, GRANT_ACCESS,
FILE_ALL_ACCESS))
if (!sandbox::AddKnownSidToKernelObject(pipe_handle_, WinWorldSid,
FILE_ALL_ACCESS))
AddDebugMessage(L"Failed to set security on pipe. Error %d",
::GetLastError());

Expand Down
18 changes: 8 additions & 10 deletions sandbox/win/src/acl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ bool GetDefaultDacl(HANDLE token,
return true;
}

bool AddSidToDacl(const Sid& sid, ACL* old_dacl, ACCESS_MODE access_mode,
ACCESS_MASK access, ACL** new_dacl) {
bool AddSidToDacl(const Sid& sid, ACL* old_dacl, ACCESS_MASK access,
ACL** new_dacl) {
EXPLICIT_ACCESS new_access = {0};
new_access.grfAccessMode = access_mode;
new_access.grfAccessMode = GRANT_ACCESS;
new_access.grfAccessPermissions = access;
new_access.grfInheritance = NO_INHERITANCE;

Expand All @@ -64,8 +64,7 @@ bool AddSidToDefaultDacl(HANDLE token, const Sid& sid, ACCESS_MASK access) {
return false;

ACL* new_dacl = NULL;
if (!AddSidToDacl(sid, default_dacl->DefaultDacl, GRANT_ACCESS, access,
&new_dacl))
if (!AddSidToDacl(sid, default_dacl->DefaultDacl, access, &new_dacl))
return false;

TOKEN_DEFAULT_DACL new_token_dacl = {0};
Expand All @@ -91,9 +90,8 @@ bool AddUserSidToDefaultDacl(HANDLE token, ACCESS_MASK access) {
access);
}

bool AddKnownSidToObject(HANDLE object, SE_OBJECT_TYPE object_type,
const Sid& sid, ACCESS_MODE access_mode,
ACCESS_MASK access) {
bool AddKnownSidToKernelObject(HANDLE object, const Sid& sid,
ACCESS_MASK access) {
PSECURITY_DESCRIPTOR descriptor = NULL;
PACL old_dacl = NULL;
PACL new_dacl = NULL;
Expand All @@ -103,12 +101,12 @@ bool AddKnownSidToObject(HANDLE object, SE_OBJECT_TYPE object_type,
&old_dacl, NULL, &descriptor))
return false;

if (!AddSidToDacl(sid.GetPSID(), old_dacl, access_mode, access, &new_dacl)) {
if (!AddSidToDacl(sid.GetPSID(), old_dacl, access, &new_dacl)) {
::LocalFree(descriptor);
return false;
}

DWORD result = ::SetSecurityInfo(object, object_type,
DWORD result = ::SetSecurityInfo(object, SE_KERNEL_OBJECT,
DACL_SECURITY_INFORMATION, NULL, NULL,
new_dacl, NULL);

Expand Down
20 changes: 9 additions & 11 deletions sandbox/win/src/acl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#ifndef SANDBOX_SRC_ACL_H_
#define SANDBOX_SRC_ACL_H_

#include <AccCtrl.h>
#include <windows.h>

#include "base/memory/scoped_ptr.h"
Expand All @@ -17,11 +16,11 @@ namespace sandbox {
bool GetDefaultDacl(HANDLE token,
scoped_ptr_malloc<TOKEN_DEFAULT_DACL>* default_dacl);

// Appends an ACE represented by |sid|, |access_mode|, and |access| to
// |old_dacl|. If the function succeeds, new_dacl contains the new dacl and
// must be freed using LocalFree.
bool AddSidToDacl(const Sid& sid, ACL* old_dacl, ACCESS_MODE access_mode,
ACCESS_MASK access, ACL** new_dacl);
// Appends an ACE represented by |sid| and |access| to |old_dacl|. If the
// function succeeds, new_dacl contains the new dacl and must be freed using
// LocalFree.
bool AddSidToDacl(const Sid& sid, ACL* old_dacl, ACCESS_MASK access,
ACL** new_dacl);

// Adds and ACE represented by |sid| and |access| to the default dacl present
// in the token.
Expand All @@ -31,11 +30,10 @@ bool AddSidToDefaultDacl(HANDLE token, const Sid& sid, ACCESS_MASK access);
// present in the token.
bool AddUserSidToDefaultDacl(HANDLE token, ACCESS_MASK access);

// Adds an ACE represented by |known_sid|, |access_mode|, and |access| to
// the dacl of the kernel object referenced by |object| and of |object_type|.
bool AddKnownSidToObject(HANDLE object, SE_OBJECT_TYPE object_type,
const Sid& sid, ACCESS_MODE access_mode,
ACCESS_MASK access);
// Adds an ACE represented by |known_sid| and |access| to the dacl of the kernel
// object referenced by |object|.
bool AddKnownSidToKernelObject(HANDLE object, const Sid& sid,
ACCESS_MASK access);

} // namespace sandbox

Expand Down
28 changes: 2 additions & 26 deletions sandbox/win/src/window.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@

#include "base/logging.h"
#include "base/memory/scoped_ptr.h"
#include "sandbox/win/src/acl.h"
#include "sandbox/win/src/sid.h"

namespace {

Expand Down Expand Up @@ -48,20 +46,8 @@ ResultCode CreateAltWindowStation(HWINSTA* winsta) {
*winsta = ::CreateWindowStationW(NULL, 0, WINSTA_ALL_ACCESS, &attributes);
LocalFree(attributes.lpSecurityDescriptor);

if (*winsta) {
// Replace the DACL on the new Winstation with a reduced privilege version.
// We can soft fail on this for now, as it's just an extra mitigation.
static const ACCESS_MASK kWinstaDenyMask = DELETE | WRITE_DAC |
WRITE_OWNER |
WINSTA_ACCESSCLIPBOARD |
WINSTA_CREATEDESKTOP |
WINSTA_ENUMDESKTOPS |
WINSTA_ENUMERATE |
WINSTA_EXITWINDOWS;
AddKnownSidToObject(*winsta, SE_WINDOW_OBJECT, Sid(WinRestrictedCodeSid),
DENY_ACCESS, kWinstaDenyMask);
if (*winsta)
return SBOX_ALL_OK;
}

return SBOX_ERROR_CANNOT_CREATE_WINSTATION;
}
Expand Down Expand Up @@ -108,18 +94,8 @@ ResultCode CreateAltDesktop(HWINSTA winsta, HDESK* desktop) {
}
}

if (*desktop) {
// Replace the DACL on the new Desktop with a reduced privilege version.
// We can soft fail on this for now, as it's just an extra mitigation.
static const ACCESS_MASK kDesktopDenyMask = WRITE_DAC | WRITE_OWNER |
DESKTOP_HOOKCONTROL |
DESKTOP_JOURNALPLAYBACK |
DESKTOP_JOURNALRECORD |
DESKTOP_SWITCHDESKTOP;
AddKnownSidToObject(*desktop, SE_WINDOW_OBJECT, Sid(WinRestrictedCodeSid),
DENY_ACCESS, kDesktopDenyMask);
if (*desktop)
return SBOX_ALL_OK;
}

return SBOX_ERROR_CANNOT_CREATE_DESKTOP;
}
Expand Down
58 changes: 3 additions & 55 deletions sandbox/win/tests/validation_tests/commands.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include <Aclapi.h>
#include <windows.h>
#include <string>

Expand Down Expand Up @@ -241,62 +240,11 @@ SBOX_TESTS_COMMAND int SwitchToSboxDesktop(int, wchar_t **) {
}

int TestSwitchDesktop() {
HDESK desktop = ::GetThreadDesktop(::GetCurrentThreadId());
if (NULL == desktop) {
HDESK sbox_desk = ::GetThreadDesktop(::GetCurrentThreadId());
if (NULL == sbox_desk) {
return SBOX_TEST_FAILED;
}
if (::SwitchDesktop(desktop)) {
return SBOX_TEST_SUCCEEDED;
}
return SBOX_TEST_DENIED;
}

SBOX_TESTS_COMMAND int OpenAlternateDesktop(int, wchar_t **argv) {
return TestOpenAlternateDesktop(argv[0]);
}

int TestOpenAlternateDesktop(wchar_t *desktop_name) {
// Test for WRITE_DAC permission on the handle.
HDESK desktop = ::GetThreadDesktop(::GetCurrentThreadId());
if (desktop) {
HANDLE test_handle;
if (::DuplicateHandle(::GetCurrentProcess(), desktop,
::GetCurrentProcess(), &test_handle,
WRITE_DAC, FALSE, 0)) {
DWORD result = ::SetSecurityInfo(test_handle, SE_WINDOW_OBJECT,
DACL_SECURITY_INFORMATION, NULL, NULL,
NULL, NULL);
::CloseHandle(test_handle);
if (result != ERROR_ACCESS_DENIED) {
return SBOX_TEST_SUCCEEDED;
}
} else if (::GetLastError() != ERROR_ACCESS_DENIED) {
return SBOX_TEST_SUCCEEDED;
}
}

// Open by name with WRITE_DAC.
if ((desktop = ::OpenDesktop(desktop_name, 0, FALSE, WRITE_DAC)) ||
::GetLastError() != ERROR_ACCESS_DENIED) {
::CloseDesktop(desktop);
return SBOX_TEST_SUCCEEDED;
}

return SBOX_TEST_DENIED;
}

BOOL CALLBACK DesktopTestEnumProc(LPTSTR desktop_name, LPARAM result) {
return TRUE;
}

SBOX_TESTS_COMMAND int EnumAlternateWinsta(int, wchar_t **) {
return TestEnumAlternateWinsta();
}

int TestEnumAlternateWinsta() {
int result = SBOX_TEST_DENIED;
// Try to enumerate the destops on the alternate windowstation.
if (::EnumDesktopsW(NULL, DesktopTestEnumProc, 0)) {
if (::SwitchDesktop(sbox_desk)) {
return SBOX_TEST_SUCCEEDED;
}
return SBOX_TEST_DENIED;
Expand Down
7 changes: 0 additions & 7 deletions sandbox/win/tests/validation_tests/commands.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,6 @@ int TestOpenInputDesktop();
// Tries to switch the interactive desktop. Returns a SboxTestResult.
int TestSwitchDesktop();

// Tries to open the alternate desktop. Returns a SboxTestResult.
int TestOpenAlternateDesktop(wchar_t *desktop_name);

// Tries to enumerate desktops on the alternate windowstation.
// Returns a SboxTestResult.
int TestEnumAlternateWinsta();

} // namespace sandbox

#endif // SANDBOX_TESTS_VALIDATION_TESTS_COMMANDS_H__
17 changes: 1 addition & 16 deletions sandbox/win/tests/validation_tests/suite.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,26 +111,11 @@ TEST(ValidationSuite, TestRegistry) {
// to get to the interactive desktop or to make the sbox desktop interactive.
TEST(ValidationSuite, TestDesktop) {
TestRunner runner;
runner.GetPolicy()->SetAlternateDesktop(true);
runner.GetPolicy()->SetAlternateDesktop(false);
EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(L"OpenInteractiveDesktop NULL"));
EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(L"SwitchToSboxDesktop NULL"));
}

// Tests that the permissions on the Windowstation does not allow the sandbox
// to get to the interactive desktop or to make the sbox desktop interactive.
TEST(ValidationSuite, TestAlternateDesktop) {
wchar_t command[1024] = {0};
TestRunner runner;
runner.SetTimeout(3600000);
runner.GetPolicy()->SetAlternateDesktop(true);
runner.GetPolicy()->SetDelayedIntegrityLevel(INTEGRITY_LEVEL_UNTRUSTED);
base::string16 desktop_name = runner.GetPolicy()->GetAlternateDesktop();
desktop_name = desktop_name.substr(desktop_name.find('\\') + 1);
wsprintf(command, L"OpenAlternateDesktop %lS", desktop_name.c_str());
EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(command));
EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(L"EnumAlternateWinsta NULL"));
}

// Tests if the windows are correctly protected by the sandbox.
TEST(ValidationSuite, TestWindows) {
TestRunner runner;
Expand Down

0 comments on commit e4c85c3

Please sign in to comment.