Obscure the cached passphrases.

agent/ChangeLog

@@ -1,3 +1,21 @@
+2010-09-02  Werner Koch  <wk@g10code.com>
+
+	* cache.c (new_data): Change arg and callers to use a string and
+	explicity return an error code.  We never used raw binary data and
+	thus it is easier to use a string.  Adjust callers.
+	(initialize_module_cache, deinitialize_module_cache): New.
+	(new_data): Encrypt the cached data.
+	(struct cache_item_s): Remove field LOCKCOUNT.  Change all users
+	accordingly.
+	(agent_unlock_cache_entry): Remove.
+	(agent_get_cache): Return an allocated string and remove CACHE_ID.
+	* genkey.c (agent_genkey): Remove cache marker stuff.
+	* findkey.c (unprotect): Ditto.
+	* cvt-openpgp.c (convert_openpgp): Ditto.
+	* command.c (cmd_get_passphrase): Ditto.
+	* gpg-agent.c (main, cleanup): Initialize and deinitialize the
+	cache module.
+
 2010-09-01  Werner Koch  <wk@g10code.com>
 
 	* call-pinentry.c (start_pinentry): Disable pinentry logging.

agent/agent.h

@@ -264,12 +264,12 @@ void agent_popup_message_stop (ctrl_t ctrl);
 
 
 /*-- cache.c --*/
+void initialize_module_cache (void);
+void deinitialize_module_cache (void);
 void agent_flush_cache (void);
 int agent_put_cache (const char *key, cache_mode_t cache_mode,
                      const char *data, int ttl);
-const char *agent_get_cache (const char *key, cache_mode_t cache_mode,
+char *agent_get_cache (const char *key, cache_mode_t cache_mode);
-                             void **cache_id);
-void agent_unlock_cache_entry (void **cache_id);
 
 
 /*-- pksign.c --*/

agent/cache.c

@@ -24,13 +24,31 @@
 #include <string.h>
 #include <time.h>
 #include <assert.h>
+#include <pth.h>
 
 #include "agent.h"
 
+/* The size of the encryption key in bytes.  */
+#define ENCRYPTION_KEYSIZE (128/8)
+
+/* A mutex used to protect the encryption.  This is required because
+   we use one context to do all encryption and decryption.  */
+static pth_mutex_t encryption_lock;
+/* The encryption context.  This is the only place where the
+   encryption key for all cached entries is available.  It would nice
+   to keep this (or just the key) in some hardware device, for example
+   a TPM.  Libgcrypt could be extended to provide such a service.
+   With the current scheme it is easy to retrieve the cached entries
+   if access to Libgcrypt's memory is available.  The encryption
+   merely avoids grepping for clear texts in the memory.  Nevertheless
+   the encryption provides the necessary infrastructure to make it
+   more secure.  */
+static gcry_cipher_hd_t encryption_handle;
+
+
 struct secret_data_s {
-  int  totallen; /* this includes the padding */
+  int  totallen; /* This includes the padding and space for AESWRAP. */
-  int  datalen;  /* actual data length */
+  char data[1];  /* A string.  */
-  char data[1];
 };
 
 typedef struct cache_item_s *ITEM;
@@ -39,47 +57,116 @@ struct cache_item_s {
   time_t created;
   time_t accessed;
   int ttl;  /* max. lifetime given in seconds, -1 one means infinite */
-  int lockcount;
   struct secret_data_s *pw;
   cache_mode_t cache_mode;
   char key[1];
 };
 
-
+/* The cache himself.  */
 static ITEM thecache;
 
 
+/* This function must be called once to initialize this module. It
+   has to be done before a second thread is spawned.  */
+void
+initialize_module_cache (void)
+{
+  static int initialized;
+  gpg_error_t err;
+  void *key;
+
+  if (!pth_mutex_init (&encryption_lock))
+    err = gpg_error_from_syserror ();
+  else
+    err = gcry_cipher_open (&encryption_handle, GCRY_CIPHER_AES128,
+                            GCRY_CIPHER_MODE_AESWRAP, GCRY_CIPHER_SECURE);
+  if (!err)
+    {
+      key = gcry_random_bytes (ENCRYPTION_KEYSIZE, GCRY_STRONG_RANDOM);
+      if (!key)
+        err = gpg_error_from_syserror ();
+      else
+        {
+          err = gcry_cipher_setkey (encryption_handle, key, ENCRYPTION_KEYSIZE);
+          xfree (key);
+        }
+    }
+  if (err)
+    log_fatal ("error initializing cache encryption context: %s\n",
+             gpg_strerror (err));
+  initialized = 1;
+}
+
+
+void
+deinitialize_module_cache (void)
+{
+  gcry_cipher_close (encryption_handle);
+  encryption_handle = NULL;
+}
+
+
 static void
 release_data (struct secret_data_s *data)
 {
    xfree (data);
 }
 
