1291472 - Add more application reputation extensions

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[François Marier [:francois]]

Attachment: download_file_types.pb

Chrome now runs a lot more file extensions through download protection.

As a first step, to try and reduce the impact of malware campaigns (e.g. bug 1282106), we should add the missing extensions.

The rest of the work to handle files of different types is tracked in bug 1213459.

Comment 1

[François Marier [:francois]]

Attachment: Bug 1291472 - Add more application reputation extensions.

Review commit: https://reviewboard.mozilla.org/r/68774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68774/

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8777152 [details]
Bug 1291472 - Add more application reputation extensions.

https://reviewboard.mozilla.org/r/68774/#review65890

This list contains every archive format known to man, including some exceedingly rare ones, that require specialized extractors. I would expect that if we land this, effectively almost every download done by a Firefox user that isn't explicitly whitelisted will cause a remote query.

I could see the necessity of including .zip in the previous lists, as Windows can extract those by default, so it's easy to catch out an unsuspecting user. And I can also see a point in extending it to things that WinZip and friends support (which might have been installed by an OEM). But scanning "paq8jd"? Those require not common available, specialized extractors. If the user has one of those on their system, they either know what they're doing or they're blindly following some instructions and may be a lost cause.

On top of that, the odds that we can ever scan inside these archives is 0, so we can't determine that they don't contain executables and again will keep sending everything to the remote server.

I'm not sure we're on the right side of the security vs privacy debate here. I'm tempted to say, as a minimum boundary, if we wouldn't ever ship a decompressor that can scan for executables in an archive format, and the format isn't something that can normally be decoded by an OEM Windows PC, then maybe we shouldn't include the format in this list.

I'm open to arguments for either side here, but for now I'm clearing the review flag.

Comment 3

