Update SafeBrowsing to use HTTPS

RESOLVED FIXED in Firefox 29

Status

()

Toolkit
Safe Browsing
--
enhancement
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: geekboy, Assigned: gcp)

Tracking

({privacy})

Trunk
Firefox 29
privacy
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments)

(Reporter)

Description

5 years ago
The SafeBrowsing API has supported HTTPS for a while now and we should change our implementation to use it.  Chrome shipped it and it works well:

http://src.chromium.org/viewvc/chrome?view=rev&revision=132456

This change would remove the need for MACing.

Summary of work (via Brian Ryner):
- All http://safebrowsing.clients.google.com/ URLs should change to https://safebrowsing.google.com/  (note that the .clients part is gone)
- /newkey requests are no longer necessary.
- sending the wrapped key (wrkey parameter) in requests is no longer necessary.
- handling of a rekey response is no longer necessary
- requesting the MAC (appending ;mac to the listname) is no longer necessary
- handling of MAC data appended to redirect URLs ("u:url,mac") in the response is no longer necessary.

More detail in the SafeBrowsing developer guide:
https://developers.google.com/safe-browsing/developers_guide_v2
Assignee: nobody → mmc
Blocks: 771788
This is one of the last parts of the browser's built-in functionality, other than non-default search engines, using non-HTTPS connections. (See all the bugs blocking bug 771788 that have been fixed recently.) It would be great to get this change done. This change seems to be quite a non-trivial privacy win (leaking your browsing history + safebrowsing tracking cookie to only Google instead of Google + people on the network).

There may be some performance implications here, regarding the added latency of SSL. I filed bug 877766 about one potential way of mitigating that.

Raymond, are you interested in taking this on too? I see you have done many of the other HTTP -> HTTPS conversions. From comment 0, it looks like the changes (besides bug 877766) are mostly deleting code that is no longer needed.
Keywords: privacy
See Also: → bug 877766
(Assignee)

Comment 2

