Page MenuHome GnuPG

Smartcard PIN stays in clear in memory
Closed, ResolvedPublic

Description

assuan_release function called from agent_reset_daemon in agent/command.c at line 4151 is not run on primary_ctx variable (primary_scd_ctx for LTS version) and thus the outbound buffer containing the PIN stays in clear in memory. The content the buffer stays in memory even after the smartcard is removed physically.

To reproduce the problem on latest GPG version (after the PIN of the dongle has been entered once):

bash
gdp gpg-agent
(gdb) attach <PID>
Attaching to program: /usr/local/bin/gpg-agent, process <PID>
(gdb) b agent_cache_housekeeping
Breakpoint 1 at 0x55571151c400: file cache.c, line 278.
(gdb) c
Continuing.

Thread 1 "gpg-agent" hit Breakpoint 1, agent_cache_housekeeping () at cache.c:278
278	  if (DBG_CACHE)
(gdb) p daemon_global[DAEMON_SCD].primary_ctx->outbound.data.line
$9 = "D 13371337", '\000' <repeats 82 times>, "\n", '\000' <repeats 908 times>

For LTS version:

bash
p primary_scd_ctx.outbound->data->line

This have been tested with a Yubikey 4 dongle.

Details

Version
2.3.6, 2.2.35

Event Timeline

werner triaged this task as High priority.May 13 2022, 2:40 PM
werner added projects: gnupg (gpg22), scd, pinentry.

AFAICS, we need to implement a new Assuan flag and wipe the data passed to the callback after the callback returned.

For this particular issue of assuan_inquire, if it's needed, the point we should fix is:

diff --git a/src/assuan-inquire.c b/src/assuan-inquire.c
index fa227a6..1cad94f 100644
--- a/src/assuan-inquire.c
+++ b/src/assuan-inquire.c
@@ -254,7 +254,10 @@ assuan_inquire (assuan_context_t ctx, const char *keyword,
 
  out:
   if (!nodataexpected)
-    free_membuf (ctx, &mb);
+    {
+      memset (mb.buf, 0, mb.len);
+      free_membuf (ctx, &mb);
+    }
   ctx->in_inquire = 0;
   return rc;
 }
diff --git a/src/client.c b/src/client.c
index 8b357e6..fb13071 100644
--- a/src/client.c
+++ b/src/client.c
@@ -290,6 +290,7 @@ assuan_transact (assuan_context_t ctx,
       else
         {
           rc = inquire_cb (inquire_cb_arg, line);
+          linelen = ctx->outbound.data.linelen + 1;
           if (!rc)
             rc = assuan_send_data (ctx, NULL, 0); /* flush and send END */
           else
@@ -301,6 +302,7 @@ assuan_transact (assuan_context_t ctx,
               assuan_send_data (ctx, NULL, 1);
               _assuan_read_from_server (ctx, &response, &off, 0);
             }
+          memset (ctx->outbound.data.line, 0, linelen);
           if (!rc)
             goto again;
         }

When new flag is added, these memset-s can be skipped.

Revised version with new flag ASSUAN_CLEAR_INQUIRY_DATA.

diff --git a/src/assuan-defs.h b/src/assuan-defs.h
index 1d1617f..2c9aef9 100644
--- a/src/assuan-defs.h
+++ b/src/assuan-defs.h
@@ -96,6 +96,7 @@ struct assuan_context_s
     unsigned int convey_comments : 1;
     unsigned int no_logging : 1;
     unsigned int force_close : 1;
+    unsigned int clear_inquiry_data : 1;
     /* From here, it's internal flag.  */
     unsigned int is_socket : 1;
   } flags;
diff --git a/src/assuan-inquire.c b/src/assuan-inquire.c
index fa227a6..18dda7a 100644
--- a/src/assuan-inquire.c
+++ b/src/assuan-inquire.c
@@ -254,7 +254,13 @@ assuan_inquire (assuan_context_t ctx, const char *keyword,
 
  out:
   if (!nodataexpected)
-    free_membuf (ctx, &mb);
+    {
+      if (ctx->flags.clear_inquiry_data)
+        memset (mb.buf, 0, mb.len);
+      free_membuf (ctx, &mb);
+    }
+  if (ctx->flags.clear_inquiry_data)
+    memset (ctx->inbound.line, 0, LINELENGTH);
   ctx->in_inquire = 0;
   return rc;
 }
diff --git a/src/assuan.h.in b/src/assuan.h.in
index e993926..5540cdb 100644
--- a/src/assuan.h.in
+++ b/src/assuan.h.in
@@ -189,6 +189,9 @@ typedef unsigned int assuan_flag_t;
 /* This flag forces a connection close.  */
 #define ASSUAN_FORCE_CLOSE 6
 
