Closed Bug 1058330 Opened 5 years ago Closed 5 years ago

Use the 'geolocation-noprompt' permission instead of 'permissions' (and possibly 'geolocation')

Categories

(Firefox OS Graveyard :: FindMyDevice, defect)

defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S4 (12sep)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: ggp, Assigned: ggp)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

The geolocation permission is too broad for our usecase, and that causes security concerns. See https://bugzilla.mozilla.org/show_bug.cgi?id=1036423#c6 .
It's not that "geolocation" is too broad or causes security concerns. It's the use of the "permission" permission that causes security concerns. By adding a "geolocation-noprompt" API we would no longer need to use the "permission" permission.
Thanks for clarifying. We can probably replace both 'geolocation' and 'permissions' with 'geolocation-noprompt', but that's a minor issue.
Summary: Use the 'geolocation-noprompt' permission instead of 'geolocation' → Use the 'geolocation-noprompt' permission instead of 'permissions' (and possibly 'geolocation')
[Blocking Requested - why for this release]: Bug 1058319 is landing, so geolocation-noprompt will be available. Fabrice, can you speak to risk?
blocking-b2g: --- → 2.0?
Flags: needinfo?(fabrice)
(In reply to Sam Penrose from comment #3)
> [Blocking Requested - why for this release]: Bug 1058319 is landing, so
> geolocation-noprompt will be available. Fabrice, can you speak to risk?

Risk is very low. We are actually removing risk.
Flags: needinfo?(fabrice)
Attached file GitHub pull request
Attachment #8481410 - Flags: review?(mgoodwin)
Vishy, this patch changes our interaction with the device's geolocation settings a little, so I'd like to hear your input from a product perspective. Specifically, this changes how FMD works when Geolocation is set to "Deny" under Settings > App Permissions > Find My Device.

The previous behavior was:
1. User enters Settings > App Permissions > Find My Device and sets Geolocation to "Deny"
2. Later, the user logs in to the FMD website.
3. On the device, FMD will automatically change its Geolocation permission to "Allow", and start tracking.

The problem is that step 3 greatly increases the potential damage caused by the FMD app if it's compromised. This patch, which mitigates these security concerns, will also make step 3 impossible; that is, the new behavior is:

1. and 2. Same as before
3. The device attempts to track, but receives an error from the Geolocation API, which it reports to the server. Tracking won't work unless Geolocation is set to "Allow" by whoever has the device.

It should be noted that we already fail to track when the global geolocation permission is set to false, in the main Settings menu, which is just as accessible to the user. Also, I don't believe the previous behavior was part of any user stories, but please correct me if I'm wrong. We do have a bug filed a long time ago for omitting FMD from the App Permissions (bug 984849), which we can pursue for 2.1 or later to have a more polished solution.
Flags: needinfo?(vkrishnamoorthy)
I think it is a sub-optimal experience but given the 2.0 timeframe and the security concerns, lets go with the patch.
 
However I think in the lost mode FMD should override the device Geo setting value. The following user story captures the requirement for a future release

https://bugzilla.mozilla.org/show_bug.cgi?id=1017268
Flags: needinfo?(vkrishnamoorthy)
Thanks Vishy! We can probably work out a way to implement that user story for a future release that doesn't involve giving FMD the 'permissions' permission.
See Also: → 1017268
Comment on attachment 8481410 [details] [review]
GitHub pull request

Looks good and works well.

Thanks.
Attachment #8481410 - Flags: review?(mgoodwin) → review+
Just got around to testing on the device, and it is working fine.
Keywords: checkin-needed
IIRC, I asked Paul about this on bug 1037682, but the conclusion that was reached originally was that there was no security concerns with the fact that bug was present. As a result, that bug got marked as a WONTFIX.

Paul - Can you weigh in here from a security perspective here? Has the perspective changed here?
Flags: needinfo?(ptheriault)
In bug 1037682 I was responding to the risk of granting geolocation without prompting. This bug is about solving this requirement in a much safer way. Furthermore, I hadn't considered the undesirable behavior which gpg outlines in comment 6. So all in all, I am very much in favor of making this change since it involves removing the "permissions" permission, from the FMD app (which in turn reduces the impact of a compromise of the FMD app)
Flags: needinfo?(ptheriault)
master: https://github.com/mozilla-b2g/gaia/commit/45ecc4c293a12fa09ef7e676b5b627dffc4a761a
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
blocking-b2g: 2.0? → 2.0+
Please request approval to land on 2.0.
Assignee: nobody → ggoncalves
And v2.1.
Flags: needinfo?(ggoncalves)
Comment on attachment 8481410 [details] [review]
GitHub pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): The old behavior was first introduced in bug 938901, but it was now deemed too risky.
[User impact] if declined: Increased potential for damage if the Find My Device app is compromised.
[Testing completed]: Manual and Gaia unit tests.
[Risk to taking this patch] (and alternatives if risky): Low risk, this is a simple change with high security impact.
[String changes made]: None.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
Attachment #8481410 - Flags: approval-gaia-v2.1?(bbajaj)
Attachment #8481410 - Flags: approval-gaia-v2.0?(bbajaj)
Flags: needinfo?(ggoncalves)
Attachment #8481410 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1+
Attachment #8481410 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
Needs rebasing for v2.0 uplift.
Flags: needinfo?(ggoncalves)
Attached file v2.0 pull request
There we go.

Sheriffs: I couldn't run the unit tests locally, so it's best to wait until Gaia-Try is over. I did try the patch manually on my device, and it's working well.
Flags: needinfo?(ggoncalves)
You need to log in before you can comment on or make changes to this bug.