bug#30186: 27.0.50; Password is not hidden in read-passwd

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Juri Linkov]

This is a regression and a security flaw.

Reading a password with ‘read-passwd’ doesn't hide inserted characters
anymore as it used to do in older versions.

When the user has such customization:

  (custom-set-variables
   '(yank-excluded-properties t))

evaluating

  (read-passwd "Prompt: ")

and yanking a password to the minibuffer with 'C-y' doesn't hide it
as it did in Emacs 25.

This can be traced down to ‘remove-yank-excluded-properties’
where ‘set-text-properties’ used to leave ‘display’ properties
(with ‘.’ over inserted characters) in the minibuffer.

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Sat, 20 Jan 2018 23:29:35 +0200
> 
> This is a regression and a security flaw.
> 
> Reading a password with ‘read-passwd’ doesn't hide inserted characters
> anymore as it used to do in older versions.
> 
> When the user has such customization:
> 
>   (custom-set-variables
>    '(yank-excluded-properties t))
> 
> evaluating
> 
>   (read-passwd "Prompt: ")
> 
> and yanking a password to the minibuffer with 'C-y' doesn't hide it
> as it did in Emacs 25.
> 
> This can be traced down to ‘remove-yank-excluded-properties’
> where ‘set-text-properties’ used to leave ‘display’ properties
> (with ‘.’ over inserted characters) in the minibuffer.

Can you spot the commit which causes this, or bisect to find it?

Thanks.

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Juri Linkov]

>> This can be traced down to ‘remove-yank-excluded-properties’
>> where ‘set-text-properties’ used to leave ‘display’ properties
>> (with ‘.’ over inserted characters) in the minibuffer.
>
> Can you spot the commit which causes this, or bisect to find it?

I'm trying to find a relevant commit in the git log manually
because bisect is not feasible for me - ‘make’ takes more than
one hour to recompile.  Is there a service like Travis CI that
could do bisect and compile its commits and check for exit codes
of the provided script containing the required condition?

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Glenn Morris]

Juri Linkov wrote:

> and yanking a password to the minibuffer with 'C-y' doesn't hide it
> as it did in Emacs 25.

In my tests, this stopped working in Emacs 25.1.

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Glenn Morris]

Juri Linkov wrote:

> I'm trying to find a relevant commit in the git log manually
> because bisect is not feasible for me - 'make' takes more than
> one hour to recompile.

Ouch. Some tips:
0) Keep old versions around to identify start/end ranges
1) Use a configure cache (one that does not get cleaned)
2) Do a minimal build. I use: make -C lib all; make -C src emacs
3) M0r3 cor3z
(About 90 seconds per revision for me with this approach.)

Anyway, I bisected it for you, to 
c5e89be20a3feba9c67be6855b1dbdc6d8ae5ce2

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Glenn Morris]

Glenn Morris wrote:

> Anyway, I bisected it for you, to 
> c5e89be20a3feba9c67be6855b1dbdc6d8ae5ce2

Here's the discussion reference that should have been in the commit message:

http://lists.gnu.org/r/emacs-devel/2015-04/msg00506.html

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Juri Linkov]

>> Anyway, I bisected it for you, to
>> c5e89be20a3feba9c67be6855b1dbdc6d8ae5ce2

Thank you very much, I was considering creating a service that
would read git-bisect params in a webform and run a multi-core
VM instance, but with your tips it should be doable locally.

> Here's the discussion reference that should have been in the commit message:
>
> http://lists.gnu.org/r/emacs-devel/2015-04/msg00506.html

I confirm that after removing with-silent-modifications from
remove-yank-excluded-properties, C-y leaves ‘display’ properties
in the minibuffer.  It's ‘(inhibit-modification-hooks t)’ in
let-bind of with-silent-modifications that prevents read-passwd
from calling hide-chars-fun again to put new ‘display’ properties
after set-text-properties removes old ones.  So this patch should
fix this issue, but I'm not sure if this is right.

diff --git a/lisp/subr.el b/lisp/subr.el
index 092850a..8673547 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3041,7 +3041,8 @@ remove-yank-excluded-properties
             (setq run-start run-end)))))
     (with-silent-modifications
       (if (eq yank-excluded-properties t)
-          (set-text-properties start end nil)
+          (let ((inhibit-modification-hooks nil))
+            (set-text-properties start end nil))
         (remove-list-of-text-properties start end yank-excluded-properties)))))
 
 (defvar yank-undo-function)

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Juri Linkov]

