866272 - expose privileged access to mcc+mnc pair for last home network and roaming network

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Privacy concerns:

- exposing last known network is not a problem I think, it can be looked up with some effort via the IP
- exposing the home network is more dicey, however, for 99% of people current == home network, so probably not that big of a deal either

Comment 3

[Michael Vines [:m1] [:evilmachines]]

(In reply to Andreas Gal :gal from comment #1)
> Created attachment 742544 [details] [diff] [review] patch

I'd prefer a more rigid interface than adding another squishy setting like "ril.lastKnownNetwork".

Comment 4

[Andreas Gal :gal]

Attachment: updated patch, require "mobilenetwork" permission and privileged/certified app status

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

I think for most people this isn't going to be privacy-invasive.  As you said, Andreas, it's something that can be inferred from the IP.

But I can imagine there are some cases where this is potentially iffy, especially if someone uses a VPN or proxy to access stuff.  Imagine someone who has lots of sim cards (not out of the question) and travels a lot.  An app or site who can read network IDs and is regularly used (think twitter) can learn a bit about my travel habits that maybe I don't want.

Granted, this is not a majority case and the privacy risks here probably aren't severe.  But I think we need some discussion in the forums about this to tease out what might not be obvious.

Comment 6

[Andreas Gal :gal]

Attachment: patch

privacy people: we only expose this information to privileged apps with the "mobilenetwork" permission

Comment 7

[[:fabrice] Fabrice Desré]

Andreas, "Preferences are as the name implies intended for preferences. There is no sane use case for storing data in preferences. I would give any patch I come across doing that an automatic sr- for poor taste and general insanity" ;)

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Andreas Gal :gal from comment #6)
> privacy people: we only expose this information to privileged apps with the
> "mobilenetwork" permission

Sounds like minimal risk, though this is different than the subject of the bug.  "expose unprivileged access..."

Comment 9

[Andreas Gal :gal]

Attachment: patch for inbound

Comment 10

[Andreas Gal :gal]

Attachment: patch for b2g18

Comment 11

[Andreas Gal :gal]

comment 7: I am duplicating what the code is currently doing, and I signed up for https://bugzilla.mozilla.org/show_bug.cgi?id=866238 to end the insanity

Comment 12

[Andreas Gal :gal]

We need marketplace to give this a try.

Comment 13

[Wil Clouser [:clouserw]]

(In reply to Andreas Gal :gal from comment #12)
> We need marketplace to give this a try.

Chris is pulling in your patch and compiling

Comment 14

[Christopher Van Wiemeersch [:cvan]]

Comment on attachment 742573 [details] [diff] [review]
patch for inbound

Review of attachment 742573 [details] [diff] [review]:
-----------------------------------------------------------------

I'm by no means suited to give real feedback on this code, but here are a few things I noticed. Thanks!

::: dom/apps/src/PermissionsTable.jsm
@@ +118,5 @@
>                               app: DENY_ACTION,
>                               privileged: DENY_ACTION,
>                               certified: ALLOW_ACTION
>                             },
> +			   mobilenetwork: {

Can we change these tabs to soft spaces?

::: dom/network/src/MobileConnection.cpp
@@ +11,5 @@
>  #include "nsIDOMCFStateChangeEvent.h"
>  #include "nsIDOMICCCardLockErrorEvent.h"
>  #include "GeneratedEvents.h"
> +#include "mozilla/Preferences.h"
> +#include "nsIPermissionManager.h"

Does order matter here?

@@ +436,5 @@
>  
>  NS_IMETHODIMP
>  MobileConnection::NotifyVoiceChanged()
>  {
> +  if (!CheckPermission("mobileconnection") || !CheckPermission("mobileconnection")) {

Is this a typo? Checking `!CheckPermission` twice?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1083,5 @@
>            } catch (e) {}
>          }
>        }
>  
> +      // Update lastKnownNetwork

For consistency with the line below we could end this comment with a period.

