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) {

[Will Harris, 2017/02/13 18:28:55]

so behavior before was that if --allow-no-sandbox-job was not specified then the
job object was always applied

behavior now is that that now tests are always run and if any of:

 - version >= win8
 - not in job
 - inside a job but it's JOB_OBJECT_LIMIT_BREAKAWAY_OK
 - not a remote session

then return true, otherwise return false.

I think what we want to UMA is the times that none of the above hit, but the
user has specified --allow-no-sandbox-job to override.

Right now the only time we record the histogram is for the final part.

So perhaps the solution here is to just move the code to the parent function and
record the whether histogram --allow-no-sandbox-job is specified or not, if
ShouldSetJobLevel returns true.

This will catch some false positives e.g. version > = win8 but I think we can
filter UMA by this. WDYT?

Or maybe I am overthinking this. I've spent an hour so far looking at this CL.

-  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;

[Will Harris, 2017/02/10 18:45:35]

are you sure that one of these earlier tests won't just pass and just return
true?

e.g. if Citrix is really running chrome browser processes inside an existing Job
object - does it set JOB_OBJECT_LIMIT_BREAKAWAY_OK?

[pastarmovj, 2017/02/13 12:26:41]

It is not setting the breakaway flag unfortunately. Also if any of those is true
there is no reason we should allow Chrome to run without a Job this is why those
return true regardless of the flag.

   }
   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)) {
+    // Measure how often we would have decided to apply the sandbox but the
+    // user actually wanted to avoid it.
+    // TODO(pastarmovj): Remove this check and the flag altogher once we are

[Will Harris, 2017/02/10 18:45:35]

nit: typo

[pastarmovj, 2017/02/14 12:10:33]

Done.

+    // convinced that the automatic logic is good enough.
+    if (cmd_line.HasSwitch(switches::kAllowNoSandboxJob)) {
+      UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.JobAvoidedCorrectly", false);

[Will Harris, 2017/02/10 18:45:35]

I'd prefer to record this histogram all the time and have a variety of cases, or
only record it once - as data analysis is hard when it's only sometimes
recorded.

[pastarmovj, 2017/02/13 12:26:41]

As the histogram is worded now it is collected every time the decision has to be
made (compared to the old logic). I guess what you are asking is to make it not
comparing the old logic but in general track the "false" state if we ever return
true. Or alternatively have an enum tracking every reason why the job is to be
applied or not applied? Am I correct about that?

[Will Harris, 2017/02/13 18:28:55]

normally best practice for histograms is to always record them but vary the
value being recorded. This makes analysis easier, and also reduces code size as
each histogram macro expands quite a bit in code.

[pastarmovj, 2017/02/14 12:10:33]

Agree. Moved that to the calling function.

+      return false;
+    }
+    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.
+  UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.JobAvoidedCorrectly", true);
   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 543 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