Closed Bug 806534 Opened 8 years ago Closed 8 years ago

Support regular expressions in extension id for blocklist entries

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- wontfix
firefox18 --- verified
firefox19 --- verified
firefox20 --- verified
firefox-esr17 --- verified

People

(Reporter: jorgev, Assigned: Unfocused)

References

Details

(Keywords: verifyme, Whiteboard: [squeaky])

Attachments

(1 file, 1 obsolete file)

We identified a large number of malicious add-on IDs in bug 806451. Luckily, they follow a pattern, so they're easy to spot. We need to block the pattern in those IDs, and the best way to do this is to use a regular expression for the extension ID, similarly to what we do for plugin blocks.

I don't think this requires any changes on the AMO side or the blocklist XML layout. We probably just need to change the check in the AOM so it supports regexp matching.
It's probably worth pointing out that a single profile can end up with multiple add-ons that match single block pattern, so we should take that case into consideration.
Blocks: 806543
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #676892 - Flags: review?(dtownsend+bugmail)
Try run for 44567f2acde5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=44567f2acde5
Results (out of 31 total builds):
    success: 28
    warnings: 1
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-44567f2acde5
Try run for 44567f2acde5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=44567f2acde5
Results (out of 32 total builds):
    success: 28
    warnings: 1
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-44567f2acde5
Is this somehow related to bug 720856? E. g. look at my bug 720856 comment 25.
I don't think this is related. This is just an expansion of the blocklist feature that allows us to block certain extensions more efficiently.
Comment on attachment 676892 [details] [diff] [review]
Patch v1

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

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +251,5 @@
> +function parseRegExp(aStr) {
> +    let lastSlash = aStr.lastIndexOf("/");
> +    let pattern = aStr.slice(1, lastSlash);
> +    let flags = aStr.slice(lastSlash + 1);
> +    return new RegExp(pattern, flags);

2 space indent please
Attachment #676892 - Flags: review?(dtownsend+bugmail) → review+
Comment on attachment 676892 [details] [diff] [review]
Patch v1

Needs moar test
Attachment #676892 - Flags: review+ → review-
Note to self: Needs a test for when two regexp blocklist entries match the same add-on.
Attached patch Patch v2Splinter Review
Now with moar test.

As discussed on IRC, if multiple blocklist entries would match a given add-on, this only matches against the first matching blocklist entry. In the mythical blocklist rewrite, this would ideally match the entry with the highest severity.
Attachment #676892 - Attachment is obsolete: true
Attachment #683818 - Flags: review?(dtownsend+bugmail)
Attachment #683818 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/integration/fx-team/rev/63905e069ee4
Whiteboard: [squeaky] → [squeaky][fixed-in-fx-team]
I want to have this bake on mozilla-central for awhile before considering uplifting it to Aurora/Beta. To help with that, we should push a regexp-based blocklist entry to production (ie, fix bug 806451/bug 806543). 

For builds that don't include this patch, such blocklist entries will effectively be ignored - they'll be treated as normal strings. Since add-on IDs can't contain '/', no add-on will match.

This functionality should be documented with the process for adding blocklist entries. Adding a regexp-based entry is the same, except for the add-on ID - which needs be in the format: /pattern/flags (eg, /^pink@.*\.info$/i). The flag is optional (eg, /^pink@.*\.info$/). 

Note that this format is *different* from the plugin match patterns, which do not include '/' before/after the pattern, and don't allow using regexp flags. Had to be done this way to keep the existing blocklist XML format - plugin match patterns are always assumed to be regexp; the /pattern/flags format gives us a reliable way to detect if it's a regexp or an ID string.
https://hg.mozilla.org/mozilla-central/rev/63905e069ee4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
I just added a testcase on bug 806543.
See bug 806543 comment #6 for a successful test of this feature.
Comment on attachment 683818 [details] [diff] [review]
Patch v2

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: The patch is critical for blocking malicious extensions which seem to be on the rise. While not a security issue in itself taking this gives us the ability to fight security issues caused by malicious add-ons in the future
User impact if declined: We will have no way to blocklist certain kinds of malicious add-ons
Fix Landed on Version: Currently in 20.0a1 nightlies
Risk to taking this patch (and alternatives if risky): No available alternative, the patch is well scoped and tested, I'd consider it a low risk
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: We will have no way to blocklist certain kinds of malicious add-ons
Testing completed (on m-c, etc.): Currently in 20.0a1 nightlies
Risk to taking this patch (and alternatives if risky): No available alternative, the patch is well scoped and tested, I'd consider it a low risk
String or UUID changes made by this patch: None
Attachment #683818 - Flags: approval-mozilla-esr17?
Attachment #683818 - Flags: approval-mozilla-beta?
Attachment #683818 - Flags: approval-mozilla-aurora?
Comment on attachment 683818 [details] [diff] [review]
Patch v2

Is there anything we can do to test/verify on Nightly and Aurora before landing to other branches? If not, what's the worst case scenario for FF18/ESR17? Is it possible that we'd be unable to blocklist other add-ons, or perhaps blocklist too many add-ons?
Attachment #683818 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Alex Keybl [:akeybl] from comment #17)
> Comment on attachment 683818 [details] [diff] [review]
> Patch v2
> 
> Is there anything we can do to test/verify on Nightly and Aurora before
> landing to other branches? If not, what's the worst case scenario for
> FF18/ESR17? Is it possible that we'd be unable to blocklist other add-ons,
> or perhaps blocklist too many add-ons?

bug 806543 comment #6 has a successful test of the feature on Nightly, we can presumably repeat that test on aurora if needed. We could also have QA run through any manual tests they have for the blocklist service. The absolute worst case is that we'd lose the ability to block extensions, but we really do have good automated testing for the blocklist service so I do think the risk is minimal here.
(In reply to Alex Keybl [:akeybl] from comment #17)
> Is it possible that we'd be unable to blocklist other add-ons,
> or perhaps blocklist too many add-ons?

Comment #12 explains how the ID format is different for all existing blocks and this new kind of block. Current blocks shouldn't be affected by this change.
(In reply to Dave Townsend (:Mossop) from comment #18)
> (In reply to Alex Keybl [:akeybl] from comment #17)
> > Comment on attachment 683818 [details] [diff] [review]
> > Patch v2
> > 
> > Is there anything we can do to test/verify on Nightly and Aurora before
> > landing to other branches? If not, what's the worst case scenario for
> > FF18/ESR17? Is it possible that we'd be unable to blocklist other add-ons,
> > or perhaps blocklist too many add-ons?
> 
> bug 806543 comment #6 has a successful test of the feature on Nightly, we
> can presumably repeat that test on aurora if needed. We could also have QA
> run through any manual tests they have for the blocklist service. The
> absolute worst case is that we'd lose the ability to block extensions, but
> we really do have good automated testing for the blocklist service so I do
> think the risk is minimal here.

Thanks - would still make me a bit more comfortable if we landed on Aurora today (and pushed https://bugzilla.mozilla.org/show_bug.cgi?id=806543#c7 to production), followed by landing on Beta/ESR17 on Tuesday. Can we move forward with that?
(In reply to Alex Keybl [:akeybl] from comment #20)
> (In reply to Dave Townsend (:Mossop) from comment #18)
> > (In reply to Alex Keybl [:akeybl] from comment #17)
> > > Comment on attachment 683818 [details] [diff] [review]
> > > Patch v2
> > > 
> > > Is there anything we can do to test/verify on Nightly and Aurora before
> > > landing to other branches? If not, what's the worst case scenario for
> > > FF18/ESR17? Is it possible that we'd be unable to blocklist other add-ons,
> > > or perhaps blocklist too many add-ons?
> > 
> > bug 806543 comment #6 has a successful test of the feature on Nightly, we
> > can presumably repeat that test on aurora if needed. We could also have QA
> > run through any manual tests they have for the blocklist service. The
> > absolute worst case is that we'd lose the ability to block extensions, but
> > we really do have good automated testing for the blocklist service so I do
> > think the risk is minimal here.
> 
> Thanks - would still make me a bit more comfortable if we landed on Aurora
> today (and pushed https://bugzilla.mozilla.org/show_bug.cgi?id=806543#c7 to
> production), followed by landing on Beta/ESR17 on Tuesday. Can we move
> forward with that?

Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/c57456dea468
Comment on attachment 683818 [details] [diff] [review]
Patch v2

Nothing's blown up in bug 806543 over the weekend, approving for branches as well.
Attachment #683818 - Flags: approval-mozilla-esr17?
Attachment #683818 - Flags: approval-mozilla-esr17+
Attachment #683818 - Flags: approval-mozilla-beta?
Attachment #683818 - Flags: approval-mozilla-beta+
Verifying per the testing done in bug 806534.
Let's stage an update for verification on ESR17, but any production uses of this blocklist mechanism in the short term should be limited to FF17 (let's not push the staged ESR block to production).
Keywords: qawanted, verifyme
(In reply to Jorge Villalobos [:jorgev] from comment #24)
> Verifying per the testing done in bug 806534.

I meant bug 806543, of course.
ESR fix also verified on bug 806543.
Removing the qawanted keyword since this bug was verified across all fixed versions.
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.