@@ +1085,5 @@
>        }
>  
> +      // Update lastKnownNetwork
> +      if (message.mcc && message.mnc) {
> +	try {

Tabs again here.

@@ +1797,5 @@
>  
> +    // Update lastKnownHomeNetwork.
> +    if (message.mcc && message.mnc) {
> +      try {
> +	services.prefs.setCharPref("ril.lastKnownHomeNetwork", message.mcc + message.mnc);

Shouldn't `services` be capital `Services`?

Comment 15

[Christopher Van Wiemeersch [:cvan]]

I'm on Mac OS X 10.8.2 and I'm having difficulties building the emulator because of this error:

    external/qemu/distrib/sdl-1.2.12/src/video/maccommon/SDL_lowvideo.h:59:2: error: unknown type name 'PaletteHandle'

My log is here:

    http://pastebin.mozilla.org/2357970

This seems to be the same issue as described in bug 814928 and http://www.andrewsavory.com/blog/2012/2463 - if anyone can point me in the right direction, that'd be awesome.

Anyhow, CC'ing Basta to see if he can spin up a build ASAP and apply the patch above on his Snow Leopard machine.

Comment 16

[[:fabrice] Fabrice Desré]

Hey Chris,

I'll take a closer look at this patch tomorrow.

Comment 17

[Wil Clouser [:clouserw]]

(In reply to Fabrice Desré [:fabrice] from comment #16)
> Hey Chris,
> 
> I'll take a closer look at this patch tomorrow.

I think Andreas wanted to uplift this today.  Andreas - what's the priority here?

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to Wil Clouser [:clouserw] from comment #17)
> (In reply to Fabrice Desré [:fabrice] from comment #16)
> > Hey Chris,
> > 
> > I'll take a closer look at this patch tomorrow.
> 
> I think Andreas wanted to uplift this today.  Andreas - what's the priority
> here?

Look at my comment date...

Comment 19

[David Chan [:dchan]]

Removing the permission check in navgiator.cpp seems like it would be the opposite of what bug 859554 wants to achieve.

Would it be possible to check in navigator.cpp and add an OR statement for this use case?

Comment 20

[Andreas Gal :gal]

How would that OR statement look like?

Comment 21

[David Chan [:dchan]]

(In reply to Andreas Gal :gal from comment #20)
> How would that OR statement look like?

My mistake, the check should really be an && in navigator.cpp

if (!CheckPermission("mobileconnection") && !CheckPermission("mobilenetwork") {		
    return NS_OK;		
}

If the page has either mobileconnection or mobilenetwork, then it gets the mobileconnection object. The per API checks would remain as in the current patch since having either the mobileconnection or mobilenetwork permission doesn't imply a principal has the other.

Comment 22

[[:fabrice] Fabrice Desré]

Attachment: updated inbound patch

Updated patch, fixing nits and Navigator.cpp to do the right thing.

Comment 23

[Andreas Gal :gal]

Is this working? Ready to land?

Comment 24

[[:fabrice] Fabrice Desré]

Nope, sadly. I'm working on adding what's needed to not be killed at https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#387

Comment 25

[[:fabrice] Fabrice Desré]

Attachment: inbound patch, working

This patch makes the "mobilenetwork" mozMobileConnection a very stripped down object (eg, no event listeners will work). That's probably enough for our immediate needs. If we need the event listeners, I can work on a updated version with more intrusive changes.

Comment 26

[Alex Keybl [:akeybl]]

(In reply to Fabrice Desré [:fabrice] from comment #25)
> Created attachment 744020 [details] [diff] [review]
> inbound patch, working
> 
> This patch makes the "mobilenetwork" mozMobileConnection a very stripped
> down object (eg, no event listeners will work). That's probably enough for
> our immediate needs. If we need the event listeners, I can work on a updated
> version with more intrusive changes.

Who can review? Is this change risky at all? If so, we'll need to target 5/10 to hit our launch date.

Comment 27

[Andreas Gal :gal]

Comment on attachment 744020 [details] [diff] [review]
inbound patch, working

This is a pretty hacky fix obviously. We need a better patch that exposes a part of the object but allows the listeners associated with it. We can do that for 1.2. We need to uplift this to b2g18.

Comment 28

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/birch/rev/a12273aa858b

Comment 29

[Andreas Gal :gal]

Actually this is wrong:

   6.60 +        services.prefs.setCharPref("ril.lastKnownHomeNetwork",
    6.61 +                                   message.mcc + "-" + message.mnc);

Fabrice can you land a follow-on?

Comment 30

[[:fabrice] Fabrice Desré]

typo fix: https://hg.mozilla.org/projects/birch/rev/1ae920a04d36

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Backed out for Marionette failures.
https://hg.mozilla.org/projects/birch/rev/dea9d8108b8e

https://tbpl.mozilla.org/php/getParsedLog.php?id=22536425&tree=Birch

Comment 32

[Jason Smith [:jsmith]]

Triage - Renom for more discussion.

See https://bugzilla.mozilla.org/show_bug.cgi?id=867793#c3 for reasoning.

Comment 33

[Jason Smith [:jsmith]]

Got clarity on this now - we can tef+ again.

But we still need to address the risks brought up in https://bugzilla.mozilla.org/show_bug.cgi?id=867793#c4.

Comment 34

[[:fabrice] Fabrice Desré]

Try build with the a small fix to pass the Mn tests:
https://tbpl.mozilla.org/?tree=Try&rev=58599c8a7c6b

Comment 35

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/birch/rev/730f44b9f32a

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/730f44b9f32a

Comment 37

[Ryan VanderMeulen [:RyanVM]]

This needs a b2g18-specific patch.

Comment 38

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0c71cbc5fe0c

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8ca3e3057c6a

Comment 40

[Nivi]

Hi Fabrice,

Why are we setting LastKnownNetwork twice in RadioInterfaceLayer.js?
Is that intentional? I don't see that in the original patch or the patch in comment 35.

Nivi.


(In reply to Fabrice Desré [:fabrice] from comment #38)
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/0c71cbc5fe0c

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Paul and Antonio: Can you make sure that the TEF apps for v1.0 that needed to have per-country specific behavior use this API rather than the other solutions that we had looked at.

Paul: Can you make sure that this API ends up in the permission matrix and make sure to document that this only has permission checks in the child process. We need to make sure we add them to the parent process too, which will mean using another solution than preferences here.

Comment 42

[[:fabrice] Fabrice Desré]

Jonas, don't you think that what we need there is a readonly access mode on the mobile connection api rather than keeping this extra permission?

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I definitely think that what was done here was a hack and we should use a different solution.

I don't think a readonly mode for mobileconnection is the way to go. There's much more information on mobileconnection than just mcc+mnc. mcc+mnc has been asked for quite a few times so I think we should only expose that information on a separate API. Additionally mobileconnection is changing a lot between releases still so we should try to not use that at all for 3rd party apps.

So what I'd like to see is an API specifically for mcc+mnc and then expose that under the new permission.

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

An alternative solution to the common mcc+mnc request is to enable some global mechanism for using different sets of preinstalled apps for different countries. And then the phone on first startup detects which apps to install based on internal knowledge about mcc+mnc during first startup.

That way we don't have to worry about the user travelling to a different country, or inserting a friends sim card, and suddenly getting a different UX. My understanding is that that is not a desirable thing for most users.

Comment 46

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Based on https://en.wikipedia.org/wiki/Mobile_country_code, Are we trying to get the which vendor the SIM was purchased from ?  

Currently the implementation seems to getting the information from the network not the SIM.

Comment 47

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Er sorry.  I think I commented in the wrong bug.  This only shows the last mcc+mnc pair.  ignore the last statement.

Comment 49

[John Hammink]

Attachment: adb dump: MCC & MNC for Entel + Ikura

ran adb logcat grepping for MCC and MNC both when 1.)turning on the device with ENTEL SIM card on chilean network and 2.) trying to launch marketplace. I've annotated the log.

Comment 50

[Christopher Van Wiemeersch [:cvan]]

John, have you gotten an update of Marketplace (version 0.0.4)? Bug 868571 makes it so 0.0.4 is the version of the Marketplace shipped with Gaia.

Comment 51

[John Hammink]

I can retest with a 5/17 vendor build if this helps.  I'm working remotely on an Ikura running a partner build (I'm at ZTE in Chile) so no possibility here to install Mozilla build.   I also do not have an opportunity to build from tip here, so I expect the fix has landed in 5/17 build.  How to check the version of Marketplace on this build?

Comment 52

[John Hammink]

I can retest with a 5/17 vendor build if this helps.  I'm working remotely on an Ikura running a partner build (I'm at ZTE in Chile) so no possibility here to install Mozilla build.   I also do not have an opportunity to build from tip here, so I expect the fix has landed in 5/17 build.  How to check the version of Marketplace on this build?

Comment 53

[John Hammink]

Sorry about the double comment - terrible network here.

Comment 54

[John Hammink]

Thanks for the clarification, cvan.

However, if I understand correctly then, the fix uplifted for 
https://bugzilla.mozilla.org/show_bug.cgi?id=868571

didn't land until afternoon PST.  

That means it's most likely not in the partner build I am running. Since I won't build from tip on this very slow network, we'll need to wait a bit to verify this fix.

Comment 55

[John Hammink]

Attachment: adb dump: MCC & MNC for Entel and Movistar prepaid SIMs on Ikura

I checked the mmc|mnc pair for two prepaid SIM cards down here in Chile; Movistar and Entel. Phone was Ikura.

One was on 5/13 partner build, other was 5/17.  In the 5/17 build I could no longer run adb as root (it was a production build) therefore, RIL debugging was not enabled for that one.  However I got the same information for both, and according to the Mobile Country code spec on wikipedia: https://en.wikipedia.org/wiki/Mobile_country_code it was correct.

Comment 56

[John Hammink]

I did notice something else.

For my Movistar prepaid SIM (on my 5/17 build), I switched Roaming on and reran logcat.  But it's showing roaming as false: 

I/Gecko   (  112): -*- QCContentHelper_QC_B2G: sendMessage to content process: RIL:DataInfoChanged{connected : false,emergencyCallsOnly : false,roaming : false,type : 'umts',signalStrength : -67,rel

File another bug?

Comment 57

[Anshul]

John, yes please file a new bug and provide the android logs using the following command:

  adb logcat -b radio -b main -v time

Comment 58

[Michael Vines [:m1] [:evilmachines]]

Roaming may be enabled in the UI but if the device isn't actually roaming then that log message looks fine to me.

Comment 59

[Christopher Van Wiemeersch [:cvan]]

John, you can run

    adb logcat | grep -Ei 'MCC|MNC'

In you see logs for the MCC and MNC and requests being made to https://marketplace.firefox.com/?mcc=<MCC>&mnc=<MNC> then you have the correct, working version of the Marketplace.

Comment 60

[John Hammink]

Sorry, not in Chile anymore - wasn't possible to retest.  Somebody should follow up on Comment 56.

Comment 61

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

Clearing secrevew as this was reviewed ages ago.(In reply to Jonas Sicking (:sicking) needinfo? or feedback? encouraged. from comment #41)

> Paul: Can you make sure that this API ends up in the permission matrix and
> make sure to document that this only has permission checks in the child
> process. We need to make sure we add them to the parent process too, which
> will mean using another solution than preferences here.

Permission is documented on https://developer.mozilla.org/en-US/Apps/Developing/App_permissions and bug 947784 is raised to implement this functionality in a sandbox safe way.