Repro video in this link:
https://pantheon.corp.google.com/storage/browser/chromiumos-test-logs/bugfiles/cr/902973/

Feels like a cryptohome issue to me where the cryptohome key is still considered timed out.

Assigning to pmalani@, but jdufault@, please, answer the questions below.

1) I checked the video. Iiuc, the PIN was entered incorrectly on the _unlock_ screen, which put the PIN in the locked screen. And after that a password was used to _unlock_ the device. 
In unlock case, Chrome doesn't need to call MountEx, since the user dir is mounted. Does it call CheckKeyEx? Jacob, can you please confirm?

I strongly suspect that we only unlock PINs in MountEx, when the user is signed in. So, likely, we need to make sure that its is also done when CheckKeyEx is called. It may affect unlock screen performance, as currently we only check in-memory secrets to unlock, but to reset PINs, we'd had to go all the way through TPM and reading & decrypting reset_secret from disk to do that.

2) If it's indeed unlock behavior thing, do we even need to reset PIN in this case? Iirc, the original requirements were to reset PIN when the user successfully logs in, not when it unlocks the screen. But maybe we need to revise that req to include unlock in the scope - it indeed seems more natural to support that case.

3) Also, marked as regression. Do we know what's the last known good version, where pinpad was displayed in this situation and PIN worked? 


4)

> Feels like a cryptohome issue to me where the cryptohome key is still considered timed out.

Yes, quite probable, but just to understand: at this point it is just a theory that it is cryptohomed that reports the key is locked, and that's why Chrome doesn't show the keypad because of that, right? Or did somebody already check Chrome logs/behavior and confirmed that?

> In unlock case, Chrome doesn't need to call MountEx, since the user dir is mounted. Does it call CheckKeyEx? Jacob, can you please confirm?

Yes, CheckKeyEx is always used to authenticate a password.

Login uses MountEx.

> 2) If it's indeed unlock behavior thing, do we even need to reset PIN in this case? Iirc, the original requirements were to reset PIN when the user successfully logs in, not when it unlocks the screen. But maybe we need to revise that req to include unlock in the scope - it indeed seems more natural to support that case.

I'm pretty sure we want to unlock PIN whenever the user authenticates. The UX would be strange if that was only login; the goal is that the user almost never logs out.

3) Also, marked as regression. Do we know what's the last known good version, where pinpad was displayed in this situation and PIN worked? 

My guess is before cryptohome-based PIN.

> 4) Yes, quite probable, but just to understand: at this point it is just a theory that it is cryptohomed that reports the key is locked, and that's why Chrome doesn't show the keypad because of that, right? Or did somebody already check Chrome logs/behavior and confirmed that?

I checked on a device with prefs-based PIN and the PIN shows up again as expected.

We can go ahead and make the change. Just want to make sure if there is a metric governing time to unlock, the people monitoring that metric are aware of the potential hit to that metric.

Also, CC-ing mnissler@ so that he is also aware of the potential change in policy here.

mkarkada@ could you kindly confirm whether logging out and the logging in again (and then locking the device) re-enables PIN?

@pmalani, yes, after user log-out & log-in again and then lock the device/ sign-out, Pinpad appears back.

Great thanks for confirming, mkarkada@; that's most helpful!

I'm going to start work on the change, but will also wait for an Ack from mnissler@ before pushing stuff for review.

Actually, I tried this on a device.

IIUC the observed behaviour is as follows:
- User signs in correctly using PIN/password (LE Credentials if any get reset here)
- User locks screen (LE credentials unaffected here)
- User attempts to *unlock* using PIN and makes the max possible incorrect attempts (again, LE credentials not involved here)
- User signs out (or locks) and when he/she attempts to sign in again (unlock), the PIN screen is not shown (PIN pad not shown isn't related to LE credential being locked, because it's not locked).

I didn't understand how above relates to LECredentials specifically. The LE Credential has not been marked as Locked out in the above flow, hence it doesn't need to get reset at all.

AFAICT Chrome seems to be marking the PIN as not to be used for either sign-in or unlock for some reason after an excessive number of "PIN unlock" attempts, and thus it isn't presenting the UI or sending the appropriate tag to cryptohome, even after one has entered the password to unlock.

It is a separate (and valid) issue that LE Credentials should indeed be reset on an password unlock anyways, but I don't think that is necessarily the source of the specific issue reported. FWIU Chrome should unset whatever flag it is using to signal PINs unusable once we enter a password to unlock.

Jacob, can you verify the above?

N.B: I tried the above on Chrome 72

Did some digging, and I believe chrome is using the API as expected. One thing to note is that Chrome uses check_key_ex to validate the PIN is correct when unlocking.

So check_key_ex should unlock/reset LE credential state.

Re: Comment #9

Just wanted to ensure that the security folks are cognizant of the fact that every PIN unlock attempt will be going through TPM (and the associated performance hit).

This may be WAI and/or it may be a performance penalty we are OK with, but just wanted to make sure apronin@ and mnissler@ are aware of it.

That said, the reset on CheckKeyEx should go in regardless. Will prep a CL.

1) +tbuckley for extra performance hit on screen unlock to perform PIN reset.
2) @pmalain, can we first check if the PIN keyset is actually locked and only reset it (and pay the performance price) if it is so. in this variation, I suspect the perf hit will be acceptable, since it will only happen for the users who have a locked PIN during screen unlock.

Re Comment #11:
- When we perform incorrect PIN unlock 5 times, and then reset the device and try to sign in, the PIN page is not presented. Judging by the codepath, and what jdufault@ mentioned, CheckKeyEx is being used, and that does a GetValidKeyset() call in HomeDirs, so it would be performing an LE credential lookup on each PIN unlock attempt.

