Fix ONC password filtering in about:policy.

chrome/browser/policy/configuration_policy_handler_chromeos.cc

 // Copyright (c) 2012 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/policy/configuration_policy_handler_chromeos.h"

[pneubeck (no reviews), 2012/08/09 07:59:20]

Nit:
btw. the header is missing at least a forward declaration of DictionaryValue.
base/values.h should be included explicitly in .cc.
'base' namespace could be added to references to types/functions of values.h

 
+#include <algorithm>
 #include <string>
+#include <vector>
 
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/string_util.h"
+#include "chrome/browser/chromeos/cros/onc_constants.h"
 #include "chrome/browser/chromeos/cros/onc_network_parser.h"
 #include "chrome/browser/policy/policy_error_map.h"
 #include "chrome/browser/policy/policy_map.h"
 #include "chrome/browser/prefs/pref_value_map.h"
 #include "chrome/browser/ui/ash/chrome_launcher_prefs.h"
 #include "chrome/common/pref_names.h"
 #include "grit/generated_resources.h"
 #include "policy/policy_constants.h"
 
+namespace onc = chromeos::onc;
+
 namespace policy {
 
 NetworkConfigurationPolicyHandler::NetworkConfigurationPolicyHandler(
     const char* policy_name,
     chromeos::NetworkUIData::ONCSource onc_source)
     : TypeCheckingPolicyHandler(policy_name, Value::TYPE_STRING),
       onc_source_(onc_source) {}
 
 NetworkConfigurationPolicyHandler::~NetworkConfigurationPolicyHandler() {}
 
@@ skipping 72 matching lines @@
                                      base::JSONWriter::OPTIONS_DO_NOT_ESCAPE |
                                          base::JSONWriter::OPTIONS_PRETTY_PRINT,
                                      &json_string);
   return Value::CreateStringValue(json_string);
 }
 
 // static
 void NetworkConfigurationPolicyHandler::StripSensitiveValues(
     DictionaryValue* network_dict) {
   // List of settings we filter from the network dictionary.
-  static const char* kFilteredSettings[] = {
-    "WiFi.Passphrase",
-    "IPsec.EAP.Password",
-    "IPsec.EAP.Password",
-    "IPsec.XAUTH.Password",
-    "L2TP.Password",
+  std::vector<std::string> filtered_settings;

[pneubeck (no reviews), 2012/08/09 07:59:20]

Unused? If so, also remove the comment above.

[Mattias Nissler (ping if slow), 2012/08/09 09:09:14]

Done.

+  static const int kMaxComponents = 4;
+  static const char* kFilteredSettings[][kMaxComponents] = {

[pneubeck (no reviews), 2012/08/09 07:59:20]

Add a comment, that these are the components of a dictionary path which are to
be garbled. Each entry must be NULL-terminated.

Overall, the terms "filter" and "strip" are not used correctly, IMO. Instead the
sensitive data is "garbled"/"hidden"/"starred"...

[Mattias Nissler (ping if slow), 2012/08/09 09:09:14]

Done.

+    { onc::kEthernet, onc::ethernet::kEAP, onc::eap::kPassword },
+    { onc::kVPN, onc::vpn::kIPsec, onc::vpn::kPSK },
+    { onc::kVPN, onc::vpn::kL2TP, onc::vpn::kPassword },
+    { onc::kVPN, onc::vpn::kOpenVPN, onc::vpn::kPassword },
+    { onc::kWiFi, onc::wifi::kEAP, onc::eap::kPassword },
+    { onc::kWiFi, onc::wifi::kPassphrase },

[Joao da Silva, 2012/08/09 07:29:33]

Suggestion: align the 2nd and 3rd column entries?

[Mattias Nissler (ping if slow), 2012/08/09 09:09:14]

That would suggest the entries correspond to each other when in fact they're
not.

   };
+
   // Placeholder to insert in place of the filtered setting.
   static const char kPlaceholder[] = "********";
 
   for (size_t i = 0; i < arraysize(kFilteredSettings); ++i) {
-    if (network_dict->Remove(kFilteredSettings[i], NULL)) {
-      network_dict->Set(kFilteredSettings[i],
-                        Value::CreateStringValue(kPlaceholder));
-    }
+    std::vector<std::string> parts(
+        kFilteredSettings[i],
+        std::find(kFilteredSettings[i],
+                  kFilteredSettings[i] + kMaxComponents - 1,
+                  static_cast<const char*>(NULL)));

[Joao da Silva, 2012/08/09 07:29:33]

I'm not sure if the memory bots won't complain about reading uninitialized
values; maybe add explicit NULLs?

[Mattias Nissler (ping if slow), 2012/08/09 09:09:14]

The C++ standard says that missing elements in array initializers are
initialized to zeros.

+    std::string key(JoinString(parts, '.'));
+    if (network_dict->Remove(key, NULL))
+      network_dict->Set(key, Value::CreateStringValue(kPlaceholder));

[pneubeck (no reviews), 2012/08/09 07:59:20]

Maybe create the StringValue only once.

[Mattias Nissler (ping if slow), 2012/08/09 09:09:14]

No, we need fresh copies for each Set()

   }
 }
 
 PinnedLauncherAppsPolicyHandler::PinnedLauncherAppsPolicyHandler()
     : ExtensionListPolicyHandler(key::kPinnedLauncherApps,
                                  prefs::kPinnedLauncherApps,
                                  false) {}
 
 PinnedLauncherAppsPolicyHandler::~PinnedLauncherAppsPolicyHandler() {}
 
@@ skipping 12 matching lines @@
         base::DictionaryValue* app_dict = new base::DictionaryValue();
         app_dict->SetString(ash::kPinnedAppsPrefAppIDPath, id);
         pinned_apps_list->Append(app_dict);
       }
     }
     prefs->SetValue(pref_path(), pinned_apps_list);
   }
 }
 
 }  // namespace policy