This adds support for encrypted ONC import

chrome/browser/chromeos/cros/onc_network_parser.cc

Index: chrome/browser/chromeos/cros/onc_network_parser.cc
diff --git a/chrome/browser/chromeos/cros/onc_network_parser.cc b/chrome/browser/chromeos/cros/onc_network_parser.cc
index 69ba0da80e4878f12978c52fb60bb23e3fee8943..f2eb75d18a0cdebba2421ef0240e6861fdd25cbc 100644
--- a/chrome/browser/chromeos/cros/onc_network_parser.cc
+++ b/chrome/browser/chromeos/cros/onc_network_parser.cc
@@ -17,6 +17,10 @@
 #include "chrome/browser/chromeos/cros/network_library.h"
 #include "chrome/browser/chromeos/cros/onc_constants.h"
 #include "chrome/common/net/x509_certificate_model.h"
+#include "crypto/hmac.h"
+#include "crypto/scoped_nss_types.h"
+#include "crypto/symmetric_key.h"
+#include "crypto/encryptor.h"

[Ryan Sleevi, 2012/01/04 01:33:43]

nit: lexicographical sort

[Greg Spencer (Chromium), 2012/01/05 22:18:03]

Done.

 #include "grit/generated_resources.h"
 #include "net/base/cert_database.h"
 #include "net/base/crypto_module.h"
@@ -199,6 +203,7 @@ std::string ConvertValueToString(const base::Value& value) {
 // -------------------- OncNetworkParser --------------------
 
 OncNetworkParser::OncNetworkParser(const std::string& onc_blob,
+                                   const std::string& passphrase,
                                    NetworkUIData::ONCSource onc_source)
     : NetworkParser(get_onc_mapper()),
       onc_source_(onc_source),
@@ -213,15 +218,25 @@ OncNetworkParser::OncNetworkParser(const std::string& onc_blob,
     LOG(WARNING) << "OncNetworkParser received bad ONC file: " << parse_error_;
   } else {
     root_dict_.reset(static_cast<DictionaryValue*>(root.release()));
+
+    // Check and see if this is an encrypted ONC file.  If so, decrypt it.
+    std::string ciphertext_test;
+    if (root_dict_->GetString("Ciphertext", &ciphertext_test))
+      root_dict_.reset(Decrypt(passphrase, root_dict_.get()));
+
+    // Decryption failed, errors will be in parse_error_;
+    if (!root_dict_.get())
+      return;
+
     // At least one of NetworkConfigurations or Certificates is required.
     bool has_network_configurations =
-      root_dict_->GetList("NetworkConfigurations", &network_configs_);
+        root_dict_->GetList("NetworkConfigurations", &network_configs_);
     bool has_certificates =
-      root_dict_->GetList("Certificates", &certificates_);
+        root_dict_->GetList("Certificates", &certificates_);
     VLOG(2) << "ONC file has " << GetNetworkConfigsSize() << " networks and "
             << GetCertificatesSize() << " certificates";
     LOG_IF(WARNING, (!has_network_configurations && !has_certificates))
-      << "ONC file has no NetworkConfigurations or Certificates.";
+        << "ONC file has no NetworkConfigurations or Certificates.";
   }
 }
 
@@ -239,6 +254,101 @@ const EnumMapper<PropertyIndex>* OncNetworkParser::property_mapper() {
   return get_onc_mapper();
 }
 
