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

[Noam Postavsky <npostavs@gmail.com>]

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

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

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

Thanks for reviewing :)

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

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

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

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

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

Makes sense, I've trimmed the docstring.

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

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


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

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

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

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

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

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