[PATCH] Add inside-Emacs mode to GUI pinentry programs

[Neal H. Walfield]

Hi Daiki,

My comments are inline.

Thanks!

:) Neal

At Tue, 02 Jun 2015 17:56:44 +0900,
Daiki Ueno wrote:
> +AM_CONDITIONAL(BUILD_LIBPINENTRY_EMACS,
> +              test "$pinentry_emacs" = "yes" -o "$inside_emacs" = "yes")

I'm a bit confused by this.  I can unstand building the emacs pinentry
without the inside emacs check, but doesn't inside_emacs imply
pinentry_emacs?  If so, we only need to check if "$pinentry_emacs" =
"yes".  Right?

> diff --git a/pinentry/pinentry-emacs.c b/pinentry/pinentry-emacs.c
> new file mode 100644
> index 0000000..280546d
> --- /dev/null
> +++ b/pinentry/pinentry-emacs.c
> +  if (strlen (tmpdir) + strlen ("/emacs") + 10 + strlen ("/")
> +      + strlen (socket_name) + 1 >= sizeof (unaddr.sun_path))
> +    {
> +      fprintf (stderr, "socket name is too long\n");
> +      return 0;
> +    }

> +  sprintf (unaddr.sun_path, "%s/emacs%lu/%s", tmpdir,
> +	   (unsigned long) uid, socket_name);

Technically, this is too conservative.  10 is the maximum length of
the uid.  If the user is 1000, then only 4 characters are needed.

> +/* Percent-escape control characters in DATA.  Return a newly
> +   allocated string, or DATA itself if there is no need to escape any
> +   character.  Store the length of the string to LENGTHP.  */
> +static char *
> +escape (const char *data, size_t *lengthp)
> +{
> +  char *buffer, *out_p;
> +  size_t length = *lengthp, buffer_length;

The comment is inaccurate.  *LENGTHP needs to contain the length of
DATA.  It also doesn't return a string, but a character vector (it is
not NUL terminated as far as I can tell).  This is actually a problem
for set_label (and perhaps elsewhere): it calls escape with the length
of the string, not including the NUL character!

Also, the return semantics are a bit too smart for my taste.  It would
be better to always allocate a new string.

> +/* The inverse of escape.  Removes quoting in string STR by modifying
> +   the string in place.  */
> +static char *
> +unescape (char *data, size_t *lengthp)
> +{

The semantics doesn't match escape's semantics: *LENGTHP is not the
length of the character vector.  Instead, this function assumes that
DATA is a NUL-terminated C string.  This lack of symmetry is another
reason to change escape.

> +static int
> +do_password (pinentry_t pe)
> +{
> +  char *escaped, *password;
> +  size_t length;
> +  assuan_error_t error;
> +
> +  set_labels (pe);
> +
> +  if (!send_to_emacs (emacs_socket, "GETPIN\n", 7))
> +    return -1;
> +
> +  escaped = read_from_emacs (emacs_socket, pe->timeout, NULL, &length, &error);

It is a bit unfortunate that the password is initially read into
insecure memory.  Let's allocate, say, 1k of secure memory and use
that.

> +  password = unescape (escaped, &length);
> +  pinentry_setbufferlen (pe, length);
> +  if (pe->pin)
> +    memcpy (pe->pin, password, length);

You should NUL terminate the password.

> +static int
> +do_confirm (pinentry_t pe)
> +{
> +  char buffer[16];
> +  size_t length;
> +  assuan_error_t error;
> +
> +  set_labels (pe);
> +
> +  if (!send_to_emacs (emacs_socket, "CONFIRM\n", 8))
> +    return 0;
> +
> +  length = sizeof (buffer);
> +  read_from_emacs (emacs_socket, pe->timeout, buffer, &length, &error);
> +  if (error != ASSUAN_No_Error)
> +    {
> +      pe->specific_err = error;
> +      return 0;
> +    }
> +
> +  return 1;

You don't handle the case when the user pressed cancelled (we need to
set pe->cancelled) or the third button.  It is important to get at
least the former right.  Are there any specific reasons that this is
difficult?

> +int
> +pinentry_inside_emacs (void)
> +{
> +  const char *envvar;
> +
> +  /* Check if INSIDE_EMACS envvar is set.  */
> +  envvar = getenv ("INSIDE_EMACS");
> +  if (!envvar || !*envvar)
> +    return 0;
> +
> +  /* FIXME: Additional checks for the value.  */
> +  return 1;
> +}
> +
> +int
> +pinentry_emacs_init (void)
> +{
> +  char buffer[256];
> +  size_t length = sizeof (buffer);
> +  assuan_error_t error;
> +
> +  /* Check if we can connect to the Emacs server socket.  */
> +  if (!set_socket ("pinentry"))
> +    return 0;
> +
> +  /* Check if the server responds.  */
> +  read_from_emacs (emacs_socket, INITIAL_TIMEOUT, buffer, &length, &error);
> +  return error == ASSUAN_No_Error;
> +}

Why doesn't pinentry_inside_emacs directly call pinentry_emacs_init?
It seems to me that the real test of whether we can use an emacs pin
is there?

Also, in your previous patch, you checked for the existence of a
particular symbol.  Why don't you do that anymore?  I think it is
reasonable that the INSIDE_EMACS is set, but the version of emacs
doesn't have support for pinentry.

Thanks,

Neal