Unfortunately since the passwords are hashed by Chrome, it'll be difficult to check it via commandline.

Unless Chrome is saving some state that tells it to sent in the password as a PIN or regular password (Jacob, can you kindly confirm), I'm reasonably sure the LE credential is getting locked.

When chrome issues the CheckKeyEx call it knows if the credential is password or PIN. If PIN, the "PIN" key label is set in the CheckKeyEx call, otherwise an empty wildcard label is used.

RE: Comment #14:

I perhaps didn't phrase my question clearly(sorry):

When the user *signs in*, at the sign-in screen a PIN Pad is presented if the user had  earlier registered a PIN. My observation is that the PIN Pad is *not* presented at sign-in if the PIN was locked out (similarly for unlock).

How does Chrome ascertain whether to present the PIN pad at sign in or not? Is it querying something in cryptohome to check whether the credential is locked? Is it part of the EVKK metadata?

Also, re: Comment #14 : May I ask how does chrome "know" whether the password is a regular password or PIN? "123456" could be a valid regular password as well as a valid PIN, so the disambiguation criteria aren't clear to me.

The login screen makes a GetKeyDataEx call which contains an `auth_locked` field. If the PIN key is locked, PIN is not shown [1].

The login UI treats an purely numeric password when the PIN keyboard is shown as a PIN. If the GAIA password is also purely numeric and the user also setup a PIN then this means the GAIA password will not be usable until PIN times out.

1: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/quick_unlock/pin_storage_cryptohome.cc?l=64&rcl=622bc8d759bfb0d22f54b46fb03c75735267f14d

Thanks Jacob :)

Andrey, 
Re Comment #11 > 2 : Shouldn't we be unconditionally resetting a PIN ? IMO sounds like when a person unlocks with his/her password, the number of *incorrect* attempts should be set back to 0, regardless of whether he/she had already reached the max number of incorrect PIN attempts or not. This is what the behavior is for sign in (MountKeyEx) currently, so perhaps we should keep the behaviour identical here too.

> Shouldn't we be unconditionally resetting a PIN ? [...] perhaps we should keep the behaviour identical here too.

Ok, makes sense.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/1afc0e97d9d8d70ae27cffd3d6c4b04a51f77503

commit 1afc0e97d9d8d70ae27cffd3d6c4b04a51f77503
Author: Prashant Malani <pmalani@chromium.org>
Date: Wed Dec 12 17:41:00 2018

cryptohome: Reset LE credentials on password unlock

If a password is used to unlock a device after exceeding the max PIN
unlock attempts, then reset the LE credentials.

BUG= chromium:902973 
TEST=- cros_run_unit_tests
     - Unlock device after execeeding max PIN attempts,

Change-Id: I095dccec3798c6a8fce70810c56668edf26efc64
Reviewed-on: https://chromium-review.googlesource.com/1338209
Commit-Ready: Prashant Malani <pmalani@chromium.org>
Tested-by: Prashant Malani <pmalani@google.com>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/1afc0e97d9d8d70ae27cffd3d6c4b04a51f77503/cryptohome/service.cc
[modify] https://crrev.com/1afc0e97d9d8d70ae27cffd3d6c4b04a51f77503/cryptohome/make_tests.cc
[modify] https://crrev.com/1afc0e97d9d8d70ae27cffd3d6c4b04a51f77503/cryptohome/mock_platform.h

The change has been submitted. Do we need to merge this to 72, Andrey?

I'd suggest merging to 72.

Do I get it right, this bug also should be marked as Fixed? (technically, only after that we can request merge)

Thank you for pointing that out. I'll go ahead and mark this as fixed.

mkarkada@ kindly re-open if you this issue on master or on 72 after it's been merged.

Assigning this to djmm@ for Merge approval (got the LDAP from go/chromeos-schedule)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/a9a25365be2dfd828e7cdcb5b2d5891d8ca730a6

commit a9a25365be2dfd828e7cdcb5b2d5891d8ca730a6
Author: Prashant Malani <pmalani@chromium.org>
Date: Thu Dec 13 19:46:15 2018

cryptohome: Reset LE credentials on password unlock

If a password is used to unlock a device after exceeding the max PIN
unlock attempts, then reset the LE credentials.

BUG= chromium:902973 
TEST=- cros_run_unit_tests
     - Unlock device after execeeding max PIN attempts,

Change-Id: I095dccec3798c6a8fce70810c56668edf26efc64
Reviewed-on: https://chromium-review.googlesource.com/1338209
Commit-Ready: Prashant Malani <pmalani@chromium.org>
Tested-by: Prashant Malani <pmalani@google.com>
Reviewed-by: Andrey Pronin <apronin@chromium.org>
(cherry picked from commit 1afc0e97d9d8d70ae27cffd3d6c4b04a51f77503)
Reviewed-on: https://chromium-review.googlesource.com/c/1374441
Reviewed-by: Prashant Malani <pmalani@google.com>
Commit-Queue: Prashant Malani <pmalani@google.com>
Trybot-Ready: Prashant Malani <pmalani@google.com>

[modify] https://crrev.com/a9a25365be2dfd828e7cdcb5b2d5891d8ca730a6/cryptohome/service.cc
[modify] https://crrev.com/a9a25365be2dfd828e7cdcb5b2d5891d8ca730a6/cryptohome/make_tests.cc
[modify] https://crrev.com/a9a25365be2dfd828e7cdcb5b2d5891d8ca730a6/cryptohome/mock_platform.h

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot