Closed Bug 1201556 Opened 5 years ago Closed 4 years ago

Send IMEI hash in update request for foxfooders

Categories

(Firefox OS Graveyard :: General, defect, P2)

defect

Tracking

(blocking-b2g:2.5+, firefox44 fixed)

VERIFIED FIXED
FxOS-S10 (30Oct)
blocking-b2g 2.5+
Tracking Status
firefox44 --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(3 files, 11 obsolete files)

5.73 KB, patch
gerard-majax
: review+
Details | Diff | Splinter Review
1.33 KB, patch
gerard-majax
: review+
Details | Diff | Splinter Review
385.76 KB, text/plain
Details
We need this to identify people we can send a FOTA.
Attached patch Send IMEI for foxfooders r=... (obsolete) — Splinter Review
A first approach. Early request will fail with RADIO_NOT_AVAILABLE errors. Then
we finally get the IMEI value and can use it for the next request.
blocking-b2g: --- → 2.5+
Paul, is there any recommandation on an hashing algorithm I should use ?
Flags: needinfo?(ptheriault)
SHA512 if you want a hash, with a device prefix(salt). Freddy suggests using PBKDF to add some work to the calculation. 

https://bugzilla.mozilla.org/show_bug.cgi?id=1171685#c12

See attachment in this comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1171685#c33 for a scratchpad which implements this pbkdf approach.
Flags: needinfo?(ptheriault)
(In reply to Paul Theriault [:pauljt] from comment #3)
> SHA512 if you want a hash, with a device prefix(salt). Freddy suggests using
> PBKDF to add some work to the calculation. 
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1171685#c12
> 
> See attachment in this comment in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1171685#c33 for a scratchpad
> which implements this pbkdf approach.

Ok, but the comment you are pointing at refers to protecting user's data usage privacy. That is much more private stuff than just using the IMEI hash to distribute FOTA to valid users. Do we really need a salt ? And PBKDF ? What will be the impact on the server-side list of device identifiers ?
Flags: needinfo?(ptheriault)
(In reply to Paul Theriault [:pauljt] from comment #3)
> SHA512 if you want a hash, with a device prefix(salt). Freddy suggests using
> PBKDF to add some work to the calculation. 
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1171685#c12
> 
> See attachment in this comment in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1171685#c33 for a scratchpad
> which implements this pbkdf approach.

We can only provide the minimum solution here with basic IMEI. If we need further improvements we need someone from the security team to take this on at client and server side.
Now we only read this if the foxfooder data tracking setting value is defined
to true, we compute sha512 and we store it in distribution.id pref so that
the nsUpdateService code can pick it.
Attachment #8656654 - Attachment is obsolete: true
Attachment #8657774 - Flags: review?(fabrice)
(In reply to Gregor Wagner [:gwagner] from comment #5)
> (In reply to Paul Theriault [:pauljt] from comment #3)
> > SHA512 if you want a hash, with a device prefix(salt). Freddy suggests using
> > PBKDF to add some work to the calculation. 
> > 
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1171685#c12
> > 
> > See attachment in this comment in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1171685#c33 for a scratchpad
> > which implements this pbkdf approach.
> 
> We can only provide the minimum solution here with basic IMEI. If we need
> further improvements we need someone from the security team to take this on
> at client and server side.

I was only providing advice since Alexandre asked. Technically hashing on its own useless, since IMEIs are partially public (you can recover the IMEI from the hash due to the properties of IMEI). So I would have said either do something which adds some (limited) security, as I proposed above, or just use the IMEI in plaintext and be done with it.

Anyways, looks like Alexandre has already done an approach based on hashing, so lets just go with that since its done.
Flags: needinfo?(ptheriault)
Comment on attachment 8657774 [details] [diff] [review]
Send hashed device id for foxfooders

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

This looks ok to me. We will just need to aware of the fact that access to settings API now gives access to a hash of the IMEI. As long as settings access stays certified that's ok, but its something to be aware of.
(In reply to Paul Theriault [:pauljt] from comment #8)
> Comment on attachment 8657774 [details] [diff] [review]
> Send hashed device id for foxfooders
> 
> Review of attachment 8657774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks ok to me. We will just need to aware of the fact that access to
> settings API now gives access to a hash of the IMEI. As long as settings
> access stays certified that's ok, but its something to be aware of.

That's a pref, not a setting. And the Settings API access for privileged apps is regulated :)
Comment on attachment 8657774 [details] [diff] [review]
Send hashed device id for foxfooders

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

That seems mostly good but I'd like to take a second look.

::: b2g/chrome/content/settings.js
@@ +539,5 @@
>  
> +#ifdef MOZ_WIDGET_GONK
> +XPCOMUtils.defineLazyServiceGetter(this, "gTelephonyService",
> +                                   "@mozilla.org/telephony/telephonyservice;1",
> +                                   "nsIGonkTelephonyService");

Why not use navigator.mozTelephony ?

@@ +544,5 @@
> +
> +// ======================= Dogfooders FOTA ==========================
> +SettingsListener.observe('debug.performance_data.dogfooding', false,
> +  isDogfooder => {
> +    // dump('AUS:Settings Checking if dogfooder? ' + isDogfooder + '\n');

nit: remove before landing.

@@ +549,5 @@
> +    if (!isDogfooder) {
> +      return;
> +    }
> +
> +    function hashDeviceId(deviceId) {

that looks like copy/paste of AppsUtils.computeHash() with SHA512 instead of MD5. Can you instead update AppsUtils.computeHash() to take a parameter that defaults to MD5?

@@ +582,5 @@
> +        // If radio is not available, wait 120 secs before a new retry.
> +        // Give up when we have no more retries allowed.
> +        if (err === 'RadioNotAvailable') {
> +          retriesLeft--;
> +          // dump('AUS:Settings Checking dogfooder: retries: ' + retriesLeft + '\n');

nit: remove before landing.

@@ +584,5 @@
> +        if (err === 'RadioNotAvailable') {
> +          retriesLeft--;
> +          // dump('AUS:Settings Checking dogfooder: retries: ' + retriesLeft + '\n');
> +          if (retriesLeft > 0) {
> +            window.setTimeout(doQueryDeviceId.bind(null), 120000);

why bind(null) ?

@@ +589,5 @@
> +          }
> +        }
> +      },
> +      notifyDialMMISuccess: function(reply) {
> +        // dump('AUS:Settings Checking dogfooder: success: ' + reply + '\n');

nit: remove before landing.

@@ +590,5 @@
> +        }
> +      },
> +      notifyDialMMISuccess: function(reply) {
> +        // dump('AUS:Settings Checking dogfooder: success: ' + reply + '\n');
> +        Services.prefs.getDefaultBranch(null).setCharPref(

I believe that you don't need the getDefaultBranch(null)

@@ +596,5 @@
> +      }
> +    };
> +
> +    function doQueryDeviceId() {
> +      // dump('AUS:Settings Checking dogfooder\n');

idem

@@ +600,5 @@
> +      // dump('AUS:Settings Checking dogfooder\n');
> +      gTelephonyService.dial(0, "*#06#", false, deviceIdCallback);
> +    }
> +
> +    doQueryDeviceId();

That will run fairly early after startup, so I would delay the first run a bit. On the other hand, we also do an update check early during gaia startup...
Attachment #8657774 - Flags: review?(fabrice) → review-
(In reply to [:fabrice] Fabrice Desré from comment #10)
> Comment on attachment 8657774 [details] [diff] [review]
> Send hashed device id for foxfooders
> 
> Review of attachment 8657774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That seems mostly good but I'd like to take a second look.
> 
> ::: b2g/chrome/content/settings.js
> @@ +539,5 @@
> >  
> > +#ifdef MOZ_WIDGET_GONK
> > +XPCOMUtils.defineLazyServiceGetter(this, "gTelephonyService",
> > +                                   "@mozilla.org/telephony/telephonyservice;1",
> > +                                   "nsIGonkTelephonyService");
> 
> Why not use navigator.mozTelephony ?

Because I forgot settings.js has access to it :)

> 
> @@ +544,5 @@
> > +
> > +// ======================= Dogfooders FOTA ==========================
> > +SettingsListener.observe('debug.performance_data.dogfooding', false,
> > +  isDogfooder => {
> > +    // dump('AUS:Settings Checking if dogfooder? ' + isDogfooder + '\n');
> 
> nit: remove before landing.

Sure.

> 
> @@ +549,5 @@
> > +    if (!isDogfooder) {
> > +      return;
> > +    }
> > +
> > +    function hashDeviceId(deviceId) {
> 
> that looks like copy/paste of AppsUtils.computeHash() with SHA512 instead of
> MD5. Can you instead update AppsUtils.computeHash() to take a parameter that
> defaults to MD5?

Good, I searched for code being able to compute hashes but I missed that one. I'll replace it of course.

> 
> @@ +582,5 @@
> > +        // If radio is not available, wait 120 secs before a new retry.
> > +        // Give up when we have no more retries allowed.
> > +        if (err === 'RadioNotAvailable') {
> > +          retriesLeft--;
> > +          // dump('AUS:Settings Checking dogfooder: retries: ' + retriesLeft + '\n');
> 
> nit: remove before landing.
> 
> @@ +584,5 @@
> > +        if (err === 'RadioNotAvailable') {
> > +          retriesLeft--;
> > +          // dump('AUS:Settings Checking dogfooder: retries: ' + retriesLeft + '\n');
> > +          if (retriesLeft > 0) {
> > +            window.setTimeout(doQueryDeviceId.bind(null), 120000);
> 
> why bind(null) ?

For no specific reason except than a bad habit I think.

> 
> @@ +589,5 @@
> > +          }
> > +        }
> > +      },
> > +      notifyDialMMISuccess: function(reply) {
> > +        // dump('AUS:Settings Checking dogfooder: success: ' + reply + '\n');
> 
> nit: remove before landing.
> 
> @@ +590,5 @@
> > +        }
> > +      },
> > +      notifyDialMMISuccess: function(reply) {
> > +        // dump('AUS:Settings Checking dogfooder: success: ' + reply + '\n');
> > +        Services.prefs.getDefaultBranch(null).setCharPref(
> 
> I believe that you don't need the getDefaultBranch(null)
> 
> @@ +596,5 @@
> > +      }
> > +    };
> > +
> > +    function doQueryDeviceId() {
> > +      // dump('AUS:Settings Checking dogfooder\n');
> 
> idem
> 
> @@ +600,5 @@
> > +      // dump('AUS:Settings Checking dogfooder\n');
> > +      gTelephonyService.dial(0, "*#06#", false, deviceIdCallback);
> > +    }
> > +
> > +    doQueryDeviceId();
> 
> That will run fairly early after startup, so I would delay the first run a
> bit. On the other hand, we also do an update check early during gaia
> startup...

Ideally I'd like to wait for telephony to signal us radio is available :)
(In reply to [:fabrice] Fabrice Desré from comment #10)
> Comment on attachment 8657774 [details] [diff] [review]
> Send hashed device id for foxfooders
> 
> Review of attachment 8657774 [details] [diff] [review]:
> -----------------------------------------------------------------

[...]

> 
> @@ +590,5 @@
> > +        }
> > +      },
> > +      notifyDialMMISuccess: function(reply) {
> > +        // dump('AUS:Settings Checking dogfooder: success: ' + reply + '\n');
> > +        Services.prefs.getDefaultBranch(null).setCharPref(
> 
> I believe that you don't need the getDefaultBranch(null)
> 

So I rechecked, and without, my pref value is not picked at all.
Fixed all the comments.
Attachment #8657774 - Attachment is obsolete: true
Attachment #8658641 - Flags: review?(fabrice)
Comment on attachment 8658641 [details] [diff] [review]
Send hashed device id for foxfooders

bug 1201538 comment 4 makes this approach obsolete ...
Attachment #8658641 - Flags: review?(fabrice)
Attachment #8658641 - Attachment is obsolete: true
Attachment #8661266 - Flags: review?(fabrice)
Comment on attachment 8661266 [details] [diff] [review]
Send hashed device id for foxfooders

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

::: dom/apps/AppsUtils.jsm
@@ +767,5 @@
>      let data = converter.convertToByteArray(aString, result);
>  
>      let hasher = Cc["@mozilla.org/security/hash;1"]
>                     .createInstance(Ci.nsICryptoHash);
> +    hasher.initWithString(aAlgorithm || "MD5");

I would rather use a default value for the parameter: see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters
Attachment #8661266 - Flags: review?(fabrice) → review+
Attachment #8661266 - Attachment is obsolete: true
Attachment #8661684 - Flags: review+
So Ben told me we cannot land change to the update URL until the Balrog server is ready otherwise we will cut updates for those people, which we probably don't want. Would you agree we land this with the app.update.url pref being not changed and have a follow up to change the update URL ? This would land when Balrog server side is ready.
Flags: needinfo?(fabrice)
Flags: needinfo?(bhearsum)
(In reply to Alexandre LISSY :gerard-majax from comment #20)
> So Ben told me we cannot land change to the update URL until the Balrog
> server is ready otherwise we will cut updates for those people, which we
> probably don't want. Would you agree we land this with the app.update.url
> pref being not changed and have a follow up to change the update URL ? This
> would land when Balrog server side is ready.

That's a really great idea actually - it should mean we can test by setting app.update.url manually rather than doing try pushes.
Flags: needinfo?(bhearsum)
Comment on attachment 8661684 [details] [diff] [review]
Send hashed device id for foxfooders

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

::: b2g/app/b2g.js
@@ +582,5 @@
>  pref("app.update.incompatible.mode", 0);
>  pref("app.update.staging.enabled", true);
>  pref("app.update.service.enabled", true);
>  
> +pref("app.update.url", "https://aus5.mozilla.org/update/3/%PRODUCT%/%VERSION%/%BUILD_ID%/%PRODUCT_DEVICE%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%IMEI%/update.xml");

I was just going over everything again, and noticed that this is slightly off, it should be:
https://aus5.mozilla.org/update/5/%PRODUCT%/%VERSION%/%BUILD_ID%/%PRODUCT_DEVICE%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%IMEI%/update.xml

The bump of 3 -> 5 is needed to signify a new version of update URL, and version 4 is already taken.
Attachment #8661684 - Attachment is obsolete: true
Attachment #8661762 - Flags: review+
Attachment #8661762 - Attachment is obsolete: true
Attachment #8661807 - Flags: review+
Attachment #8661807 - Attachment is obsolete: true
Attachment #8661808 - Flags: review+
Attachment #8662249 - Flags: review+
(In reply to Alexandre LISSY :gerard-majax from comment #26)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fa876f0d9f1

Let's merge attachment 8662249 [details] [diff] [review]. Adding leave-open so we can merge attachment 8661808 [details] [diff] [review] when server-side is ready.
Component: General Automation → General
Product: Release Engineering → Firefox OS
QA Contact: catlee
Target Milestone: --- → FxOS-S7 (18Sep)
Whiteboard: [systemsfe]
the patch didn't apply cleanly to b2g-i

adding 1201556 to series file
renamed 1201556 -> Bug-1201556---Send-hashed-device-id-for-foxfooders.patch
applying Bug-1201556---Send-hashed-device-id-for-foxfooders.patch
patching file b2g/chrome/content/settings.js
Hunk #1 FAILED at 531
1 out of 1 hunks FAILED -- saving rejects to file b2g/chrome/content/settings.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-1201556---Send-hashed-device-id-for-foxfooders.patch

could you take a look, thanks!
Flags: needinfo?(lissyx+mozillians)
Keywords: checkin-needed
Rebased.
Attachment #8662249 - Attachment is obsolete: true
Attachment #8662337 - Flags: review+
Rebased.
Attachment #8661808 - Attachment is obsolete: true
Attachment #8662338 - Flags: review+
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(fabrice)
Looks like pushing new pref value gets overridden by |syncCharPref()|. And I don't see an easy way out of this. Any idea Jan ?
Flags: needinfo?(janx)
(In reply to Alexandre LISSY :gerard-majax from comment #34)
> Looks like pushing new pref value gets overridden by |syncCharPref()|. And I
> don't see an easy way out of this. Any idea Jan ?

Ok, situation may not be as bad as it looks: pref value will always be at first the value read from prefs.js. The app.update.url() value overriden by the setting value disappears at each reboot. So we have a way out of this.
The setting that overrides the pref (not persistently) on every boot was introduced to allow users to switch to an alternative update URL (e.g. a community-managed update server for an otherwise unsupported device). This mechanism assumes that the pref will never change, which is a problem here because we'd like to push a new pref value. I see two solutions to this problem.

Current behavior:
- Device is newly flashed.
- On first boot the pref is copied into the setting.
- On every subsequent boot, the setting overrides the pref, essentially becoming the source of truth.
- At any time, the user can persistently modify the setting.

Solution 1: Blow up the setting whenever the underlying pref changes:
- Device is newly flashed.
- On first boot the pref is copied into the setting. A backup setting is also created, in order to detect pref changes.
- On every subsequent boot, the setting overrides the pref, except if the pref is different from the backup setting, in which case the new pref is copied into the setting.
- At any time, the user can persistently modify the setting, but this new value will be lost if the underlying pref ever changes (not often).

Solution 2: Make overriding the update URL a temporary thing:
- Device is newly flashed.
- On every boot, the pref is copied into the setting.
- At any time, the user can modify the setting, but the new value will be lost after the next reboot. (This is useful once for switching to a different update server. The new server should then send an update that durably changes the underlying pref).

The second solution seems cleaner and simpler, but it might break workflows we don't know about.
Flags: needinfo?(janx)
Depends on: 1206741
I think we will want solution 2 implemented: it will fix our current problem, while allowing the user to override the value for the current session.

People should not have to rely on app.update.url changed by hand for a long time: proper usecase would be to allow them to easily switch to a new update provider which will push Gecko builds that contains a proper update URL.
Depends on: 1207313
Alexandre, bug 926737 added a ?custom=%CUSTOM% query parameter exactly for these kind of use cases and that should be filed with the content of the app.update.custom preference.

IMHO we should be using that preference and that query parameter instead of adding an extra %IMEI% one to the update URL.

Is there any reason why you chose not to use the mechanism implemented in bug 926737?
Flags: needinfo?(lissyx+mozillians)
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #38)
> Alexandre, bug 926737 added a ?custom=%CUSTOM% query parameter exactly for
> these kind of use cases and that should be filed with the content of the
> app.update.custom preference.
> 
> IMHO we should be using that preference and that query parameter instead of
> adding an extra %IMEI% one to the update URL.
> 
> Is there any reason why you chose not to use the mechanism implemented in
> bug 926737?

That is not my decision, as stated in comment 15.
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(ferjmoreno)
Flags: needinfo?(bhearsum)
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #38)
> Alexandre, bug 926737 added a ?custom=%CUSTOM% query parameter exactly for
> these kind of use cases and that should be filed with the content of the
> app.update.custom preference.
> 
> IMHO we should be using that preference and that query parameter instead of
> adding an extra %IMEI% one to the update URL.
> 
> Is there any reason why you chose not to use the mechanism implemented in
> bug 926737?

That may have worked when B2G maintained its own update server, but that's never something that we supported in Balrog (the current update server). We're all going to need to get better about communicating when changing the client or the server, these surprises are no fun for anybody...
Flags: needinfo?(bhearsum)
Ok, got it. Thanks for the explanation
Flags: needinfo?(ferjmoreno)
No longer blocks: 1201538
Depends on: 1201538
Can we get QA to cross check the update url change patch effect after bug 1206741 landed ? Thanks!
Flags: needinfo?(nhirata.bugzilla)
Keywords: qaurgent, qawanted
Depends on: 1211973
QA ping
Rebased.
Attachment #8662338 - Attachment is obsolete: true
Attachment #8673142 - Flags: review+
Sorry.  Making my own build with these changes.  I manually applied the other patch as it was bitrot as well.  
(referring to https://bug1201556.bmoattachments.org/attachment.cgi?id=8662337 )

Alexandre, I apologize I didn't get to this in time.  Could you unbitrot the patch please?
Flags: needinfo?(lissyx+mozillians)
Naoki, the code you said bitrot is the patch that landed in comment 33
Flags: needinfo?(lissyx+mozillians)
Oh, that explains a lot; I was confused why certain parts were already in; I missed the comment.  I guess it's the other patch I need which has not bitrot.
Flags: needinfo?(nhirata.bugzilla)
Flags: needinfo?(nhirata.bugzilla)
Are we good to land that remaining part?
Flags: needinfo?(catlee)
Priority: -- → P2
Chris, any update here?
go ahead
Flags: needinfo?(catlee)
Attachment #8673368 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e376b24d15c5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: FxOS-S7 (18Sep) → FxOS-S10 (30Oct)
Setting to verified, in production now and works like a charm.
Status: RESOLVED → VERIFIED
Flags: needinfo?(nhirata.bugzilla)
Removing qawanted tags per comment 56.
Flags: needinfo?(ktucker)
Keywords: qaurgent, qawanted
QA Whiteboard: [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.