[FindMyDevice] remote lock does not override existing pass code when asked to

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
FindMyDevice
RESOLVED FIXED
3 years ago
2 years 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

(feature-b2g:2.2r+, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
str
Find My Device has a lock command that lock the device in case of loss and takes an optional argument for a new pass code to set. 

However, in

https://mxr.mozilla.org/gaia/source/apps/findmydevice/js/commands.js#161

There's an explicit check that prevents overwriting an existing pass code, thwarting the victim chance to lock out a "finder" who knows the current pass code (think smudge attack).

The obvious fix is to remove '!this.deviceHasPasscode() &&' from:

      if (!this.deviceHasPasscode() && passcode) {
        console.log("setting passcode", passcode)
        pr = PasscodeHelper.set(passcode);
      }

I haven't checked the higher command handling layers in FindMyDevice. They might exhibit this questionable behavior as well.

STR:
 - setup pass code "1234" in FxOS Settings
 - connect WebIDE to FindMyDevice
 - Issue on the process' Console:
   Commands._commands.lock.bind(Commands, "thief!", "0000", function(){})();
   - alternatively:
     Issue an actual FindMyDevice lock through the service, setting a new pass code

Actual result:
 - pass code is still "1234"

Expected result:
 - pass code should become "0000"
(Assignee)

Comment 1

3 years ago
Alexandre, I can't find Guilherme anywhere for ni. Who is FMD module owner now? Or perhaps you can already tell this is intended behavior?

In any case, it looks like https://wiki.mozilla.org/Modules/All#Find_My_Device needs an update?
Flags: needinfo?(lissyx+mozillians)
(Assignee)

Comment 2

3 years ago
This is not a regression.

0f4f8eeb (Guilherme Goncalves 2014-06-16 22:27:44 -0300 161)  if (!this.deviceHasPasscode() && passcode) {
e0cdccd2 (Frederik Braun      2015-07-07 09:10:53 +0200 162)    pr = PasscodeHelper.set(passcode);
0f4f8eeb (Guilherme Goncalves 2014-06-16 22:27:44 -0300 163)  }
(In reply to Christiane Ruetten [:cr] from comment #1)
> Alexandre, I can't find Guilherme anywhere for ni. Who is FMD module owner
> now? Or perhaps you can already tell this is intended behavior?

Sadly I got into this project late and as a peer only to help reviewing.
The commit 0f4f8eeb is explicitely before I came in :)

I'd understand this as "if the device has a passcode lets use it, and force one if there is no passcode currently".
Flags: needinfo?(lissyx+mozillians)
(Assignee)

Comment 4

3 years ago
The module owner currently listed, Guilherme Gonçalves, seems to have left Mozilla. Who is currently responsible for FMD?
Whiteboard: qawanted, moduleownerwanted
(Assignee)

Comment 5

3 years ago
Alexandre pointed out to me that according to bug 945889, this is not a feature, but a bug.
(Assignee)

Updated

3 years ago
See Also: → bug 945889
(Assignee)

Updated

3 years ago
Keywords: sec-other
(Assignee)

Comment 6

3 years ago
I don't think this bug needs to be closed, because knowledge about it does not expose additional attack surface.
(Assignee)

Updated

3 years ago
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-v2.2r: --- → affected
status-b2g-master: --- → affected
(Assignee)

Updated

3 years ago
Assignee: nobody → cr
Created attachment 8657930 [details] [review]
[gaia] cr:fmdpasscodefix > mozilla-b2g:master
Created attachment 8657931 [details] [diff] [review]
[gaia] cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2
(Assignee)

Comment 9

3 years ago
Comment on attachment 8657930 [details] [review]
[gaia] cr:fmdpasscodefix > mozilla-b2g:master

Trivial fix, please review.
Attachment #8657930 - Flags: review?(lissyx+mozillians)
(Assignee)

Updated

3 years ago
Whiteboard: qawanted, moduleownerwanted
(Assignee)

Updated

3 years ago
Blocks: 1175623
(Assignee)

Comment 10

3 years ago
Comment on attachment 8657931 [details] [diff] [review]
[gaia] cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2

Trivial fix, please review.
Attachment #8657931 - Flags: review?(lissyx+mozillians)
(Assignee)

Updated

3 years ago
Depends on: 1195872
Comment on attachment 8657930 [details] [review]
[gaia] cr:fmdpasscodefix > mozilla-b2g:master

Looks good, but I think we want tests ?
Attachment #8657930 - Flags: review?(lissyx+mozillians) → review+
Attachment #8657931 - Flags: review?(lissyx+mozillians) → review+
(Assignee)

Comment 12

3 years ago
(In reply to Alexandre LISSY :gerard-majax from comment #11)
> Comment on attachment 8657930 [details] [review]
> [gaia] cr:fmdpasscodefix > mozilla-b2g:master
> 
> Looks good, but I think we want tests ?

Now with more tests.
(Assignee)

Comment 13

3 years ago
Comment on attachment 8657931 [details] [diff] [review]
[gaia] cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Caused by a design flaw in Find My Device's lock function.

[User impact] if declined:
Even if the user requests to set a new pass code on FMD lock, the old pass code will remain active.

[Testing completed]:
Local gaia FMD tests passing.

[Risk to taking this patch] (and alternatives if risky):
Low risk. One-liner change, and a matching test is implemented, too.

[String changes made]:
None

Please also approve 2.2r uplift.
Attachment #8657931 - Flags: approval-gaia-v2.2?(mpotharaju)
(Assignee)

Comment 14

3 years ago
RTL on master
Keywords: checkin-needed
(In reply to Christiane Ruetten [:cr] from comment #12)
> (In reply to Alexandre LISSY :gerard-majax from comment #11)
> > Comment on attachment 8657930 [details] [review]
> > [gaia] cr:fmdpasscodefix > mozilla-b2g:master
> > 
> > Looks good, but I think we want tests ?
> 
> Now with more tests.

Perfect !
https://github.com/mozilla-b2g/gaia/commit/2ba46821f800d7170d54d22118a2c73f2b341ce9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)

Updated

3 years ago
Keywords: checkin-needed
Comment on attachment 8657931 [details] [diff] [review]
[gaia] cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2

Based on comment 13, approved to uplift to 2.2
Attachment #8657931 - Flags: approval-gaia-v2.2?(mpotharaju) → approval-gaia-v2.2+
merged 2.2 to 2.2r
status-b2g-v2.2r: affected → fixed
christiane, could you take a look at https://treeherder.mozilla.org/logviewer.html#?job_id=169578&repo=mozilla-b2g37_v2_2 - not sure if thats intermittent or not (retriggered tests, but definitly something we should fix i guess)
Flags: needinfo?(cr)
sorry had to revert this for perma failing on 2.2 and 2.2r see comment #21 and #20
status-b2g-v2.2: fixed → affected
status-b2g-v2.2r: fixed → affected
is this still wanted on 2.2 ? this got backed up for test failures 2 weeks ago ?
Flags: needinfo?(mpotharaju)
Flags: needinfo?(jocheng)
(Assignee)

Comment 24

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #23)
> is this still wanted on 2.2 ? this got backed up for test failures 2 weeks
> ago ?

Yes. I didn't get around to analyze the issue, yet. Things worked fine when I submitted, and I can't even fathom how it could break the way it did. So I figure it requires some time to understand the problem and fix the code and/or tests, which I won't have until late October. :/
Flags: needinfo?(cr)

Comment 25

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #23)
> is this still wanted on 2.2 ? this got backed up for test failures 2 weeks
> ago ?

Currently we only have partner device launch on 2.2r but not 2.2. Thus 2.2 can wait.
NI Wesley and Vance for whether 2.2r needs this and how much time do we have.
Flags: needinfo?(whuang)
Flags: needinfo?(vchen)
Flags: needinfo?(jocheng)
feature-b2g: --- → 2.2r+
Flags: needinfo?(whuang)
(Assignee)

Comment 26

3 years ago
Comment on attachment 8657931 [details] [diff] [review]
[gaia] cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2

Fixed failing test, else identical.
Attachment #8657931 - Attachment filename: Github pull request #31731 → Github pull request #32267
Attachment #8657931 - Attachment is patch: true
Attachment #8657931 - Attachment mime type: text/x-github-pull-request → text/plain
(Assignee)

Updated

3 years ago
Attachment #8657931 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Created attachment 8670292 [details] [review]
cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2

Same patch, fixed test.
(Assignee)

Comment 28

3 years ago
Comment on attachment 8670292 [details] [review]
cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2

[Approval Request Comment]
Same patch as approved before, but with a fixed test.
Attachment #8670292 - Flags: review?(lissyx+mozillians)
Attachment #8670292 - Flags: approval-gaia-v2.2r?(mpotharaju)
Attachment #8670292 - Flags: approval-gaia-v2.2?(mpotharaju)
Comment on attachment 8670292 [details] [review]
cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2

More confident r+ given we have tests running on gaia-try for v2.2r now :)
Attachment #8670292 - Flags: review?(lissyx+mozillians) → review+
(Assignee)

Comment 30

3 years ago
(In reply to Alexandre LISSY :gerard-majax from comment #29)
> More confident r+ given we have tests running on gaia-try for v2.2r now :)

Indeed more confident, but there's no way you could have caught this issue. It was caused by test mismatch introduced by another patch that landed after your review and before this one made it into the tree.
Clearing NI based on comment 25 from Josh.
Flags: needinfo?(mpotharaju)
(Assignee)

Comment 32

3 years ago
Comment on attachment 8670292 [details] [review]
cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2

I think we can do without :mahe's approval for the new patch. He already approved uplifting of the previous version.
Attachment #8670292 - Flags: approval-gaia-v2.2r?(mpotharaju)
Attachment #8670292 - Flags: approval-gaia-v2.2?(mpotharaju)
(Assignee)

Comment 33

3 years ago
Checkin to v2.2 needed. Uplift approval was already given in attachment 8657931 [details] [diff] [review].
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
status-b2g-v2.0: affected → wontfix
status-b2g-v2.0M: affected → wontfix
status-b2g-v2.1: affected → wontfix
status-b2g-v2.1S: affected → wontfix
status-b2g-v2.2: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/da21225cfeca

merged over to 2.2r
status-b2g-v2.2r: affected → fixed
You need to log in before you can comment on or make changes to this bug.