1258565 - Youtube Unblocker Remediation

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Per bug 1251911 Youtube Unblocker has been blocked but many users have been left with additional add-ons with random IDs and a user.js file that breaks the blocklist service as well as add-on updates.

To try to undo that damage I have developed a system add-on that we can deploy to any version of Firefox since 44. It takes the following steps:

1. Uninstalls the Youtube Unblocker add-on (should we just disable it?)

2. Checks if user.js exists in the profile and if it matches a specific hash. If so it deletes it, resets all the prefs it changes and then saves the prefs to disk.

3. Attempts to find any additionally installed add-ons and uninstalls them. At the moment it checks for any add-on that uses an updateURL beginning with "https://update.malware-protcet.com/".

The latter two are very targeted and any slight deviations means we will miss things but I don't know how much we want to widen that and risk changing things a user intentionally put there.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch

Kev, does this fit what you are looking for or are there different things I should be doing here.

Comment 2

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

The code looks okay to me. My only note is that the minVersion should be 44.0 instead of 44.

Comment 3

[Kris Maglione [:kmag]]

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

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

::: browser/extensions/moz.build
@@ +8,5 @@
>      'e10srollout',
>      'loop',
>      'pdfjs',
>      'pocket',
> +    'remediation'

Please add trailing comma.

::: browser/extensions/remediation/bootstrap.js
@@ +30,5 @@
> +  // return the two-digit hexadecimal code for a byte
> +  let toHexString = charCode => ("0" + charCode.toString(16)).slice(-2);
> +
> +  // convert the binary hash data to a hex string.
> +  let binary = hasher.finish(false);

Would it be easier to just use `finish(true)` here, and store the hash in base64?

