Changed OAuth token+secret encryption to use supplemental user key.

crypto/nss_util.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/nss_util.h"
 #include "crypto/nss_util_internal.h"
 
 #include <nss.h>
 #include <plarena.h>
 #include <prerror.h>
@@ skipping 13 matching lines @@
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/native_library.h"
 #include "base/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
 #include "crypto/scoped_nss_types.h"
 
+#if defined(OS_CHROMEOS)
+#include "crypto/symmetric_key.h"
+#endif
+
 // USE_NSS means we use NSS for everything crypto-related.  If USE_NSS is not
 // defined, such as on Mac and Windows, we use NSS for SSL only -- we don't
 // use NSS for crypto or certificate verification, and we don't use the NSS
 // certificate and key databases.
 #if defined(USE_NSS)
 #include "base/synchronization/lock.h"
 #include "crypto/crypto_module_blocking_password_delegate.h"
 #endif  // defined(USE_NSS)
 
 namespace crypto {
@@ skipping 32 matching lines @@
     return dir;
   }
   dir = dir.AppendASCII(".pki").AppendASCII("nssdb");
   if (!file_util::CreateDirectory(dir)) {
     LOG(ERROR) << "Failed to create " << dir.value() << " directory.";
     dir.clear();
   }
   return dir;
 }
 
+#if defined(OS_CHROMEOS)
+// Supplemental user key id.
+static unsigned char kSupplementalUserKeyId[] = {
+  0xCC, 0x13, 0x19, 0xDE, 0x75, 0x5E, 0xFE, 0xFA,
+  0x5E, 0x71, 0xD4, 0xA6, 0xFB, 0x00, 0x00, 0xCC
+};

[wtc, 2011/09/06 21:35:17]

Nit: on line 92, remove 'static'.

You said:
You mean it's 16 random bytes?  The reason I suspected it
may be a non-random key ID was that there were two 0x00
bytes.

The key ID doesn't need to be meaningful nor registered.

[zel, 2011/09/06 22:33:35]

Done.

+// Supplemental user key nickname.
+const char kSupplementalKeyNickname[] = "ChromeOS_SupplementalUserKey";

[wtc, 2011/09/06 21:35:17]

I believe the nickname can contain spaces, so you don't need
to use an underscore '_' or CamelCase.

[zel, 2011/09/06 22:33:35]

I have removed the nickname, I have no good use for it anyway.

