[PATCH] Add inside-Emacs mode to GUI pinentry programs
Daiki Ueno
ueno at gnu.org
Mon Jun 8 06:05:01 CEST 2015
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>
More information about the Gnupg-devel
mailing list