bug#17127: `call-process' circumvents password concealment w/ `read-passwd'

bug#17127: `call-process' circumvents password concealment w/ `read-passwd'

[Nathan Trapuzzano]

To reproduce with emacs -nw -q on 24.3 and trunk:

  M-: (global-set-key
        (kbd "C-c C-c")
        (lambda ()
          (interactive)
          (call-process "echo" nil t nil "-n" "foobar")))

  M-: (read-passwd "Password: ")

  C-c C-c

"foobar" is printed in the minibuffer rather than "......", whereas,
e.g., yanking from the kill ring print dots.

bug#17127: `call-process' circumvents password concealment w/ `read-passwd'

[Stefan Monnier]

> To reproduce with emacs -nw -q on 24.3 and trunk:

>   M-: (global-set-key
>         (kbd "C-c C-c")
>         (lambda ()
>           (interactive)
>           (call-process "echo" nil t nil "-n" "foobar")))

>   M-: (read-passwd "Password: ")

>   C-c C-c

This looks fairly contrived.  How did you stumble upon this problem?


        Stefan

bug#17127: `call-process' circumvents password concealment w/ `read-passwd'

[Nathan Trapuzzano]

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

> This looks fairly contrived.  How did you stumble upon this problem?

Copy/pasting passwords from console password manager to emacs running on
terminal emulator in X.  The built-in copy/paste functionaly for the X
clipboard only works (AFAIK) with graphical emacs, so I use my own
commands to make it work on a terminal.  Here's the one that made me
catch it:

(defun paste-from-X-clipboard ()
  "Insert the X clipboard contents at point."
  (interactive)
  (call-process "xclip" nil t nil "-selection" "clipboard" "-o"))

I use that to paste passwords when, e.g., finding remote files via
ssh/TRAMP.

bug#17127: `call-process' circumvents password concealment w/ `read-passwd'

[Lars Ingebrigtsen]

Nathan Trapuzzano <nbtrap@nbtrap.com> writes:

> To reproduce with emacs -nw -q on 24.3 and trunk:
>
>   M-: (global-set-key
>         (kbd "C-c C-c")
>         (lambda ()
>           (interactive)
>           (call-process "echo" nil t nil "-n" "foobar")))
>
>   M-: (read-passwd "Password: ")
>
>   C-c C-c
>
> "foobar" is printed in the minibuffer rather than "......", whereas,
> e.g., yanking from the kill ring print dots.

The following patch fixes this, I think, by using post-command-hook
instead of after-change-functions.

It seems to work for me -- does anybody see a problem with doing it this
way?

diff --git a/lisp/subr.el b/lisp/subr.el
index 45b99a82d2..9e4553dcbb 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2426,6 +2426,12 @@ read-passwd-map
     map)
   "Keymap used while reading passwords.")
 
+(defun read-password--hide-password ()
+  (let ((beg (minibuffer-prompt-end)))
+             (dotimes (i (1+ (- (buffer-size) beg)))
+               (put-text-property (+ i beg) (+ 1 i beg)
+                                  'display (string (or read-hide-char ?*))))))
+
 (defun read-passwd (prompt &optional confirm default)
   "Read a password, prompting with PROMPT, and return it.
 If optional CONFIRM is non-nil, read the password twice to make sure.
@@ -2450,15 +2456,7 @@ read-passwd
               (message "Password not repeated accurately; please start over")
               (sit-for 1))))
         success)
-    (let ((hide-chars-fun
-           (lambda (beg end _len)
-             (clear-this-command-keys)
-             (setq beg (min end (max (minibuffer-prompt-end)
-                                     beg)))
-             (dotimes (i (- end beg))
-               (put-text-property (+ i beg) (+ 1 i beg)
-                                  'display (string (or read-hide-char ?*))))))
-          minibuf)
+    (let (minibuf)
       (minibuffer-with-setup-hook
           (lambda ()
             (setq minibuf (current-buffer))
@@ -2469,7 +2467,7 @@ read-passwd
             (use-local-map read-passwd-map)
             (setq-local inhibit-modification-hooks nil) ;bug#15501.
 	    (setq-local show-paren-mode nil)		;bug#16091.
-            (add-hook 'after-change-functions hide-chars-fun nil 'local))
+            (add-hook 'post-command-hook 'read-password--hide-password nil t))
         (unwind-protect
             (let ((enable-recursive-minibuffers t)
 		  (read-hide-char (or read-hide-char ?*)))
@@ -2479,7 +2477,8 @@ read-passwd
               ;; Not sure why but it seems that there might be cases where the
               ;; minibuffer is not always properly reset later on, so undo
               ;; whatever we've done here (bug#11392).
-              (remove-hook 'after-change-functions hide-chars-fun 'local)
+              (remove-hook 'after-change-functions 'read-password--hide-password
+                           'local)
               (kill-local-variable 'post-self-insert-hook)
               ;; And of course, don't keep the sensitive data around.
               (erase-buffer))))))))


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#17127: `call-process' circumvents password concealment w/ `read-passwd'

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> "foobar" is printed in the minibuffer rather than "......", whereas,
>> e.g., yanking from the kill ring print dots.
>
> The following patch fixes this, I think, by using post-command-hook
> instead of after-change-functions.
>
> It seems to work for me -- does anybody see a problem with doing it this
> way?

There were no comments in two weeks, so I've now applied the patch.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#17127: `call-process' circumvents password concealment w/ `read-passwd'

[Stefan Monnier]

> The following patch fixes this, I think, by using post-command-hook
> instead of after-change-functions.

Actually, in theory after-change-functions should catch all cases
whereas post-command-hook might miss some (i.e. chars inserted not
while running a command, e.g. from a process filter).

So while your new code probably works fine in practice (and is a good
workaround for now) , I think the original code is "more correct" and we
should try and figure out why it didn't work: how come
after-change-functions is not run (or not correctly) by call-process?


        Stefan

bug#17127: `call-process' circumvents password concealment w/ `read-passwd'

[Lars Ingebrigtsen]

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

>> The following patch fixes this, I think, by using post-command-hook
>> instead of after-change-functions.
>
> Actually, in theory after-change-functions should catch all cases
> whereas post-command-hook might miss some (i.e. chars inserted not
> while running a command, e.g. from a process filter).
>
> So while your new code probably works fine in practice (and is a good
> workaround for now) , I think the original code is "more correct" and we
> should try and figure out why it didn't work: how come
> after-change-functions is not run (or not correctly) by call-process?

Yeah, that's a good point.  Data inserted by call-process definitely
changes the buffer, so after-change-functions should be run.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no