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

RESOLVED FIXED in Firefox OS master

Status

Firefox OS
Gaia::System::Lockscreen
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: cr, Assigned: cr)

Tracking

({sec-other})

unspecified
FxOS-S7 (18Sep)
ARM
Gonk (Firefox OS)
sec-other
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.5+, b2g-v2.0 affected, b2g-v2.0M ?, b2g-v2.1 affected, b2g-v2.1S ?, b2g-v2.2 affected, b2g-v2.2r ?, b2g-master fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
str
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
(Assignee)

Updated

2 years ago
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
Keywords: sec-other
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
Created attachment 8636682 [details] [diff] [review]
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.
Given the fix it and comment 1, this looks like a Lockscreen issue.
Flags: needinfo?(gweng)
(Assignee)

Updated

2 years ago
Blocks: 1186124
(Assignee)

Comment 4

2 years ago
Created https://github.com/mozilla-b2g/gaia/pull/31060 which is identical to the attached patch.
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.
Flags: needinfo?(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.
(Assignee)

Comment 7

2 years ago
Right, I missed the handler in lockscreen.js. Amended the pull request accordingly.
(Assignee)

Comment 8

2 years ago
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.
Assignee: nobody → cr
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → ?
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → ?
status-b2g-v2.2r: --- → ?
(Assignee)

Updated

2 years ago
Blocks: 1187368
(Assignee)

Comment 9

2 years ago
Created attachment 8638937 [details] [review]
[gaia] cr:immediatelockfix > mozilla-b2g:master
Attachment #8636682 - Attachment is obsolete: true
Attachment #8638937 - Flags: sec-approval+
Attachment #8638937 - Flags: review+
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.
(Assignee)

Updated

2 years ago
Attachment #8638937 - Flags: sec-approval+
Attachment #8638937 - Flags: review+
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Comment 12

2 years ago
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.
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.
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 14

2 years ago
Greg, I rebased the PR to current master. Please review. If OK, can you take care of landing this while I'm on PTO?
Flags: needinfo?(gweng)
Okay, will do. But I couldn't set review flag to myself, could you do that for me?
Flags: needinfo?(gweng)
(Assignee)

Comment 16

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

Setting review+ as per :gweng's request in Comment 15
Attachment #8638937 - Flags: review+
This is an important security issue and will have regulartory impacts for the 2.2r branch.
blocking-b2g: --- → 2.5+
Blocks: 1175623
(Assignee)

Comment 18

2 years ago
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.
Attachment #8638937 - Flags: review+ → review?(gweng)
(Assignee)

Comment 19

2 years ago
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).
(Assignee)

Updated

2 years ago
Depends on: 1202484
Comment on attachment 8638937 [details] [review]
[gaia] cr:immediatelockfix > mozilla-b2g:master

Okay. It's now only with one comment nit. LGTM.
Attachment #8638937 - Flags: review?(gweng) → review+
(Assignee)

Comment 21

2 years ago
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.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/7d85f3d27f126e81be2b1b8160562627519f80c5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)
(Assignee)

Updated

2 years ago
status-b2g-master: affected → fixed
Group: b2g-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.