Add a killswitch for CSD malware IP match and report feature. Use a new killswitch whitelist URL wh…

Add a killswitch for CSD malware IP match and report feature. Use a new killswitch whitelist URL which is part of the CSD whitelist.

BUG=176647
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203400

[kewang, 2013-05-09 07:16:15]

Unittests will come after if this approach is okay.

[Brian Ryner, 2013-05-09 20:31:36]

I'm not sure I follow why you store the killswitch status in
ClientSideDetectionService, rather than just checking its state from
OnPhishingDetectionDone?

[kewang, 2013-05-09 22:49:13]

On Fri, May 10, 2013 at 4:31 AM,  <bryner@chromium.org> wrote:
> I'm not sure I follow why you store the killswitch status in
> ClientSideDetectionService, rather than just checking its state from
> OnPhishingDetectionDone?
>
Yes, I noticed that host.cc is the one accessing the database_manager_
to get the killswitch status and we can just call it in
OnPhishingDetectionDone(), which is simpler. It's because I found the
host.cc code seems is for handling all the RPC calls and does not
store any csd service related status.

I actually wanted to ask whether it's okay keep the bool variable just
in host.cc. If you think it's okay, happy to change it. :)

Thanks,


> https://codereview.chromium.org/14999008/



--
Ke

[kewang, 2013-05-13 06:15:17]

On 2013/05/09 22:49:13, kewang wrote:
> On Fri, May 10, 2013 at 4:31 AM,  <mailto:bryner@chromium.org> wrote:
> > I'm not sure I follow why you store the killswitch status in
> > ClientSideDetectionService, rather than just checking its state from
> > OnPhishingDetectionDone?
> >
> Yes, I noticed that host.cc is the one accessing the database_manager_
> to get the killswitch status and we can just call it in
> OnPhishingDetectionDone(), which is simpler. It's because I found the
> host.cc code seems is for handling all the RPC calls and does not
> store any csd service related status.
> 
> I actually wanted to ask whether it's okay keep the bool variable just
> in host.cc. If you think it's okay, happy to change it. :)
> 
> Thanks,
> 
> 
> > https://codereview.chromium.org/14999008/
> 

Add the killswitch variable in host.cc/h, and reverted the service files. Please
take another look. Thanks,

> 
> --
> Ke

[mattm, 2013-05-13 20:39:25]

https://codereview.chromium.org/14999008/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/14999008/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/safe_browsing_database.h:168: 
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

