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

[Daiki Ueno]

Hello Neal,

Thanks for the prompt review.  The new patch attached below should
address the issues.

"Neal H. Walfield" <neal at walfield.org> writes:

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

I think the check logic was rather wrong and the build settings of
inside_emacs and pinentry_emacs should be orthogonal, i.e., GUI
pinentries can be built with support for INSIDE_EMACS, but without
building the pinentry-emacs program itself (and vice versa).

It should be fixed now.

>> +  if (strlen (tmpdir) + strlen ("/emacs") + 10 + strlen ("/")
>> +      + strlen (socket_name) + 1 >= sizeof (unaddr.sun_path))
>
> Technically, this is too conservative.  10 is the maximum length of
> the uid.  If the user is 1000, then only 4 characters are needed.

It seems that I misunderstood your previous suggestion regarding
asprintf.  I added an extra allocation to relax the check.

>> +/* 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!

Oops.

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

I see.  Now all the string related functions (i.e., escape, unescape,
send_to_emacs, and read_from_emacs) treat input/output NUL terminated.
That significantly simplified the code, indeed.

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

Good idea, done.

>> +  password = unescape (escaped, &length);
>> +  pinentry_setbufferlen (pe, length);
>> +  if (pe->pin)
>> +    memcpy (pe->pin, password, length);
>
> You should NUL terminate the password.

Done.

>> +static int
>> +do_confirm (pinentry_t pe)
>> +{
>
> 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?

Thanks, I missed the field in the pinentry struct.  Fixed.

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

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

Good idea, done.

>> +  /* Check if the server responds.  */
>> +  read_from_emacs (emacs_socket, INITIAL_TIMEOUT, buffer, &length, &error);
>> +  return error == ASSUAN_No_Error;
>
> 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.

Because the new patch no longer uses Elisp REPL, but the Assuan-like
protocol.  We could send "NOP" when connecting, but it is defined that
an Assuan server should send an "OK" response on a new connection, and
it is already checked here.

Regards,
-- 
Daiki Ueno
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-inside-Emacs-mode-to-GUI-pinentry-programs.patch
Type: text/x-patch
Size: 33674 bytes
Desc: not available
URL: </pipermail/attachments/20150608/e1ed6e0d/attachment-0001.bin>