Closed Bug 1237103 (CVE-2016-1947) Opened 8 years ago Closed 8 years ago

Application Reputation remote lookups don't work

Categories

(Toolkit :: Safe Browsing, defect)

43 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox42 --- unaffected
firefox43 + affected
firefox44 + verified
firefox45 + verified
firefox46 + verified

People

(Reporter: francois, Assigned: francois)

References

(Depends on 1 open bug)

Details

(Keywords: regression, sec-moderate, Whiteboard: [adv-main44])

Attachments

(2 files, 2 obsolete files)

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.
Attached patch Fix (obsolete) — Splinter Review
Attached patch FixSplinter Review
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.
Attachment #8704390 - Attachment is obsolete: true
Attachment #8704401 - Flags: review?(gpascutto)
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.
Attachment #8704401 - Flags: feedback+
Francois, can this be fixed with a hotfix for Fx43?
QA Contact: mwobensmith
(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%"
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.
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.
(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 on attachment 8704401 [details] [diff] [review]
Fix

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

Agree on the reasoning this is very provider specific anyway.
Attachment #8704401 - Flags: review?(gpascutto) → review+
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
Tracking for an obvious reason.
Attached patch Hotfix (obsolete) — Splinter Review
And now, he hotfix.
Attachment #8704544 - Flags: review?(jorge)
Comment on attachment 8704544 [details] [diff] [review]
Hotfix

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

Looks good.
Attachment #8704544 - Flags: review?(jorge) → review+
(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.
Depends on: 1237370
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!
Flags: needinfo?(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.
Flags: needinfo?(francois)
(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.
https://hg.mozilla.org/mozilla-central/rev/8dff95d86acb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Confirmed issue in latest Fx43 and Fx46.0a1 Nightly 2016-01-05.

Verified fixed in Fx46.0a1 Nightly, 2016-01-07.
Depends on: 1237763
Depends on: 1237766
Group: toolkit-core-security → core-security-release
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
Attachment #8704544 - Attachment is obsolete: true
Attachment #8706852 - Flags: review?(jorge)
Comment on attachment 8706852 [details] [diff] [review]
manage-two-previous-hotfix.diff

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

Looks good to me.
Attachment #8706852 - Flags: review?(jorge) → review+
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
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 on attachment 8704401 [details] [diff] [review]
Fix

Requesting uplift approval for beta and aurora since the patch has been marked as verified by Matt.
Attachment #8704401 - Flags: approval-mozilla-beta?
Attachment #8704401 - Flags: approval-mozilla-aurora?
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.
Attachment #8704401 - Flags: approval-mozilla-beta?
Attachment #8704401 - Flags: approval-mozilla-beta+
Attachment #8704401 - Flags: approval-mozilla-aurora?
Attachment #8704401 - Flags: approval-mozilla-aurora+
(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.
Depends on: 1239587
Verified application reputation issue on today's beta and Aurora builds, 2016-01-19.
Status: RESOLVED → VERIFIED
Whiteboard: [adv-main44]
Alias: CVE-2016-1947
Group: core-security-release
Regressions: 1688871
You need to log in before you can comment on or make changes to this bug.