Send malware reports when a user opts-in from the safe browsing interstitial ...

Send malware reports when a user opts-in from the safe browsing interstitial page. In this first iteration, the reports are minimal, but I created a new class because I would like to expand them.

Workflow: A SafeBrowsingBlockingPage is shown. A new MalwareReport object is created. The blocking page goes away, either when the user clicks proceed, go back or closed the tab. We read the user's preference, and if we have a pending report, we pass it on to the SafeBrowsingService so it can send it.

Depends on: http://codereview.chromium.org/4827001/
Also relevant: http://codereview.chromium.org/5102001/
Design doc: https://docs.google.com/document/edit?id=1s-7qMjm23onV68SyqO6yi-xsfFzz4OBSOyHoV3cY8mQ&authkey=CMCF-MID

BUG=60831
TEST=unit_tests,relevant browser_test


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

[lzheng, 2010-11-16 00:18:12]

+ Scott, he is working in this area.

I have a rough first pass. Please see some comments below.

Also, as we have more features and logics in the blocking interstitial page, we
should add a browser test to cover the different actions. Any plan on that?

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/csd.proto (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/csd.proto:34: }
csd.proto was for client side detection. I suggest to create a new file for
safebrowsing report. Also, add a brief description on the purpose of this proto
buf.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/csd.proto:45: optional string verb = 1;
what is verb? You might need to add some descriptions to the field whose usages
are not obvious.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/malware_report.cc (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.cc:26: 
I think we should not show the "report" checkbox when the content is not valid
for reporting (internal sites, https, etc.). Can you confirm that?

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.cc:34:
safe_browsing::ClientMalwareReportRequest::Resource* resource =
Where is the resource released?

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.cc:36: if (!IsPublicUrl(GURL(url)))
Will the resource be leaked here?

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/malware_report.h (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.h:21: #include "base/linked_ptr.h"
header orders: linked_ptr.h should be moved before scoped_ptr.h.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.h:27: class
SafeBrowsingMalwareReport: public base::RefCountedThreadSafe<
there should be a space before ':'. And I would suggest to move ':' and public
.... to the line under SafeBrowsingMalwareReport.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:609: const std::string*
request_data) {
why it is not const std::string& for request_data?

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:725:
mackey_url_prefix_.c_str(),
Why do we use the mackey_url_prefix_ for malwareclientreport? Why can't we use
SafeBrowsingReportUrl? It is a little bit confusing to have two report urls in
this class. I wonder if the two urls could just be one but with different level
of reporting details.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:122: // Users can opt-in on the
SafeBrowsing interstitial to send advanced
Maybe we should say 'detailed' instead of 'advanced'?

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:517: // Q: Reading
from the preference -- should it happen in another thread?
It has to be called on the same thread where pref is created.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:142: 
Instead of the warning, do you mean the blocking page?

[panayiotis, 2010-11-18 22:04:36]

Thanks Lei, I have addressed your comments.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/csd.proto (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/csd.proto:34: }
On 2010/11/16 00:18:12, lzheng wrote:
> csd.proto was for client side detection. I suggest to create a new file for
> safebrowsing report. Also, add a brief description on the purpose of this
proto
> buf.

Done.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/csd.proto:45: optional string verb = 1;
On 2010/11/16 00:18:12, lzheng wrote:
> what is verb? You might need to add some descriptions to the field whose
usages
> are not obvious.

Done.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/malware_report.cc (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.cc:26: 
On 2010/11/16 00:18:12, lzheng wrote:
> I think we should not show the "report" checkbox when the content is not valid
> for reporting (internal sites, https, etc.). Can you confirm that?

Agreed. I added a comment on the relevant CL so we don't forget.
(http://codereview.chromium.org/5102001/) 

That said, if the page url is http we still want to filter out any https
sub-resources from the report, so this is still needed.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.cc:34:
safe_browsing::ClientMalwareReportRequest::Resource* resource =
On 2010/11/16 00:18:12, lzheng wrote:
> Where is the resource released? 

Oops, I got the linked_ptr semantics completely wrong. Fixed, thanks.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.cc:36: if (!IsPublicUrl(GURL(url)))
On 2010/11/16 00:18:12, lzheng wrote:
> Will the resource be leaked here?

Oops, yes. moved the exit condition above.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/malware_report.h (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.h:21: #include "base/linked_ptr.h"
On 2010/11/16 00:18:12, lzheng wrote:
> header orders: linked_ptr.h should be moved before scoped_ptr.h.

Done.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.h:27: class
SafeBrowsingMalwareReport: public base::RefCountedThreadSafe<
On 2010/11/16 00:18:12, lzheng wrote:
> there should be a space before ':'. And I would suggest to move ':' and public
> .... to the line under SafeBrowsingMalwareReport.

Done.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:609: const std::string*
request_data) {
On 2010/11/16 00:18:12, lzheng wrote:
> why it is not const std::string& for request_data?

Done.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:725:
mackey_url_prefix_.c_str(),
these reports will go to https://sb-ssl.google.com/ instead of
http://safebrowsing.clients.google.com/ (no ssl)

It is confusing indeed. One solution is to change the server-side so that we can
handle both "pings" and advanced reports from the same services, but that may
not be trivial. 

Another idea is to rename the old "uma reports" as "pings" here so we can easily
distinguish the two. That would be faster since we don't have to change the
server-side. 


On 2010/11/16 00:18:12, lzheng wrote:
> Why do we use the mackey_url_prefix_ for malwareclientreport? Why can't we use
> SafeBrowsingReportUrl? It is a little bit confusing to have two report urls in
> this class. I wonder if the two urls could just be one but with different
level
> of reporting details.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:122: // Users can opt-in on the
SafeBrowsing interstitial to send advanced
On 2010/11/16 00:18:12, lzheng wrote:
> Maybe we should say 'detailed' instead of 'advanced'?

Done.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:517: // Q: Reading
from the preference -- should it happen in another thread?
On 2010/11/16 00:18:12, lzheng wrote:
> It has to be called on the same thread where pref is created.

thanks.

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

http://codereview.chromium.org/4822002/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:142: 
On 2010/11/16 00:18:12, lzheng wrote:
> Instead of the warning, do you mean the blocking page?

Done.

[Scott Hess - ex-Googler, 2010-11-23 01:59:39]

Just a bunch of comments, I'm going OOT so I'm unlikely to get to LGTM.  [and
I've not digested enough to say for sure what's up :-).]

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/malware_report.cc (right):

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.cc:41: urls_[url] = resource;
Hmm, I'm not entirely clear that this should be linked_ptr versus putting
STLDeleteContainerPairSecondPointers() in the destructor.  Though I see that
people are using linked_ptr in various places these days.

Actually ... AFAICT urls_ only exists so that StartCollection() can call
AddNode().  Is the intention, here, that the map is used in case of duplicates? 
Otherwise, why not just have AddNode() live in an anonymous namespace and accept
report_ as a parameter and add items to it directly?  Then you don't need to
manage two structures at all.

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/report.proto (right):

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/report.proto:84: optional string malware_url = 1;
Can there be more than one?

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:23: #include
"chrome/browser/prefs/pref_service.h"
'e' < 'o'.

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:25: #include
"chrome/browser/safe_browsing/malware_report.h"
'm' < 's'.

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:533:
malware_report_));
Is there any advantage to sending |malware_report_| rather than the serialized
string, here?  [I'm wondering if you could avoid scoped_refptr<> that way.]

[panayiotis, 2010-11-24 22:40:15]

Thanks.

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/malware_report.cc (right):

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_report.cc:41: urls_[url] = resource;
The intention is to be able to (a) eliminate duplicates and (b) find a node fast
so we can modify eg its parent. The protocol buffer's vector-like structure is
not good for that, hence the intermediate map.

In the future, we'll have the renderer send us resource info, possibly with
duplicates.

As for the linked_ptr, I couldn't find any discussion suggesting not to use them
in chromium-dev -- They are supposedly really good for STL containers.

I can switch to STLDelete ... I'd have to manually delete entries that are
duplicate, that's the only reason I'd prefer the linked_ptr.


On 2010/11/23 01:59:39, shess wrote:
> Hmm, I'm not entirely clear that this should be linked_ptr versus putting
> STLDeleteContainerPairSecondPointers() in the destructor.  Though I see that
> people are using linked_ptr in various places these days.
> 
> Actually ... AFAICT urls_ only exists so that StartCollection() can call
> AddNode().  Is the intention, here, that the map is used in case of
duplicates? 
> Otherwise, why not just have AddNode() live in an anonymous namespace and
accept
> report_ as a parameter and add items to it directly?  Then you don't need to
> manage two structures at all.

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/report.proto (right):

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/report.proto:84: optional string malware_url = 1;
On 2010/11/23 01:59:39, shess wrote:
> Can there be more than one?

No, we stop at the first malicious resource and show a warning.

There's one case, where a user proceeds through the warning and then more
malicious resources are loaded. We don't generate a malware report for those
cases though. It's hard to tell, but it's in 

safe_browsing_blocking_page.cc:

  if (unsafe_resources.size() == 1 && ....


We are independently working to simplify safe_browsing_blocking_page actually to
not show additional warnings when a user clicks through the first warning.
That should make at least that file much simpler.  For now I extended the
comment in that file.

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:23: #include
"chrome/browser/prefs/pref_service.h"
On 2010/11/23 01:59:39, shess wrote:
> 'e' < 'o'.

Done.

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:25: #include
"chrome/browser/safe_browsing/malware_report.h"
On 2010/11/23 01:59:39, shess wrote:
> 'm' < 's'.

Done.

http://codereview.chromium.org/4822002/diff/28001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:533:
malware_report_));
On 2010/11/23 01:59:39, shess wrote:
> Is there any advantage to sending |malware_report_| rather than the serialized
> string, here?  [I'm wondering if you could avoid scoped_refptr<> that way.]

As it is now, no, but as soon as the malware_report_ starts doing asynchronous
calls to eg the HistoryService, it needs to outlive the
safe_browsing_blocking_page. 

So at that point I though I'd pass it on to the service, which will wait for it
to be done and send it over.

I'm happy to do what you suggest now, and change it back in the future, if you
prefer. Please let me know.

[panayiotis, 2010-11-30 21:32:34]

Kind ping...

The browser test has been committed (I'm looking to make the phishing tests less
flaky due to a log parser bug
http://code.google.com/p/chromium/issues/detail?id=64792), but we should be more
confident now with a browser test in place.

Thanks

[lzheng, 2010-11-30 23:14:49]

My only concern is that we should make clear the safebrowsinghit report and
malwarereport are different reports.

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:725:
mackey_url_prefix_.c_str(),
If safebrowsing backends has only one https url and http url, maybe we could
just call the url_prefixes as http_url_prefix_ and https_url_prefix? We could
use different one for different purpose?

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:166: // Generates URL for
reporting safe browsing hits.
SafeBrowsingReportUrl and MalwareClientReportUrl are two similar too and
comments here are not very helpful. 
I wonder if there are better names for them? In comments, we should explain
under what circumstance they will be used too.

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.h:179: virtual void
SendMalwareReport(
Call this ReprotMalwareDetails and explain the difference between this one and
"ReportSafeBrowsingHit"? I found it is hard to tell the difference between the
two functions by reading function names and comments.

[panayiotis, 2010-12-01 22:34:52]

Agreed, the naming was confusing. I like "MalwareDetails", I switched to that. 

Please have another look. 

Should we add a browser_test for this feature in this CL or a follow-up? The
existing browser tests pass.

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:725:
mackey_url_prefix_.c_str(),
On 2010/11/30 23:14:49, lzheng wrote:
> If safebrowsing backends has only one https url and http url, maybe we could
> just call the url_prefixes as http_url_prefix_ and https_url_prefix? We could
> use different one for different purpose?

Done.

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:166: // Generates URL for
reporting safe browsing hits.
On 2010/11/30 23:14:49, lzheng wrote:
> SafeBrowsingReportUrl and MalwareClientReportUrl are two similar too and
> comments here are not very helpful. 
> I wonder if there are better names for them? In comments, we should explain
> under what circumstance they will be used too.

Done.

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/4822002/diff/44001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.h:179: virtual void
SendMalwareReport(
On 2010/11/30 23:14:49, lzheng wrote:
> Call this ReprotMalwareDetails and explain the difference between this one and
> "ReportSafeBrowsingHit"? I found it is hard to tell the difference between the
> two functions by reading function names and comments.
> 

Done.

[panayiotis, 2010-12-01 23:37:10]

Also added a browser_test.

[lzheng, 2010-12-03 01:16:57]

LGTM 
with one nit.

http://codereview.chromium.org/4822002/diff/84002/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

http://codereview.chromium.org/4822002/diff/84002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:61: LOG(INFO)
<< "ReportMalwareDetails";
Nit: You might want to get rid of these LOG(INFO)s or change to DVLOG(1) if you
really need them, to avoid unnecessary logs in output.