Change Win32k PPAPI lockdown to use finch params for mime type.

content/browser/ppapi_plugin_process_host.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/browser/ppapi_plugin_process_host.h"
 
 #include <stddef.h>
 
 #include <string>
 #include <utility>
@@ skipping 18 matching lines @@
 #include "content/public/common/pepper_plugin_info.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/sandbox_type.h"
 #include "content/public/common/sandboxed_process_launcher_delegate.h"
 #include "ipc/ipc_switches.h"
 #include "net/base/network_change_notifier.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ui/base/ui_base_switches.h"
 
 #if defined(OS_WIN)
+#include "base/strings/string_tokenizer.h"
+#include "base/strings/string_util.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h"
 #include "content/common/sandbox_win.h"
 #include "sandbox/win/src/process_mitigations.h"
 #include "sandbox/win/src/sandbox_policy.h"
 #include "ui/gfx/win/dpi.h"
 #endif
 
 namespace content {
 
+#if defined(OS_WIN)
+namespace {
+
+// Returns whether Win32k PPAPI lockdown is enabled for a specific mime type.
+bool IsWin32kLockdownEnabledForMimeType(const std::string& mime_type) {
+  // Consider PPAPI lockdown a superset of renderer lockdown.
+  if (!IsWin32kRendererLockdownEnabled())
+    return false;
+
+  std::map<std::string, std::string> mime_params;
+

[Alexei Svitkine (slow), 2016/01/20 16:19:52]

Nit: Remove empty line.

[Will Harris, 2016/01/25 19:50:28]

Done.

+  if (variations::GetVariationParams("EnableWin32kLockDownMimeTypes",

[Will Harris, 2016/01/25 19:08:10]

Q: should this call be above line 57 to ensure that all clients get placed into
an experiment group? Line 57 will return false for all platforms before windows
8, so I suppose it depends whether I want to filter the experiment groups on the
client or the server, what do you think is best here?

[Alexei Svitkine (slow), 2016/01/25 19:32:48]

So, if its here, those users won't show up on the dashboard. The other option is
to move this above and then use go/variations-flags in the config, so that you
can see the users who explicitly use this flag via the dashboard. Whichever you
prefer. However, if you do move it above, you should make sure to have the flag
in the config, else you'll get misleading data for these users (they'll show up
as being in the group even if it's disabled).

[Will Harris, 2016/01/25 19:35:39]

sounds like leaving it here is easier, so users who can't use win32k lockdown
just won't appear in the experiment at all. Thanks!

+                                     &mime_params)) {
+    bool enabled = false;
+    for (auto param : mime_params) {

[Alexei Svitkine (slow), 2016/01/22 19:27:50]

Nit: const auto&

[Will Harris, 2016/01/25 19:50:28]

Done.

+      if (param.first == mime_type || param.first == "*") {

[Alexei Svitkine (slow), 2016/01/20 16:19:52]

Hmm, I was thinking you just have a single "MimeTypes" param and treat its
content the same as what's specified via the kEnableWin32kLockDownMimeTypes
command-line. That way, you have a single code path to parse these params, which
seems simpler to me and less error-prone.

[Will Harris, 2016/01/20 16:44:08]

This code allows us to enable for all plugins except one e.g. if we want to
experiment with enabled for all plugins except Flash, then we can set the value
to:

| key                           | value    |
+-------------------------------+----------+
| application/x-shockwave-flash | Disabled |
| application/futuresplash      | Disabled |
| *                             | Enabled  |

That was the reasoning why, but forshaw might have views on whether we would
ever use this or not...

[forshaw, 2016/01/20 16:57:48]

Well I could see it being useful in this scenario although probably we're not
likely to test all plugins. It would also be helpful in order to override
existing user preferences if necessary (say flash lockdown is found to be
completely broken). But I'm happy either way.

[Will Harris, 2016/01/25 19:50:28]

Acknowledged.

+        // Disabled entries take precedence over Enabled entries.
+        if (base::StartsWith(param.second, "Disabled",
+                             base::CompareCase::INSENSITIVE_ASCII)) {
+          return false;
+        }
+        if (base::StartsWith(param.second, "Enabled",
+                             base::CompareCase::INSENSITIVE_ASCII)) {
+          enabled = true;
+        }
+      }
+    }
+    if (enabled)
+      return true;

[Alexei Svitkine (slow), 2016/01/22 19:27:50]

So if something is not explicitly listed as enabled in the params (or via *),
there's a fallback to use the command-line param?

I'd add a comment about this behavior to clarify. Although, it seems strange to
support this hybrid behavior. Maybe better to always use one or the other?

Also, usually it's a good idea to make the command line override the experiment
params, rather than vice versa. But in some cases (e.g. if you suspect malware
might use the command line to mess with it), the opposite might be preferable.
I'll leave it up to you to choose which one this should be, but it should likely
have a comment about it.

[Will Harris, 2016/01/25 18:20:28]

The idea is that there is a global disable - which is on line 57. this can
disable everything via command line.

then, finch can selectively enable or disable for certain mime types or for
everything.

then, the user can selectively enable or disable via command line (also exposed
by chrome://flags).

I think this covers all the cases. Does that seem reasonable or would you prefer
a different order of precedence here?

[Alexei Svitkine (slow), 2016/01/25 18:54:51]

That's fine. In that case, I would suggest changing this line to "return
enabled;" - so that if the params are used, it doesn't fall through to also use
the kEnableWin32kLockDownMimeTypes command line. 

[Will Harris, 2016/01/25 19:08:10]

okay yes that makes good sense, will add comments too.

+  }
+
+  const base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
+
+  if (!cmd_line->HasSwitch(switches::kEnableWin32kLockDownMimeTypes))
+    return false;
+
+  std::string mime_types =
+      cmd_line->GetSwitchValueASCII(switches::kEnableWin32kLockDownMimeTypes);
+
+  // Consider the value * to enable all mime types for lockdown.
+  if (mime_types == "*")
+    return true;
+
+  base::StringTokenizer tokenizer(mime_types, ",");
+  tokenizer.set_quote_chars("\"");
+  while (tokenizer.GetNext()) {
+    if (tokenizer.token() == mime_type)
+      return true;
+  }
+
+  return false;
+}
+
+}  // namespace
+#endif  // OS_WIN
+
 // NOTE: changes to this class need to be reviewed by the security team.
 class PpapiPluginSandboxedProcessLauncherDelegate
     : public content::SandboxedProcessLauncherDelegate {
  public:
   PpapiPluginSandboxedProcessLauncherDelegate(bool is_broker,
                                               const PepperPluginInfo& info,
                                               ChildProcessHost* host)
       :
 #if defined(OS_WIN)
         info_(info),
@@ skipping 455 matching lines @@
   // sent_requests_ queue should be the one that the plugin just created.
   Client* client = sent_requests_.front();
   sent_requests_.pop();
 
   const ChildProcessData& data = process_->GetData();
   client->OnPpapiChannelOpened(channel_handle, base::GetProcId(data.handle),
                                data.id);
 }
 
 }  // namespace content