557752 - Unbranded releases should use a different phishing shavar

Status: RESOLVED DUPLICATE of bug 1288840

(Toolkit :: Safe Browsing, defect, P5)

Description

[Mike Hommey [:glandium]]

This was brought by Ian Fette when I asked if and how we could get safebrowsing working on iceweasel with or without pretending to be firefox, and it turns out we should be using googpub-phish-shavar instead of goog-phish-shavar. (requiring the client name to be allowed by google is another matter)

Now, I'm not quite sure *all* unbranded releases should be using this different shavar, but there should definitely be somewhere to note this difference. Anyways, I have the feeling that the urlclassifier.gethashtables could be in a #ifdef MOZ_OFFICIAL or something like that, with the alternative value of "googpub-phish-shavar,goog-malware-shavar".

There is another place where goog-phish-shavar is used, namely PROT_Application.prototype.initialize, and I do wonder if there is a reason for the "black table" registrations to be harcoded instead of using the urlclassifier.gethashtables variable ?

Comment 1

[Mike Hommey [:glandium]]

Attachment: Patch

Comment 2

[Tony Chang (Google)]

Do you intentionally use MOZILLA_OFFICIAL in one file and OFFICIAL_BUILD in the other?

Did you test with wireshark to see if this produced actual data or does the client name need to be allowed first?

Comment 3

[Mike Hommey [:glandium]]

