947855 - Automatic selection of APN relying on the IMSI code in the ICC card

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[José Antonio Olivera Ortega [:jaoo]]

The APN to be used might rely also on the IMSI code in the ICC card. This is the case for pre-paid and post-paid customers. The APN to be used depends on the 6th digit of the IMSI, since it identifies the type of subscription.

We should add a general mechanism for handling this since this feature affects many carriers.

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Need to figure out whether this bugs affects Gaia, Gecko or both. I'll keep this on RIL at the moment.

Comment 2

[Andreas Gal :gal]

I think this is all in the system app in Gaia.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Just found AOSP has added some support for accomplishing this through a couple of new properties (mvno_match_data, mvno_type) in the APN. Well this is for MVNOs but the mechanism could apply to solve what we are dealing with here. I'll keep you updated.

Comment 4

[Beatriz Rodríguez [:brg]]

If Germany is a launch country for v1.3, this functionality is a Mandatory requirement that will block the certification process. Nominating accordingly.

Comment 5

[Preeti Raghunath(:Preeti)]

1.3+ for cert blocker

Comment 6

[Peter Dolanjski [:pdol]]

PM retriaged this and we agree with the blocking status.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Just talked to Jessica on IRC. The idea would be to hack mainly in Gaia side for this bug but we would need some support from gecko side (RIL plumbing). Since the APN pre-selection relies on the IMSI code in the ICC card we would need a function (or a couple of functions) to check whether the mvno_match_data (a IMSI pattern) property from the APN matches the IMSI code. FYI the IMSI code cannot be exposed to content and we cannot perform this check in Gaia side without gecko support.

I'll write down some notes about the design and share with you guys so that way we can discuss it later.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #7)

> I'll write down some notes about the design and share with you guys so that
> way we can discuss it later.

Here are some notes about the design I have in mind. Hsin-Yi, Jessica, could you take a look at it and provide some feedback please? Thanks!

https://docs.google.com/document/d/1mbY85HBu-pHi5ZmINsX-Buhuw_1avgycoz3T4mX8WZM

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP branch at https://github.com/jaoo/gaia/tree/947855

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #7)
> 
> > I'll write down some notes about the design and share with you guys so that
> > way we can discuss it later.
> 
> Here are some notes about the design I have in mind. Hsin-Yi, Jessica, could
> you take a look at it and provide some feedback please? Thanks!
> 
> https://docs.google.com/document/d/1mbY85HBu-pHi5ZmINsX-
> Buhuw_1avgycoz3T4mX8WZM

Hi José,

Thanks for the document. It's so nice to know more about this.

So, on android, imsiMatches() and mvnoMatches() both are on the framework layer. However, since in FFOS, apn.json is addressed on gaia and in short-term we don't have plans to move the logic to gecko, in this case, we need imsiMatches() on gecko with mvnoMatches() on gaia. My understanding correct?

One more thought: since 'imsi' is critical and imsiMatches() could still provide useful information for hackers (they could provide different patterns and repeatedly calls imsiMatches() until they get the match), are we going to have security mechanism to forbid retry too many times in a very short time?

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #10)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> > (In reply to José Antonio Olivera Ortega [:jaoo] from comment #7)
> > 
> > > I'll write down some notes about the design and share with you guys so that
> > > way we can discuss it later.
> > 
> > Here are some notes about the design I have in mind. Hsin-Yi, Jessica, could
> > you take a look at it and provide some feedback please? Thanks!
> > 
> > https://docs.google.com/document/d/1mbY85HBu-pHi5ZmINsX-
> > Buhuw_1avgycoz3T4mX8WZM
> 
> Hi José,
> 
> Thanks for the document. It's so nice to know more about this.
> 
> So, on android, imsiMatches() and mvnoMatches() both are on the framework
> layer. However, since in FFOS, apn.json is addressed on gaia and in
> short-term we don't have plans to move the logic to gecko, in this case, we
> need imsiMatches() on gecko with mvnoMatches() on gaia. My understanding
> correct?
> 
> One more thought: since 'imsi' is critical and imsiMatches() could still
> provide useful information for hackers (they could provide different
> patterns and repeatedly calls imsiMatches() until they get the match), are
> we going to have security mechanism to forbid retry too many times in a very
> short time?

