Update Public Session permissions (manifest features part)

chrome/browser/chromeos/extensions/device_local_account_management_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/device_local_account_management_policy_provider.h"
 
 #include <stddef.h>
 
 #include <cstddef>
 #include <string>
@@ skipping 106 matching lines @@
     // Testing extensions:
     "ongnjlefhnoajpbodoldndkbkdgfomlp",  // Show Managed Storage
     "ilnpadgckeacioehlommkaafedibdeob",  // Enterprise DeviceAttributes
     "oflckobdemeldmjddmlbaiaookhhcngo",  // Citrix Receiver QA version
     "ljacajndfccfgnfohlgkdphmbnpkjflk",  // Chrome Remote Desktop (Dev Build)
 };
 
 // List of manifest entries from https://developer.chrome.com/apps/manifest.
 // Unsafe entries are commented out and special cases too.
 const char* const kSafeManifestEntries[] = {
-    // Special-cased in IsPlatformAppSafeForPublicSession().
+    emk::kAboutPage,
+
+    // Special-cased in IsSafeForPublicSession().
+    // Only allow hosted_app, platform_app.
     // emk::kApp,
 
-    // Special-cased in IsPlatformAppSafeForPublicSession().
-    // emk::kManifestVersion,
-
-    // Just a display string.
-    emk::kName,
-
-    // Just a display string.
-    emk::kShortName,
-
-    // Version string (for app updates).
-    emk::kVersion,
-
-    // Name of directory containg default strings.
-    emk::kDefaultLocale,
+    // Documented in https://developer.chrome.com/extensions/manifest but not
+    // implemented anywhere.  Still, a lot of apps use it.
+    "author",
+
+    // Allows inspection of page contents, not enabled on stable anyways except
+    // for whitelist.
+    // emk::kAutomation,
+
+    "background",

[Thiemo Nagel, 2016/09/26 15:10:29]

Why is there no string constant for it?  Is it even used at all?

[Ivan Šandrk, 2016/09/27 08:40:45]

Needs more investigation, may be the same case as "author".

+
+    emk::kBackgroundPageLegacy,
+
+    emk::kBackgroundPersistent,
+
+    emk::kBluetooth,
+
+    emk::kBrowserAction,
+
+    // Allows to replace the search provider which is somewhat risky - need to
+    // double check how the search provider policy behaves in PS.
+    emk::kSettingsOverride,
+
+    // Custom bookmark managers - I think this is fair game, bookmarks should be
+    // URLs only, and it's restricted to whitelist on stable.
+    emk::kUIOverride,
+
+    // Bookmark manager, history, new tab - should be safe.
+    emk::kChromeURLOverrides,
+
+    // General risk of capturing user input, but key combos must include Ctrl or
+    // Alt, so I think this is safe.
+    emk::kCommands,
+
+    // General risk of capturing user input, but key combos must include Ctrl or
+    // Alt, so I think this is safe.
+    emk::kContentCapabilities,
+
+    // Access to web content.
+    emk::kContentScripts,
+
+    emk::kContentSecurityPolicy,
+
+    // Access to web content.
+    emk::kConvertedFromUserScript,
 
     // An implementation detail (actually written by Chrome, not the app
     // author).
     emk::kCurrentLocale,
 
+    // Name of directory containg default strings.
+    emk::kDefaultLocale,
+
     // Just a display string.
     emk::kDescription,
 
+    // Access to web content.
+    emk::kDevToolsPage,
+
+    // Restricted to whitelist already.
+    emk::kDisplayInLauncher,
+
+    emk::kDisplayInNewTabPage,
+
+    // This allows to declaratively filter web requests and content, matching on
+    // content data. Doesn't allow direct access to request/content data. Can be
+    // used to brute-force e.g. cookies (reload with filter rules adjusted to
+    // match all possible cookie values) - but that's equivalent to an
+    // off-device brute-force attack.
+    // Looks safe in general with one exception: There's an action that allows
+    // to insert content scripts on matching content. We can't allow this, need
+    // to check whether there's also a host permission required for this case.
+    // emk::kEventRules,
+
+    // Shared Modules configuration: Allow other extensions to access resources.
+    emk::kExport,
+
+    emk::kExternallyConnectable,
+
+    emk::kFileBrowserHandlers,
+
+    // Extension file handlers are restricted to whitelist which only contains
+    // quickoffice.
+    emk::kFileHandlers,
+
+    emk::kFileSystemProviderCapabilities,
+
+    emk::kHomepageURL,
+
     // Just UX.
     emk::kIcons,
 
-    // Documented in https://developer.chrome.com/extensions/manifest but not
-    // implemented anywhere.  Still, a lot of apps use it.
-    "author",
-
-    // TBD
-    // emk::kBluetooth,
-
-    // TBD
-    // emk::kCommands,
-
-    // TBD, looks unsafe
-    // emk::kEventRules,
-
-    // TBD
-    // emk::kExternallyConnectable,
-
-    // TBD
-    // emk::kFileHandlers,
-
-    // TBD
-    // emk::kFileSystemProviderCapabilities,
-
     // Shared Modules configuration: Import resources from another extension.
     emk::kImport,
 
-    // Shared Modules configuration: Allow other extensions to access resources.
-    emk::kExport,
+    emk::kIncognito,
+
+    // Keylogging.
+    emk::kInputComponents,
 
     // Shared Modules configuration: Specify extension id for development.
     emk::kKey,
 
-    // Descriptive statement about the app.
+    emk::kKiosk,
+
     emk::kKioskEnabled,
 
-    // Contradicts the purpose of running inside a Public Session.
-    // emk::kKioskOnly,
-
-    // Descriptive statement about the app.
+    // Not useful since it will prevent app from running, but we don't care.
+    emk::kKioskOnly,
+
+    emk::kKioskRequiredPlatformVersion,
+
+    // Not useful since it will prevent app from running, but we don't care.
+    emk::kKioskSecondaryApps,
+
+    // Whitelisted to only allow Google Now.
+    emk::kLauncherPage,
+
+    // Special-cased in IsSafeForPublicSession() (value == 2).
+    // emk::kManifestVersion,
+
+    emk::kMIMETypes,
+
+    // Whitelisted to only allow browser tests and PDF viewer.
+    emk::kMimeTypesHandler,
+
     emk::kMinimumChromeVersion,
 
     // NaCl modules are bound to app permissions just like the rest of the app
     // and thus should not pose a risk.
     emk::kNaClModules,
 
-    // TBD, doc missing
-    // emk::kOAuth2,
-
-    // Descriptive statement about the app.
+    // Just a display string.
+    emk::kName,
+
+    // Used in conjunction with the identity API - not really used when there's
+    // no GAIA user signed in.
+    emk::kOAuth2,
+
+    // Generally safe (i.e. only whitelist apps), except for the policy to
+    // whitelist apps for auto-approved token minting (we should just ignore
+    // this in public sessions). Risk is that admin mints OAuth tokens to access
+    // services on behalf of the user silently.
+    // emk::kOAuth2AutoApprove,
+
     emk::kOfflineEnabled,
 
-    // Special-cased in IsPlatformAppSafeForPublicSession().
+    // A bit risky as the extensions sees all keystrokes entered into the
+    // omnibox after the search key matches, but generally we deem URLs fair
+    // game.
+    // emk::kOmnibox,
+
+    // Special-cased in IsSafeForPublicSession(). Subject to permission
+    // restrictions.
     // emk::kOptionalPermissions,
 
-    // Special-cased in IsPlatformAppSafeForPublicSession().
+    emk::kOptionsPage,
+
+    emk::kOptionsUI,
+
+    emk::kPageAction,
+
+    // Special-cased in IsSafeForPublicSession(). Subject to permission
+    // restrictions.
     // emk::kPermissions,
 
-    // No constant in manifest_constants.cc.
+    // No constant in manifest_constants.cc. Declared as a feature, but unused.
     // "platforms",
 
-    // Descriptive statement about the app.
+    // N/A on Chrome OS, so we don't care.
+    emk::kPlugins,
+
+    // Stated 3D/WebGL/plugin requirements of an app.
     emk::kRequirements,
 
     // Execute some pages in a separate sandbox.  (Note: Using string literal
     // since extensions::manifest_keys only has constants for sub-keys.)
     "sandbox",
 
-    // TBD, doc missing
+    // Just a display string.
+    emk::kShortName,
+
+    // Doc missing. Declared as a feature, but unused.
     // emk::kSignature,
 
     // Network access.
     emk::kSockets,
 
-    // TBD.  (Note: Using string literal since extensions::manifest_keys only
-    // has constants for sub-keys.)
-    // "storage",
-
-    // TBD, doc missing
-    // emk::kSystemIndicator,
+    // Just provides dictionaries, no access to content.
+    emk::kSpellcheck,
+
+    // Update comment.  (Note: Using string literal since
+    // extensions::manifest_keys only has constants for sub-keys.)
+    "storage",
+
+    // Only hangouts is whitelisted.

[Thiemo Nagel, 2016/09/26 15:10:29]

Nit: Hangouts is spelled with a capital H.

[Ivan Šandrk, 2016/09/27 08:40:45]

Done.

+    emk::kSystemIndicator,
+
+    emk::kTheme,
+
+    // Might need this for accessibilty, but has content access. Manual
+    // whitelisting might be reasonable here?
+    // emk::kTtsEngine,
 
     // TODO(tnagel): Ensure that extension updates query UserMayLoad().
     // https://crbug.com/549720
     emk::kUpdateURL,
 
     // Apps may intercept navigations to URL patterns for domains for which the
     // app author has proven ownership of to the Web Store.  (Chrome starts the
     // app instead of fulfilling the navigation.)  This is only safe for apps
     // that have been loaded from the Web Store and thus is special-cased in
-    // IsPlatformAppSafeForPublicSession().
+    // IsSafeForPublicSession().
     // emk::kUrlHandlers,
 
-    // TBD
-    // emk::kUsbPrinters,
+    emk::kUsbPrinters,
+
+    // Version string (for app updates).
+    emk::kVersion,
 
     // Just a display string.
     emk::kVersionName,
 
+    emk::kWebAccessibleResources,
+
     // Webview has no special privileges or capabilities.
     emk::kWebview,
 };
 
 // List of permission strings based on [1] and [2].  See |kSafePermissionDicts|
 // for permission dicts.  Since Public Session users may be fully unaware of any
 // apps being installed, their consent to access any kind of sensitive
 // information cannot be assumed.  Therefore only APIs are whitelisted which
 // should not leak sensitive data to the caller.  Since the privacy boundary is
 // drawn at the API level, no safeguards are required to prevent exfiltration
