Malformed PortRange or ThirdPartyAuthConfig trigger OnPolicyError.

remoting/host/policy_watcher.cc

Index: remoting/host/policy_watcher.cc
diff --git a/remoting/host/policy_watcher.cc b/remoting/host/policy_watcher.cc
index 2cf7be8889425262c78ef45fb113b347959b9b5b..e53ff82a376b9981277decbe7f373910577cbe2d 100644
--- a/remoting/host/policy_watcher.cc
+++ b/remoting/host/policy_watcher.cc
@@ -21,6 +21,8 @@
 #include "components/policy/core/common/schema_registry.h"
 #include "policy/policy_constants.h"
 #include "remoting/host/dns_blackhole_checker.h"
+#include "remoting/host/third_party_auth_config.h"
+#include "remoting/protocol/port_range.h"
 
 #if !defined(NDEBUG)
 #include "base/json/json_reader.h"
@@ -44,15 +46,15 @@ namespace {
 // Copies all policy values from one dictionary to another, using values from
 // |default_values| if they are not set in |from|.
 scoped_ptr<base::DictionaryValue> CopyValuesAndAddDefaults(
-    const base::DictionaryValue* from,
-    const base::DictionaryValue* default_values) {
-  scoped_ptr<base::DictionaryValue> to(default_values->DeepCopy());
-  for (base::DictionaryValue::Iterator i(*default_values); !i.IsAtEnd();
+    const base::DictionaryValue& from,
+    const base::DictionaryValue& default_values) {
+  scoped_ptr<base::DictionaryValue> to(default_values.DeepCopy());
+  for (base::DictionaryValue::Iterator i(default_values); !i.IsAtEnd();
        i.Advance()) {
     const base::Value* value = nullptr;
 
     // If the policy isn't in |from|, use the default.
-    if (!from->Get(i.key(), &value)) {
+    if (!from.Get(i.key(), &value)) {
       continue;
     }
 
@@ -63,8 +65,8 @@ scoped_ptr<base::DictionaryValue> CopyValuesAndAddDefaults(
 #if !defined(NDEBUG)
   // Replace values with those specified in DebugOverridePolicies, if present.
   std::string policy_overrides;
-  if (from->GetString(key::kRemoteAccessHostDebugOverridePolicies,
-                      &policy_overrides)) {
+  if (from.GetString(key::kRemoteAccessHostDebugOverridePolicies,
+                     &policy_overrides)) {
     scoped_ptr<base::Value> value(base::JSONReader::Read(policy_overrides));
     const base::DictionaryValue* override_values;
     if (value && value->GetAsDictionary(&override_values)) {
@@ -92,6 +94,58 @@ scoped_ptr<policy::SchemaRegistry> CreateSchemaRegistry() {
   return schema_registry.Pass();
 }
 
+scoped_ptr<base::DictionaryValue> CopyChromotingPoliciesIntoDictionary(
+    const policy::PolicyMap& current) {
+  const char kPolicyNamePrefix[] = "RemoteAccessHost";
+  scoped_ptr<base::DictionaryValue> policy_dict(new base::DictionaryValue());
+  for (auto it = current.begin(); it != current.end(); ++it) {
+    const std::string& key = it->first;
+    const base::Value* value = it->second.value;
+
+    // Copying only Chromoting-specific policies helps avoid false alarms
+    // raised by NormalizePolicies below (such alarms shutdown the host).
+    // TODO(lukasza): Removing this somewhat brittle filtering will be possible
+    //                after having separate, Chromoting-specific schema.
+    if (key.find(kPolicyNamePrefix) != std::string::npos) {

[rmsousa, 2015/02/27 23:56:14]

doesn't prefix imply key.find must be 0?

[Łukasz Anforowicz, 2015/03/02 17:34:47]

I'll rename "prefix" to "substring".  This is really a prefix for Chromoting
policy names, but we are searching for any substring here to catch misnamed
policies that include "our" substring anywhere inside.

Right now the only side-effect of having a policy with the substring is a logged
warning message saying that this was an unrecognized policy name.  I probably
ought to have unit tests to cover this.  I'll work on that as a separate
changelist (probably need a separate class to intercept log messages via test
wrapper of SetLogMessageHandler - this is a bit complicated by the fact that
SetLogMessageHandler takes a raw function pointer [i.e. not a base::Callback and
not a function pointer + a void* context] so some state would need to be stored
in a global [or tls, but global probably better reflects the nature of
logging]).

+      policy_dict->Set(key, value->DeepCopy());
+    }
+  }
+
+  return policy_dict.Pass();
+}
+
+// Takes a dictionary containing only 1) recognized policy names and 2)
+// well-typed policy values and further verifies policy contents.
+bool VerifyWellformedness(const base::DictionaryValue& changed_policies) {
+  // Verify ThirdPartyAuthConfig policies.
+  std::string token_url;
+  std::string token_validation_url;
+  std::string token_validation_cert_issuer;
+  bool changed_entries_present = ThirdPartyAuthConfig::ExtractPolicyValues(
+      changed_policies, &token_url, &token_validation_url,
+      &token_validation_cert_issuer);
+  ThirdPartyAuthConfig third_party_auth_config;
+  if (changed_entries_present &&
+      !ThirdPartyAuthConfig::Parse(token_url, token_validation_url,
+                                   token_validation_cert_issuer,
+                                   &third_party_auth_config)) {
+    return false;
+  }
+
+  // Verify UdpPortRange policy.
+  std::string udp_port_range_string;
+  PortRange udp_port_range;
+  if (changed_policies.GetString(policy::key::kRemoteAccessHostUdpPortRange,
+                                 &udp_port_range_string)) {
+    if (!PortRange::Parse(udp_port_range_string, &udp_port_range)) {
+      return false;
+    }
+  }
+
+  // Report that all the policies were well-formed.
+  return true;
+}
+
 }  // namespace
 
 void PolicyWatcher::StartWatching(
@@ -114,36 +168,6 @@ void PolicyWatcher::StartWatching(
   }
 }
 
-void PolicyWatcher::UpdatePolicies(
-    const base::DictionaryValue* new_policies_raw) {
-  DCHECK(CalledOnValidThread());
-
-  // Use default values for any missing policies.
-  scoped_ptr<base::DictionaryValue> new_policies =
-      CopyValuesAndAddDefaults(new_policies_raw, default_values_.get());
-
-  // Find the changed policies.
-  scoped_ptr<base::DictionaryValue> changed_policies(
-      new base::DictionaryValue());
-  base::DictionaryValue::Iterator iter(*new_policies);
-  while (!iter.IsAtEnd()) {
-    base::Value* old_policy;
-    if (!(old_policies_->Get(iter.key(), &old_policy) &&
-          old_policy->Equals(&iter.value()))) {
-      changed_policies->Set(iter.key(), iter.value().DeepCopy());
-    }
-    iter.Advance();
-  }
-
-  // Save the new policies.
-  old_policies_.swap(new_policies);
-
-  // Notify our client of the changed policies.
-  if (!changed_policies->empty()) {
-    policy_updated_callback_.Run(changed_policies.Pass());
-  }
-}
-
 void PolicyWatcher::SignalPolicyError() {
   old_policies_->Clear();
   policy_error_callback_.Run();
@@ -201,24 +225,7 @@ const policy::Schema* PolicyWatcher::GetPolicySchema() const {
   return owned_schema_registry_->schema_map()->GetSchema(GetPolicyNamespace());
 }
 
-void PolicyWatcher::OnPolicyUpdated(const policy::PolicyNamespace& ns,
-                                    const policy::PolicyMap& previous,
-                                    const policy::PolicyMap& current) {
-  const char kPolicyNamePrefix[] = "RemoteAccessHost";
-  scoped_ptr<base::DictionaryValue> policy_dict(new base::DictionaryValue());
-  for (auto it = current.begin(); it != current.end(); ++it) {
-    const std::string& key = it->first;
-    const base::Value* value = it->second.value;
-
-    // Copying only Chromoting-specific policies helps avoid false alarms
-    // raised by Schema::Normalize below (such alarms shutdown the host).
-    // TODO(lukasza): Removing this somewhat brittle filtering will be possible
-    //                after having separate, Chromoting-specific schema.
-    if (key.find(kPolicyNamePrefix) != std::string::npos) {
-      policy_dict->Set(key, value->DeepCopy());
-    }
-  }
-
+bool PolicyWatcher::NormalizePolicies(base::DictionaryValue* policy_dict) {
   // Allowing unrecognized policy names allows presence of
   // 1) comments (i.e. JSON of the form: { "_comment": "blah", ... }),
   // 2) policies intended for future/newer versions of the host,
@@ -231,16 +238,95 @@ void PolicyWatcher::OnPolicyUpdated(const policy::PolicyNamespace& ns,
   std::string error;
   bool changed = false;
   const policy::Schema* schema = GetPolicySchema();
-  if (schema->Normalize(policy_dict.get(), strategy, &path, &error, &changed)) {
+  if (schema->Normalize(policy_dict, strategy, &path, &error, &changed)) {
     if (changed) {
       LOG(WARNING) << "Unknown (unrecognized or unsupported) policy: " << path
                    << ": " << error;
     }
-    UpdatePolicies(policy_dict.get());
+    return true;
   } else {
     LOG(ERROR) << "Invalid policy contents: " << path << ": " << error;
+    return false;
+  }
+}
+
+namespace {
+void CopyDictionaryValue(const base::DictionaryValue& from,
+                         base::DictionaryValue& to,
+                         std::string key) {
+  const base::Value* value;
+  if (from.Get(key, &value)) {
+    to.Set(key, value->DeepCopy());
+  }
+}
+}  // namespace
+
+scoped_ptr<base::DictionaryValue>
+PolicyWatcher::StoreNewAndReturnChangedPolicies(
+    scoped_ptr<base::DictionaryValue> new_policies) {
+  // Find the changed policies.
+  scoped_ptr<base::DictionaryValue> changed_policies(
+      new base::DictionaryValue());
+  base::DictionaryValue::Iterator iter(*new_policies);
+  while (!iter.IsAtEnd()) {
+    base::Value* old_policy;
+    if (!(old_policies_->Get(iter.key(), &old_policy) &&
+          old_policy->Equals(&iter.value()))) {
+      changed_policies->Set(iter.key(), iter.value().DeepCopy());
+    }
+    iter.Advance();
+  }
+
+  // If one of ThirdPartyAuthConfig policies changed, we need to include all.
+  if (changed_policies->HasKey(key::kRemoteAccessHostTokenUrl) ||
+      changed_policies->HasKey(key::kRemoteAccessHostTokenValidationUrl) ||
+      changed_policies->HasKey(
+          key::kRemoteAccessHostTokenValidationCertificateIssuer)) {
+    CopyDictionaryValue(*new_policies, *changed_policies,
+                        key::kRemoteAccessHostTokenUrl);
+    CopyDictionaryValue(*new_policies, *changed_policies,
+                        key::kRemoteAccessHostTokenValidationUrl);
+    CopyDictionaryValue(*new_policies, *changed_policies,
+                        key::kRemoteAccessHostTokenValidationCertificateIssuer);
+  }
+
+  // Save the new policies.
+  old_policies_.swap(new_policies);
+
+  return changed_policies.Pass();
+}
+
+void PolicyWatcher::OnPolicyUpdated(const policy::PolicyNamespace& ns,
+                                    const policy::PolicyMap& previous,
+                                    const policy::PolicyMap& current) {
+  scoped_ptr<base::DictionaryValue> new_policies =
+      CopyChromotingPoliciesIntoDictionary(current);
+
+  // Check for mistyped values and get rid of unknown policies.
+  if (!NormalizePolicies(new_policies.get())) {
     SignalPolicyError();
+    return;
   }
+
+  // Use default values for any missing policies.
+  scoped_ptr<base::DictionaryValue> filled_policies =
+      CopyValuesAndAddDefaults(*new_policies, *default_values_);
+
+  // Limit reporting to only the policies that were changed.
+  scoped_ptr<base::DictionaryValue> changed_policies =
+      StoreNewAndReturnChangedPolicies(filled_policies.Pass());
+  if (changed_policies->empty()) {
+    return;
+  }
+
+  // Verify that we are calling the callback with valid policies.
+  if (!VerifyWellformedness(*changed_policies)) {
+    SignalPolicyError();
+    return;
+  }
+
+  // Notify our client of the changed policies.
+  policy_updated_callback_.Run(changed_policies.Pass());
 }
 
 void PolicyWatcher::OnPolicyServiceInitialized(policy::PolicyDomain domain) {