GNU bug report logs -
#30190
27.0.50; term run in line mode shows user passwords
Previous Next
Reported by: Tino Calancha <tino.calancha <at> gmail.com>
Date: Sun, 21 Jan 2018 12:17:02 UTC
Severity: normal
Tags: confirmed, fixed, security
Found in versions 27.0.50, 24.3
Fixed in version 26.2
Done: Noam Postavsky <npostavs <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 30190 in the body.
You can then email your comments to 30190 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 21 Jan 2018 12:17:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Tino Calancha <tino.calancha <at> gmail.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Sun, 21 Jan 2018 12:17:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
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))
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 21 Jan 2018 14:02:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 30190 <at> debbugs.gnu.org (full text, mbox):
found 30190 24.3
tags 30190 + confirmed
quit
Tino Calancha <tino.calancha <at> 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 Marked as found in versions 24.3.
Request was from
Noam Postavsky <npostavs <at> users.sourceforge.net>
to
control <at> debbugs.gnu.org.
(Sun, 21 Jan 2018 14:02:03 GMT)
Full text and
rfc822 format available.
Added tag(s) confirmed.
Request was from
Noam Postavsky <npostavs <at> users.sourceforge.net>
to
control <at> debbugs.gnu.org.
(Sun, 21 Jan 2018 14:02:03 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 21 Jan 2018 21:09:01 GMT)
Full text and
rfc822 format available.
Message #15 received at 30190 <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sat, 03 Feb 2018 16:17:01 GMT)
Full text and
rfc822 format available.
Message #18 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Noam Postavsky <npostavs <at> 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 <at> 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)
;; 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 <at> GNU.ORG: '.
-;; ksu prints a prompt like `Kerberos password for devnull/root <at> GNU.ORG: '.
-;; ssh-add prints a prompt like `Enter passphrase: '.
-;; plink prints a prompt like `Passphrase for key "root <at> 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)
+
;; 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 <at> 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:
+
+
+;; 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 <at> GNU.ORG: '.
+;; ksu prints a prompt like `Kerberos password for devnull/root <at> GNU.ORG: '.
+;; ssh-add prints a prompt like `Enter passphrase: '.
+;; plink prints a prompt like `Passphrase for key "root <at> 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---
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sat, 03 Feb 2018 16:45:02 GMT)
Full text and
rfc822 format available.
Message #21 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Tino Calancha <tino.calancha <at> gmail.com> writes:
> Noam Postavsky <npostavs <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sat, 03 Feb 2018 17:10:02 GMT)
Full text and
rfc822 format available.
Message #24 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> From: Tino Calancha <tino.calancha <at> gmail.com>
> Date: Sun, 04 Feb 2018 01:15:54 +0900
> Cc: 30190 <at> debbugs.gnu.org
>
> Noam Postavsky <npostavs <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 04 Feb 2018 02:24:01 GMT)
Full text and
rfc822 format available.
Message #27 received at 30190 <at> debbugs.gnu.org (full text, mbox):
On Sat, 3 Feb 2018, Noam Postavsky wrote:
> Tino Calancha <tino.calancha <at> gmail.com> writes:
>
>> Noam Postavsky <npostavs <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 04 Feb 2018 02:27:02 GMT)
Full text and
rfc822 format available.
Message #30 received at 30190 <at> debbugs.gnu.org (full text, mbox):
On Sat, 3 Feb 2018, Eli Zaretskii wrote:
>> From: Tino Calancha <tino.calancha <at> gmail.com>
>> Date: Sun, 04 Feb 2018 01:15:54 +0900
>> Cc: 30190 <at> debbugs.gnu.org
>>
>> Noam Postavsky <npostavs <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 04 Feb 2018 02:30:03 GMT)
Full text and
rfc822 format available.
Message #33 received at 30190 <at> debbugs.gnu.org (full text, mbox):
On Sat, Feb 3, 2018 at 9:23 PM, Tino Calancha <tino.calancha <at> 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)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 04 Feb 2018 03:38:01 GMT)
Full text and
rfc822 format available.
Message #36 received at 30190 <at> debbugs.gnu.org (full text, mbox):
On Sat, 3 Feb 2018, Noam Postavsky wrote:
> On Sat, Feb 3, 2018 at 9:23 PM, Tino Calancha <tino.calancha <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 04 Feb 2018 03:41:01 GMT)
Full text and
rfc822 format available.
Message #39 received at 30190 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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 <at> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 04 Feb 2018 12:41:01 GMT)
Full text and
rfc822 format available.
Message #42 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Tino Calancha <tino.calancha <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 04 Feb 2018 12:49:01 GMT)
Full text and
rfc822 format available.
Message #45 received at 30190 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Sun, 4 Feb 2018, Noam Postavsky wrote:
> Tino Calancha <tino.calancha <at> 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 <at> 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---
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Mon, 05 Feb 2018 01:08:02 GMT)
Full text and
rfc822 format available.
Message #48 received at 30190 <at> debbugs.gnu.org (full text, mbox):
[[[ 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.
Added tag(s) security.
Request was from
Tino Calancha <tino.calancha <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Mon, 05 Feb 2018 06:39:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Thu, 15 Feb 2018 00:11:02 GMT)
Full text and
rfc822 format available.
Message #53 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Tino Calancha <tino.calancha <at> 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 <at> 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))))
+
;;; 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)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 21 Feb 2018 10:19:01 GMT)
Full text and
rfc822 format available.
Message #56 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Tino Calancha <tino.calancha <at> gmail.com> writes:
> Tino Calancha <tino.calancha <at> gmail.com> writes:
>
>
> This is the updated patch:
>
> commit 6254b0aca3c91ebd6d41f865c1cdcc13166c066d
> Author: tino calancha <tino.calancha <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 21 Feb 2018 17:48:02 GMT)
Full text and
rfc822 format available.
Message #59 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> From: Tino Calancha <tino.calancha <at> gmail.com>
> Cc: 30190 <at> debbugs.gnu.org, Richard Stallman <rms <at> gnu.org>, npostavs <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sat, 10 Mar 2018 08:53:01 GMT)
Full text and
rfc822 format available.
Message #62 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Tino Calancha <tino.calancha <at> gmail.com>
>> Cc: 30190 <at> debbugs.gnu.org, Richard Stallman <rms <at> gnu.org>, npostavs <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sat, 10 Mar 2018 10:26:02 GMT)
Full text and
rfc822 format available.
Message #65 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> From: Tino Calancha <tino.calancha <at> gmail.com>
> Cc: 30190 <at> debbugs.gnu.org, rms <at> gnu.org, npostavs <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sat, 10 Mar 2018 10:45:01 GMT)
Full text and
rfc822 format available.
Message #68 received at 30190 <at> debbugs.gnu.org (full text, mbox):
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 ;-)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sat, 10 Mar 2018 12:08:02 GMT)
Full text and
rfc822 format available.
Message #71 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> From: Tino Calancha <tino.calancha <at> gmail.com>
> Date: Sat, 10 Mar 2018 19:44:13 +0900 (JST)
> cc: Tino Calancha <tino.calancha <at> gmail.com>, 30190 <at> debbugs.gnu.org,
> rms <at> gnu.org, npostavs <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sat, 10 Mar 2018 13:18:01 GMT)
Full text and
rfc822 format available.
Message #74 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> 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...)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sat, 10 Mar 2018 15:52:02 GMT)
Full text and
rfc822 format available.
Message #77 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> From: Tino Calancha <tino.calancha <at> gmail.com>
> Cc: 30190 <at> debbugs.gnu.org, rms <at> gnu.org, npostavs <at> 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...
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 11 Mar 2018 11:03:02 GMT)
Full text and
rfc822 format available.
Message #80 received at 30190 <at> debbugs.gnu.org (full text, mbox):
On Sat, 10 Mar 2018, Eli Zaretskii wrote:
>> From: Tino Calancha <tino.calancha <at> gmail.com>
>> Cc: 30190 <at> debbugs.gnu.org, rms <at> gnu.org, npostavs <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 11 Mar 2018 17:06:01 GMT)
Full text and
rfc822 format available.
Message #83 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> From: Tino Calancha <tino.calancha <at> gmail.com>
> Date: Sun, 11 Mar 2018 20:02:08 +0900 (JST)
> cc: Tino Calancha <tino.calancha <at> gmail.com>, 30190 <at> debbugs.gnu.org,
> rms <at> gnu.org, npostavs <at> 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...
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 20 Jun 2018 04:10:02 GMT)
Full text and
rfc822 format available.
Message #86 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Tino Calancha <tino.calancha <at> gmail.com> writes:
> Eli Zaretskii <eliz <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 20 Jun 2018 16:28:02 GMT)
Full text and
rfc822 format available.
Message #89 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> From: Noam Postavsky <npostavs <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 30190 <at> debbugs.gnu.org, rms <at> gnu.org, npostavs <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 20 Jun 2018 23:29:01 GMT)
Full text and
rfc822 format available.
Message #92 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Thu, 21 Jun 2018 01:32:02 GMT)
Full text and
rfc822 format available.
Message #95 received at 30190 <at> debbugs.gnu.org (full text, mbox):
On Wed, 20 Jun 2018, Noam Postavsky wrote:
> Eli Zaretskii <eliz <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Thu, 21 Jun 2018 02:45:02 GMT)
Full text and
rfc822 format available.
Message #98 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> From: Noam Postavsky <npostavs <at> gmail.com>
> Cc: 30190 <at> debbugs.gnu.org, tino.calancha <at> gmail.com
> Date: Wed, 20 Jun 2018 19:28:32 -0400
>
> Eli Zaretskii <eliz <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Thu, 21 Jun 2018 03:09:04 GMT)
Full text and
rfc822 format available.
Message #101 received at 30190 <at> debbugs.gnu.org (full text, mbox):
On Thu, 21 Jun 2018, Eli Zaretskii wrote:
>> From: Noam Postavsky <npostavs <at> gmail.com>
>> Cc: 30190 <at> debbugs.gnu.org, tino.calancha <at> gmail.com
>> Date: Wed, 20 Jun 2018 19:28:32 -0400
>>
>> Eli Zaretskii <eliz <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Thu, 21 Jun 2018 19:18:01 GMT)
Full text and
rfc822 format available.
Message #104 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Fri, 22 Jun 2018 03:35:02 GMT)
Full text and
rfc822 format available.
Message #107 received at 30190 <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Fri, 22 Jun 2018 12:46:01 GMT)
Full text and
rfc822 format available.
Message #110 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> * 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 18 Jul 2018 11:57:01 GMT)
Full text and
rfc822 format available.
Message #113 received at 30190 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Stefan Monnier <monnier <at> 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.
[v3-0001-Prevent-line-mode-term-from-showing-user-password.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 18 Jul 2018 12:33:01 GMT)
Full text and
rfc822 format available.
Message #116 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 18 Jul 2018 14:25:02 GMT)
Full text and
rfc822 format available.
Message #119 received at 30190 <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 18 Jul 2018 14:57:02 GMT)
Full text and
rfc822 format available.
Message #122 received at 30190 <at> debbugs.gnu.org (full text, mbox):
>> @@ -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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 18 Jul 2018 15:55:02 GMT)
Full text and
rfc822 format available.
Message #125 received at 30190 <at> debbugs.gnu.org (full text, mbox):
>> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Wed, 18 Jul 2018 23:29:01 GMT)
Full text and
rfc822 format available.
Message #128 received at 30190 <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Thu, 19 Jul 2018 00:03:01 GMT)
Full text and
rfc822 format available.
Message #131 received at 30190 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Stefan Monnier <monnier <at> 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).
[v4-0001-Prevent-line-mode-term-from-showing-user-password.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Thu, 19 Jul 2018 01:59:01 GMT)
Full text and
rfc822 format available.
Message #134 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Thu, 19 Jul 2018 02:01:02 GMT)
Full text and
rfc822 format available.
Message #137 received at 30190 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Thu, 19 Jul 2018 02:28:02 GMT)
Full text and
rfc822 format available.
Message #140 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> 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).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Thu, 19 Jul 2018 12:46:01 GMT)
Full text and
rfc822 format available.
Message #143 received at 30190 <at> debbugs.gnu.org (full text, mbox):
>> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Fri, 20 Jul 2018 07:35:02 GMT)
Full text and
rfc822 format available.
Message #146 received at 30190 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Wed, 18 Jul 2018, Noam Postavsky wrote:
> Stefan Monnier <monnier <at> 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 ?*).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 22 Jul 2018 18:34:01 GMT)
Full text and
rfc822 format available.
Message #149 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Sun, 22 Jul 2018 18:45:02 GMT)
Full text and
rfc822 format available.
Message #152 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> From: Noam Postavsky <npostavs <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 30190 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
> Date: Sun, 22 Jul 2018 14:33:18 -0400
>
> Stefan Monnier <monnier <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Mon, 23 Jul 2018 12:23:02 GMT)
Full text and
rfc822 format available.
Message #155 received at 30190 <at> debbugs.gnu.org (full text, mbox):
tags 30190 fixed
close 30190 26.2
quit
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Noam Postavsky <npostavs <at> gmail.com>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>, 30190 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
>> Date: Sun, 22 Jul 2018 14:33:18 -0400
>>
>> Stefan Monnier <monnier <at> 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
Added tag(s) fixed.
Request was from
Noam Postavsky <npostavs <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Mon, 23 Jul 2018 12:23:02 GMT)
Full text and
rfc822 format available.
bug marked as fixed in version 26.2, send any further explanations to
30190 <at> debbugs.gnu.org and Tino Calancha <tino.calancha <at> gmail.com>
Request was from
Noam Postavsky <npostavs <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Mon, 23 Jul 2018 12:23:03 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Mon, 23 Jul 2018 12:47:01 GMT)
Full text and
rfc822 format available.
Message #162 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Mon, 23 Jul 2018 12:57:02 GMT)
Full text and
rfc822 format available.
Message #165 received at 30190 <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Tue, 24 Jul 2018 00:29:02 GMT)
Full text and
rfc822 format available.
Message #168 received at 30190 <at> debbugs.gnu.org (full text, mbox):
Tino Calancha <tino.calancha <at> 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...
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Tue, 24 Jul 2018 02:36:02 GMT)
Full text and
rfc822 format available.
Message #171 received at 30190 <at> debbugs.gnu.org (full text, mbox):
> From: Noam Postavsky <npostavs <at> gmail.com>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 30190 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> Date: Mon, 23 Jul 2018 20:28:27 -0400
>
> Tino Calancha <tino.calancha <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#30190; Package
emacs.
(Mon, 30 Jul 2018 01:16:01 GMT)
Full text and
rfc822 format available.
Message #174 received at 30190 <at> debbugs.gnu.org (full text, mbox):
>> >>> 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
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Mon, 27 Aug 2018 11:24:06 GMT)
Full text and
rfc822 format available.
This bug report was last modified 5 years and 235 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.