@@ skipping 137 matching lines @@
 // Some permissions take the form of a dictionary.  See |kSafePermissionStrings|
 // for permission strings (and for more documentation).
 const char* const kSafePermissionDicts[] = {
     // TBD
     // "fileSystem",
 
     // Just another type of connectivity.
     "socket",
 };
 
+// List of allowed app strings.
+const char* const kSafeAppStrings[] = {
+    "background",
+    "content_security_policy",
+    "icon_color",
+    "isolation",
+    "launch",
+    "linked_icons",
+};
+
 // Return true iff |entry| is contained in |char_array|.
 bool ArrayContainsImpl(const char* const char_array[],
                        size_t entry_count,
                        const std::string& entry) {
   for (size_t i = 0; i < entry_count; ++i) {
     if (entry == char_array[i]) {
       return true;
     }
   }
   return false;
 }
 
 // See http://blogs.msdn.com/b/the1/archive/2004/05/07/128242.aspx for an
 // explanation of array size determination.
 template <size_t N>
 bool ArrayContains(const char* const (&char_array)[N],
                    const std::string& entry) {
   return ArrayContainsImpl(char_array, N, entry);
 }
 
-// Returns true for platform apps that are considered safe for Public Sessions,
+// Returns true for extensions 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 IsPlatformAppSafeForPublicSession(const extensions::Extension* extension) {
+bool IsSafeForPublicSession(const extensions::Extension* extension) {
   bool safe = true;
-  if (extension->GetType() != extensions::Manifest::TYPE_PLATFORM_APP) {
-    LOG(ERROR) << extension->id() << " is not a platform app.";
+  if (!extension->is_extension() &&
+      !extension->is_hosted_app() &&
+      !extension->is_platform_app() &&
+      !extension->is_shared_module() &&
+      !extension->is_theme()) {
+    LOG(ERROR) << extension->id() << " is not of a supported type. "
+               << "Extension type: " << extension->GetType();
     safe = false;
   }
 
   for (base::DictionaryValue::Iterator it(*extension->manifest()->value());
        !it.IsAtEnd(); it.Advance()) {
     if (ArrayContains(kSafeManifestEntries, it.key())) {
       continue;
     }
 
     // Permissions must be whitelisted in |kSafePermissionStrings| or
@@ skipping 49 matching lines @@
                              base::CompareCase::SENSITIVE) ||
             base::StartsWith(permission_string, "ftp://",
                              base::CompareCase::SENSITIVE)) {
           continue;
         }
         LOG(ERROR) << extension->id()
                    << " requested non-whitelisted permission: "
                    << permission_string;
         safe = false;
       }
