bug#30190: 27.0.50; term run in line mode shows user passwords

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

1. Start a new terminal emulator in line mode.
M-x term RET
C-c C-j

2. run a command that prompt for user's password
   (e.g., 'sudo ls' or 'git push'); the user password
   visible.

* If you run the terminal in char modem then the password is not shown.
* If you run a dumb shell instead, then the password is read from the
  minibuffer with `send-invisible', thereby the password is not shown.




In GNU Emacs 27.0.50 (build 10, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2018-01-21 built on calancha-pc
Repository revision: 64c846738617d1d037eac0cefb6586c04317b0a1
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description: Debian GNU/Linux 9.3 (stretch)

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --enable-checking=yes,glyphs --enable-check-lisp-object-type
 'CFLAGS=-O0 -g3''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 THREADS LIBSYSTEMD LCMS2

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Term

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib dired dired-loaddefs
format-spec rfc822 mml mml-sec password-cache epa derived epg epg-config
gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils term disp-table easymenu ehelp
ring elec-pair time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 97899 5997)
 (symbols 48 20698 1)
 (miscs 40 46 116)
 (strings 32 29399 1942)
 (string-bytes 1 774861)
 (vectors 16 14837)
 (vector-slots 8 500989 12084)
 (floats 8 49 127)
 (intervals 56 247 0)
 (buffers 992 12))

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

found 30190 24.3
tags 30190 + confirmed
quit

Tino Calancha <tino.calancha@gmail.com> writes:

> 1. Start a new terminal emulator in line mode.
> M-x term RET
> C-c C-j
>
> 2. run a command that prompt for user's password
>    (e.g., 'sudo ls' or 'git push'); the user password
>    visible.

Yes, seems to have been the case for a long time, I can reproduce back
to 24.3 (oldest Emacs version I have running).

> * If you run the terminal in char modem then the password is not shown.
> * If you run a dumb shell instead, then the password is read from the
>   minibuffer with `send-invisible', thereby the password is not shown.

What do you mean by "dumb shell"?

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Sun, 21 Jan 2018, Noam Postavsky wrote:

> Yes, seems to have been the case for a long time, I can reproduce back
> to 24.3 (oldest Emacs version I have running).
It's even worse; the password is stored in the history of commands; after 
introduce the password 'M-p' will show it.

>> * If you run the terminal in char modem then the password is not shown.
>> * If you run a dumb shell instead, then the password is read from the
>>   minibuffer with `send-invisible', thereby the password is not shown.
>
> What do you mean by "dumb shell"?
I mean this:
M-x shell RET
sudo ls
;; Here password is read (invisible) from the minibuffer. You still can
jump to the *shell* buffer and input th epasword there; in that case
the password is visible, but as longer as you input it from the minibuffer
everything is OK.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> Yes, seems to have been the case for a long time, I can reproduce back
> to 24.3 (oldest Emacs version I have running).
This is a security risk.  I would like to have it fixed ASAP.
Below patch seems to work.  Any feedback would be appreciated.

--8<-----------------------------cut here---------------start------------->8---
commit 089c86c2d8e46fd3f134826881c010409b65d364
Author: tino calancha <tino.calancha@gmail.com>
Date:   Sun Feb 4 01:00:29 2018 +0900

    Prevent term run in line mode from showing user passwords
    
    For buffers whose mode derive from comint-mode, the user
    password is read from the minibuffer and it's hidden.
    A buffer in term-mode and line submode, instead shows
    the passwords.
    This commit forces buffers in line term-mode to hide
    passwords (Bug#30190).
    
    * lisp/term-utils: New file.  Move into it some common code
    shared by comint.el and term.el.
    
    * lisp/comint.el (comint-password-prompt-regexp):
    (send-invisible)
    Move them to the new file.  Prefix them with 'term-utils-'.
    Define aliases to the original names.
    
    * lisp/term.el (term-output-filter-functions): New hook.
    (term-send-input, term-emulate-terminal): Call it.

diff --git a/lisp/comint.el b/lisp/comint.el
index 8dba317099..0e9e13f12e 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -104,6 +104,7 @@
 (require 'ring)
 (require 'ansi-color)
 (require 'regexp-opt)                   ;For regexp-opt-charset.
+(require 'term-utils)
 \f
 ;; Buffer Local Variables:
 ;;============================================================================
@@ -342,35 +343,6 @@ comint-process-echoes
   :type 'boolean
   :group 'comint)
 
-;; AIX puts the name of the person being su'd to in front of the prompt.
-;; kinit prints a prompt like `Password for devnull@GNU.ORG: '.
-;; ksu prints a prompt like `Kerberos password for devnull/root@GNU.ORG: '.
-;; ssh-add prints a prompt like `Enter passphrase: '.
-;; plink prints a prompt like `Passphrase for key "root@GNU.ORG": '.
-;; Ubuntu's sudo prompts like `[sudo] password for user:'
-;; Some implementations of passwd use "Password (again)" as the 2nd prompt.
-;; Something called "perforce" uses "Enter password:".
-;; See M-x comint-testsuite--test-comint-password-prompt-regexp.
-(defcustom comint-password-prompt-regexp
-  (concat
-   "\\(^ *\\|"
-   (regexp-opt
-    '("Enter" "enter" "Enter same" "enter same" "Enter the" "enter the"
-      "Old" "old" "New" "new" "'s" "login"
-      "Kerberos" "CVS" "UNIX" " SMB" "LDAP" "PEM" "SUDO"
-      "[sudo]" "Repeat" "Bad" "Retype")
-    t)
-   " +\\)"
-   "\\(?:" (regexp-opt password-word-equivalents) "\\|Response\\)"
-   "\\(?:\\(?:, try\\)? *again\\| (empty for no passphrase)\\| (again)\\)?"
-   ;; "[[:alpha:]]" used to be "for", which fails to match non-English.
-   "\\(?: [[:alpha:]]+ .+\\)?[\\s  ]*[:：៖][\\s  ]*\\'")
-  "Regexp matching prompts for passwords in the inferior process.
-This is used by `comint-watch-for-password-prompt'."
-  :version "27.1"
-  :type 'regexp
-  :group 'comint)
-
 ;; Here are the per-interpreter hooks.
 (defvar comint-get-old-input (function comint-get-old-input-default)
   "Function that returns old text in Comint mode.
@@ -399,7 +371,7 @@ comint-input-filter-functions
 These functions get one argument, a string containing the text to send.")
 
 ;;;###autoload
-(defvar comint-output-filter-functions '(ansi-color-process-output comint-postoutput-scroll-to-bottom comint-watch-for-password-prompt)
+(defvar comint-output-filter-functions '(ansi-color-process-output comint-postoutput-scroll-to-bottom term-utils-watch-for-password-prompt)
   "Functions to call after output is inserted into the buffer.
 One possible function is `comint-postoutput-scroll-to-bottom'.
 These functions get one argument, a string containing the text as originally
@@ -2342,41 +2314,8 @@ comint-read-noecho
 ;; saved -- typically passwords to ftp, telnet, or somesuch.
 ;; Just enter m-x send-invisible and type in your line.
 
