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

crypto/nss_util.cc

Index: crypto/nss_util.cc
===================================================================
--- crypto/nss_util.cc	(revision 99168)
+++ crypto/nss_util.cc	(working copy)
@@ -31,6 +31,10 @@
 #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
@@ -83,6 +87,17 @@
   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
@@ -288,6 +303,51 @@
     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)
 
 
@@ -702,6 +762,9 @@
   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