STR: 1) Lock the screen with a PIN 2) Try an incorrect PIN multiple times. 3) Observe how the delay for trying again gets longer and longer 4) While waiting with the red buttons still glowing, press the home button (or power twice, to lock and unlock again) 5) Note how you can try another pin just right away.

Paul since you implemented this originally (couldn't find the bug though). Would you be willing to take another look=

I think we should find regression window first, if this doesn't exist in previous versions. But I don't know how to handle secure bug to let QA find the window.

I wouldn't be surprised if this is a long standing bug. But we can involve QA as needed, just CC them to the bug.

So looking at this I think I know what the bug is. Im not sure if it was always there (the pad used to be part of the same file, so I think might have broken with it was split out?)But that's a guess. It may have never worked. In any case I think I know how to fix it. Basically in the original implementation of this, all I did was add the following to LockScreen.prototype.onPasscodeValidationFailedL: 1096 this.kPassCodeErrorCounter++; 1097 //double delay if >5 failed attempts 1098 if (this.kPassCodeErrorCounter > 5) { 1099 this.kPassCodeErrorTimeout = 2 * this.kPassCodeErrorTimeout; 1100 } When you get the passcode wrong, we show the red dots and vibrate, vi 1093 window.dispatchEvent(new CustomEvent( 1094 'lockscreen-notify-passcode-validationfailed')); The pad catches this event here and shows the red dots. https://mxr.mozilla.org/gaia/source/apps/system/lockscreen/js/lockscreen_inputpad.js#95 It looks like my original logic has been replicated in the passcodepad itself: https://mxr.mozilla.org/gaia/source/apps/system/lockscreen/js/lockscreen_inputpad.js#142 Now I'm mostly guessing this last part, but it looks like that when you press the homescreen, the lockscreen app is closed, and the passcodepad recieve an event and removes the block UI. https://mxr.mozilla.org/gaia/source/apps/system/lockscreen/js/lockscreen_inputpad.js#105 105 case 'lockscreen-appclosed': 106 this.removeErrorPasscodeUI(); 107 this.updatePassCodeUI(); I assume there needs to be a check when the passcodepad starts to see if it should be locked or not, instead of just clearing when the lockscreen app is closed. What do you think Greg?

Adding naoki, but this strikes me as a bug that is very difficult to test automatically, and therefore difficult to check the regression.

Sorry I would check this as soon as possible. Since I'm working on another secure bug (Bug 1173284) which involves lots of performance issues and underlying implementations.

[Blocking Requested - why for this release]: Thanks Greg, no worries. Sooner the better, but as long as it makes 2.5 that's ok I think since its a sec-moderate.

Fixing lockscreen delay bypass by rewriting core logic This is fixing bug 1181571 where lockscreen delay on repeated pass code failure could be trivially bypassed by pressing the home button. The faulty behavior was caused by a limitation of the previous architecture which was only making the delay occurr *after* a wrong pass code had been entered. Consequently, whenever the input pad would be restarted, like after closing it with the home button or when a screen timeout occurred, the pending delay would not persist and pass code entry would be accepted right away. The fix made it necessary to rewrite parts of lockscreen's messaging and state handling. Previous changes in architecture caused pass code failure counting and delay timer handling to be maintained in two places: in LockScreen (for pass code checking) and in LockScreenInputPad (for diplaying the error state to the user). Logic error caused the two states to even get out of sync under certain conditions. Now all state and timeout related to pass code checking are maintained by LockScreen, which signals LockScreenInputManager with three distinct messages of failure (on wrong pass code), reset (after error delay has passed), and success (on correct pass code). LockScreenInputPad now is always aware of a passcode error dealy ongoing in LockScreen, and every UI update is rendered according to this state, and key input is ignored when necessary.

The problem sounds simple, but the patch became amazingly extensive. I could not come up with an easy fix, because architecture and signalling logic were too convoluted. It looked like things broke during recent LockScreen architecture changes. This patch should definitely break some LockScreen tests, but I haven't worked on this front, yet. I could use some some help there once we decide whether my patch is worth landing.

Greg, has my patch any chance of landing in the middle of the ongoing redesign?

I need help with writing tests that test code paths running asynchronously in promises or setTimeout() blocks. See PR where I added test prototypes.

Hi Christiane, I think the code is good to me, except some nits majorly about comments. Please fix it and you may set review to me. Thanks.

Fixes are in, but did you get a chance to look at the tests? I'm having trouble testing async code paths. Should I just live-patch setTimeout() et al.?

Hi Christiane: I replied the usual way we test async functions on the PR page. You may take a look and complete the patch. Thanks.

I rebased the patch and revised the tests. How does it look?

Sorry for the delay reply. I recently landed the patch to respect timeout after rebooting again, so I think we can continue the rebasing and landing.

Greg, I rebased the patch and tested it on device and it works as expected. But for some obscure reason – which I can't investigate before my PTO departure – I currently can't run any unit tests. Could you give it a try? Apart from that, lights are green from my side for landing this patch.

(In reply to Christiane Ruetten [:cr] (PTO Aug 8-23) from comment #18) > Greg, I rebased the patch and tested it on device and it works as expected. > But for some obscure reason – which I can't investigate before my PTO > departure – I currently can't run any unit tests. Could you give it a try? > > Apart from that, lights are green from my side for landing this patch. This comment is marked as private, which I think means that its only viewable by core-security group. So I dont think greg will be able to see it despite the need-info. I don't think thats what you meant so I removed the private flag.

Will review that after Bug 1189641 landed, since we may need to review all situation after we fix the major bypass flaw.

Comment on attachment 8639054 [details] [gaia] cr:pctofix > mozilla-b2g:master Code looks good to me. Thanks. And please make sure it works well on device and test again. Because it's a long while after the patch submitted.

Rebased to latest master and tested on-device successfully. New tests are passing. I see four failing lockscreen notification tests locally, but they're not caused by this patch. Greg, what's the next step to get this landed?

Master: https://github.com/mozilla-b2g/gaia/commit/14f32ddf49e9c1f2b30c391a26ba2dc867e948c1

Since it's landed, let's see if any new problems emerge. I will try to fix the failed tests. It's now in another bug.

Is this something you want to backport to v2.2/v2.1s? Being sec-moderate, it'll need to go through the regular approval process if so.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25) > Is this something you want to backport to v2.2/v2.1s? Being sec-moderate, > it'll need to go through the regular approval process if so. Yes this is very important to backport. Esp for 2.2r as this affect legal requirements for FMD.

Please request approval on the patch then. It's only sec-moderate, so it doesn't have auto-approval.

Ryan, I requested review from Greg, but who do I use as a requestee for approval-gaia-v2.2? And do we need a separate pull request for 2.2r? It looks like the patch requires no change for that.

See https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_3. If it lands on v2.2, it'll be merged to v2.2r automatically.

Oops, that makes the last attachment obsolete. Deleting.

Committed a workaround for the eslint issue with the test using innerHTML. See bug 1200298 is you care.

Comment on attachment 8654822 [details] [gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2 Okay, the code looks good to me except one nit. Thanks.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25) > Is this something you want to backport to v2.2/v2.1s? Being sec-moderate, > it'll need to go through the regular approval process if so. (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27) > Please request approval on the patch then. It's only sec-moderate, so it > doesn't have auto-approval.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #35) > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25) > > Is this something you want to backport to v2.2/v2.1s? Being sec-moderate, > > it'll need to go through the regular approval process if so. > > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27) > > Please request approval on the patch then. It's only sec-moderate, so it > > doesn't have auto-approval. Sorry for nativity here - how do we "request approval on the patch". Do I set b2g-blocking to 2.2? and then add a reason? I hope so, this is what Im doing below. [Blocking Requested - why for this release]: The 2.2r branch needs this as a legal requirement for the Killswitch feature which is built on FMD and the lockscreen code.

Comment on attachment 8654822 [details] [gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2 [Approval Request Comment] [Bug caused by] (feature/regressing bug #): [User impact] if declined: [Testing completed]: [Risk to taking this patch] (and alternatives if risky): [String changes made]:

Comment on attachment 8654822 [details] [gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2 [Approval Request Comment] [Bug caused by] (feature/regressing bug #): [User impact] if declined: [Testing completed]: [Risk to taking this patch] (and alternatives if risky): [String changes made]:

cr - can you please just add the testing you completed below, so this can get approval? THanks . (In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #37) > Comment on attachment 8654822 [details] > [gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2 > > [Approval Request Comment] [Bug caused by] (feature/regressing bug #): Unknown [User impact] if declined: Legal requirement for killswitch (partner feature). Also without [Testing completed]: <cr to add> [Risk to taking this patch] (and alternatives if risky): Small patch so low risk? [String changes made]: None (I think?)

Oh on the impact section I meant to add the user it at risk, since their passcode could be bypassed by a determined local attacker without the delay control.

(In reply to Paul Theriault [:pauljt] from comment #39) > cr - can you please just add the testing you completed below, so this can > get approval? THanks . > > > (In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from > comment #37) > > Comment on attachment 8654822 [details] [gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2 [Approval Request Comment] [Bug caused by] (feature/regressing bug #): Exact bug # is unknown, but it regressed as part of the 2.1 lockscreen rewrite. [User impact] if declined: Legal requirement for killswitch (partner feature). Also without patch, the pass cade can be much more easily brute-forced, further aiding for example a smudge attack. [Testing completed]: - Test automation is currently broken by bug 1195872: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=462e78fbe388061d64c04b8b3d0854b1f87f75e0 - local gaia tests are passing - manual functional test is passing [Risk to taking this patch] (and alternatives if risky): Contained patch so low risk [String changes made]: None

Comment on attachment 8654822 [details] [gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2 Based on CR's comments, approved for uplift.

The pull request is actually for the v2.2 branch, and I was told by RaynVM that once it lands on 2.2, it'll be landed in 2.2r automatically. I don't know why Naoki changed my approval request to 2.2r, but do we have approval to land on 2.2, too?

Comment on attachment 8654822 [details] [gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2 [Approval Request Comment] [Bug caused by] (feature/regressing bug #): [User impact] if declined: [Testing completed]: [Risk to taking this patch] (and alternatives if risky): [String changes made]:

Comment on attachment 8654822 [details] [gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2 CR, Yes. This patch is approved to land on 2.2. Thanks

This or bug 1190079 is causing Gaia unit test and Linter failures on v2.2. https://treeherder.mozilla.org/logviewer.html#?job_id=169065&repo=mozilla-b2g37_v2_2 https://treeherder.mozilla.org/logviewer.html#?job_id=169064&repo=mozilla-b2g37_v2_2

Looks like the Linter failures went away on a later push once Bumper Bot got un-stuck, but the Gaia unit test failures remain.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #47) > This or bug 1190079 is causing Gaia unit test and Linter failures on v2.2. > https://treeherder.mozilla.org/logviewer.html#?job_id=169065&repo=mozilla- > b2g37_v2_2 > https://treeherder.mozilla.org/logviewer.html#?job_id=169064&repo=mozilla- > b2g37_v2_2 See bug 1204859 for explanation and fix.