806534 - Support regular expressions in extension id for blocklist entries

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 3

[Mozilla RelEng Bot]

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/[e-mail]-44567f2acde5

Comment 4

[Mozilla RelEng Bot]

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/[e-mail]-44567f2acde5

Comment 5

[Florian Bender]

Is this somehow related to bug 720856? E. g. look at my bug 720856 comment 25.

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

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 7

[Dave Townsend [:mossop]]

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

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 676892 [details] [diff] [review]
Patch v1

Needs moar test

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Note to self: Needs a test for when two regexp blocklist entries match the same add-on.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

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.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/63905e069ee4

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 13

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/63905e069ee4

Comment 14

[Jorge Villalobos [:jorgev] (he/him)]

I just added a testcase on bug 806543.

Comment 15

[Jorge Villalobos [:jorgev] (he/him)]

See bug 806543 comment #6 for a successful test of this feature.

Comment 16

[Dave Townsend [:mossop]]

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

Comment 17

[Alex Keybl [:akeybl]]

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?

Comment 18

[Dave Townsend [:mossop]]

(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.

Comment 19

[Jorge Villalobos [:jorgev] (he/him)]

(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.

Comment 20

[Alex Keybl [:akeybl]]

(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?

Comment 21

[Dave Townsend [:mossop]]

(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 22

[Alex Keybl [:akeybl]]

Comment on attachment 683818 [details] [diff] [review]
Patch v2

Nothing's blown up in bug 806543 over the weekend, approving for branches as well.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/14eb65de9248
https://hg.mozilla.org/releases/mozilla-esr17/rev/b693c6e51390

Comment 24

[Jorge Villalobos [:jorgev] (he/him)]

Verifying per the testing done in bug 806534.

Comment 25

[Alex Keybl [:akeybl]]

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).

Comment 26

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Jorge Villalobos [:jorgev] from comment #24)
> Verifying per the testing done in bug 806534.

I meant bug 806543, of course.

Comment 27

[Jorge Villalobos [:jorgev] (he/him)]

ESR fix also verified on bug 806543.

Comment 28

[Vasilica Tamas, QA [:vtamas]]

Removing the qawanted keyword since this bug was verified across all fixed versions.