Smartcard PIN stays in clear in memory
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	spelissier
	May 13 2022, 9:34 AM

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):

gdb 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:

p primary_scd_ctx.outbound->data->line

This have been tested with a Yubikey 4 dongle.

Details

Version: 2.3.6, 2.2.35

Revisions and Commits

rG GnuPG
	rGaeee62593ae9 agent,scd: Make sure to set CONFIDENTIAL flag in Assuan.
	rG052f58422dca agent,scd: Make sure to set CONFIDENTIAL flag in Assuan.
rA Assuan
	rA5ac7e6ba19fa server,client: Wipe the outbound buffer when CONFIDENTIAL.
	rAaac300389210 client: Wipe the inbound buffer when CONFIDENTIAL.
	rA4bfcd8a0f6f3 server: Wipe out the memory used by assuan_inquire if CONFIDENTIAL.
	rA2a5550b7a13c client: Handle inquiry from server with CONFIDENTIAL.
	rAfd1ac5cdf804 client: Wipe the inbound buffer when CONFIDENTIAL.
	rA89e8f265e377 server,client: Wipe the outbound buffer when CONFIDENTIAL.
	rAd812e28af637 server: Wipe out the memory used by assuan_inquire if CONFIDENTIAL.
rP Pinentry
	rPc405add93a77 Mark with ASSUAN_CONFIDENTIAL flag.

Related Objects

Mentioned In: T5566: Release Pinentry 1.2.x
T6542: Release libassuan 2.5.6
T5949: Release GnuPG 2.2.36
Mentioned Here: rG052f58422dca: agent,scd: Make sure to set CONFIDENTIAL flag in Assuan.

Event Timeline

spelissier created this task.May 13 2022, 9:34 AM

• 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.

• werner added a project: libassuan.May 18 2022, 9:14 AM

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: