Add a whitelist for the new binary download protection.

Add a whitelist for the new binary download protection.

BUG=None
TEST=SafeBrowsingDatabaseTest


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

[panayiotis, 2011-09-09 20:33:35]

fyi

http://codereview.chromium.org/7863006/diff/12/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/7863006/diff/12/chrome/common/pref_names.h#new...
chrome/common/pref_names.h:108: extern const char
kSafeBrowsingDownloadProtectionEnabled[];
kSafeBrowsingDownloadProtectionOptinEnabled ? To differentiate from the already
existing download protection?

[noelutz, 2011-09-10 00:41:16]

renamed.  please take another look.
thanks,
noe.

On 2011/09/09 20:33:35, panayiotis wrote:
> fyi
> 
> http://codereview.chromium.org/7863006/diff/12/chrome/common/pref_names.h
> File chrome/common/pref_names.h (right):
> 
>
http://codereview.chromium.org/7863006/diff/12/chrome/common/pref_names.h#new...
> chrome/common/pref_names.h:108: extern const char
> kSafeBrowsingDownloadProtectionEnabled[];
> kSafeBrowsingDownloadProtectionOptinEnabled ? To differentiate from the
already
> existing download protection?

[mattm, 2011-09-10 01:12:48]

http://codereview.chromium.org/7863006/diff/15/chrome/browser/safe_browsing/s...
File chrome/browser/safe_browsing/safe_browsing_util.h (right):

http://codereview.chromium.org/7863006/diff/15/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_util.h:279: DOWNLOADWHITELIST = 6,
What happened to 5?

[noelutz, 2011-09-10 01:24:20]

http://codereview.chromium.org/7863006/diff/15/chrome/browser/safe_browsing/s...
File chrome/browser/safe_browsing/safe_browsing_util.h (right):

http://codereview.chromium.org/7863006/diff/15/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_util.h:279: DOWNLOADWHITELIST = 6,
On 2011/09/10 01:12:48, mattm wrote:
> What happened to 5?

SafeBrowsing lists are stored in pairs.  The list id module 2 is used to know
whether it's the 0th or 1st list in the file.  If I use 5 the download whitelist
will become the second list in the new file + we won't be able to add a second
list to the csd whitelist file (unless we hack something together).  I added a
comment.

[mattm, 2011-09-10 01:55:33]

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:592:
WhitelistEverything(&download_whitelist_);
Hm, does this mean that if the whitelist file doesn't exist we whitelist
everything?  And this will be reset once we've downloaded the whitelist?

[noelutz, 2011-09-10 02:30:56]

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:592:
WhitelistEverything(&download_whitelist_);
On 2011/09/10 01:55:33, mattm wrote:
> Hm, does this mean that if the whitelist file doesn't exist we whitelist
> everything?  And this will be reset once we've downloaded the whitelist?

Exactly.  That's a precaution to protect the user's privacy at the expense of
maybe some protection.  If the whitelist file doesn't exist or if there was an
error during download we'll whitelist everything.  Meaning we'll never send a
ping to Google with the download URL & hash.

[mattm, 2011-09-10 02:39:42]

LGTM

[Brian Ryner, 2011-09-12 21:45:18]

Looks nice, just a few questions.

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:130: PrefService::UNSYNCABLE_PREF);
Is there a reason this (and the ReportingEnabled pref, actually) is UNSYNCABLE?

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:46: // Maximum number of
entries we allow in any of the two whitelists.
May want to remove "two" so that this comment doesn't fall out of date if more
whitelists are added.

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:97: bool has_empty_path =
false;
I'm not sure I understand the extra logic you're adding here.  Doesn't
GeneratePathsToCheck always return "/" as one of the paths?  If so, wouldn't you
accomplish the same thing by removing the "paths.size() > 1" check?  Note that I
think we added that check originally because the CSD whitelist does not use
trailing slashes if there is just a hostname.

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.h:281: // result in all
lookups to the whitelist to return true.
result in -> cause

