Add policy provider that would filter extensions/apps allowed on the

chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc

Index: chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc
diff --git a/chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc b/chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c3a60ab5c777ac155d7cb8947ec77e52342a1cf9
--- /dev/null
+++ b/chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc
@@ -0,0 +1,227 @@
+// Copyright 2013 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 "chrome/browser/chromeos/extensions/signin_screen_policy_provider.h"
+
+#include <stddef.h>
+
+#include <cstddef>
+#include <string>
+
+#include "base/logging.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "chrome/grit/generated_resources.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/manifest.h"
+#include "extensions/common/manifest_constants.h"
+#include "ui/base/l10n/l10n_util.h"
+
+namespace chromeos {
+
+namespace {
+
+namespace emk = extensions::manifest_keys;
+
+// Apps/extensions explicitly whitelisted for use on signin screen.
+const char* const kWhitelistedApps[]{
+    "kmendfapggjehodndflmmgagdbamhnfd",  // Gnubby component extension.
+    "beknehfpfkghjoafdifaflglpjkojoco",  // Gnubby app
+    "ljoammodoonkhnehlncldjelhidljdpi",  // Genius app (help)
+    "mppnpdlheglhdfmldimlhpnegondlapf",  // Virtual Keyboard
+    nullptr                              // End of list
+};
+
+const char* const kSafePermissionStrings[] = {
+    "usb",
+    "certificateProvider",
+    "storage",

[asargent_no_longer_on_chrome, 2016/07/22 00:13:29]

You should use APIPermission::ID here instead of strings, and rename to
something like kSafePermissions. 

[Denis Kuznetsov (DE-MUC), 2016/07/22 14:37:14]

See reasoning below.
I agree that we'd better to have constants (for static typing), but direct
enum-based approach will not work (see below), and there is no easy way to get
string based on enum.

+    nullptr  // End of list
+};
+
+// Some permissions take the form of a dictionary.  See |kSafePermissionStrings|
+// for permission strings (and for more documentation).
+const char* const kSafePermissionDicts[] = {

[asargent_no_longer_on_chrome, 2016/07/22 00:13:29]

Same thing here, although I wonder if you should even leave this in if we aren't
using it yet? 

+    // No dictionary permissions are allowed at the moment.
+    nullptr  // End of list
+};
+
+// Checks if given this provider applies to given extension.

[asargent_no_longer_on_chrome, 2016/07/22 00:13:28]

grammar nit: "if given this provider applies to given"

maybe you meant "Checks if this provider applies to given extension." ?

[Denis Kuznetsov (DE-MUC), 2016/07/22 14:37:14]

Done.

+bool IsApplicableForSigninPolicy(const extensions::Extension* extension) {
+  if (extension->location() == extensions::Manifest::COMPONENT) {
+    // Do not apply to component extensions.
+    return false;
+  }
+  return true;
+}
+
+// Checks if given this provider applies to given extension.

[asargent_no_longer_on_chrome, 2016/07/22 00:13:29]

same grammar nit here

[Denis Kuznetsov (DE-MUC), 2016/07/22 14:37:14]

Done.

+bool IsProhibitedForSignin(const extensions::Extension* extension) {
+  if (extension->GetType() != extensions::Manifest::TYPE_HOSTED_APP &&
+      extension->GetType() != extensions::Manifest::TYPE_PLATFORM_APP &&
+      extension->GetType() != extensions::Manifest::TYPE_LEGACY_PACKAGED_APP) {
+    LOG(ERROR) << extension->id()

[asargent_no_longer_on_chrome, 2016/07/22 00:13:28]

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

"Remove most logging calls before checking in. Unless you're adding temporary
logging to track down a specific bug, and you have a plan for how to collect the
logged data from user machines, you should generally not add logging statements.

For the rare case when logging needs to stay in the codebase for a while, prefer
DVLOG(1) to other logging methods. This avoids bloating the release executable
and in debug can be selectively enabled at runtime by command-line arguments
"

Do you really need this logging? (here and elsewhere)

[Denis Kuznetsov (DE-MUC), 2016/07/22 14:37:14]

Yes. Because with ChromeOS login screen the extension developer will not have
much debugging capabilities, logs are one of the very few. So we want to make it
clear for developer what could block the extension - that's the reason for
extensive logging, and using ERROR level.

[asargent_no_longer_on_chrome, 2016/07/25 21:27:01]

Ok, that makes sense. 

+               << " is not an application: " << extension->GetType();
+    return true;
+  }
+  if (extension->location() == extensions::Manifest::EXTERNAL_POLICY_DOWNLOAD ||
+      extension->location() == extensions::Manifest::EXTERNAL_POLICY) {
+    return false;
+  }
+  LOG(ERROR) << extension->id()
+             << " location is prohibited: " << extension->location();
+  return true;
+}
+
+// Return true iff |entry| is contained in |char_array|.
+bool ArrayContains(const char* const char_array[], const std::string& entry) {
+  for (size_t i = 0; char_array[i] != nullptr; ++i) {
+    if (entry == char_array[i]) {
+      return true;
+    }
+  }
+  return false;
+}
+
+// Returns true for platform apps that are considered safe for Public Sessions,
+// which among other things requires the manifest top-level entries to be
+// contained in the |kSafeManifestEntries| whitelist and all permissions to be
+// contained in |kSafePermissionStrings| or |kSafePermissionDicts|.  Otherwise
+// returns false and logs all reasons for failure.
+bool IsSafeForSignin(const extensions::Extension* extension) {
+  bool safe = true;
+  for (base::DictionaryValue::Iterator entry(*extension->manifest()->value());
+       !entry.IsAtEnd(); entry.Advance()) {
+    // Permissions must be whitelisted in |kSafePermissionStrings|.
+    if (entry.key() == emk::kPermissions ||
+        entry.key() == emk::kOptionalPermissions) {
+      const base::ListValue* list_value;
+      if (!entry.value().GetAsList(&list_value)) {
+        LOG(ERROR) << extension->id() << ": " << entry.key()
+                   << " is not a list.";
+        safe = false;
+        continue;
+      }
+      for (auto it = list_value->begin(); it != list_value->end(); ++it) {
+        // Try to read as dictionary.
+        const base::DictionaryValue* dict_value;
+        if ((*it)->GetAsDictionary(&dict_value)) {
+          if (dict_value->size() != 1) {
+            LOG(ERROR) << extension->id()
+                       << " has dict in permission list with size "
+                       << dict_value->size() << ".";
+            safe = false;
+            continue;
+          }
+          for (base::DictionaryValue::Iterator it2(*dict_value); !it2.IsAtEnd();
+               it2.Advance()) {
+            if (!ArrayContains(kSafePermissionDicts, it2.key())) {
+              LOG(ERROR) << extension->id()
+                         << " has non-whitelisted dict in permission list: "
+                         << it2.key();
+              safe = false;
+              continue;
+            }
+          }
+          continue;
+        }
+        // Try to read as string.
+        std::string permission_string;
+        if (!(*it)->GetAsString(&permission_string)) {
+          LOG(ERROR) << extension->id() << ": " << entry.key()
+                     << " contains a token that's neither a string nor a dict.";
+          safe = false;
+          continue;
+        }

[asargent_no_longer_on_chrome, 2016/07/22 00:13:28]

The code in the above couple of blocks seems a little too tightly coupled to the
implementation details of how Extension objects get their permissions from a
manifest. What I think would be better is if you instead used the
permissions_data() accessor on the Extension and then called
PermissionsData::HasAPIPermission with the APIPermission::ID from your list of
allowed permissions from above in the file. 

[Denis Kuznetsov (DE-MUC), 2016/07/22 14:37:14]

We do not want to "allow app if it has permission". We want to "block all apps
that have any permissions other than above".
And PermissionsData does not allow iterating over all permissions - only checks
if it has one.

[asargent_no_longer_on_chrome, 2016/07/25 21:27:01]

Do you just care about API permissions or other types of permissions like host
access or content scripts?

If you just care about API permissions, you can get the PermissionsData for the
extension, take the union of the active and withheld API PermissionSet's, then
take the difference of that against a manually constructed set of
PermissionSet's that you want to allow and see if the result is empty. 

If that's still not enough, we can add additional structured access via APIs, as
I really don't like the idea of having code outside the extensions system using
direct string-based access against the raw manifest DictionaryValue. 

[emaxx, 2016/07/26 02:55:30]

I would agree that the low-level parsing should ideally be kept separate from
this logic.

On the other side, the resulting solution should leave no gray areas (about
whose security it's difficult to reason) and, ideally, sustain against adding
new permissions and new manifest keys without risks of opening the security
gaps.

Looking through the app manifest documentation, the following keys look
dangerous and probably have to be prohibited in our case (note that this list is
the very rough estimation): "bluetooth" (this one is actually under
discussions), "commands", "event_rules", "file_handlers", "nacl_modules",
"url_handlers", "usb_printers", "usb_printers".

This diverse (and yet incomplete) list explains why the way of explicit
whitelisting looked more robust to us.

Or, Antony, do you maybe see an option to make such kind of whitelisting through
some structured API?

[asargent_no_longer_on_chrome, 2016/07/27 21:00:14]

Yes, one of the reasons I'm arguing for structured API access instead of
unstructured manifest DictionaryValue manual parsing is precisely that we
sometimes have introduced new "features" via top-level manifest keys instead of
permissions entries, such as the examples you cite. On the other hand, sometimes
we introduce new manifest keys that would have no security implications, but
could become required always or for certain kinds of extensions/apps (eg
"manifest_version", or "requirements"). I'd much rather that we have an API
somewhere in the extensions system itself that lets you assert "the app is only
allowed to do x,y,z" that is close to where we'd be making these changes, so
that it can evolve without the code in this file having to know about manifest
keys. 

If it's not just API permissions but more generally "extensions system features"
that you're concerned with, we already have a pretty flexible declarative system
for describing these and limiting access to them in various ways. See the
_*features.json files in chrome/common/extensions/api/ and
extensions/common/api/, which let us do things like restrict access based on
extension type, javascript context type, extension id (whitelist), etc. It could
make a lot of sense to add a new type of restriction, something like
"allowed_on_signin_screen": true, and probably we'd want to implement your
whitelist that way too.

+        // Accept whitelisted permissions.
+        if (ArrayContains(kSafePermissionStrings, permission_string)) {
+          continue;
+        }
+        LOG(ERROR) << extension->id()
+                   << " requested non-whitelisted permission: "
+                   << permission_string;
+        return false;
+      }
+      // "app" may only contain "background".
+    } else if (entry.key() == emk::kApp) {
+      const base::DictionaryValue* dict_value;
+      if (!entry.value().GetAsDictionary(&dict_value)) {
+        LOG(ERROR) << extension->id() << ": app is not a dictionary.";
+        safe = false;
+        continue;
+      }
+      for (base::DictionaryValue::Iterator it(*dict_value); !it.IsAtEnd();
+           it.Advance()) {
+        if (it.key() != "background") {

[asargent_no_longer_on_chrome, 2016/07/22 00:13:28]

I have a similar concern here about this being too tightly coupled to the
implementation details of a manifest. What is it you are trying to actually
prevent? I.e. what is it in the "app" section of an app's manifest leads to
giving it an ability that you're trying to prevent here?

If we don't already have a way for you to get that information in a structured
way (without having to try and re-parse the manifest) from an Extension object
or its permissions_data(), perhaps we can add a way. 

[Denis Kuznetsov (DE-MUC), 2016/07/22 14:37:14]

We want to allow 3rd-party apps on chromeos sign-in screen to enable hardware
token-based flow in SAML signin process (e.g. login by smartcards).
But we want to limit these app as hard as possible so that such apps will not
spoil sign-in UI nor will lower the sign-in flow security.

+          LOG(ERROR) << extension->id()
+                     << " has non-whitelisted manifest entry: " << entry.key()
+                     << "." << it.key();
+          safe = false;
+          continue;
+        }
+      }
+      // Require v2 because that's the only version we understand.
+    } else if (entry.key() == emk::kManifestVersion) {
+      int version;

[asargent_no_longer_on_chrome, 2016/07/22 00:13:29]

Instead of examining the dictionary value from the manifest to determine this,
you should instead just call:

extension->manifest()->GetManifestVersion()

[Denis Kuznetsov (DE-MUC), 2016/07/22 14:37:14]

Done.

+      if (!entry.value().GetAsInteger(&version)) {
+        LOG(ERROR) << extension->id() << ": " << emk::kManifestVersion
+                   << " is not an integer.";
+        safe = false;
+        continue;
+      }
+      if (version != 2) {
+        LOG(ERROR) << extension->id()
+                   << " has non-whitelisted manifest version.";
+        safe = false;
+        continue;
+      }
+    } else {
+      // Everything else is a warning.
+      LOG(WARNING) << extension->id()
+                   << " has non-whitelisted manifest entry: " << entry.key();
+    }
+  }
+  return safe;
+}
+
+}  // namespace
+
+SigninScreenPolicyProvider::SigninScreenPolicyProvider() {}
+
+SigninScreenPolicyProvider::~SigninScreenPolicyProvider() {}
+
+std::string SigninScreenPolicyProvider::GetDebugPolicyProviderName() const {
+#if defined(NDEBUG)
+  NOTREACHED();
+  return std::string();
+#else
+  return "permissions for sign-in screen";
+#endif
+}
+
+bool SigninScreenPolicyProvider::UserMayLoad(
+    const extensions::Extension* extension,
+    base::string16* error) const {
+  if (!IsApplicableForSigninPolicy(extension)) {
+    return true;
+  }
+  if (ArrayContains(kWhitelistedApps, extension->id())) {
+    return true;
+  }
+  // Allow force-installed platform app if all manifest contents are
+  // whitelisted.
+  if (!IsProhibitedForSignin(extension) && IsSafeForSignin(extension)) {
+    return true;
+  }
+  // Disallow all other extensions.
+  if (error) {
+    *error =
+        l10n_util::GetStringFUTF16(IDS_EXTENSION_CANT_INSTALL_ON_LOGIN_SCREEN,
+                                   base::UTF8ToUTF16(extension->name()),
+                                   base::UTF8ToUTF16(extension->id()));
+  }
+  return false;
+}
+
+}  // namespace chromeos