+#endif  // defined(OS_CHROMEOS)
+
+
 // On non-chromeos platforms, return the default config directory.
 // On chromeos, return a read-only directory with fake root CA certs for testing
 // (which will not exist on non-testing images).  These root CA certs are used
 // by the local Google Accounts server mock we use when testing our login code.
 // If this directory is not present, NSS_Init() will fail.  It is up to the
 // caller to failover to NSS_NoDB_Init() at that point.
 FilePath GetInitialConfigDirectory() {
 #if defined(OS_CHROMEOS)
   return FilePath(kReadOnlyCertDB);
 #else
@@ skipping 185 matching lines @@
   bool IsTPMTokenReady() {
     return tpm_slot_ != NULL;
   }
 
   PK11SlotInfo* GetTPMSlot() {
     std::string token_name;
     GetTPMTokenInfo(&token_name, NULL);
     return FindSlotWithTokenName(token_name);
   }
 
+  SymmetricKey* GetSupplementalUserKey() {
+    DCHECK(chromeos_user_logged_in_);
+
+    PK11SlotInfo* slot = NULL;
+    PK11SymKey *key = NULL;
+    PLArenaPool *arena = 0;

[wtc, 2011/09/06 21:35:17]

Nit: on line 310, put '*' next to the type.

On line 311, delete 'arena' (and all the code that creates
and frees 'arena' below).  'arena' is no longer being used.

[zel, 2011/09/06 22:33:35]

Done.

+    SECItem keyID;
+    CK_MECHANISM_TYPE type = CKM_AES_ECB;

[Will Drewry, 2011/09/06 20:20:21]

Not that it really matters for this usage, but might be good to pick something
other than ECB (CBC, OFB maybe? :)

[zel, 2011/09/06 21:02:22]

crypto::Encryptor class seems to support CKM_AES_CBC_PAD (Encryptor::CBC) and
CKM_AES_ECB (Encryptor::CTR) only. Since CBC needed IV and I didn't have any, I
went with CKM_AES_ECB (Encryptor::CTR). WE already used that one for other AES
encryption in ParallelAuthenticator. 

+
+    arena = PORT_NewArena(SEC_ASN1_DEFAULT_ARENA_SIZE);
+    if (!arena)
+      goto done;
+
+    slot = GetPublicNSSKeySlot();
+    if (!slot)
+      goto done;
+
+    if (PK11_Authenticate(slot, PR_TRUE, NULL) != SECSuccess)
+      goto done;
+
+    keyID.type = siBuffer;
+    keyID.data = kSupplementalUserKeyId;
+    keyID.len = static_cast<int>(sizeof(kSupplementalUserKeyId));
+
+    // Find/generate AES key.
+    key = PK11_FindFixedKey(slot, type, &keyID, NULL);
+    if (!key) {
+      key = PK11_TokenKeyGen(slot, type, NULL,
+                             32,   /* keysize in bytes*/

[wtc, 2011/09/06 21:35:17]

Nit: use C++ style comment:
  // keysize in bytes

Please create a constant for 32 around line 95 above.

[zel, 2011/09/06 22:33:35]

Done.

+                             &keyID, PR_TRUE, NULL);
+      if (key && PK11_SetSymKeyNickname(key,
+             kSupplementalKeyNickname) != SECSuccess) {

[wtc, 2011/09/06 21:35:17]

I think this if statement should say:
    if (!key || PK11_SetSymKeyNickname(key,
         kSupplementalKeyNickname) != SECSuccess) {

IMPORTANT: does the supplemental key have to have a nickname?
I don't think a nickname is necessary.  If you get rid of
the PK11_SetSymKeyNickname call, then you don't need to
fix the leak that Will Drewry pointed out.

[zel, 2011/09/06 22:33:35]

Done.

+          goto done;

[Will Drewry, 2011/09/06 20:20:21]

How is this any different than doing nothing since the next bit after the
conditonal is the done label.

Should it release the key if key != NULL but the nickname fails or will this
possibly leak generated keys?

[zel, 2011/09/06 21:02:22]

Done.

+      }
+    }
+
+  done:
+    if (arena)
+      PORT_FreeArena(arena, PR_TRUE);
+
+    if (slot)
+      PK11_FreeSlot(slot);
+
+    return key ? new SymmetricKey(key) : NULL;
+  }
 #endif  // defined(OS_CHROMEOS)
 
 
   bool OpenTestNSSDB(const FilePath& path, const char* description) {
     test_slot_ = OpenUserDB(path, description);
     return !!test_slot_;
   }
 
   void CloseTestNSSDB() {
     if (test_slot_) {
@@ skipping 394 matching lines @@
 }
 
 bool IsTPMTokenReady() {
   return g_nss_singleton.Get().IsTPMTokenReady();
 }
 
 bool EnsureTPMTokenReady() {
   return g_nss_singleton.Get().EnsureTPMTokenReady();
 }
 
+SymmetricKey* GetSupplementalUserKey() {
+  return g_nss_singleton.Get().GetSupplementalUserKey();
+}
 #endif  // defined(OS_CHROMEOS)
 
 // TODO(port): Implement this more simply.  We can convert by subtracting an
 // offset (the difference between NSPR's and base::Time's epochs).
 base::Time PRTimeToBaseTime(PRTime prtime) {
   PRExplodedTime prxtime;
   PR_ExplodeTime(prtime, PR_GMTParameters, &prxtime);
 
   base::Time::Exploded exploded;
   exploded.year         = prxtime.tm_year;
@@ skipping 10 matching lines @@
 
 PK11SlotInfo* GetPublicNSSKeySlot() {
   return g_nss_singleton.Get().GetPublicNSSKeySlot();
 }
 
 PK11SlotInfo* GetPrivateNSSKeySlot() {
   return g_nss_singleton.Get().GetPrivateNSSKeySlot();
 }
 
 }  // namespace crypto