Closed
Bug 1151511
Opened 9 years ago
Closed 9 years ago
Implement the periodic scan for unsigned add-ons
Categories
(Toolkit :: Add-ons Manager, defect, P1)
Toolkit
Add-ons Manager
Tracking
()
People
(Reporter: mossop, Assigned: mossop)
References
Details
(Whiteboard: [fxsearch][searchhijacking])
Attachments
(1 file, 1 obsolete file)
17.93 KB,
patch
|
dveditz
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Updated•9 years ago
|
Iteration: --- → 40.1 - 13 Apr
Updated•9 years ago
|
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Assignee | ||
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
> 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•9 years ago
|
||
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/
Updated•9 years ago
|
Flags: needinfo?(dtownsend)
Assignee | ||
Comment 5•9 years ago
|
||
(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)
Comment 6•9 years ago
|
||
(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•9 years ago
|
||
Comment on attachment 8595602 [details] [diff] [review] patch r=dveditz
Attachment #8595602 -
Flags: review?(dveditz) → review+
Comment 8•9 years ago
|
||
(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)
Assignee | ||
Comment 9•9 years ago
|
||
I think we'll go with the patch we have and can tweak when it runs later
Flags: needinfo?(kev)
Flags: needinfo?(dtownsend)
Assignee | ||
Updated•9 years ago
|
Whiteboard: [fxsearch][searchhijacking]
Assignee | ||
Updated•9 years ago
|
Priority: -- → P1
Updated•9 years ago
|
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Assignee | ||
Comment 11•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/002bcc21b354
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
QA Contact: vasilica.mihasca
Comment 13•9 years ago
|
||
Is there any proper way of manually testing this?
Flags: needinfo?(dtownsend)
Assignee | ||
Comment 14•9 years ago
|
||
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)
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
(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)
Comment 17•9 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•