1186100 - FindMyDevice / 'lockscreen.lock-immediately' does not lock immediately when pass code timeout set

Status: RESOLVED FIXED

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

Description

[Christiane Ruetten [INACTIVE]]

Find My Device has a command that enables screen lock, pass code, and sets a server-provided pass code, and then it sends a 'lockscreen.lock-immediately' to lock up the device before the thief can access it.

However, when a pass code timeout has already been set, 'lockscreen.lock-immediately' will lock the screen, but won't require a password before the timeout has passed.

Steps to reproduce:

1. Set screen lock and pass code with a long timeout
2. Connect WebIDE debugger to Find My Device process
3. Issue following command on console:
- Commands._commands.lock.bind(Commands, "thief!", "0000", function(){})();

What should happen:
- Device should lock immediately, locking out the user until pass code 0000 is entered

What actually happens:
- Thief keeps using the phone until it runs into the timeout

Comment 1

[Christiane Ruetten [INACTIVE]]

I see two obvious approaches to fix this:

1. FindMyDevice sets pass code timeout to 0 ("immediately)" before firing the 'lockscreen.lock-immediately' event

2. Make the 'lockscreen.lock-immediately' listener enforce that the pass code is required immediately.

I strongly prefer the second approach because there are other uses for 'lockscreen.lock-immediately'-type functionality. I found this bug while working on a new 'lock-immediately' use case.

Comment 2

[Christiane Ruetten [INACTIVE]]

Attachment: Proposed patch fixing the 'lock-immediately' flaw

The proposed patch follows the second approach:

    Making 'ls.lock-immediately' lock immediately
    
    This fixes a bug where 'lockscreen.lock-immediately' as sent by
    FindMyDevice doesn't lock the device immediately with a passcode
    when a pass code timeout has been set.
    
    On unlock, LockScreen.checkPassCodeTimeout() only asks for the pass code
    when either .fetchLockedInterval() or .fetchUnlockedInterval() exceed
    the pass code timeout.
    
    Setting both _lastLockedTimeStamp and _lastUnlockedTimeStamp to
    something deep in the past will enforce pass code entry on unlock and
    avoid a potential race condition.

But I'm not sure if it is an acceptable approach to make LockScreenWindowManager call window.lockScreen methods directly. I'm also wondering whether resetting _lastLocked/UnlockedTimeStamp has any averse side effects.

Comment 3

[:gerard-majax]

Given the fix it and comment 1, this looks like a Lockscreen issue.

Comment 4

[Christiane Ruetten [INACTIVE]]

Created https://github.com/mozilla-b2g/gaia/pull/31060 which is identical to the attached patch.

Comment 5

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

Hi Christiane: I think your analyses are correct. However, since LockScreen itself is listening the 'lockscreen.lock-immediately' at

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/lockscreen/js/lockscreen.js#L437

so you just need to add the callback at that section. Although it's an unfortunate legacy, but that means there is no need to call LockScreen from LockScreenWindowManager. The principle is LockScreenWindowManager should only open and close the window, and it should not deal with other internal logic of LockScreen itself. If you have discovered other side-effects or concerns made you couldn't patch it like that, we can have some following discussions.

Comment 6

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

Oh, and the method name: you can just call it as 'resetTimeoutForcibly' and add some comments about that. Since the method does no real unlocking like changing UI, it just reset the timeouts.

Comment 7

[Christiane Ruetten [INACTIVE]]

Right, I missed the handler in lockscreen.js. Amended the pull request accordingly.

Comment 8

[Christiane Ruetten [INACTIVE]]

FindMyDevice and consequently LockScreen's flawed 'lockscreen.lock-immediately' handling were introduced in b2g-v2.0. Prior branches are not affected unless FindMyDevice was backported.

Wondering about our current 2.0+ partner branches... I guess that Stingray isn't affected. Who knows about them? Please ni.

Comment 9

[Christiane Ruetten [INACTIVE]]

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

Comment 10

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

For LockScreen and System app your patch must be with an new or modified unit test to get a r+. You can set it as feedback? to get granted without tests first, but if you want to land it you still need to have a r+ by other peers/owners with a patch and test. Can you reset the flag and add the test? We can follow the normal process to land it soon, since your change is simple and the test would not be difficult.

Comment 11

[Christiane Ruetten [INACTIVE]]

Sorry, Greg, I was trying to request review, not grant it. It's my first testing work in gaia, but I'll give it a shot.

Comment 12

[Christiane Ruetten [INACTIVE]]

Added two new tests for the new functionality and a fix the old broken test to the pull request. This is the main commit:

    Add and fix lock-immediately tests
    
    This extends lockscreen_test.js with two new tests that check whether
    resetting timeouts will trigger pass code check, and whether lock-immediately
    triggers timeout reset.
    
    This also fixes the old lock-immediately test which didn't trigger the
    observer callback and effectively tested whether a locked lockscreen would be
    locked, which resulted in a broken test that never fails.

Comment 13

[:gerard-majax]

Christiane, I've took the liberty to comment your PR to make sure we do not introduce back any mozSettings observer leak within the system app. More on this topic in bug 1105639.

Comment 14

[Christiane Ruetten [INACTIVE]]

Greg, I rebased the PR to current master. Please review. If OK, can you take care of landing this while I'm on PTO?

Comment 15

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

Okay, will do. But I couldn't set review flag to myself, could you do that for me?

Comment 16

[Christiane Ruetten [INACTIVE]]

Comment on attachment 8638937 [details] [review]
[gaia] cr:immediatelockfix > mozilla-b2g:master

Setting review+ as per :gweng's request in Comment 15

Comment 17

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

This is an important security issue and will have regulartory impacts for the 2.2r branch.

Comment 18

[Christiane Ruetten [INACTIVE]]

Comment on attachment 8638937 [details] [review]
[gaia] cr:immediatelockfix > mozilla-b2g:master

Rebased, local tests passing, manual check according to STR is positive now. Please review.

Comment 19

[Christiane Ruetten [INACTIVE]]

Since this patch is integrated with the new state manager, I propose not to backport, but to apply a hotfix to FindMyDevice instead (setting passcode timeout setting to immediately).

Comment 20

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

Comment on attachment 8638937 [details] [review]
[gaia] cr:immediatelockfix > mozilla-b2g:master

Okay. It's now only with one comment nit. LGTM.

Comment 21

[Christiane Ruetten [INACTIVE]]

Please re-read that comment. It is actually still valid, so I'm leaving it. Fixing the signalling will not be dealt with in this bug.

Comment 22

[Carsten Book [:Tomcat]]

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