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 232 matching lines @@
         sizeof(session_id), &session_id_length));
     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;
-
+bool ShouldSetJobLevel() {
   // 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

[Will Harris, 2017/02/03 17:38:50]

can you link the source?

[pastarmovj, 2017/02/06 13:05:27]

Done.

+  // Server 2012 and newer so if we do the check last we should be on the safe

[Will Harris, 2017/02/03 17:38:50]

we can't just check for this version of OS instead?

How come we can't just detect the Job, as above? Why is that code not correctly
detecting the failure?

[pastarmovj, 2017/02/06 13:05:27]

Not sure what do you mean? What I am trying to say is that if we were to rely on
that on a newer Windows version we might end up assuming we should not allow the
Job escape hatch while we are actually running in a remote session. But at least
according to the documentation this function is always correct for Windows
Server 2008 . The check above on line 256 do check the OS version.

+  // side.
+  if (!::GetSystemMetrics(SM_REMOTESESSION))

[Will Harris, 2017/02/03 17:38:50]

how is this tested? is there a set of automated tests, or a test plan?

[pastarmovj, 2017/02/06 13:05:27]

This is something I will start another thread about this week :)

Right now I tested it by building a fresh branded build and running it on my
Terminal Services installation on SkyTap.

I would love to have some automated testing and have started doing some tests
how to build that but I will love some more input on this however it is beyond
the scope of this CL I think.

+    return true;
+
+  // 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 269 matching lines @@
   if (command_line.HasSwitch(switches::kDisableAppContainer))
     return false;
   if (command_line.HasSwitch(switches::kEnableAppContainer))
     return true;
   return base::StartsWith(appcontainer_group_name, "Enabled",
                           base::CompareCase::INSENSITIVE_ASCII);
 }
 
 }  // namespace
 
-sandbox::ResultCode SetJobLevel(const base::CommandLine& cmd_line,
-                                sandbox::JobLevel job_level,
+sandbox::ResultCode SetJobLevel(sandbox::JobLevel job_level,
                                 uint32_t ui_exceptions,
                                 sandbox::TargetPolicy* policy) {
-  if (!ShouldSetJobLevel(cmd_line))
+  if (!ShouldSetJobLevel())
     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 93 matching lines @@
     base::CommandLine* cmd_line,
     const base::HandlesToInheritVector& handles_to_inherit,
     base::Process* process) {
   DCHECK(delegate);
   const base::CommandLine& browser_command_line =
       *base::CommandLine::ForCurrentProcess();
   std::string type_str = cmd_line->GetSwitchValueASCII(switches::kProcessType);
 
   TRACE_EVENT1("startup", "StartProcessWithAccess", "type", type_str);
 
-  // Propagate the --allow-no-job flag if present.
-  if (browser_command_line.HasSwitch(switches::kAllowNoSandboxJob) &&
-      !cmd_line->HasSwitch(switches::kAllowNoSandboxJob)) {
-    cmd_line->AppendSwitch(switches::kAllowNoSandboxJob);
-  }
-
   ProcessDebugFlags(cmd_line);
 
   if ((!delegate->ShouldSandbox()) ||
       browser_command_line.HasSwitch(switches::kNoSandbox) ||
       cmd_line->HasSwitch(switches::kNoSandbox)) {
     base::LaunchOptions options;
 
     base::HandlesToInheritVector handles = handles_to_inherit;
     if (!handles_to_inherit.empty()) {
       options.inherit_handles = true;
@@ skipping 40 matching lines @@
 #endif
 
   // Post-startup mitigations.
   mitigations = sandbox::MITIGATION_STRICT_HANDLE_CHECKS |
                 sandbox::MITIGATION_DLL_SEARCH_ORDER;
 
   result = policy->SetDelayedProcessMitigations(mitigations);
   if (result != sandbox::SBOX_ALL_OK)
     return result;
 
-  result = SetJobLevel(*cmd_line, sandbox::JOB_LOCKDOWN, 0, policy);
+  result = SetJobLevel(sandbox::JOB_LOCKDOWN, 0, policy);
   if (result != sandbox::SBOX_ALL_OK)
     return result;
 
   if (!delegate->DisableDefaultPolicy()) {
     result = AddPolicyForSandboxedProcess(policy);
     if (result != sandbox::SBOX_ALL_OK)
       return result;
   }
 
 #if !defined(NACL_WIN64)
@@ skipping 68 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