4 years ago
>This change seems to be quite a non-trivial privacy win (leaking your browsing history

SafeBrowsing does not leak your browsing history. Not to Google, not to anyone else.
Brian, Gian-Carlo is right. We are already implementing version 2 of the spec, which downloads the entire safebrowsing database in chunks and looks up URLs in downloaded database. We are *not* using the Safebrowsing Lookup API that sends every URL to the service.

The only thing this bug will solve is letting us get rid of a bunch of MACs intended to detect database corruption, which will be redundant if we get it over SSL.
Keywords: privacy
(In reply to Monica Chew [:mmc] (please use ?needinfo) from comment #3)
> Brian, Gian-Carlo is right. We are already implementing version 2 of the
> spec, which downloads the entire safebrowsing database in chunks and looks
> up URLs in downloaded database. We are *not* using the Safebrowsing Lookup
> API that sends every URL to the service.

Right. It was my mistake to mix up the browsing history aspect with the tracking cookie aspect. But, the tracking cookie is still problematic, because it allows your browsing to be linked across network connections (e.g. one Wifi hotspot to another, or 3G <-> wifi). Obviously, almost any non-SSL website will cause the same issue, but I think it is worthwhile to ensure that the built-in functionality of the browser avoids such issues, especially when doing so is easy.

> The only thing this bug will solve is letting us get rid of a bunch of MACs
> intended to detect database corruption, which will be redundant if we get it
> over SSL.

Besides the tracking cookie issue, switching to HTTPS has performance consequences for the cases where we need to do the check. The performance could either be improved due to SPDY connection coalescing if you already have an open connection to another Google site, or bad if you don't already have an open connection to Google. That was/is being discussed in bug 877766.
Keywords: privacy
Whiteboard: [Must ask gavin or dougt for super-review of patch]
In that case, you might be interested in this old bug: https://bugzilla.mozilla.org/show_bug.cgi?id=368255
Blocks: 662819
No longer blocks: 662819
(Assignee)

Comment 6

4 years ago
>This change would remove the need for MACing.
>- requesting the MAC (appending ;mac to the listname) is no longer necessary
>- handling of MAC data appended to redirect URLs ("u:url,mac") in the response is no longer necessary.

FWIW, I just did a quick check here by switching all our URLs to HTTPS and seeing if things still work. They do. However, the update links we get are still of the form:

http://safebrowsing-cache.google.com/safebrowsing/rd/ChNnb29nLW1hbHdhcmUtc2hhdmFyEAAYwYMIIICGCDItwQECAP_____7_________7___-_r3_3___3_____f_7___________v7__8A

Which obviously aren't safe without the MAC. I'm going to make a guess here that Google sends us HTTP links if we request MAC usage. I'll try hacking away the MACs and see what happens.
(Assignee)

Comment 7

4 years ago
>I'm going to make a guess here that Google sends us HTTP links if we request MAC usage.

Whoops, we do that ourselves. The code that does it even points to this bug :(
(Assignee)

Comment 8

4 years ago
Created attachment 8358438 [details] [diff] [review]
Patch 1. Remove MAC support.

Do we need to play the UUID dance with the changed interfaces?
Assignee: mmc → gpascutto
Attachment #8358438 - Flags: review?(mmc)
Attachment #8358438 - Flags: review?(dcamp)
(Assignee)

Comment 9

4 years ago
Created attachment 8358439 [details] [diff] [review]
Patch 2. Download forwards via HTTPS

I hope this catches all the tests...
Attachment #8358439 - Flags: review?(mmc)
Attachment #8358439 - Flags: review?(dcamp)
(Assignee)

Comment 10

4 years ago
Created attachment 8358440 [details] [diff] [review]
Patch 3. Remove all MAC-related tests.
Attachment #8358440 - Flags: review?(mmc)
Attachment #8358440 - Flags: review?(dcamp)
(Assignee)

Comment 11

4 years ago
Created attachment 8358441 [details] [diff] [review]
Patch 4. Update settings for all Firefox versions (includes apikey "key' add)
Attachment #8358441 - Flags: superreview?(gavin.sharp)
Attachment #8358441 - Flags: review?(mmc)
Attachment #8358441 - Flags: review?(dcamp)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 779317

Comment 13

4 years ago
I heard that back when this was implemented, Google didn't want us to use https for performance or load reasons, have we cleared with them that it's OK now? (I guess with the new technologies Google and us implemented in that area, those connections might have become lighter anyhow.)
(Assignee)

Comment 14

4 years ago
Yes, see comment 0. The input is from Brian Ryner, which deals with SafeBrowsing at Google. Chrome already switched.
Comment on attachment 8358438 [details] [diff] [review]
Patch 1. Remove MAC support.

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

Looks good.

::: toolkit/components/url-classifier/nsIUrlClassifierDBService.idl
@@ +31,1 @@
>  interface nsIUrlClassifierUpdateObserver : nsISupports {

This requires a uuid change (./mach uuid) right above this line.

::: toolkit/components/url-classifier/nsIUrlClassifierHashCompleter.idl
@@ +16,1 @@
>     * nsIUrlClassifierCompleter::complete() call.

uuid change

::: toolkit/components/url-classifier/nsIUrlClassifierStreamUpdater.idl
@@ +22,1 @@
>    /**

uuid change

::: toolkit/components/url-classifier/nsIUrlListManager.idl
@@ +22,1 @@
>  interface nsIUrlListManager : nsISupports

Same here

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ -266,5 @@
>    return NS_OK;
>  }
>  
> -NS_IMETHODIMP
> -nsUrlClassifierStreamUpdater::RekeyRequested()

The rekeying tests from netwerk/test/unit/test_cookiejars_safebrowsing.js can also be removed.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
@@ -389,5 @@
> -  nsAutoCString base64(key);
> -  UnUrlsafeBase64(base64);
> -
> -  // PL_Base64Decode doesn't null-terminate unless we let it allocate,
> -  // so we need to calculate the length ourselves.

Wow. Thank you for eliminating this.
Attachment #8358438 - Flags: review?(mmc) → review+
Attachment #8358439 - Flags: review?(mmc) → review+
Comment on attachment 8358440 [details] [diff] [review]
Patch 3. Remove all MAC-related tests.

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

I don't understand how this interacts with the competer changes in test_partial. Other than that, looks good.
Attachment #8358440 - Flags: review?(mmc) → review+
Comment on attachment 8358441 [details] [diff] [review]
Patch 4. Update settings for all Firefox versions (includes apikey "key' add)

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

Thanks very much for taking this bug. How does try look?

Please fix the uuid issues and either file a bug for removing the rekey testing from netwerk/test/unit/test_cookiejars_safebrowsing.js or include it in this patch.

::: b2g/app/b2g.js
@@ +317,1 @@
>  #ifdef MOZ_SAFE_BROWSING

This is not enabled on b2g, right?

::: browser/metro/profile/metro.js
@@ +571,1 @@
>  #ifdef MOZ_SAFE_BROWSING

Is this enabled on metro?

::: testing/profiles/prefs_general.js
@@ +67,5 @@
>  user_pref("camino.warn_when_closing", false); // Camino-only, harmless to others
>  
>  // Make url-classifier updates so rare that they won't affect tests
>  user_pref("urlclassifier.updateinterval", 172800);
>  // Point the url-classifier to the local testing server for fast failures

Can you include a note that we can't use https for localhost?
Attachment #8358441 - Flags: review?(mmc) → review+
(Assignee)

Comment 18

4 years ago
>I don't understand how this interacts with the competer changes in test_partial. Other than that, looks good.

Excellent question! This comment should answer it:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsIUrlClassifierHashCompleter.idl#24

Apparently this was needed for cases where we get a hit, need to do a completion, but don't have a valid key.

>This is not enabled on b2g, right?
>Is this enabled on metro?

It's not enabled on those. I've just changed those URLs already so it's good to go whenever we activate it.

>How does try look?

I have only verified that this actually (seems to) work, not that it passes tests :) It passes xpcshell locally, though. I'll fix up cookiejar and give it a spin.
(Assignee)

Comment 19

4 years ago
Created attachment 8359146 [details] [diff] [review]
Patch 5. Remove cookie jar rekeying
Attachment #8359146 - Flags: review?(mmc)
(Assignee)

Comment 20

4 years ago
Created attachment 8359167 [details] [diff] [review]
Patch 6. Update application reputation for api changes
Attachment #8359167 - Flags: review?(mmc)
Attachment #8359146 - Flags: review?(mmc) → review+
Attachment #8359167 - Flags: review?(mmc) → review+
(Assignee)

Comment 21

4 years ago
Addressed review comments, fixed one URL that I missed:
https://tbpl.mozilla.org/?tree=Try&rev=a9f871d1c2d4

Seems to still work on Android too.

If gavin or dougt give the thumbs up, and dcamp can take a look, this is good to go (I'm assuming that try there ^^^ will go green).

Updated

4 years ago
Attachment #8358438 - Flags: review?(dcamp) → review+

Updated

4 years ago
Attachment #8358440 - Flags: review?(dcamp) → review+

Updated

4 years ago
Attachment #8358441 - Flags: review?(dcamp) → review+

Updated

4 years ago
Attachment #8358439 - Flags: review?(dcamp) → review+
i am all for using https everywhere we can.  if you have a specific code question, or a question about our agreement with google, please ask (or send me mail).
(Assignee)

Comment 23

4 years ago
The whiteboard says this needs sr=dougt|gavin. I take it your comment means sr: dougt+ then?
Philip, I'm not sure if this requires seamonkey changes.
Flags: needinfo?(philip.chee)
Comment on attachment 8358441 [details] [diff] [review]
Patch 4. Update settings for all Firefox versions (includes apikey "key' add)

Please get explicit acknowledgement from an appropriate Google contact that we're going to be making this change.
Attachment #8358441 - Flags: superreview?(gavin.sharp) → superreview+

Comment 26

4 years ago
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #24)
> Philip, I'm not sure if this requires seamonkey changes.
Anything in /browser/ would need corresponding changes. Thanks for the heads up. I'm currently tracking this in SeaMonkey Bug 920951. By the way does s/apikey/key/ apply only to Firefox? Or to everyone using this API?
(In reply to Philip Chee from comment #26)
> (In reply to Monica Chew [:mmc] (please use needinfo) from comment #24)
> > Philip, I'm not sure if this requires seamonkey changes.
> Anything in /browser/ would need corresponding changes. Thanks for the heads
> up. I'm currently tracking this in SeaMonkey Bug 920951. By the way does
> s/apikey/key/ apply only to Firefox? Or to everyone using this API?

It applies to everyone using this API.
(Assignee)

Comment 28

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/661680041755
https://hg.mozilla.org/integration/mozilla-inbound/rev/e20b9bc867ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/db7475f5aecb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f48e08e09d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8df7eda20b61
https://hg.mozilla.org/integration/mozilla-inbound/rev/913a1c5086c5
Comment on attachment 8358441 [details] [diff] [review]
Patch 4. Update settings for all Firefox versions (includes apikey "key' add)

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

::: browser/app/profile/firefox.js
@@ +795,1 @@
>  pref("browser.safebrowsing.reportURL", "http://safebrowsing.clients.google.com/safebrowsing/report?");

You missed updating this one.
(Assignee)

Comment 30

4 years ago
The actual commit should be fine? I noticed it before landing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f48e08e09d#l3.18

Updated

4 years ago
Blocks: 960453
https://hg.mozilla.org/mozilla-central/rev/661680041755
https://hg.mozilla.org/mozilla-central/rev/e20b9bc867ab
https://hg.mozilla.org/mozilla-central/rev/db7475f5aecb
https://hg.mozilla.org/mozilla-central/rev/a4f48e08e09d
https://hg.mozilla.org/mozilla-central/rev/8df7eda20b61
https://hg.mozilla.org/mozilla-central/rev/913a1c5086c5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [Must ask gavin or dougt for super-review of patch]
Target Milestone: --- → Firefox 29

Comment 32

4 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/ebf1b43421ac1e4792d07b2297459a99b7fd1997
Bug 783047 - Update SafeBrowsing URLs to use HTTPS. r=mmc,dcamp sr=gavin
Blocks: 960771
Blocks: 891289
Depends on: 966659
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit

Updated

2 years ago
Flags: needinfo?(philip.chee)
You need to log in before you can comment on or make changes to this bug.