926737 - FOTA update includes device GUID for identification purpose

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Kai-Zhen Li [:seinlin][:kli]]

In current design, the following information can be included in the URL for checking update. All of them are common information, and can't be used for identification purpose.
 %PRODUCT%
 %VERSION%/
 %BUILD_ID%
 %BUILD_TARGET%
 %OS_VERSION%
 %LOCALE%
 %CHANNEL%
 %PLATFORM_VERSION%
 %DISTRIBUTION%
 %DISTRIBUTION_VERSION%
 %PRODUCT_MODEL%
 %B2G_VERSION%

For identification purpose need to include extra information such IMEI.

Does include IMEI in the update URL cause the privacy issue?

Or should enhance the update protocol to include sensitive data?

Comment 1

[Kai-Zhen Li [:seinlin][:kli]]

Hi Dave, The partner would like to have identification feature in FOTA update. Include IMEI could be a solution. How do you think?

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

FTR I already did a patch for this for TEF dogfooding program

https://github.com/ferjm/firefoxos-dogfood/blob/master/gecko/addimeiupdate.patch

Comment 3

[Dave Hylands [:dhylands]]

For the most part, ferjm's patch looks reasonable to me.

I'd treat the IMEI just like any other substitution though (it should come in through the getUpdateURL function).
We should also probably only retrieve the IMEI once, and stash it in a global ala gProductModel.

And all IMEI related code in the updater needs to be inside #ifdef MOZ_WIDGET_GONK since this is GONK specific.

Comment 4

[Robert Kaiser]

Are we sure that sending the IMEI over the network in a URL is something Mozilla wants to endorse?

Comment 5

[Robert Strong (they/them - no direct email)]

Privacy and policy issues aside... instead of changing checkForUpdates it seems like it would be much simpler to just check if the value is available and add it to the url in the #ifdef MOZ_WIDGET_GONK section linked to below

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3543

Comment 6

[Robert Strong (they/them - no direct email)]

Note: that will also add it for the background update check case
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2541

Comment 7

[Kai-Zhen Li [:seinlin][:kli]]

I got similar idea as rtrong, such as add a keyword %IMEI% for url and then replace it as needed.

And we should encourage partner to use https when it is used in url. I think https can prevent imei to be sent in plain text.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

I can update the patch with the previous suggestions.

I'm also unsure about how this fits in Mozilla's "do not track" policy. As I said in my previous comment we added this patch only in TEF builds for our internal dogfooding program. Testers were aware of this.

ni? some security folks.

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

