1151511 - Implement the periodic scan for unsigned add-ons

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Comment 1

[Dave Townsend [:mossop]]

Attachment: WIP

To help get the UI moving this patch shows the notification I'm using to send the list of changed add-ons. In includes the add-ons disabled and also any enabled. I don't think we'll use the latter at all in the UI right now but we might want to in the future.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch

This implements the periodic check and sends a message through the observer service when changes are detected. The timer uses the regular timer manager but the length is hardcoded to make it harder to break. This makes it harder to tweak for QA purposes.

Comment 3

[Daniel Veditz [:dveditz]]

> The timer uses the regular timer manager but the length is hardcoded to make it harder to break.

Good catch. Sucks for testing but I can't think of anything else we could do. Would it be worth the trouble to do a dev-build-only time pref?

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 8595602 [details] [diff] [review]
patch

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

Looks good except for my concerns about modified prefs. What do you think there?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +2366,5 @@
> +      let timerManager = Cc["@mozilla.org/updates/timer-manager;1"].
> +                         getService(Ci.nsIUpdateTimerManager);
> +      timerManager.registerTimer("xpi-signature-verification", () => {
> +        this.verifySignatures();
> +      }, XPI_SIGNATURE_CHECK_PERIOD);

Saving the last-run time in a pref somewhat defeats the purpose of hardcoding the time interval. Can't a pref-modifying local attacker just set the last-run time to far in the future so we never discover their injected add-on?

Couldn't we set a more immediate timer, like 5 minutes after startup, and check then? And if the user keeps their browser running forever it's OK not to check until next restart because how would they sneak in another add-on without triggering the install checks?

Or does this service check for lastupdate times in the future and reset it to 0? That's probably a good idea anyway.

@@ +4622,5 @@
>     *         not change. If true this will force userDisabled to be true
> +   * @return a tri-state indicating the action taken for the add-on:
> +   *           - undefined: The add-on did not change state
> +   *           - true: The add-on because disabled
> +   *           - false: The add-on became enabled

s/because/became/

Comment 5

[Dave Townsend [:mossop]]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> > The timer uses the regular timer manager but the length is hardcoded to make it harder to break.
> 
> Good catch. Sucks for testing but I can't think of anything else we could
> do. Would it be worth the trouble to do a dev-build-only time pref?

Maybe, we could even make it preffable in nightly/aurora without really causing problems.

(In reply to Daniel Veditz [:dveditz] from comment #4)
> Saving the last-run time in a pref somewhat defeats the purpose of
> hardcoding the time interval. Can't a pref-modifying local attacker just set
> the last-run time to far in the future so we never discover their injected
> add-on?
> 
> Couldn't we set a more immediate timer, like 5 minutes after startup, and
> check then? And if the user keeps their browser running forever it's OK not
> to check until next restart because how would they sneak in another add-on
> without triggering the install checks?
> 
> Or does this service check for lastupdate times in the future and reset it
> to 0? That's probably a good idea anyway.

The timer manager grabs the last update pref when we call registerTimer which is before any add-on code can run, if the last update is in the future it resets it to 0 (the check will run quite soon). There are still some ways to work around this though. Maybe you're right and we should just do this a little while after startup

Comment 6

[Daniel Veditz [:dveditz]]

(In reply to Dave Townsend [:mossop] from comment #5)
> The timer manager grabs the last update pref when we call registerTimer
> which is before any add-on code can run, if the last update is in the future
> it resets it to 0 (the check will run quite soon).

I missed the reset-if-future bit--that's good. I'm not worried so much about the add-on setting this but that the installer that side-loaded an add-on (or modified one) would. I guess there's still the chance the sideloader has a process running and keeps updating the pref to a few hours ago -- not future, but too recent to run the scan. Then only users to keep the browser running more than a day will get scans.

> There are still some ways to work around this though. Maybe you're right
> and we should just do this a little while after startup

In the long run that seems more reliable. But if the obvious "check me in 2038" hole isn't a problem then this seem good enough for a start.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 8595602 [details] [diff] [review]
patch

r=dveditz

Comment 8

[Markus Jaritz [:designakt] (UX)]

(In reply to Dave Townsend [:mossop] from comment #5)
> The timer manager grabs the last update pref when we call registerTimer
> which is before any add-on code can run, if the last update is in the future
> it resets it to 0 (the check will run quite soon). There are still some ways
> to work around this though. Maybe you're right and we should just do this a
> little while after startup

I am not sure on what you want my opinion. If it's about when the check should run (or the notification should be shown), I'd say on startup. And ideally it is the only notification shown at that time.

Comment 9

[Dave Townsend [:mossop]]

I think we'll go with the patch we have and can tweak when it runs later

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/002bcc21b354

Comment 11

[Dave Townsend [:mossop]]

I've landed this without the changes to the timer manager. That was causing some test failures and after fixing I decided it warranted Rob to look at it. No need to hold up the rest of this bug for that though so I filed bug 1160263.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/002bcc21b354

Comment 13

[Vasilica Tamas, QA [:vtamas]]

Is there any proper way of manually testing this?

Comment 14

[Dave Townsend [:mossop]]

Here's how you can test:

1. Launch nightly and enable xpinstall.signatures.required
2. Install a signed add-on
3. Without closing Firefox change the add-on in the profile by adding a file to its directory or XPI
4. From the browser console run 'Components.utils.import("resource://gre/modules/addons/XPIProvider.jsm").XPIProvider.verifySignatures();'

That should cause the signature verification to run and trigger the notification.

To test the timer aspect of this you'd actually have to leave nightly open for a full day. We've made the timer deliberately hard to meddle with which makes manual testing harder.

Comment 15

[Vasilica Tamas, QA [:vtamas]]

I tested this issue on Firefox 41.0a1 (2015-06-11) and Firefox 40.0a2 (2015-06-11) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5 following the steps from Comment 14 

The warning notification bar appeared automatically and the add-on was disabled in about:addons just after the specified command had been run in browser console on Windows and Ubuntu.

On Mac OS X, the notification display and the add-on disabling happened after almost an hour from the console command.

Is this the right behavior?

Comment 16

[Dave Townsend [:mossop]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #15)
> I tested this issue on Firefox 41.0a1 (2015-06-11) and Firefox 40.0a2
> (2015-06-11) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5
> following the steps from Comment 14 
> 
> The warning notification bar appeared automatically and the add-on was
> disabled in about:addons just after the specified command had been run in
> browser console on Windows and Ubuntu.
> 
> On Mac OS X, the notification display and the add-on disabling happened
> after almost an hour from the console command.
> 
> Is this the right behavior?

That's very strange and doesn't match what I see. As long as the disabling is actually happening then I'm less concerned but if you could check again and it still happens then lets get a bug on file to track it anyway.

Comment 17

[Vasilica Tamas, QA [:vtamas]]

Tested again on Firefox 41.0a1 (2015-06-16) and Firefox 40.0a2 (2015-06-16) under Windows 8.1 32-bit, Ubuntu 14.04 32-bit and on a different Mac OS X 10.9.5.  The notification display and the add-on disabling happened just after the specified command had been run in browser console.

Based on the above mentioned, I am marking this bug as Verified Fixed.