Youtube Unblocker Remediation

VERIFIED FIXED

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: mossop, Assigned: kmag)

Tracking

(Blocks: 1 bug, {leave-open, sec-other})

Trunk
leave-open, sec-other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected)

Details

(Whiteboard: triaged)

Attachments

(9 attachments, 10 obsolete attachments)

5.89 KB, patch
Details | Diff | Splinter Review
7.77 KB, patch
Details | Diff | Splinter Review
674 bytes, application/javascript
Details
44.77 KB, application/zip
Details
44.78 KB, application/zip
Details
19.05 KB, application/zip
Details
14.71 KB, patch
rhelmer
: review+
Details | Diff | Splinter Review
4.23 KB, application/x-xpinstall
Details
2.49 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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.
Attachment #8733120 - Flags: feedback?(kev)
Keywords: sec-other

Updated

2 years ago
Flags: needinfo?(jorge)
The code looks okay to me. My only note is that the minVersion should be 44.0 instead of 44.
Flags: needinfo?(jorge)
(Reporter)

Updated

2 years ago
Assignee: dtownsend → nobody
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>remediation@mozilla.org</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['remediation@mozilla.org'] += [
> +  'bootstrap.js',
> +  'install.rdf'

Please add trailing comma.

Comment 4

2 years ago
(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

2 years ago
As per the meeting today, Kris will build this as a seperate add-on.
Assignee: nobody → kmaglione+bmo
Created attachment 8749861 [details] [diff] [review]
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 "youtubeunblocker@unblocker.yt" 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.
Attachment #8749861 - Flags: feedback?(kev)

Comment 7

2 years ago
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?
Attachment #8749861 - Flags: feedback?(kev) → feedback+

Updated

2 years ago
Attachment #8733120 - Flags: feedback?(kev)

Comment 8

2 years ago
Reviewing to remove sounds like a good idea.

Comment 9

2 years ago
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.
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

2 years ago
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.
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

2 years ago
(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

2 years ago
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.
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?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(isegall)

Comment 16

a year ago
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?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(isegall)
Flags: needinfo?(amckay)
(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.
Flags: needinfo?(amckay)

Comment 18

a year ago
No history needed, so we can actually do everything with lua filters. Great news.
(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
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.
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

a year ago
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.
Created attachment 8758471 [details] [diff] [review]
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.
Attachment #8758471 - Flags: review?(rhelmer)
Attachment #8758471 - Flags: review?(benjamin)
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.
Attachment #8758471 - Flags: review?(rhelmer) → review+
(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.
(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.
(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.

Updated

a year ago
Whiteboard: triaged

Updated

a year ago
Depends on: 1277321

Comment 28

a year ago
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
Attachment #8758471 - Flags: review?(benjamin) → review-
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.
Flags: needinfo?(benjamin)

Updated

a year ago
Flags: needinfo?(benjamin)

Comment 30

a year ago
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.
(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
(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.
Hey Kris, how's the system add-on shaping up? Do we have an ETA for when it'll be ready for testing?
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
(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.
Created attachment 8773469 [details] [diff] [review]
System add-on patch
Attachment #8758471 - Attachment is obsolete: true
Attachment #8773469 - Flags: review?(rhelmer)
Attachment #8773469 - Flags: review?(benjamin)
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?
Attachment #8773469 - Flags: review?(rhelmer) → review+
(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 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.
Attachment #8773469 - Flags: review?(benjamin) → review?(rweiss)

Comment 40

a year ago
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?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kev)
Flags: needinfo?(amckay)
> 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.
Flags: needinfo?(amckay)
(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.
Flags: needinfo?(kmaglione+bmo)

Comment 43

a year ago
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?
Flags: needinfo?(kmaglione+bmo)
Created attachment 8774832 [details]
Packaged add-on
(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.
Flags: needinfo?(kmaglione+bmo)
(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

a year ago
Signing off; note that I have not reviewed the actual measurement instrumentation, I am only signing off on data review.
Flags: needinfo?(kev)
Depends on: 1290141
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.
Created attachment 8777539 [details]
user.js
Created attachment 8777562 [details]
malware from yt unblocker.xpi

watcher.xpi used by youtube unblocker
I have a couple of near-identical clones of the attachment in comment 50, the only difference being metadata in install.rdf
Attachment #8777562 - Attachment mime type: application/x-xpinstall → application/zip
Created attachment 8777565 [details]
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.
Created attachment 8777567 [details]
watcher_deobfuscated.xpi

I ran the watcher.xpi used by youtube-unblocker through a deobfuscation tool for easier code inspection.
Attachment #8777565 - Attachment mime type: application/x-xpinstall → application/zip
Attachment #8777567 - Attachment mime type: application/x-xpinstall → application/zip
Depends on: 1292584
Depends on: 1293303

Updated

a year ago
Depends on: 1293649
Created attachment 8779554 [details] [diff] [review]
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.
Attachment #8773469 - Attachment is obsolete: true
Attachment #8773469 - Flags: review?(rweiss)
Attachment #8779554 - Flags: review?(rhelmer)
Created attachment 8779555 [details]
Packaged add-on
Attachment #8774832 - Attachment is obsolete: true
Created attachment 8779559 [details] [diff] [review]
System add-on patch

Updated again to also handle the malicious stylesheet in a newer version of the add-on.
Attachment #8779554 - Attachment is obsolete: true
Attachment #8779554 - Flags: review?(rhelmer)
Attachment #8779559 - Flags: review?(rhelmer)
Attachment #8779559 - Flags: review?(rhelmer) → review+
Created attachment 8779577 [details]
Packaged add-on
Attachment #8779555 - Attachment is obsolete: true
Depends on: 1294418

Updated

a year ago
Depends on: 1295545
Depends on: 1295560
Created attachment 8782180 [details]
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.
Attachment #8779577 - Attachment is obsolete: true
Depends on: 1296133
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
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.
(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.
Ah ok - then it looks like everything's working as intended :).
Created attachment 8788609 [details]
Packaged add-on

Updated add-on to deal with newer IDs for YouTube Unblocker.
Attachment #8782180 - Attachment is obsolete: true
Created attachment 8788621 [details]
Packaged add-on

Updated again to add a couple more ID patterns.
Attachment #8788609 - Attachment is obsolete: true
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 "axter@axtara.com" 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.
(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).
Created attachment 8788661 [details]
Packaged add-on version 1.1
Attachment #8788621 - Attachment is obsolete: true
Depends on: 1300915
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?
Flags: needinfo?(kraj)

Comment 69

a year ago
(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 :(
Flags: needinfo?(kraj)
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.
Flags: needinfo?(kraj)
Depends on: 1302217

Comment 71

a year ago
(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.
Flags: needinfo?(kraj)
1.1 is now available on release-sysaddons being served to 46.* and is ready for QA testing.
Created attachment 8792676 [details] [diff] [review]
metrics.patch
Attachment #8792676 - Flags: review?(benjamin)

Comment 74

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

Pls fix "No newline at end of file"

data-review=me
Attachment #8792676 - Flags: review?(benjamin) → review+
Depends on: 1304325
Created attachment 8793327 [details] [diff] [review]
metrics-two.patch
Attachment #8792676 - Attachment is obsolete: true
Sheriffs, please check-in the metrics-two.patch (r+'d in comment 74) and leave the bug open. Thank you.
Keywords: checkin-needed, leave-open
Keywords: checkin-needed
Comment on attachment 8793327 [details] [diff] [review]
metrics-two.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a52aa73d6ea
Attachment #8793327 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/6a52aa73d6ea
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.
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.
Flags: needinfo?(dveditz)
Blocks: 1307861
Bug 1307861 should be made public at the same time as this one.
Group: firefox-core-security
Flags: needinfo?(dveditz)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED

Comment 82

a year ago
This was tested as a system add-on on 44, 45 and 46.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.