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

chrome/browser/chromeos/extensions/signin_screen_policy_provider.cc

+// 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[]{

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

nit: Please use some consistent style, either " = {" (like with the next
constant) or "{".

[Denis Kuznetsov (DE-MUC), 2016/08/29 15:14:40]

Gone.

+    "kmendfapggjehodndflmmgagdbamhnfd",  // Gnubby component extension.

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

Is this needed? The code below seems to allow the component extensions. 

[Denis Kuznetsov (DE-MUC), 2016/08/29 15:14:40]

Gone.

+    "beknehfpfkghjoafdifaflglpjkojoco",  // Gnubby app

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

Is it confirmed that this app is normally installed and running on the signin
screen?
I may be wrong, but I thought that for gnubby authentication only the component
extension is required, while the app is only for the user sessions (and provides
more functionality like UI for updating the key, etc.).

[Denis Kuznetsov (DE-MUC), 2016/08/29 15:14:40]

Gone.

+    "ljoammodoonkhnehlncldjelhidljdpi",  // Genius app (help)

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

This and the next one also seem to be the component extensions, which shouldn't
be required to listed here.

[Denis Kuznetsov (DE-MUC), 2016/08/29 15:14:40]

Gone.
But we explicitly block all component extensions on login screen, so we want to
whitelist these ones.

[emaxx, 2016/08/30 13:01:41]

Isn't it possible to allow all component extensions in
SigninScreenPolicyProvider?
Tracking the component extensions in a separate whitelist may be bad when
someone who doesn't know about these login-screen special whitelists adds a new
component extension into Chrome.

+    "mppnpdlheglhdfmldimlhpnegondlapf",  // Virtual Keyboard
+    nullptr                              // End of list
+};
+
+const char* const kSafePermissionStrings[] = {
+    "usb",

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

nit: Please sort the list lexicographically.

[Denis Kuznetsov (DE-MUC), 2016/08/29 15:14:40]

Gone.

+    "certificateProvider",
+    "storage",
+    nullptr  // End of list
+};
+
+// Some permissions take the form of a dictionary.  See |kSafePermissionStrings|
+// for permission strings (and for more documentation).

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

nit: s/ (and for more documentation)//
(there's no documentation for kSafePermissionStrings).

[Denis Kuznetsov (DE-MUC), 2016/08/29 15:14:40]

Here and below : Gone.

+const char* const kSafePermissionDicts[] = {
+    // No dictionary permissions are allowed at the moment.

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

Actually, we already need "usbDevices" dictionary permission for the Smart Card
Connector app (id khpfeaanjngmcnplbdlpegiifgpfgdco). It's closely related to the
"usb" permission: https://developer.chrome.com/apps/app_usb#manifest

[Denis Kuznetsov (DE-MUC), 2016/08/29 15:14:40]

Acknowledged.

+    nullptr  // End of list
+};
+
+// Checks if this provider applies to given extension.
+bool IsApplicableForSigninPolicy(const extensions::Extension* extension) {
+  if (extension->location() == extensions::Manifest::COMPONENT) {

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

Shouldn't EXTERNAL_COMPONENT be also specified here?
According to its documentation, it also corresponds to the component extensions
(in the sense that it also represents an internal piece of Chrome).

BTW, there's a function Manifest::IsComponentLocation.

[Denis Kuznetsov (DE-MUC), 2016/08/29 15:14:40]

Acknowledged.

+    // Do not apply to component extensions.
+    return false;
+  }
+  return true;

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

Not sure, but it probably also makes sense to always allow the case of
COMMAND_LINE. This may be useful for the development purposes.

[Denis Kuznetsov (DE-MUC), 2016/08/29 15:14:40]

Acknowledged.

+}
+
+// Checks if this provider applies to given extension.

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

nit: Please fix the comment.

[Denis Kuznetsov (DE-MUC), 2016/08/29 15:14:40]

Acknowledged.

+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()
+               << " is not an application: " << extension->GetType();
+    return true;
+  }
+  if (extension->location() == extensions::Manifest::EXTERNAL_POLICY_DOWNLOAD ||

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

BTW, there's a function Manifest::IsPolicyLocation. (Though not insisting, as
some may consider explicit enum values to be more readable.)

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

IMO, the logic would be simpler if you would negate the condition here. Because
currently it's a bit difficult to read this function and understand which cases
lead to "true" and which to "false".

+      extension->location() == extensions::Manifest::EXTERNAL_POLICY) {
+    return false;
+  }
+  LOG(ERROR) << extension->id()
+             << " location is prohibited: " << extension->location();

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

I haven't found operator<< for Manifest::Location. So if this prints its numeric
value, then it's not very good log message.

+  return true;
+}
+
+// Return true iff |entry| is contained in |char_array|.
+bool ArrayContains(const char* const char_array[], const std::string& entry) {

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

You may consider passing the array as a reference with a template length
parameter. This would allow to rid of these sentinel nullptrs, and use the STL
algorithm instead of the raw loop.

+  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,

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

nit: s/Public Sessions/the login screen/ (or signin screen).

+// which among other things requires the manifest top-level entries to be
+// contained in the |kSafeManifestEntries| whitelist and all permissions to be

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

nit: Remove references to kSafeManifestEntries.

+// 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;
+        }
+        // Accept whitelisted permissions.
+        if (ArrayContains(kSafePermissionStrings, permission_string)) {
+          continue;
+        }
+        LOG(ERROR) << extension->id()
+                   << " requested non-whitelisted permission: "
+                   << permission_string;
+        return false;

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

Shouldn't this be "safe = false", as in other similar cases in this function?

+      }
+      // "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") {

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

This check would fail for the current versions of our example login screen apps
(Smart Card Connector app khpfeaanjngmcnplbdlpegiifgpfgdco and Charismathics
Smart Card extension haeblkpifdemlfnkogkipmghfcbonief): they also contain the
"persistent" key (which is actually redundant in these cases and doesn't change
any default behavior, but this still shows that the check is too restrictive).

+          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) {
+      if (extension->manifest()->GetManifestVersion() != 2) {
+        LOG(ERROR) << extension->id()
+                   << " has non-whitelisted manifest version.";
+        safe = false;
+        continue;
+      }
+    } else {
+      // Everything else is a warning.

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

Looking at our examples of the login screen apps, looks like this would produce
too much useless warnings:
"name", "short_name", "description", "version", "minimum_chrome_version",
"default_locale", "icons", "storage", "update_url".

+      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