https://codereview.chromium.org/14999008/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/safe_browsing_database.h:284: virtual bool
GetMalwareKillSwitch() const {
name the function and the variable more precisely, so that it is not confused
with other SB malware checking

https://codereview.chromium.org/14999008/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/safe_browsing_database.h:285: return
malware_kill_switch_;
If you are wrapping the setting of this member with lookup_lock_, it should be
used here too.

https://codereview.chromium.org/14999008/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/safe_browsing_database.h:286: } OVERRIDE;
OVERRIDE goes before the {}.
Wait, this isn't actually an override, was it supposed to be declared up there
at the uuuuuuuuuuuuuu?

https://codereview.chromium.org/14999008/diff/10001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/14999008/diff/10001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/client_side_detection_host.cc:388: if
(!malware_killswitch_on_) {
why not just do the database_manager_->MalwareKillSwitchOn() call here?

[Brian Ryner, 2013-05-13 23:26:32]

On 2013/05/09 22:49:13, kewang wrote:
> On Fri, May 10, 2013 at 4:31 AM,  <mailto:bryner@chromium.org> wrote:
> > I'm not sure I follow why you store the killswitch status in
> > ClientSideDetectionService, rather than just checking its state from
> > OnPhishingDetectionDone?
> >
> Yes, I noticed that host.cc is the one accessing the database_manager_
> to get the killswitch status and we can just call it in
> OnPhishingDetectionDone(), which is simpler. It's because I found the
> host.cc code seems is for handling all the RPC calls and does not
> store any csd service related status.
> 
> I actually wanted to ask whether it's okay keep the bool variable just
> in host.cc. If you think it's okay, happy to change it. :)

I'm not aware of any intentional separation between _host.cc and the database
manager.  If it's possible for it to access the killswitch state directly this
certainly seems like a simpler approach.

thanks,

[kewang, 2013-05-14 08:21:40]

https://codereview.chromium.org/14999008/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/14999008/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/safe_browsing_database.h:168: 
On 2013/05/13 20:39:25, mattm wrote:
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
yes, this should be place to declare the virtural GetMalwareKillSwitch().

https://codereview.chromium.org/14999008/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/safe_browsing_database.h:284: virtual bool
GetMalwareKillSwitch() const {
On 2013/05/13 20:39:25, mattm wrote:
> name the function and the variable more precisely, so that it is not confused
> with other SB malware checking

Done.

https://codereview.chromium.org/14999008/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/safe_browsing_database.h:285: return
malware_kill_switch_;
On 2013/05/13 20:39:25, mattm wrote:
> If you are wrapping the setting of this member with lookup_lock_, it should be
> used here too.

Done.

https://codereview.chromium.org/14999008/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/safe_browsing_database.h:285: return
malware_kill_switch_;
On 2013/05/13 20:39:25, mattm wrote:
> If you are wrapping the setting of this member with lookup_lock_, it should be
> used here too.

Done.

https://codereview.chromium.org/14999008/diff/10001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/14999008/diff/10001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/client_side_detection_host.cc:388: if
(!malware_killswitch_on_) {
On 2013/05/13 20:39:25, mattm wrote:
> why not just do the database_manager_->MalwareKillSwitchOn() call here?

I saw the above code where it access the database_manager is on IO thread, while
this function is on UI thread. Seems some methods in database_manager not
require IO thread; putting the function call here is definitely better then.

BTW, what's the rule of requiring a function to be on IO or UI thread in
general?

[mattm, 2013-05-15 07:12:37]

https://codereview.chromium.org/14999008/diff/10001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/14999008/diff/10001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/client_side_detection_host.cc:388: if
(!malware_killswitch_on_) {
On 2013/05/14 08:21:41, kewang wrote:
> On 2013/05/13 20:39:25, mattm wrote:
> > why not just do the database_manager_->MalwareKillSwitchOn() call here?
> 
> I saw the above code where it access the database_manager is on IO thread,
while
> this function is on UI thread. Seems some methods in database_manager not
> require IO thread; putting the function call here is definitely better then.

Sorry, I think my above comment was incorrect, this probably should only be
called on the IO thread.

> BTW, what's the rule of requiring a function to be on IO or UI thread in
> general?

In general you need to check the whatever members the method accesses, see what
threads those members are set&read on, and whether they have locking protecting
them or not.  In the case of afeBrowsingDatabaseNew::MalwareIPMatchKillSwitchOn
it does have locking so I initially thought it would be ok to call this on any
thread, but looking at the other methods it needs to go through to get to there
it doesn't look safe actually. (SafeBrowsingDatabaseManager::MalwareKillSwitchOn
specifically, the database_ member looks like it's intended to be used on the IO
thread)

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:280: return
database_->MalwareIPMatchKillSwitchOn();
Oops, didn't notice, I think this needs similar checks like the other methods
above before accessing database_

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.h:386: bool
malware_kill_switch_;
more specific name of this too

[kewang, 2013-05-15 13:45:07]

https://codereview.chromium.org/14999008/diff/10001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/14999008/diff/10001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/client_side_detection_host.cc:388: if
(!malware_killswitch_on_) {
On 2013/05/15 07:12:37, mattm wrote:
> On 2013/05/14 08:21:41, kewang wrote:
> > On 2013/05/13 20:39:25, mattm wrote:
> > > why not just do the database_manager_->MalwareKillSwitchOn() call here?
> > 
> > I saw the above code where it access the database_manager is on IO thread,
> while
> > this function is on UI thread. Seems some methods in database_manager not
> > require IO thread; putting the function call here is definitely better then.
> 
> Sorry, I think my above comment was incorrect, this probably should only be
> called on the IO thread.
> 
> > BTW, what's the rule of requiring a function to be on IO or UI thread in
> > general?
> 
> In general you need to check the whatever members the method accesses, see
what
> threads those members are set&read on, and whether they have locking
protecting
> them or not.  In the case of
afeBrowsingDatabaseNew::MalwareIPMatchKillSwitchOn
> it does have locking so I initially thought it would be ok to call this on any
> thread, but looking at the other methods it needs to go through to get to
there
> it doesn't look safe actually.
(SafeBrowsingDatabaseManager::MalwareKillSwitchOn
> specifically, the database_ member looks like it's intended to be used on the
IO
> thread)
So I kept the code to check switch from database_manager_ at the
CheckCsdWhitelist() in IO thread, set the variable in host_, and do check here.
This looks ok?

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:280: return
database_->MalwareIPMatchKillSwitchOn();
On 2013/05/15 07:12:38, mattm wrote:
> Oops, didn't notice, I think this needs similar checks like the other methods
> above before accessing database_

Done.

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.h:386: bool
malware_kill_switch_;
On 2013/05/15 07:12:38, mattm wrote:
> more specific name of this too

Done.

[mattm, 2013-05-15 23:46:26]

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:280: return
database_->MalwareIPMatchKillSwitchOn();
On 2013/05/15 13:45:07, kewang wrote:
> On 2013/05/15 07:12:38, mattm wrote:
> > Oops, didn't notice, I think this needs similar checks like the other
methods
> > above before accessing database_
> 
> Done.

I think also the enabled and database available checks are needed here right?

[kewang, 2013-05-20 10:23:37]

I found there are two whitelists: csd_whitelist and download_whitelist. I assume
the malware IP killswitch will only be in csd_whitelist. Since they call the
same functions, I added a bool control to check whether set the killswitch
variable. Would like to hear your comment on this approach. Thanks,

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/14999008/diff/15001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:280: return
database_->MalwareIPMatchKillSwitchOn();
On 2013/05/15 23:46:26, mattm wrote:
> On 2013/05/15 13:45:07, kewang wrote:
> > On 2013/05/15 07:12:38, mattm wrote:
> > > Oops, didn't notice, I think this needs similar checks like the other
> methods
> > > above before accessing database_
> > 
> > Done.
> 
> I think also the enabled and database available checks are needed here right?
Yes, added the check. Thanks,

[mattm, 2013-05-21 02:14:09]

https://codereview.chromium.org/14999008/diff/32001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/14999008/diff/32001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.cc:1438: if
(!check_malware_killswitch) {
I tihnk it would be clearer to move the check into the calling function rather
than having this arg for this function to special case it.

Actually I wonder if it would be acceptable to just make
MalwareIPMatchKillSwitchOn itself do a search (just call
ContainsWhitelistedHashes?) rather than having the result stored as a bool
member of the class?

https://codereview.chromium.org/14999008/diff/32001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.cc:1445: if
(std::binary_search(new_whitelist.begin(), new_whitelist.end(),
I think this is a bug, new_whitelist here will actually be the old whitelist
unless the new whitelist also containts the CSD kill switch url ( due to
whitelist->first.swap(new_whitelist) above.)

I guses the unittest should also test the malware kill switch independently of
the csd whitelist killswitch.

[kewang, 2013-05-22 10:14:00]

https://codereview.chromium.org/14999008/diff/32001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/14999008/diff/32001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.cc:1438: if
(!check_malware_killswitch) {
On 2013/05/21 02:14:09, mattm wrote:
> I tihnk it would be clearer to move the check into the calling function rather
> than having this arg for this function to special case it.
> 
> Actually I wonder if it would be acceptable to just make
> MalwareIPMatchKillSwitchOn itself do a search (just call
> ContainsWhitelistedHashes?) rather than having the result stored as a bool
> member of the class?

I think this is a good idea. changed to call ContainsWhitelistedHashes.

https://codereview.chromium.org/14999008/diff/32001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.cc:1445: if
(std::binary_search(new_whitelist.begin(), new_whitelist.end(),
On 2013/05/21 02:14:09, mattm wrote:
> I think this is a bug, new_whitelist here will actually be the old whitelist
> unless the new whitelist also containts the CSD kill switch url ( due to
> whitelist->first.swap(new_whitelist) above.)
> 
> I guses the unittest should also test the malware kill switch independently of
> the csd whitelist killswitch.

removed this part totally.

[mattm, 2013-05-23 06:59:40]

lgtm with nit

https://codereview.chromium.org/14999008/diff/43001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/14999008/diff/43001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.h:357: // and
|malware_kill_switch_|.
update comment

[kewang, 2013-05-23 08:22:33]

https://codereview.chromium.org/14999008/diff/43001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/14999008/diff/43001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.h:357: // and
|malware_kill_switch_|.
On 2013/05/23 06:59:41, mattm wrote:
> update comment

Done.

[mattm, 2013-05-23 19:37:02]

lgtm

[commit-bot: I haz the power, 2013-05-23 19:38:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kewang@google.com/14999008/50001

[commit-bot: I haz the power, 2013-05-23 20:13:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-31 11:56:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kewang@google.com/14999008/100001

[commit-bot: I haz the power, 2013-05-31 13:32:09]

Change committed as 203400