+base::DictionaryValue* OncNetworkParser::Decrypt(
+    const std::string& passphrase,
+    base::DictionaryValue* root) {
+  std::string salt;
+  int iterations;
+  const int key_size_in_bits = 256;
+  std::string initial_vector_str;
+  std::string ciphertext;
+  std::string cipher;
+  std::string hmac_method;
+  std::string hmac;
+
+  if (!root->GetString("Salt", &salt) ||
+      !root->GetInteger("Iterations", &iterations) ||
+      !root->GetString("Cipher", &cipher) ||
+      !root->GetString("HMACMethod", &hmac_method) ||
+      !root->GetString("HMAC", &hmac) ||
+      !root->GetString("IV", &initial_vector_str) ||
+      !root->GetString("Ciphertext", &ciphertext)) {
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_MALFORMED);
+    return NULL;
+  }
+
+  if (hmac_method != "SHA1" || cipher != "AES256") {
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_UNSUPPORTED_ENCRYPTION);
+    return NULL;
+  }
+
+  if (!base::Base64Decode(salt, &salt)) {
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_UNABLE_TO_DECODE);
+    return NULL;
+  }
+
+  scoped_ptr<crypto::SymmetricKey> key(
+      crypto::SymmetricKey::DeriveKeyFromPassword(crypto::SymmetricKey::AES,
+                                                  passphrase,
+                                                  salt,
+                                                  iterations,
+                                                  key_size_in_bits));
+
+
+  crypto::Encryptor decryptor;
+  std::string initial_vector;
+  std::string plaintext;
+  if (!base::Base64Decode(initial_vector_str, &initial_vector)) {
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_UNABLE_TO_DECODE);
+    return NULL;
+  }
+  if (!decryptor.Init(key.get(), crypto::Encryptor::CBC, initial_vector))  {
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_UNABLE_TO_DECRYPT);
+    return NULL;
+  }
+  if (!base::Base64Decode(ciphertext, &ciphertext)) {
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_UNABLE_TO_DECODE);
+    return NULL;
+  }
+  if (!base::Base64Decode(hmac, &hmac)) {

[Ryan Sleevi, 2012/01/04 01:33:43]

nit: Move these decodes (Line 304, 314, 319) above the decryptor init (309) and
HMAC verify (328).

The reasoning (which is admittedly weak) is that .Init() may perform non-trivial
work (eg: a really high iteration) and thus may block for a while.

Related to that above statement, is it safe to assume this code all runs on a
worker thread? If not, what prevents someone from sending a really high
iteration count and blocking a browser thread (assuming FILE || IO)

[Greg Spencer (Chromium), 2012/01/05 22:18:03]

Done.
There's really nothing preventing that at the moment.  When we integrate this
into the final UI, it will need to be run on a worker thread.

Maybe I should add a sanity check for the iteration count (so it has to be, say,
under 50K)?

[Ryan Sleevi, 2012/01/05 22:34:11]

For in-progress code, I think it's fine as-is, as long as you're aware of the
risk and plan to address it. If you think it might get dropped, perhaps add a
TODO here and file a tracking bug for it, just so that we can revisit?

A worker thread + a timer will be a much more robust solution than a fixed upper
limit, since as processors improve (and algorithms are weakened), it's natural
to see the iteration count go up. Classic example is iPhone going from 4K
iterations -> 10K -> 40K presently, IIRC.

I think 50K may be a bit too low (even if it's probably a reasonable number).
Setting the upper limit to something closer to 100-150K is probably good enough
for the paranoid 'for now' (and for the +6 month timeframe that it would take to
deploy an update if the max needed to be bumped)

But for this CL, as long as you're aware of it, I don't think anything else
codewise needs to be done.

+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_UNABLE_TO_DECODE);
+    return NULL;
+  }
+
+  // Verify the HMAC.  If it fails, then we will still try to decrypt because
+  // we don't want to leak timing information.
+  bool hmac_failed = false;
+  crypto::HMAC hmac_verifier(crypto::HMAC::SHA1);
+  if (!hmac_verifier.Init(*key) || !hmac_verifier.Verify(ciphertext, hmac))
+    hmac_failed = true;
+
+  if (!decryptor.Decrypt(ciphertext, &plaintext) || hmac_failed) {
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_UNABLE_TO_DECRYPT);
+    return NULL;
+  }
+
+  // Now we've decrypted it, let's deserialize the decrypted data.
+  JSONStringValueSerializer deserializer(plaintext);
+  deserializer.set_allow_trailing_comma(true);
+  scoped_ptr<base::Value> new_root(deserializer.Deserialize(NULL,
+                                                            &parse_error_));
+  if (!new_root.get() || new_root->GetType() != base::Value::TYPE_DICTIONARY) {

[Ryan Sleevi, 2012/01/04 01:33:43]

nit: I believe the preference is to use

new_root->IsType(base::Value::TYPE_DICTIONARY)

[Greg Spencer (Chromium), 2012/01/05 22:18:03]

Done.

+    if (parse_error_.empty())
+      parse_error_ = l10n_util::GetStringUTF8(
+          IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
+    return NULL;
+  }
+  return static_cast<base::DictionaryValue*>(new_root.release());

[Ryan Sleevi, 2012/01/04 01:33:43]

side nit: From an API perspective, it's unclear which is more correct - this
call (assuming DictionaryValue) or using the base::Value::GetAsDictionary()
method.

From just reading the headers, it seems like GetAsDictionary is "more" correct,
but perhaps this is just trivium.

[Greg Spencer (Chromium), 2012/01/05 22:18:03]

I see your point, but this is what it translates into, since I still need to
release the pointer from the scoped_ptr:

  base::DictionaryValue* result = NULL;
  new_root->GetAsDictionary(&result);
  new_root.release();
  return result;

And that seems a lot more cumbersome, and no safer in the end, since we've
already verified that it really is a dictionary by this point.

[Ryan Sleevi, 2012/01/05 22:34:11]

Agreed - but it does seem like the API (of the base::Value) is a bit "broken" in
that it provides a convenience method (a /virtual/ one, on top of that, so
clearly meant to accommodate class hierarchies) that is less useful/more
cumbersome for users.

It's a side nit, and leaving it as is seems to make sense.

+}
+
 int OncNetworkParser::GetNetworkConfigsSize() const {
   return network_configs_ ? network_configs_->GetSize() : 0;
 }