This adds support for encrypted ONC import

crypto/hmac.cc

 // Copyright (c) 2011 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 "crypto/hmac.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "crypto/secure_util.h"
+#include "crypto/symmetric_key.h"
 
 namespace crypto {
 
+bool HMAC::Init(const SymmetricKey& key) {
+  std::string raw_key;
+  key.GetRawKey(&raw_key);

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

GetRawKey can fail (eg: non-exportable)

bool result = key.GetRawKey(&raw_key) && Init(raw_key);

I suppose a TODO at some point will be to not require exporting it to a raw key
(It really isn't needed if we have a SymmetricKey, from an underlying OS
perspective - so it's solely an API design issue)

+  bool result = Init(raw_key);
+  // Zero out key copy.
+  raw_key.assign(raw_key.length(), std::string::value_type());
+  raw_key.clear();
+  raw_key.reserve(0);

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

Unfortunately, it's possible (and in practice, likely) for this to be optimized
out. Namely, the compiler can realize this is a memset(raw_ptr, 0, size)
followed by a free(raw_ptr), see that raw_ptr is never accessed after the
memset, and optimize out the memset.

So if you're trying to do the equivalent of SecureZeroMemory and wipe the
memory, more is needed. For more info about the ongoing arms race w/ compilers,
see
https://www.securecoding.cert.org/confluence/display/cplusplus/MSC06-CPP.+Be+...

If it's not that important (again, depends on the risk scenario, although I am a
fan of memory scrubbing), it's probably safe to remove.

If you do add it, see crypto/secure_util.h for the ideal place :)

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

I took this code from the HMAC destructor on mac, so that will probably need
fixing too.

Is it sufficient to call std::fill(key_.begin(), key_.end(), 0)?  I'm guessing
not, especially in a destructor where the compiler knows you're not going to
touch it again.  In that case, we also have problems in the SymmetricKey
destructor(s).

I started to implement SecureZeroMemory and SecureZeroString, and decided that
although it is probably correct to blank out any key material, simply using
std::string to store the key material is problematic in that regard: there's no
controlling when it will realloc and copy without clearing the old key data
memory.  The only way to do that would be to implement our own string allocator
and create a different basic_string specialization, and I think that's probably
way beyond the scope of this CL.

I'm going to go ahead and switch this to use std::fill as in the SymmetricKey
destructors, and then assign the result of raw_key.c_str() to a volatile char*
in the hopes that this will force the compiler to leave it around, but I'm not
optimistic that it will be sufficient in the long run (it might just touch the
first byte).  When compiling a test program with -O3 (and -g for debug info), it
appears to work in the debugger, but that might not be enough of a test, since
g++ has so many different optimization modes.

I've also noticed several places (mostly in the sync code) were we don't even
zero things out after using the key, so perhaps it's not that critical.

I could still implement the functions if you want, but probably not in this CL.

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

std::fill() just gets turned into a memset(), due to the iterator traits of
.begin()/.end() (pointer type). Which then goes back to the original problem.

I agree absolutely that using std::string() to secure secrets is problematic, IF
operating under a threat-model that keeping secrets in RAM may cause them to be
leak. I'm not aware of us in general really considering that threat model as
valid - otherwise we'd also look at things like
CryptProtectData/CryptUnprotectData for tranient work.

I agree it's beyond the scope of this CL - which is where I was trying to go
with it. Under an optimization pass, this can ("should") be optimized out, so it
doesn't really buy much from security; thus, it could be removed. If it is a
concern/part of the threat model, we should file a bug and start auditing code.
But like I said above, I don't really think this has been a threat-model we've
entertained.

+  return result;
+}
+
 size_t HMAC::DigestLength() const {
   switch (hash_alg_) {
     case SHA1:
       return 20;
     case SHA256:
       return 32;
     default:
       NOTREACHED();
       return 0;
   }
@@ skipping 14 matching lines @@
   scoped_array<unsigned char> computed_digest(
       new unsigned char[digest_length]);
   if (!Sign(data, computed_digest.get(), static_cast<int>(digest_length)))
     return false;
 
   return SecureMemEqual(digest.data(), computed_digest.get(),
                         std::min(digest.size(), digest_length));
 }
 
 }  // namespace crypto