Now, mozIccManager and mozMobileConnections are certified-app only, and I don't see plans to bend the privilege rules on these APIs, so it might be okay without other security mechanism in additional to the existing permission control. However, I'd like to see if Paul has other concerns. :)

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #10)

> Thanks for the document. It's so nice to know more about this.

Thanks for your feedback!

> So, on android, imsiMatches() and mvnoMatches() both are on the framework
> layer. However, since in FFOS, apn.json is addressed on gaia and in
> short-term we don't have plans to move the logic to gecko, in this case, we
> need imsiMatches() on gecko with mvnoMatches() on gaia. My understanding
> correct?

Yes, it is. mvnoMatches() function will live on Gaia side.

> One more thought: since 'imsi' is critical and imsiMatches() could still
> provide useful information for hackers (they could provide different
> patterns and repeatedly calls imsiMatches() until they get the match), are
> we going to have security mechanism to forbid retry too many times in a very
> short time?

Oops, I did realize this could happen. Thanks for raising this. Let's wait Paul's feedback.

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #12)
> Oops, I did realize this could happen. Thanks for raising this. Let's wait
> Paul's feedback.

I didn't.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Hi José,

I am fine with it as it seems our only option if imsi can not be exposed. But it involves api changes, so we need approval from webapi and security people. Let's wait for their feedback! :)

Comment 15

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #14)

Thanks for your feedback too!

> I am fine with it as it seems our only option if imsi can not be exposed.
> But it involves api changes, so we need approval from webapi and security
> people. Let's wait for their feedback! :)

For the webapi team approval for this function, I guess we should request feedback to Jonas I'll do it them.

Jonas, we need to add a new function to the mozMobileConnection API (or mozIccManager (mozIcc) API), there some background at comment #7 and #8. Moreover you can take a look at the discussion through the comments. Could you provide some feedback please? Thanks!

Comment 16

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

I'm not familiar with any security requirements of the IMSI (carriers may not want the IMSI exposed to third party apps?) There is a privacy component, but this is similar to the user's phone number from a privacy perspective. (perhaps even less important than phone number). So from a privacy perspective I don't see any reason not to expose this certified apps only.

BTW: imsiMatches() is equivalent security to just exposing the IMSI as Hsin-Yi points out. (Especially since IMSI is MCC + MNC + MSIN and we already expose MCC & MNC) 

But I don't see any issues with adding this to mozMobileConnection API - it is certified only. We may expose a subset of this API to privileged apps in the future (bug 855167) but I think we would want to keep the ISMI restricted to certified apps only.

Comment 17

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

To be clear: also fine to add to IccManager too if you would prefer from a security perspective.

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Paul Theriault [:pauljt] from comment #17)
> To be clear: also fine to add to IccManager too if you would prefer from a
> security perspective.

Thanks, Paul!

Then, it looks fine to me to introduce imsiMatches() to IccManager API.

Comment 19

[Jessica Jong [:jessica] (inactive)]

IMHO, for a long-term solution, I think we should expose the API as: 'mvnoMatches(mvnoType, matchPattern)', but we will only support 'imsi' mvnoType for now. Then, mvnoMatches() will call imsiMatches() internally. In the future, we can add support for 'spn' and 'gid' mvnoType as well, like Android.
What do you think?

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #19)
> IMHO, for a long-term solution, I think we should expose the API as:
> 'mvnoMatches(mvnoType, matchPattern)', but we will only support 'imsi'
> mvnoType for now. Then, mvnoMatches() will call imsiMatches() internally. In
> the future, we can add support for 'spn' and 'gid' mvnoType as well, like
> Android.
> What do you think?

