bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Sebastian Wiesner]

In a non-interactive session, i.e. "emacs -Q --batch …", `read-passwd'
currently echoes the password input on the TTY.

I would expect `read-passwd' to suppress input echo while reading a
password, like suo, sudo, SSH, GPG and similar programs do.

In my opinion, the current behaviour is harmful, because from their
experience with these programs user will likely expect that input
following a "Password: " prompt is hidden, and thus may accidentally
expose their password, being unaware that Emacs behaves differently.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Andreas Schwab]

Sebastian Wiesner <swiesner@lunaryorn.com> writes:

> In a non-interactive session, i.e. "emacs -Q --batch …", `read-passwd'
> currently echoes the password input on the TTY.

Batch mode isn't designed for interaction.  It uses standard I/O,
oblivious to who is consuming the input.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Sebastian Wiesner]

Am 23.06.2014 um 17:46 schrieb Andreas Schwab <schwab@suse.de>:

> Sebastian Wiesner <swiesner@lunaryorn.com> writes:
> 
>> In a non-interactive session, i.e. "emacs -Q --batch …", `read-passwd'
>> currently echoes the password input on the TTY.
> 
> Batch mode isn't designed for interaction. It uses standard I/O,
> oblivious to who is consuming the input.

In this case `read-passwd’ should at least signal an error when called in non-interactive mode, and have a warning in its doctoring.  

Currently it is simply insecure in non-interactive mode, and neither its docstring nor the Emacs Lisp manual document that the password is exposed when called in non-interactive mode.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Glenn Morris]

Sebastian Wiesner wrote:

>> Batch mode isn't designed for interaction. It uses standard I/O,
>> oblivious to who is consuming the input.
>
> In this case `read-passwd' should at least signal an error when called
> in non-interactive mode,

I think that would be overkill.

> and have a warning in its doctoring.

A notice perhaps.

> Currently it is simply insecure in non-interactive mode, and neither
> its docstring nor the Emacs Lisp manual document that the password is
> exposed when called in non-interactive mode.

It's in the manual section on minibuffer input, and in batch mode there
is no minibuffer. For example, read-file-name doesn't offer completion
in batch-mode. It doesn't provide history. ctrl-k doesn't work. Etc.
I see no point in mentioning these things in the doc-string of every
function that uses the minibuffer.

But yes, read-passwd is a slightly special case and could stand to
mention batch mode in its doc.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Sebastian Wiesner]

Am 24.06.2014 um 20:41 schrieb Glenn Morris <rgm@gnu.org>:

> Sebastian Wiesner wrote:
> 
>>> Batch mode isn't designed for interaction. It uses standard I/O,
>>> oblivious to who is consuming the input.
>> 
>> In this case `read-passwd' should at least signal an error when called
>> in non-interactive mode,
> 
> I think that would be overkill.

I think that `read-passwd’ is a special case, because it *leaks a secret* when used in non-interactive mode, and the fact that it does is not immediately obvious.  To learn this *in advance*, that is, before actually using this function in non-interactive code, one has to conclude from some rather abstract descriptions of Emacs’ behavior in the Emacs manual.

>> Currently it is simply insecure in non-interactive mode, and neither
>> its docstring nor the Emacs Lisp manual document that the password is
>> exposed when called in non-interactive mode.
> 
> It's in the manual section on minibuffer input, and in batch mode there
> is no minibuffer. For example, read-file-name doesn't offer completion
> in batch-mode. It doesn't provide history. ctrl-k doesn't work. Etc.
> I see no point in mentioning these things in the doc-string of every
> function that uses the mini buffer.

There is a difference, I think.  Completion, history, C-k, etc. are not crucial for entering a file name, but hiding input is absolutely crucial to entering a password securely.  I can perfectly enter a file name without history or completion, but I cannot securely enter a password if it is shown during input.  

So `read-file-name’ works in non-interactive mode, albeit less conveniently, but `read-passwd’ arguably does not.

Pointing out that non-interactive mode isn’t designed for interaction is right, probably, but misses the point imho.

Besides, “non-interactive” is a little vague.  It’s obvious that `--batch’ is non-interactive, but is `--script’ as well?  In other languages, e.g. Python or Perl, scripts regularly do interaction, including reading passwords.

