Remove the --allow-no-sandbox-job flag in favor of automatic recognition.

content/common/sandbox_win.cc

 // 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.
 
 #include "content/common/sandbox_win.h"
 
 #include <stddef.h>
 
 #include <string>
 
@@ skipping 233 matching lines @@
     CloseHandle(token);
     if (session_id)
       s_session_id = session_id;
   }
 
   return base::StringPrintf(L"\\Sessions\\%lu%ls", s_session_id, object);
 }
 
 // Checks if the sandbox should be let to run without a job object assigned.
 bool ShouldSetJobLevel(const base::CommandLine& cmd_line) {
-  if (!cmd_line.HasSwitch(switches::kAllowNoSandboxJob))
-    return true;
-
   // Windows 8 allows nested jobs so we don't need to check if we are in other
   // job.
   if (base::win::GetVersion() >= base::win::VERSION_WIN8)
     return true;
 
   BOOL in_job = true;
   // Either there is no job yet associated so we must add our job,
   if (!::IsProcessInJob(::GetCurrentProcess(), NULL, &in_job))
     NOTREACHED() << "IsProcessInJob failed. " << GetLastError();
   if (!in_job)
     return true;
 
   // ...or there is a job but the JOB_OBJECT_LIMIT_BREAKAWAY_OK limit is set.
   JOBOBJECT_EXTENDED_LIMIT_INFORMATION job_info = {};
   if (!::QueryInformationJobObject(NULL,
                                    JobObjectExtendedLimitInformation, &job_info,
                                    sizeof(job_info), NULL)) {
     NOTREACHED() << "QueryInformationJobObject failed. " << GetLastError();
     return true;
   }
   if (job_info.BasicLimitInformation.LimitFlags & JOB_OBJECT_LIMIT_BREAKAWAY_OK)
     return true;
 
+  // Lastly in place of the flag which was supposed to be used only for running
+  // Chrome in remote sessions we do this check explicitly here.
+  // According to MS this flag can be false for a remote session only on Windows
+  // Server 2012 and newer so if we do the check last we should be on the safe
+  // side. See: https://msdn.microsoft.com/en-us/library/aa380798.aspx.
+  if (!::GetSystemMetrics(SM_REMOTESESSION)) {
+    // TODO(pastarmovj): Remove this check and the flag altogether once we are
+    // convinced that the automatic logic is good enough.
+    return !cmd_line.HasSwitch(switches::kAllowNoSandboxJob);
+  }
+
+  // Allow running without the sandbox in this case. This slightly reduces the
+  // ability of the sandbox to protect its children from spawning new processes
+  // or preventing them from shutting down Windows or accessing the clipboard.
   return false;
 }
 
 // Adds the generic policy rules to a sandbox TargetPolicy.
 sandbox::ResultCode AddGenericPolicy(sandbox::TargetPolicy* policy) {
   sandbox::ResultCode result;
 
   // Add the policy for the client side of a pipe. It is just a file
   // in the \pipe\ namespace. We restrict it to pipes that start with
   // "chrome." so the sandboxed process cannot connect to system services.
@@ skipping 273 matching lines @@
   return base::StartsWith(appcontainer_group_name, "Enabled",
                           base::CompareCase::INSENSITIVE_ASCII);
 }
 
 }  // namespace
 
 sandbox::ResultCode SetJobLevel(const base::CommandLine& cmd_line,
                                 sandbox::JobLevel job_level,
                                 uint32_t ui_exceptions,
                                 sandbox::TargetPolicy* policy) {
-  if (!ShouldSetJobLevel(cmd_line))
+  const bool should_set_job = ShouldSetJobLevel(cmd_line);
+  UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.ShouldSetJobNone", should_set_job);

[Will Harris, 2017/02/16 23:48:17]

I feel like I'm being a terrible reviewer here and making you go round in
circles but I think the point of having the UMA histogram is to detect the times
when

 - enterprise is using --allow-no-sandbox-job
 - had they not used this switch, then the sandbox would not have correctly
worked.

I think the code right now just reports when --allow-no-sandbox-job is being
used, but not when it would have worked had it not been used.

+  if (!should_set_job)
     return policy->SetJobLevel(sandbox::JOB_NONE, 0);
 
 #ifdef _WIN64
   sandbox::ResultCode ret =
       policy->SetJobMemoryLimit(4ULL * 1024 * 1024 * 1024);
   if (ret != sandbox::SBOX_ALL_OK)
     return ret;
 #endif
   return policy->SetJobLevel(job_level, ui_exceptions);
 }
@@ skipping 249 matching lines @@
   }
 
   delegate->PostSpawnTarget(target.process_handle());
 
   CHECK(ResumeThread(target.thread_handle()) != static_cast<DWORD>(-1));
   *process = base::Process(target.TakeProcessHandle());
   return sandbox::SBOX_ALL_OK;
 }
 
 }  // namespace content