Personally, I don't like it, at all. It's not a big issue (since it's just the OS update check/download), but it's most definitely tracking. Using it internally on a controlled environment is one thing, using it for the general public is a different beast altogether.

Comment 10

[Kai-Zhen Li [:seinlin][:kli]]

Include IMEI could be quite sensitive to most case. I think we could also consider to add another element which also can be used, such as serial number. 
Add %SN% which will be replace by device serial number as described in Comment 5. I think it could be more neutral than imei.

Comment 11

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

My thoughts echo those above, and the dicussion on the mailing list in that I think using IMEI is a bad idea. Using a device serial number isn't much better if there is no user control here. Going back to the mozilla privacy principles (http://www.mozilla.org/en-US/privacy/) there are some suggestions I would make, but I don't know how much these conflict with the use case:


1. No Surprises
I don't really have an opinion on this one - I don't expect most users would be aware their device was uniquely identifiable, but given that devices have a unique phone number it doesn't seem to really conflict with this principle in my mind.

2. Real Choices
Comes down to the privacy policy basically, and how available that is to a user (inc prior to purchase).

3. Sensible default settings
Could this be opt-in? (I assume not given the use cases discussed in the thread)

4. Limited data
There were several suggestions in the email thread around alternatives - using the IMEI just because it is useful and convenient is a poor reason in my mind.

5. User Control control
To me this is the most important one:

   

    Educate users whenever we collect any personal information and give them a choice whenever possible.
    Sensible Settings

    Establish default settings that balance safety and user experience appropriately.
    Limited Data

    Collect and retain the least amount of user information necessary. Try to share anonymous aggregate data whenever possible, and then only when it benefits the web, users or developers.
    User Control

    Do not disclose personal user experience without the user's consent. Innovate, develop and advocate for privacy enhancements that put users in control of their online experiences.
    Trusted Third Parties

    Make privacy a factor in selecting and interacting with partners.

Comment 12

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

(accidental return, continuing from 5, ignore copy paste from previous post)

5. User Control control
To me this is the most important one:
 - Can the user disable this identifier? 
 - Can the user cycle the identifier (ie generate a new one) ?

6. Trusted Third Parties
Not sure that this is directly relevant given that we already have a contractual and trusted relationship with our OEM partners

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Thanks for your comments.

What I understand so far is:

1- IMEI is not a good identifier. SN seems like a better one. I'm not sure how this number is exposed, but I'll take a look if we finally agree to use it.

2- We must let the user know about her device being uniquely identified.

3- We must allow the user to choose not to be identified.

I agree that these are valid concerns, but I would like to confirm with the partner that these conditions are OK for them before writing any patch for this.

Comment 14

[Kai-Zhen Li [:seinlin][:kli]]

Yes, I agree with you. Different partner will get their own OTA server and they will also get their concern.

I think to get the serial number of the device is much simple than to get imei. May be we can simply get it by "libcutils.property_get(...)".

Comment 15

[Robert Kaiser]

(In reply to Kai-Zhen Li from comment #10)
> Include IMEI could be quite sensitive to most case. I think we could also
> consider to add another element which also can be used, such as serial
> number. 

What's the difference there? Both are AFAIK unique identifiers of the piece of hardware, so why is there any difference in terms of privacy?

Comment 16

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

Popping this over to the privacy team who is looking into this.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

I misread comment 11. I agree that there is no real difference between the IMEI and the serial number in terms of tracking. Even if the IMEI apparently provides further information [1], we would still be allowing user tracking. So point 1 from comment 13 is not valid.

Can we get an agreement on points 2. and 3. from comment 13 so we can move forward with this?

That is, we can expose the IMEI or serial number, but we will need to let the user know about this and allow her to disable this exposure.

If we agree on this, we'll need UX input about how to do the above mentioned.

[1] http://www.imei.info/

Comment 18

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

I re-pinged the privacy team yesterday, so hopefully they can provide insight quickly.

Comment 19

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC, please) from comment #17)
> I misread comment 11. I agree that there is no real difference between the
> IMEI and the serial number in terms of tracking. Even if the IMEI apparently
> provides further information [1], we would still be allowing user tracking.
> So point 1 from comment 13 is not valid.

Yes, I agree that IMEI and serial number are unique to device and they are in terms of tracking. Actually IMEI is already used every phone call in existing cellular network.
But the thing we are going to do is sending one of this unique identifier to a server which could be located in the operator network or oem/odm network.

> Can we get an agreement on points 2. and 3. from comment 13 so we can move
> forward with this?
> 
> That is, we can expose the IMEI or serial number, but we will need to let
> the user know about this and allow her to disable this exposure.
> 
> If we agree on this, we'll need UX input about how to do the above mentioned.

I also agree to points 2. and 3. from comment 13, we should let the user know about it.
I propose we can include it in "do not track". When do not track is selected, we won't send any of the unique identifier to somewhere. But the use of IMEI in existing cellular network is out of our control.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #18)
> I re-pinged the privacy team yesterday, so hopefully they can provide
> insight quickly.

Thanks Jonas!

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Kai-Zhen Li from comment #19)
> I also agree to points 2. and 3. from comment 13, we should let the user
> know about it.
> I propose we can include it in "do not track". When do not track is
> selected, we won't send any of the unique identifier to somewhere. But the
> use of IMEI in existing cellular network is out of our control.

The "Do not track" option sounds good to me. Thanks

Comment 22

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

I'll let Alina comment, but do-not-track for not sending IMEI seems to be a good start (at least that addresses principle 5 of the privacy principles: user control).

Even better from a privacy standpoint would be if IMEI wasn't used at all though - is there any reason why randomly generated GUID is not sufficient here? Maybe it's not critical but I don't see the point in compromise privacy if not necessary. Even better would be a randomly generated GUID that the user could cycle (e.g. every time you turn do-no-track OFF, you get a new tracking ID).

Comment 23

[Antonio Manuel Amaya Calvo (:amac)]

Sorry for the delay.

I don't have much to add to what Paul already said, other than one thing that's important (and might even change the legal repercussions on some countries like, say, in Spain ;) ) is that the identifier used to track the downloads should be dissociated from the user completely. That is, it should not be something that can be used to identify the user on any other place.