I think it’s only natural that Emacs users will try to do the same in Emacs Lisp, encouraged by the existence of `--script’, so they’ll sooner or later hit this issue.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Andreas Schwab]

Sebastian Wiesner <swiesner@lunaryorn.com> writes:

> Besides, “non-interactive” is a little vague.  It’s obvious that `--batch’ is non-interactive, but is `--script’ as well?  In other languages, e.g. Python or Perl, scripts regularly do interaction, including reading passwords.

Emacs is an editor, not a script language.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

Andreas Schwab <schwab@suse.de> writes:

> Sebastian Wiesner <swiesner@lunaryorn.com> writes:
>
>> Besides, “non-interactive” is a little vague.  It’s obvious that
>> `--batch’ is non-interactive, but is `--script’ as well?  In other
>> languages, e.g. Python or Perl, scripts regularly do interaction,
>> including reading passwords.
>
> Emacs is an editor, not a script language.

An example where cleartext passwords are annoying me for a while is
running the test suite. Use another remote host for Tramp tests, like
this:

# env REMOTE_TEMPORARY_FILE_DIRECTORY=/sudo::/tmp make check

> Andreas.

Best regards, Michael.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Andreas Schwab]

Michael Albinus <michael.albinus@gmx.de> writes:

> An example where cleartext passwords are annoying me for a while is
> running the test suite. Use another remote host for Tramp tests, like
> this:
>
> # env REMOTE_TEMPORARY_FILE_DIRECTORY=/sudo::/tmp make check

So use a method that does not need to ask for a password.  Both sudo and
ssh can be configured like this.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

Andreas Schwab <schwab@suse.de> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> An example where cleartext passwords are annoying me for a while is
>> running the test suite. Use another remote host for Tramp tests, like
>> this:
>>
>> # env REMOTE_TEMPORARY_FILE_DIRECTORY=/sudo::/tmp make check
>
> So use a method that does not need to ask for a password.  Both sudo and
> ssh can be configured like this.

That's not the problem, I know how to configure them. I gave sudo just
as an example you could reproduce yourself.

When preparing a Tramp release, I run the Tramp test suite for about 20
different remote file names. This includes connection methods like smb,
dav(s), telnet, nc, adb. Not all of them can be configured to work
password-less.

And sometimes I also want to test the case of providing a wrong password.

> Andreas.

Best regards, Michael.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Andreas Schwab]

Michael Albinus <michael.albinus@gmx.de> writes:

> When preparing a Tramp release, I run the Tramp test suite for about 20
> different remote file names. This includes connection methods like smb,
> dav(s), telnet, nc, adb. Not all of them can be configured to work
> password-less.

So provide it on stdin.  You want to automate your tests anyway, don't
you?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Sebastian Wiesner]

Am 25.06.2014 um 09:47 schrieb Andreas Schwab <schwab@suse.de>:

> Sebastian Wiesner <swiesner@lunaryorn.com> writes:
> 
>> Besides, “non-interactive” is a little vague.  It’s obvious that `--batch’ is non-interactive, but is `--script’ as well?  In other languages, e.g. Python or Perl, scripts regularly do interaction, including reading passwords.
> 
> Emacs is an editor, not a script language.

I’m sorry to see that you are stuck with this view, and I am disappointed that this prevents an important issue from being addressed.  

The community has gone beyond that, and uses Emacs Lisp for other purposes as well.

Anyhow, seeing that there is no interest in fixing this issue, I see no point in continuing this discussion.  How can I unsubscribe from bugs?

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

Andreas Schwab <schwab@suse.de> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> When preparing a Tramp release, I run the Tramp test suite for about 20
>> different remote file names. This includes connection methods like smb,
>> dav(s), telnet, nc, adb. Not all of them can be configured to work
>> password-less.
>
> So provide it on stdin.  You want to automate your tests anyway, don't
> you?

It depends on timing. Sometimes, during the test runs, the password
expires and will be requested, again.

And it isn't just me. (Tramp) users are encouraged to run the test suite
in their local environment, when Tramp produces an error. 

> Andreas.

Best regards, Michael.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Stefan Monnier]

>> Besides, “non-interactive” is a little vague.  It’s obvious that `--batch’
>> is non-interactive, but is `--script’ as well?  In other languages,
>> e.g. Python or Perl, scripts regularly do interaction, including
>> reading passwords.
> Emacs is an editor, not a script language.

There are all kinds of reasons why the current behavior is "logical",
but indeed, it's not desirable.  Could someone install the obvious patch
in read-passwd to emit a warning (right after the prompt, so the user
will necessarily see it)?

If someone is motivated, I would even accept a patch that turns echo off
temporarily.


        Stefan

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Glenn Morris]

Version: 24.4

Stefan Monnier wrote:

> but indeed, it's not desirable.  Could someone install the obvious patch
> in read-passwd to emit a warning (right after the prompt, so the user
> will necessarily see it)?

Done and documented.
(Feel free to adjust wording.)

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> If someone is motivated, I would even accept a patch that turns echo off
> temporarily.

Well, I've tried this 'cos I believe it is important. The idea is to
give the prompt in read-passwd the text property 'hide-chars. In
noninteractive mode, emacs writes "." instead of echoing the password
while typing. You can test it with

# emacs -batch -eval '(progn (message (read-string "Prompt1: ")) (message (read-passwd "Prompt2: ")) (message (read-string "Prompt3: ")))'

The patch is not perfect (it doesn't handled multi-byte chars, and I
have tested it only under Gnu/Linux), but it is a first step.

Comments?

>         Stefan

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff --]
[-- Type: text/x-patch, Size: 3943 bytes --]

=== modified file 'lisp/subr.el'
--- lisp/subr.el	2014-06-28 17:27:29 +0000
+++ lisp/subr.el	2014-07-10 13:53:13 +0000
@@ -2058,7 +2058,7 @@
             (let ((enable-recursive-minibuffers t))
               (read-string
                (if noninteractive
-                   (format "%s[INPUT WILL NOT BE HIDDEN!] " prompt) ; bug#17839
+		   (propertize prompt 'hide-chars t) ; bug#17839
                  prompt)
                nil t default)) ; t = "no history"
           (when (buffer-live-p minibuf)

=== modified file 'src/minibuf.c'
--- src/minibuf.c	2014-05-26 02:28:09 +0000
+++ src/minibuf.c	2014-07-10 14:19:50 +0000
@@ -35,6 +35,10 @@
 #include "keymap.h"
 #include "termhooks.h"

+#include "systty.h"
+extern void emacs_get_tty (int, struct emacs_tty *);
+extern int emacs_set_tty (int, struct emacs_tty *, bool);
+
 /* List of buffers for use as minibuffers.
    The first element of the list is used for the outermost minibuffer
    invocation, the next element is used for a recursive minibuffer
@@ -209,6 +213,8 @@
 }


+static Lisp_Object Qhide_chars;
+
 /* Like read_minibuf but reading from stdin.  This function is called
    from read_minibuf to do the job if noninteractive.  */

@@ -224,6 +230,20 @@
   char *line;
   Lisp_Object val;
   int c;
+  bool hide_chars;
+  struct emacs_tty old, new;
+
+  /* Check, whether we need to suppress echoing.  */
+  hide_chars = ! (NILP (Fget_text_property
+			(make_number (0), Qhide_chars, prompt)));
+  if (hide_chars)
+    {
+      emacs_get_tty (fileno (stdin), &old);
+      new = old;
+      new.main.c_lflag &= ~ICANON;	/* Disable buffering */
+      new.main.c_lflag &= ~ECHO;	/* Disable echoing */
+      emacs_set_tty (fileno (stdin), &new, 0);
+    }

   fprintf (stdout, "%s", SDATA (prompt));
   fflush (stdout);
@@ -240,8 +260,17 @@
 	  if (errno != EINTR)
 	    break;
 	}
+      else if (hide_chars && (c == 127)) /* DEL */
+	{
+	  /* Unfortunately, we cannot edit stdout.  */
+	  // fprintf (stdout, "%c", c);
+	  /* Hmm, this doesn't work for multi-byte characters.  */
+	  (len > 0) && len--;
+	}
       else
 	{
+	  if (hide_chars)
+	    fprintf (stdout, ".");
 	  if (len == size)
 	    {
 	      if (STRING_BYTES_BOUND / 2 < size)
@@ -253,6 +282,13 @@
 	}
     }

+  /* Reset tty.  */
+  if (hide_chars)
+    {
+      fprintf (stdout, "\n");
+      emacs_set_tty (fileno (stdin), &old, 0);
+    }
+
   if (len || c == '\n')
     {
       val = make_string (line, len);
@@ -1935,6 +1971,7 @@
   DEFSYM (Qactivate_input_method, "activate-input-method");
   DEFSYM (Qcase_fold_search, "case-fold-search");
   DEFSYM (Qmetadata, "metadata");
+  DEFSYM (Qhide_chars, "hide-chars");

   DEFVAR_LISP ("read-expression-history", Vread_expression_history,
 	       doc: /* A history list for arguments that are Lisp expressions to evaluate.

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2014-06-09 14:50:57 +0000
+++ src/sysdep.c	2014-07-10 10:07:05 +0000
@@ -105,8 +105,8 @@
 #include "syssignal.h"
 #include "systime.h"

-static void emacs_get_tty (int, struct emacs_tty *);
-static int emacs_set_tty (int, struct emacs_tty *, bool);
+void emacs_get_tty (int, struct emacs_tty *) EXTERNALLY_VISIBLE;
+int emacs_set_tty (int, struct emacs_tty *, bool) EXTERNALLY_VISIBLE;

 /* ULLONG_MAX is missing on Red Hat Linux 7.3; see Bug#11781.  */
 #ifndef ULLONG_MAX
@@ -779,7 +779,7 @@

 /* Set *TC to the parameters associated with the terminal FD,
    or clear it if the parameters are not available.  */
-static void
+void
 emacs_get_tty (int fd, struct emacs_tty *settings)
 {
   /* Retrieve the primary parameters - baud rate, character size, etcetera.  */
@@ -795,7 +795,7 @@
    *SETTINGS.  If FLUSHP, discard input.
    Return 0 if all went well, and -1 (setting errno) if anything failed.  */

-static int
+int
 emacs_set_tty (int fd, struct emacs_tty *settings, bool flushp)
 {
   /* Set the primary parameters - baud rate, character size, etcetera.  */

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Stefan Monnier]

> Well, I've tried this 'cos I believe it is important.  The idea is to
> give the prompt in read-passwd the text property 'hide-chars.  In
> noninteractive mode, emacs writes "." instead of echoing the password
> while typing. You can test it with

I think a more idiomatic way to do that would be to use a global Lisp
var that's let-bound in read-passwd.

> +      else if (hide_chars && (c == 127)) /* DEL */
> +	{
> +	  /* Unfortunately, we cannot edit stdout.  */
> +	  // fprintf (stdout, "%c", c);
> +	  /* Hmm, this doesn't work for multi-byte characters.  */
> +	  (len > 0) && len--;
> +	}

I don't think that's worth the trouble.


        Stefan

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Sebastian Wiesner]

Am 10.07.2014 um 16:36 schrieb Michael Albinus <michael.albinus@gmx.de>:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
>> If someone is motivated, I would even accept a patch that turns echo off
>> temporarily.
> 
> Well, I've tried this 'cos I believe it is important.

Many thanks!

Sebastian

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Well, I've tried this 'cos I believe it is important.  The idea is to
>> give the prompt in read-passwd the text property 'hide-chars.  In
>> noninteractive mode, emacs writes "." instead of echoing the password
>> while typing. You can test it with
>
> I think a more idiomatic way to do that would be to use a global Lisp
> var that's let-bound in read-passwd.

OK. There is now `read-hide-char', which triggers it. Users can let-bind
it to the character they prefer for hiding. This will be used in
`read-passwd', choosing the default ?. if it is not let-bound already.

>> +      else if (hide_chars && (c == 127)) /* DEL */
>> +	{
>> +	  /* Unfortunately, we cannot edit stdout.  */
>> +	  // fprintf (stdout, "%c", c);
>> +	  /* Hmm, this doesn't work for multi-byte characters.  */
>> +	  (len > 0) && len--;
>> +	}
>
> I don't think that's worth the trouble.

I've removed this.

The patch is committed to the trunk as revision 117510.

>         Stefan

Best regards, Michael.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

> The patch is committed to the trunk as revision 117510.

Grrrr. 117511, of course.

Best regards, Michael.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Fri, 11 Jul 2014 11:15:14 +0200
> Cc: Andreas Schwab <schwab@suse.de>, 17839@debbugs.gnu.org,
> 	Sebastian Wiesner <swiesner@lunaryorn.com>
> 
> OK. There is now `read-hide-char', which triggers it. Users can let-bind
> it to the character they prefer for hiding. This will be used in
> `read-passwd', choosing the default ?. if it is not let-bound already.
> 
> >> +      else if (hide_chars && (c == 127)) /* DEL */
> >> +	{
> >> +	  /* Unfortunately, we cannot edit stdout.  */
> >> +	  // fprintf (stdout, "%c", c);
> >> +	  /* Hmm, this doesn't work for multi-byte characters.  */
> >> +	  (len > 0) && len--;
> >> +	}
> >
> > I don't think that's worth the trouble.
> 
> I've removed this.
> 
> The patch is committed to the trunk as revision 117510.

Which breaks the MS-Windows build, of course, since Windows doesn't
have termios.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Which breaks the MS-Windows build, of course, since Windows doesn't
> have termios.

That's what I have said my first message: I could test it for Gnu/Linux only.
Shall I add "#ifndef WINDOWSNT" at the usual places?

Alternatively, shall we add (disable|enable)_echo functions to sysdep.c?
While looking for the tty solution, I have also seen that there are
Windows specific functions for that.

Best regards, Michael.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Eli Zaretskii]

> Date: Fri, 11 Jul 2014 12:45:14 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: schwab@suse.de, 17839@debbugs.gnu.org, swiesner@lunaryorn.com
> 
> > From: Michael Albinus <michael.albinus@gmx.de>
> > Date: Fri, 11 Jul 2014 11:15:14 +0200
> > Cc: Andreas Schwab <schwab@suse.de>, 17839@debbugs.gnu.org,
> > 	Sebastian Wiesner <swiesner@lunaryorn.com>
> > 
> > OK. There is now `read-hide-char', which triggers it. Users can let-bind
> > it to the character they prefer for hiding. This will be used in
> > `read-passwd', choosing the default ?. if it is not let-bound already.
> > 
> > >> +      else if (hide_chars && (c == 127)) /* DEL */
> > >> +	{
> > >> +	  /* Unfortunately, we cannot edit stdout.  */
> > >> +	  // fprintf (stdout, "%c", c);
> > >> +	  /* Hmm, this doesn't work for multi-byte characters.  */
> > >> +	  (len > 0) && len--;
> > >> +	}
> > >
> > > I don't think that's worth the trouble.
> > 
> > I've removed this.
> > 
> > The patch is committed to the trunk as revision 117510.
> 
> Which breaks the MS-Windows build, of course, since Windows doesn't
> have termios.

I installed a temporary fix, to allow the build to succeed, but it
means that currently `read-hide-char' is a no-op on MS-Windows.  Stay
tuned.

