ssh: Fix allocation of pinentry buffer
NIIBE Yutaka
gniibe at fsij.org
Thu Oct 1 06:27:23 CEST 2015
On 09/30/2015 05:48 PM, Neal H. Walfield wrote:
> I think your patch is correct, but I'd prefer a change that increases
> robustness rather than one that preserves this fragility. Second,
> this bug is also present in agent/genkey.c. Can you fix it there as
> well?
Thank you for reviewing. Actually, the code around pinentry
invocation has more bugs, and more fixes are expected.
So, I was considering a step-by-step approach would be better and
cherry-picking some of patches into 2.0. I should have expressed
that. (Well, my patch yesterday include change of agent/genkey.c.)
It's complicated because there are multiple communications, and some
arbitrary numbers. I think that you've changed a number like 100 by
MAX_PASSPHRASE_LEN, but there are still numbers like 90 and 100.
Here is a picture:
loopback
gpg <-------> PIN
gpg-agent <---> scdaemon
pinentry <-->
passphrase / PIN
Passphrase for local keys and PIN for card are different things, and
it would make sense to have different values for each. But, the
length limitation would be ok to be same.
Following is current my working patch around passphrase/pin inputs.
There are semantic inconsistency (of buffer length or of passphrase
length), off-by-one errors, allocation assumption mistake
(caller/callee allocation responsibility), and inconsistent string
handling (sending all buffer or only string).
Following patch is not intended to commit directly. It's to show
more to be done.
Perhaps, it would be good to do:
fixed size allocation and fixed size communication
scdaemon can limit the length of PIN to lower value than the one
for passphrase
diff --git a/agent/agent.h b/agent/agent.h
index b3e8470..0057c03 100644
--- a/agent/agent.h
+++ b/agent/agent.h
@@ -260,10 +260,8 @@ struct pin_entry_info_s
int (*check_cb)(struct pin_entry_info_s *); /* CB used to check the PIN */
void *check_cb_arg; /* optional argument which might be of use in the CB */
const char *cb_errtext; /* used by the cb to display a specific error */
- size_t max_length; /* Allocated length of the buffer PIN. */
- char pin[1]; /* The buffer to hold the PIN or passphrase.
- It's actual allocated length is given by
- MAX_LENGTH (above). */
+ /* The buffer to hold the PIN or passphrase. */
+ char pin[MAX_PASSPHRASE_LEN+1];
};
@@ -350,8 +348,7 @@ void bump_key_eventcounter (void);
void bump_card_eventcounter (void);
void start_command_handler (ctrl_t, gnupg_fd_t, gnupg_fd_t);
gpg_error_t pinentry_loopback (ctrl_t, const char *keyword,
- unsigned char **buffer, size_t *size,
- size_t max_length);
+ unsigned char **buffer, size_t *size);
#ifdef HAVE_W32_SYSTEM
int serve_mmapped_ssh_request (ctrl_t ctrl,
diff --git a/agent/call-pinentry.c b/agent/call-pinentry.c
index 9845a03..c16c874 100644
--- a/agent/call-pinentry.c
+++ b/agent/call-pinentry.c
@@ -829,8 +829,7 @@ agent_askpin (ctrl_t ctrl,
size_t size;
*pininfo->pin = 0; /* Reset the PIN. */
- rc = pinentry_loopback(ctrl, "PASSPHRASE", &passphrase, &size,
- pininfo->max_length);
+ rc = pinentry_loopback(ctrl, "PASSPHRASE", &passphrase, &size);
if (rc)
return rc;
@@ -848,7 +847,7 @@ agent_askpin (ctrl_t ctrl,
return gpg_error(GPG_ERR_NO_PIN_ENTRY);
}
- if (!pininfo || pininfo->max_length < 1)
+ if (!pininfo)
return gpg_error (GPG_ERR_INV_VALUE);
if (!desc_text && pininfo->min_digits)
desc_text = L_("Please enter your PIN, so that the secret key "
@@ -933,7 +932,7 @@ agent_askpin (ctrl_t ctrl,
for (;pininfo->failed_tries < pininfo->max_tries; pininfo->failed_tries++)
{
memset (&parm, 0, sizeof parm);
- parm.size = pininfo->max_length;
+ parm.size = MAX_PASSPHRASE_LEN;
*pininfo->pin = 0; /* Reset the PIN. */
parm.buffer = (unsigned char*)pininfo->pin;
@@ -1062,10 +1061,9 @@ agent_get_passphrase (ctrl_t ctrl,
if (ctrl->pinentry_mode == PINENTRY_MODE_LOOPBACK)
{
size_t size;
- size_t len = ASSUAN_LINELENGTH/2;
- unsigned char *buffer = gcry_malloc_secure (len);
+ unsigned char *buffer;
- rc = pinentry_loopback(ctrl, "PASSPHRASE", &buffer, &size, len);
+ rc = pinentry_loopback(ctrl, "PASSPHRASE", &buffer, &size);
if (rc)
xfree(buffer);
else
diff --git a/agent/call-scd.c b/agent/call-scd.c
index 6cd5825..bb46e3f 100644
--- a/agent/call-scd.c
+++ b/agent/call-scd.c
@@ -711,7 +711,7 @@ inq_needpin (void *opaque, const char *line)
if ((s = has_leading_keyword (line, "NEEDPIN")))
{
line = s;
- pinlen = 90;
+ pinlen = MAX_PASSPHRASE_LEN+1;
pin = gcry_malloc_secure (pinlen);
if (!pin)
return out_of_core ();
diff --git a/agent/command-ssh.c b/agent/command-ssh.c
index 8be1255..4469e46 100644
--- a/agent/command-ssh.c
+++ b/agent/command-ssh.c
@@ -3101,17 +3101,15 @@ 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 (2, sizeof (*pi));
if (!pi)
{
err = gpg_error_from_syserror ();
goto out;
}
- pi2 = pi + (sizeof *pi + MAX_PASSPHRASE_LEN + 1);
- pi->max_length = MAX_PASSPHRASE_LEN + 1;
+ pi2 = pi + 1;
pi->max_tries = 1;
pi->with_repeat = 1;
- pi2->max_length = MAX_PASSPHRASE_LEN + 1;
pi2->max_tries = 1;
pi2->check_cb = reenter_compare_cb;
pi2->check_cb_arg = pi->pin;
@@ -3155,8 +3153,8 @@ ssh_identity_register (ctrl_t ctrl, ssh_key_type_spec_t *spec,
out:
- if (pi && pi->max_length)
- wipememory (pi->pin, pi->max_length);
+ if (pi)
+ wipememory (pi->pin, MAX_PASSPHRASE_LEN+1);
xfree (pi);
xfree (buffer);
xfree (comment);
diff --git a/agent/command.c b/agent/command.c
index f09a2ff..63d2d21 100644
--- a/agent/command.c
+++ b/agent/command.c
@@ -3250,18 +3250,17 @@ start_command_handler (ctrl_t ctrl, gnupg_fd_t listen_fd, gnupg_fd_t fd)
parameters on to the client. */
gpg_error_t
pinentry_loopback(ctrl_t ctrl, const char *keyword,
- unsigned char **buffer, size_t *size,
- size_t max_length)
+ unsigned char **buffer, size_t *size)
{
gpg_error_t rc;
assuan_context_t ctx = ctrl->server_local->assuan_ctx;
- rc = print_assuan_status (ctx, "INQUIRE_MAXLEN", "%zu", max_length);
+ rc = print_assuan_status (ctx, "INQUIRE_MAXLEN", "%zu", MAX_PASSPHRASE_LEN);
if (rc)
return rc;
assuan_begin_confidential (ctx);
- rc = assuan_inquire (ctx, keyword, buffer, size, max_length);
+ rc = assuan_inquire (ctx, keyword, buffer, size, MAX_PASSPHRASE_LEN);
assuan_end_confidential (ctx);
return rc;
}
diff --git a/agent/cvt-openpgp.c b/agent/cvt-openpgp.c
index fb5a473..b42084c 100644
--- a/agent/cvt-openpgp.c
+++ b/agent/cvt-openpgp.c
@@ -916,10 +916,9 @@ convert_from_openpgp_main (ctrl_t ctrl, gcry_sexp_t s_pgp,
struct pin_entry_info_s *pi;
struct try_do_unprotect_arg_s pi_arg;
- pi = xtrycalloc_secure (1, sizeof (*pi) + MAX_PASSPHRASE_LEN + 1);
+ pi = xtrycalloc_secure (1, sizeof (*pi));
if (!pi)
return gpg_error_from_syserror ();
- pi->max_length = MAX_PASSPHRASE_LEN + 1;
pi->min_digits = 0; /* We want a real passphrase. */
pi->max_digits = 16;
pi->max_tries = 3;
@@ -954,7 +953,7 @@ convert_from_openpgp_main (ctrl_t ctrl, gcry_sexp_t s_pgp,
cache_value = agent_get_cache (cache_nonce, CACHE_MODE_NONCE);
if (cache_value)
{
- if (strlen (cache_value) < pi->max_length)
+ if (strlen (cache_value) < MAX_PASSPHRASE_LEN)
strcpy (pi->pin, cache_value);
xfree (cache_value);
}
@@ -963,7 +962,7 @@ convert_from_openpgp_main (ctrl_t ctrl, gcry_sexp_t s_pgp,
}
else if (from_native)
{
- if (strlen (passphrase) < pi->max_length)
+ if (strlen (passphrase) < MAX_PASSPHRASE_LEN)
strcpy (pi->pin, passphrase);
err = try_do_unprotect_cb (pi);
}
diff --git a/agent/divert-scd.c b/agent/divert-scd.c
index a2da9e7..955233c 100644
--- a/agent/divert-scd.c
+++ b/agent/divert-scd.c
@@ -260,10 +260,9 @@ getpin_cb (void *opaque, const char *info, char *buf, size_t maxbuf)
mess because we should call the card's verify function from the
pinentry check pin CB. */
again:
- pi = gcry_calloc_secure (1, sizeof (*pi) + maxbuf + 10);
+ pi = gcry_calloc_secure (1, sizeof (*pi));
if (!pi)
return gpg_error_from_syserror ();
- pi->max_length = maxbuf-1;
pi->min_digits = 0; /* we want a real passphrase */
pi->max_digits = 16;
pi->max_tries = 3;
@@ -275,14 +274,13 @@ getpin_cb (void *opaque, const char *info, char *buf, size_t maxbuf)
if (!rc && newpin)
{
struct pin_entry_info_s *pi2;
- pi2 = gcry_calloc_secure (1, sizeof (*pi) + maxbuf + 10);
+ pi2 = gcry_calloc_secure (1, sizeof (*pi));
if (!pi2)
{
rc = gpg_error_from_syserror ();
xfree (pi);
return rc;
}
- pi2->max_length = maxbuf-1;
pi2->min_digits = 0;
pi2->max_digits = 16;
pi2->max_tries = 1;
diff --git a/agent/findkey.c b/agent/findkey.c
index c49c37a..e70aace 100644
--- a/agent/findkey.c
+++ b/agent/findkey.c
@@ -450,10 +450,9 @@ unprotect (ctrl_t ctrl, const char *cache_nonce, const char *desc_text,
}
}
- pi = gcry_calloc_secure (1, sizeof (*pi) + MAX_PASSPHRASE_LEN + 1);
+ pi = gcry_calloc_secure (1, sizeof (*pi));
if (!pi)
return gpg_error_from_syserror ();
- pi->max_length = MAX_PASSPHRASE_LEN + 1;
pi->min_digits = 0; /* we want a real passphrase */
pi->max_digits = 16;
pi->max_tries = 3;
diff --git a/agent/genkey.c b/agent/genkey.c
index 13858ca..f6fab33 100644
--- a/agent/genkey.c
+++ b/agent/genkey.c
@@ -357,10 +357,9 @@ agent_ask_new_passphrase (ctrl_t ctrl, const char *prompt,
if (ctrl->pinentry_mode == PINENTRY_MODE_LOOPBACK)
{
size_t size;
- size_t len = 100;
unsigned char *buffer;
- err = pinentry_loopback(ctrl, "NEW_PASSPHRASE", &buffer, &size, len);
+ err = pinentry_loopback(ctrl, "NEW_PASSPHRASE", &buffer, &size);
if (!err)
{
if (size)
@@ -374,13 +373,11 @@ 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->max_length = MAX_PASSPHRASE_LEN + 1;
+ pi = gcry_calloc_secure (2, sizeof (*pi));
+ pi2 = pi + 1;
pi->max_tries = 3;
pi->with_qualitybar = 1;
pi->with_repeat = 1;
- pi2->max_length = MAX_PASSPHRASE_LEN + 1;
pi2->max_tries = 3;
pi2->check_cb = reenter_compare_cb;
pi2->check_cb_arg = pi->pin;
--
More information about the Gnupg-devel
mailing list