That's one reason of why the IMEI is not a good identifier for this use case: because that identifier is attached to all the calls the user makes (amongst other things) so adding this data to the patch downloads could allow to create a correlation between the downloads and the cell calls (which I'm not saying would be useful for anything, just that it would be possible). The IMEI could then be seen as a personal data and thus the download register would need to be registered with the APD (in Spain).

Any random identifier that can also be changed at the user request (as Paul's previous comment says) would be much better.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Thanks for the feedback. I'll work on a patch to:

- Add a GUID to the OTA URL iif do not track is disabled.
- Change this GUID everytime the do not track option is re-enabled.

Are we sure that this is what partners are expecting? Who can confirm this?

Comment 25

[Robert Kaiser]

(In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC, please) from comment #24)
> - Add a GUID to the OTA URL iif do not track is disabled.

I think it's best to only do this when do not track is set to explicitly allow tracking ("disabled" can also mean when it's set to "do not state any preference to track or not to", which is the default state, so let's be explicit here).

Also, I guess it's best to provide a variable that can be used in the update URLs and set it to "1" or something like that by default, and then do what your proposed when DNT is set to "track me" - you could even go and set that GUID back to "1" whenever you switch DNT to neutral or no tracking and generate a new GUID when it's being switched to "track".

Just my 2c here. ;-)

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

This patch is working fine for me. It sends a random UUID if the user does not specifically disallow tracking and this value is reset if the user changes the do-not-track value.

I still need to write some tests, but I could use some feedback in the meantime. Thanks!

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

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

Sorry, I meant to ask for feedback only.

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Sorry I didn't notice this comment.

(In reply to Robert Kaiser (:[e-mail]) from comment #25)
> (In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC,
> please) from comment #24)
> > - Add a GUID to the OTA URL iif do not track is disabled.
> 
> I think it's best to only do this when do not track is set to explicitly
> allow tracking ("disabled" can also mean when it's set to "do not state any
> preference to track or not to", which is the default state, so let's be
> explicit here).
>

I'm ok with that, but this probably won't fulfill partner's requirements. I can't see any user setting this value to allow tracking, but the opposite.

Can we get some attention from the partners that are asking for this feature in this bug, please?

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

I would really appreciate confirmation from the partners about what they really expect from this bug.

Comment 30

[Kai-Zhen Li [:seinlin][:kli]]

There are some use cases for this requirement, for example
- Maintain a list on update server to feed beta/test release to a group of devices.
- Devices in different batch of production may use different hardware components. In case of there is an issue due to hardware component but it can be fixed by software, they can feed different update to different batch.

From partner point of view, they prefer to use serial number, mac address or imei which already in their production database. Using GUID could be a fair method for our position, but it could be hard to meet their original goal.

AFAICT, we can finish this bug with current patch for reference. But it could be out of our scope how partner use it base on their privacy policy.

Comment 31

[Robert Strong (they/them - no direct email)]

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

For data that isn't necessary for app update to perform an update check I would very much prefer keeping this generic and available to all consumers. A simple way would be to use a preference, if the preference value is present replace it in the url, and if it is not it would replace it with an empty string. The pref name should be along the lines of app.update.custom and all of the code to maintain the preference which is needed for this bug should be outside of app update.

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

Thanks for the feedback Robert! :)

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #31)
> Comment on attachment 8348869 [details] [diff] [review]
> v1
> 
> For data that isn't necessary for app update to perform an update check I
> would very much prefer keeping this generic and available to all consumers.
> A simple way would be to use a preference, if the preference value is
> present replace it in the url, and if it is not it would replace it with an
> empty string. The pref name should be along the lines of app.update.custom
> and all of the code to maintain the preference which is needed for this bug
> should be outside of app update.

If I am not wrong the attached patch fulfills your description except for where the preference is maintained. Any suggestion of where should this code live if not within the app update code? Thanks!

Comment 33

[Robert Strong (they/them - no direct email)]

Somewhere in B2G code.

Comment 34

[Robert Strong (they/them - no direct email)]

To be more specific, in the case of desktop Firefox we have code that executes in delayed startup. I don't know B2G code well enough to know where it performs these types of operations.

Comment 35

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: B2G

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: nsUpdateService

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Test

Comment 38

