Closed Bug 785072 Opened 7 years ago Closed 7 years ago

B2G RIL: Expose Integrated Circuit Card Identifier (ICCID) to certified apps through DOM API

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: sonmarce, Assigned: jaoo)

References

Details

(Keywords: feature, Whiteboard: [WebAPI:P0])

Attachments

(3 files, 4 obsolete files)

We need to expose the ICCID to content, as it is required for cost control application to uniquely identify a SIM. There are a number of scenarios where the information must be linked to the SIM, and for this reason we need a way to identify the SIM. For example, check balance and top-up functionalities are directly related to the SIM inserted in the phone, so all data we need to store in the phone must be associated to the SIM.
See Also: → 782603
Can you explain how this relates to bug 782603? Would the IMSI be sufficient for your purposes or do you need the ICC ID?
Summary: Expose Integrated Circuit Card Identifier (ICCID) to certified apps → B2G RIL: Expose Integrated Circuit Card Identifier (ICCID) in privileged DOM API
Well, as it seem it is not possible to use IMSI because of privacy problems, we could use ICCID instead. What we need is a way to identify the SIM, so ICCID is ok for us.
(In reply to Marcelino Veiga Tuimil from comment #2)
> Well, as it seem it is not possible to use IMSI because of privacy problems,
> we could use ICCID instead. What we need is a way to identify the SIM, so
> ICCID is ok for us.

How does the ICCID solve the privacy problem that the IMSI can't?
ICCID identifies the piece of Hardware and hence it is not related at all with the customer subscription-

IMSI is linked to the mobile subscription and having access to it opens a lot of security concerns, such as http://en.wikipedia.org/wiki/IMSI-catcher
(In reply to Daniel Coloma from comment #5)
> ICCID identifies the piece of Hardware and hence it is not related at all
> with the customer subscription-
> 
> IMSI is linked to the mobile subscription and having access to it opens a
> lot of security concerns, such as http://en.wikipedia.org/wiki/IMSI-catcher

That makes sense, thanks!
(In reply to Daniel Coloma from comment #5)
> ICCID identifies the piece of Hardware and hence it is not related at all
> with the customer subscription-
> 
> IMSI is linked to the mobile subscription and having access to it opens a
> lot of security concerns, such as http://en.wikipedia.org/wiki/IMSI-catcher

FWIW, Apple used to expose a UDID (unique device ID) to apps in it's Objective-C API, but removed it because of privacy concerns. 

http://lifehacker.com/5898282/what-a-udid-is-and-why-apples-rejecting-apps-that-want-yours

This is slightly different, because the APIs will require a higher level of permission (and not be available to just any old web app), but we should keep that in mind. These days, iOS apps will use app-install specific generated UUIDs to track users, but at least.

I have a few follow up questions:
- Other than SIM-specific queries such as cost control, balance, etc.. What (if any) data would be stored on the phone that is tied to the ICCID?
- Would the user swapping the SIM cause any problems with this approach?
- Presumably you plan to store the ICCID in IndexedDB? Would any protections be needed to ensure it is not in the clear and easily pulled from /data/local?
For a better understanding of the scenario we want to implement, you can see https://wiki.mozilla.org/images/8/87/Credit_and_Data_Usage_App.pdf (pages 47-52).
(In reply to Marshall Culpepper [:marshall_law] from comment #7)
> (In reply to Daniel Coloma from comment #5)
> > ICCID identifies the piece of Hardware and hence it is not related at all
> > with the customer subscription-
> > 
> > IMSI is linked to the mobile subscription and having access to it opens a
> > lot of security concerns, such as http://en.wikipedia.org/wiki/IMSI-catcher
> 
> FWIW, Apple used to expose a UDID (unique device ID) to apps in it's
> Objective-C API, but removed it because of privacy concerns. 
> 
> http://lifehacker.com/5898282/what-a-udid-is-and-why-apples-rejecting-apps-
> that-want-yours
> 
> This is slightly different, because the APIs will require a higher level of
> permission (and not be available to just any old web app), but we should
> keep that in mind. These days, iOS apps will use app-install specific
> generated UUIDs to track users, but at least.
> 

It is not only different because of that, but because UDID identifies the device and ICCID identifies the SIM Card. 

> I have a few follow up questions:
> - Other than SIM-specific queries such as cost control, balance, etc.. What
> (if any) data would be stored on the phone that is tied to the ICCID?

Operator variant might also benefit from this parameter and check every time a new SIM Card is inserted if new network configuration parameters (APNs, voice mail numbers..)

> - Would the user swapping the SIM cause any problems with this approach?

No, the other way around, this is to provide a way to handle that scenario

> - Presumably you plan to store the ICCID in IndexedDB? Would any protections
> be needed to ensure it is not in the clear and easily pulled from
> /data/local?
Marcelino, how are we actually going to use this API? Sorry, I just need a short answer to make a blocking call and the pdf is pretty long. Just a short description about which UI features in which apps would use this new property would be very helpful.
Whiteboard: [blocked-on-input Marcelino Veiga Tuimil]
Does this need to be exposed to privileged & certified, or can it be for certified only?
(In reply to Jonas Sicking (:sicking) from comment #10)
> Marcelino, how are we actually going to use this API? Sorry, I just need a
> short answer to make a blocking call and the pdf is pretty long. Just a
> short description about which UI features in which apps would use this new
> property would be very helpful.

You have to read just a few pages, not the whole document. Anyway I will try to summarize. This is the functionality we want to provide: check balance, top-up, show outgoing call minutes, SMS sent & data usage, set consumption/usage limits, reminders & alerts. As you can see, the whole thing is tied to a SIM card, and if we change the card all the information we have stored is useless, unless we have a way to identify the SIM card, so that we can store information together with SIM id.
(In reply to Jonas Sicking (:sicking) from comment #10)
> Marcelino, how are we actually going to use this API? Sorry, I just need a
> short answer to make a blocking call and the pdf is pretty long. Just a
> short description about which UI features in which apps would use this new
> property would be very helpful.

After the F2F session in Sao Paulo, can we remove the [blocked-on-input Marcelino Veiga Tuimil] flag?
Attached patch Part 1: DOM API - WIP. (obsolete) — Splinter Review
Attached patch Part 2: RIL impl - WIP. (obsolete) — Splinter Review
Attachment #657829 - Attachment description: Part 3: RIL impl - WIP. → Part 2: RIL impl - WIP.
Attached patch Part 3: Tests - WIP. (obsolete) — Splinter Review
(In reply to Jonas Sicking (:sicking) from comment #10)
> Marcelino, how are we actually going to use this API? Sorry, I just need a
> short answer to make a blocking call and the pdf is pretty long. Just a
> short description about which UI features in which apps would use this new
> property would be very helpful.

I am not sure about the protocol, I guess 'blocked-on-input' means you want I give you feedback. As I already did it, I removed blocked-on-input.
Whiteboard: [blocked-on-input Marcelino Veiga Tuimil]
Comment on attachment 657829 [details] [diff] [review]
Part 2: RIL impl - WIP.

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

::: dom/system/gonk/ril_worker.js
@@ +1070,5 @@
>      Buf.writeString(aid ? aid : this.aid);
>      Buf.sendParcel();
>    },
>  
> +  /** 

trailing white space,

@@ +1077,5 @@
> +  getICCID: function getICCID() {
> +    function callback() {
> +      let length = Buf.readUint32();
> +      let pairs = length * 2;
> +      this.iccInfo.iccid = GsmPDUHelper.readSwappedNibbleBcdString(pairs);

Here the length is the number of characters, 
for example, 'ab' should be of string of length 2, 
and they should be 1 pair,
so pairs should be length / 2,

@@ +1078,5 @@
> +    function callback() {
> +      let length = Buf.readUint32();
> +      let pairs = length * 2;
> +      this.iccInfo.iccid = GsmPDUHelper.readSwappedNibbleBcdString(pairs);
> +      

trailing white space

@@ +1084,5 @@
> +      if (this.iccInfo.iccid) {
> +        this._handleICCInfoChange();
> +      }
> +    }
> +    

ditto
Seems like these use-cases are certified-only.  If so the bug title is confusing as it should be clear that privileged apps won't have access.
(In reply to Lucas Adamski from comment #19)
> Seems like these use-cases are certified-only.

Probably. I used "privileged" as a general concept, not in the specific sense of the security model that's under development. So whatever security level navigator.mozMobileConnection will be exposed at is what we're talking about here. We could even create tighter security for navigator.mozMobileConnection.iccInfo if desired.
Adds the `iccid` property to nsIDOMMozMobileICCInfo object.
Attachment #657828 - Attachment is obsolete: true
Attachment #657829 - Attachment is obsolete: true
Attachment #658496 - Flags: review?(jonas)
Attached patch Part 2: RIL impl - V2. (obsolete) — Splinter Review
RIL implementation. Adds a function in ril_worker in charge of reading that id from the ICC card. Some feedback from Yoshi will be also welcome (since I've change a bit the `_getPathIdForICCRecord` function).
Attachment #658497 - Flags: review?(marshall)
Attachment #658497 - Flags: feedback?(allstars.chh)
Comment on attachment 657830 [details] [diff] [review]
Part 3: Tests - WIP.

I'm afraid that the emulator lacks of support for reading ICC records. I've not been able to find any information about that. Have you guys some information regarding how to fetch ICC records on the emulator?
Attachment #657830 - Flags: feedback?(marshall)
Attachment #657830 - Flags: feedback?(allstars.chh)
Comment on attachment 658497 [details] [diff] [review]
Part 2: RIL impl - V2.

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

Punting review to Vicamo, as the SMS filetype changes outside my knowledge area. Just one comment below.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1021,5 @@
>      let oldIcc = this.rilContext.icc;
>      this.rilContext.icc = message;
> +    if (oldIcc &&
> +        (oldIcc.iccid == message.iccid) &&
> +        (oldIcc.mcc == message.mcc || oldIcc.mnc == message.mnc)) {

This logic looks wrong.. shouldn't all of the fields be the same to early escape from this function?

In general, it would probably be easier and more efficient to just check if any field changed rather than if all fields are the same, i.e:

let infoChanged = !oldIcc ||
    oldIcc.iccid != message.iccid ||
    oldIcc.mcc != message.mcc ||
    oldIcc.mnc != message.mnc;


if (infoChanged) {
  ppmm.broadcastAsyncMessage(..)
}
Attachment #658497 - Flags: review?(marshall) → review?(vyang)
Comment on attachment 657830 [details] [diff] [review]
Part 3: Tests - WIP.

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

Is the emulator actually reporting 0 for it's ICCID? According to the emulator's SIM card code, there is a hard coded ICCID stored. The definition looks like:

static const byte_t _const_iccid[10] = {
    0x98, 0x10, 0x14, 0x30, 0x12, 0x11, 0x81, 0x15, 0x70, 0x02
};

See more here:
https://github.com/android/platform_external_qemu/blob/master/telephony/sim_card.c#L299
Attachment #657830 - Flags: feedback?(marshall)
Ok, so sounds like this is required in order to detect the user switching simcard. If that's the case then this is indeed a blocker.
blocking-basecamp: ? → +
Summary: B2G RIL: Expose Integrated Circuit Card Identifier (ICCID) in privileged DOM API → B2G RIL: Expose Integrated Circuit Card Identifier (ICCID) to certified apps through DOM API
Comment on attachment 657830 [details] [diff] [review]
Part 3: Tests - WIP.

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

As Marshall already pointed out the source,
so I don't give feedback here.
Attachment #657830 - Flags: feedback?(allstars.chh)
Comment on attachment 658497 [details] [diff] [review]
Part 2: RIL impl - V2.

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

::: dom/system/gonk/ril_worker.js
@@ +1091,5 @@
>      Buf.writeString(aid ? aid : this.aid);
>      Buf.sendParcel();
>    },
>  
> +  /** 

trailing ws

@@ +1092,5 @@
>      Buf.sendParcel();
>    },
>  
> +  /** 
> +   * Read the IICD from the ICC card.

typo: should be ICCID.

@@ +1098,5 @@
> +  getICCID: function getICCID() {
> +    function callback() {
> +      let length = Buf.readUint32();
> +      let pairs = length/2;
> +      this.iccInfo.iccid = GsmPDUHelper.readSwappedNibbleBcdString(pairs);

space between operators.
Or maybe you don't have to add another variable 'pairs',
just divide it directly in the function.

@@ +1099,5 @@
> +    function callback() {
> +      let length = Buf.readUint32();
> +      let pairs = length/2;
> +      this.iccInfo.iccid = GsmPDUHelper.readSwappedNibbleBcdString(pairs);
> +

Should have Buf.readStringDelimeter(length) here.
The original format of this response is of type String.
Attachment #658497 - Flags: feedback?(allstars.chh) → feedback+
Comment on attachment 658497 [details] [diff] [review]
Part 2: RIL impl - V2.

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

::: dom/system/gonk/ril_worker.js
@@ +2327,5 @@
>      if (!app) {
>        return null;
>      }
>  
> +    // Here we handle only file ids that are common to RUIM, SIM, USIM 

trailing ws here.
Attachment #658497 - Flags: review?(vyang) → review+
(In reply to Jonas Sicking (:sicking) from comment #26)
> Ok, so sounds like this is required in order to detect the user switching
> simcard. If that's the case then this is indeed a blocker.

Exactly, sorry for not making it clear... and thanks for pointing it out
Whiteboard: [WebAPI:P0]
Addressed all comments. Already reviewed by vicamo (r+).
Attachment #658497 - Attachment is obsolete: true
Attachment #659259 - Flags: review+
Attachment #659259 - Attachment is patch: true
Attachment #657830 - Attachment is obsolete: true
Attachment #659273 - Flags: review?(marshall)
(In reply to Marshall Culpepper [:marshall_law] from comment #25)
> Comment on attachment 657830 [details] [diff] [review]
> Part 3: Tests - WIP.
> 
> Review of attachment 657830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is the emulator actually reporting 0 for it's ICCID? 

Nope, this patch was only a WIP version. I didn't tested it. 

> See more here:
> https://github.com/android/platform_external_qemu/blob/master/telephony/
> sim_card.c#L299

Thanks for the information Marshall!. Patch updated.
Keywords: feature
Since José is doing the work on this, assigning to him.
Assignee: nobody → josea.olivera
Comment on attachment 659273 [details] [diff] [review]
Part 3: Tests - V1.

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

Looks good!

::: dom/network/tests/marionette/test_mobile_iccinfo.js
@@ +10,5 @@
>     "connection is instanceof " + connection.constructor);
>  
> +// The emulator's hard coded iccid value.
> +// See it here {B2G_HOME}/external/qemu/telephony/sim_card.c#L299.
> +is(connection.iccInfo.iccid, 89014103211118510720);

Double checking -- this ICCID has been verified?
Attachment #659273 - Flags: review?(marshall) → review+
(In reply to Marshall Culpepper [:marshall_law] from comment #35)
> > +is(connection.iccInfo.iccid, 89014103211118510720);
> 
> Double checking -- this ICCID has been verified?

The emulator doesn't like me and I've never been able to run a test on the emulator. This is what I get on the emulator radio log. 

D/AT      (   35): AT> AT+CRSM=176,12258,0,0,10
D/AT      (   35): AT< +CRSM: 144,0,98101430121181157002
D/AT      (   35): AT< OK
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #21)
> Created attachment 658496 [details] [diff] [review]
> Part 1: DOM API - V1.
> 
> Adds the `iccid` property to nsIDOMMozMobileICCInfo object.

Hey Jonas, could you take a look at it please? Thanks.
Comment on attachment 658496 [details] [diff] [review]
Part 1: DOM API - V1.

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

I thought that we had elsewhere opted to not expose the ICCD for privacy reasons. But I don't know these things well enough to have an informed opinion. API-wise this looks good though.

And since it's only exposed on .mobileConnection it's only exposed to certified apps, right?
Attachment #658496 - Flags: review?(jonas) → review+
(In reply to Jonas Sicking (:sicking) from comment #38)
> I thought that we had elsewhere opted to not expose the ICCD for privacy
> reasons.

You're probably remembering the IMSI, not the ICCID.

> And since it's only exposed on .mobileConnection it's only exposed to
> certified apps, right?

Right.
Sounds good. I very well could have been thinking of IMSI.
You need to log in before you can comment on or make changes to this bug.