After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 708186 - Unlock dialog deletes parts of your password while typing
Unlock dialog deletes parts of your password while typing
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: lock-screen
3.9.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2013-09-16 19:33 UTC by Colin Guthrie
Modified: 2013-09-23 11:34 UTC
See Also:
GNOME target: 3.10
GNOME version: ---


Attachments
authPrompt: Don't clear entry when starting a new conversation (1.02 KB, patch)
2013-09-18 19:53 UTC, Florian Müllner
none Details | Review
ShellAuthPrompt: don't clear or update sensitivity on failure (1.60 KB, patch)
2013-09-19 12:10 UTC, Giovanni Campagna
none Details | Review
authPrompt: Clear _queryingService on verification failure (1.17 KB, patch)
2013-09-19 14:15 UTC, Florian Müllner
committed Details | Review

Description Colin Guthrie 2013-09-16 19:33:09 UTC
1. Lock your session.
2. Enter an icorrect password.
3. Yellow text appears telling you that you got your p/w wrong.
4. Start typing immediately, as the yellow text fades, the first few characters of what you have managed to type is deleted.
5. If you type quickly, this results in an incorrect password being entered and the whole process repeats!

This may also happen at the login screen as well as the lock screen - not really tried.
Comment 1 Matthias Clasen 2013-09-17 23:18:55 UTC
I've noticed this as well - very annoying
Comment 2 Florian Müllner 2013-09-18 19:53:57 UTC
Created attachment 255249 [details] [review]
authPrompt: Don't clear entry when starting a new conversation

In case a previous conversation failed, the entry should have been
cleared from _reset() already. Furthermore, the user may already
have started to re-enter their password, in which case the additional
clear is both unexpected and annoying.
Comment 3 Florian Müllner 2013-09-18 20:06:46 UTC
I didn't see any regressions in either unlock or login dialog in a quick test, but then I'm not using any fancy options either - Ray, does this look safe to you?
Comment 4 Matthias Clasen 2013-09-19 11:41:37 UTC
Hey Florian, could you also look at bug 708361, which is a somewhat similar issue ?
Comment 5 Giovanni Campagna 2013-09-19 11:48:53 UTC
I think this change is wrong. We may get two queries from the auth service, for some reason, in which case we need to clear after the first one.

What should happen, though, is that on failure _queryingService is set to null. Why is that not happening?
Comment 6 Giovanni Campagna 2013-09-19 12:10:20 UTC
Created attachment 255297 [details] [review]
ShellAuthPrompt: don't clear or update sensitivity on failure

Wait for the next reset instead, otherwise the user can start
typing and then we clear again.
Also, make sure the prompt is sensitive when asking for the username,
which fixes the entry sensitivity when the password is failed more than
three times.

This is a different take on the issue. For some reason, there is a delay
between the verification-failed signal and reset, so there is a window
in which the authentication failed message is visible but the entry
is full and insensitive.
I don't think that's too big of a problem, we could even market it
as a security feature.
Comment 7 Florian Müllner 2013-09-19 13:06:48 UTC
(In reply to comment #5)
> I think this change is wrong.

Yeah, that's why I was asking :-)


> We may get two queries from the auth service, for some reason, in which 
> case we need to clear after the first one.

Mmmh, that's a part that I don't understand - why would we need to delete a partially typed password at this point (or stop a spinner that has already been stopped)?


> What should happen, though, is that on failure _queryingService is set to null.

Sure, that's the alternative - I can take a look.


(In reply to comment #6)
> For some reason, there is a delay between the verification-failed signal 
> and reset, so there is a window in which the authentication failed 
> message is visible but the entry is full and insensitive.
> I don't think that's too big of a problem

It's not a big problem, but a small annoyance, so I'd prefer to avoid it anyway (I don't see how it adds much security - to brute force the UI, you need access to gdm's X session, at which point a timeout doesn't look like a huge show stopper)
Comment 8 Giovanni Campagna 2013-09-19 13:18:42 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > I think this change is wrong.
> 
> Yeah, that's why I was asking :-)
> 
> 
> > We may get two queries from the auth service, for some reason, in which 
> > case we need to clear after the first one.
> 
> Mmmh, that's a part that I don't understand - why would we need to delete a
> partially typed password at this point (or stop a spinner that has already been
> stopped)?

Because it's a different question. Also, the spinner is running at point.
The workflow I'm thinking of is something like
auth required pam_unix.so
auth required pam_otp.so
which would ask in a row for your password and for the HW token, before answering yes or no.

The other setup is when account management fails asking for a new password, so we two questions in a row (new password and password confirm). This could happen if the password expired, or if the admin set the user to change the password at the next login.

In any case, by the time we reach the second question, the spinner is running (started when the user clicked unlock the first time) and the entry is full with the unix password, which is useless at that point.

> (In reply to comment #6)
> > For some reason, there is a delay between the verification-failed signal 
> > and reset, so there is a window in which the authentication failed 
> > message is visible but the entry is full and insensitive.
> > I don't think that's too big of a problem
> 
> It's not a big problem, but a small annoyance, so I'd prefer to avoid it anyway
> (I don't see how it adds much security - to brute force the UI, you need access
> to gdm's X session, at which point a timeout doesn't look like a huge show
> stopper)

I didn't say it adds security, I said we could market it that way.
Comment 9 Florian Müllner 2013-09-19 14:15:02 UTC
Created attachment 255306 [details] [review]
authPrompt: Clear _queryingService on verification failure

A conversation is finished after failing, and we are expecting a new
one to be started shortly after. However if we encounter an existing
reference to a previously set _queryingService, we will clear the
password entry, which might already contain a partially typed password
at that point. The behavior does make sense in the case of conflicting
conversations, but in the failure case it is both unexpected and
annoying, so clear _queryingService early to prevent this.


OK, so here's the third approach ...
Comment 10 Giovanni Campagna 2013-09-20 16:29:25 UTC
Review of attachment 255306 [details] [review]:

I think this solution is the best one.
Comment 11 Matthias Clasen 2013-09-20 17:57:27 UTC
Florian, want to request a freeze break for it ?
Comment 12 Florian Müllner 2013-09-21 14:20:38 UTC
(In reply to comment #11)
> Florian, want to request a freeze break for it ?

Yes, but I'd like to bundle the request with bug 708187 ...
Comment 13 Florian Müllner 2013-09-23 11:34:25 UTC
Attachment 255306 [details] pushed as 02c99e4 - authPrompt: Clear _queryingService on verification failure