[Fernando Jiménez Moreno [:ferjm]]

Try build: https://tbpl.mozilla.org/?tree=Try&rev=23b9e043e740

Comment 39

[Robert Strong (they/them - no direct email)]

Comment on attachment 8349990 [details] [diff] [review]
Part 2: nsUpdateService

>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -35,35 +35,36 @@ const PREF_APP_UPDATE_ENABLED           
> const PREF_APP_UPDATE_METRO_ENABLED       = "app.update.metro.enabled";
> const PREF_APP_UPDATE_IDLETIME            = "app.update.idletime";
> const PREF_APP_UPDATE_INCOMPATIBLE_MODE   = "app.update.incompatible.mode";
> const PREF_APP_UPDATE_INTERVAL            = "app.update.interval";
> const PREF_APP_UPDATE_LASTUPDATETIME      = "app.update.lastUpdateTime.background-update-timer";
> const PREF_APP_UPDATE_LOG                 = "app.update.log";
> const PREF_APP_UPDATE_MODE                = "app.update.mode";
> const PREF_APP_UPDATE_NEVER_BRANCH        = "app.update.never.";
>+const PREF_APP_UPDATE_NOTIFIEDUNSUPPORTED = "app.update.notifiedUnsupported";
> const PREF_APP_UPDATE_POSTUPDATE          = "app.update.postupdate";
> const PREF_APP_UPDATE_PROMPTWAITTIME      = "app.update.promptWaitTime";
> const PREF_APP_UPDATE_SHOW_INSTALLED_UI   = "app.update.showInstalledUI";
> const PREF_APP_UPDATE_SILENT              = "app.update.silent";
> const PREF_APP_UPDATE_STAGING_ENABLED     = "app.update.staging.enabled";
>-const PREF_APP_UPDATE_NOTIFIEDUNSUPPORTED = "app.update.notifiedUnsupported";
> const PREF_APP_UPDATE_URL                 = "app.update.url";
> const PREF_APP_UPDATE_URL_DETAILS         = "app.update.url.details";
> const PREF_APP_UPDATE_URL_OVERRIDE        = "app.update.url.override";
> const PREF_APP_UPDATE_SERVICE_ENABLED     = "app.update.service.enabled";
> const PREF_APP_UPDATE_SERVICE_ERRORS      = "app.update.service.errors";
> const PREF_APP_UPDATE_SERVICE_MAX_ERRORS  = "app.update.service.maxErrors";
> const PREF_APP_UPDATE_SOCKET_ERRORS       = "app.update.socket.maxErrors";
> const PREF_APP_UPDATE_RETRY_TIMEOUT       = "app.update.socket.retryTimeout";
> 
> const PREF_APP_DISTRIBUTION               = "distribution.id";
> const PREF_APP_DISTRIBUTION_VERSION       = "distribution.version";
> 
> const PREF_APP_B2G_VERSION                = "b2g.version";
>+const PREF_APP_B2G_DEVICE_ID              = "b2g.device.id";
Instead of making this b2g specific just use const PREF_APP_UPDATE_CUSTOM = app.update.custom or similar. Then other apps can put whatever else they want in the url.

>@@ -1349,16 +1350,36 @@ function getDistributionPrefValue(aPrefN
>   } catch (e) {
>     // use default when pref not found
>   }
> 
>   return prefValue;
> }
> 
> /**
>+ * For tracking purposes some partners require us to add the device ID to the
>+ * update URL. This UUID will be an empty string if the user specifically
>+ * disallows tracking.
>+ */
>+#ifdef MOZ_B2G
>+function getDeviceId() {
>+  let deviceId = '';
>+  try {
>+    deviceId =
>+      Services.prefs.getPrefType(PREF_APP_B2G_DEVICE_ID) ==
>+      Ci.nsIPrefBranch.PREF_STRING &&
>+      Services.prefs.getCharPref(PREF_APP_B2G_DEVICE_ID);
>+  } catch (e) {
>+    LOG("Error getting device ID " + e);
>+  }
>+  return deviceId;
>+}
>+#endif
Remove per below

