1688871 - Download protection requests don't set API key

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Dimi Lee [:dimi]]

Google reported to us that download protection requests made from Firefox do not set the API key correctly.

The current request is:
https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_SAFEBROWSING_API_KEY%
The right one should look like:
https://sb-ssl.google.com/safebrowsing/clientreport/download?key=AIzaSyC7jspt...

From a quick look, this seems to be related to the fix in Bug 1237103, so this bug might have been here since Firefox 44. This bug didn’t affect the functionality of the download protection because Google didn’t drop requests without the API key previously. However, they are migrating the backends onto a newer infrastructure (1% of traffic at this point), which will drop requests without the correct API key set. This means users will not be protected by download protection.

We should fix this issue as soon as possible. For the older version of Firefox, we are still discussing if there is any possible workaround for this issue.

Comment 1

[Dimi Lee [:dimi]]

Attachment: Bug 1688871 - Fix remote query error r=gcp

Comment 2

[Dimi Lee [:dimi]]

Comment on attachment 9199251 [details]
Bug 1688871 - Fix remote query error r=gcp

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This is unclear at this point because it depends on what Google will do for download protection requests sent by Firefox. Basically the attacker can learn from the patch that we didn't send the API key.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 44
• If not all supported branches, which bug introduced the flaw?: Bug 1237103
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: We should be able to apply the change easily, the code hasn't been changed for a while
• How likely is this patch to cause regressions; how much testing does it need?: Not likely, the logic in the patch is straightforward

Comment 3

[Daniel Veditz [:dveditz]]

This bug potentially makes Firefox users less safe than we want them to be (if the missing key means Google doesn't tell us about bad executables), but I think the severity should be capped at sec-moderate because it's not a primary defense against being hacked through a web page.

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 9199251 [details]
Bug 1688871 - Fix remote query error r=gcp

As a sec-moderate this doesn't need a sec-approval. Would like to see it backported to ESR and beta.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/a42864e8df66da986fdf49935b130ac399249364
https://hg.mozilla.org/mozilla-central/rev/a42864e8df66

Comment 6

[Dimi Lee [:dimi]]

Comment on attachment 9199251 [details]
Bug 1688871 - Fix remote query error r=gcp

Beta/Release Uplift Approval Request

• User impact if declined: Download protection will not work, users will not be warned when they download a malware
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The logic in the patch is straightforward, we only call an additional function to format the URL
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Although this is not a sec:critical function, but without this fix, the whole download protection feature will not work
• User impact if declined: Download protection will not work, users will not be warned when they download a malware
• Fix Landed on Version: 87
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The logic in the patch is straightforward, we only call an additional function to format the URL
• String or UUID changes made by this patch: None

Comment 7

[Julien Cristau [:jcristau]]

Comment on attachment 9199251 [details]
Bug 1688871 - Fix remote query error r=gcp

approved for 86.0b3 and 78.8esr

Comment 8

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/68c4fca0dca0

Comment 9

[Dimi Lee [:dimi]]

Comment on attachment 9199251 [details]
Bug 1688871 - Fix remote query error r=gcp

Beta/Release Uplift Approval Request

• User impact if declined: Download protection will not work, users will not be warned when they download a malware.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The logic in the patch is straightforward, we only call an additional function to format the URL.
  Google verified that they received the right API key in Nightly build and so far we have not seen any bugs related to this issue.
• String changes made/needed: No

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9199251 [details]
Bug 1688871 - Fix remote query error r=gcp

approved for 85.0.1

Comment 11

[Julien Cristau [:jcristau]]

We'll get this in 78.7.1 as well.

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/9e4567b541c4

Comment 13

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/073a47a8d73d43494317681944eb475018081b70 (default - 78.8esr)
https://hg.mozilla.org/releases/mozilla-esr78/rev/894bf27f0b18d5194f08b574bb5d364fa3619374 (FIREFOX_ESR_78_7_X_RELBRANCH - 78.7.1esr)