(In reply to comment #2)
> Do you intentionally use MOZILLA_OFFICIAL in one file and OFFICIAL_BUILD in the
> other?

Yes.
That's because of this in browser/app/Makefile.in:
ifdef MOZILLA_OFFICIAL
DEFINES += -DMOZILLA_OFFICIAL
endif
And that in browser/components/safebrowsing/Makefile.in:
ifdef MOZILLA_OFFICIAL
DEFINES += -DOFFICIAL_BUILD=1
endif

> Did you test with wireshark to see if this produced actual data or does the
> client name need to be allowed first?

The client name needs to be allowed first, though the default client for unofficial builds should be "firefox", which is probably allowed.

I got "iceweasel" allowed a while ago, so I know for sure that this one works.

Comment 4

[Mehdi Mulani [:mmm] (I don't check this)]

Would this patch change it so that simple Firefox builds (as per MDC) use "googpub-phish-shavar" instead of "goog-phish-shavar"?

Comment 5

[Mike Hommey [:glandium]]

(In reply to comment #4)
> Would this patch change it so that simple Firefox builds (as per MDC) use
> "googpub-phish-shavar" instead of "goog-phish-shavar"?

most probably, yes.

Comment 6

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #5)
> most probably, yes.

I'm not quite sure on the Iceweasel/Firefox disparity (i.e. what other than the branding is changed) but could we fix the black table registration to also be set by the pref and then have Iceweasel merely change the pref on their side?

I ask because it seems that if a Firefox profile runs on a simple Firefox build and an official Nightly (as developers commonly do) then we will end up either clearing part of the urlclassifier3.sqlite or having an extra table which is only used in one case.

Comment 7

[Mike Hommey [:glandium]]

Theorically, it's not only an Iceweasel problem. Unofficial Firefox builds are not bound to the third-party services agreements.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 526228 [details] [diff] [review]
Patch

This can use MOZ_OFFICIAL_BRANDING now. Might be a good opportunity to check the other uses of OFFICIAL_BUILD/MOZILLA_OFFICIAL in safebrowsing code in particular (there are other bugs on sorting that out tree-wide, I think).

Comment 9

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #8)
> This can use MOZ_OFFICIAL_BRANDING now. Might be a good opportunity to check
> the other uses of OFFICIAL_BUILD/MOZILLA_OFFICIAL in safebrowsing code in
> particular (there are other bugs on sorting that out tree-wide, I think).

Do you know where the relevant documentation is for when MOZILLA_OFFICIAL vs MOZ_OFFICIAL_BRANDING is defined?
I just ran a simple Firefox build with this patch included and it seems that the goog-phish-shavar is still used so my concern in comment 6 is moot.

Comment 10

[Mike Hommey [:glandium]]

(In reply to comment #9)
> (In reply to comment #8)
> > This can use MOZ_OFFICIAL_BRANDING now. Might be a good opportunity to check
> > the other uses of OFFICIAL_BUILD/MOZILLA_OFFICIAL in safebrowsing code in
> > particular (there are other bugs on sorting that out tree-wide, I think).
> 
> Do you know where the relevant documentation is for when MOZILLA_OFFICIAL vs
> MOZ_OFFICIAL_BRANDING is defined?
> I just ran a simple Firefox build with this patch included and it seems that
> the goog-phish-shavar is still used so my concern in comment 6 is moot.

That's somehow surprising. Did you check in about:config that urlclassifier.gethashtables was good ?

Comment 11

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #10)
> That's somehow surprising. Did you check in about:config that
> urlclassifier.gethashtables was good ?

Yeah, I even used the getTables call on nsIUrlClassifierDBService. I'll try again later with MOZ_OFFICIAL_BRANDING.

Comment 12

[Mehdi Mulani [:mmm] (I don't check this)]

Alright, with MOZ_OFFICIAL_BRANDING my Nightly build ends up using the googpub-phish-shavar as intended.

The number of entries in the goog-phish-shavar table was at 354,501 before switching.
The number of entries in the googpub-phish-shavar table was at 271,528 after updating a few times (to try and reach the maximum).

From casually glancing at about:memory it looks like there isn't much of a memory hit but I can't be sure. The size of my urlclassifier3.sqlite grew by about 25MB (from 45MB).

So it seems that the goog-phish-shavar entries are kept around (which makes sense) but this could cause a problem if we do not update one of the phishing tables. If someone ran an unofficial build and then ran an official build in the same profile the googpub table would not get updated but still be taken into account. (AIUI we simply check whether the table name matching phish or malware when a hit occurs)

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> Do you know where the relevant documentation is for when MOZILLA_OFFICIAL vs
> MOZ_OFFICIAL_BRANDING is defined?

I don't know of any documentation. MOZILLA_OFFICIAL (and its sibling OFFICIAL_BUILD) should be avoided - it's set in the mozconfigs for MoCo build machines, including those used for nightly builds.

MOZ_OFFICIAL_BRANDING isn't quite right semantically, but since it overlaps almost exactly with the cases where we want to enable these things, it's probably the best thing to use. If it doesn't, we should add another configure argument, rather than depending on environment variables.

Comment 14

[Mike Hommey [:glandium]]

Seeing comment 12, maybe we should take a few steps back.

When I started caring about this issue in february last year, I got in contact with ifette who told me the reason for the two shavars was deals with third party providers, but that the subset that can't be provided for this reason was decreasing. Maybe things have somewhat improved since then? Is having two different shavars for phishing still relevant today?

Is it even clear for consumers which one to use? FWIW, http://code.google.com/intl/fr/apis/safebrowsing/developers_guide_v2.html#Overview still talks about googpub-phish-shavar, and http://code.google.com/p/google-safe-browsing/wiki/Protocolv2Spec mentions goog-phish-shavar.

It also looks like Chromium (the open source version of Chrome) uses goog-phish-shavar.

And interestingly, doing a google search for googpub-phish-shavar actually shows results for goog-phish-shavar.

Comment 15

[Frank Wein [:mcsmurf]]

Just a heads-up: Bug 825417 now fixed SafeBrowsing.jsm so that the new  SafeBrowsing.jsm code will use the urlclassifier.gethashtables pref to get the phishing/malware table names.

Comment 16

[Mike Hommey [:glandium]]

(In reply to Frank Wein [:mcsmurf] from comment #15)
> Just a heads-up: Bug 825417 now fixed SafeBrowsing.jsm so that the new 
> SafeBrowsing.jsm code will use the urlclassifier.gethashtables pref to get
> the phishing/malware table names.

Erf, I had a similar patch in the works.

Comment 17

[François Marier [:francois]]

V4 of the protocol (bug 1167038) doesn't seem to have that googpub-phish-shavar v. goog-phish-shavar distinction, so we'll probably mark this as WONTFIX once we've switched away from version 2.2 of Safe Browsing.