Yeah, I was thinking about the concept, too. Vote for it!!!
With the security perspective, the security concern is the same. Regarding the API design, I like this more.

Comment 21

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #20)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #19)
> > IMHO, for a long-term solution, I think we should expose the API as:
> > 'mvnoMatches(mvnoType, matchPattern)', but we will only support 'imsi'
> > mvnoType for now. Then, mvnoMatches() will call imsiMatches() internally. In
> > the future, we can add support for 'spn' and 'gid' mvnoType as well, like
> > Android.
> > What do you think?
> 
> Yeah, I was thinking about the concept, too. Vote for it!!!
> With the security perspective, the security concern is the same. Regarding
> the API design, I like this more.

Security wise, this will be behind the behind the mobileconnection permission, I assume (as part of the mozIcc interface?). And if a hacker gets hold of an app with the mobile connection permission...

* He can lock the SIM card by sending an invalid PIN repeatedly, leaving the user without service
* He can send any random APDU to the card
* He can read *and* modify the contacts stored on the SIM card

that just to mention some of the interesting things he can do :). I don't think being able to profile the user card by matching a IMSI regexp is going to add much over that (maybe only as a part of a combined attack if for some reason he wanted to lock the cards only of prepaid users...). Risk wise, as Paul said, I don't see any problem with this either.

Comment 22

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #19)
> IMHO, for a long-term solution, I think we should expose the API as:
> 'mvnoMatches(mvnoType, matchPattern)', but we will only support 'imsi'
> mvnoType for now. Then, mvnoMatches() will call imsiMatches() internally. In
> the future, we can add support for 'spn' and 'gid' mvnoType as well, like
> Android.

This is really good, the imsi case was the first step and what is a requirement right now. By exposing 'mvnoMatches(mvnoType, matchPattern)' function we will fully cover the automatic selection of APNs for MVNO carriers as well.

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #22)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #19)
> > IMHO, for a long-term solution, I think we should expose the API as:
> > 'mvnoMatches(mvnoType, matchPattern)', but we will only support 'imsi'
> > mvnoType for now. Then, mvnoMatches() will call imsiMatches() internally. In
> > the future, we can add support for 'spn' and 'gid' mvnoType as well, like
> > Android.
> 
> This is really good, the imsi case was the first step and what is a
> requirement right now. By exposing 'mvnoMatches(mvnoType, matchPattern)'
> function we will fully cover the automatic selection of APNs for MVNO
> carriers as well.

Great! Bug 963516 is filed for gecko part!
Thanks, jaoo and Jessica :)

Comment 24

[José Antonio Olivera Ortega [:jaoo]]

Clearing ni? to Jonas.

Comment 25

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

As long as Hsin-Yi is fine with changes to the IccManager or MobileConnection API then that's fine with me.

Comment 26

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/15869

Comment 27

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

Some background for the reviewer first. The patch in this bug allows the device to pre-select the APN relying on the IMSI code in the ICC card. The APNs in the apn.json database have a couple of new properties (such as mvno_type and mvno_macth_data) that allows to accomplish this feature. The MVNO rules (the properties before mentioned) allow us to pre-select the APN relying on several things such as the IMSI code, the carrier name in the ICC card, etc. A new function has been added to the mozIcc objects to check whether given an APN with these properties it might be the one to be used for the ICC card in use. So having all those pieces we just needed to add a function to pre-select the APN on boot. This patch adds this function to the operator variant logic so once we have the list of all APNs for the MCC and MNC codes in the SIM we just need to filter the list by using the matching function.

Fabien, since you already know the big picture of the APNs thing you are the one that will review this change better so, could you take a look at it please? Thanks!

PS. The PR link is also attached, please feel free to review the patch through the PR if you prefer. I'll be happy to help you to understand what is going on if needed, just ping me on IRC.