-(defun send-invisible (&optional prompt)
-  "Read a string without echoing.
-Then send it to the process running in the current buffer.
-The string is sent using `comint-input-sender'.
-Security bug: your string can still be temporarily recovered with
-\\[view-lossage]; `clear-this-command-keys' can fix that."
-  (interactive "P")			; Defeat snooping via C-x ESC ESC
-  (let ((proc (get-buffer-process (current-buffer)))
-	(prefix
-	 (if (eq (window-buffer) (current-buffer))
-	     ""
-	   (format "(In buffer %s) "
-		   (current-buffer)))))
-    (if proc
-	(let ((str (read-passwd (concat prefix
-					(or prompt "Non-echoed text: ")))))
-	  (if (stringp str)
-	      (progn
-		(comint-snapshot-last-prompt)
-		(funcall comint-input-sender proc str))
-	    (message "Warning: text will be echoed")))
-      (error "Buffer %s has no process" (current-buffer)))))
-
-(defun comint-watch-for-password-prompt (string)
-  "Prompt in the minibuffer for password and send without echoing.
-This function uses `send-invisible' to read and send a password to the buffer's
-process if STRING contains a password prompt defined by
-`comint-password-prompt-regexp'.
-
-This function could be in the list `comint-output-filter-functions'."
-  (when (let ((case-fold-search t))
-	  (string-match comint-password-prompt-regexp string))
-    (when (string-match "^[ \n\r\t\v\f\b\a]+" string)
-      (setq string (replace-match "" t t string)))
-    (send-invisible string)))
+(defalias 'comint-watch-for-password-prompt 'term-utils-watch-for-password-prompt)
+
 \f
 ;; Low-level process communication
 
diff --git a/lisp/term-utils.el b/lisp/term-utils.el
new file mode 100644
index 0000000000..7edb741bc3
--- /dev/null
+++ b/lisp/term-utils.el
@@ -0,0 +1,118 @@
+;;; term-utils.el --- Common code used by term.el and comint.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2018  Tino Calancha
+
+;; Author: Tino Calancha <tino.calancha@gmail.com>
+;; Keywords: processes, terminals
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;
+
+;;; code:
+\f
+
+;; Retrieve the right defvar value according with the buffer mode.
+(defmacro term-utils--defvar-value (suffix)
+  "Build a defvar with the `major-mode' and SUFFIX, return its value.
+The buffer major mode must be `term-mode' or `comint-mode'."
+  (declare (debug (symbolp)) (indent 0))
+  `(symbol-value
+    (intern-soft
+     (format "%S%S" (if (derived-mode-p 'term-mode) 'term- 'comint-)
+             ',suffix))))
+
+;; AIX puts the name of the person being su'd to in front of the prompt.
+;; kinit prints a prompt like `Password for devnull@GNU.ORG: '.
+;; ksu prints a prompt like `Kerberos password for devnull/root@GNU.ORG: '.
+;; ssh-add prints a prompt like `Enter passphrase: '.
+;; plink prints a prompt like `Passphrase for key "root@GNU.ORG": '.
+;; Ubuntu's sudo prompts like `[sudo] password for user:'
+;; Some implementations of passwd use "Password (again)" as the 2nd prompt.
+;; Something called "perforce" uses "Enter password:".
+;; See M-x comint-testsuite--test-comint-password-prompt-regexp.
+(defcustom term-utils-password-prompt-regexp
+  (concat
+   "\\(^ *\\|"
+   (regexp-opt
+    '("Enter" "enter" "Enter same" "enter same" "Enter the" "enter the"
+      "Old" "old" "New" "new" "'s" "login"
+      "Kerberos" "CVS" "UNIX" " SMB" "LDAP" "PEM" "SUDO"
+      "[sudo]" "Repeat" "Bad" "Retype")
+    t)
+   " +\\)"
+   "\\(?:" (regexp-opt password-word-equivalents) "\\|Response\\)"
+   "\\(?:\\(?:, try\\)? *again\\| (empty for no passphrase)\\| (again)\\)?"
+   ;; "[[:alpha:]]" used to be "for", which fails to match non-English.
+   "\\(?: [[:alpha:]]+ .+\\)?[\\s  ]*[:：៖][\\s  ]*\\'")
+  "Regexp matching prompts for passwords in the inferior process.
+This is used by `term-utils-watch-for-password-prompt'."
+  :version "27.1"
+  :type 'regexp
+  :group 'term-utils)
+
+(declare-function comint-snapshot-last-prompt "comint")
+
+(defun term-utils-send-invisible (&optional prompt)
+  "Read a string without echoing with PROMPT.
+Then send it to the process running in the current buffer.
+The string is sent using `comint-input-sender' or `term-input-sender'
+depending of the buffer mode.
+Security bug: your string can still be temporarily recovered with
+\\[view-lossage]; `clear-this-command-keys' can fix that."
+  (interactive "P")			; Defeat snooping via C-x ESC ESC
+  (let ((proc (get-buffer-process (current-buffer)))
+	(prefix
+	 (if (eq (window-buffer) (current-buffer))
+	     ""
+	   (format "(In buffer %s) "
+		   (current-buffer)))))
+    (if proc
+	(let ((str (read-passwd (concat prefix
+					(or prompt "Non-echoed text: ")))))
+	  (if (stringp str)
+	      (let ((input-sender-fun (term-utils--defvar-value input-sender)))
+		(if (derived-mode-p 'comint-mode) (comint-snapshot-last-prompt))
+		(funcall input-sender-fun proc str))
+	    (message "Warning: text will be echoed")))
+      (error "Buffer %s has no process" (current-buffer)))))
+
+(defalias 'send-invisible 'term-utils-send-invisible)
+
+(defvar term-raw-map) ; Defined in term.el
+(defun term-utils-term-in-line-mode-p ()
+  "Return non-nil if the buffer is in `term-mode' and line submode."
+  (and (derived-mode-p 'term-mode) (eq (current-local-map) term-raw-map)))
+
+(defun term-utils-watch-for-password-prompt (string)
+  "Prompt in the minibuffer for password and send without echoing.
+This function uses `send-invisible' to read and send a password to the buffer's
+process if STRING contains a password prompt defined by
+`term-utils-password-prompt-regexp'.
+
+This function could be in the lists `comint-output-filter-functions'
+and `term-output-filter-functions'."
+  ;; Do nothing if buffer is in term-mode with line submode
+  (unless (term-utils-term-in-line-mode-p)
+    (when (let ((case-fold-search t))
+	    (string-match term-utils-password-prompt-regexp string))
+      (when (string-match "^[ \n\r\t\v\f\b\a]+" string)
+        (setq string (replace-match "" t t string)))
+      (term-utils-send-invisible string))))
+
+
+(provide 'term-utils)
+;;; term-utils.el ends here
diff --git a/lisp/term.el b/lisp/term.el
index a0313d88da..ed0f1715b2 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -340,6 +340,7 @@ term-protocol-version
 (eval-when-compile (require 'cl-lib))
 (require 'ring)
 (require 'ehelp)
+(require 'term-utils)
 
 (declare-function ring-empty-p "ring" (ring))
 (declare-function ring-ref "ring" (ring index))
@@ -575,6 +576,16 @@ term-input-filter-functions
 
 This variable is buffer-local.")
 
+;;;###autoload
+(defvar term-output-filter-functions '(term-utils-watch-for-password-prompt)
+  "Functions to call after output is inserted into the buffer.
+One possible function is `term-utils-watch-for-password-prompt'.
+These functions get one argument, a string containing the text as originally
+inserted.
+
+You can use `add-hook' to add functions to this list
+either globally or locally.")
+
 (defvar term-input-sender (function term-simple-send)
   "Function to actually send to PROCESS the STRING submitted by user.
 Usually this is just `term-simple-send', but if your mode needs to
@@ -2106,7 +2117,8 @@ term-send-input
 	  (set-marker term-pending-delete-marker pmark-val)
 	  (set-marker (process-mark proc) (point)))
 	(goto-char pmark)
-	(funcall term-input-sender proc input)))))
+	(funcall term-input-sender proc input)
+        (run-hook-with-args 'term-output-filter-functions "")))))
 
 (defun term-get-old-input-default ()
   "Default for `term-get-old-input'.
@@ -3017,6 +3029,8 @@ term-emulate-terminal
 	  (term-handle-deferred-scroll))
 
 	(set-marker (process-mark proc) (point))
+        ;; Run these hooks with point where the user had it.
+        (run-hook-with-args 'term-output-filter-functions str)
 	(when save-point
 	  (goto-char save-point)
 	  (set-marker save-point nil))

--8<-----------------------------cut here---------------end--------------->8---

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

Tino Calancha <tino.calancha@gmail.com> writes:

> Noam Postavsky <npostavs@users.sourceforge.net> writes:
>
>> Yes, seems to have been the case for a long time, I can reproduce back
>> to 24.3 (oldest Emacs version I have running).
> This is a security risk.  I would like to have it fixed ASAP.
> Below patch seems to work.  Any feedback would be appreciated.

Doesn't look like that much of a risk to me: the user immediately sees
the problem, so it's more of a minor nuisance.

> -(defcustom comint-password-prompt-regexp

I don't see an alias for this one.  Otherwise I think it's okay.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sun, 04 Feb 2018 01:15:54 +0900
> Cc: 30190@debbugs.gnu.org
> 
> Noam Postavsky <npostavs@users.sourceforge.net> writes:
> 
> > Yes, seems to have been the case for a long time, I can reproduce back
> > to 24.3 (oldest Emacs version I have running).
> This is a security risk.  I would like to have it fixed ASAP.
> Below patch seems to work.  Any feedback would be appreciated.

My feedback is that such a radical solution with so many lines of code
is a no-no for the release branch.  Please look for a simpler
solution, perhaps don't create a new file?

Thanks.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Sat, 3 Feb 2018, Noam Postavsky wrote:

> Tino Calancha <tino.calancha@gmail.com> writes:
>
>> Noam Postavsky <npostavs@users.sourceforge.net> writes:
>>
>>> Yes, seems to have been the case for a long time, I can reproduce back
>>> to 24.3 (oldest Emacs version I have running).
>> This is a security risk.  I would like to have it fixed ASAP.
>> Below patch seems to work.  Any feedback would be appreciated.
>
> Doesn't look like that much of a risk to me: the user immediately sees
> the problem, so it's more of a minor nuisance.
It depends of the situation.  Few years ago, my boss watched my password
because this thing; if the password would be an offensive word
against him (it wasn't, he was nice) I could be fired.  I remember he 
mnetioned very proudly that in vi editor the password is always hidden...

This is also a risk while pair-programming; recently I am doing a lot with
several buddies.  I suspect one of my passwords might be compromised.
>> -(defcustom comint-password-prompt-regexp
>
> I don't see an alias for this one.  Otherwise I think it's okay.
Thanks, I will fix that.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Sat, 3 Feb 2018, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Date: Sun, 04 Feb 2018 01:15:54 +0900
>> Cc: 30190@debbugs.gnu.org
>>
>> Noam Postavsky <npostavs@users.sourceforge.net> writes:
>>
>>> Yes, seems to have been the case for a long time, I can reproduce back
>>> to 24.3 (oldest Emacs version I have running).
>> This is a security risk.  I would like to have it fixed ASAP.
>> Below patch seems to work.  Any feedback would be appreciated.
>
> My feedback is that such a radical solution with so many lines of code
> is a no-no for the release branch.  Please look for a simpler
> solution, perhaps don't create a new file?
OK, I will prepare one less verbose by today.
I agree it would be nice to have it in the next release.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

On Sat, Feb 3, 2018 at 9:23 PM, Tino Calancha <tino.calancha@gmail.com> wrote:

>> Doesn't look like that much of a risk to me: the user immediately sees
>> the problem, so it's more of a minor nuisance.
>
> It depends of the situation.  Few years ago, my boss watched my password
> because this thing; if the password would be an offensive word
> against him (it wasn't, he was nice) I could be fired.  I remember he
> mnetioned very proudly that in vi editor the password is always hidden...
>
> This is also a risk while pair-programming; recently I am doing a lot with
> several buddies.  I suspect one of my passwords might be compromised.

But why wouldn't you just switch to char-mode before entering your
password? (which is kind of annoying to have to do, but still)

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Sat, 3 Feb 2018, Noam Postavsky wrote:

> On Sat, Feb 3, 2018 at 9:23 PM, Tino Calancha <tino.calancha@gmail.com> wrote:
>
>>> Doesn't look like that much of a risk to me: the user immediately sees
>>> the problem, so it's more of a minor nuisance.
>>
>> It depends of the situation.  Few years ago, my boss watched my password
>> because this thing; if the password would be an offensive word
>> against him (it wasn't, he was nice) I could be fired.  I remember he
>> mnetioned very proudly that in vi editor the password is always hidden...
>>
>> This is also a risk while pair-programming; recently I am doing a lot with
>> several buddies.  I suspect one of my passwords might be compromised.
>
> But why wouldn't you just switch to char-mode before entering your
> password? (which is kind of annoying to have to do, but still)
Because I am so focused in the coding task on hands that I hardly can
remember this password thing.

Such bookeeping is not a human task, it must be done by computers. Like 
remember aniversaries: most of the people use tools to remember that its 
your birthday.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

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



On Sat, 3 Feb 2018, Eli Zaretskii wrote:

> My feedback is that such a radical solution with so many lines of code
> is a no-no for the release branch.  Please look for a simpler
> solution, perhaps don't create a new file?
A suitable patch for the next release for discussion below:

--8<-----------------------------cut here---------------start------------->8---
commit 2a0f243ff63772bb16ce4e3666b5fd5e696ea68f
Author: tino calancha <tino.calancha@gmail.com>
Date:   Sun Feb 4 12:32:04 2018 +0900

     Prevent term run in line mode from showing user passwords

     For buffers whose mode derive from comint-mode, the user
     password is read from the minibuffer and it's hidden.
     A buffer in term-mode and line submode, instead shows
     the passwords.
     This commit forces buffers in line term-mode to hide
     passwords (Bug#30190).

     * lisp/term.el (term-password-prompt-regexp): New user option.
     (term-watch-for-password-prompt): New function.

     (term-send-input, term-emulate-terminal): Call it.
     (term-output-filter-hook): New hook.  Add term-watch-for-password-prompt
     to it.

     (term-send-input, term-emulate-terminal): Call the new hook each time
     we receive output.

diff --git a/lisp/term.el b/lisp/term.el
index 3970e93cf1..49fbc58cf3 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -558,6 +558,27 @@ term-suppress-hard-newline
  ;; indications of the current pc.
  (defvar term-pending-frame nil)

+;; Stolen from comint.el
+(defcustom term-password-prompt-regexp
+  (concat
+   "\\(^ *\\|"
+   (regexp-opt
+    '("Enter" "enter" "Enter same" "enter same" "Enter the" "enter the"
+      "Old" "old" "New" "new" "'s" "login"
+      "Kerberos" "CVS" "UNIX" " SMB" "LDAP" "PEM" "SUDO"
+      "[sudo]" "Repeat" "Bad" "Retype")
+    t)
+   " +\\)"
+   "\\(?:" (regexp-opt password-word-equivalents) "\\|Response\\)"
+   "\\(?:\\(?:, try\\)? *again\\| (empty for no passphrase)\\| (again)\\)?"
+   ;; "[[:alpha:]]" used to be "for", which fails to match non-English.
+   "\\(?: [[:alpha:]]+ .+\\)?[\\s  ]*[:：៖][\\s  ]*\\'")
+  "Regexp matching prompts for passwords in the inferior process.
+This is used by `term-watch-for-password-prompt'."
+  :version "27.1"
+  :type 'regexp
+  :group 'comint)
+
  ;;; Here are the per-interpreter hooks.
  (defvar term-get-old-input (function term-get-old-input-default)
    "Function that submits old text in term mode.
@@ -586,6 +607,17 @@ term-input-filter-functions

  This variable is buffer-local.")

+;;; Stolen from comint.el
+;;;###autoload
+(defvar term-output-filter-hook '(term-watch-for-password-prompt)
+  "Functions to call after output is inserted into the buffer.
+One possible function is `term-watch-for-password-prompt'.
+These functions get one argument, a string containing the text as originally
+inserted.
+
+You can use `add-hook' to add functions to this list
+either globally or locally.")
+
  (defvar term-input-sender (function term-simple-send)
    "Function to actually send to PROCESS the STRING submitted by user.
  Usually this is just `term-simple-send', but if your mode needs to
@@ -2134,7 +2166,8 @@ term-send-input
  	  (set-marker term-pending-delete-marker pmark-val)
  	  (set-marker (process-mark proc) (point)))
  	(goto-char pmark)
-	(funcall term-input-sender proc input)))))
+	(funcall term-input-sender proc input)
+        (run-hook-with-args 'term-output-filter-hook "")))))

  (defun term-get-old-input-default ()
    "Default for `term-get-old-input'.
@@ -2264,6 +2297,21 @@ term-send-invisible
      (term-send-string proc str)
      (term-send-string proc "\n")))

+;;; Stolen from comint.el
+;; TODO: This file share plenty of code with comint.el; it might be worth
+;; to extract the common functionality into a new file.
+(defun term-watch-for-password-prompt (string)
+  "Prompt in the minibuffer for password and send without echoing.
+This function uses `term-send-invisible' to read and send a password to the buffer's
+process if STRING contains a password prompt defined by
+`term-password-prompt-regexp'.
+
+This function could be in the list `term-emulate-terminal'."
+  (when (term-in-line-mode)
+    (when (let ((case-fold-search t))
+	    (string-match term-password-prompt-regexp string))
+      (term-send-invisible nil))))
+

  ;;; Low-level process communication

@@ -3121,6 +3169,8 @@ term-emulate-terminal
  	  (term-handle-deferred-scroll))

  	(set-marker (process-mark proc) (point))
+        ;; Run these hooks with point where the user had it.
+        (run-hook-with-args 'term-output-filter-hook str)
  	(when save-point
  	  (goto-char save-point)
  	  (set-marker save-point nil))
--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.91 (build 8, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
  of 2018-02-04 built on calancha-pc
Repository revision: aafcd12eebff1549b3d4b4f8f8d0f24498c1aedf

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

Tino Calancha <tino.calancha@gmail.com> writes:

> On Sat, 3 Feb 2018, Eli Zaretskii wrote:
>
>> My feedback is that such a radical solution with so many lines of code
>> is a no-no for the release branch.  Please look for a simpler
>> solution, perhaps don't create a new file?
> A suitable patch for the next release for discussion below:
>

> +;; Stolen from comint.el
> +(defcustom term-password-prompt-regexp

> +  :version "27.1"

I guess this should say "26.1".  Although maybe we should just use
comint-password-prompt-regexp in term.el instead?

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

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



On Sun, 4 Feb 2018, Noam Postavsky wrote:

> Tino Calancha <tino.calancha@gmail.com> writes:
>
>> On Sat, 3 Feb 2018, Eli Zaretskii wrote:
>>
>>> My feedback is that such a radical solution with so many lines of code
>>> is a no-no for the release branch.  Please look for a simpler
>>> solution, perhaps don't create a new file?
>> A suitable patch for the next release for discussion below:
>>
>
>> +;; Stolen from comint.el
>> +(defcustom term-password-prompt-regexp
>
>> +  :version "27.1"
That's right.  Well catched, thank you!
Updated patch.
>
> I guess this should say "26.1".  Although maybe we should just use
> comint-password-prompt-regexp in term.el instead?
Part of the fun is to prevent term.el from requiring comint.el as
always has be done; just for using one variable I would not require
comint.el.
--8<-----------------------------cut here---------------start------------->8---
commit 6187b493ded4bdcfd3c6b6fa91333c381fba8913
Author: tino calancha <tino.calancha@gmail.com>
Date:   Sun Feb 4 21:43:43 2018 +0900

     Prevent term run in line mode from showing user passwords

     For buffers whose mode derive from comint-mode, the user
     password is read from the minibuffer and it's hidden.
     A buffer in term-mode and line submode, instead shows
     the passwords.
     This commit forces buffers in line term-mode to hide
     passwords (Bug#30190).

     * lisp/term.el (term-password-prompt-regexp): New user option.
     (term-watch-for-password-prompt): New function.

     (term-send-input, term-emulate-terminal): Call it.
     (term-output-filter-hook): New hook.  Add term-watch-for-password-prompt
     to it.

     (term-send-input, term-emulate-terminal): Call the new hook each time
     we receive output.

diff --git a/lisp/term.el b/lisp/term.el
index 3970e93cf1..6fddef6f82 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -558,6 +558,27 @@ term-suppress-hard-newline
  ;; indications of the current pc.
  (defvar term-pending-frame nil)

+;; Stolen from comint.el
+(defcustom term-password-prompt-regexp
+  (concat
+   "\\(^ *\\|"
+   (regexp-opt
+    '("Enter" "enter" "Enter same" "enter same" "Enter the" "enter the"
+      "Old" "old" "New" "new" "'s" "login"
+      "Kerberos" "CVS" "UNIX" " SMB" "LDAP" "PEM" "SUDO"
+      "[sudo]" "Repeat" "Bad" "Retype")
+    t)
+   " +\\)"
+   "\\(?:" (regexp-opt password-word-equivalents) "\\|Response\\)"
+   "\\(?:\\(?:, try\\)? *again\\| (empty for no passphrase)\\| (again)\\)?"
+   ;; "[[:alpha:]]" used to be "for", which fails to match non-English.
+   "\\(?: [[:alpha:]]+ .+\\)?[\\s  ]*[:：៖][\\s  ]*\\'")
+  "Regexp matching prompts for passwords in the inferior process.
+This is used by `term-watch-for-password-prompt'."
+  :version "26.1"
+  :type 'regexp
+  :group 'comint)
+
  ;;; Here are the per-interpreter hooks.
  (defvar term-get-old-input (function term-get-old-input-default)
    "Function that submits old text in term mode.
@@ -586,6 +607,17 @@ term-input-filter-functions

  This variable is buffer-local.")

+;;; Stolen from comint.el
+;;;###autoload
+(defvar term-output-filter-hook '(term-watch-for-password-prompt)
+  "Functions to call after output is inserted into the buffer.
+One possible function is `term-watch-for-password-prompt'.
+These functions get one argument, a string containing the text as originally
+inserted.
+
+You can use `add-hook' to add functions to this list
+either globally or locally.")
+
  (defvar term-input-sender (function term-simple-send)
    "Function to actually send to PROCESS the STRING submitted by user.
  Usually this is just `term-simple-send', but if your mode needs to
@@ -2134,7 +2166,8 @@ term-send-input
  	  (set-marker term-pending-delete-marker pmark-val)
  	  (set-marker (process-mark proc) (point)))
  	(goto-char pmark)
-	(funcall term-input-sender proc input)))))
+	(funcall term-input-sender proc input)
+        (run-hook-with-args 'term-output-filter-hook "")))))

  (defun term-get-old-input-default ()
    "Default for `term-get-old-input'.
@@ -2264,6 +2297,21 @@ term-send-invisible
      (term-send-string proc str)
      (term-send-string proc "\n")))

+;;; Stolen from comint.el
+;; TODO: This file share plenty of code with comint.el; it might be worth
+;; to extract the common functionality into a new file.
+(defun term-watch-for-password-prompt (string)
+  "Prompt in the minibuffer for password and send without echoing.
+This function uses `term-send-invisible' to read and send a password to the buffer's
+process if STRING contains a password prompt defined by
+`term-password-prompt-regexp'.
+
+This function could be in the list `term-emulate-terminal'."
+  (when (term-in-line-mode)
+    (when (let ((case-fold-search t))
+	    (string-match term-password-prompt-regexp string))
+      (term-send-invisible nil))))
+

  ;;; Low-level process communication

@@ -3121,6 +3169,8 @@ term-emulate-terminal
  	  (term-handle-deferred-scroll))

  	(set-marker (process-mark proc) (point))
+        ;; Run these hooks with point where the user had it.
+        (run-hook-with-args 'term-output-filter-hook str)
  	(when save-point
  	  (goto-char save-point)
  	  (set-marker save-point nil))
--8<-----------------------------cut here---------------end--------------->8---

bug#30190: 27.0.50; term run in line mode shows user passwords

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Passwords should be hidden, and we must fix this bug.
Nobody should argue for sloppiness about this.


-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

Tino Calancha <tino.calancha@gmail.com> writes:

> On Sat, 3 Feb 2018, Eli Zaretskii wrote:
>
>> My feedback is that such a radical solution with so many lines of code
>> is a no-no for the release branch.  Please look for a simpler
>> solution, perhaps don't create a new file?
> A suitable patch for the next release for discussion below:

My patch is not satisfactory.  It uses functions that haven't been
tested enough: `term-send-invisible' and `term-read-noecho' are not
used in the Emacs source tree:  after playing a bit with the patch
I found some problems (I even crashed Emacs several times, not so fun).

I) `term-read-noecho' doesn't expect the user might hit <up>, <down>,
`C-p' or `C-n'; it will try to convert these things to strings and return
them.

II) `term-send-invisible' doesn't cancel if the user hit `C-g'; instead
it pass nil as the string for the process.  The reason is how
`term-read-noecho'  handles `C-g'.

My Emacs session crashes if I do (using the patch in this thread):
M-x term RET
C-c C-j
sudo ls RET
C-g C-g ;; Emacs crash!

Note that I) and II) don't suppose any problem in a *shell* buffer if
calling `send-invisible', which uses the more robust `read-passwd'.

So I want to avoid using the not well tested `term-read-noecho'.  Instead,
I propose to use `read-passwd' as elsewhere within Emacs.

This is the updated patch:
--8<-----------------------------cut here---------------start------------->8---
commit 6254b0aca3c91ebd6d41f865c1cdcc13166c066d
Author: tino calancha <tino.calancha@gmail.com>
Date:   Thu Feb 15 08:58:45 2018 +0900

    Prevent term run in line mode from showing user passwords
    
    For buffers whose mode derive from comint-mode, the user
    password is read from the minibuffer and it's hidden.
    A buffer in term-mode and line submode, instead shows
    the passwords.
    This commit forces buffers in line term-mode to hide
    passwords (Bug#30190).
    
    * lisp/term.el (term-send-invisible):
    Prefer the more robust `read-passwd' instead of `term-read-noecho'.
    
    (term-password-prompt-regexp): New user option.
    (term-watch-for-password-prompt): New function.
    
    (term-send-input, term-emulate-terminal): Call it.
    (term-output-filter-hook): New hook.  Add term-watch-for-password-prompt
    to it.
    
    (term-send-input, term-emulate-terminal): Call the new hook each time
    we receive output.

diff --git a/lisp/term.el b/lisp/term.el
index 3970e93cf1..484a26cd7a 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -558,6 +558,27 @@ term-suppress-hard-newline
 ;; indications of the current pc.
 (defvar term-pending-frame nil)
 
+;; Stolen from comint.el
+(defcustom term-password-prompt-regexp
+  (concat
+   "\\(^ *\\|"
+   (regexp-opt
+    '("Enter" "enter" "Enter same" "enter same" "Enter the" "enter the"
+      "Old" "old" "New" "new" "'s" "login"
+      "Kerberos" "CVS" "UNIX" " SMB" "LDAP" "PEM" "SUDO"
+      "[sudo]" "Repeat" "Bad" "Retype")
+    t)
+   " +\\)"
+   "\\(?:" (regexp-opt password-word-equivalents) "\\|Response\\)"
+   "\\(?:\\(?:, try\\)? *again\\| (empty for no passphrase)\\| (again)\\)?"
+   ;; "[[:alpha:]]" used to be "for", which fails to match non-English.
+   "\\(?: [[:alpha:]]+ .+\\)?[\\s  ]*[:：៖][\\s  ]*\\'")
+  "Regexp matching prompts for passwords in the inferior process.
+This is used by `term-watch-for-password-prompt'."
+  :version "26.1"
+  :type 'regexp
+  :group 'comint)
+
 ;;; Here are the per-interpreter hooks.
 (defvar term-get-old-input (function term-get-old-input-default)
   "Function that submits old text in term mode.
@@ -586,6 +607,17 @@ term-input-filter-functions
 
 This variable is buffer-local.")
 
+;;; Stolen from comint.el
+;;;###autoload
+(defvar term-output-filter-hook '(term-watch-for-password-prompt)
+  "Functions to call after output is inserted into the buffer.
+One possible function is `term-watch-for-password-prompt'.
+These functions get one argument, a string containing the text as originally
+inserted.
+
+You can use `add-hook' to add functions to this list
+either globally or locally.")
+
 (defvar term-input-sender (function term-simple-send)
   "Function to actually send to PROCESS the STRING submitted by user.
 Usually this is just `term-simple-send', but if your mode needs to
@@ -2134,7 +2166,8 @@ term-send-input
 	  (set-marker term-pending-delete-marker pmark-val)
 	  (set-marker (process-mark proc) (point)))
 	(goto-char pmark)
-	(funcall term-input-sender proc input)))))
+	(funcall term-input-sender proc input)
+        (run-hook-with-args 'term-output-filter-hook "")))))
 
 (defun term-get-old-input-default ()
   "Default for `term-get-old-input'.
@@ -2255,7 +2288,8 @@ term-send-invisible
 \\[view-lossage]."
   (interactive "P") ; Defeat snooping via C-x esc
   (when (not (stringp str))
-    (setq str (term-read-noecho "Non-echoed text: " t)))
+    (let ((read-hide-char ?*))
+      (setq str (read-passwd "Non-echoed text: "))))
   (when (not proc)
     (setq proc (get-buffer-process (current-buffer))))
   (if (not proc) (error "Current buffer has no process")
@@ -2264,6 +2298,21 @@ term-send-invisible
     (term-send-string proc str)
     (term-send-string proc "\n")))
 
+;;; Stolen from comint.el
+;; TODO: This file share plenty of code with comint.el; it might be worth
+;; to extract the common functionality into a new file.
+(defun term-watch-for-password-prompt (string)
+  "Prompt in the minibuffer for password and send without echoing.
+This function uses `term-send-invisible' to read and send a password to the buffer's
+process if STRING contains a password prompt defined by
+`term-password-prompt-regexp'.
+
+This function could be in the list `term-emulate-terminal'."
+  (when (term-in-line-mode)
+    (when (let ((case-fold-search t))
+	    (string-match term-password-prompt-regexp string))
+      (term-send-invisible nil))))
+
 \f
 ;;; Low-level process communication
 
@@ -3121,6 +3170,8 @@ term-emulate-terminal
 	  (term-handle-deferred-scroll))
 
 	(set-marker (process-mark proc) (point))
+        ;; Run these hooks with point where the user had it.
+        (run-hook-with-args 'term-output-filter-hook str)
 	(when save-point
 	  (goto-char save-point)
 	  (set-marker save-point nil))
--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.91 (build 15, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2018-02-15 built on calancha-pc
Repository revision: 874c0edf30308392bdba870e92247d7e4b0e66f4
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description:	Debian GNU/Linux 9.3 (stretch)

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

Tino Calancha <tino.calancha@gmail.com> writes:

> Tino Calancha <tino.calancha@gmail.com> writes:
>
>
> This is the updated patch:
>
> commit 6254b0aca3c91ebd6d41f865c1cdcc13166c066d
> Author: tino calancha <tino.calancha@gmail.com>
> Date:   Thu Feb 15 08:58:45 2018 +0900
.
.
.

If anyone can show just cause why this patch cannot lawfully be joined
together in Emacs-26 branch, let them speak now or forever hold their
peace.
Otherwise I will push this fix in a couple of days, and what the
Emacs hacker has joined, no men ever separate.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 30190@debbugs.gnu.org,  Richard Stallman <rms@gnu.org>,  npostavs@users.sourceforge.net
> Date: Wed, 21 Feb 2018 19:18:31 +0900
> 
> If anyone can show just cause why this patch cannot lawfully be joined
> together in Emacs-26 branch, let them speak now or forever hold their
> peace.

You'll have to convince me that all of that is needed to fix the bug
exposed by your recipe, and that we really cannot live with the bug
until Emacs 27.

Thanks.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Cc: 30190@debbugs.gnu.org,  Richard Stallman <rms@gnu.org>,  npostavs@users.sourceforge.net
>> Date: Wed, 21 Feb 2018 19:18:31 +0900
>> 
>> If anyone can show just cause why this patch cannot lawfully be joined
>> together in Emacs-26 branch, let them speak now or forever hold their
>> peace.

Thanks for the replay, and sorry for the late response; I am quite busy
guy last 2 months (next week even more :-S ).
> You'll have to convince me that
> 1. we really cannot live with the bug until Emacs 27.
You can live with it.  Many people can live with it.  Indeed, this bug
has been there since the addition of this lib. several releases before.

I cannot live with it;  any user using 'term.el' in line mode
should not live with it.  It's a security issue and should be
taken seriously.  IMO, Emacs sends the wrong message delivering a new
release with a security bug, having a simple and well understood
fix for it.

Last week one of my teachers saw my email password in my screen.  He
was very serious about that, and requested me to please, _inmediately_
change my password.  Ciertanly, many developers care about these kind
of issues.

>2. all of that is needed to fix the bug exposed by your recipe.
The patch is crafted so that:
* It just modifies one file, i.e. term.el.
* Don't stablishes new dependencies between comint.el and term.el.

With that in mind, you can how simple is the patch.  It _just_ copy
step by step what it is done in comint.el:

term-password-prompt-regexp    <--> comint-password-prompt-regexp
term-output-filter-hook        <--> comint-output-filter-functions
term-watch-for-password-prompt <--> comint-watch-for-password-prompt

Run hook 'term-output-filter-hook' in 'term-input-sender' <-->
Run hook 'comint-output-filter-functions' comint-output-filter-functions

'term-send-invisible' uses `read-passwd' <--> 'send-invisible' uses
`read-passwd'

Run hook 'term-output-filter-hook' in 'term-emulate-terminal' <-->
Run hook 'comint-output-filter-functions' in 'comint-output-filter'


IMO the patch is simple, necessary and save to be included in Emacs-26.

PD: Later on, for Emacs-27 we might want to reduce code duplication
withing comint.el and term.el, for instance with the addition
of a new file.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 30190@debbugs.gnu.org,  rms@gnu.org,  npostavs@users.sourceforge.net
> Date: Sat, 10 Mar 2018 17:52:25 +0900
> 
> > You'll have to convince me that
> > 1. we really cannot live with the bug until Emacs 27.
> You can live with it.  Many people can live with it.  Indeed, this bug
> has been there since the addition of this lib. several releases before.
> 
> I cannot live with it;  any user using 'term.el' in line mode
> should not live with it.  It's a security issue and should be
> taken seriously.  IMO, Emacs sends the wrong message delivering a new
> release with a security bug, having a simple and well understood
> fix for it.
> 
> Last week one of my teachers saw my email password in my screen.  He
> was very serious about that, and requested me to please, _inmediately_
> change my password.  Ciertanly, many developers care about these kind
> of issues.
> 
> >2. all of that is needed to fix the bug exposed by your recipe.
> The patch is crafted so that:
> * It just modifies one file, i.e. term.el.
> * Don't stablishes new dependencies between comint.el and term.el.
> 
> With that in mind, you can how simple is the patch.  It _just_ copy
> step by step what it is done in comint.el:

Here's what bothers me about the patch:

 . it installs the filter even when term.el is not in line mode
 . it uses many constructs in term-password-prompt-regexp that could
   happen in unrelated text--does that mean such unrelated text will
   become invisible, thus making the session at least look buggy?

The 2nd issue looks to me like a more serious one, unless I'm missing
something.  Is it possible to make sure we don't mistakenly take some
innocent text as a password?  Did you try in your testing to type text
that matches this regexp, and if so, what did you see as result?

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Sat, 10 Mar 2018, Eli Zaretskii wrote:

> Here's what bothers me about the patch:
>
> . it installs the filter even when term.el is not in line mode
It must install it by default, because the user might switch several
times between char mode <-> line mode (I do a lot).  Furthermore,
'term-watch-for-password-prompt' is a noop in char-mode.  Besides that,
'term-send-input' is just called in line-mode, so the filter would ever
be called in char mode.

> . it uses many constructs in term-password-prompt-regexp that could
>   happen in unrelated text--does that mean such unrelated text will
>   become invisible, thus making the session at least look buggy?
Regarding this issue, it would behave exactly as a dumb shell buffer does
M-x shell RET
I've never seen a bug report about that for comint.el.  If you see that, 
you should definitely open a bug report!

> The 2nd issue looks to me like a more serious one, unless I'm missing
> something.  Is it possible to make sure we don't mistakenly take some
> innocent text as a password?  Did you try in your testing to type text
> that matches this regexp, and if so, what did you see as result?

I tried just after I read your message.  I don't find a problem.  And as
a pointed out above, it uses the same mechanism as comint.el (e.g. 
dumb shell buffers), so I don't think you should worry about it.

> If anyone can show just cause why this patch cannot lawfully be joined
> together in Emacs-26 branch, let them speak now or forever hold their
> peace.
I have booked all the celebration... :-S I hope I can marriage soon ;-)

bug#30190: 27.0.50; term run in line mode shows user passwords

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sat, 10 Mar 2018 19:44:13 +0900 (JST)
> cc: Tino Calancha <tino.calancha@gmail.com>, 30190@debbugs.gnu.org, 
>     rms@gnu.org, npostavs@users.sourceforge.net
> 
> > . it uses many constructs in term-password-prompt-regexp that could
> >   happen in unrelated text--does that mean such unrelated text will
> >   become invisible, thus making the session at least look buggy?
> Regarding this issue, it would behave exactly as a dumb shell buffer does
> M-x shell RET
> I've never seen a bug report about that for comint.el.  If you see that, 
> you should definitely open a bug report!
> 
> > The 2nd issue looks to me like a more serious one, unless I'm missing
> > something.  Is it possible to make sure we don't mistakenly take some
> > innocent text as a password?  Did you try in your testing to type text
> > that matches this regexp, and if so, what did you see as result?
> 
> I tried just after I read your message.  I don't find a problem.

Can you please show some examples?  First, what text triggers the new
functionality correctly, when the user types a password at some
relevant prompt, and then what happens when an unrelated prompt is
taken by the filter function as a prompt for a password.  I'd like to
understand better what happens in each case.

> And as a pointed out above, it uses the same mechanism as comint.el
> (e.g.  dumb shell buffers), so I don't think you should worry about
> it.

Sorry, this doesn't really tell me enough, because I don't think I
understand the relevance of dumb shells and comint to the issue at
hand.

Thanks.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:

>> I tried just after I read your message.  I don't find a problem.
>
> Can you please show some examples?  First, what text triggers the new
> functionality correctly, when the user types a password at some
> relevant prompt, and then what happens when an unrelated prompt is
> taken by the filter function as a prompt for a password.  I'd like to
> understand better what happens in each case.
Good behaviour:
sudo ls
# you are prompted in the minibuffer for your pass

Bad behaviour:
[sudo] password for foo:
# This throws 'command not found' BUT _sometimes_ you are prompted for
# your password in the minibuffer.
# Note: This happens in a dumb shell buffer as well.

>> And as a pointed out above, it uses the same mechanism as comint.el
>> (e.g.  dumb shell buffers), so I don't think you should worry about
>> it.
>
> Sorry, this doesn't really tell me enough, because I don't think I
> understand the relevance of dumb shells and comint to the issue at
> hand.

The relevance is that:
I have copied from comint.el how to recognize a password prompt to
redirect the prompt into the minibuffer (hidding the password).

Whatever misfunction of my patch should happen in a dumb shell buffer
started with:
M-x shell

IMO such side case is not an argument to reject this patch fixing
a serious thing.

I have uploaded a video running the above examples:
https://www.dropbox.com/s/onr7peue6xd5fqh/record-desktop.mkv?dl=01
(BTW, got an offer to be the next 007 right after upload this video.
Let's see.  I am considering it...)

bug#30190: 27.0.50; term run in line mode shows user passwords

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 30190@debbugs.gnu.org,  rms@gnu.org,  npostavs@users.sourceforge.net
> Date: Sat, 10 Mar 2018 22:17:13 +0900
> 
> Bad behaviour:
> [sudo] password for foo:
> # This throws 'command not found' BUT _sometimes_ you are prompted for
> # your password in the minibuffer.
> # Note: This happens in a dumb shell buffer as well.

What happens if you have a command (say, a shell script) that prompts
for something that is not a password with a prompt that starts with
text that matches the regexp -- what is the behavior then, after your
changes?  What I see here is that the filter redirects that to the
minibuffer, and doesn't show the text I type, unlike what happened
before your changes.  Wouldn't that look like a bug and cause bug
reports?

I'm also worried by the "_sometimes_" part: does it mean the behavior
is not deterministic?  Why?

> Whatever misfunction of my patch should happen in a dumb shell buffer
> started with:
> M-x shell

Yes, but two wrongs don't make a right...

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Sat, 10 Mar 2018, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Cc: 30190@debbugs.gnu.org,  rms@gnu.org,  npostavs@users.sourceforge.net
>> Date: Sat, 10 Mar 2018 22:17:13 +0900
>>
>> Bad behaviour:
>> [sudo] password for foo:
>> # This throws 'command not found' BUT _sometimes_ you are prompted for
>> # your password in the minibuffer.
>> # Note: This happens in a dumb shell buffer as well.
>
> What happens if you have a command (say, a shell script) that prompts
> for something that is not a password with a prompt that starts with
> text that matches the regexp -- what is the behavior then, after your
> changes?  What I see here is that the filter redirects that to the
> minibuffer, and doesn't show the text I type, unlike what happened
> before your changes.  Wouldn't that look like a bug and cause bug
> reports?
IMO, if the regexp is matched, then you must be prompt in the minibuffer.
It is the responsability of the person writing the script to chose
sensible variable names, and right prompts.  If I am prompted and I
expect I shouldn't, then what is happening is that I wrote a poor script.


> I'm also worried by the "_sometimes_" part: does it mean the behavior
> is not deterministic?  Why?
This is not crafted from me; it how it's designed in comint.el.  It must
mean that the long strings are send in chunks.  That would be a totally
independent bug report.  Actually if it's a bug or not is arguable:
don't think it is, at least until we canot fire it in a more sensical
example that the toy string:
[sudo] password for:

>> Whatever misfunction of my patch should happen in a dumb shell buffer
>> started with:
>> M-x shell
>
> Yes, but two wrongs don't make a right...
There levels of wrongs: showing a password is simply too wrong.

And 2 wrongs, sharing same code give more testers, i.e., more chances 
to detect the anomaly to finally fix it.

Anyway I already have patched my local sources and I am 
happy with that.  I don't have time to argue further, so I give up.
My team is pushing me to focus in our project.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sun, 11 Mar 2018 20:02:08 +0900 (JST)
> cc: Tino Calancha <tino.calancha@gmail.com>, 30190@debbugs.gnu.org, 
>     rms@gnu.org, npostavs@users.sourceforge.net
> 
> I don't have time to argue further, so I give up.

Too bad, because I wasn't arguing at all, just trying to figure out
how safe is this patch...

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

Tino Calancha <tino.calancha@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> You'll have to convince me that
>> 1. we really cannot live with the bug until Emacs 27.
> You can live with it.  Many people can live with it.  Indeed, this bug
> has been there since the addition of this lib. several releases before.

This patch missed 26.1, but perhaps we should consider it for 26.2?

bug#30190: 27.0.50; term run in line mode shows user passwords

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  30190@debbugs.gnu.org,  rms@gnu.org,  npostavs@users.sourceforge.net
> Date: Wed, 20 Jun 2018 00:09:22 -0400
> 
> This patch missed 26.1, but perhaps we should consider it for 26.2?

I'm for fixing this in Emacs 26.2, but I still don't think I
understand why the latest patch proposed in the discussion of this bug
needs to "steal" so much from comint.el?

Also, why does term-watch-for-password-prompt need to be invoked via a
hook?

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm for fixing this in Emacs 26.2, but I still don't think I
> understand why the latest patch proposed in the discussion of this bug
> needs to "steal" so much from comint.el?
>
> Also, why does term-watch-for-password-prompt need to be invoked via a
> hook?

I don't these things are really required; as far as I understand, Tino
did it that way in order to be safer: the "stealing" is to avoid loading
comint.el, and using the hook is to keep the code closer to the already
working example it's being copied from.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Wed, 20 Jun 2018, Noam Postavsky wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> I'm for fixing this in Emacs 26.2, but I still don't think I
>> understand why the latest patch proposed in the discussion of this bug
>> needs to "steal" so much from comint.el?
>>
>> Also, why does term-watch-for-password-prompt need to be invoked via a
>> hook?
>
> I don't these things are really required; as far as I understand, Tino
> did it that way in order to be safer: the "stealing" is to avoid loading
> comint.el, and using the hook is to keep the code closer to the already
> working example it's being copied from.
Exactly, I almost forgot this stuff.  Noam now understands my patch better
than me.  Feel free to adjust it as you wish, you are very smart.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: 30190@debbugs.gnu.org,  tino.calancha@gmail.com
> Date: Wed, 20 Jun 2018 19:28:32 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm for fixing this in Emacs 26.2, but I still don't think I
> > understand why the latest patch proposed in the discussion of this bug
> > needs to "steal" so much from comint.el?
> >
> > Also, why does term-watch-for-password-prompt need to be invoked via a
> > hook?
> 
> I don't these things are really required; as far as I understand, Tino
> did it that way in order to be safer: the "stealing" is to avoid loading
> comint.el, and using the hook is to keep the code closer to the already
> working example it's being copied from.

Why is it a problem to load comint?  Either in this case or even
always?

As for the hook: it looks strange to me to use hooks for this purpose,
since IMO we are supposed to refrain from doing that as much as
possible.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Thu, 21 Jun 2018, Eli Zaretskii wrote:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Cc: 30190@debbugs.gnu.org,  tino.calancha@gmail.com
>> Date: Wed, 20 Jun 2018 19:28:32 -0400
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>> I'm for fixing this in Emacs 26.2, but I still don't think I
>>> understand why the latest patch proposed in the discussion of this bug
>>> needs to "steal" so much from comint.el?
>>>
>>> Also, why does term-watch-for-password-prompt need to be invoked via a
>>> hook?
>>
>> I don't these things are really required; as far as I understand, Tino
>> did it that way in order to be safer: the "stealing" is to avoid loading
>> comint.el, and using the hook is to keep the code closer to the already
>> working example it's being copied from.
>
> Why is it a problem to load comint?  Either in this case or even
> always?
I have the bias/personal-preference to avoid load new things when I make a 
change.  Let's call it: 'disturb the least' with my patches.

> As for the hook: it looks strange to me to use hooks for this purpose,
> since IMO we are supposed to refrain from doing that as much as
> possible.
I must admit it: my patch brings cargo-cult from `comint.el'; comint.el 
performs such hook calls.

My patch just tried to mimic what is done in `comint.el' and reproduce
it in `term.el'.  My hope was that the patch would be accepted 
frictionless: if it's already done in `comint.el', 
why not doing the same in `term.el'?

The discussion turned out about point the implementation that we
have in `comint.el', which is also good and interesting topic.  A bit
out of scope of my initial intentions, but very welcome anyway.  Improve
code is always a good thing.

Stefan opinion on these 2 general questions might be very valuable.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Stefan Monnier]

> I have the bias/personal-preference to avoid load new things when
> I make a change.  Let's call it: 'disturb the least' with my patches.

I'd prefer to re-use comint code rather than copy it.  Copying might be
tolerable for term-watch-for-password-prompt (it could conceivably
require adjustments for the context of term-mode), but it's out of the
question for term-password-prompt-regexp.

> My patch just tried to mimic what is done in `comint.el' and reproduce
> it in `term.el'.  My hope was that the patch would be accepted frictionless:
> if it's already done in `comint.el', why not doing the same in `term.el'?

I'm not a great fan of the hooks in comint (tho I must say I haven't
come up with anything really better either).

And to the extent that you can set *-password-prompt-regexp
buffer-locally in order to prevent *-watch-for-password-prompt from
getting in the way, I don't see much benefit of going through a hook.
[ The hook might be beneficial in itself, with a nil default value, but
  that's a different discussion.  ]


        Stefan

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Thu, 21 Jun 2018, Stefan Monnier wrote:

> I'd prefer to re-use comint code rather than copy it.  Copying might be
> tolerable for term-watch-for-password-prompt (it could conceivably
> require adjustments for the context of term-mode), but it's out of the
> question for term-password-prompt-regexp.
>
>> My patch just tried to mimic what is done in `comint.el' and reproduce
>> it in `term.el'.  My hope was that the patch would be accepted frictionless:
>> if it's already done in `comint.el', why not doing the same in `term.el'?
>
> I'm not a great fan of the hooks in comint (tho I must say I haven't
> come up with anything really better either).
>
> And to the extent that you can set *-password-prompt-regexp
> buffer-locally in order to prevent *-watch-for-password-prompt from
> getting in the way, I don't see much benefit of going through a hook.
> [ The hook might be beneficial in itself, with a nil default value, but
>  that's a different discussion.  ]

Thank you for the feedback!

When my current mundane duties (cooking, studying, cleaning, grocery, 
jogging, take care of my cat and cactus, etc) offer me a break,
I will prepare a new patch (not sure when I will find the time, 
hopefully soon):

* It will recycle (require) `comint.el', i.e., it will use 
`comint-password-prompt-regexp'.

* It will use the hook strategy until someone comes with something 
smarter.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Stefan Monnier]

> * It will use the hook strategy until someone comes with something smarter.

Something smarter is to unconditionally call *-watch-for-password-prompt
right at the place where you added the "run-hook" (actually, only one
of the two places),


        Stefan

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

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

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> unconditionally call *-watch-for-password-prompt right at the place
> where you added the "run-hook" (actually, only one of the two places),

Here is a modified version of Tino's patch which uses comint.el and does
the above.  Another difference is that we call
term-watch-for-password-prompt on the decoded-substring (I believe
that's required to match localized non-ASCII prompts correctly).

I wasn't able to reproduce the problems described in #74 with any
version of the patch (maybe it's dependent on timing?), so I'm not sure
how much of a concern that is.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2749 bytes --]

From 5e8f7abce1b1dd9796e5baab57dc6215850b3416 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Thu, 15 Feb 2018 09:09:50 +0900
Subject: [PATCH v3] Prevent line-mode term from showing user passwords

For buffers whose mode derive from comint-mode, the user password is
read from the minibuffer and it's hidden.  A buffer in term-mode and
line submode, instead shows the passwords.  Make buffers in line
term-mode to hide passwords too (Bug#30190).

* lisp/term.el (term-send-invisible): Prefer the more robust
`read-passwd' instead of `term-read-noecho'.
(term-watch-for-password-prompt): New function.
(term-emulate-terminal): Call it each time we receive non-escape
sequence output.

Co-authored-by: Noam Postavsky <npostavs@gmail.com>
---
 lisp/term.el | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/lisp/term.el b/lisp/term.el
index b7f5b0e7f2..f7cd7dcd6a 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -347,6 +347,7 @@ term-protocol-version
 (eval-when-compile (require 'cl-lib))
 (require 'ring)
 (require 'ehelp)
+(require 'comint) ; Password regexp.
 
 (declare-function ring-empty-p "ring" (ring))
 (declare-function ring-ref "ring" (ring index))
@@ -2288,7 +2289,8 @@ term-send-invisible
 \\[view-lossage]."
   (interactive "P") ; Defeat snooping via C-x esc
   (when (not (stringp str))
-    (setq str (term-read-noecho "Non-echoed text: " t)))
+    (let ((read-hide-char ?*))
+      (setq str (read-passwd "Non-echoed text: "))))
   (when (not proc)
     (setq proc (get-buffer-process (current-buffer))))
   (if (not proc) (error "Current buffer has no process")
@@ -2297,6 +2299,17 @@ term-send-invisible
     (term-send-string proc str)
     (term-send-string proc "\n")))
 
+;; TODO: Maybe combine this with `comint-watch-for-password-prompt'.
+(defun term-watch-for-password-prompt (string)
+  "Prompt in the minibuffer for password and send without echoing.
+This function uses `term-send-invisible' to read and send a password to the buffer's
+process if STRING contains a password prompt defined by
+`comint-password-prompt-regexp'."
+  (when (term-in-line-mode)
+    (when (let ((case-fold-search t))
+	    (string-match comint-password-prompt-regexp string))
+      (term-send-invisible (read-passwd string)))))
+
 \f
 ;;; Low-level process communication
 
@@ -3152,6 +3165,9 @@ term-emulate-terminal
 	  (term-handle-deferred-scroll))
 
 	(set-marker (process-mark proc) (point))
+        (when (stringp decoded-substring)
+          (term-watch-for-password-prompt (prog1 decoded-substring
+                                            (setq decoded-substring nil))))
 	(when save-point
 	  (goto-char save-point)
 	  (set-marker save-point nil))
-- 
2.11.0

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

> Here is a modified version of Tino's patch which uses comint.el and does
> the above.
Thank you Noam!  Very happy to see this.
The patch hide my password nicely :-)
> Another difference is that we call
> term-watch-for-password-prompt on the decoded-substring (I believe
> that's required to match localized non-ASCII prompts correctly).
Good observation.

> I wasn't able to reproduce the problems described in #74 with any
> version of the patch (maybe it's dependent on timing?), so I'm not sure
> how much of a concern that is.
I follow the recipe in my video and I cannot reproduce the funny issue 
mentioned there either.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Stefan Monnier]

Thanks Noam, looks good.
Couldn't help send you some nitpicks, tho,


        Stefan


> @@ -2288,7 +2289,8 @@ term-send-invisible
>  \\[view-lossage]."
>    (interactive "P") ; Defeat snooping via C-x esc
>    (when (not (stringp str))
> -    (setq str (term-read-noecho "Non-echoed text: " t)))
> +    (let ((read-hide-char ?*))
> +      (setq str (read-passwd "Non-echoed text: "))))
>    (when (not proc)
>      (setq proc (get-buffer-process (current-buffer))))
>    (if (not proc) (error "Current buffer has no process")

Why do we need to bind `read-hide-char` here?
More specifically, shouldn't `read-passwd` do that for us (hence if it
doesn't yet, then the right patch is to add this let-binding to
`read-passwd`)?

> @@ -2297,6 +2299,17 @@ term-send-invisible
>      (term-send-string proc str)
>      (term-send-string proc "\n")))
>  
> +;; TODO: Maybe combine this with `comint-watch-for-password-prompt'.

Would be nice, yes.

> +(defun term-watch-for-password-prompt (string)
> +  "Prompt in the minibuffer for password and send without echoing.
> +This function uses `term-send-invisible' to read and send a password to the buffer's
> +process if STRING contains a password prompt defined by
> +`comint-password-prompt-regexp'."

"... uses `term-send-invisible' to read [...] a password ..." is
incorrect, since the password is read by `read-passwd` rather than by
term-send-invisible.  But in any case I don't see any reason to document
in the docstring what internal mechanism is used [ I just fixed the comint
version of the function accordingly.  ]

> @@ -3152,6 +3165,9 @@ term-emulate-terminal
>  	  (term-handle-deferred-scroll))
>  
>  	(set-marker (process-mark proc) (point))
> +        (when (stringp decoded-substring)
> +          (term-watch-for-password-prompt (prog1 decoded-substring
> +                                            (setq decoded-substring nil))))

I suggest you add a comment explaining why we set decoded-substring to nil.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

>> @@ -2288,7 +2289,8 @@ term-send-invisible
>>  \\[view-lossage]."
>>    (interactive "P") ; Defeat snooping via C-x esc
>>    (when (not (stringp str))
>> -    (setq str (term-read-noecho "Non-echoed text: " t)))
>> +    (let ((read-hide-char ?*))
>> +      (setq str (read-passwd "Non-echoed text: "))))
>>    (when (not proc)
>>      (setq proc (get-buffer-process (current-buffer))))
>>    (if (not proc) (error "Current buffer has no process")
>
> Why do we need to bind `read-hide-char` here?
I made this binding so that the user observes same characters (?*)
to hide the input as with command:
M-x  term-send-invisble RET

> More specifically, shouldn't `read-passwd` do that for us (hence if it
> doesn't yet, then the right patch is to add this let-binding to
> `read-passwd`)?
I don't think so.  `read-passwd' uses ?. as default.  The docstring 
suggest us to let-bind `read-hide-char' in case we wish another char.
Alternatively we could use ?. always as default, and change 
`term-send-invisble'.
Personaly, I prefer ?* because my vision is quite poor and ?. looks too 
small :-|


>> @@ -2297,6 +2299,17 @@ term-send-invisible
>>      (term-send-string proc str)
>>      (term-send-string proc "\n")))
>>
>> +;; TODO: Maybe combine this with `comint-watch-for-password-prompt'.
>
> Would be nice, yes.
Indeed, one of my favourite Beach Boys songs.

>> @@ -3152,6 +3165,9 @@ term-emulate-terminal
>>  	  (term-handle-deferred-scroll))
>>
>>  	(set-marker (process-mark proc) (point))
>> +        (when (stringp decoded-substring)
>> +          (term-watch-for-password-prompt (prog1 decoded-substring
>> +                                            (setq decoded-substring nil))))
>
> I suggest you add a comment explaining why we set decoded-substring to nil.
Agreed.  It's not obvious at first glance.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Stefan Monnier]

>> More specifically, shouldn't `read-passwd` do that for us (hence if it
>> doesn't yet, then the right patch is to add this let-binding to
>> `read-passwd`)?
> I don't think so.  `read-passwd' uses ?. as default.  The docstring suggest
> us to let-bind `read-hide-char' in case we wish another char.

But why does term-mode want to use a different char?
What's so different about term-mode?

> Alternatively we could use ?. always as default, and change
> `term-send-invisble'.

I don't understand what change to term-send-invisble you're thinking of.

> Personaly, I prefer ?* because my vision is quite poor and ?. looks too
> small :-|

But your vision is not poor only in term-mode, right?
So, what you're really saying here is that you'd like to change
read-passwd to use ?* instead of ?., isn't it?  If so, I have nothing
against it, but it's a separate concern from that of bug#30190 and it
should apply to all uses of read-passwd.


        Stefan

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Wed, 18 Jul 2018, Stefan Monnier wrote:

>>> More specifically, shouldn't `read-passwd` do that for us (hence if it
>>> doesn't yet, then the right patch is to add this let-binding to
>>> `read-passwd`)?
>> I don't think so.  `read-passwd' uses ?. as default.  The docstring suggest
>> us to let-bind `read-hide-char' in case we wish another char.
>
> But why does term-mode want to use a different char?
> What's so different about term-mode?
Of course, nothing.  I imagine it's for historical reasons; probably
someone introduced ?* at some point in term.el and nobody cared about it.

>> Alternatively we could use ?. always as default, and change
>> `term-send-invisble'.
>
> I don't understand what change to term-send-invisble you're thinking of.
I mean not passing non-nil 2nd argument here:
   (when (not (stringp str))
     (setq str (term-read-noecho "Non-echoed text: " t)))

;; Above code is from `term-send-invisible'.


>> Personaly, I prefer ?* because my vision is quite poor and ?. looks too
>> small :-|
>
> But your vision is not poor only in term-mode, right?
> So, what you're really saying here is that you'd like to change
> read-passwd to use ?* instead of ?., isn't it?  If so, I have nothing
> against it, but it's a separate concern from that of bug#30190 and it
> should apply to all uses of read-passwd.
Let's be realistic, these kind of changes usually are not welcome.  Not a 
problem though.  It's very minor issue and many people would love ?.

Since you look interested I tell a bit more; while I am introducing a
hidden text (usually a password), I count the number
of ?. to see if matches the length of the password.  This is a fast mental
check, don't bother to select the minibuffer contents and check its size.

I find easier to count ?* than ?.
But more than this personal issue from a handicapped person (visually), I
care more about the lack of consistency, as you do: yeah, we should
present uniformly the same char for any command hiding its input.
How to achieve that?  I am sure Eli find the proper way.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

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

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Couldn't help send you some nitpicks, tho,

Thanks for reviewing :)

>> @@ -2288,7 +2289,8 @@ term-send-invisible
>>  \\[view-lossage]."
>>    (interactive "P") ; Defeat snooping via C-x esc
>>    (when (not (stringp str))
>> -    (setq str (term-read-noecho "Non-echoed text: " t)))
>> +    (let ((read-hide-char ?*))
>> +      (setq str (read-passwd "Non-echoed text: "))))
>>    (when (not proc)
>>      (setq proc (get-buffer-process (current-buffer))))
>>    (if (not proc) (error "Current buffer has no process")
>
> Why do we need to bind `read-hide-char` here?
> More specifically, shouldn't `read-passwd` do that for us (hence if it
> doesn't yet, then the right patch is to add this let-binding to
> `read-passwd`)?

Tino mentioned "*" being more visible than ".", but poking at this a bit
more, I notice that term-read-noecho uses "*", so I guess that was the
original motivation.  I've dropped the read-hide-char binding, I think
it probably doesn't matter much either way.

Another thing I noticed is that read-passwd doesn't have the
view-lossage leak that term-read-noecho has, so I've removed that note
from the docstring.

>> +(defun term-watch-for-password-prompt (string)
>> +  "Prompt in the minibuffer for password and send without echoing.
>> +This function uses `term-send-invisible' to read and send a password to the buffer's
>> +process if STRING contains a password prompt defined by
>> +`comint-password-prompt-regexp'."

> I don't see any reason to document in the docstring what internal
> mechanism is used

Makes sense, I've trimmed the docstring.

>> @@ -3152,6 +3165,9 @@ term-emulate-terminal
>>  	  (term-handle-deferred-scroll))
>>  
>>  	(set-marker (process-mark proc) (point))
>> +        (when (stringp decoded-substring)
>> +          (term-watch-for-password-prompt (prog1 decoded-substring
>> +                                            (setq decoded-substring nil))))
>
> I suggest you add a comment explaining why we set decoded-substring to nil.

Ah, I carefully wrote a comment explaining why I did that, and then I
realized it was wrong.  There's not actually any need for it (I had got
a bit mixed up and thought we might loop around and prompt twice, but
this call is already after the loop).


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2917 bytes --]

From 429082a5e14abefb503d39390044b92cd2328462 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Thu, 15 Feb 2018 09:09:50 +0900
Subject: [PATCH v4] Prevent line-mode term from showing user passwords

For buffers whose mode derive from comint-mode, the user password is
read from the minibuffer and it's hidden.  A buffer in term-mode and
line submode, instead shows the passwords.  Make buffers in line
term-mode to hide passwords too (Bug#30190).

* lisp/term.el (term-send-invisible): Prefer the more robust
`read-passwd' instead of `term-read-noecho'.
(term-watch-for-password-prompt): New function.
(term-emulate-terminal): Call it each time we receive non-escape
sequence output.

Co-authored-by: Noam Postavsky <npostavs@gmail.com>
---
 lisp/term.el | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/lisp/term.el b/lisp/term.el
index b7f5b0e7f2..adbc0b0d88 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -347,6 +347,7 @@ term-protocol-version
 (eval-when-compile (require 'cl-lib))
 (require 'ring)
 (require 'ehelp)
+(require 'comint) ; Password regexp.
 
 (declare-function ring-empty-p "ring" (ring))
 (declare-function ring-ref "ring" (ring index))
@@ -2283,12 +2284,10 @@ term-read-noecho
 (defun term-send-invisible (str &optional proc)
   "Read a string without echoing.
 Then send it to the process running in the current buffer.  A new-line
-is additionally sent.  String is not saved on term input history list.
-Security bug: your string can still be temporarily recovered with
-\\[view-lossage]."
+is additionally sent.  String is not saved on term input history list."
   (interactive "P") ; Defeat snooping via C-x esc
   (when (not (stringp str))
-    (setq str (term-read-noecho "Non-echoed text: " t)))
+    (setq str (read-passwd "Non-echoed text: ")))
   (when (not proc)
     (setq proc (get-buffer-process (current-buffer))))
   (if (not proc) (error "Current buffer has no process")
@@ -2297,6 +2296,16 @@ term-send-invisible
     (term-send-string proc str)
     (term-send-string proc "\n")))
 
+;; TODO: Maybe combine this with `comint-watch-for-password-prompt'.
+(defun term-watch-for-password-prompt (string)
+  "Prompt in the minibuffer for password and send without echoing.
+Checks if STRING contains a password prompt as defined by
+`comint-password-prompt-regexp'."
+  (when (term-in-line-mode)
+    (when (let ((case-fold-search t))
+	    (string-match comint-password-prompt-regexp string))
+      (term-send-invisible (read-passwd string)))))
+
 \f
 ;;; Low-level process communication
 
@@ -3152,6 +3161,8 @@ term-emulate-terminal
 	  (term-handle-deferred-scroll))
 
 	(set-marker (process-mark proc) (point))
+        (when (stringp decoded-substring)
+          (term-watch-for-password-prompt decoded-substring))
 	(when save-point
 	  (goto-char save-point)
 	  (set-marker save-point nil))
-- 
2.11.0

bug#30190: 27.0.50; term run in line mode shows user passwords

[Stefan Monnier]

> Of course, nothing.  I imagine it's for historical reasons; probably
> someone introduced ?* at some point in term.el and nobody cared about it.

I think this ?* came before the ?. of read-passwd, actually.

> Let's be realistic, these kind of changes usually are not welcome.
> Not a problem though.  It's very minor issue and many people would love ?.

Could be.  I personally really don't have a preference, but I can
definitely see that ?* might be more legible, and hence preferable for
"rational" reasons rather than a mere preference.

And I'm having trouble imagining people being emotionally tied to ?.


        Stefan

bug#30190: 27.0.50; term run in line mode shows user passwords

[Stefan Monnier]

Thanks.

> -    (setq str (term-read-noecho "Non-echoed text: " t)))
> +    (setq str (read-passwd "Non-echoed text: ")))

I believe this remove the last use of term-read-noecho, so I think we
can then get rid of it (or at least mark it obsolete).


        Stefan

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> And I'm having trouble imagining people being emotionally tied to ?.

Yeah, ?* (?\x2A) probably won't be nearly controversial enough,
to really get people going we should propose a fancy character
like ?• (?\x2022) or ?● (?\x25CF).

bug#30190: 27.0.50; term run in line mode shows user passwords

[Stefan Monnier]

>> And I'm having trouble imagining people being emotionally tied to ?.
> Yeah, ?* (?\x2A) probably won't be nearly controversial enough,
> to really get people going we should propose a fancy character
> like ?• (?\x2022) or ?● (?\x25CF).

Now you're talking!


        Stefan

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

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



On Wed, 18 Jul 2018, Noam Postavsky wrote:

> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>
>> And I'm having trouble imagining people being emotionally tied to ?.
>
> Yeah, ?* (?\x2A) probably won't be nearly controversial enough,
> to really get people going we should propose a fancy character
> like ?• (?\x2022) or ?● (?\x25CF).
I will open a bug report with this wishlist (I mean ?*).

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> -    (setq str (term-read-noecho "Non-echoed text: " t)))
>> +    (setq str (read-passwd "Non-echoed text: ")))
>
> I believe this remove the last use of term-read-noecho, so I think we
> can then get rid of it (or at least mark it obsolete).

Makes sense.  I guess the obsoletion should go to master?

Eli, is the patch in #131 okay for emacs-26?

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30190#131

bug#30190: 27.0.50; term run in line mode shows user passwords

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  30190@debbugs.gnu.org,  Tino Calancha <tino.calancha@gmail.com>
> Date: Sun, 22 Jul 2018 14:33:18 -0400
> 
> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> 
> >> -    (setq str (term-read-noecho "Non-echoed text: " t)))
> >> +    (setq str (read-passwd "Non-echoed text: ")))
> >
> > I believe this remove the last use of term-read-noecho, so I think we
> > can then get rid of it (or at least mark it obsolete).
> 
> Makes sense.  I guess the obsoletion should go to master?
> 
> Eli, is the patch in #131 okay for emacs-26?
> 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30190#131

Yes, thanks.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

tags 30190 fixed
close 30190 26.2
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  30190@debbugs.gnu.org,  Tino Calancha <tino.calancha@gmail.com>
>> Date: Sun, 22 Jul 2018 14:33:18 -0400
>> 
>> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>> 
>> >> -    (setq str (term-read-noecho "Non-echoed text: " t)))
>> >> +    (setq str (read-passwd "Non-echoed text: ")))
>> >
>> > I believe this remove the last use of term-read-noecho, so I think we
>> > can then get rid of it (or at least mark it obsolete).
>> 
>> Makes sense.  I guess the obsoletion should go to master?
>> 
>> Eli, is the patch in #131 okay for emacs-26?
>> 
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30190#131
>
> Yes, thanks.

Pushed to emacs-26.

[1: 2b70b54739]: 2018-07-23 08:20:07 -0400
  Prevent line-mode term from showing user passwords
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=2b70b54739a8a422aff85f0183fb69eb339c35d4

bug#30190: 27.0.50; term run in line mode shows user passwords

[Stefan Monnier]

> Makes sense.  I guess the obsoletion should go to master?

FWIW, I think obsoletions are a form of documentation, so the earlier we
announce them, the better.  IOW if the patch goes to emacs-26, then the
obsoletion should go there as well.


        Stefan

bug#30190: 27.0.50; term run in line mode shows user passwords

[Tino Calancha]

On Mon, 23 Jul 2018, Stefan Monnier wrote:

>> Makes sense.  I guess the obsoletion should go to master?
>
> FWIW, I think obsoletions are a form of documentation, so the earlier we
> announce them, the better.  IOW if the patch goes to emacs-26, then the
> obsoletion should go there as well.

Good point.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

Tino Calancha <tino.calancha@gmail.com> writes:

> On Mon, 23 Jul 2018, Stefan Monnier wrote:
>
>>> Makes sense.  I guess the obsoletion should go to master?
>>
>> FWIW, I think obsoletions are a form of documentation, so the earlier we
>> announce them, the better.  IOW if the patch goes to emacs-26, then the
>> obsoletion should go there as well.
>
> Good point.

This sounds good in theory, although in practice people tend to treat it
as more than just documentation (e.g., Bug#30039).

On the other hand, a search for "term-read-noecho" on the web gets 9
hits, all of which are either this bug thread, or copies of
term.el/Emacs commit logs.  So maybe this is all theoretical anyway...

bug#30190: 27.0.50; term run in line mode shows user passwords

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  30190@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 23 Jul 2018 20:28:27 -0400
> 
> Tino Calancha <tino.calancha@gmail.com> writes:
> 
> > On Mon, 23 Jul 2018, Stefan Monnier wrote:
> >
> >>> Makes sense.  I guess the obsoletion should go to master?
> >>
> >> FWIW, I think obsoletions are a form of documentation, so the earlier we
> >> announce them, the better.  IOW if the patch goes to emacs-26, then the
> >> obsoletion should go there as well.
> >
> > Good point.
> 
> This sounds good in theory, although in practice people tend to treat it
> as more than just documentation (e.g., Bug#30039).
> 
> On the other hand, a search for "term-read-noecho" on the web gets 9
> hits, all of which are either this bug thread, or copies of
> term.el/Emacs commit logs.  So maybe this is all theoretical anyway...

My POV on this is that we shouldn't in general deprecate anything in a
bugfix release.  However, I agree that this particular case is not
really important to justify bikeshedding, so I won't holler if we did
it this time.

bug#30190: 27.0.50; term run in line mode shows user passwords

[Noam Postavsky]

>> >>> Makes sense.  I guess the obsoletion should go to master?
>> >>
>> >> FWIW, I think obsoletions are a form of documentation, so the earlier we
>> >> announce them, the better.  IOW if the patch goes to emacs-26, then the
>> >> obsoletion should go there as well.
>> >
>> > Good point.
>> 
>> This sounds good in theory, although in practice people tend to treat it
>> as more than just documentation (e.g., Bug#30039).
>> 
>> On the other hand, a search for "term-read-noecho" on the web gets 9
>> hits, all of which are either this bug thread, or copies of
>> term.el/Emacs commit logs.  So maybe this is all theoretical anyway...
>
> My POV on this is that we shouldn't in general deprecate anything in a
> bugfix release.  However, I agree that this particular case is not
> really important to justify bikeshedding, so I won't holler if we did
> it this time.

Well, I've pushed the obsoletion to master.

[1: bd36ab560d]: 2018-07-29 21:13:48 -0400
  * lisp/term.el (term-read-noecho): Mark obsolete.
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=bd36ab560d5efcc5853e455c2312cf1a104e78ea