Hi Alan,

Do you agree we could remove the effect of
with-silent-modifications around set-text-properties,
and leave it only on remove-list-of-text-properties?
This will help to fix the reported regression.

>>> Anyway, I bisected it for you, to
>>> c5e89be20a3feba9c67be6855b1dbdc6d8ae5ce2
>
>> Here's the discussion reference that should have been in the commit message:
>>
>> http://lists.gnu.org/r/emacs-devel/2015-04/msg00506.html
>
> I confirm that after removing with-silent-modifications from
> remove-yank-excluded-properties, C-y leaves ‘display’ properties
> in the minibuffer.  It's ‘(inhibit-modification-hooks t)’ in
> let-bind of with-silent-modifications that prevents read-passwd
> from calling hide-chars-fun again to put new ‘display’ properties
> after set-text-properties removes old ones.  So this patch should
> fix this issue, but I'm not sure if this is right.
>
> diff --git a/lisp/subr.el b/lisp/subr.el
> index 092850a..8673547 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -3041,7 +3041,8 @@ remove-yank-excluded-properties
>              (setq run-start run-end)))))
>      (with-silent-modifications
>        (if (eq yank-excluded-properties t)
> -          (set-text-properties start end nil)
> +          (let ((inhibit-modification-hooks nil))
> +            (set-text-properties start end nil))
>          (remove-list-of-text-properties start end yank-excluded-properties)))))
>  
>  (defvar yank-undo-function)

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Alan Mackenzie]

Hello, Juri.

On Tue, Jan 23, 2018 at 23:38:10 +0200, Juri Linkov wrote:
> Hi Alan,

> Do you agree we could remove the effect of
> with-silent-modifications around set-text-properties,
> and leave it only on remove-list-of-text-properties?
> This will help to fix the reported regression.

I'm not sure about this.  Doesn't `set-text-properties' need to be
"protected", too?

I'm not sure why you want to do this.  Why do you want to do this?

One thing which is puzzling me is that `with-silent-modifications' is a
macro which is defined in subr.el, but later in the file.  Won't this
invocation of w-s-m get compiled as a function call because of this?

> >>> Anyway, I bisected it for you, to
> >>> c5e89be20a3feba9c67be6855b1dbdc6d8ae5ce2

> >> Here's the discussion reference that should have been in the commit message:

> >> http://lists.gnu.org/r/emacs-devel/2015-04/msg00506.html

> > I confirm that after removing with-silent-modifications from
> > remove-yank-excluded-properties, C-y leaves ‘display’ properties
> > in the minibuffer.  It's ‘(inhibit-modification-hooks t)’ in
> > let-bind of with-silent-modifications that prevents read-passwd
> > from calling hide-chars-fun again to put new ‘display’ properties
> > after set-text-properties removes old ones.  So this patch should
> > fix this issue, but I'm not sure if this is right.

> > diff --git a/lisp/subr.el b/lisp/subr.el
> > index 092850a..8673547 100644
> > --- a/lisp/subr.el
> > +++ b/lisp/subr.el
> > @@ -3041,7 +3041,8 @@ remove-yank-excluded-properties
> >              (setq run-start run-end)))))
> >      (with-silent-modifications
> >        (if (eq yank-excluded-properties t)
> > -          (set-text-properties start end nil)
> > +          (let ((inhibit-modification-hooks nil))
> > +            (set-text-properties start end nil))
> >          (remove-list-of-text-properties start end yank-excluded-properties)))))

> >  (defvar yank-undo-function)

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Juri Linkov]

>> Do you agree we could remove the effect of
>> with-silent-modifications around set-text-properties,
>> and leave it only on remove-list-of-text-properties?
>> This will help to fix the reported regression.
>
> I'm not sure about this.  Doesn't `set-text-properties' need to be
> "protected", too?

I'm not sure either.  Do you think that `set-text-properties' without
`with-silent-modifications' will cause the same problem that you
described in http://lists.gnu.org/r/emacs-devel/2015-04/msg00506.html

> I'm not sure why you want to do this.  Why do you want to do this?

Doing yank `C-y' in the minibuffer of `read-passwd' puts dots `.'
over the yanked characters using `display' properties, then later
`set-text-properties' removes all properties (exposing the yanked
characters), but without `with-silent-modifications' it used to put
`display' properties back.

After the change that added `with-silent-modifications',
the hook that puts `display' properties back doesn't run.

> One thing which is puzzling me is that `with-silent-modifications' is a
> macro which is defined in subr.el, but later in the file.  Won't this
> invocation of w-s-m get compiled as a function call because of this?