-static struct secret_data_s *
+static gpg_error_t
-new_data (const void *data, size_t length)
+new_data (const char *string, struct secret_data_s **r_data)
 {
-  struct secret_data_s *d;
+  gpg_error_t err;
+  struct secret_data_s *d, *d_enc;
+  size_t length;
   int total;
   
-  /* we pad the data to 32 bytes so that it get more complicated
+  *r_data = NULL;
+
+  if (!encryption_handle)
+    return gpg_error (GPG_ERR_NOT_INITIALIZED);
+
+  length = strlen (string) + 1;
+
+  /* We pad the data to 32 bytes so that it get more complicated
      finding something out by watching allocation patterns.  This is
-     usally not possible but we better assume nothing about our
+     usally not possible but we better assume nothing about our secure
-     secure storage provider*/
+     storage provider.  To support the AESWRAP mode we need to add 8
-  total = length + 32 - (length % 32);
+     extra bytes as well. */
+  total = (length + 8) + 32 - ((length+8) % 32);
 
-  d = gcry_malloc_secure (sizeof *d + total - 1);
+  d = xtrymalloc_secure (sizeof *d + total - 1);
-  if (d)
+  if (!d)
+    return gpg_error_from_syserror ();
+  memcpy (d->data, string, length);
+
+  d_enc = xtrymalloc (sizeof *d_enc + total - 1);
+  if (!d_enc)
     {
-      d->totallen = total;
+      err = gpg_error_from_syserror ();
-      d->datalen  = length;
+      xfree (d);
-      memcpy (d->data, data, length);
+      return err;
     }
-  return d;
+
+  d_enc->totallen = total;
+  if (!pth_mutex_acquire (&encryption_lock, 0, NULL))
+    log_fatal ("failed to acquire cache encryption mutex\n");
+  err = gcry_cipher_encrypt (encryption_handle, d_enc->data, total,
+                             d->data, total - 8);
+  xfree (d);
+  if (!pth_mutex_release (&encryption_lock))
+    log_fatal ("failed to release cache encryption mutex\n");
+  if (err)
+    {
+      xfree (d_enc);
+      return err;
+    }
+  *r_data = d_enc;
+  return 0;
 }
 
 
 