[noelutz, 2011-09-12 22:09:30]

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:130: PrefService::UNSYNCABLE_PREF);
On 2011/09/12 21:45:18, Brian Ryner wrote:
> Is there a reason this (and the ReportingEnabled pref, actually) is
UNSYNCABLE?

I figured it should be similar to the malware reporting.  Either way is fine
with me.

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:46: // Maximum number of
entries we allow in any of the two whitelists.
On 2011/09/12 21:45:18, Brian Ryner wrote:
> May want to remove "two" so that this comment doesn't fall out of date if more
> whitelists are added.

Done.

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:97: bool has_empty_path =
false;
On 2011/09/12 21:45:18, Brian Ryner wrote:
> I'm not sure I understand the extra logic you're adding here.  Doesn't
> GeneratePathsToCheck always return "/" as one of the paths?  If so, wouldn't
you
> accomplish the same thing by removing the "paths.size() > 1" check?  Note that
I
> think we added that check originally because the CSD whitelist does not use
> trailing slashes if there is just a hostname.

Duh!  I just realized that I misunderstood GeneratePathsToCheck.  I thought it
would include the 4 path components starting from the end not the beginning. 
Good catch.  Removed this special casing.

[Brian Ryner, 2011-09-12 22:43:27]

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:130: PrefService::UNSYNCABLE_PREF);
On 2011/09/12 22:09:30, noelutz wrote:
> On 2011/09/12 21:45:18, Brian Ryner wrote:
> > Is there a reason this (and the ReportingEnabled pref, actually) is
> UNSYNCABLE?
> 
> I figured it should be similar to the malware reporting.  Either way is fine
> with me.

Ok.  I'm just wondering if this was a copy and paste error when the malware
reporting pref was added -- the one right above that (session exited uncleanly)
is marked unsyncable, but it seems like most user-settable prefs are syncable
(see kSafeBrowsingEnabled for example).

[noelutz, 2011-09-12 22:55:12]

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7863006/diff/1016/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:130: PrefService::UNSYNCABLE_PREF);
On 2011/09/12 22:43:27, Brian Ryner wrote:
> On 2011/09/12 22:09:30, noelutz wrote:
> > On 2011/09/12 21:45:18, Brian Ryner wrote:
> > > Is there a reason this (and the ReportingEnabled pref, actually) is
> > UNSYNCABLE?
> > 
> > I figured it should be similar to the malware reporting.  Either way is fine
> > with me.
> 
> Ok.  I'm just wondering if this was a copy and paste error when the malware
> reporting pref was added -- the one right above that (session exited
uncleanly)
> is marked unsyncable, but it seems like most user-settable prefs are syncable
> (see kSafeBrowsingEnabled for example).

Done.

[Brian Ryner, 2011-09-12 23:11:17]

lgtm

[mattm, 2011-09-13 00:29:02]

http://codereview.chromium.org/7863006/diff/8002/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7863006/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:918: pref_value);
Oh, thinking about this a bit harder, this isn't quite right. 
Profile::RegisterUserPrefs is registering the pref in the profile prefs, not the
local state prefs.   You could register it in local_state, but for the
multi-profile world it would be best to keep it registered in the profile.  

That means you'd have to either change the code to keep track of whether any
loaded profiles have it enabled (similar to the way safebrowsing itself is
enabled) or just always update the download whitelist.

[noelutz, 2011-09-13 20:35:39]

http://codereview.chromium.org/7863006/diff/8002/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7863006/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:918: pref_value);
On 2011/09/13 00:29:02, mattm wrote:
> Oh, thinking about this a bit harder, this isn't quite right. 
> Profile::RegisterUserPrefs is registering the pref in the profile prefs, not
the
> local state prefs.   You could register it in local_state, but for the
> multi-profile world it would be best to keep it registered in the profile.  
> 
> That means you'd have to either change the code to keep track of whether any
> loaded profiles have it enabled (similar to the way safebrowsing itself is
> enabled) or just always update the download whitelist.