@@ +75,5 @@
> +      console.log("Removed user.js.");
> +
> +      for (let pref of AFFECTED_PREFS) {
> +        Services.prefs.clearUserPref(pref);
> +        Services.prefs.savePrefFile(null);

I don't think we need to save the pref file after we change each individual pref.

@@ +93,5 @@
> + * Removes any detected malicious add-ons.
> + */
> +function* removeChildAddons() {
> +  try {
> +    let addons = yield (new Promise(resolve => AddonManager.getAllAddons(resolve)));

This should probably be `getAddonsByTypes(["extension"], ...)`

@@ +97,5 @@
> +    let addons = yield (new Promise(resolve => AddonManager.getAllAddons(resolve)));
> +    for (let addon of addons) {
> +      let updateURL = addon.updateURL;
> +      if (updateURL && updateURL.startsWith("https://update.malware-protcet.com/")) {
> +        console.log(`Removing potentially malicious add-on ${addon.id}`);

Might be nice to include the name here, with something like `"${addon.name}" (ID: ${addon.id})`

@@ +113,5 @@
> +  }
> +}
> +
> +let startup = Task.async(function*() {
> +  yield removeMainAddon();

We should skip this if MAIN_ID is null.

@@ +115,5 @@
> +
> +let startup = Task.async(function*() {
> +  yield removeMainAddon();
> +  yield checkUserJS();
> +  yield removeChildAddons();

Do we really want to do all of this on every startup?

::: browser/extensions/remediation/install.rdf
@@ +6,5 @@
> +<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
> +     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> +
> +  <Description about="urn:mozilla:install-manifest">
> +    <em:id>[e-mail]</em:id>

Maybe "malware-remediation@" or "addon-remediation@"? "remediation" is pretty broad and vague.

::: browser/extensions/remediation/moz.build
@@ +5,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +FINAL_TARGET_FILES.features['[e-mail]'] += [
> +  'bootstrap.js',
> +  'install.rdf'

Please add trailing comma.

Comment 4

[Kev Needham [:kev]]

(In reply to Dave Townsend [:mossop] from comment #1)
> Created attachment 8733120 [details] [diff] [review]
> patch
> 
> Kev, does this fit what you are looking for or are there different things I
> should be doing here.

This fits, but do think it should be disabled vs. uninstalled given the corrective action taken, and the updated version that removes the ability to drop arbitrary files. The thing we want is to unhose the update/telemetry settings, and get a clean version on vs. removing something the user might be looking for. Disabling it then correcting the block info will allow the user to update and get the notification in add-on manager.

Apologies, hadn't realized this was still an open question.

Comment 5

[Andy McKay]

As per the meeting today, Kris will build this as a seperate add-on.

Comment 6

[Kris Maglione [:kmag]]

Attachment: malware-remediation.patch

This version does a few different things:

- If it finds a "user.js" file with changes to all of the preferences changed by this add-on, and only to those preferences, it removes the file and resets the preferences.

- If it finds a "secmodd.db" file, it removes the extension referenced in that file, and removes the file as well.

- If it finds any extensions with update URLs that start with "https://update.malware-protcet.com/" or end with ".cloudfront.net/watcher/update.rdf", it uninstalls the add-ons.

- Any time it uninstalls an add-on, it also removes the stylesheet that watcher.xpi installs to hide the add-on with that ID.

- If it finds an add-on with the ID "[e-mail]" which is not disabled, it makes sure that the blocklist service is enabled, refreshes the blocklist, and, if the add-on is still not disabled at that point, it manually disables it.

This last step may be a bit harsh, since the extension does not currently uninstall or disable itself after its first run, so it will wind up doing this every startup, if it finds an active version of the add-on.

Comment 7

[Kev Needham [:kev]]

Comment on attachment 8749861 [details] [diff] [review]
malware-remediation.patch

Makes sense, and we should probably revisit in 60 days post-release whether we should remove the code after that point. This is where event-based logging would be helpful in understanding scope as well (of both the downloader add-on and the watcher-ware).

Does reverting this add-on after 60 days make sense to everyone?

Comment 8

[Andy McKay]

Reviewing to remove sounds like a good idea.

Comment 9

[Kev Needham [:kev]]

Adding mgrimes as well. Matt, is checking frequency of when this addon is enabled something that shield would be useful for, or would it be more along the lines of telemetry? We're looking for an event-based pingback to let us know how often the addon performs it's function to get an understanding of how useful this kind of remediation effort is.

Comment 10

[Matt Grimes [:Matt_G]]

Frequency is something we'd need to pull from telemetry. We could do remediation through Shield, but it sounds like you're all over that. Ilana would know more about tracking in telemetry.

Comment 11

[Ilana]

Kev, do you mean a notification for how often this addon is installed and removed? We can use Telemetry to track the number of users with active and disabled versions of the addon, but a single data packet will require a minimum of about a day to gather and then upload progress to the pipeline. This isn't really an "event-based pingback" if I understand it correctly, but if you required something realtime, there might be a mechanism for that.

What's your ultimate goal? To make sure the removals are working/sticking and that the instances in the wild aren't increasing? If so, we can do this pretty quickly.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

We were wondering today if there's any way we could collect some telemetry data on # of affected users beforehand so we can better assess risk here as well. Seems like that'd be a reasonably easy system addon?

Comment 13

[Andy McKay]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> We were wondering today if there's any way we could collect some telemetry
> data on # of affected users beforehand so we can better assess risk here as
> well. Seems like that'd be a reasonably easy system addon?

The add-on turns off telemetry so you can't get the numbers without fixing it.

Comment 14

[Ilana]

If you're interested in seeing what the effect was post hoc, we can certainly do that in a few ways using telemetry. One is by counting the number of "disabled" instances of the addon there are. Another way would be by writing to something like a histogram logging what there was a removal/any other activity. For precision, I'd recommend using the histograms.

Comment 15

[Andy McKay]

Kris, do you know how to add in a histogram data? That seems to be the best bet.

Ilana, if we do that do we have to do anything special to process it on Firefox 44 or is this something I can put a dashboard together in re:dash?

Comment 16

[Ilana]

Unfortunately, this data isn't available with re:dash and it's a little painful to add. There are still options. 

- Use a lua filter to get near-realtime data for the requested information in your histogram. Unfortunately, this cannot be "back-filled;" we'd only have the data from the time it was implemented onwards. Then someone writes html/js to render into a dashboard (ex: https://metrics.services.mozilla.com/telemetry-budget-dashboard/).

- Write a script in spark that queries for the information you want daily and dumps it somewhere

- some hybrid of the two (get some historical data with spark, and use lua going forward)

I'd recommend option 3, unless you're not worried about historical data. Which one sounds preferable?

Comment 17

[Andy McKay]

(In reply to Ilana from comment #16)
> Unfortunately, this data isn't available with re:dash and it's a little
> painful to add. There are still options. 
> 
> - Use a lua filter to get near-realtime data for the requested information
> in your histogram. Unfortunately, this cannot be "back-filled;" we'd only
> have the data from the time it was implemented onwards. Then someone writes
> html/js to render into a dashboard (ex:
> https://metrics.services.mozilla.com/telemetry-budget-dashboard/).
> 
> - Write a script in spark that queries for the information you want daily
> and dumps it somewhere
> 
> - some hybrid of the two (get some historical data with spark, and use lua
> going forward)
> 
> I'd recommend option 3, unless you're not worried about historical data.
> Which one sounds preferable?

I have no idea since I haven't used any of the above. Since there is no historical data because we are adding this histogram in the add-on, version 3 sounds ok.

Comment 18

[Ilana]

No history needed, so we can actually do everything with lua filters. Great news.

Comment 19

[Matthew N. [:MattN]]

(In reply to Andy McKay [:andym] from comment #15)
> Kris, do you know how to add in a histogram data? That seems to be the best
> bet.

I think you want registerAddonHistogram but I've never used it before: https://mxr.mozilla.org/mozilla-central/search?string=registerAddonHistogram

I'm not sure if there is a list of allowed histograms e.g. histogram-whitelists.json that you need to be on though

Comment 20

[Kris Maglione [:kmag]]

I can do the Firefox side of this, yes. Or, at least, I can figure out the parts I don't know.

Do we want this done in the add-on, or in-tree? Or both? Most of the users we're targeting with this are stuck on old versions of Firefox, and it's hard to say if they'll get upgrades after the fix, so it would be nice to try to get data for them regardless.

Comment 21

[Andy McKay]

I would assume in the add-on if that's possible. 

I'm hoping they'll get back on the Firefox upgrade train, but they may not actually upgrade meaning we wouldn't get the data for other reasons.

Comment 22

[Benjamin Smedberg]

I recommend that you use a custom ping type, instead of addonhistograms. You can use TelemetryController.submitExternalPing to send arbitrary data (I'll want to do a data-review).

You'll also need to alert mreid so that the data pipeline is ready to accept the new ping type.

Comment 23

[Kris Maglione [:kmag]]

Attachment: System add-on patch

r?rhelmer for a general review, and bsmedberg for telemetry code and data review.

The behavior of this version of the patch is slightly different than the last. It first collects the following information:

 - Whether there is a user.js file which changes only the set of preference this add-on is known to change. If found, we remove it and reset the relevant preferences.
 - If there is a "secmodd.db" file in the profile, the ID of the add-on referenced in it. If the file exists, we remove it.
 - The IDs of any add-ons which are hidden from about:addons using a particular stylesheet.
 - The IDs and update URLs of any add-ons which match the known update URL patterns for the watcher add-on.

Any add-ons found in the above steps are removed, and the data is added to a telemetry payload.

After this, it checks for the presence of the YouTube Unblocker add-on, and adds its enabled and blocklist status to the telemetry ping.

At this point, if we find that the blocklist is disabled, and we've found either a YouTube Unblocker add-on or a malicious user.js file, we re-enable and update it.

If the YouTube Unblocker add-on is still enabled at this point, and we have found any other indications of malware, we also force disable that add-on.

Comment 24

[Robert Helmer [:rhelmer]]

Comment on attachment 8758471 [details] [diff] [review]
System add-on patch

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

This looks correct to me. I double-checked all the pref names and they look good as well.

It would be nice to have a test here too, with an example of the code it is cleaning up (I realize this is security sensitive and we may want to hold back on pushing that part). Just a simple test to make sure the the system add-on gets installed and starts up correctly would be nice at minimum, e.g. https://dxr.mozilla.org/mozilla-central/source/browser/extensions/webcompat/test/browser_webcompat_stub_check.js so if it's broken for any reason it'll start failing on try and commit etc.

I know we don't want this code to live long-term, but if we have to iterate on it at all (e.g. slight variations of the malicious add-on found in the wild) I think this would keep our frustration to a minimum.

::: browser/extensions/malware-remediation/bootstrap.js
@@ +39,5 @@
> +}
> +
> +// Note: Preserving the original indentation of the template string is
> +// important here.
> +const USER_SHEET = '\

Why does this need to be a global const? Using the name `USER_SHEET` is a little confusing with the stylesheet constant used below too.

Could this be a `let` inside `getMaliciousCSSURL()` below, and a template literal instead of the `USER_SHEET.replace`? Template literals can be multi-line without needing \ as well.

@@ +96,5 @@
> +      Services.obs.removeObserver(this, "blocklist-updated");
> +      resolve();
> +    }
> +    Service.obs.addObserver(observer, "blocklist-updated", false);
> +    Cc["@mozilla.org/extensions/blocklist;1"].getService(Ci.nsITimerCallback).notify(null);

nit: would be more readable like:
```
let blocklistNotifier = Cc["@mozilla.org/extensions/blocklist;1"]
                        .getService(Ci.nsITimerCallback);
blocklistNotifier.notify(null);
```

@@ +160,5 @@
> +      if (!line) {
> +        continue;
> +      }
> +
> +      let match = /^user_pref\(("(?:[^\\"]|\\.)*"),/.exec(line);

Can you give an example of what this is expected to match, in a comment? I don't understand what this is for from the comments in the bug so far.

Comment 25

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #24)
> Comment on attachment 8758471 [details] [diff] [review]
> @@ +160,5 @@
> > +      if (!line) {
> > +        continue;
> > +      }
> > +
> > +      let match = /^user_pref\(("(?:[^\\"]|\\.)*"),/.exec(line);
> 
> Can you give an example of what this is expected to match, in a comment? I
> don't understand what this is for from the comments in the bug so far.

Actually a test would be even better than a comment :) then there's no question about whether the regex is matching what we expect.

Comment 26

[Kris Maglione [:kmag]]

(In reply to Robert Helmer [:rhelmer] from comment #24)
> It would be nice to have a test here too, with an example of the code it is
> cleaning up (I realize this is security sensitive and we may want to hold
> back on pushing that part). Just a simple test to make sure the the system
> add-on gets installed and starts up correctly would be nice at minimum, e.g.
> https://dxr.mozilla.org/mozilla-central/source/browser/extensions/webcompat/
> test/browser_webcompat_stub_check.js so if it's broken for any reason it'll
> start failing on try and commit etc.
> 
> I know we don't want this code to live long-term, but if we have to iterate
> on it at all (e.g. slight variations of the malicious add-on found in the
> wild) I think this would keep our frustration to a minimum.

I'm on the fence about adding tests. Since this is a security bug, we won't
actually be able to land them until after the add-on has been deployed and
retired. Given the amount of work required to properly mock profiles and
add-ons for these tests, I'm not sure it's worth the time and effort.

I did manually test this pretty rigorously, though, and QA already has a test
plan put together, so what it lacks in automated testing, it will get in
manual testing.

> Could this be a `let` inside `getMaliciousCSSURL()` below, and a template
> literal instead of the `USER_SHEET.replace`? Template literals can be
> multi-line without needing \ as well.

I copied this from the extension, and I wanted to change it as little as
possible, since even a slight mismatch means the stylesheets won't match.
Template strings help with the newlines, either, since the escaped newlines
are actually dropped from the string, so they'd still need to be escaped in a
template string.

> @@ +160,5 @@
> > +      if (!line) {
> > +        continue;
> > +      }
> > +
> > +      let match = /^user_pref\(("(?:[^\\"]|\\.)*"),/.exec(line);
> 
> Can you give an example of what this is expected to match, in a comment? I
> don't understand what this is for from the comments in the bug so far.

It matches all of the `user_pref` calls in the user.js file, and returns the
string containing the pref name. It's a bit ugly, but given how specific the
files that we're looking for are, it does the job.

Comment 27

[Robert Helmer [:rhelmer]]

(In reply to Kris Maglione [:kmag] from comment #26)
> (In reply to Robert Helmer [:rhelmer] from comment #24)
> > It would be nice to have a test here too, with an example of the code it is
> > cleaning up (I realize this is security sensitive and we may want to hold
> > back on pushing that part). Just a simple test to make sure the the system
> > add-on gets installed and starts up correctly would be nice at minimum, e.g.
> > https://dxr.mozilla.org/mozilla-central/source/browser/extensions/webcompat/
> > test/browser_webcompat_stub_check.js so if it's broken for any reason it'll
> > start failing on try and commit etc.
> > 
> > I know we don't want this code to live long-term, but if we have to iterate
> > on it at all (e.g. slight variations of the malicious add-on found in the
> > wild) I think this would keep our frustration to a minimum.
> 
> I'm on the fence about adding tests. Since this is a security bug, we won't
> actually be able to land them until after the add-on has been deployed and
> retired. Given the amount of work required to properly mock profiles and
> add-ons for these tests, I'm not sure it's worth the time and effort.
> 
> I did manually test this pretty rigorously, though, and QA already has a test
> plan put together, so what it lacks in automated testing, it will get in
> manual testing.
> 
> > Could this be a `let` inside `getMaliciousCSSURL()` below, and a template
> > literal instead of the `USER_SHEET.replace`? Template literals can be
> > multi-line without needing \ as well.
> 
> I copied this from the extension, and I wanted to change it as little as
> possible, since even a slight mismatch means the stylesheets won't match.
> Template strings help with the newlines, either, since the escaped newlines
> are actually dropped from the string, so they'd still need to be escaped in a
> template string.
> 
> > @@ +160,5 @@
> > > +      if (!line) {
> > > +        continue;
> > > +      }
> > > +
> > > +      let match = /^user_pref\(("(?:[^\\"]|\\.)*"),/.exec(line);
> > 
> > Can you give an example of what this is expected to match, in a comment? I
> > don't understand what this is for from the comments in the bug so far.
> 
> It matches all of the `user_pref` calls in the user.js file, and returns the
> string containing the pref name. It's a bit ugly, but given how specific the
> files that we're looking for are, it does the job.

All good points! r+ stands.

Comment 28

[Benjamin Smedberg]

Comment on attachment 8758471 [details] [diff] [review]
System add-on patch

*the data-review part*

Normally what I like to do a data-review on is a doc describing the ping format. This can either be an .rst file in mozilla-central (used to create gecko.readthedocs.org) or a markdown file if this ends up living in github.

As an example, see https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/crash-ping.html which is generated from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/docs/crash-ping.rst

It should document *when* the ping is sent (what conditions, etc), and a human-readable schema/description of the data that is collected.

I *think* I understand the structure of `payload`, but because constructing that is spread across a bunch of functions, I'm not sure I got all the nuance. I'm mainly looking for any privacy risk of collecting private data.

What I don't yet understand is when this ping is sent. Is it sent for everyone? Or only for users where we've detected bad things we're trying to fix? Is it sent only the first time we fix it, or at every startup?

If we're sending the ping from everyone, not just affected users, that dramatically increases the processing required.

*the code-review part*

The function checkUserJS is called with `payload` but the argument is never used

Comment 29

[Kris Maglione [:kmag]]

I can add an RST document to the patch, if you'd like, but since this is a security bug, I don't think we'll actually be able to land it until we're done with the ping. I'll include it inline for now:

"malware-addon-states" ping
===========================

This ping is captured at browser startup for all users of the malware
remediation add-on. It contains non-identifying information about the presence
and impact of specific malware add-ons on the user's system.

Structure::

    {
      type: "malware-addon-states",
      ...
      clientId: <UUID>,
      environment: { ... },
      payload: {
        blocklistDisabled: <bool>, // True is the blocklist was disabled at startup time.
        mainAddonActive: <bool | null>, // True if the add-on exists and is enabled,
                                        // false if it exists and is disabled,
                                        // or null if the add-on was not found.
        mainAddonBlocked: <bool | null>, // True if the add-on exists and is blocked,
                                         // false if it exists and is not blocked,
                                         // or null if the add-on was not found.
        foundUserJS: <bool>, // True if a malicious user.js file was found in the profile.
        secmoddbAddon: <string | null>, // If a malicious secmodd.db file was found,
                                        // the extension ID that the file contained.
        hiddenAddons: [ // A list of IDs for extensions which were hidden by malicious CSS.
          <string>,
          ...
        ],
        updateURLs: {
          // A mapping of installed add-on IDs with known malicious
          // update URL patterns to their exact update URLs.
          <extensionID>: <updateURL>,
          ...
        }
      }
    }


This does send the ping at every startup for users of the add-on, but we're only planning to deploy the add-on to users of Firefox 44-46 (and in the case of 46, only after 47 has been released), so it should only impact a subset of users.

I think that we could reduce this to send only the first clean ping, along with any pings with signs of malware. If we reduce it any more than that, I don't think we'll be able to get a good sense of how effective the deployment was.

Comment 30

[Benjamin Smedberg]

This (with the environment block) is a lot of extra data to send at each startup. I'd really prefer to send as few clean pings as will solve your data analysis needs.

I sent a note about public checkin of the data docs by private email, to get a legal opinion.

Comment 31

[Georg Fritzsche [:gfritzsche]]

(In reply to Kris Maglione [:kmag] from comment #29)
> This ping is captured at browser startup for all users of the malware
> remediation add-on.

This has non-trivial storage cost if we send this for all release users.
Isn't it enough to send this for "affected" users?
Or more specifically if any actions were performed etc.?

[...]
> This does send the ping at every startup for users of the add-on, but we're
> only planning to deploy the add-on to users of Firefox 44-46 (and in the
> case of 46, only after 47 has been released), so it should only impact a
> subset of users.
> 
> I think that we could reduce this to send only the first clean ping, along
> with any pings with signs of malware. If we reduce it any more than that, I
> don't think we'll be able to get a good sense of how effective the
> deployment was.

You can see the full population that has your addon installed by looking at existing Telemetry data.
It includes data on the active addons in the environment:
http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/environment.html

Comment 32

[Kris Maglione [:kmag]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #31)
> (In reply to Kris Maglione [:kmag] from comment #29)
> > This ping is captured at browser startup for all users of the malware
> > remediation add-on.
> 
> This has non-trivial storage cost if we send this for all release users.
> Isn't it enough to send this for "affected" users?
> Or more specifically if any actions were performed etc.?

We're never going to be distributing the remediation add-on to release users.

> You can see the full population that has your addon installed by looking at
> existing Telemetry data.
> It includes data on the active addons in the environment:
> http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/
> environment.html

That should tell me about the number of people with the add-on installed, but
it doesn't tell me whether they were able to fully initialize, gather data,
and send the telemetry ping.

The purpose of this add-on is to reverse the damage done by a malware add-on
that, among other things, disables telemetry. We're deploying these add-ons
into a hostile environment, so I don't think it's entirely save to assume that
installed implies working. We're also going to have to compare the number of
instances that were deployed to the number of pings we actually received from
installed instances, and having all of that data in a single bucket should
simplify things considerably.

I'm happy to change the reporting code to only report one clean result per
install, though. I think that should be enough data to tell me what I need to
know, and should reduce the storage cost significantly.

Comment 33

[Andrei Vaida [:avaida]]

Hey Kris, how's the system add-on shaping up? Do we have an ETA for when it'll be ready for testing?

Comment 34

[Andy McKay]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #33)
> Hey Kris, how's the system add-on shaping up? Do we have an ETA for when
> it'll be ready for testing?

The add-on works, but not sure we give an ETA yet, we are currently in discussion with metrics to see if we can include metrics in the add-on.

Comment 35

[Andy McKay]

(In reply to Kris Maglione [:kmag] from comment #32) 
> I'm happy to change the reporting code to only report one clean result per
> install, though. I think that should be enough data to tell me what I need to
> know, and should reduce the storage cost significantly.

That sounds reasonable.

Comment 36

[Kris Maglione [:kmag]]

Attachment: System add-on patch

Comment 37

[Robert Helmer [:rhelmer]]

Comment on attachment 8773469 [details] [diff] [review]
System add-on patch

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

Overall lgtm, some nits.

::: browser/extensions/malware-remediation/bootstrap.js
@@ +35,5 @@
> +
> +const PREF_BLOCKLIST = "extensions.blocklist.enabled";
> +const PREF_BLOCKLIST_URL = "extensions.blocklist.url";
> +
> +const prefs = new Preferences("extensions.malware-remediation.");

Shouldn't the `prefs` global const be in caps?

@@ +79,5 @@
> +        } catch (e) {
> +          console.error(e);
> +        }
> +      }
> +      return true;

Do you still want to return true here regardless if exceptions are thrown above and the add-on can't be removed or disabled?

@@ +293,5 @@
> +
> +  let addons = yield new Promise(resolve => AddonManager.getAddonsByTypes(["extension"], resolve));
> +
> +  payload.foundUserJS = yield checkUserJS();
> +  payload.secmoddAddon = yield checkSecmoddbFile(foundAddons);

Not a big deal for this code, but in general I'd prefer not mutate things passed arguments... these functions could each return a new `Set` and create a union with the result (something like `Set([...foundAddons, ...newAddons])`)

@@ +305,5 @@
> +  if (!prefs.has("first-results")) {
> +    prefs.set("first-results", JSON.stringify(payload));
> +  }
> +  prefs.set("last-results", JSON.stringify(payload));
> +

Are the "first-results" and "last-results" prefs used or reported anywhere?

Comment 38

[Kris Maglione [:kmag]]

(In reply to Robert Helmer [:rhelmer] from comment #37)
> ::: browser/extensions/malware-remediation/bootstrap.js
> @@ +35,5 @@
> > +
> > +const PREF_BLOCKLIST = "extensions.blocklist.enabled";
> > +const PREF_BLOCKLIST_URL = "extensions.blocklist.url";
> > +
> > +const prefs = new Preferences("extensions.malware-remediation.");
> 
> Shouldn't the `prefs` global const be in caps?

I generally only use all caps for immutable values.

> @@ +79,5 @@
> > +        } catch (e) {
> > +          console.error(e);
> > +        }
> > +      }
> > +      return true;
> 
> Do you still want to return true here regardless if exceptions are thrown
> above and the add-on can't be removed or disabled?

Yeah. What matters is whether we found the add-on, not whether we successfully
removed it.

> @@ +305,5 @@
> > +  if (!prefs.has("first-results")) {
> > +    prefs.set("first-results", JSON.stringify(payload));
> > +  }
> > +  prefs.set("last-results", JSON.stringify(payload));
> > +
> 
> Are the "first-results" and "last-results" prefs used or reported anywhere?

No, but I decided to store them in case we find something unexpected after we
start deploying. If we need to push an update, it might need to make decisions
based on the initial state of things, before we did any cleanup.

Comment 39

[Kris Maglione [:kmag]]

Comment on attachment 8773469 [details] [diff] [review]
System add-on patch

Rebecca, Benjamin asked that I redirect that data review to you. Can you please take a look?

Thanks.

Comment 40

[[e-mail]]

First, let me quickly review this thread to get caught up.  Forgive the ob(li)viousness.

A) Malware has struck the Firefox population.  An add-on has been installed on numerous clients that disables Telemetry and other badness.  

B) The system add-on has been written to fix these issues (and remove malware).  This add-on will be deployed to all prerelease and release clients running versions 44-46.  A payload with the schema described in comment 29 is created.  Depending on legal review, this will be stored as an .rst in mozilla-central.

C) Upon installation of the system add-on, if the malware is detected on the client, the malware is removed and prior preferences are reinstated.  Regardless of whether the malware is detected, a custom ping with the specified payload is sent once (and never again).  The system add-on remains installed but does not send any further data.  As a result, we only get one ping per client with the system add-on installed. 

D) The data is sent via submitExternalPing and thus all data is collected via Unified Telemetry (and obeys the established data collection protocol that all UT pings follow).

:kmag, Are all these correct (in particularly point C)?  In which case, a few additional questions:

1) Given the security risk, the need to know this information is clear.  This ping sounds opt-out.  That’s correct?

2) Who will be primarily looking at the data collected by this ping?  As Ilana described, some hybrid between lua filters and a Spark job could handle this.  :andym or :kev, is someone on your team going to own the data retrieval?

3) How long will this data collection last?  It looks like 60 days as per comment 7.  Correct?

Comment 41

[Andy McKay]

> 2) Who will be primarily looking at the data collected by this ping?  As
> Ilana described, some hybrid between lua filters and a Spark job could
> handle this.  :andym or :kev, is someone on your team going to own the data
> retrieval?

dzeber is helping us out on that.

Comment 42

[Kris Maglione [:kmag]]

(In reply to Rebecca Weiss from comment #40)
> First, let me quickly review this thread to get caught up.  Forgive the
> ob(li)viousness.
> 
> A) Malware has struck the Firefox population.  An add-on has been installed
> on numerous clients that disables Telemetry and other badness.  
> 
> B) The system add-on has been written to fix these issues (and remove
> malware).  This add-on will be deployed to all prerelease and release
> clients running versions 44-46.  A payload with the schema described in
> comment 29 is created.  Depending on legal review, this will be stored as an
> .rst in mozilla-central.
> 
> C) Upon installation of the system add-on, if the malware is detected on the
> client, the malware is removed and prior preferences are reinstated. 
> Regardless of whether the malware is detected, a custom ping with the
> specified payload is sent once (and never again).  The system add-on remains
> installed but does not send any further data.  As a result, we only get one
> ping per client with the system add-on installed. 
> 
> D) The data is sent via submitExternalPing and thus all data is collected
> via Unified Telemetry (and obeys the established data collection protocol
> that all UT pings follow).
> 
> :kmag, Are all these correct (in particularly point C)?  In which case, a
> few additional questions:

All are correct, except for a few details about point C:

The add-on only sends one telemetry ping once the browser is free of malware.
If malware is found, it always sends a ping. If malware was found in the last
session, it also always sends a ping. If malware was not found in the last
session, and is also not found in this session, it does not send a ping.

> 1) Given the security risk, the need to know this information is clear. 
> This ping sounds opt-out.  That’s correct?

Yes.

> 3) How long will this data collection last?  It looks like 60 days as per
> comment 7.  Correct?

Yes, or shorter if possible.

Comment 43

[[e-mail]]

So the ping process looks like this:

System add-on installed -> Session without malware -> send ping -> Session without malware -> No ping sent from this point on

System add-on installed -> Session with malware detected -> send ping -> remove malware -> Session without malware -> send ping -> Session without malware -> No ping sent from this point on

By this model, the only way a client will continuously send pings is if a client is in some scenario where the malicious add-on is reinstalling itself (either by the user or by some other software), correct?  And it is sent on a session level, so if this scenario is occurring we would see a high volume of pings for a small set of clients.  

Finally, since this will last 60 days or less, this will not happen permanently, so eventually we will not see any more pings of this type.

If the above is true, I will sign off on the data review.  :kmag can you confirm?

Comment 44

[Kris Maglione [:kmag]]

Attachment: Packaged add-on

Comment 45

[Kris Maglione [:kmag]]

(In reply to Rebecca Weiss from comment #43)
> So the ping process looks like this:
>
> System add-on installed -> Session without malware -> send ping -> Session
> without malware -> No ping sent from this point on
>
> System add-on installed -> Session with malware detected -> send ping ->
> remove malware -> Session without malware -> send ping -> Session without
> malware -> No ping sent from this point on
>
> By this model, the only way a client will continuously send pings is if a
> client is in some scenario where the malicious add-on is reinstalling itself
> (either by the user or by some other software), correct?  And it is sent on
> a session level, so if this scenario is occurring we would see a high volume
> of pings for a small set of clients.
>
> Finally, since this will last 60 days or less, this will not happen
> permanently, so eventually we will not see any more pings of this type.
>
> If the above is true, I will sign off on the data review.  :kmag can you
> confirm?

All of the above is correct.

Thanks.

Comment 46

[Georg Fritzsche [:gfritzsche]]

(In reply to Kris Maglione [:kmag] from comment #45)
> All of the above is correct.

Can we have this kind of description included in the documentation?
This will be valuable for future understanding.

Comment 47

[[e-mail]]

Signing off; note that I have not reviewed the actual measurement instrumentation, I am only signing off on data review.

Comment 48

[Dave Zeber [:dzeber]]

I've set up a Spark job to do a quick count of received pings, as per Andy's request. At the moment, it counts total pings received and unique # clients by submission date as well as app/channel/version. At this point, it is intended mainly for testing purposes (make sure we are getting pings, and from the right population segment) and can be expanded as necessary.

The results are available as a CSV at https://metrics.mozilla.com/protected/dzeber/malware-ping/malware_addon_counts.csv, updated on a daily basis (provided new pings were received, otherwise it is not updated). The code for the Spark job is at https://github.com/mozilla/addon-adhoc-analysis/blob/master/malware-ping/Malware%20addon%20ping.ipynb, and the repo also contains a bash script for updating the CSV on the web server. The repo is private and you may not automatically have access - let me know if there is a Github group I should add that would cover people on this bug.

As of now (the last week or two), I'm not seeing any new pings. The only ones found are test pings sent around June 1. Not sure if that's expected or not.

Comment 49

[Andreas Wagner [:TheOne] [use NI]]

Attachment: user.js

Comment 50

[Andreas Wagner [:TheOne] [use NI]]

Attachment: malware from yt unblocker.xpi

watcher.xpi used by youtube unblocker

Comment 51

[Andreas Wagner [:TheOne] [use NI]]

I have a couple of near-identical clones of the attachment in comment 50, the only difference being metadata in install.rdf

Comment 52

[Andreas Wagner [:TheOne] [use NI]]

Attachment: malware found by kev.xpi

watcher.xpi malware kev found. Seems to be an earlier version from 2014. Also it's completely unobfuscated and contains code comments.

Comment 53

[Andreas Wagner [:TheOne] [use NI]]

Attachment: watcher_deobfuscated.xpi

I ran the watcher.xpi used by youtube-unblocker through a deobfuscation tool for easier code inspection.

Comment 54

[Kris Maglione [:kmag]]

Attachment: System add-on patch

Added code to poison existing instance of the malware add-on to prevent it from interfering with blocklist pings or reinstalling itself, and also reset a few preferences that the add-on was setting outside of user.js.

Comment 55

[Kris Maglione [:kmag]]

Attachment: Packaged add-on

Comment 56

[Kris Maglione [:kmag]]

Attachment: System add-on patch

Updated again to also handle the malicious stylesheet in a newer version of the add-on.

Comment 57

[Kris Maglione [:kmag]]

Attachment: Packaged add-on

Comment 58

[Kris Maglione [:kmag]]

Attachment: Packaged add-on

Slight update to fix the issue reported in bug 1294418:

Newer versions of the malware add-on are not signed, so when we reset the signatures.required pref, they're immediately disabled. Then, when we uninstall them, their bootstrap.js is re-loaded, and they re-register their request blocking code.

This change just reorders the operations slightly so that the add-on is uninstalled before the preferences are reset.

Comment 59

[Mark Banner (:standard8)]

cc'ing release drivers so they are aware of this as well. Note that release-drivers should be notified about the pending release (and getting approval for it), although I'm not sure how that combines with this being a security issue.

https://wiki.mozilla.org/Firefox/Go_Faster/Process

Comment 60

[Dave Zeber [:dzeber]]

Some updates on the data we're receiving from this ping:

- Data has been coming in since the end of July. We are seeing around 50-60 pings per day.

- A CSV summarizing the ping contents is available at https://metrics.mozilla.com/protected/dzeber/malware-ping/malware_addon_counts.csv (behind LDAP), updated on a daily basis. It contains counts of pings and unique clients by submission date/app/channel/version, as well as by summarized payload contents.

Two comments about the payload contents:

- The schema lists the field
secmoddbAddon: <string | null>, // If a malicious secmodd.db file was found, 
                                // the extension ID that the file contained". 
In the payload, the field name has a typo and is listed as "secmoddAddon" (missing the 'b'). This is no problem so long as we use this spelling in the analysis.

- The field
mainAddonBlocked: <bool | null>, // True if the add-on exists and is blocked,
                                 // false if it exists and is not blocked,
                                 // or null if the add-on was not found.
shows up as <int | null> rather than boolean. The source shows that this is read from the "blocklistState" add-on property, which is integer-valued.

Comment 61

[Kris Maglione [:kmag]]

(In reply to Dave Zeber [:dzeber] from comment #60)
> Two comments about the payload contents:
> 
> - The schema lists the field
> secmoddbAddon: <string | null>, // If a malicious secmodd.db file was found, 
>                                 // the extension ID that the file
> contained". 
> In the payload, the field name has a typo and is listed as "secmoddAddon"
> (missing the 'b'). This is no problem so long as we use this spelling in the
> analysis.

Sorry, that was a typo. It should be "secmoddAddon".

> - The field
> mainAddonBlocked: <bool | null>, // True if the add-on exists and is blocked,
>                                  // false if it exists and is not blocked,
>                                  // or null if the add-on was not found.
> shows up as <int | null> rather than boolean. The source shows that this is
> read from the "blocklistState" add-on property, which is integer-valued.

I think this changed after I wrote the initial schema docs. The field should
be an integer value of the add-on's blocklist state if the add-on is
installed, or null if it isn't.

Comment 62

[Dave Zeber [:dzeber]]

Ah ok - then it looks like everything's working as intended :).

Comment 63

[Kris Maglione [:kmag]]

Attachment: Packaged add-on

Updated add-on to deal with newer IDs for YouTube Unblocker.

Comment 64

[Kris Maglione [:kmag]]

Attachment: Packaged add-on

Updated again to add a couple more ID patterns.

Comment 65

[Kris Maglione [:kmag]]

In the latest version, any add-on ID that ends with "@unblocker.yt",
"@sparpilot.com", "@suchpony.de", or "@axtara.com", or with the exact ID
"addon@gemaoff", will be detected as the main add-on.

Any add-on that ends with "@unblocker.yt" or "@starpilot.com", or the exact
IDs "[e-mail]" or "addon@gemaoff", will be force-disabled, even if it
somehow remains enabled after the blocklist is re-enabled.

In practice, that should only happen if the browser is prevented from updating
the blocklist by the malware. Which, in practice, should not happen.

Comment 66

[Osmose [:osmose, :mkelly]]

(In reply to Kris Maglione [:kmag] from comment #64)
> Created attachment 8788621 [details]
> Packaged add-on
> 
> Updated again to add a couple more ID patterns.

Can you put up a build with a bumped version number? Since 1.0 already got signed I'd like the next signed copy to be a different version (1.1 is fine).

Comment 67

[Kris Maglione [:kmag]]

Attachment: Packaged add-on version 1.1

Comment 68

[Osmose [:osmose, :mkelly]]

The 1.1 version should now be served from Balrog to Firefox 45 using the release-sysaddon testing channel.

krupa: Can you verify if the 1.1 version is being sent properly to the test channel?

Comment 69

[krupa raj [:krupa--use this to needinfo]]

(In reply to Michael Kelly [:mkelly,:Osmose] from comment #68)
> The 1.1 version should now be served from Balrog to Firefox 45 using the
> release-sysaddon testing channel.
> 
> krupa: Can you verify if the 1.1 version is being sent properly to the test
> channel?

I think the channel is still serving 1.0 :(

Comment 70

[Osmose [:osmose, :mkelly]]

Yeah, my fault. I got the correct version signed now, and uploaded it. I tested locally this time and was able to get the 1.1 version, so I _think_ it's all good now.

Comment 71

[krupa raj [:krupa--use this to needinfo]]

(In reply to Michael Kelly [:mkelly,:Osmose] from comment #70)
> Yeah, my fault. I got the correct version signed now, and uploaded it. I
> tested locally this time and was able to get the 1.1 version, so I _think_
> it's all good now.

1.1 is now being served for all 45 users.

Comment 72

[Mark Banner (:standard8)]

1.1 is now available on release-sysaddons being served to 46.* and is ready for QA testing.

Comment 73

[Andy McKay]

Attachment: metrics.patch

Comment 74

[Benjamin Smedberg]

Comment on attachment 8792676 [details] [diff] [review]
metrics.patch

Pls fix "No newline at end of file"

data-review=me

Comment 75

[Andy McKay]

Attachment: metrics-two.patch

Comment 76

[Andy McKay]

Sheriffs, please check-in the metrics-two.patch (r+'d in comment 74) and leave the bug open. Thank you.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8793327 [details] [diff] [review]
metrics-two.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a52aa73d6ea

Comment 78

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a52aa73d6ea

Comment 79

[Andy McKay]

Just a note that version 1.2 of the add-on is on bug 1304325, the versions 1.1 and lower on this bug should not be used.

Comment 80

[Andy McKay]

We've rolled this out to users of Firefox 44-46 and feel like we shouldn't do much more on this. I think next steps are probably to mark resolved fix and make public. But I think we need dveditz to make it public.

Comment 81

[Kris Maglione [:kmag]]

Bug 1307861 should be made public at the same time as this one.

Comment 82

[krupa raj [:krupa--use this to needinfo]]

This was tested as a system add-on on 44, 45 and 46.

Comment 83

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.