Allow extension policy to specify sensitive values

Allow extension policy schemas to specify "sensitiveValue": true, which
hides the policy value from the chrome://policy UI, from policy exports
and from being sent as part of enterprise reporting.
The "sensitiveValue": true annotation can be placed on any level in the
schema declaration. An example extension storage schema may be:
{
  "type": "object",
  "properties": {
    "VisibleStringPolicy": {
      "type": "string"
    },
    "SensitiveStringPolicy": {
      "type": "string",
      "sensitiveValue": true
    },
    "VisibleDictPolicy": {
      "type": "object",
      "properties": {
        "some_bool": { "type": "boolean" },
        "some_string": { "type": "boolean" }
      }
    }
    "SensitiveDictPolicy": {
      "type": "object",
      "properties": {
        "some_bool": { "type": "boolean" },
        "some_string": { "type": "boolean" }
      },
      "sensitiveValue": true
    }
  }
}
In this case, the values of VisibleStringPolicy and VisibleDictPolicy
will be displayed on chrome://policy. The values of
SensitiveStringPolicy and SensitiveDictPolicy will be replaced with the
masking string "********"".

      browser_tests --gtest_filter=*PolicyUITest*

Test: components_unittests --gtest_filter=*Schema* && \
Bug: 849657
Change-Id: I07fc5f30945dccd05f5ccb8f8d071e25b4105c79
Reviewed-on: https://chromium-review.googlesource.com/1140301Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578367}

chrome/browser/policy/policy_conversions.cc

@@ -4,9 +4,9 @@
 
 #include "chrome/browser/policy/policy_conversions.h"
 
-#include <unordered_set>
-
+#include "base/containers/flat_map.h"
 #include "base/json/json_writer.h"
+#include "base/optional.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
@@ -64,6 +64,11 @@ const PolicyStringMap kPolicySources[policy::POLICY_SOURCE_COUNT] = {
     {"sourcePlatform", IDS_POLICY_SOURCE_PLATFORM},
 };
 
