Use an arena better in private_key_export

Both allocate and free the arena in the caller.  Use the arena to
allocate der_info to make it invisible to the caller.

1 files changed, 35 insertions, 37 deletions

diff --git a/lib/ncrypto_nss.c b/lib/ncrypto_nss.c
index 0c57b0f..ac0818b 100644
--- a/lib/ncrypto_nss.c
+++ b/lib/ncrypto_nss.c
@@ -433,20 +433,18 @@ private_key_create (struct ncr_private_key **key, CK_KEY_TYPE type,
   return CKR_GENERAL_ERROR;
 }
 
-/* The caller is responsible for freeing the arena and der_info. */
 static CK_RV
-private_key_export (struct ncr_private_key *key, SECItem *der_key,
-		    PRArenaPool **arena_ptr, SECItem *der_info)
+private_key_export (struct ncr_private_key *key, PRArenaPool *arena,
+		    SECItem *der_key)
 {
   static const uint8_t wrap_key[32]; /* = { 0, }; */
   static const uint8_t wrap_iv[16]; /* = { 0, }; */
 
   PK11SlotInfo *slot;
-  SECItem key_item, iv_item, wrapped_item;
+  SECItem key_item, iv_item, wrapped_item, der_info;
   PK11SymKey *wrapping_key;
   PK11Context *ctx;
   int der_info_len;
-  PRArenaPool *arena;
   struct private_key_info der_output;
   SECStatus ss;
   CK_RV res;
@@ -500,54 +498,44 @@ private_key_export (struct ncr_private_key *key, SECItem *der_key,
       res = CKR_GENERAL_ERROR;
       goto err_wrapped_item;
     }
-  memset (der_info, 0, sizeof (*der_info));
-  if (SECITEM_AllocItem (NULL, der_info, wrapped_item.len) == NULL)
+  memset (&der_info, 0, sizeof (der_info));
+  if (SECITEM_AllocItem (arena, &der_info, wrapped_item.len) == NULL)
     {
       PK11_DestroyContext (ctx, PR_TRUE);
       res = CKR_HOST_MEMORY;
       goto err_wrapped_item;
     }
-  if (PK11_CipherOp (ctx, der_info->data, &der_info_len, der_info->len,
+  if (PK11_CipherOp (ctx, der_info.data, &der_info_len, der_info.len,
 		     wrapped_item.data, wrapped_item.len) != SECSuccess)
     {
       PK11_DestroyContext (ctx, PR_TRUE);
       res = CKR_GENERAL_ERROR;
-      goto err_der_info;
+      goto err_wrapped_item;
     }
   /* C_DecryptFinal is only available through this function.  "Nice.". */
-  ss = PK11_DigestFinal (ctx, der_info->data + der_info_len,
-			 &der_info->len, der_info->len - der_info_len);
+  ss = PK11_DigestFinal (ctx, der_info.data + der_info_len,
+			 &der_info.len, der_info.len - der_info_len);
   PK11_DestroyContext (ctx, PR_TRUE);
   if (ss != SECSuccess)
     {
       res = CKR_GENERAL_ERROR;
-      goto err_der_info;
-    }
-  der_info->len += der_info_len;
-
-  /* Ugly... the PLArenaPool type is from NSPR, but NSS implementation accesses
-     memory only initialized through NSS's PORT_* */
-  arena = PORT_NewArena (SEC_ASN1_DEFAULT_ARENA_SIZE);
-  if (arena == NULL)
-    {
-      res = CKR_HOST_MEMORY;
-      goto err_der_info;
+      goto err_wrapped_item;
     }
+  der_info.len += der_info_len;
 
   /* "type" is accessed by the decoder for ASN1_INTEGER */
   der_output.version.type = siUnsignedInteger;
   if (SEC_QuickDERDecodeItem (arena, &der_output,
-			      private_key_info_asn1_template, der_info)
+			      private_key_info_asn1_template, &der_info)
       != SECSuccess)
     {
       res = CKR_GENERAL_ERROR;
-      goto err_arena;
+      goto err_wrapped_item;
     }
 
   /* Should we validate the version and algorithm ID here? */
 
   *der_key = der_output.private_key;
-  *arena_ptr = arena;
 
   SECITEM_ZfreeItem (&wrapped_item, PR_FALSE);
   PK11_FreeSymKey (wrapping_key);
@@ -555,10 +543,6 @@ private_key_export (struct ncr_private_key *key, SECItem *der_key,
 
   return CKR_OK;
 
- err_arena:
-  PORT_FreeArena (arena, PR_TRUE);
- err_der_info:
-  SECITEM_ZfreeItem (der_info, PR_FALSE);
  err_wrapped_item:
   SECITEM_ZfreeItem (&wrapped_item, PR_FALSE);
  err_wrapping_key:
@@ -611,14 +595,24 @@ ncr_private_key_export (struct ncr_private_key *key, void *dest,
 			size_t *dest_size_ptr)
 {
   PRArenaPool *arena;
-  SECItem der_info, der_key;
+  SECItem der_key;
   CK_RV res;
 
+  res = ensure_ncr_is_open ();
+  if (res != CKR_OK)
+    return res;
+
   g_return_val_if_fail (dest_size_ptr != NULL, CKR_ARGUMENTS_BAD);
 
-  res = private_key_export (key, &der_key, &arena, &der_info);
+  /* Ugly... the PLArenaPool type is from NSPR, but NSS implementation accesses
+     memory only initialized through NSS's PORT_* */
+  arena = PORT_NewArena (SEC_ASN1_DEFAULT_ARENA_SIZE);
+  if (arena == NULL)
+    return CKR_HOST_MEMORY;
+
+  res = private_key_export (key, arena, &der_key);
   if (res != CKR_OK)
-    return res;
+    goto end;
 
   if (dest == NULL)
     {
@@ -639,7 +633,6 @@ ncr_private_key_export (struct ncr_private_key *key, void *dest,
 
  end:
   PORT_FreeArena (arena, PR_TRUE);
-  SECITEM_ZfreeItem (&der_info, PR_FALSE);
   return res;
 }
 
@@ -879,7 +872,7 @@ ncr_private_key_export_rsa (struct ncr_private_key *key, void *modulus,
 {
   struct rsa_private_key der_output;
   PRArenaPool *arena;
-  SECItem der_info, der_key;
+  SECItem der_key;
   CK_RV res;
 
   /* This works in C because "INT" is expanded only at the point where ALL_INTS
@@ -902,9 +895,15 @@ ncr_private_key_export_rsa (struct ncr_private_key *key, void *modulus,
   ALL_INTS;
 #undef INT
 
-  res = private_key_export (key, &der_key, &arena, &der_info);
+  /* Ugly... the PLArenaPool type is from NSPR, but NSS implementation accesses
+     memory only initialized through NSS's PORT_* */
+  arena = PORT_NewArena (SEC_ASN1_DEFAULT_ARENA_SIZE);
+  if (arena == NULL)
+    return CKR_HOST_MEMORY;
+
+  res = private_key_export (key, arena, &der_key);
   if (res != CKR_OK)
-    return res;
+    goto end;
 
   /* Setting type to siUnsignedInteger requests removal of leading zeroes. */
   der_output.version.type = siUnsignedInteger;
@@ -948,7 +947,6 @@ ncr_private_key_export_rsa (struct ncr_private_key *key, void *modulus,
 
  end:
   PORT_FreeArena (arena, PR_TRUE);
-  SECITEM_ZfreeItem (&der_info, PR_FALSE);
   return res;
 #undef DO_INTS
 }