Btw, I think it's a mistake to expose termios bowels of struct
emacs_tty in minibuf.c.  I think we should move that code to a
separate function in sysdep.c, which will be called from minibuf.c.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: monnier@iro.umontreal.ca,  schwab@suse.de,  17839@debbugs.gnu.org,  swiesner@lunaryorn.com
> Date: Fri, 11 Jul 2014 11:58:51 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Which breaks the MS-Windows build, of course, since Windows doesn't
> > have termios.
> 
> That's what I have said my first message: I could test it for Gnu/Linux only.

Testing on GNU/Linux is not the same as installing Posix-only code in
a function that is compiled on non-Posix platforms.

> Shall I add "#ifndef WINDOWSNT" at the usual places?

I already did that.

> Alternatively, shall we add (disable|enable)_echo functions to sysdep.c?

Yes, please.  This will make emulating termios for Windows easier.

Thanks.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> I installed a temporary fix, to allow the build to succeed, but it
> means that currently `read-hide-char' is a no-op on MS-Windows.  Stay
> tuned.

Not fully a no-op. As side effect, you could apply your own hiding
character even in interactive mode. ?* instead of ?. as you like ...

> Btw, I think it's a mistake to expose termios bowels of struct
> emacs_tty in minibuf.c.  I think we should move that code to a
> separate function in sysdep.c, which will be called from minibuf.c.

... as I have proposed the other message as well. I will work on this;
unfortunately too much pressure on work these days. It might take some
days.

Best regards, Michael.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> That's what I have said my first message: I could test it for Gnu/Linux only.
>
> Testing on GNU/Linux is not the same as installing Posix-only code in
> a function that is compiled on non-Posix platforms.

At least it forces answers to my "Comments?" question ...

Best regards, Michael.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Alternatively, shall we add (disable|enable)_echo functions to sysdep.c?
>
> Yes, please.  This will make emulating termios for Windows easier.

I've moved the code to the new function suppress_echo_on_tty of
sysdep.c. For resetting the tty, no new function was necessary.

> Thanks.

Best regards, Michael.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: monnier@iro.umontreal.ca,  schwab@suse.de,  17839@debbugs.gnu.org,  swiesner@lunaryorn.com
> Date: Fri, 11 Jul 2014 14:43:10 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Alternatively, shall we add (disable|enable)_echo functions to sysdep.c?
> >
> > Yes, please.  This will make emulating termios for Windows easier.
> 
> I've moved the code to the new function suppress_echo_on_tty of
> sysdep.c. For resetting the tty, no new function was necessary.

Thanks.  I added implementation for MS-Windows.

The result is not 100% satisfactory, as one needs to press RET twice
to finish the input, and it looks like some garbage is left in the
input buffer, since the following (normal) input gets something
strange.  A workaround is to press C-z, which produces EOF, instead of
RET, when password entry is finished.

I also needed a change in minibuf.c to end the reading loop on CR, not
just a newline.  If this is bad news for Posix platforms, we can make
that code conditional on Windows.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> The result is not 100% satisfactory, as one needs to press RET twice
> to finish the input, and it looks like some garbage is left in the
> input buffer, since the following (normal) input gets something
> strange.  A workaround is to press C-z, which produces EOF, instead of
> RET, when password entry is finished.

Maybe because just "\n" is sent by fprintf()? In the Windows case,
"\r\n" might be the better choice?

And maybe we must handle the case, that RET sends 2 characters, which
must be handled by a double call of getchar()?

> I also needed a change in minibuf.c to end the reading loop on CR, not
> just a newline.  If this is bad news for Posix platforms, we can make
> that code conditional on Windows.

That's OK, I've just tested.

When I've read the code of read_minibuf_noninteractive for the first
time, I've seen that just CR was used for finishing input, and I've
thought that nobody ever used this for MS Windows. This is one of the
reasons I was somehow Windows agnostic during my work.

Best regards, Michael.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: monnier@iro.umontreal.ca,  schwab@suse.de,  17839@debbugs.gnu.org,  swiesner@lunaryorn.com
> Date: Fri, 11 Jul 2014 16:57:40 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The result is not 100% satisfactory, as one needs to press RET twice
> > to finish the input, and it looks like some garbage is left in the
> > input buffer, since the following (normal) input gets something
> > strange.  A workaround is to press C-z, which produces EOF, instead of
> > RET, when password entry is finished.
> 
> Maybe because just "\n" is sent by fprintf()? In the Windows case,
> "\r\n" might be the better choice?

I don't think so.  The problem is not what is printed, the problem is
that the loop is not exited on the 1st RET, and in fact it looks like
nothing is returned by getchar until you press the 2nd RET.

> And maybe we must handle the case, that RET sends 2 characters, which
> must be handled by a double call of getchar()?

It doesn't look like it sends 2 characters: the next call to getchar
after exiting the loop waits for more input.

> When I've read the code of read_minibuf_noninteractive for the first
> time, I've seen that just CR was used for finishing input

You mean, just the newline, right?

> and I've thought that nobody ever used this for MS Windows.

That code worked perfectly well with cooked input mode, since then RET
is converted to a newline by the stdio machinery (as it uses text
mode).

And that just gave me an idea: switch stdin to binary when not echoing
input.  Which solved the problem of double RET (and now it's clear why
it was being eaten up).

Thanks.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Sebastian Wiesner]

I’m sorry to bring this issue up again, but I noticed that the patch that hides input was only committed to Emacs trunk.  Would it be possible to backport this patch to the Emacs 24 branch as well, so that it the patch is also included Emacs 24.4?  

I know, Emacs is in feature freeze, and I understand that this patch required some changes to Emacs’ internals, but considering the security implications I think it’d be important to include this patch in Emacs 24.4 as well, to bring it downstream as soon as possible.

Greetings,
Sebastian Wiesner

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Stefan Monnier]

> I know, Emacs is in feature freeze, and I understand that this patch
> required some changes to Emacs’ internals, but considering the security
> implications I think it’d be important to include this patch in Emacs 24.4
> as well, to bring it downstream as soon as possible.

AFAIK, this problem is far from new, so there's no real hurry.
At least

   emacs23 -Q --batch --eval '(read-passwd "hello: ")'

does not hide the password for me.


        Stefan

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Sebastian Wiesner]

Am Mittwoch, 6. August 2014, 13:39:21 schrieb Stefan Monnier:
> > I know, Emacs is in feature freeze, and I understand that this patch
> > required some changes to Emacs’ internals, but considering the security
> > implications I think it’d be important to include this patch in Emacs 24.4
> > as well, to bring it downstream as soon as possible.
> 
> AFAIK, this problem is far from new, so there's no real hurry.

I know, but I'd argue that since it's kind of a security issue, albeit small, 
it deserves a timely fix.  YMMV, though, and I'm ok with either decision you 
make.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Stefan Monnier]

> I know, but I'd argue that since it's kind of a security issue, albeit
> small,  it deserves a timely fix.  YMMV, though, and I'm ok with
> either decision you  make.

I'm OK with installing a fix for it in emacs-24, but the fix we have in
trunk is not "obviously safe" enough to be appropriate for emacs-24.
And I think the warning we added to emacs-24 is a sufficient (tho not
ideal) stop-gap for emacs-24.

IOW feel free to send a safer patch (with corresponding copyright
paperwork, of course),


        Stefan

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Sebastian Wiesner]

Am 07.08.2014 um 15:01 schrieb Stefan Monnier <monnier@iro.umontreal.ca>:

>> I know, but I'd argue that since it's kind of a security issue, albeit
>> small,  it deserves a timely fix.  YMMV, though, and I'm ok with
>> either decision you  make.
> 
> I'm OK with installing a fix for it in emacs-24, but the fix we have in
> trunk is not "obviously safe" enough to be appropriate for emacs-24.

I know.

> And I think the warning we added to emacs-24 is a sufficient (tho not
> ideal) stop-gap for emacs-24.

Ok.

> 
> IOW feel free to send a safer patch (with corresponding copyright
> paperwork, of course),

I don’t think that I’m competent enough to do that.  If I understand correctly, Windows support is the main issue, and I do not use Windows.

Besides, you know my opinion of your copyright paperwork, and it hasn’t changed lately.  I won’t contribute to Emacs.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Eli Zaretskii]

> From: Sebastian Wiesner <swiesner@lunaryorn.com>
> Date: Thu, 7 Aug 2014 15:12:27 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>,
>  Michael Albinus <michael.albinus@gmx.de>,
>  schwab@suse.de,
>  17839@debbugs.gnu.org
> 
> If I understand correctly, Windows support is the main issue

??? No, it isn't.  Once Michael added the code to do this on Posix
platforms, the Windows code was added to follow suit.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Sebastian Wiesner]

Am 07.08.2014 um 17:30 schrieb Eli Zaretskii <eliz@gnu.org>:

>> From: Sebastian Wiesner <swiesner@lunaryorn.com>
>> Date: Thu, 7 Aug 2014 15:12:27 +0200
>> Cc: Eli Zaretskii <eliz@gnu.org>,
>> Michael Albinus <michael.albinus@gmx.de>,
>> schwab@suse.de,
>> 17839@debbugs.gnu.org
>> 
>> If I understand correctly, Windows support is the main issue
> 
> ??? No, it isn't.  Once Michael added the code to do this on Posix
> platforms, the Windows code was added to follow suit.

I’m sorry.  I did not follow the entire discussion, since I couldn’t comment on the implementation anyway.  All that I read was that there was some Windows-specific discussion, and from Stefan’s comment I presumed that Windows was still an issue.

But if it isn’t, I don’t see why this patch shouldn’t be “safe” for Emacs 24?  I’m sorry if I’m asking a stupid question, but I’m not familiar with Emacs’ policies.

bug#17839: 24.4.50; read-passwd echoes password input in non-interactive sessions

[Eli Zaretskii]

> From: Sebastian Wiesner <swiesner@lunaryorn.com>
> Date: Thu, 7 Aug 2014 18:08:28 +0200
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>  Michael Albinus <michael.albinus@gmx.de>,
>  Andreas Schwab <schwab@suse.de>,
>  17839@debbugs.gnu.org
> 
> >> If I understand correctly, Windows support is the main issue
> > 
> > ??? No, it isn't.  Once Michael added the code to do this on Posix
> > platforms, the Windows code was added to follow suit.
> 
> I’m sorry.  I did not follow the entire discussion, since I couldn’t comment on the implementation anyway.  All that I read was that there was some Windows-specific discussion, and from Stefan’s comment I presumed that Windows was still an issue.

The Windows-specific discussion was because implementing the same
feature on Windows had some subtle issues.

> But if it isn’t, I don’t see why this patch shouldn’t be “safe” for Emacs 24?

I guess because the code changes are not entirely trivial.