Add types.private.ChromeDirectSetting and Connect it to preferencesPrivate.googleGeolocationAccessE…

chrome/browser/extensions/api/preference/chrome_direct_setting.cc

Index: chrome/browser/extensions/api/preference/chrome_direct_setting.cc
diff --git a/chrome/browser/extensions/api/preference/chrome_direct_setting.cc b/chrome/browser/extensions/api/preference/chrome_direct_setting.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2444dcf84b60de49b5a2efc27ce2647133460cba
--- /dev/null
+++ b/chrome/browser/extensions/api/preference/chrome_direct_setting.cc
@@ -0,0 +1,82 @@
+// Copyright (c) 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/extensions/api/preference/chrome_direct_setting.h"
+
+#include "base/prefs/pref_service.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/api/preference/preference_api_constants.h"
+#include "chrome/browser/profiles/profile.h"
+
+namespace extensions {
+namespace chromedirectsetting {
+
+void DirectSettingFunctionBase::CheckCalledFromComponentExtension() {
+  bool isComponentExtension =
+      (GetExtension()->location() == Manifest::COMPONENT);
+
+  // If we've made it this far and the caller isn't a component extension,
+  // this means the API permissions system has failed.

[Bernhard Bauer, 2013/07/09 07:34:32]

Is the API permissions system enforced on the browser side? That is, can this
*only* happen due to a bug in Chrome, or also for example if a renderer is
compromised? If the latter, I don't think crashing is right, as that would
enable a denial-of-service attack. Seeing as we already have
EXTENSION_FUNCTION_VALIDATE below, could you make this return a bool and wrap it
in EXTENSION_FUNCTION_VALIDATE?

[robliao, 2013/07/09 17:27:45]

The renderer performs the API check, so compromised renderers would send the
function call to the frame process. I've relaxed this check.
On 2013/07/09 07:34:32, Bernhard Bauer wrote:

+  // Fatal check to prevent any damage from being done.
+  CHECK(isComponentExtension);
+}
+
+PrefService* DirectSettingFunctionBase::GetPrefService() {
+  return profile()->GetPrefs();
+}
+
+bool GetDirectSettingFunction::RunImpl() {
+  CheckCalledFromComponentExtension();
+
+  std::string pref_key;
+  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &pref_key));
+
+  const PrefService::Preference* preference =
+      GetPrefService()->FindPreference(pref_key.c_str());

[Bernhard Bauer, 2013/07/09 17:58:27]

I'm also worried about directly using the passed in pref key instead of going
through the mapping like for the regular preferences API. That means that the
component extension could access any other (registered) preference. In addition,
the API is available to any component extension, so *any* compromised component
extension could change *any* preference. I would feel much more at ease if we
had a mapping or a whitelist of accessible preferences here.

[robliao, 2013/07/09 18:15:53]

Isn't the JSON API declaration sufficient for limiting prefs that can be
accessed?

What I wanted to avoid was a declaration in the JSON API and a redundant list
that listed the allowed prefs.
On 2013/07/09 17:58:27, Bernhard Bauer wrote:

[Bernhard Bauer, 2013/07/09 18:45:41]

Sorry, no :-(

The JSON API here is really just used to construct a convenience object that
will call the right API method with the right parameter. All of this happens on
the renderer side though, so the browser can't trust it. (The browser does check
that the extension is allowed to call the API itself, but that mechanism doesn't
know about the pref key parameter).
We could parse the JSON API in the browser too to extract the preference
whitelist, but that would be additional work. If you're willing to implement
that, I'm happy to review it :)

[robliao, 2013/07/09 18:53:38]

I'm going to think about the best way for getting this through.
On 2013/07/09 18:45:41, Bernhard Bauer wrote:

+  const base::Value* value = preference->GetValue();

[Bernhard Bauer, 2013/07/09 07:34:32]

This will crash if the pref_key isn't found. Can you wrap it in
EXTENSION_FUNCTION_VALIDATE?

[robliao, 2013/07/09 17:27:45]

This was a deliberate decision for the following reasons
* Pref Keys are specified by Chrome Developers the API JSON definition file.
* The Extension Prefs similarly fail fast (crash) in missing extension pref
situations.
* Pref Keys tend not to be removed once they are checked in.

Crashing fast here allows Chrome Developers to easily see their error and fix
it.

On 2013/07/09 07:34:32, Bernhard Bauer wrote:

[Bernhard Bauer, 2013/07/09 17:58:27]

The preference API checks with a EXTENSION_FUNCTION_VALIDATE whether the
extension pref exists
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...).
EXTENSION_FUNCTION_VALIDATE will still crash in a Debug build, but in a Release
build it will just kill the renderer process, not the browser. Both of these
should be visible to Chrome developers.

[robliao, 2013/07/09 18:15:53]

Take a look at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

The set pref path will make it here and crash for a pref that's not in the
extension pref value map (in our case, the google geolocation preference)

I'll go ahead and make the check here.

On 2013/07/09 17:58:27, Bernhard Bauer wrote:

[Bernhard Bauer, 2013/07/09 18:45:41]

Thanks!

This API function won't touch ExtensionPrefValueMap though, because we treat it
as a user preference.

[robliao, 2013/07/09 18:53:38]

That's correct. Part of the impetus of this change is due to the issues we
encountered previously with ExtensionPrefValueMap (which incidentally, did cause
crashes)
On 2013/07/09 18:45:41, Bernhard Bauer wrote:

+
+  scoped_ptr<DictionaryValue> result(new DictionaryValue);
+  result->Set(preference_api_constants::kValue, value->DeepCopy());
+  SetResult(result.release());
+
+  return true;
+}
+
+bool SetDirectSettingFunction::RunImpl() {
+  CheckCalledFromComponentExtension();
+
+  std::string pref_key;
+  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &pref_key));
+
+  DictionaryValue* details = NULL;
+  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(1, &details));
+
+  Value* value = NULL;
+  EXTENSION_FUNCTION_VALIDATE(
+      details->Get(preference_api_constants::kValue, &value));
+
+  PrefService* prefService = GetPrefService();

[Bernhard Bauer, 2013/07/09 07:34:32]

Local variables are named unix_hacker_style.

[robliao, 2013/07/09 17:27:45]

Habits from a previous life. Fixed.
On 2013/07/09 07:34:32, Bernhard Bauer wrote:

+  const PrefService::Preference* preference =
+      prefService->FindPreference(pref_key.c_str());
+
+  EXTENSION_FUNCTION_VALIDATE(value->GetType() == preference->GetType());
+
+  prefService->Set(pref_key.c_str(), *value);
+
+  return true;
+}
+
+bool ClearDirectSettingFunction::RunImpl() {
+  CheckCalledFromComponentExtension();
+
+  std::string pref_key;
+  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &pref_key));
+  GetPrefService()->ClearPref(pref_key.c_str());
+
+  return true;
+}
+
+}  // namespace chromedirectsetting
+}  // namespace extensions
+