[François Marier [:francois]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> I'm not sure we're on the right side of the security vs privacy debate here.
> I'm tempted to say, as a minimum boundary, if we wouldn't ever ship a
> decompressor that can scan for executables in an archive format, and the
> format isn't something that can normally be decoded by an OEM Windows PC,
> then maybe we shouldn't include the format in this list.

As a first step, how about we include all of these extensions:

- it's directly executable on Windows/Mac/Linux, or
- it can be open directly (e.g. .zip, .dmg) on Windows/Mac/Linux
- it's executable using very commonly available sofware (e.g. .swf, .pdf)

I can do a first pass through these extensions and see what will be missing. Do you think that's reasonable?

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Yeah, sounds great!

Comment 5

[François Marier [:francois]]

Comment on attachment 8777152 [details]
Bug 1291472 - Add more application reputation extensions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68774/diff/1-2/

Comment 6

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #3)
> I can do a first pass through these extensions and see what will be missing.

Revision 2 of my patch is the result of my going through even single extension and trying to figure out what they are. I've annotated most of them.

As far as I can see, most can be opened or executed directly on Windows, Mac or Linux.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8777152 [details]
Bug 1291472 - Add more application reputation extensions.

https://reviewboard.mozilla.org/r/68774/#review67126

Can we check the % of downloads sent for remote verification before and after this patch? I'd expect a significant uptick.

Comment 8

[François Marier [:francois]]

Comment on attachment 8777152 [details]
Bug 1291472 - Add more application reputation extensions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68774/diff/2-3/

Comment 9

[François Marier [:francois]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #7)
> Can we check the % of downloads sent for remote verification before and
> after this patch? I'd expect a significant uptick.

We have the number of downloads sent to the remote server if we sum these up:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-08-09&keys=__none__!__none__!__none__&max_channel_version=nightly%252F51&measure=APPLICATION_REPUTATION_SERVER&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-08-01&table=1&trim=1&use_submission_date=0

and then we have the number of downloads that go through the Application Reputation pipeline here:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-08-09&keys=__none__!__none__!__none__&max_channel_version=nightly%252F51&measure=APPLICATION_REPUTATION_COUNT&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-08-01&table=1&trim=1&use_submission_date=0

However, neither of these numbers currently include the file extensions that are not considered executable and that will become executable once this patch lands.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5d6844dce12
Add more application reputation extensions. r=gcp

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5d6844dce12

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed on Mac/Linux/Windows using latest Fx51.

I enabled logging, went to http://testsafebrowsing.appspot.com/ to use their test links, as well as made samples of a variety of new file types we wish to look up.

I verified that downloading items on the list caused us to log decisions on whether to look them up or not, consistent with the logic of this feature. I also made sure that ZIP files were only looked up on Windows. 

Had issues getting logging to stdout on Windows working, but a combination of log file and some other magic finally got it working.

Comment 13

[François Marier [:francois]]

Comment on attachment 8777152 [details]
Bug 1291472 - Add more application reputation extensions.

Approval Request Comment
[Feature/regressing bug #]: Expands the list of file extensions that are checked by download protection.
[User impact if declined]: Malware campaigns like bug 1282106 continue to evade detection through the use of different file extensions.
[Describe test coverage new/current, TreeHerder]: Manual tests
[Risks and why]: Low risk, it's just adding more file extensions to the list. I've already confirmed with Google that they can handle the extra load on their infrastructure.
[String/UUID change made/needed]: None

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8777152 [details]
Bug 1291472 - Add more application reputation extensions.

Expanding the list of extensions checked sounds good. Let's take this for aurora and for beta 5. 
gcp, can you suggest a release note? Do you think it is worth listing the extensions/filetypes checked somewhere, maybe in SUMO?

Comment 15

[François Marier [:francois]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Better protection against malware/unwanted downloads.
[Suggested wording]: Download protection now covers a much larger number of executable file types on Windows, Mac and Linux.
[Links (documentation, blog post, etc)]: The full list of extensions is here: https://hg.mozilla.org/mozilla-central/file/054d4856cea6/toolkit/components/downloads/ApplicationReputation.cpp#l399

Comment 16

[François Marier [:francois]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Do you think it is worth listing the extensions/filetypes checked somewhere, maybe in SUMO?

We intend to keep up with new extensions as they are added to Chrome, so it might be a little tedious to keep that list up to date on SUMO. Also, it's a rather large list.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5872b231bb7a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/02a10b6c0a95

Comment 19

[Chris H-C :chutten]

We're seeing[1] a sudden increase in responses of 2 (INVALID) in the telemetry probe APPLICATION_REPUTATION_SERVER.

The changelog[2] for the alert[3] has a change referencing this bug.

Would this happen to be the cause? Is this expected? Is this worrisome?

[1]: https://mzl.la/2bfJSYx
[2]: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6cf0089510fad8deb866136f5b92bbced9498447&tochange=0502bd9e025edde29777ba1de4280f9b52af4663
[3]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/420/alerts/?from=2016-08-11&to=2016-08-11

Comment 20

[Gian-Carlo Pascutto [:gcp]]

(In reply to François Marier [:francois] from comment #9)
> However, neither of these numbers currently include the file extensions that
> are not considered executable and that will become executable once this
> patch lands.

Yes, but I would expect a change in ratio. Remote verdicts are 1/20 right now (which is excellent, really!). I would think that adding all these extensions will increase the remote verdict ratio. (I'd be happy if it didn't).

Comment 21

[François Marier [:francois]]

(In reply to Chris H-C :chutten from comment #19)
> Would this happen to be the cause? Is this expected? Is this worrisome?

Given that INVALID is caused by a failure to parse the protobuf response on an HTTP 200:

https://dxr.mozilla.org/mozilla-central/rev/97a52326b06a07930216ebefa5af333271578904/toolkit/components/downloads/ApplicationReputation.cpp#1396-1403

this is not something I was expecting.

It's unclear why Firefox can't parse these responses, but hopefully updating the protobuf file will fix most of these. I filed bug 1296472 for that.

Comment 22

[Chris H-C :chutten]

We're now seeing (as of August 20) an increase in the number of 5 (UNKNOWN) responses from the reputation server: https://mzl.la/2bNKGBB

This lines up with bug 1296472's landing, and the return of APPLICATION_REPUTATION_SERVER responses to normal: https://mzl.la/2bfJSYx

Should 7% of responses be Unknown? Seems a little high.

Comment 23

[Gian-Carlo Pascutto [:gcp]]

...In addition to that, the Histogram specification should be updated to add this new "5 (UNKNOWN)" response.

Comment 24

[Chris H-C :chutten]

Also, while updating Histograms.json, you may wish to consider adding email addresses to the alert_emails fields of the application reputation probes. That way those email addresses (or mailing lists) will receive notification as soon as our automated systems detect a change in those probes... (in other words, you don't have to wait for me to find time to triage alerts on histograms you care about).

Comment 25

[Gian-Carlo Pascutto [:gcp]]

The histograms that were recently touched have alert_emails, these have just been there from before alert_emails was a thing. We should add them for URLCLASSIFIER_* too.

Comment 26

[François Marier [:francois]]

(In reply to Chris H-C :chutten from comment #22)
> We're now seeing (as of August 20) an increase in the number of 5 (UNKNOWN)
> responses from the reputation server: https://mzl.la/2bNKGBB

The UNKNOWN verdict (5) is also relatively new, so I'm not exactly sure what it means. I will ask Google.

It's possible they've started to issue this one instead of SAFE when they're not sure. Prior to that being added, it had to be SAFE unless they found it to be bad.

(In reply to Chris H-C :chutten from comment #24)
> Also, while updating Histograms.json, you may wish to consider adding email
> addresses to the alert_emails fields of the application reputation probes.

I've filed bug 1297865 to track this.

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx50 release notes