-    // "app" may only contain "background".
+    // "app" may contain one of: background, content_security_policy,
+    // icon_color, isolation, launch, linked_icons.
+    // "app" is only allowed for hosted_app or platform_app.
     } else if (it.key() == emk::kApp) {
+      if (!extension->is_hosted_app() &&
+          !extension->is_platform_app()) {
+        LOG(ERROR) << extension->id()
+                   << ": app manifest entry is allowed only for"
+                   << "hosted_app or platform_app extension type. "
+                   << "Current extension type: " << extension->GetType();
+        safe = false;
+      }
       const base::DictionaryValue *dict_value;
       if (!it.value().GetAsDictionary(&dict_value)) {
         LOG(ERROR) << extension->id() << ": app is not a dictionary.";
         safe = false;
         continue;
       }
       for (base::DictionaryValue::Iterator it2(*dict_value);
            !it2.IsAtEnd(); it2.Advance()) {
-        if (it2.key() != "background") {
+        if (!ArrayContains(kSafeAppStrings, it2.key())) {
           LOG(ERROR) << extension->id()
                      << " has non-whitelisted manifest entry: "
                      << it.key() << "." << it2.key();
           safe = false;
           continue;
         }
       }
     // Require v2 because that's the only version we understand.
     } else if (it.key() == emk::kManifestVersion) {
       int version;
@@ skipping 67 matching lines @@
     // Allow extension if its specific ID is whitelisted for use in public
     // sessions.
     if (ArrayContains(kPublicSessionWhitelist, extension->id())) {
       return true;
     }
 
     // Allow force-installed platform app if all manifest contents are
     // whitelisted.
     if ((extension->location() == extensions::Manifest::EXTERNAL_POLICY_DOWNLOAD
          || extension->location() == extensions::Manifest::EXTERNAL_POLICY)
-        && IsPlatformAppSafeForPublicSession(extension)) {
+        && IsSafeForPublicSession(extension)) {
       return true;
     }
   } else if (account_type_ == policy::DeviceLocalAccount::TYPE_KIOSK_APP) {
     // For single-app kiosk sessions, allow platform apps, extesions and shared
     // modules.
     if (extension->GetType() == extensions::Manifest::TYPE_PLATFORM_APP ||
         extension->GetType() == extensions::Manifest::TYPE_SHARED_MODULE ||
         extension->GetType() == extensions::Manifest::TYPE_EXTENSION) {
       return true;
     }
   }
 
   // Disallow all other extensions.
   if (error) {
     *error = l10n_util::GetStringFUTF16(
           IDS_EXTENSION_CANT_INSTALL_IN_DEVICE_LOCAL_ACCOUNT,
           base::UTF8ToUTF16(extension->name()),
           base::UTF8ToUTF16(extension->id()));
   }
   return false;
 }
 
 }  // namespace chromeos