ssh: Fix allocation of pinentry buffer

[Neal H. Walfield]

Hi Niibe-san,

At Thu, 01 Oct 2015 13:27:23 +0900,
NIIBE Yutaka wrote:
> 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.

I prefer commits that encapsulate a single change.  This is the
logical unit of change.  And, this can later help to identify the
cause of problems using git bisect.

Werner sometimes has slightly larger commits arguing that git bisect
is rarely needed in practice and, when it is used, the added
resolution doesn't help find the problem much faster.  Further, he
points out that breaking up and testing the individual commits adds
overhead.

> (Well, my patch yesterday include change of agent/genkey.c.)

Whoops.  I somehow overlooked that the second change was for genkey.c!
I'm sorry!

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

Werner and I chatted about this a while ago and these constants should
be replaced by symbolic expressions.

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

What do you mean by different values?  Do you mean lengths?

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

Thanks for auditing this!

> Perhaps, it would be good to do:
>     fixed size allocation and fixed size communication

If we have a small limit, it is probably always better to use fixed
size allocations rather than attempting to save a few bytes while
increasing the code's complexity.

> 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];
>  };

This is a good change, I think.

> @@ -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);

Shouldn't this be <=, not <?  I think the convention is to allocated
MAX_PASSPHRASE_LEN+1 bytes for the buffer.


Thanks!

:) Neal