1181571 - (CVE-2015-8512) Lockscreen delay bypass in Firefox OS

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Description

[Frederik Braun [:freddy]]

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.

Comment 1

[Frederik Braun [:freddy]]

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

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

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.

Comment 3

[Frederik Braun [:freddy]]

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.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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?

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Comment 6

[Greg Weng [:snowmantw][:gweng][:λ]]

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.

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

[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.

Comment 8

[Christiane Ruetten [INACTIVE]]

Attachment: Proposed patch for the pass code delay bypass

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.

Comment 9

[Christiane Ruetten [INACTIVE]]

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.

Comment 10

[Christiane Ruetten [INACTIVE]]

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

Comment 11

[Christiane Ruetten [INACTIVE]]

Attachment: [gaia] cr:pctofix > mozilla-b2g:master

Comment 12

[Christiane Ruetten [INACTIVE]]

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

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

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.

Comment 14

[Christiane Ruetten [INACTIVE]]

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.?

Comment 15

[Greg Weng [:snowmantw][:gweng][:λ]]

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.

Comment 16

[Christiane Ruetten [INACTIVE]]

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

Comment 17

[Greg Weng [:snowmantw][:gweng][:λ]]

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.

Comment 18

[Christiane Ruetten [INACTIVE]]

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.

Comment 19

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(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.

Comment 20

[Greg Weng [:snowmantw][:gweng][:λ]]

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

Comment 21

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8639054 [details] [review]
[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.

Comment 22

[Christiane Ruetten [INACTIVE]]

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?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Greg Weng [:snowmantw][:gweng][:λ]]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 26

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(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.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Christiane Ruetten [INACTIVE]]

Attachment: [gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2

Comment 29

[Christiane Ruetten [INACTIVE]]

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 31

[Christiane Ruetten [INACTIVE]]

Attachment: [gaia] cr:pctofix-v2.2r > mozilla-b2g:v2.2r

Comment 32

[Christiane Ruetten [INACTIVE]]

Oops, that makes the last attachment obsolete. Deleting.

Comment 33

[Christiane Ruetten [INACTIVE]]

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

Comment 34

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8654822 [details] [review]
[gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2

Okay, the code looks good to me except one nit. Thanks.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 36

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(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 37

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Comment on attachment 8654822 [details] [review]
[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 38

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Comment on attachment 8654822 [details] [review]
[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 39

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Comment 40

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 41

[Christiane Ruetten [INACTIVE]]

(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] [review]

[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 42

[Mahendra Potharaju [:mahe]]

Comment on attachment 8654822 [details] [review]
[gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2

Based on CR's comments, approved for uplift.

Comment 43

[Christiane Ruetten [INACTIVE]]

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 44

[Christiane Ruetten [INACTIVE]]

Comment on attachment 8654822 [details] [review]
[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 45

[Mahendra Potharaju [:mahe]]

Comment on attachment 8654822 [details] [review]
[gaia] cr:pctofix-v2.2 > mozilla-b2g:v2.2

CR, Yes. This patch is approved to land on 2.2. 

Thanks

Comment 46

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/7a427e0f8aa6c185a9e22358006b97c19435ca4a

Comment 47

[Ryan VanderMeulen [:RyanVM]]

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

Comment 48

[Ryan VanderMeulen [:RyanVM]]

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

Comment 49

[Christiane Ruetten [INACTIVE]]

(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.