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"

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?

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

The module owner currently listed, Guilherme Gonçalves, seems to have left Mozilla. Who is currently responsible for FMD?

Alexandre pointed out to me that according to bug 945889, this is not a feature, but a bug.

I don't think this bug needs to be closed, because knowledge about it does not expose additional attack surface.

Comment on attachment 8657930 [details] [gaia] cr:fmdpasscodefix > mozilla-b2g:master Trivial fix, please review.

Comment on attachment 8657931 [details] [diff] [review] [gaia] cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2 Trivial fix, please review.

Comment on attachment 8657930 [details] [gaia] cr:fmdpasscodefix > mozilla-b2g:master Looks good, but I think we want tests ?

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

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.

RTL on master

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

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

merged 2.2 to 2.2r

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)

It's also broken on 2.2r, for the record: https://treeherder.mozilla.org/#/jobs?repo=mozilla-b2g37_v2_2r&fromchange=03267130c48e&filter-searchStr=%28Gu%29

sorry had to revert this for perma failing on 2.2 and 2.2r see comment #21 and #20

is this still wanted on 2.2 ? this got backed up for test failures 2 weeks 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. :/

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

Comment on attachment 8657931 [details] [diff] [review] [gaia] cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2 Fixed failing test, else identical.

Same patch, fixed test.

Comment on attachment 8670292 [details] cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2 [Approval Request Comment] Same patch as approved before, but with a fixed test.

Comment on attachment 8670292 [details] cr:fmdpasscodefix-v2.2 > mozilla-b2g:v2.2 More confident r+ given we have tests running on gaia-try for v2.2r now :)

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

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

Checkin to v2.2 needed. Uplift approval was already given in attachment 8657931 [details] [diff] [review].

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/da21225cfeca merged over to 2.2r