>+/**
>  * An enumeration of items in a JS array.
>  * @constructor
>  */
> function ArrayEnumerator(aItems) {
>   this._index = 0;
>   if (aItems) {
>     for (var i = 0; i < aItems.length; ++i) {
>       if (!aItems[i])
>@@ -3597,16 +3618,20 @@ Checker.prototype = {
>                       getDistributionPrefValue(PREF_APP_DISTRIBUTION_VERSION));
>     url = url.replace(/\+/g, "%2B");
> 
> #ifdef MOZ_WIDGET_GONK
>     url = url.replace(/%PRODUCT_MODEL%/g, gProductModel);
>     url = url.replace(/%B2G_VERSION%/g, getPref("getCharPref", PREF_APP_B2G_VERSION, null));
> #endif
> 
>+#ifdef MOZ_B2G
>+    url = url.replace(/%DEVICE_ID%/g, getDeviceId());
>+#endif
Make this generic as well and place it with the other all platform replacements above. Also, should be able to just use the following to get the value
getPref("getCharPref", PREF_APP_UPDATE_CUSTOM, "");
or
getPref("getCharPref", PREF_APP_UPDATE_CUSTOM, null);

Also, update the test so it is for all platforms. Thanks!

Comment 40

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

Fernando: What identifiers do these patches send to the update server?

Comment 41

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: B2G

Comment 42

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: nsUpdateService

Comment 43

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Test

Comment 44

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jonas Sicking (:sicking) needinfo? or feedback? encouraged. from comment #40)
> Fernando: What identifiers do these patches send to the update server?

It sends a random UUID if the user does not specifically disallow tracking via the do-not-track feature. This UUID is reset every time the user changes the do-not-track value from disallow to allow. As suggested in comment 22 and reaffirmed in comment 23.

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

New try build: https://tbpl.mozilla.org/?tree=Try&rev=8edb18d64256

Comment 46

[Robert Strong (they/them - no direct email)]

Comment on attachment 8350582 [details] [diff] [review]
Part 2: nsUpdateService

Looks good

Comment 47

[[:fabrice] Fabrice Desré]

Comment on attachment 8350581 [details] [diff] [review]
Part 1: B2G

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

::: b2g/chrome/content/settings.js
@@ +480,5 @@
> +  try {
> +    let dntEnabled = Services.prefs.getBoolPref('privacy.donottrackheader.enabled');
> +    let dntValue =  Services.prefs.getIntPref('privacy.donottrackheader.value');
> +    // If the user specifically decides to disallow tracking (1), we just bail out.
> +    if (dntEnabled && (dntValue == 1)) {

shouldn't this be if (!dntEnabled || (dntValue == 1)) ?

@@ +493,5 @@
> +    // If there is no previous registered tracking ID, we generate a new one.
> +    // This should only happen on first usage or after changing the
> +    // do-not-track value from disallow to allow.
> +    if (!trackingId) {
> +      trackingId = uuidgen.generateUUID().toString();

this will give you a value like '{1290d6af-dafd-4203-9c28-cda5a60c0412}'. A minor improvement is to remove the curly braces.

Comment 48

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fabrice Desré [:fabrice] from comment #47)
> Comment on attachment 8350581 [details] [diff] [review]
> Part 1: B2G
> 
> Review of attachment 8350581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/chrome/content/settings.js
> @@ +480,5 @@
> > +  try {
> > +    let dntEnabled = Services.prefs.getBoolPref('privacy.donottrackheader.enabled');
> > +    let dntValue =  Services.prefs.getIntPref('privacy.donottrackheader.value');
> > +    // If the user specifically decides to disallow tracking (1), we just bail out.
> > +    if (dntEnabled && (dntValue == 1)) {
> 
> shouldn't this be if (!dntEnabled || (dntValue == 1)) ?

No. If DNT is not enabled, the user does not specifically disallow tracking.

Comment 49

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: B2G

This patch adds the suggestion for removing the curly brackets from the generated UUID.

Fabrice, let me know if my previous comment makes sense, please.

Thanks!

Comment 50

[Fernando Jiménez Moreno [:ferjm]]

Thanks Fabrice and Robert!

https://hg.mozilla.org/integration/b2g-inbound/rev/c24e5fdfd8f3
https://hg.mozilla.org/integration/b2g-inbound/rev/fc3ac7eb4035
https://hg.mozilla.org/integration/b2g-inbound/rev/dca3b3b25a44

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c24e5fdfd8f3
https://hg.mozilla.org/mozilla-central/rev/fc3ac7eb4035
https://hg.mozilla.org/mozilla-central/rev/dca3b3b25a44