1237103 - (CVE-2016-1947) Application Reputation remote lookups don't work

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[François Marier [:francois]]

The remote lookup service which protects downloads from malware on all platform is effectively disabled starting in Firefox 43.

This bug was introduced by the fix for bug 1107372 which failed to update this pref name:
https://dxr.mozilla.org/mozilla-central/rev/29258f59e5456a1a518ccce6b473b50c1173477e/toolkit/components/downloads/ApplicationReputation.cpp#64

As it stands, Firefox doesn't know where the application reputation server is and just bails without blocking downloaded files.

Comment 1

[François Marier [:francois]]

Attachment: Fix

Comment 2

[François Marier [:francois]]

Attachment: Fix

Dan pointed out that we only have a single provider for Application Reputation (it's all very Google-specific) so the new pref names don't make a lot of sense.

This patch simply reverts the renaming of browser.safebrowsing.appRepURL that took place in bug 1107372.

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 8704401 [details] [diff] [review]
Fix

A one-line change in 4 or 5 similar default pref files is better for uplifting than making 20 or more changes scattered around in tests on top. We need this fixed and uplifted ASAP (a ride-along for 43.0.4 even? unlikely) and less change is better for beta uplift. If we want to make this match the .provider style later do that in a new bug.

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Francois, can this be fixed with a hotfix for Fx43?

Comment 5

[François Marier [:francois]]

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #4)
> Francois, can this be fixed with a hotfix for Fx43?

Yes, we just need to set this pref:

browser.safebrowsing.appRepURL = "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%"

Comment 6

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

Sounds like we will need to hotfix this as soon as possible. I did already start the 43.0.4 build and don't want to hold that back. 

Adding this to the 43 postmortem discussion.

Comment 7

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

Whatever tests we need to add to cover this for 44 onward, we definitely should. 
I am worried that we broke this feature and just didn't notice for weeks.

Comment 8

[François Marier [:francois]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #7)
> Whatever tests we need to add to cover this for 44 onward, we definitely
> should. 
> I am worried that we broke this feature and just didn't notice for weeks.

Agreed. I'll be adding a test for this exact problem tomorrow and also investigating something weird I noticed in the tests today.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8704401 [details] [diff] [review]
Fix

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

Agree on the reasoning this is very provider specific anyway.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

In reply to Liz Henry (:lizzard) (needinfo? me) from comment #7)
> Whatever tests we need to add to cover this for 44 onward, we definitely
> should. 
> I am worried that we broke this feature and just didn't notice for weeks.

This particular flaw might've been caught by a local test, but generally speaking regarding our interactions with the remote server:

https://bugzilla.mozilla.org/show_bug.cgi?id=1175562#c15
https://bugzilla.mozilla.org/show_bug.cgi?id=1175562#c18

Fixing this bug would've raised Telemetry alerts:

https://bugzilla.mozilla.org/show_bug.cgi?id=1150921

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Tracking for an obvious reason.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Attachment: Hotfix

And now, he hotfix.

Comment 13

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

Comment on attachment 8704544 [details] [diff] [review]
Hotfix

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

Looks good.

Comment 14

[François Marier [:francois]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> Created attachment 8704544 [details] [diff] [review]
> Hotfix
> 
> And now, he hotfix.

To test this (same under Win/Mac/Linux):

1. Go to https://testsafebrowsing.appspot.com/
2. Under "Download Warnings", click on "2. Should show a "malicious" warning: link"
3. Choose "Save File" in the download popup.
4. Click the download manager arrow in the toolbar and verify that download was blocked.

Comment 15

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

Francois, how will this be fixed for 44, 45 and 45? Will there be a patch the needs to be uplifted to beta/aurora/central? Thanks!

Comment 16

[François Marier [:francois]]

I just pushed the attached "Fix" patch to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dff95d86acb

We should uplift the same patch to aurora and beta once Matt has verified it.

Comment 17

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

(In reply to François Marier [:francois] from comment #16)
> I just pushed the attached "Fix" patch to inbound:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8dff95d86acb
> 
> We should uplift the same patch to aurora and beta once Matt has verified it.

Great! Sounds good.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8dff95d86acb

Comment 19

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/releases/firefox-hotfixes/rev/7b386aaeec11e23258874a007ad7774ff8897f6d
Bug 1237103 - hotfix to fix the Application Reputation remote lookups r=jorge

Comment 20

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

Confirmed issue in latest Fx43 and Fx46.0a1 Nightly 2016-01-05.

Verified fixed in Fx46.0a1 Nightly, 2016-01-07.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/releases/firefox-hotfixes/rev/c0e0bda6c4fecfc7a793b7289dd77d6fc7406885
Bug 1237103 - hotfix to fix the Application Reputation remote lookups. Fix a typo

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Attachment: manage-two-previous-hotfix.diff

Jorge, here is a new version of the hotfix which manages the two previous hotfixes (youtube hotfix (bug 1236042) and the fragmented-mp4 (bug 1234802)). Thanks

Comment 23

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

Comment on attachment 8706852 [details] [diff] [review]
manage-two-previous-hotfix.diff

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

Looks good to me.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/releases/firefox-hotfixes/rev/5f3e2a43c8fafe1e001b921b9b45320c4e950e50
Bug 1237103 - Update the hotfix to manage the two previous hotfixes + change the youtube logic to put it back on Windows and change the UA to 43

Comment 25

[Sylvestre Ledru [:Sylvestre]]

I did a last minute change because of bug 1238816
I did that:
    if (Services.prefs.getPrefType(YOUTUBE_OVERRIDE)) {
      Services.prefs.setBoolPref(YOUTUBE_OVERRIDE, true);
      // It is a string on purpose
      Services.prefs.setCharPref(YOUTUBE_OVERRIDE_TO, "43");
    }

Comment 26

[François Marier [:francois]]

Comment on attachment 8704401 [details] [diff] [review]
Fix

Requesting uplift approval for beta and aurora since the patch has been marked as verified by Matt.

Comment 27

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

Comment on attachment 8704401 [details] [diff] [review]
Fix

This is a combined patch IIUC for all the hot fixes we did for Fx43. The fix has been verified, taking it in Aurora45 and Beta44.

Comment 28

[François Marier [:francois]]

(In reply to Ritu Kothari (:ritu) from comment #27)
> This is a combined patch IIUC for all the hot fixes we did for Fx43. The fix
> has been verified, taking it in Aurora45 and Beta44.

Just to be clear, attachment 8704401 [details] [diff] [review] only addresses the Safe Browsing issue that we fixed in 43 and 46. I know nothing about the YouTube issue that got rolled into the same 43 hotfix.

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/55e14bd31950

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a6a8fe3ec9d

Comment 31

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

Verified application reputation issue on today's beta and Aurora builds, 2016-01-19.