GNU bug report logs - #30190
27.0.50; term run in line mode shows user passwords

Previous Next

Full log

Message #131 received at 30190 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 30190 <at> debbugs.gnu.org,
 Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#30190: 27.0.50; term run in line mode shows user passwords
Date: Wed, 18 Jul 2018 20:02:12 -0400

[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)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.