ssh: Fix allocation of pinentry buffer

[Werner Koch]

On Thu,  1 Oct 2015 06:27, gniibe at fsij.org said:

> Thank you for reviewing.  Actually, the code around pinentry
> invocation has more bugs, and more fixes are expected.

AFAIC, these bugs are due to the avoidance of a second malloc.  This was
my fault initially.  Later others copied that code for use at other
places.  Interestingly agent/divert-scd does it correctly.

>     fixed size allocation and fixed size communication

The pin_entry_info_s is allocated in secure memory thus when piossible
it should be limited in size.

What about this basic fix?  If there are other problem they can be
applied on top of this.


>From 3e71ed12acbb54ec5cf5d347f62b57bb8a0ad233 Mon Sep 17 00:00:00 2001
From: Werner Koch <wk at gnupg.org>
Date: Thu, 1 Oct 2015 13:21:25 +0200
Subject: [PATCH] agent: Fix alignment problem with the second passphrase
 struct.

* agent/genkey.c (agent_ask_new_passphrase): Use a separate malloc for
PI2.  Check return value of the malloc function.
* agent/command-ssh.c (ssh_identity_register): Use a separate malloc
for PI2.  Wipe PI2.
--

For whatever stupid reasons I once allocated only one memory area and
split that into PI and PI2.  This is actually a common pattern with
malloc but here we used a made up object size and do not take the
extra alignment required into account.  One of these not yet hit by
a (sig)bus PC/VAX hacker bugs.

Instead of trying to fix the alignment, it is better to use a second
calloc for the second struct.

GnuPG-bug-id: 2112
Signed-off-by: Werner Koch <wk at gnupg.org>
---
 agent/command-ssh.c | 15 ++++++++++++---
 agent/genkey.c      | 13 +++++++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/agent/command-ssh.c b/agent/command-ssh.c
index 8be1255..0aa0098 100644
--- a/agent/command-ssh.c
+++ b/agent/command-ssh.c
@@ -3070,7 +3070,8 @@ ssh_identity_register (ctrl_t ctrl, ssh_key_type_spec_t *spec,
   char *comment = NULL;
   char *key_fpr = NULL;
   const char *initial_errtext = NULL;
-  struct pin_entry_info_s *pi = NULL, *pi2;
+  struct pin_entry_info_s *pi = NULL;
+  struct pin_entry_info_s *pi2 = NULL;
 
   err = ssh_key_grip (key, key_grip_raw);
   if (err)
@@ -3101,13 +3102,18 @@ ssh_identity_register (ctrl_t ctrl, ssh_key_type_spec_t *spec,
       goto out;
     }
 
-  pi = gcry_calloc_secure (2, sizeof (*pi) + MAX_PASSPHRASE_LEN + 1);
+  pi = gcry_calloc_secure (1, sizeof (*pi) + MAX_PASSPHRASE_LEN + 1);
   if (!pi)
     {
       err = gpg_error_from_syserror ();
       goto out;
     }
-  pi2 = pi + (sizeof *pi + MAX_PASSPHRASE_LEN + 1);
+  pi2 = gcry_calloc_secure (1, sizeof (*pi2) + MAX_PASSPHRASE_LEN + 1);
+  if (!pi2)
+    {
+      err = gpg_error_from_syserror ();
+      goto out;
+    }
   pi->max_length = MAX_PASSPHRASE_LEN + 1;
   pi->max_tries = 1;
   pi->with_repeat = 1;
@@ -3155,6 +3161,9 @@ ssh_identity_register (ctrl_t ctrl, ssh_key_type_spec_t *spec,
 
 
  out:
+  if (pi2 && pi2->max_length)
+    wipememory (pi2->pin, pi2->max_length);
+  xfree (pi2);
   if (pi && pi->max_length)
     wipememory (pi->pin, pi->max_length);
   xfree (pi);
diff --git a/agent/genkey.c b/agent/genkey.c
index 13858ca..e8195c2 100644
--- a/agent/genkey.c
+++ b/agent/genkey.c
@@ -374,8 +374,16 @@ agent_ask_new_passphrase (ctrl_t ctrl, const char *prompt,
 	return err;
     }
 
-  pi = gcry_calloc_secure (2, sizeof (*pi) + MAX_PASSPHRASE_LEN + 1);
-  pi2 = pi + (sizeof *pi + MAX_PASSPHRASE_LEN + 1);
+  pi = gcry_calloc_secure (1, sizeof (*pi) + MAX_PASSPHRASE_LEN + 1);
+  if (!pi)
+    return gpg_error_from_syserror ();
+  pi2 = gcry_calloc_secure (1, sizeof (*pi2) + MAX_PASSPHRASE_LEN + 1);
+  if (!pi2)
+    {
+      err = gpg_error_from_syserror ();
+      xfree (pi2);
+      return err;
+    }
   pi->max_length = MAX_PASSPHRASE_LEN + 1;
   pi->max_tries = 3;
   pi->with_qualitybar = 1;
@@ -422,6 +430,7 @@ agent_ask_new_passphrase (ctrl_t ctrl, const char *prompt,
     }
 
   xfree (initial_errtext);
+  xfree (pi2);
   xfree (pi);
   return err;
 }
-- 
2.1.4



Shalom-Salam,

   Werner

-- 
Die Gedanken sind frei.  Ausnahmen regelt ein Bundesgesetz.