Closed Bug 1151511 Opened 5 years ago Closed 5 years ago

Implement the periodic scan for unsigned add-ons

Categories

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

defect
Points:
3

Tracking

()

VERIFIED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Whiteboard: [fxsearch][searchhijacking])

Attachments

(1 file, 1 obsolete file)

No description provided.
Blocks: 1151509
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
Attached patch WIP (obsolete) — Splinter Review
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.
Status: NEW → ASSIGNED
Iteration: --- → 40.1 - 13 Apr
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Attached patch patchSplinter Review
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.
Attachment #8589327 - Attachment is obsolete: true
Attachment #8595602 - Flags: review?(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 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/
Flags: needinfo?(dtownsend)
(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
Flags: needinfo?(mjaritz)
Flags: needinfo?(kev)
Flags: needinfo?(dtownsend)
(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 on attachment 8595602 [details] [diff] [review]
patch

r=dveditz
Attachment #8595602 - Flags: review?(dveditz) → review+
(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.
Flags: needinfo?(mjaritz) → needinfo?(dtownsend)
I think we'll go with the patch we have and can tweak when it runs later
Flags: needinfo?(kev)
Flags: needinfo?(dtownsend)
Whiteboard: [fxsearch][searchhijacking]
Priority: -- → P1
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
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.
Depends on: 1160263
https://hg.mozilla.org/mozilla-central/rev/002bcc21b354
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
QA Contact: vasilica.mihasca
Is there any proper way of manually testing this?
Flags: needinfo?(dtownsend)
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.
Flags: needinfo?(dtownsend)
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?
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.