Comment 28

[Anthony Ricaud (:rik)]

Comment on attachment 8368552 [details] [diff] [review]
v1

Adding Vivien as a reviewer to add some parallelism. Whoever gets to review it first.

Comment 29

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8368552 [details] [diff] [review]
v1

Kaze makes much more sense than me here :)

Comment 30

[José Antonio Olivera Ortega [:jaoo]]

Thanks for the review Fabien!

Travis went green. Landed on gaia master branch at https://github.com/mozilla-b2g/gaia/commit/8b298d51cb928f92b216b0f47e75d224568b9dfb

Comment 31

[Jonathan Griffin (:jgriffin)]

Backed out for causing a perma-orange on gaia-unit tests on TBPL:

https://github.com/mozilla-b2g/gaia/commit/734d35f1b175456784ca41b69f45568aaa689cd6

Logs:

https://tbpl.mozilla.org/php/getParsedLog.php?id=34093518&tree=B2g-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=34091734&tree=B2g-Inbound&full=1

Comment 32

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi José,
Bad news that this patch was backed out.
Could we have an ETA for getting this fixed? 
Just mark ETA:2/? on the whiteboard.

Comment 33

[Kai-Chih Hu [:khu]]

Can we get ETA to fix this bug? Thank you.

Comment 34

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Jonathan Griffin (:jgriffin) from comment #31)
> Backed out for causing a perma-orange on gaia-unit tests on TBPL:
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=34093518&tree=B2g-
> Inbound&full=1
> https://tbpl.mozilla.org/php/getParsedLog.php?id=34091734&tree=B2g-
> Inbound&full=1

Sorry for the inconveniences. BTW this error should show up in Travis, shouldn't it? Working on a fix right now.

Comment 35

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/15968

Comment 36

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

Fabien, this version of the patch fixes the gaia-unit bustage on TBPL. Could you take a look a it please? Thanks.

PS. Feel free to review the patch through the PR link. I didn't squash the commits so you will see the fix clearly.

Comment 37

[Jonathan Griffin (:jgriffin)]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #34)
> 
> Sorry for the inconveniences. BTW this error should show up in Travis,
> shouldn't it? Working on a fix right now.

The way the tests are run in TBPL provide better test isolation than the way they're run in Travis, unfortunately, so Travis doesn't necessarily catch all the problems that TBPL will.

Comment 38

[Fabien Cazenave [:kaze]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #36)
> PS. Feel free to review the patch through the PR link. I didn't squash the
> commits so you will see the fix clearly.

Awesome, thanks! <3

Comment 39

[Fabien Cazenave [:kaze]]

Comment on attachment 8370619 [details] [diff] [review]
v2

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

LGTM. Thanks for making it so easy to review.

Comment 40

[José Antonio Olivera Ortega [:jaoo]]

Thanks for the review Fabien.

Landed on Gaia master branch -again- at https://github.com/mozilla-b2g/gaia/commit/f0f269123886391106ebb37ce738274c1c91df84

Jonathan, the patch landed seems to fix the issue you found. Travis went green. I wasn't able to run all the unit test from the system app. Even on a clean clone of gaia some tests in the system app fails (How is that possible?). I ended up running the unit test failing. These tests are now passing so I expect this patch won't brake anything.

Comment 41

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted f0f269123886391106ebb37ce738274c1c91df84 to:
v1.3: 0e68cbaefcfd32cdbb914d1175d98254234804d2

Comment 42

[Loli (:lolimartinezcr)]

Tested (02/10/2014) and working:
1.3
Gecko b71e4b8
Gaia bf79ve1

Comment 43

[Loli (:lolimartinezcr)]

Only tried that with this development continues to operate the detection of apn, I change status to "Resolved Fixed".

For verified this bug is necesary also: https://bugzilla.mozilla.org/show_bug.cgi?id=965826 and I need to test it with Germany sim card