+/* This flag forces wiping out data after inquiry.  */
+#define ASSUAN_CLEAR_INQUIRY_DATA 7
+
 
 /* For context CTX, set the flag FLAG to VALUE.  Values for flags
  * are usually 1 or 0 but certain flags might allow for other values;
diff --git a/src/client.c b/src/client.c
index 8b357e6..1c3b76b 100644
--- a/src/client.c
+++ b/src/client.c
@@ -301,6 +301,8 @@ assuan_transact (assuan_context_t ctx,
               assuan_send_data (ctx, NULL, 1);
               _assuan_read_from_server (ctx, &response, &off, 0);
             }
+          if (ctx->flags.clear_inquiry_data)
+            memset (ctx->outbound.data.line, 0, LINELENGTH);
           if (!rc)
             goto again;
         }
diff --git a/src/context.c b/src/context.c
index 82166bb..ccef0e1 100644
--- a/src/context.c
+++ b/src/context.c
@@ -93,6 +93,10 @@ assuan_set_flag (assuan_context_t ctx, assuan_flag_t flag, int value)
     case ASSUAN_FORCE_CLOSE:
       ctx->flags.force_close = 1;
       break;
+
+    case ASSUAN_CLEAR_INQUIRY_DATA:
+      ctx->flags.clear_inquiry_data = 1;
+      break;
     }
 }
 
@@ -133,6 +137,10 @@ assuan_get_flag (assuan_context_t ctx, assuan_flag_t flag)
     case ASSUAN_FORCE_CLOSE:
       res = ctx->flags.force_close;
       break;
+
+    case ASSUAN_CLEAR_INQUIRY_DATA:
+      res = ctx->flags.clear_inquiry_data;
+      break;
     }
 
   return TRACE_SUC1 ("flag_value=%i", res);

In this revised version, inbound buffer is also cleared.
When clearing the outbound buffer, it is with LINELENGTH as it may be used already with longer data before the last use of the buffer.

After this patch is applied, gpg-agent should be needed to use ASSUAN_CLEAR_INQUIRY_DATA.

Or, it would be good for client side (in this case, gpg-agent) to specify the flag in the inquiry callback, that is, it's a kind of transient flag for a single transaction.

We can also stretch the semantics of inquiry callback; When it returns GPG_ERR_TRUE, it means, "please wipe the outbound buffer after sending".

^-- I withdraw the solution (with error value) above.

Perhaps, no new flag would be needed, but, we assume that "ASSUAN_CONFIDENTIAL" flag should be used in the inqury (both of client side and server side).

Then, following patch is the solution.

diff --git a/src/assuan-defs.h b/src/assuan-defs.h
index 1d1617f..5081c10 100644
--- a/src/assuan-defs.h
+++ b/src/assuan-defs.h
@@ -98,6 +98,8 @@ struct assuan_context_s
     unsigned int force_close : 1;
     /* From here, it's internal flag.  */
     unsigned int is_socket : 1;
+    unsigned int in_inquiry : 1; /* Client: inquire callback is active   */
+    unsigned int confidential_inquiry : 1;
   } flags;
 
   /* If set, this is called right before logging an I/O line.  */
diff --git a/src/assuan-inquire.c b/src/assuan-inquire.c
index fa227a6..40679b3 100644
--- a/src/assuan-inquire.c
+++ b/src/assuan-inquire.c
@@ -254,7 +254,13 @@ assuan_inquire (assuan_context_t ctx, const char *keyword,
 
  out:
   if (!nodataexpected)
-    free_membuf (ctx, &mb);
+    {
+      if (ctx->flags.confidential)
+        memset (mb.buf, 0, mb.len);
+      free_membuf (ctx, &mb);
+    }
+  if (ctx->flags.confidential)
+    memset (ctx->inbound.line, 0, LINELENGTH);
   ctx->in_inquire = 0;
   return rc;
 }
diff --git a/src/client.c b/src/client.c
index 8b357e6..69ba16d 100644
--- a/src/client.c
+++ b/src/client.c
@@ -289,6 +289,9 @@ assuan_transact (assuan_context_t ctx,
         }
       else
         {
+          ctx->flags.confidential_inquiry = 0;
+          ctx->flags.in_inquiry = 1;
+
           rc = inquire_cb (inquire_cb_arg, line);
           if (!rc)
             rc = assuan_send_data (ctx, NULL, 0); /* flush and send END */
@@ -301,6 +304,12 @@ assuan_transact (assuan_context_t ctx,
               assuan_send_data (ctx, NULL, 1);
               _assuan_read_from_server (ctx, &response, &off, 0);
             }
+          if (ctx->flags.confidential_inquiry)
+            memset (ctx->outbound.data.line, 0, LINELENGTH);
+
+          ctx->flags.confidential_inquiry = 0;
+          ctx->flags.in_inquiry = 0;
+
           if (!rc)
             goto again;
         }
diff --git a/src/context.c b/src/context.c
index 82166bb..b01e147 100644
--- a/src/context.c
+++ b/src/context.c
@@ -76,6 +76,8 @@ assuan_set_flag (assuan_context_t ctx, assuan_flag_t flag, int value)
 
     case ASSUAN_CONFIDENTIAL:
       ctx->flags.confidential = value;
+      if (ctx->flags.in_inquiry && value);
+          ctx->flags.confidential_inquiry = value;
       break;
 
     case ASSUAN_NO_FIXSIGNALS:
gniibe added a project: Restricted Project.May 25 2022, 9:39 AM

Pushed the solution which doesn't require new flag for libassuan.

For GnuPG 2.2, this should be backported:
rG052f58422dca: agent,scd: Make sure to set CONFIDENTIAL flag in Assuan.

Then, linking with new libassuan, those memory are wiped out after use.

gniibe removed a project: Restricted Project.Jul 27 2022, 2:39 AM

New release of libassuan is expected to make sure it's cleared off.