Fixing a couple of issues in sandbox::RestrictedToken:

 - Specify access bits on the duplicated handle correctly.
 - Avoid touching an uninitialized buffer in case of an error.

BUG=139841
TEST=RestrictedTokenTest.DenyOwnerSidCustom, RestrictedTokenTest.AddRestrictingSidCurrentUserCustom

Review URL: https://chromiumcodereview.appspot.com/10844003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@149475 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 82 insertions, 13 deletions

diff --git a/sandbox/win/src/restricted_token.cc b/sandbox/win/src/restricted_token.cc
index 8a096a1..6948d8a 100644
--- a/sandbox/win/src/restricted_token.cc
+++ b/sandbox/win/src/restricted_token.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -26,9 +26,9 @@ unsigned RestrictedToken::Init(const HANDLE effective_token) {
                           effective_token,
                           ::GetCurrentProcess(),
                           &effective_token_dup,
-                          DUPLICATE_SAME_ACCESS,
+                          0,
                           FALSE,
-                          0)) {  // no special options
+                          DUPLICATE_SAME_ACCESS)) {
       effective_token_ = effective_token_dup;
     } else {
       return ::GetLastError();
@@ -263,13 +263,16 @@ unsigned RestrictedToken::AddUserSidForDenyOnly() {
                                       size,
                                       &size);
 
-  Sid user = reinterpret_cast<SID*>(token_user->User.Sid);
-  delete[] reinterpret_cast<BYTE*>(token_user);
-
-  if (!result)
+  if (!result) {
+    delete[] reinterpret_cast<BYTE*>(token_user);
     return ::GetLastError();
+  }
 
+  Sid user = reinterpret_cast<SID*>(token_user->User.Sid);
   sids_for_deny_only_.push_back(user);
+
+  delete[] reinterpret_cast<BYTE*>(token_user);
+
   return ERROR_SUCCESS;
 }
 
@@ -323,6 +326,7 @@ unsigned RestrictedToken::DeleteAllPrivileges(
   }
 
   delete[] reinterpret_cast<BYTE *>(token_privileges);
+
   return ERROR_SUCCESS;
 }
 
@@ -406,14 +410,16 @@ unsigned RestrictedToken::AddRestrictingSidCurrentUser() {
                                       size,
                                       &size);
 
-  Sid user = reinterpret_cast<SID*>(token_user->User.Sid);
-  delete[] reinterpret_cast<BYTE*>(token_user);
-
-
-  if (!result)
+  if (!result) {
+    delete[] reinterpret_cast<BYTE*>(token_user);
     return ::GetLastError();
+  }
 
+  Sid user = reinterpret_cast<SID*>(token_user->User.Sid);
   sids_to_restrict_.push_back(user);
+
+  delete[] reinterpret_cast<BYTE*>(token_user);
+
   return ERROR_SUCCESS;
 }
 
diff --git a/sandbox/win/src/restricted_token_unittest.cc b/sandbox/win/src/restricted_token_unittest.cc
index df35f1c..4948ad1 100644
--- a/sandbox/win/src/restricted_token_unittest.cc
+++ b/sandbox/win/src/restricted_token_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -292,6 +292,44 @@ TEST(RestrictedTokenTest, DenyOwnerSid) {
   }
 }
 
+// Tests test method AddOwnerSidForDenyOnly with a custom effective token.
+TEST(RestrictedTokenTest, DenyOwnerSidCustom) {
+  // Get the current process token.
+  HANDLE token_handle = INVALID_HANDLE_VALUE;
+  ASSERT_TRUE(::OpenProcessToken(::GetCurrentProcess(), TOKEN_ALL_ACCESS,
+                                 &token_handle));
+
+  ASSERT_NE(INVALID_HANDLE_VALUE, token_handle);
+
+  ATL::CAccessToken access_token;
+  access_token.Attach(token_handle);
+
+  RestrictedToken token;
+  ASSERT_EQ(ERROR_SUCCESS, token.Init(access_token.GetHandle()));
+  ASSERT_EQ(ERROR_SUCCESS, token.AddUserSidForDenyOnly());
+  ASSERT_EQ(ERROR_SUCCESS, token.GetRestrictedTokenHandle(&token_handle));
+
+  ATL::CAccessToken restricted_token;
+  restricted_token.Attach(token_handle);
+
+  ATL::CTokenGroups groups;
+  ASSERT_TRUE(restricted_token.GetGroups(&groups));
+
+  ATL::CSid::CSidArray sids;
+  ATL::CAtlArray<DWORD> attributes;
+  groups.GetSidsAndAttributes(&sids, &attributes);
+
+  ATL::CSid user_sid;
+  ASSERT_TRUE(restricted_token.GetUser(&user_sid));
+
+  for (unsigned int i = 0; i < sids.GetCount(); ++i) {
+    if (user_sid == sids[i]) {
+      ASSERT_EQ(SE_GROUP_USE_FOR_DENY_ONLY,
+                attributes[i] & SE_GROUP_USE_FOR_DENY_ONLY);
+    }
+  }
+}
+
 // Tests the method DeleteAllPrivileges.
 TEST(RestrictedTokenTest, DeleteAllPrivileges) {
   RestrictedToken token;
@@ -433,6 +471,31 @@ TEST(RestrictedTokenTest, AddRestrictingSidCurrentUser) {
   CheckRestrictingSid(restricted_token, user, 1);
 }
 
+// Tests the method AddRestrictingSidCurrentUser with a custom effective token.
+TEST(RestrictedTokenTest, AddRestrictingSidCurrentUserCustom) {
+  // Get the current process token.
+  HANDLE token_handle = INVALID_HANDLE_VALUE;
+  ASSERT_TRUE(::OpenProcessToken(::GetCurrentProcess(), TOKEN_ALL_ACCESS,
+                                 &token_handle));
+
+  ASSERT_NE(INVALID_HANDLE_VALUE, token_handle);
+
+  ATL::CAccessToken access_token;
+  access_token.Attach(token_handle);
+
+  RestrictedToken token;
+  ASSERT_EQ(ERROR_SUCCESS, token.Init(access_token.GetHandle()));
+  ASSERT_EQ(ERROR_SUCCESS, token.AddRestrictingSidCurrentUser());
+  ASSERT_EQ(ERROR_SUCCESS, token.GetRestrictedTokenHandle(&token_handle));
+
+  ATL::CAccessToken restricted_token;
+  restricted_token.Attach(token_handle);
+  ATL::CSid user;
+  restricted_token.GetUser(&user);
+
+  CheckRestrictingSid(restricted_token, user, 1);
+}
+
 // Tests the method AddRestrictingSidLogonSession.
 TEST(RestrictedTokenTest, AddRestrictingSidLogonSession) {
   RestrictedToken token;