Closed Bug 947855 Opened 8 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S1 (14feb)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jaoo, Assigned: jaoo)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(2 files, 3 obsolete files)

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.
Need to figure out whether this bugs affects Gaia, Gecko or both. I'll keep this on RIL at the moment.
I think this is all in the system app in Gaia.
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.
If Germany is a launch country for v1.3, this functionality is a Mandatory requirement that will block the certification process. Nominating accordingly.
blocking-b2g: --- → 1.3?
1.3+ for cert blocker
blocking-b2g: 1.3? → 1.3+
PM retriaged this and we agree with the blocking status.
Whiteboard: [FT:RIL]
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.
(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
Flags: needinfo?(jjong)
Flags: needinfo?(htsai)
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
(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?
Flags: needinfo?(htsai)
(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. :)
Flags: needinfo?(ptheriault)
(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.
(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.
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! :)
Flags: needinfo?(jjong)
(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!
Flags: needinfo?(jonas)
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.
Flags: needinfo?(ptheriault)
To be clear: also fine to add to IccManager too if you would prefer from a security perspective.
(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.
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?
(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.
(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.
(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.
(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 :)
Attachment #8363087 - Attachment description: WIP branch at https://github.com/jaoo/gaia/commits/947855 → WIP branch at https://github.com/jaoo/gaia/tree/947855
Component: RIL → Gaia::Settings
Clearing ni? to Jonas.
Flags: needinfo?(jonas)
As long as Hsin-Yi is fine with changes to the IccManager or MobileConnection API then that's fine with me.
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #8368552 - Flags: review?(kaze)
See Also: → 965826
Comment on attachment 8368552 [details] [diff] [review]
v1

Adding Vivien as a reviewer to add some parallelism. Whoever gets to review it first.
Attachment #8368552 - Flags: review?(21)
Comment on attachment 8368552 [details] [diff] [review]
v1

Kaze makes much more sense than me here :)
Attachment #8368552 - Flags: review?(21)
Attachment #8368552 - Flags: review?(kaze) → review+
Thanks for the review Fabien!

Travis went green. Landed on gaia master branch at https://github.com/mozilla-b2g/gaia/commit/8b298d51cb928f92b216b0f47e75d224568b9dfb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(josea.olivera)
Can we get ETA to fix this bug? Thank you.
(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.
Flags: needinfo?(josea.olivera)
Attached patch v2Splinter Review
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.
Attachment #8368552 - Attachment is obsolete: true
Attachment #8370619 - Flags: review?(kaze)
(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.
(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 on attachment 8370619 [details] [diff] [review]
v2

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

LGTM. Thanks for making it so easy to review.
Attachment #8370619 - Flags: review?(kaze) → review+
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.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: 1.3 C3/1.4 S3(31jan) → 1.4 S1 (14feb)
Uplifted f0f269123886391106ebb37ce738274c1c91df84 to:
v1.3: 0e68cbaefcfd32cdbb914d1175d98254234804d2
Tested (02/10/2014) and working:
1.3
Gecko b71e4b8
Gaia bf79ve1
Status: RESOLVED → VERIFIED
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
Status: VERIFIED → RESOLVED
Closed: 7 years ago7 years ago
You need to log in before you can comment on or make changes to this bug.