That does seem a bit complicated.  How about we always update that whitelist
since it is so small?  Also, we're talking about making that feature an opt-out
like SafeBrowsing so most people will have it enabled.

[mattm, 2011-09-14 01:15:18]

http://codereview.chromium.org/7863006/diff/8002/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7863006/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:918: pref_value);
On 2011/09/13 20:35:39, noelutz wrote:
> On 2011/09/13 00:29:02, mattm wrote:
> > Oh, thinking about this a bit harder, this isn't quite right. 
> > Profile::RegisterUserPrefs is registering the pref in the profile prefs, not
> the
> > local state prefs.   You could register it in local_state, but for the
> > multi-profile world it would be best to keep it registered in the profile.  
> > 
> > That means you'd have to either change the code to keep track of whether any
> > loaded profiles have it enabled (similar to the way safebrowsing itself is
> > enabled) or just always update the download whitelist.
> 
> That does seem a bit complicated.  How about we always update that whitelist
> since it is so small?  Also, we're talking about making that feature an
opt-out
> like SafeBrowsing so most people will have it enabled.

Yeah, just always updating it sounds fine.

[noelutz, 2011-09-19 17:21:02]

On 2011/09/14 01:15:18, mattm wrote:
>
http://codereview.chromium.org/7863006/diff/8002/chrome/browser/safe_browsing...
> File chrome/browser/safe_browsing/safe_browsing_service.cc (right):
> 
>
http://codereview.chromium.org/7863006/diff/8002/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/safe_browsing_service.cc:918: pref_value);
> On 2011/09/13 20:35:39, noelutz wrote:
> > On 2011/09/13 00:29:02, mattm wrote:
> > > Oh, thinking about this a bit harder, this isn't quite right. 
> > > Profile::RegisterUserPrefs is registering the pref in the profile prefs,
not
> > the
> > > local state prefs.   You could register it in local_state, but for the
> > > multi-profile world it would be best to keep it registered in the profile.
 
> > > 
> > > That means you'd have to either change the code to keep track of whether
any
> > > loaded profiles have it enabled (similar to the way safebrowsing itself is
> > > enabled) or just always update the download whitelist.
> > 
> > That does seem a bit complicated.  How about we always update that whitelist
> > since it is so small?  Also, we're talking about making that feature an
> opt-out
> > like SafeBrowsing so most people will have it enabled.
> 
> Yeah, just always updating it sounds fine.

Replaced pref with a flag.  Please take another look.
Thanks,
noe.

[mattm, 2011-09-19 21:27:36]

http://codereview.chromium.org/7863006/diff/16001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7863006/diff/16001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:912:
enable_download_whitelist_ = switches::kEnableImprovedDownloadProtection;
cmdline->HasSwitch(...) ?

http://codereview.chromium.org/7863006/diff/16001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7863006/diff/16001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:451: "enable-improved-download-protection";
I assume this is just going to be for testing purposes and we will go to having
a per-profile pref once we get the UI stuff sorted?

[noelutz, 2011-09-19 21:41:43]

Argh!  So sorry about this.

http://codereview.chromium.org/7863006/diff/16001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7863006/diff/16001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:912:
enable_download_whitelist_ = switches::kEnableImprovedDownloadProtection;
On 2011/09/19 21:27:36, mattm wrote:
> cmdline->HasSwitch(...) ?

Duh, so sorry about that.  I must have been absent minded.  Fixed.

http://codereview.chromium.org/7863006/diff/16001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7863006/diff/16001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:451: "enable-improved-download-protection";
On 2011/09/19 21:27:36, mattm wrote:
> I assume this is just going to be for testing purposes and we will go to
having
> a per-profile pref once we get the UI stuff sorted?

That's the plan.

[mattm, 2011-09-19 21:46:43]

LGTM

[commit-bot: I haz the power, 2011-09-21 20:28:43]

Change committed as 102158