Sandbox: Make PolicyBase::MakeTokens return ScopedHandes

sandbox/win/src/sandbox_policy_base.cc

Index: sandbox/win/src/sandbox_policy_base.cc
diff --git a/sandbox/win/src/sandbox_policy_base.cc b/sandbox/win/src/sandbox_policy_base.cc
index 6df2cb3d3819d33c2e742efdc441192de16ca996..57232b1de779fd3ef0ba1361747508b8522d1852 100644
--- a/sandbox/win/src/sandbox_policy_base.cc
+++ b/sandbox/win/src/sandbox_policy_base.cc
@@ -547,7 +547,8 @@ ResultCode PolicyBase::MakeJobObject(HANDLE* job) {
   return SBOX_ALL_OK;
 }
 
-ResultCode PolicyBase::MakeTokens(HANDLE* initial, HANDLE* lockdown) {
+ResultCode PolicyBase::MakeTokens(base::win::ScopedHandle* initial,
+                                  base::win::ScopedHandle* lockdown) {
   if (appcontainer_list_.get() && appcontainer_list_->HasAppContainer() &&
       lowbox_sid_) {
     return SBOX_ERROR_BAD_PARAMS;
@@ -555,11 +556,14 @@ ResultCode PolicyBase::MakeTokens(HANDLE* initial, HANDLE* lockdown) {
 
   // Create the 'naked' token. This will be the permanent token associated
   // with the process and therefore with any thread that is not impersonating.
-  DWORD result = CreateRestrictedToken(lockdown, lockdown_level_,
+  HANDLE temp_handle;
+  DWORD result = CreateRestrictedToken(&temp_handle, lockdown_level_,
                                        integrity_level_, PRIMARY);
   if (ERROR_SUCCESS != result)
     return SBOX_ERROR_GENERIC;
 
+  lockdown->Set(temp_handle);
+
   // If we're launching on the alternate desktop we need to make sure the
   // integrity label on the object is no higher than the sandboxed process's
   // integrity level. So, we lower the label on the desktop process if it's
@@ -590,9 +594,11 @@ ResultCode PolicyBase::MakeTokens(HANDLE* initial, HANDLE* lockdown) {
     if (lockdown_level_ < USER_LIMITED || lockdown_level_ != initial_level_)
       return SBOX_ERROR_CANNOT_INIT_APPCONTAINER;
 
-    *initial = INVALID_HANDLE_VALUE;
+    *initial = base::win::ScopedHandle();
     return SBOX_ALL_OK;
-  } else if (lowbox_sid_) {
+  }
+
+  if (lowbox_sid_) {

[Will Harris, 2015/07/10 18:11:31]

maybe DCHECK that we are not also using AppContainer since these approaches are
mutually exclusive.

[rvargas (doing something else), 2015/07/10 18:31:56]

Do you mean line the if at line 522? :)

[Will Harris, 2015/07/10 18:45:07]

ah yes. :)

     NtCreateLowBoxToken CreateLowBoxToken = NULL;
     ResolveNTFunctionPtr("NtCreateLowBoxToken", &CreateLowBoxToken);
     OBJECT_ATTRIBUTES obj_attr;
@@ -608,7 +614,7 @@ ResultCode PolicyBase::MakeTokens(HANDLE* initial, HANDLE* lockdown) {
     HANDLE saved_handles[1] = {lowbox_directory_.Get()};
     DWORD saved_handles_count = lowbox_directory_.IsValid() ? 1 : 0;
 
-    NTSTATUS status = CreateLowBoxToken(&token_lowbox, *lockdown,
+    NTSTATUS status = CreateLowBoxToken(&token_lowbox, lockdown->Get(),
                                         TOKEN_ALL_ACCESS, &obj_attr,
                                         lowbox_sid_, 0, NULL,
                                         saved_handles_count, saved_handles);
@@ -616,19 +622,18 @@ ResultCode PolicyBase::MakeTokens(HANDLE* initial, HANDLE* lockdown) {
       return SBOX_ERROR_GENERIC;
 
     DCHECK(token_lowbox);
-    ::CloseHandle(*lockdown);
-    *lockdown = token_lowbox;
+    lockdown->Set(token_lowbox);

[Will Harris, 2015/07/10 18:11:31]

scoped handles really does make this a lot more elegant :)

   }
 
   // Create the 'better' token. We use this token as the one that the main
   // thread uses when booting up the process. It should contain most of
   // what we need (before reaching main( ))
-  result = CreateRestrictedToken(initial, initial_level_,
+  result = CreateRestrictedToken(&temp_handle, initial_level_,
                                  integrity_level_, IMPERSONATION);
-  if (ERROR_SUCCESS != result) {
-    ::CloseHandle(*lockdown);
+  if (ERROR_SUCCESS != result)
     return SBOX_ERROR_GENERIC;
-  }
+
+  initial->Set(temp_handle);
   return SBOX_ALL_OK;
 }