-/* check whether there are items to expire */
+/* Check whether there are items to expire.  */
 static void
 housekeeping (void)
 {
@@ -89,8 +176,7 @@ housekeeping (void)
   /* First expire the actual data */
   for (r=thecache; r; r = r->next)
     {
-      if (!r->lockcount && r->pw
+      if (r->pw && r->ttl >= 0 && r->accessed + r->ttl < current)
-	  && r->ttl >= 0 && r->accessed + r->ttl < current)
         {
           if (DBG_CACHE)
             log_debug ("  expired `%s' (%ds after last access)\n",
@@ -112,7 +198,7 @@ housekeeping (void)
         case CACHE_MODE_SSH: maxttl = opt.max_cache_ttl_ssh; break;
         default: maxttl = opt.max_cache_ttl; break;
         }
-      if (!r->lockcount && r->pw && r->created + maxttl < current)
+      if (r->pw && r->created + maxttl < current)
         {
           if (DBG_CACHE)
             log_debug ("  expired `%s' (%lus after creation)\n",
@@ -128,17 +214,6 @@ housekeeping (void)
   for (rprev=NULL, r=thecache; r; )
     {
       if (!r->pw && r->ttl >= 0 && r->accessed + 60*30 < current)
-        {
-          if (r->lockcount)
-            {
-              log_error ("can't remove unused cache entry `%s' (mode %d) due to"
-                         " lockcount=%d\n",
-                         r->key, r->cache_mode, r->lockcount);
-              r->accessed += 60*10; /* next error message in 10 minutes */
-              rprev = r;
-              r = r->next;
-            }
-          else
         {
           ITEM r2 = r->next;
           if (DBG_CACHE)
@@ -151,7 +226,6 @@ housekeeping (void)
             rprev->next = r2;
           r = r2;
         }
-        }
       else
         {
           rprev = r;
@@ -171,7 +245,7 @@ agent_flush_cache (void)
 
   for (r=thecache; r; r = r->next)
     {
-      if (!r->lockcount && r->pw)
+      if (r->pw)
         {
           if (DBG_CACHE)
             log_debug ("  flushing `%s'\n", r->key);
@@ -179,28 +253,22 @@ agent_flush_cache (void)
           r->pw = NULL;
           r->accessed = 0;
         }
-      else if (r->lockcount && r->pw)
-        {
-          if (DBG_CACHE)
-            log_debug ("    marked `%s' for flushing\n", r->key);
-          r->accessed = 0;
-          r->ttl = 0;
-        }
     }
 }
 
 
 
-/* Store DATA of length DATALEN in the cache under KEY and mark it
+/* Store the string DATA in the cache under KEY and mark it with a
-   with a maximum lifetime of TTL seconds.  If there is already data
+   maximum lifetime of TTL seconds.  If there is already data under
-   under this key, it will be replaced.  Using a DATA of NULL deletes
+   this key, it will be replaced.  Using a DATA of NULL deletes the
-   the entry.  A TTL of 0 is replaced by the default TTL and a TTL of
+   entry.  A TTL of 0 is replaced by the default TTL and a TTL of -1
-   -1 set infinite timeout.  CACHE_MODE is stored with the cache entry
+   set infinite timeout.  CACHE_MODE is stored with the cache entry
    and used to select different timeouts.  */
 int
 agent_put_cache (const char *key, cache_mode_t cache_mode,
                  const char *data, int ttl)
 {
+  gpg_error_t err = 0;
   ITEM r;
 
   if (DBG_CACHE)
@@ -221,15 +289,14 @@ agent_put_cache (const char *key, cache_mode_t cache_mode,
 
   for (r=thecache; r; r = r->next)
     {
-      if (!r->lockcount
+      if (((cache_mode != CACHE_MODE_USER
-          && ((cache_mode != CACHE_MODE_USER
             && cache_mode != CACHE_MODE_NONCE)
            || r->cache_mode == cache_mode)
           && !strcmp (r->key, key))
         break;
     }
-  if (r)
+  if (r) /* Replace.  */
-    { /* replace */
+    {
       if (r->pw)
         {
           release_data (r->pw);
@@ -240,46 +307,47 @@ agent_put_cache (const char *key, cache_mode_t cache_mode,
           r->created = r->accessed = gnupg_get_time (); 
           r->ttl = ttl;
           r->cache_mode = cache_mode;
-          r->pw = new_data (data, strlen (data)+1);
+          err = new_data (data, &r->pw);
-          if (!r->pw)
+          if (err)
-            log_error ("out of core while allocating new cache item\n");
+            log_error ("error replacing cache item: %s\n", gpg_strerror (err));
         }
     }
-  else if (data)
+  else if (data) /* Insert.  */
-    { /* simply insert */
+    {
       r = xtrycalloc (1, sizeof *r + strlen (key));
       if (!r)
-        log_error ("out of core while allocating new cache control\n");
+        err = gpg_error_from_syserror ();
       else
         {
           strcpy (r->key, key);
           r->created = r->accessed = gnupg_get_time (); 
           r->ttl = ttl;
           r->cache_mode = cache_mode;
-          r->pw = new_data (data, strlen (data)+1);
+          err = new_data (data, &r->pw);
-          if (!r->pw)
+          if (err)
-            {
-              log_error ("out of core while allocating new cache item\n");
             xfree (r);
-            }
           else
             {
               r->next = thecache;
               thecache = r;
             }
         }
+      if (err)
+        log_error ("error inserting cache item: %s\n", gpg_strerror (err));
     }
-  return 0;
+  return err;
 }
 
 
 /* Try to find an item in the cache.  Note that we currently don't
    make use of CACHE_MODE except for CACHE_MODE_NONCE and
    CACHE_MODE_USER.  */
-const char *
+char *
-agent_get_cache (const char *key, cache_mode_t cache_mode, void **cache_id)
+agent_get_cache (const char *key, cache_mode_t cache_mode)
 {
+  gpg_error_t err;
   ITEM r;
+  char *value = NULL;
 
   if (cache_mode == CACHE_MODE_IGNORE)
     return NULL;
@@ -288,28 +356,6 @@ agent_get_cache (const char *key, cache_mode_t cache_mode, void **cache_id)
     log_debug ("agent_get_cache `%s' (mode %d) ...\n", key, cache_mode);
   housekeeping ();
 
-  /* first try to find one with no locks - this is an updated cache
-     entry: We might have entries with a lockcount and without a
-     lockcount. */
-  for (r=thecache; r; r = r->next)
-    {
-      if (!r->lockcount && r->pw
-          && ((cache_mode != CACHE_MODE_USER
-               && cache_mode != CACHE_MODE_NONCE)
-              || r->cache_mode == cache_mode)
-          && !strcmp (r->key, key))
-        {
-          /* put_cache does only put strings into the cache, so we
-             don't need the lengths */
-          r->accessed = gnupg_get_time ();
-          if (DBG_CACHE)
-            log_debug ("... hit\n");
-          r->lockcount++;
-          *cache_id = r;
-          return r->pw->data;
-        }
-    }
-  /* again, but this time get even one with a lockcount set */
   for (r=thecache; r; r = r->next)
     {
       if (r->pw
@@ -320,35 +366,36 @@ agent_get_cache (const char *key, cache_mode_t cache_mode, void **cache_id)
         {
           r->accessed = gnupg_get_time ();
           if (DBG_CACHE)
-            log_debug ("... hit (locked)\n");
+            log_debug ("... hit\n");
-          r->lockcount++;
+          if (r->pw->totallen < 32)
-          *cache_id = r;
+            err = gpg_error (GPG_ERR_INV_LENGTH);
-          return r->pw->data;
+          else if (!encryption_handle)
+            err = gpg_error (GPG_ERR_NOT_INITIALIZED);
+          else if (!(value = xtrymalloc_secure (r->pw->totallen - 8)))
+            err = gpg_error_from_syserror ();
+          else
+            {
+              if (!pth_mutex_acquire (&encryption_lock, 0, NULL))
+                log_fatal ("failed to acquire cache encryption mutex\n");
+              err = gcry_cipher_decrypt (encryption_handle,
+                                         value, r->pw->totallen - 8,
+                                         r->pw->data, r->pw->totallen);
+              if (!pth_mutex_release (&encryption_lock))
+                log_fatal ("failed to release cache encryption mutex\n");
+            }
+          if (err)
+            {
+              xfree (value);
+              value = NULL;
+              log_error ("retrieving cache entry `%s' failed: %s\n",
+                         key, gpg_strerror (err));
+            }
+          return value;
         }
     }
   if (DBG_CACHE)
     log_debug ("... miss\n");
 
-  *cache_id = NULL;
   return NULL;
 }
 
-
-void
-agent_unlock_cache_entry (void **cache_id)
-{
-  ITEM r;
-
-  for (r=thecache; r; r = r->next)
-    {
-      if (r == *cache_id)
-        {
-          if (!r->lockcount)
-            log_error ("trying to unlock non-locked cache entry `%s'\n",
-                       r->key);
-          else
-            r->lockcount--;
-          return;
-        }
-    }
-}

agent/command.c

@@ -1069,12 +1069,11 @@ cmd_get_passphrase (assuan_context_t ctx, char *line)
 {
   ctrl_t ctrl = assuan_get_pointer (ctx);
   int rc;
-  const char *pw;
+  char *pw;
   char *response;
   char *cacheid = NULL, *desc = NULL, *prompt = NULL, *errtext = NULL;
   const char *desc2 = _("Please re-enter this passphrase");
   char *p;
-  void *cache_marker;
   int opt_data, opt_check, opt_no_ask, opt_qualbar;
   int opt_repeat = 0;
   char *repeat_errtext = NULL;
@@ -1135,12 +1134,11 @@ cmd_get_passphrase (assuan_context_t ctx, char *line)
   if (!strcmp (desc, "X"))
     desc = NULL;
 
-  pw = cacheid ? agent_get_cache (cacheid, CACHE_MODE_NORMAL, &cache_marker)
+  pw = cacheid ? agent_get_cache (cacheid, CACHE_MODE_NORMAL) : NULL;
-               : NULL;
   if (pw)
     {
       rc = send_back_passphrase (ctx, opt_data, pw);
-      agent_unlock_cache_entry (&cache_marker);
+      xfree (pw);
     }
   else if (opt_no_ask)
     rc = gpg_error (GPG_ERR_NO_DATA);

agent/cvt-openpgp.c

@@ -766,16 +766,14 @@ convert_openpgp (ctrl_t ctrl, gcry_sexp_t s_pgp,
   err = gpg_error (GPG_ERR_BAD_PASSPHRASE);
   if (cache_nonce)
     {
-      void *cache_marker = NULL;
+      char *cache_value;
-      const char *cache_value;
 
-      cache_value = agent_get_cache (cache_nonce, CACHE_MODE_NONCE,
+      cache_value = agent_get_cache (cache_nonce, CACHE_MODE_NONCE);
-                                     &cache_marker);
       if (cache_value)
         {
           if (strlen (cache_value) < pi->max_length)
             strcpy (pi->pin, cache_value);
-          agent_unlock_cache_entry (&cache_marker);
+          xfree (cache_value);
         }
       if (*pi->pin)
         err = try_do_unprotect_cb (pi);

agent/findkey.c

@@ -291,14 +291,13 @@ unprotect (ctrl_t ctrl, const char *cache_nonce, const char *desc_text,
   /* Initially try to get it using a cache nonce.  */
   if (cache_nonce)
     {
-      void *cache_marker;
+      char *pw;
-      const char *pw;
       
-      pw = agent_get_cache (cache_nonce, CACHE_MODE_NONCE, &cache_marker);
+      pw = agent_get_cache (cache_nonce, CACHE_MODE_NONCE);
       if (pw)
         {
           rc = agent_unprotect (*keybuf, pw, NULL, &result, &resultlen);
-          agent_unlock_cache_entry (&cache_marker);
+          xfree (pw);
           if (!rc)
             {
               xfree (*keybuf);
@@ -312,15 +311,14 @@ unprotect (ctrl_t ctrl, const char *cache_nonce, const char *desc_text,
      unprotect it, we fall back to ask the user */
   if (cache_mode != CACHE_MODE_IGNORE)
     {
-      void *cache_marker;
+      char *pw;
-      const char *pw;
       
     retry:
-      pw = agent_get_cache (hexgrip, cache_mode, &cache_marker);
+      pw = agent_get_cache (hexgrip, cache_mode);
       if (pw)
         {
           rc = agent_unprotect (*keybuf, pw, NULL, &result, &resultlen);
-          agent_unlock_cache_entry (&cache_marker);
+          xfree (pw);
           if (!rc)
             {
               xfree (*keybuf);

agent/genkey.c

@@ -359,7 +359,7 @@ agent_genkey (ctrl_t ctrl, const char *cache_nonce,
               membuf_t *outbuf) 
 {
   gcry_sexp_t s_keyparam, s_key, s_private, s_public;
-  char *passphrase = NULL;
+  char *passphrase;
   int rc;
   size_t len;
   char *buf;
@@ -372,21 +372,7 @@ agent_genkey (ctrl_t ctrl, const char *cache_nonce,
     }
 
   /* Get the passphrase now, cause key generation may take a while. */
-  if (cache_nonce)
+  passphrase = cache_nonce? agent_get_cache (cache_nonce, CACHE_MODE_NONCE):NULL;
-    {
-      void *cache_marker = NULL;
-      const char *cache_value;
-
-      cache_value = agent_get_cache (cache_nonce, CACHE_MODE_NONCE,
-                                     &cache_marker);
-      if (cache_value)
-        {
-          passphrase = xtrymalloc_secure (strlen (cache_value)+1);
-          if (passphrase)
-            strcpy (passphrase, cache_value);
-          agent_unlock_cache_entry (&cache_marker);
-        }
-    }
   if (passphrase)
     rc = 0;
   else

agent/gpg-agent.c

@@ -440,6 +440,7 @@ remove_socket (char *name)
 static void
 cleanup (void)
 {
+  deinitialize_module_cache ();
   remove_socket (socket_name);
   remove_socket (socket_name_ssh);
 }
@@ -842,6 +843,7 @@ main (int argc, char **argv )
       exit (1);
     }
 
+  initialize_module_cache ();
   initialize_module_call_pinentry ();
   initialize_module_call_scd ();
   initialize_module_trustlist ();

common/ChangeLog

@@ -1,3 +1,7 @@
+2010-09-02  Werner Koch  <wk@g10code.com>
+
+	* util.h (GPG_ERR_NOT_INITIALIZED): Define if not defined.
+
 2010-09-01  Marcus Brinkmann  <marcus@g10code.de>
 
 	* estream.c (_es_set_std_fd): Disable debug output.

common/util.h

@@ -30,6 +30,10 @@
 #ifndef GPG_ERR_LIMIT_REACHED
 #define GPG_ERR_LIMIT_REACHED 183
 #endif
+#ifndef GPG_ERR_NOT_INITIALIZED
+#define GPG_ERR_NOT_INITIALIZED 184
+#endif
+
 
 /* Hash function used with libksba. */
 #define HASH_FNC ((void (*)(void *, const void*,size_t))gcry_md_write)