+// Maps known policy names to their schema. If a policy is not present, it is
+// not known (either through policy_templates.json or through an extenion's
+// managed storage schema).
+using PolicyToSchemaMap = base::flat_map<std::string, policy::Schema>;
+
 // Utility function that returns a JSON serialization of the given |dict|.
 std::string DictionaryToJSONString(const Value& dict) {
   std::string json_string;
@@ -75,19 +80,23 @@ std::string DictionaryToJSONString(const Value& dict) {
 // Returns a copy of |value|. If necessary (which is specified by
 // |convert_values|), converts some values to a representation that
 // i18n_template.js will display.
-Value CopyAndMaybeConvert(const Value& value, bool convert_values) {
+Value CopyAndMaybeConvert(const Value& value,
+                          bool convert_values,
+                          const base::Optional<policy::Schema>& schema) {
+  Value value_copy = value.Clone();
+  if (schema.has_value())
+    schema->MaskSensitiveValues(&value_copy);
   if (!convert_values)
-    return value.Clone();
+    return value_copy;
   if (value.is_dict())
-    return Value(DictionaryToJSONString(value));
+    return Value(DictionaryToJSONString(value_copy));
 
   if (!value.is_list()) {
-    return value.Clone();
+    return value_copy;
   }
 
   Value result(Value::Type::LIST);
-  for (size_t i = 0; i < value.GetList().size(); ++i) {
-    const auto& element = value.GetList()[i];
+  for (const auto& element : value_copy.GetList()) {
     if (element.is_dict()) {
       result.GetList().emplace_back(Value(DictionaryToJSONString(element)));
     } else {
@@ -102,17 +111,31 @@ PolicyService* GetPolicyService(content::BrowserContext* context) {
       ->policy_service();
 }
 
-// Inserts a description of each policy in |policy_map| into |values|, using
-// the optional errors in |errors| to determine the status of each policy. If
+// Returns the Schema for |policy_name| if that policy is known. If the policy
+// is unknown, returns |base::nullopt|.
+base::Optional<policy::Schema> GetKnownPolicySchema(
+    const base::Optional<PolicyToSchemaMap>& known_policy_schemas,
+    const std::string& policy_name) {
+  if (!known_policy_schemas.has_value())
+    return base::nullopt;
+  auto known_policy_iterator = known_policy_schemas->find(policy_name);
+  if (known_policy_iterator == known_policy_schemas->end())
+    return base::nullopt;
+  return known_policy_iterator->second;
+}
+
+// Inserts a description of each policy in |map| into |values|, using the
+// optional errors in |errors| to determine the status of each policy. If
 // |convert_values| is true, converts the values to show them in javascript.
-// |policy_names| contains all valid policies in the same policy namespace of
-// |policy_map|. A policy in |map| but not|policy_names| is an unknown policy.
+// |known_policy_schemas| contains |Schema|s for known policies in the same
+// policy namespace of |map|. A policy in |map| but without an entry
+// |known_policy_schemas| is an unknown policy.
 void GetPolicyValues(
     const policy::PolicyMap& map,
     policy::PolicyErrorMap* errors,
     bool with_user_policies,
     bool convert_values,
-    std::unique_ptr<std::unordered_set<std::string>> policy_names,
+    const base::Optional<PolicyToSchemaMap>& known_policy_schemas,
     Value* values) {
   DCHECK(values);
   for (const auto& entry : map) {
@@ -121,8 +144,11 @@ void GetPolicyValues(
     if (policy.scope == policy::POLICY_SCOPE_USER && !with_user_policies)
       continue;
 
+    base::Optional<policy::Schema> known_policy_schema =
+        GetKnownPolicySchema(known_policy_schemas, policy_name);
     Value value(Value::Type::DICTIONARY);
-    value.SetKey("value", CopyAndMaybeConvert(*policy.value, convert_values));
+    value.SetKey("value", CopyAndMaybeConvert(*policy.value, convert_values,
+                                              known_policy_schema));
     value.SetKey("scope", Value((policy.scope == policy::POLICY_SCOPE_USER)
                                     ? "user"
                                     : "machine"));
@@ -132,9 +158,9 @@ void GetPolicyValues(
                            : "mandatory"));
     value.SetKey("source", Value(kPolicySources[policy.source].key));
     base::string16 error;
-    if (policy_names &&
-        policy_names->find(policy_name) == policy_names->end()) {
-      // We don't know what this policy is. This is an important error to show.
+    if (!known_policy_schema.has_value()) {
+      // We don't know what this policy is. This is an important error to
+      // show.
       error = l10n_util::GetStringUTF16(IDS_POLICY_UNKNOWN);
     } else {
       // The PolicyMap contains errors about retrieving the policy, while the
@@ -148,21 +174,24 @@ void GetPolicyValues(
   }
 }
 
-std::unique_ptr<std::unordered_set<std::string>> GetPolicyNameSet(
+base::Optional<PolicyToSchemaMap> GetKnownPolicies(
     const scoped_refptr<policy::SchemaMap> schema_map,
     const PolicyNamespace& policy_namespace) {
   const Schema* schema = schema_map->GetSchema(policy_namespace);
   // There is no policy name verification without valid schema.
   if (!schema || !schema->valid())
-    return nullptr;
-
-  auto policy_names = std::make_unique<std::unordered_set<std::string>>();
+    return base::nullopt;
 
+  // Build a vector first and construct the PolicyToSchemaMap (which is a
+  // |flat_map|) from that. The reason is that insertion into a |flat_map| is
+  // O(n), which would make the loop O(n^2), but constructing from a
+  // pre-populated vector is less expensive.
+  std::vector<std::pair<std::string, policy::Schema>> policy_to_schema_entries;
   for (policy::Schema::Iterator it = schema->GetPropertiesIterator();
        !it.IsAtEnd(); it.Advance()) {
-    policy_names->insert(it.key());
+    policy_to_schema_entries.push_back(std::make_pair(it.key(), it.schema()));
   }
-  return policy_names;
+  return PolicyToSchemaMap(std::move(policy_to_schema_entries));
 }
 
 void GetChromePolicyValues(content::BrowserContext* context,
@@ -200,7 +229,7 @@ void GetChromePolicyValues(content::BrowserContext* context,
   handler_list->PrepareForDisplaying(&map);
 
   GetPolicyValues(map, &errors, keep_user_policies, convert_values,
-                  GetPolicyNameSet(schema_map, policy_namespace), values);
+                  GetKnownPolicies(schema_map, policy_namespace), values);
 }
 
 }  // namespace
@@ -252,7 +281,7 @@ Value GetAllPolicyValuesAsDictionary(content::BrowserContext* context,
     policy::PolicyErrorMap empty_error_map;
     GetPolicyValues(GetPolicyService(context)->GetPolicies(policy_namespace),
                     &empty_error_map, with_user_policies, convert_values,
-                    GetPolicyNameSet(schema_map, policy_namespace),
+                    GetKnownPolicies(schema_map, policy_namespace),
                     &extension_policies);
     extension_values.SetKey(extension->id(), std::move(extension_policies));
   }

components/policy/core/common/schema.cc

@@ -33,6 +33,7 @@ using internal::PropertyNode;
 using internal::RestrictionNode;
 using internal::SchemaData;
 using internal::SchemaNode;
+using internal::SchemaNodeMetadata;
 
 namespace {
 
@@ -68,6 +69,15 @@ struct StorageSizes {
   size_t string_enums;
 };
 
+// A policy-specific extension to schema. If a schema contains this key and the
+// value is true, the policy value should not be displayed on the UI.
+constexpr char kSensitiveValue[] = "sensitiveValue";
+
+// |Schema::MaskSensitiveValues| will replace sensitive values with this string.
+// It should be consistent with the mask |NetworkConfigurationPolicyHandler|
+// uses for network credential fields.
+constexpr char kSensitiveValueMask[] = "********";
+
 // An invalid index, indicating that a node is not present; similar to a NULL
 // pointer.
 const int kInvalid = -1;
@@ -199,6 +209,21 @@ class Schema::InternalStorage
     return schema_data_.string_enums + index;
   }
 
+  // Returns the metadata entry for the |SchemaNode| at index
+  // |schema_node_index| or nullptr if the |SchemaNode| has no metadata.
+  const SchemaNodeMetadata* metadata(int schema_node_index) const {
+    if (!schema_data_.schema_nodes_metadata)
+      return nullptr;
+    return schema_data_.schema_nodes_metadata + schema_node_index;
+  }
+
+  // Returns true if there is a |SchemaNodeMetadata| entry for at least one
+  // |SchemaNode| in this |InternalStorage|. If this function returns false,
+  // |metadata(index)| calls will return nullptr for all valid |index| values.
+  bool has_metadata_for_any_schema_node() const {
+    return schema_data_.schema_nodes_metadata;
+  }
+
   // Compiles regular expression |pattern|. The result is cached and will be
   // returned directly next time.
   re2::RE2* CompileRegex(const std::string& pattern) const;
@@ -280,6 +305,7 @@ class Schema::InternalStorage
   std::vector<const char*> required_properties_;
   std::vector<int> int_enums_;
   std::vector<const char*> string_enums_;
+  std::vector<SchemaNodeMetadata> schema_nodes_metadata_;
 
   DISALLOW_COPY_AND_ASSIGN(InternalStorage);
 };
@@ -303,6 +329,7 @@ scoped_refptr<const Schema::InternalStorage> Schema::InternalStorage::Wrap(
   storage->schema_data_.string_enums = data->string_enums;
   storage->schema_data_.validation_schema_root_index =
       data->validation_schema_root_index;
+  storage->schema_data_.schema_nodes_metadata = data->schema_nodes_metadata;
   return storage;
 }
 
@@ -326,6 +353,7 @@ Schema::InternalStorage::ParseSchema(const base::DictionaryValue& schema,
   storage->required_properties_.reserve(sizes.required_properties);
   storage->int_enums_.reserve(sizes.int_enums);
   storage->string_enums_.reserve(sizes.string_enums);
+  storage->schema_nodes_metadata_.reserve(sizes.schema_nodes);
 
   int root_index = kInvalid;
   IdMap id_map;
@@ -357,6 +385,14 @@ Schema::InternalStorage::ParseSchema(const base::DictionaryValue& schema,
   if (!ResolveReferences(id_map, reference_list, error))
     return nullptr;
 
+  bool schema_nodes_metadata_used = false;
+  for (const SchemaNodeMetadata& metadata : storage->schema_nodes_metadata_) {
+    if (metadata.is_sensitive_value) {
+      schema_nodes_metadata_used = true;
+      break;
+    }
+  }
+
   SchemaData* data = &storage->schema_data_;
   data->schema_nodes = storage->schema_nodes_.data();
   data->property_nodes = storage->property_nodes_.data();
@@ -366,6 +402,12 @@ Schema::InternalStorage::ParseSchema(const base::DictionaryValue& schema,
   data->int_enums = storage->int_enums_.data();
   data->string_enums = storage->string_enums_.data();
   data->validation_schema_root_index = -1;
+  if (schema_nodes_metadata_used) {
+    data->schema_nodes_metadata = storage->schema_nodes_metadata_.data();
+  } else {
+    storage->schema_nodes_metadata_.clear();
+    data->schema_nodes_metadata = nullptr;
+  }
   return storage;
 }
 
@@ -495,9 +537,12 @@ bool Schema::InternalStorage::Parse(const base::DictionaryValue& schema,
 
   *index = static_cast<int>(schema_nodes_.size());
   schema_nodes_.push_back(SchemaNode());
+  schema_nodes_metadata_.push_back(SchemaNodeMetadata());
   SchemaNode* schema_node = &schema_nodes_.back();
+  SchemaNodeMetadata* schema_node_metadata = &schema_nodes_metadata_.back();
   schema_node->type = type;
   schema_node->extra = kInvalid;
+  schema_node_metadata->is_sensitive_value = false;
 
   if (type == base::Value::Type::DICTIONARY) {
     if (!ParseDictionary(schema, schema_node, id_map, reference_list, error))
@@ -529,6 +574,10 @@ bool Schema::InternalStorage::Parse(const base::DictionaryValue& schema,
     (*id_map)[id_string] = *index;
   }
 
+  bool is_sensitive_value = false;
+  if (schema.GetBoolean(kSensitiveValue, &is_sensitive_value))
+    schema_node_metadata->is_sensitive_value = is_sensitive_value;
+
   return true;
 }
 
@@ -1023,6 +1072,18 @@ bool Schema::Normalize(base::Value* value,
   return Validate(*value, strategy, error_path, error);
 }
 
+void Schema::MaskSensitiveValues(base::Value* value) const {
+  if (!valid())
+    return;
+
+  // If there's no metadata in the |storage_|, no value has been marked as
+  // sensitive.
+  if (!storage_->has_metadata_for_any_schema_node())
+    return;
+
+  MaskSensitiveValuesRecurse(value);
+}
+
 // static
 Schema Schema::Parse(const std::string& content, std::string* error) {
   // Validate as a generic JSON schema, and ignore unknown attributes; they
@@ -1194,6 +1255,34 @@ bool Schema::ValidateStringRestriction(int index, const char* str) const {
   }
 }
 
+void Schema::MaskSensitiveValuesRecurse(base::Value* value) const {
+  if (IsSensitiveValue())
+    *value = base::Value(kSensitiveValueMask);
+
+  if (value->type() != type())
+    return;
+
+  base::DictionaryValue* dict = nullptr;
+  base::ListValue* list = nullptr;
+  if (value->GetAsDictionary(&dict)) {
+    // Iterating over |base::Value::DictItems()| yields temporaries of the type
+    // |std::pair<const std::string&, base::Value&>|. It is not possible to
+    // capture the returned |std::pair| in a reference. However, it is still
+    // possible to mutate the |base::Value| through the |pair|'s |second|, which
+    // itself is a reference.
+    for (std::pair<const std::string&, base::Value&> dict_item :
+         dict->DictItems()) {
+      auto& value = dict_item.second;
+      SchemaList schema_list = GetMatchingProperties(dict_item.first);
+      for (const auto& schema_item : schema_list)
+        schema_item.MaskSensitiveValuesRecurse(&value);
+    }
+  } else if (value->GetAsList(&list)) {
+    for (auto& list_elem : list->GetList())
+      GetItems().MaskSensitiveValuesRecurse(&list_elem);
+  }
+}
+
 Schema Schema::GetValidationSchema() const {
   CHECK(valid());
   const SchemaNode* validation_schema_root_node =
@@ -1203,4 +1292,17 @@ Schema Schema::GetValidationSchema() const {
   return Schema(storage_, validation_schema_root_node);
 }
 
+bool Schema::IsSensitiveValue() const {
+  CHECK(valid());
+
+  // This is safe because |node_| is guaranteed to have been returned from
+  // |storage_| and |storage_->root_node()| always returns to the |SchemaNode|
+  // with index 0.
+  int index = node_ - storage_->root_node();
+  const SchemaNodeMetadata* metadata = storage_->metadata(index);
+  if (!metadata)
+    return false;
+  return metadata->is_sensitive_value;
+}
+
 }  // namespace policy

components/policy/core/common/schema.h

@@ -117,6 +117,13 @@ class POLICY_EXPORT Schema {
                  std::string* error,
                  bool* changed) const;
 
+  // Modifies |value| in place - masks values that have been marked as sensitive
+  // ("sensitiveValue": true) in this Schema. Note that |value| may not be
+  // schema-valid according to this Schema after this function returns - the
+  // masking is performed by replacing values with string values, so the value
+  // types may not correspond to this Schema anymore.
+  void MaskSensitiveValues(base::Value* value) const;
+
   // Used to iterate over the known properties of Type::DICTIONARY schemas.
   class POLICY_EXPORT Iterator {
    public:
@@ -193,6 +200,10 @@ class POLICY_EXPORT Schema {
   // |chrome_schema| is the root schema that has all policies as children.
   Schema GetValidationSchema() const;
 
+  // If this returns true, the schema metadata says that the value described by
+  // this schema should not be displayed on the UI.
+  bool IsSensitiveValue() const;
+
  private:
   // Builds a schema pointing to the inner structure of |storage|,
   // rooted at |node|.
@@ -202,6 +213,8 @@ class POLICY_EXPORT Schema {
   bool ValidateIntegerRestriction(int index, int value) const;
   bool ValidateStringRestriction(int index, const char* str) const;
 
+  void MaskSensitiveValuesRecurse(base::Value* value) const;
+
   scoped_refptr<const InternalStorage> storage_;
   const internal::SchemaNode* node_;
 };

components/policy/core/common/schema_internal.h

@@ -122,6 +122,11 @@ union POLICY_EXPORT RestrictionNode {
   } string_pattern_restriction;
 };
 
+// Contains metadata for a SchemaNode. This is separate from SchemaNode, because
+// schemas which don't use metadata will not have this.
+struct SchemaNodeMetadata {
+  bool is_sensitive_value;
+};
 
 // Contains arrays of related nodes. All of the offsets in these nodes reference
 // other nodes in these arrays.
@@ -135,6 +140,10 @@ struct POLICY_EXPORT SchemaData {
   const int* int_enums;
   const char* const* string_enums;
   int validation_schema_root_index;
+
+  // May be nullptr. If this is not nullptr, uses the same indices as
+  // |schema_nodes|.
+  const SchemaNodeMetadata* schema_nodes_metadata;
 };
 
 }  // namespace internal

components/policy/core/common/schema_unittest.cc

@@ -590,19 +590,19 @@ TEST(SchemaTest, Wrap) {
   };
 
   const internal::SchemaData kData = {
-    kSchemas,
-    kPropertyNodes,
-    kProperties,
-    kRestriction,
-    kRequired,
-    kIntEnums,
-    kStringEnums,
+      kSchemas,  kPropertyNodes, kProperties,  kRestriction,
+      kRequired, kIntEnums,      kStringEnums,
+      -1,       // validation_schema_root_index
+      nullptr,  // schema_nodes_metadata
   };
 
   Schema schema = Schema::Wrap(&kData);
   ASSERT_TRUE(schema.valid());
   EXPECT_EQ(base::Value::Type::DICTIONARY, schema.type());
 
+  // Wrapped schemas have no |SchemaNodeMetadata| elements.
+  EXPECT_FALSE(schema.IsSensitiveValue());
+
   struct {
     const char* key;
     base::Value::Type type;
@@ -1239,6 +1239,80 @@ TEST(SchemaTest, ItemsReference) {
   ASSERT_EQ(base::Value::Type::BOOLEAN, items.type());
 }
 
+TEST(SchemaTest, SchemaNodeMetadataSensitiveValues) {
+  std::string error;
+  Schema schema = Schema::Parse(R"({
+    "type": "object",
+    "properties": {
+      "foo": {
+        "type": "boolean"
+      },
+      "bar": {
+        "type": "string",
+        "sensitiveValue": true
+      }
+    }
+  })",
+                                &error);
+  ASSERT_TRUE(schema.valid()) << error;
+  ASSERT_EQ(base::Value::Type::DICTIONARY, schema.type());
+  EXPECT_FALSE(schema.IsSensitiveValue());
+
+  Schema foo_schema = schema.GetKnownProperty("foo");
+  ASSERT_TRUE(foo_schema.valid());
+  EXPECT_EQ(base::Value::Type::BOOLEAN, foo_schema.type());
+  EXPECT_FALSE(foo_schema.IsSensitiveValue());
+
+  Schema bar_schema = schema.GetKnownProperty("bar");
+  ASSERT_TRUE(bar_schema.valid());
+  EXPECT_EQ(base::Value::Type::STRING, bar_schema.type());
+  EXPECT_TRUE(bar_schema.IsSensitiveValue());
+
+  // Run |MaskSensitiveValues| on the top-level schema
+  base::Value value(base::Value::Type::DICTIONARY);
+  value.SetKey("foo", base::Value(true));
+  value.SetKey("bar", base::Value("testvalue"));
+  schema.MaskSensitiveValues(&value);
+
+  base::Value value_expected(base::Value::Type::DICTIONARY);
+  value_expected.SetKey("foo", base::Value(true));
+  value_expected.SetKey("bar", base::Value("********"));
+  EXPECT_EQ(value_expected, value);
+
+  // Run |MaskSensitiveValues| on a sub-schema
+  base::Value bar_value("testvalue");
+  bar_schema.MaskSensitiveValues(&bar_value);
+  EXPECT_EQ(base::Value("********"), bar_value);
+}
+
+TEST(SchemaTest, SchemaNodeNoSensitiveValues) {
+  std::string error;
+  Schema schema = Schema::Parse(R"({
+    "type": "object",
+    "properties": {
+      "foo": {
+        "type": "boolean"
+      }
+    }
+  })",
+                                &error);
+  ASSERT_TRUE(schema.valid()) << error;
+  ASSERT_EQ(base::Value::Type::DICTIONARY, schema.type());
+  EXPECT_FALSE(schema.IsSensitiveValue());
+
+  Schema foo = schema.GetKnownProperty("foo");
+  ASSERT_TRUE(foo.valid());
+  EXPECT_EQ(base::Value::Type::BOOLEAN, foo.type());
+  EXPECT_FALSE(foo.IsSensitiveValue());
+
+  base::Value value(base::Value::Type::DICTIONARY);
+  value.SetKey("foo", base::Value(true));
+
+  base::Value expected_value = value.Clone();
+  schema.MaskSensitiveValues(&value);
+  EXPECT_EQ(expected_value, value);
+}
+
 TEST(SchemaTest, EnumerationRestriction) {
   // Enum attribute is a list.
   EXPECT_TRUE(ParseFails(SchemaObjectWrapper(R"({

components/policy/tools/generate_policy_source.py

@@ -715,6 +715,7 @@ class SchemaNodesGenerator:
     f.write('  kStringEnumerations,\n' if self.string_enums else '  NULL,\n')
     f.write('  %d,  // validation_schema root index\n'
         % self.validation_schema_root_index)
+    f.write('  nullptr,  // schema_nodes_metadata\n')
     f.write('};\n\n')
 
   def GetByID(self, id_str):