`with-silent-modifications' is a macro that let-binds
`inhibit-modification-hooks' to `t', thus preventing the
hook in `read-passwd' from running.

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Alan Mackenzie]

Hello, Juri.

Firstly, apologies for answering your first post without having
understood it properly.

On Thu, Jan 25, 2018 at 23:15:41 +0200, Juri Linkov wrote:
> >> Do you agree we could remove the effect of
> >> with-silent-modifications around set-text-properties,
> >> and leave it only on remove-list-of-text-properties?
> >> This will help to fix the reported regression.

> > I'm not sure about this.  Doesn't `set-text-properties' need to be
> > "protected", too?

> I'm not sure either.  Do you think that `set-text-properties' without
> `with-silent-modifications' will cause the same problem that you
> described in http://lists.gnu.org/r/emacs-devel/2015-04/msg00506.html

set-text-properties outwith a with-silent-modifications will most
definitely cause that problem.

> > I'm not sure why you want to do this.  Why do you want to do this?

> Doing yank `C-y' in the minibuffer of `read-passwd' puts dots `.'
> over the yanked characters using `display' properties, then later
> `set-text-properties' removes all properties (exposing the yanked
> characters), but without `with-silent-modifications' it used to put
> `display' properties back.

OK, I understand this, now.

> After the change that added `with-silent-modifications',
> the hook that puts `display' properties back doesn't run.

Yes.

[ .... ]

The problem here appears to be a fundamental design bug in Emacs: that
text properties are regarded as part of the buffer rather than something
accompanying the buffer, as overlays are.  before/after-change-functions
are applied alike to proper buffer changes and text property changes.

read-passwd absolutely needs an after-change-function to run on the
text-property changes in remove-yank-excluded-properties.  This seems
fair enough.  CC Mode absolutely requires the change hooks _not_ to run
on the text-property changes in remove-yank-excluded-properties.  The
two conflict with eachother.

A workaround might be for read-passwd to use an overlay rather than a
text property for display.  But this doesn't solve the underlying
problem.  I now see that the invocation of with-silent-modifications in
remove-yank-excluded-properties is not the Right Thing.  It breaks the
model of text properties being an integral part of the buffer.

Perhaps an alternative would be for Emacs to provide a flag which
indicates to a before/after-change-function whether the current change
is a "proper" change or merely a text property change.  Change hook
functions could then test this flag and, for example, refrain from doing
anything for a text property change.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Eli Zaretskii]

> Date: Fri, 26 Jan 2018 18:37:02 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: 30186@debbugs.gnu.org
> 
> The problem here appears to be a fundamental design bug in Emacs: that
> text properties are regarded as part of the buffer rather than something
> accompanying the buffer, as overlays are.

It's not a bug, it's a feature (and a very important one).

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Alan Mackenzie]

Good evening, Eli.

On Fri, Jan 26, 2018 at 21:08:24 +0200, Eli Zaretskii wrote:
> > Date: Fri, 26 Jan 2018 18:37:02 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: 30186@debbugs.gnu.org

> > The problem here appears to be a fundamental design bug in Emacs: that
> > text properties are regarded as part of the buffer rather than something
> > accompanying the buffer, as overlays are.

> It's not a bug, it's a feature (and a very important one).

Yes.  We see its utility in read-passwd.  But don't worry, I wasn't
proposing to "fix" it.  :-)

Do you have any opinion on my suggestion:

> > Perhaps an alternative would be for Emacs to provide a flag which
> > indicates to a before/after-change-function whether the current
> > change is a "proper" change or merely a text property change.
> > Change hook functions could then test this flag and, for example,
> > refrain from doing anything for a text property change.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#30186: 27.0.50; Password is not hidden in read-passwd

[martin rudalics]

 > The problem here appears to be a fundamental design bug in Emacs: that
 > text properties are regarded as part of the buffer rather than something
 > accompanying the buffer, as overlays are.

Is that interpretation correct?  I always regard text properties part
of some text (as they are retained when copying text from one buffer
to another) and only their text part of the buffer.  And I do regard
overlays parts of their buffers.  Am I wrong?

martin

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Alan Mackenzie]

Hello, Martin.

On Sat, Jan 27, 2018 at 09:27:46 +0100, martin rudalics wrote:
>  > The problem here appears to be a fundamental design bug in Emacs:
>  > that text properties are regarded as part of the buffer rather than
>  > something accompanying the buffer, as overlays are.

> Is that interpretation correct?

No.  I'm half joking.  But an awful lot of Lisp code runs an awful lot of
code separating text properties from actual text, mainly by preventing
the change hooks being run for text property changes.

read-passwd is an example (the only one I know) of the change hooks being
an essential part of text property manipulation.

> I always regard text properties part of some text (as they are retained
> when copying text from one buffer to another) and only their text part
> of the buffer.  And I do regard overlays parts of their buffers.  Am I
> wrong?

Text properties are indeed part of the buffer (or string).  But I don't
think overlays are - if you have an overlay on part of a buffer, and copy
that part into a string, I don't think the overlay stays on the copy.

> martin

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Eli Zaretskii]

> Date: Fri, 26 Jan 2018 20:00:27 +0000
> Cc: juri@linkov.net, 30186@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> Do you have any opinion on my suggestion:
> 
> > > Perhaps an alternative would be for Emacs to provide a flag which
> > > indicates to a before/after-change-function whether the current
> > > change is a "proper" change or merely a text property change.
> > > Change hook functions could then test this flag and, for example,
> > > refrain from doing anything for a text property change.

I'm not sure it would be possible to provide such a flag.  Did you
look at the internals involved, and if so, can you tell where do we
know which kind of change caused the hooks to run?

I also don't understand the problem you have in CC Mode with
text-property changes.  Can you elaborate on that?

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Alan Mackenzie]

Hello, Eli.

On Sat, Jan 27, 2018 at 11:40:36 +0200, Eli Zaretskii wrote:
> > Date: Fri, 26 Jan 2018 20:00:27 +0000
> > Cc: juri@linkov.net, 30186@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > Do you have any opinion on my suggestion:

> > > > Perhaps an alternative would be for Emacs to provide a flag which
> > > > indicates to a before/after-change-function whether the current
> > > > change is a "proper" change or merely a text property change.
> > > > Change hook functions could then test this flag and, for example,
> > > > refrain from doing anything for a text property change.

> I'm not sure it would be possible to provide such a flag.  Did you
> look at the internals involved, and if so, can you tell where do we
> know which kind of change caused the hooks to run?

I envisage adding an extra boolean argument to prepare_to_modify_buffer,
and to signal_after_change.  When called from the text property
routines, that argument would be true, otherwise it would be false.

The essence of this argument would be a guarantee that the change is not
going to alter the buffer text.  There may be other primitives besides
text properties for which we could set this argument.

> I also don't understand the problem you have in CC Mode with
> text-property changes.  Can you elaborate on that?

Yes.  In a largish CC Mode file, mark the entire buffer, kill-ring-save,
and append it after itself, with

    C-x h, M-w, M->, C-y

.  In buffer-undo-list there are no entries for text property changes.
Before the with-silent-modifications was put in, there were many such
entries.  Assume this in the next paragraph

Now undo this latest change with C-_.  Each of the entries for text
property changes wants to invoke CC Mode's
before/after-change-functions, which would make the operation slow.
(But see below.)

The workaround (currently in Emacs) for this, back in 2015, was to put
with-silent-modifications around the text property manipulations in
remove-yank-excluded-properties.  This prevents these manipulations
getting into the undo list, but also stops read-passwd from working
properly.

I now see there is a second workaround, in CC Mode itself, where
c-before-change and c-after-change use backtrace-frame to check the
primitive invoking them, and do nothing if that primitive is, e.g.,
put-text-property.  This is not an elegant workaround.

So, I'm changing my mind, after looking into it a bit more.  Removing
the with-silent-modifications from remove-yank-excluded-properties would
not slow down undo in CC Mode buffers noticeably.  It might slow down
other modes which make extensive use of before/after-change-functions.

The extra flag for the change hooks might still be a good idea.  It no
longer seems pertinent for solving the current bug, though.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Eli Zaretskii]

> Date: Sat, 27 Jan 2018 11:37:13 +0000
> Cc: juri@linkov.net, 30186@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > I'm not sure it would be possible to provide such a flag.  Did you
> > look at the internals involved, and if so, can you tell where do we
> > know which kind of change caused the hooks to run?
> 
> I envisage adding an extra boolean argument to prepare_to_modify_buffer,
> and to signal_after_change.  When called from the text property
> routines, that argument would be true, otherwise it would be false.

What happens when both the text and the properties are changed?

> So, I'm changing my mind, after looking into it a bit more.  Removing
> the with-silent-modifications from remove-yank-excluded-properties would
> not slow down undo in CC Mode buffers noticeably.

So let's do that now.  I think the problem with read-passwd is a
security issue, so it should go to emacs-26, do you agree?

> It might slow down other modes which make extensive use of
> before/after-change-functions.

Let's see if any such modes show up.

> The extra flag for the change hooks might still be a good idea.  It no
> longer seems pertinent for solving the current bug, though.

If it can be definitive, I might agree with you.

Alternatively, we could introduce a mechanism for interested modes to
prevent such changes from getting into buffer-undo-list.

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Alan Mackenzie]

Hello, Eli and Juri.

On Sat, Jan 27, 2018 at 14:23:31 +0200, Eli Zaretskii wrote:
> > Date: Sat, 27 Jan 2018 11:37:13 +0000
> > Cc: juri@linkov.net, 30186@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

[ .... ]

> > So, I'm changing my mind, after looking into it a bit more.
> > Removing the with-silent-modifications from
> > remove-yank-excluded-properties would not slow down undo in CC Mode
> > buffers noticeably.

> So let's do that now.  I think the problem with read-passwd is a
> security issue, so it should go to emacs-26, do you agree?

Yes, i think it should go into emacs-26.

Juri, do you see any problem with just removing that
with-silent-modifications altogether?  I.e. this:



Allow read-passwd to hide characters inserted by C-y.  (Security fix.)

This fixes bug #30186.  The with-silent-modifications was there to prevent
records of text property manipulations being put into buffer-undo-list.  These
had been causing a significant slowdown in CC Mode with C-_ after a large
C-y.  This CC Mode problem has since been solved by a different workaround.

* lisp/subr.el (remove-yank-excluded-properties): Remove the invocation of
with-silent-modifications around the text property manipulations.



diff --git a/lisp/subr.el b/lisp/subr.el
index 052d9cd821..64cbbd52ab 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3019,10 +3019,9 @@ remove-yank-excluded-properties
                           run-start prop nil end)))
             (funcall fun value run-start run-end)
             (setq run-start run-end)))))
-    (with-silent-modifications
-      (if (eq yank-excluded-properties t)
-          (set-text-properties start end nil)
-        (remove-list-of-text-properties start end yank-excluded-properties)))))
+    (if (eq yank-excluded-properties t)
+        (set-text-properties start end nil)
+      (remove-list-of-text-properties start end yank-excluded-properties))))
 
 (defvar yank-undo-function)
 

[ .... ]


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Juri Linkov]

>> So let's do that now.  I think the problem with read-passwd is a
>> security issue, so it should go to emacs-26, do you agree?
>
> Yes, i think it should go into emacs-26.
>
> Juri, do you see any problem with just removing that
> with-silent-modifications altogether?  I.e. this:

I agree that removing with-silent-modifications would be
the best fix.  Thanks for taking care of this.

bug#30186: 27.0.50; Password is not hidden in read-passwd

[Alan Mackenzie]

Hello, Eli and Juri.

On Sat, Jan 27, 2018 at 23:43:34 +0200, Juri Linkov wrote:
> >> So let's do that now.  I think the problem with read-passwd is a
> >> security issue, so it should go to emacs-26, do you agree?

> > Yes, i think it should go into emacs-26.

> > Juri, do you see any problem with just removing that
> > with-silent-modifications altogether?  I.e. this:

> I agree that removing with-silent-modifications would be
> the best fix.  Thanks for taking care of this.

Thanks.  I've made this change and committed it to the emacs-26 branch.

I'm closing the bug with this post.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#30186: 27.0.50; Password is not hidden in read-passwd

[martin rudalics]

 > Text properties are indeed part of the buffer (or string).  But I don't
 > think overlays are - if you have an overlay on part of a buffer, and copy
 > that part into a string, I don't think the overlay stays on the copy.

That's what I meant: The overlay stays with the buffer (unless its
'evaporate' property is non-nil) when you remove the text it covers.
A text property OTOH gets removed with the text and does not stay with
the buffer.  Hence a text property is not part of a buffer (or at most
indirectly so) while an overlay is.

martin