Adding a new delayed analysis that verify binaries signature.

Adding a new delayed analysis that verifies the signatures of Chrome's binaries


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

[pmonette_google.com, 2014-08-06 21:23:54]

Added mattm for owners review.

Take a look please!

This adds a process-wide analysis for the incident reporting service.

Question : Just because of the WideToUTF8 call, the VerifyBinaryIntegrity()
function has to be implemented in the *_win.cc even if the rest of the function
is platform-agnostic. Any way to fix that elegantly?

[grt (UTC plus 2), 2014-08-07 02:12:46]

Very cool! I've just taken a very cursory look so far. I'm not sure about the
way the registration is done. Maybe SafeBrowsingService should expose a
RegisterAllDelayedAnalysisCallbacks() function that is called during browser
start-up. Then this verifier and others (like Robert's) can add to that function
rather than adding to BrowserProcessImpl.

Have you started on the changes to the incident reporting service to handle this
new incident?

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_service.cc (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service.cc:22:
safe_browsing_service->RegisterDelayedAnalysisCallback(callback);
inline base::Bind here

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_service_win.cc (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:42:
binary_integrity->set_file(base::WideToUTF8(binary_path.value()));
you can use binary_path.AsUTF8Unsafe() here

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:54: const wchar_t*
const critical_binary_name[] = {
static

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:65: for (size_t i =
0; i < arraysize(critical_binary_name); ++i) {
i like the use of the array to build the output, but i don't like that the array
contains different kinds of items (some in the version dir, one that isn't) for
which a single sprintf is used. i think it'd be cleaner to have something like:

  static const base::FilePath::CharType* const kUnversionedFiles[] = {
    FILE_PATH_LITERAL("chrome.exe"),
  };
  static const base::FilePath::CharType* const kVersionedFIles[] = {
    FILE_PATH_LITERAL("chrome.dll"),
    FILE_PATH_LITERAL("chrome_elf.dll"),
  };

and then process these separately:

  critical_binary.reserve(arraysize(kUnversionedFIles) +
arraysize(kVersionedFIles));
  // append each unversioned file to chrome_exe_dir and push_back
  base::FilePath version_dir(chrome_exe_dir.Append(chrome_version));
  // append each versioned file to version_dir and push_back

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:67:
base::StringPrintf(critical_binary_name[i], GetChromeVersion().c_str());
remove GetChromeVersion altogether and either use chrome::VersionInfo outside of
the loop to get the version string, or #include "version.h" and use the macro
CHROME_VERSION_STRING, which expands to a string literal.

https://codereview.chromium.org/444123002/diff/40001/chrome/common/safe_brows...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/common/safe_brows...
chrome/common/safe_browsing/csd.proto:355: optional string file = 1;
if this is only the name of the file rather than the path, please use
"file_basename", which matches the name of a similar field in
ClientDownloadRequest.

[robertshield, 2014-08-07 14:57:24]

Looks awesome, just minor nits from me.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_service.h (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service.h:19: // Registers a
process-wide analysis to the incident reporting service that
s/to/with/

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service.h:20: // verify the
signature of the most critical binary used by Chrome. It will
s/verify/will verity/
s/binary/binaries/

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service.h:29: base::string16
GetChromeVersion();
I think there's something that will do this in
src/chrome/common/chrome_version_info.h

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service.h:31: // Returns a vector
containing the paths to all the binary to verify.
s/binary/binaries/

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_service_win.cc (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:23: // Hold to the
same signature info instance until we use it in a report.
s/Hold/Hold on/

[mattm, 2014-08-07 20:16:01]

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:1047:
safe_browsing::RegisterBinaryIntegrityAnalysis();
Agree about avoiding adding more stuff here, maybe it can just be put in
SafeBrowsingService::Initialize, or the CreateSafeBrowsingService function?

(may also need some #if defined(FULL_SAFE_BROWSING) check depending where it
goes)

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_service_win.cc (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:25: new
ClientDownloadRequest_SignatureInfo());
I think a new SignatureInfo needs to be created for each call to CheckSignature,
or it needs to be cleared between calls, otherwise it looks like data from
multiple calls could get mashed together.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:63:
std::vector<base::FilePath> critical_binary;
critical_binaries?

[pmonette_google.com, 2014-08-07 21:29:29]

Added the modification to the incident reporting service to support the
BINARY_INTEGRITY incident type.

I changed my file names to binary_integrity_analyzer because "service" felt
wrong for a one time analysis. This kinda screwed up the version comparisons so
sorry about that!

There is one question for greg about the protobuf in the comments.

Take another look please.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:1047:
safe_browsing::RegisterBinaryIntegrityAnalysis();
On 2014/08/07 20:16:01, mattm wrote:
> Agree about avoiding adding more stuff here, maybe it can just be put in
> SafeBrowsingService::Initialize, or the CreateSafeBrowsingService function?
> 
> (may also need some #if defined(FULL_SAFE_BROWSING) check depending where it
> goes)

Okay I've added a function to register all the delayed analysis in
safe_browsing_service.cc. Let me know what you think.

I don't think the FULL_SAFE_BROWSING check is needed yet. There has been some
discussion about adding more thorough checks for full safe-browsing enabled
profiles sometime in the future though.

I also think that we thought about reporting more information in the reports if
full safe-browsing is enabled but if this ever happens, the incident reporting
service will take care of that.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_service.cc (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service.cc:22:
safe_browsing_service->RegisterDelayedAnalysisCallback(callback);
On 2014/08/07 02:12:46, grt (no reviews Aug 9 - 27) wrote:
> inline base::Bind here

Done.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_service.h (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service.h:19: // Registers a
process-wide analysis to the incident reporting service that
On 2014/08/07 14:57:24, robertshield wrote:
> s/to/with/

Done.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service.h:20: // verify the
signature of the most critical binary used by Chrome. It will
On 2014/08/07 14:57:24, robertshield wrote:
> s/verify/will verity/
> s/binary/binaries/

Done.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service.h:29: base::string16
GetChromeVersion();
On 2014/08/07 14:57:24, robertshield wrote:
> I think there's something that will do this in
> src/chrome/common/chrome_version_info.h

Im using the CHROME_VERSION_STRING macro in "version.h" now.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service.h:31: // Returns a vector
containing the paths to all the binary to verify.
On 2014/08/07 14:57:24, robertshield wrote:
> s/binary/binaries/

Done.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_service_win.cc (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:23: // Hold to the
same signature info instance until we use it in a report.
On 2014/08/07 14:57:24, robertshield wrote:
> s/Hold/Hold on/

Done.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:25: new
ClientDownloadRequest_SignatureInfo());
On 2014/08/07 20:16:01, mattm wrote:
> I think a new SignatureInfo needs to be created for each call to
CheckSignature,
> or it needs to be cleared between calls, otherwise it looks like data from
> multiple calls could get mashed together.

That is true. Done.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:42:
binary_integrity->set_file(base::WideToUTF8(binary_path.value()));
On 2014/08/07 02:12:46, grt (no reviews Aug 9 - 27) wrote:
> you can use binary_path.AsUTF8Unsafe() here

Done.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:54: const wchar_t*
const critical_binary_name[] = {
On 2014/08/07 02:12:46, grt (no reviews Aug 9 - 27) wrote:
> static

Done.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:63:
std::vector<base::FilePath> critical_binary;
On 2014/08/07 20:16:01, mattm wrote:
> critical_binaries?

Done.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:65: for (size_t i =
0; i < arraysize(critical_binary_name); ++i) {
On 2014/08/07 02:12:46, grt (no reviews Aug 9 - 27) wrote:
> i like the use of the array to build the output, but i don't like that the
array
> contains different kinds of items (some in the version dir, one that isn't)
for
> which a single sprintf is used. i think it'd be cleaner to have something
like:
> 
>   static const base::FilePath::CharType* const kUnversionedFiles[] = {
>     FILE_PATH_LITERAL("chrome.exe"),
>   };
>   static const base::FilePath::CharType* const kVersionedFIles[] = {
>     FILE_PATH_LITERAL("chrome.dll"),
>     FILE_PATH_LITERAL("chrome_elf.dll"),
>   };
> 
> and then process these separately:
> 
>   critical_binary.reserve(arraysize(kUnversionedFIles) +
> arraysize(kVersionedFIles));
>   // append each unversioned file to chrome_exe_dir and push_back
>   base::FilePath version_dir(chrome_exe_dir.Append(chrome_version));
>   // append each versioned file to version_dir and push_back

Done.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_service_win.cc:67:
base::StringPrintf(critical_binary_name[i], GetChromeVersion().c_str());
On 2014/08/07 02:12:46, grt (no reviews Aug 9 - 27) wrote:
> remove GetChromeVersion altogether and either use chrome::VersionInfo outside
of
> the loop to get the version string, or #include "version.h" and use the macro
> CHROME_VERSION_STRING, which expands to a string literal.

Went with the second suggestion.

https://codereview.chromium.org/444123002/diff/40001/chrome/common/safe_brows...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/common/safe_brows...
chrome/common/safe_browsing/csd.proto:355: optional string file = 1;
On 2014/08/07 02:12:46, grt (no reviews Aug 9 - 27) wrote:
> if this is only the name of the file rather than the path, please use
> "file_basename", which matches the name of a similar field in
> ClientDownloadRequest.

In its current form, I am serializing the whole path to the file. But after
thinking about it again maybe just the basename is sufficient. Is there any
advantages to having the whole path instead of only the base name in your
opinion?

(Maybe finding where chrome is installed or identifying bugs but these are very
weak points)

[mattm, 2014-08-07 22:18:30]

https://codereview.chromium.org/444123002/diff/40001/chrome/common/safe_brows...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/common/safe_brows...
chrome/common/safe_browsing/csd.proto:355: optional string file = 1;
On 2014/08/07 21:29:29, pmonette wrote:
> On 2014/08/07 02:12:46, grt (no reviews Aug 9 - 27) wrote:
> > if this is only the name of the file rather than the path, please use
> > "file_basename", which matches the name of a similar field in
> > ClientDownloadRequest.
> 
> In its current form, I am serializing the whole path to the file. But after
> thinking about it again maybe just the basename is sufficient. Is there any
> advantages to having the whole path instead of only the base name in your
> opinion?
> 
> (Maybe finding where chrome is installed or identifying bugs but these are
very
> weak points)

If it does include the full path, should it use
PathSanitizer::StripHomeDirectory?

[mattm, 2014-08-07 23:08:47]

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:101:
COMPILE_ASSERT(TRACKED_PREFERENCE + 2 == NUM_INCIDENT_TYPES,
change to BINARY_INTEGRITY + 1?

https://codereview.chromium.org/444123002/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2715:
'browser/safe_browsing/binary_integrity_incident_handlers.h',
Think we're getting enough files to make a subdirectory for the incident
reporting stuff? (in a separate CL)

[grt (UTC plus 2), 2014-08-08 02:23:06]

looking really nice.

+asvitkine: please see my question aimed at you in the comments below.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:1047:
safe_browsing::RegisterBinaryIntegrityAnalysis();
On 2014/08/07 21:29:28, pmonette wrote:
> On 2014/08/07 20:16:01, mattm wrote:
> > Agree about avoiding adding more stuff here, maybe it can just be put in
> > SafeBrowsingService::Initialize, or the CreateSafeBrowsingService function?
> > 
> > (may also need some #if defined(FULL_SAFE_BROWSING) check depending where it
> > goes)
> 
> Okay I've added a function to register all the delayed analysis in
> safe_browsing_service.cc. Let me know what you think.
> 
> I don't think the FULL_SAFE_BROWSING check is needed yet. There has been some
> discussion about adding more thorough checks for full safe-browsing enabled
> profiles sometime in the future though.
> 
> I also think that we thought about reporting more information in the reports
if
> full safe-browsing is enabled but if this ever happens, the incident reporting
> service will take care of that.

I think you're thinking of extended safe browsing rather than
FULL_SAFE_BROWSING. The former is a user-controlled setting in the browser,
while the latter is a build-time switch that turns on/off various safe browsing
features. The incident reporting service and all related code is
FULL_SAFE_BROWSING only.

https://codereview.chromium.org/444123002/diff/40001/chrome/common/safe_brows...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/common/safe_brows...
chrome/common/safe_browsing/csd.proto:355: optional string file = 1;
On 2014/08/07 21:29:29, pmonette wrote:
> On 2014/08/07 02:12:46, grt (no reviews Aug 9 - 27) wrote:
> > if this is only the name of the file rather than the path, please use
> > "file_basename", which matches the name of a similar field in
> > ClientDownloadRequest.
> 
> In its current form, I am serializing the whole path to the file. But after
> thinking about it again maybe just the basename is sufficient. Is there any
> advantages to having the whole path instead of only the base name in your
> opinion?
> 
> (Maybe finding where chrome is installed or identifying bugs but these are
very
> weak points)

Let's go with the basename for now. If we find that knowing where Chrome is
installed (sanitized, as Matt points out) useful, then we can add that as its
own thing in the environmental info.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_analyzer.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:10: #include
"base/file_version_info.h"
unused

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:11: #include
"base/path_service.h"
unused

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:41:
binary_feature_extractor->CheckSignature(binary_path, signature_info.get());
please add some use of UMA_HISTOGRAM_TIMES so we can see how long this takes for
each binary. it would be best to have distinct histograms for the different
binaries. this probably means using base::Histogram::FactoryTimeGet with a name
along the lines of "SBIRS.VerifyBinaryIntegrity.N" where N is the index into the
array. maybe there's an even better way. asvitkine will know.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:58: 
you need this for non-win platform, no?
#if !defined(OS_WIN)
std::vector<base::FilePath> GetCriticalBinariesPath() {
  return std::vector<base::FilePath>();
}
#endif  // !defined(OS_WIN)

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_analyzer.h (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer.h:10: #include
"base/strings/string16.h"
unused

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_analyzer_win.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer_win.cc:10: #include
"base/strings/stringprintf.h"
unused

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc
(right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc:15:
#include "testing/gmock/include/gmock/gmock.h"
unused

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc:29: bool
g_callback_called = false;
please avoid the global:
- make this an instance variable of BinaryIntegrityAnalyzerWinTest (e.g.,
callback_called_)
- make MockAddIncidentCallback a member of that class (i prefer OnAddIncident,
but that's just my thing)
- bind the method with
base::Bind(&BinaryIntegrityAnalyzerWinTest::OnAddIncident,
base::Unretained(this))

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc:111:
ASSERT_TRUE(g_callback_called);
could you test that the data made it into the proto? OnAddIncident could take
ownership of the incident into the test fixture, and then you could verify the
paths, or that the signature info is present or something (no need to delve into
the signature info itself since that is tested elsewhere.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_incident_handlers.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_incident_handlers.cc:17: return
incident_data.binary_integrity().file();
this needs to be consistent across chrome updates, which won't be the case if
file() returns the full (or even sanitized) paths of items in the version
directory. if file() was really file_basename(), then this would be okay.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:101:
COMPILE_ASSERT(TRACKED_PREFERENCE + 2 == NUM_INCIDENT_TYPES,
On 2014/08/07 23:08:47, mattm wrote:
> change to BINARY_INTEGRITY + 1? 

yes, please. below as well.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_service.cc:269: // Register all the
delayed analysis to the incident reporting service.
may as well go ahead and wrap this (and the function definition) in #if
defined(FULL_SAFE_BROWSING).

https://codereview.chromium.org/444123002/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2715:
'browser/safe_browsing/binary_integrity_incident_handlers.h',
On 2014/08/07 23:08:47, mattm wrote:
> Think we're getting enough files to make a subdirectory for the incident
> reporting stuff? (in a separate CL)

sgtm.

[grt (UTC plus 2), 2014-08-08 02:26:08]

oh, in cl description: "verify binaries signature" -> "verifies the signatures
of Chrome's binaries"

also, please file a new bug in crbug for this (so that the BUG= line has
meaning). a simple summary like "Generate an incident for Chrome binaries with
invalid signatures." mention safe browsing in the description somewhere and use
the Cr-UI-Browser-SafeBrowsing label. thanks.

[pmonette_google.com, 2014-08-08 15:05:43]

Take another look please.

grt: I'm still waiting on asvitkine's input for the UMA_HISTOGRAM_TIMES.

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:1047:
safe_browsing::RegisterBinaryIntegrityAnalysis();
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> On 2014/08/07 21:29:28, pmonette wrote:
> > On 2014/08/07 20:16:01, mattm wrote:
> > > Agree about avoiding adding more stuff here, maybe it can just be put in
> > > SafeBrowsingService::Initialize, or the CreateSafeBrowsingService
function?
> > > 
> > > (may also need some #if defined(FULL_SAFE_BROWSING) check depending where
it
> > > goes)
> > 
> > Okay I've added a function to register all the delayed analysis in
> > safe_browsing_service.cc. Let me know what you think.
> > 
> > I don't think the FULL_SAFE_BROWSING check is needed yet. There has been
some
> > discussion about adding more thorough checks for full safe-browsing enabled
> > profiles sometime in the future though.
> > 
> > I also think that we thought about reporting more information in the reports
> if
> > full safe-browsing is enabled but if this ever happens, the incident
reporting
> > service will take care of that.
> 
> I think you're thinking of extended safe browsing rather than
> FULL_SAFE_BROWSING. The former is a user-controlled setting in the browser,
> while the latter is a build-time switch that turns on/off various safe
browsing
> features. The incident reporting service and all related code is
> FULL_SAFE_BROWSING only.

Oh this is confusing. Thanks!

https://codereview.chromium.org/444123002/diff/40001/chrome/common/safe_brows...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/444123002/diff/40001/chrome/common/safe_brows...
chrome/common/safe_browsing/csd.proto:355: optional string file = 1;
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> On 2014/08/07 21:29:29, pmonette wrote:
> > On 2014/08/07 02:12:46, grt (no reviews Aug 9 - 27) wrote:
> > > if this is only the name of the file rather than the path, please use
> > > "file_basename", which matches the name of a similar field in
> > > ClientDownloadRequest.
> > 
> > In its current form, I am serializing the whole path to the file. But after
> > thinking about it again maybe just the basename is sufficient. Is there any
> > advantages to having the whole path instead of only the base name in your
> > opinion?
> > 
> > (Maybe finding where chrome is installed or identifying bugs but these are
> very
> > weak points)
> 
> Let's go with the basename for now. If we find that knowing where Chrome is
> installed (sanitized, as Matt points out) useful, then we can add that as its
> own thing in the environmental info.

Done.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_analyzer.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:10: #include
"base/file_version_info.h"
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> unused

Removed.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:11: #include
"base/path_service.h"
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> unused

Removed.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:58: 
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> you need this for non-win platform, no?
> #if !defined(OS_WIN)
> std::vector<base::FilePath> GetCriticalBinariesPath() {
>   return std::vector<base::FilePath>();
> }
> #endif  // !defined(OS_WIN)

Done.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_analyzer.h (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer.h:10: #include
"base/strings/string16.h"
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> unused

Removed.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_analyzer_win.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer_win.cc:10: #include
"base/strings/stringprintf.h"
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> unused

Removed.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc
(right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc:15:
#include "testing/gmock/include/gmock/gmock.h"
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> unused

I'm actually using it for the ASSERT_THAT macro to compare the 2 vectors in the
GetCriticalBinariesPath test.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc:29: bool
g_callback_called = false;
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> please avoid the global:
> - make this an instance variable of BinaryIntegrityAnalyzerWinTest (e.g.,
> callback_called_)
> - make MockAddIncidentCallback a member of that class (i prefer OnAddIncident,
> but that's just my thing)
> - bind the method with
> base::Bind(&BinaryIntegrityAnalyzerWinTest::OnAddIncident,
> base::Unretained(this))

Done.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc:111:
ASSERT_TRUE(g_callback_called);
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> could you test that the data made it into the proto? OnAddIncident could take
> ownership of the incident into the test fixture, and then you could verify the
> paths, or that the signature info is present or something (no need to delve
into
> the signature info itself since that is tested elsewhere.

Done.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_incident_handlers.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_incident_handlers.cc:17: return
incident_data.binary_integrity().file();
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> this needs to be consistent across chrome updates, which won't be the case if
> file() returns the full (or even sanitized) paths of items in the version
> directory. if file() was really file_basename(), then this would be okay.

Okay makes sense. It's now file_basename anyway!

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:101:
COMPILE_ASSERT(TRACKED_PREFERENCE + 2 == NUM_INCIDENT_TYPES,
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> On 2014/08/07 23:08:47, mattm wrote:
> > change to BINARY_INTEGRITY + 1? 
> 
> yes, please. below as well.

Done.

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_service.cc:269: // Register all the
delayed analysis to the incident reporting service.
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> may as well go ahead and wrap this (and the function definition) in #if
> defined(FULL_SAFE_BROWSING).

Done.

https://codereview.chromium.org/444123002/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2715:
'browser/safe_browsing/binary_integrity_incident_handlers.h',
On 2014/08/08 02:23:06, grt (no reviews Aug 9 - 27) wrote:
> On 2014/08/07 23:08:47, mattm wrote:
> > Think we're getting enough files to make a subdirectory for the incident
> > reporting stuff? (in a separate CL)
> 
> sgtm.

I can do that.

[Alexei Svitkine (slow), 2014-08-08 15:14:59]

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/binary_integrity_analyzer.cc (right):

https://codereview.chromium.org/444123002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:41:
binary_feature_extractor->CheckSignature(binary_path, signature_info.get());
On 2014/08/08 02:23:05, grt (no reviews Aug 9 - 27) wrote:
> please add some use of UMA_HISTOGRAM_TIMES so we can see how long this takes
for
> each binary. it would be best to have distinct histograms for the different
> binaries. this probably means using base::Histogram::FactoryTimeGet with a
name
> along the lines of "SBIRS.VerifyBinaryIntegrity.N" where N is the index into
the
> array. maybe there's an even better way. asvitkine will know.

Yeah, Histogram::FactoryTimeGet() is the right way to do something like this
(since a macro can't be used with variable names to caching) - you can get the
params from the expansion of one of the macros. In xml file, you can use a
histograms_suffixes element to avoid duplication.

[grt (UTC plus 2), 2014-08-08 15:29:06]

lgtm with downcase. asvitkine's suggestion for the histogram sounds good to me.
he's your captain for that. :-)

https://codereview.chromium.org/444123002/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/binary_integrity_analyzer.cc (right):

https://codereview.chromium.org/444123002/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:48:
binary_integrity->set_file_basename(binary_path.BaseName()
hmm, perhaps downcase this so that changes in case don't mess up the handler's
key generation.

[grt (UTC plus 2), 2014-08-08 18:08:01]

https://codereview.chromium.org/444123002/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/binary_integrity_analyzer_win.cc (right):

https://codereview.chromium.org/444123002/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/binary_integrity_analyzer_win.cc:19:
FILE_PATH_LITERAL("chrome.dll"), FILE_PATH_LITERAL("chrome_elf.dll"),
please add "chrome_child.dll" to this list

[robertshield, 2014-08-08 18:16:50]

I think you are about to get bitten by the thing Kristina got bitten by, you
will need to update unit_tests.isolate with the checked-in DLL:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/unit_tests....

[pmonette_google.com, 2014-08-08 18:57:00]

Adding UMA stat for the time it takes to verify each binary and fixing trybot
errors.

Please take another look.

https://codereview.chromium.org/444123002/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/binary_integrity_analyzer.cc (right):

https://codereview.chromium.org/444123002/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:48:
binary_integrity->set_file_basename(binary_path.BaseName()
On 2014/08/08 15:29:05, grt (no reviews Aug 9 - 27) wrote:
> hmm, perhaps downcase this so that changes in case don't mess up the handler's
> key generation.

Done.

https://codereview.chromium.org/444123002/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/binary_integrity_analyzer_win.cc (right):

https://codereview.chromium.org/444123002/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/binary_integrity_analyzer_win.cc:19:
FILE_PATH_LITERAL("chrome.dll"), FILE_PATH_LITERAL("chrome_elf.dll"),
On 2014/08/08 18:08:01, grt (no reviews Aug 9 - 27) wrote:
> please add "chrome_child.dll" to this list

Done.

[grt (UTC plus 2), 2014-08-08 20:37:25]

https://codereview.chromium.org/444123002/diff/280001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/binary_integrity_analyzer.cc (right):

https://codereview.chromium.org/444123002/diff/280001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:28: static const char*
kHistogramName = "SBIRS.VerifyBinaryIntegrity.";
nit: static const char kHistogramName[] = "..."

https://codereview.chromium.org/444123002/diff/280001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:77:
binary_integrity->set_file_basename(base::StringToLowerASCII(
ASCII makes me nervous here. is it the case that these basenames will always be
the exact strings from GetCriticalBinariesPath? if so, maybe i sent you down the
wrong path and this downcase isn't needed. if you think there's value in the
explicit downcase, please use something like
WideToUTF8(base::i18n::ToLower(binary_path.BaseName().value())).

https://codereview.chromium.org/444123002/diff/280001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc
(right):

https://codereview.chromium.org/444123002/diff/280001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc:130:
"chrome_elf.dll");
expected value comes first for the ASSERT_EQ macro

[Alexei Svitkine (slow), 2014-08-11 13:51:54]

histograms.xml lgtm

[mattm, 2014-08-11 23:29:15]

lgtm

[pmonette_google.com, 2014-08-13 15:31:31]

https://codereview.chromium.org/444123002/diff/280001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/binary_integrity_analyzer.cc (right):

https://codereview.chromium.org/444123002/diff/280001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:28: static const char*
kHistogramName = "SBIRS.VerifyBinaryIntegrity.";
On 2014/08/08 20:37:24, grt (no reviews Aug 9 - 27) wrote:
> nit: static const char kHistogramName[] = "..."

Done.

https://codereview.chromium.org/444123002/diff/280001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/binary_integrity_analyzer.cc:77:
binary_integrity->set_file_basename(base::StringToLowerASCII(
On 2014/08/08 20:37:24, grt (no reviews Aug 9 - 27) wrote:
> ASCII makes me nervous here. is it the case that these basenames will always
be
> the exact strings from GetCriticalBinariesPath? if so, maybe i sent you down
the
> wrong path and this downcase isn't needed. if you think there's value in the
> explicit downcase, please use something like
> WideToUTF8(base::i18n::ToLower(binary_path.BaseName().value())).

Removed the case conversion.

https://codereview.chromium.org/444123002/diff/280001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc
(right):

https://codereview.chromium.org/444123002/diff/280001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/binary_integrity_analyzer_win_unittest.cc:130:
"chrome_elf.dll");
On 2014/08/08 20:37:24, grt (no reviews Aug 9 - 27) wrote:
> expected value comes first for the ASSERT_EQ macro

Done.

[pmonette_google.com, 2014-08-14 15:17:48]

The CQ bit was checked by pmonette@google.com

[commit-bot: I haz the power, 2014-08-14 15:22:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pmonette@google.com/444123002/480001

[commit-bot: I haz the power, 2014-08-14 19:49:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-14 19:57:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-14 19:57:56]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pmonette_google.com, 2014-08-14 21:08:39]

csharp@ : Need owners lgtm for chrome/unit_tests.isolate.

[csharp, 2014-08-14 21:11:17]

lgtm

[pmonette_google.com, 2014-08-14 21:12:30]

The CQ bit was checked by pmonette@google.com

[commit-bot: I haz the power, 2014-08-14 21:16:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pmonette@google.com/444123002/480001

[commit-bot: I haz the power, 2014-08-14 21:29:06]

Committed patchset #12 (480001) as 289703