Implement anonymous, opt-in, collection of OS X binary integrity incidents.

Implement anonymous, opt-in, collection of OS X binary integrity incidents.

This change implements anonymous collection of data about binary integrity
incidents on OS X. A binary integrity incident means that Chrome's application
bundle was modified on disk by some unauthorized source. This change collects
information about modified files. The checks are enabled on a sampling of user's
browsers and run at browser startup if enabled.

BUG=530314
Committed: https://crrev.com/2b08c52ad7d79322756e93b7fb4dfb0a00f402e4
Cr-Commit-Position: refs/heads/master@{#357469}

[Greg K, 2015-09-28 21:14:00]

Patchset #1 (id:1) has been deleted

[Greg K, 2015-10-01 17:53:24]

Patchset #1 (id:20001) has been deleted

[Greg K, 2015-10-01 17:53:33]

Patchset #1 (id:40001) has been deleted

[Greg K, 2015-10-01 17:53:40]

Patchset #1 (id:60001) has been deleted

[Greg K, 2015-10-01 17:53:46]

Patchset #1 (id:80001) has been deleted

[Greg K, 2015-10-01 17:53:55]

Patchset #1 (id:100001) has been deleted

[Greg K, 2015-10-01 17:54:03]

Patchset #1 (id:120001) has been deleted

[Greg K, 2015-10-02 20:49:08]

kerrnel@chromium.org changed reviewers:
+ mark@chromium.org, rsesek@chromium.org

[Greg K, 2015-10-02 20:49:09]

Mark and Robert, please review as you see fit.

In the test/data/safe_browsing/mach_o folder, the .app bundles are all generated
from the Makefile and the ResourceRules, so those are probably the most
interesting to look at, instead of trying to read all the CodeResources xml
files.

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc
(right):

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc:32:
std::string(kHistogramName) + base::IntToString(file_index),
This looks like a merge gone bad. It should be SizeTToString.

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:38:
// Returns a vector contains the paths to the binaries to verify,
"a vector of pairs, each of which contains ..." 

Typo: contains -> containing

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:10:

Why the blank space?

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:23:
/* Get the path to the main executable */
Wrong comment style.

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc
(right):

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:32:
// Helper function to erase the content of a binary to make sure the signature
s/erase/corrupt

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.h:36: // The requirement
string must be a valid "Code Signing Requirement Language"
Probably better to just say what it does:

, which describes the identity of the signer.

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.h:63: // It is useful to
store this as a class member variable because the
Probably don't need to explain why we store it as a class member.

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:24: // a list of the
extended attributes slots used in Security.framework currently.
current used in*

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:84:
sub_incident->set_file_basename(path.BaseName().AsUTF8Unsafe());
Why UTF8Unsafe? Figure that out.

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:90: if
(!bfe->ExtractImageFeatures(
Does returning false mean no embedded signature was found? I think it means it's
not mach-o.

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:97: ssize_t rc =
getxattr(path.value().c_str(), attr, NULL, 0, 0, 0);
no need for the variable rc?

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:146: // Check all
architectures?
already done

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
File chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc (right):

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:76:
std::vector<uint8_t> buf(10);  // 10 bytes is a nice arbitrary value
use a period, and maybe do better than arbitrary?

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:104: // This is
a slightly more interesting test which checks the signer against
"This test checks ... "

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:121: // Now run
this test against /bin/ls, but with a designated requirement that
against /bin/ls?

https://chromiumcodereview.appspot.com/1363613004/diff/140001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:207: //
executable modified. The reporting will stop when it sees the main
shorten the comment

[Greg K, 2015-10-02 20:49:52]

Patchset #1 (id:140001) has been deleted

[grt (UTC plus 2), 2015-10-03 20:03:00]

drive-by comment or two and a question

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:40:
std::vector<std::pair<base::FilePath, std::string>>
#include <string> for std::string
#include <utility> for std::pair

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:63:
for (size_t i = 0; i < bundles_and_requirements.size(); i++) {
  for (const auto& p : bundles_and_requirements) {

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:68:
VerifyBinaryIntegrityHelper(incident_receiver.Pass(), path, requirement);
don't Pass() ownership of the receiver to the helper func here since it will be
destroyed after the first call.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
File chrome/browser/safe_browsing/incident_reporting/incident.h (right):

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/incident.h:30: OSX_INCIDENT = 7,
why not generalize BINARY_INTEGRITY so that it's suitable for both platforms?

[Mark Mentovai, 2015-10-05 15:02:12]

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:43:
// alternatively signed code.
What does “alternatively signed” mean?

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
File
chrome/browser/safe_browsing/incident_reporting/osx_binary_integrity_incident.h
(right):

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/osx_binary_integrity_incident.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Is this file supposed to only be built on Mac? Name these files
binary_integrity_incident_mac.* (and name the class similarly to avoid
confusion). That way, they’ll automatically be built on Mac but not on other
platforms. See build/filename_rules.gypi.

But as grt suggested, it seems that this isn’t strictly needed, and this can
just use BinaryIntegrityIncident.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.h:10: #if
!defined(OS_MACOSX)
Not necessary. The _mac name makes sure that no other platform will see
signature_evaluator_mac.mm. (So does the .mm, in fact.) Because of that, the
#include above is also not necessary.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.h:15: #include <string>
Blank line between C and C++ system headers.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.h:53: // This is the path
to the code object on disk.
“This is” on almost everything here (including Initialize()), kinda redundant.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.h:69: void
report_altered_files(
1. Naming: ReportAlteredFiles().
http://google.github.io/styleguide/cppguide.html#Function_Names
2. Ordering: methods come before data.
http://google.github.io/styleguide/cppguide.html#Declaration_Order

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.h:70: NSObject* detail,
NSObject* or id?

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.h:72: };
DISALLOW_COPY_AND_ASSIGN

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:22: namespace {
Blank line after this.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:25: // currently.
Provide a reference to where in Security.framework they’re defined, so that we
can check against changes in the future.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:26: const char* xattrs[]
= {
Make this more const.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:27:
"com.apple.cs.CodeDirectory",    "com.apple.cs.CodeSignature",
One per line. Especially when everything looks so similar, that helps spot that
there are in fact six elements and helps distinguish one from another.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:34: bool
getPathFromNSObject(NSObject* obj, std::string* output) {
id for a generic Objective-C object?

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:34: bool
getPathFromNSObject(NSObject* obj, std::string* output) {
Naming: GetPath, capital G.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:35: if ([obj
isKindOfClass:[NSString class]]) {
Seems like a job for ObjCCast.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:73: if ([detail
isKindOfClass:[NSArray class]]) {
ObjCCast

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:88: // call has to
change. `ExtractImageFeatures` maps the file, which will
What if it maps it without making the pages executable?

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:97: ssize_t rc =
getxattr(path.value().c_str(), attr, NULL, 0, 0, 0);
nullptr

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:114:
base::scoped_nsobject<NSString> url_str([[NSString alloc]
SysUTF8ToNSString()? Elsewhere too.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:158: if (errors) {
Is there a more specific return value from
SecStaticCodeCheckValidityWithErrors() to use to signal that the signature was
violated rather than just some generic thing like a read permission error?

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:162:
report_altered_files(detail, incident);
{} since the condition got long and wrapped weirdly.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:163: if (id detail =
[info
Loop over a list of interesting keys?

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:168: return err ==
errSecSuccess;
You can get rid of a level of indentation for the “meat” here by making
SecStaticCodeCheckValidityWithErrors() == errSecSuccess into an early “return
true” above, and having the rest of the function dedicated to dealing with !=
errSecSuccess.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
File chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc (right):

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:8: #include
<string>
Separate C from C++ system headers.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:25: const char*
xattrs[] = {
It’s unfortunate that this isn’t sharing with the identical list in the other
file.

https://chromiumcodereview.appspot.com/1363613004/diff/160001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:30: }
// namespace

[Robert Sesek, 2015-10-05 22:19:08]

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:42:
// This is a helper stub to allow us to test this mechanism with
Rather than exposing the helper directly, you should create a function
VerifyBinaryIntegrityForTesting() that calls the internal version. *ForTesting
is forbidden by presubmit from being called in non-test files.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc
(right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:35:
FILE* file = base::OpenFile(file_path, "r+");
base/files/file.h is probably a better interface to use for this, since it's got
RAII semantics.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:63:
CHECK(PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir_));
Don't CHECK in tests. Use ASSERT_TRUE instead.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:67:
CHECK(temp_dir_.CreateUniqueTempDir());
Same, no CHECK.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:72:
signed_bundle_path = signed_bundle_path.Append(kBundleBase);
It's typical to chain the Append calls:

  base::FilePath signed_bundle_path =
base::FilePath(test_data_dir_).Append(kBundleBase);

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:75:
CHECK(base::CopyDirectory(signed_bundle_path, copied_bundle_path, true));
No CHECK.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:32: class
MacSignatureEvaluator {
Needs a class-levelc omment.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc (right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:45:
CHECK(temp_dir_.CreateUniqueTempDir());
No CHECK

https://codereview.chromium.org/1363613004/diff/160001/chrome/test/data/safe_...
File chrome/test/data/safe_browsing/mach_o/Makefile (right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/test/data/safe_...
chrome/test/data/safe_browsing/mach_o/Makefile:69: codesign -f -s
$(KEYCHAIN_IDENTITY) --keychain $(PWD)/codesign.keychain \
Keep 80 columns. Here and throughout.

https://codereview.chromium.org/1363613004/diff/160001/chrome/test/data/safe_...
File chrome/test/data/safe_browsing/mach_o/ResourceRules (right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/test/data/safe_...
chrome/test/data/safe_browsing/mach_o/ResourceRules:23: <real>1100</real>
Mixed tabs and spaces?

[Greg K, 2015-10-07 20:29:16]

Patchset #2 (id:180001) has been deleted

[Greg K, 2015-10-07 20:29:24]

Patchset #2 (id:200001) has been deleted

[Greg K, 2015-10-07 22:54:30]

Please ignore the changes in tools/checkperms/checkperms.py for now, I am
submitting those in a separate CL.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:40:
std::vector<std::pair<base::FilePath, std::string>>
On 2015/10/03 20:02:59, grt (very slow until Oct 12) wrote:
> #include <string> for std::string
> #include <utility> for std::pair

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:42:
// This is a helper stub to allow us to test this mechanism with
On 2015/10/05 22:19:07, Robert Sesek wrote:
> Rather than exposing the helper directly, you should create a function
> VerifyBinaryIntegrityForTesting() that calls the internal version. *ForTesting
> is forbidden by presubmit from being called in non-test files.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:43:
// alternatively signed code.
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> What does “alternatively signed” mean?

Clarified the comment.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:63:
for (size_t i = 0; i < bundles_and_requirements.size(); i++) {
On 2015/10/03 20:03:00, grt (very slow until Oct 12) wrote:
>   for (const auto& p : bundles_and_requirements) {

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:68:
VerifyBinaryIntegrityHelper(incident_receiver.Pass(), path, requirement);
On 2015/10/03 20:02:59, grt (very slow until Oct 12) wrote:
> don't Pass() ownership of the receiver to the helper func here since it will
be
> destroyed after the first call.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc
(right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:35:
FILE* file = base::OpenFile(file_path, "r+");
On 2015/10/05 22:19:07, Robert Sesek wrote:
> base/files/file.h is probably a better interface to use for this, since it's
got
> RAII semantics.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:63:
CHECK(PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir_));
On 2015/10/05 22:19:07, Robert Sesek wrote:
> Don't CHECK in tests. Use ASSERT_TRUE instead.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:67:
CHECK(temp_dir_.CreateUniqueTempDir());
On 2015/10/05 22:19:07, Robert Sesek wrote:
> Same, no CHECK.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:72:
signed_bundle_path = signed_bundle_path.Append(kBundleBase);
On 2015/10/05 22:19:07, Robert Sesek wrote:
> It's typical to chain the Append calls:
> 
>   base::FilePath signed_bundle_path =
> base::FilePath(test_data_dir_).Append(kBundleBase);

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:75:
CHECK(base::CopyDirectory(signed_bundle_path, copied_bundle_path, true));
On 2015/10/05 22:19:07, Robert Sesek wrote:
> No CHECK.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:125:
VerifyBinaryIntegrityHelper(mock_receiver.Pass(), exe, requirement);
This should be .get()

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/incident.h (right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/incident.h:30: OSX_INCIDENT = 7,
On 2015/10/03 20:03:00, grt (very slow until Oct 12) wrote:
> why not generalize BINARY_INTEGRITY so that it's suitable for both platforms?

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:10: #if
!defined(OS_MACOSX)
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> Not necessary. The _mac name makes sure that no other platform will see
> signature_evaluator_mac.mm. (So does the .mm, in fact.) Because of that, the
> #include above is also not necessary.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:15: #include <string>
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> Blank line between C and C++ system headers.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:32: class
MacSignatureEvaluator {
On 2015/10/05 22:19:07, Robert Sesek wrote:
> Needs a class-levelc omment.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:53: // This is the path
to the code object on disk.
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> “This is” on almost everything here (including Initialize()), kinda redundant.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:69: void
report_altered_files(
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> 1. Naming: ReportAlteredFiles().
> http://google.github.io/styleguide/cppguide.html#Function_Names
> 2. Ordering: methods come before data.
> http://google.github.io/styleguide/cppguide.html#Declaration_Order

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:70: NSObject* detail,
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> NSObject* or id?

id<NSObject>

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:72: };
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:22: namespace {
On 2015/10/05 15:02:12, Mark Mentovai wrote:
> Blank line after this.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:25: // currently.
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> Provide a reference to where in Security.framework they’re defined, so that we
> can check against changes in the future.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:26: const char* xattrs[]
= {
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> Make this more const.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:27:
"com.apple.cs.CodeDirectory",    "com.apple.cs.CodeSignature",
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> One per line. Especially when everything looks so similar, that helps spot
that
> there are in fact six elements and helps distinguish one from another.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:34: bool
getPathFromNSObject(NSObject* obj, std::string* output) {
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> Naming: GetPath, capital G.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:34: bool
getPathFromNSObject(NSObject* obj, std::string* output) {
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> id for a generic Objective-C object?

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:35: if ([obj
isKindOfClass:[NSString class]]) {
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> Seems like a job for ObjCCast.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:73: if ([detail
isKindOfClass:[NSArray class]]) {
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> ObjCCast

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:88: // call has to
change. `ExtractImageFeatures` maps the file, which will
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> What if it maps it without making the pages executable?

Sadly, it turns out not to matter. CS_KILL seems to apply to all signed pages,
not just executable pages, so it cannot map a file with a broken signature at
all.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:97: ssize_t rc =
getxattr(path.value().c_str(), attr, NULL, 0, 0, 0);
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> nullptr

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:114:
base::scoped_nsobject<NSString> url_str([[NSString alloc]
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> SysUTF8ToNSString()? Elsewhere too.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:158: if (errors) {
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> Is there a more specific return value from
> SecStaticCodeCheckValidityWithErrors() to use to signal that the signature was
> violated rather than just some generic thing like a read permission error?

We report back all error code in the set_sec_error() field and can bucket them
server side.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:162:
report_altered_files(detail, incident);
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> {} since the condition got long and wrapped weirdly.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:163: if (id detail =
[info
On 2015/10/05 15:02:11, Mark Mentovai wrote:
> Loop over a list of interesting keys?

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:168: return err ==
errSecSuccess;
On 2015/10/05 15:02:12, Mark Mentovai wrote:
> You can get rid of a level of indentation for the “meat”&nbsp;here by making
> SecStaticCodeCheckValidityWithErrors() == errSecSuccess into an early “return
> true” above, and having the rest of the function dedicated to dealing with !=
> errSecSuccess.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc (right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:8: #include
<string>
On 2015/10/05 15:02:12, Mark Mentovai wrote:
> Separate C from C++ system headers.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:25: const char*
xattrs[] = {
On 2015/10/05 15:02:12, Mark Mentovai wrote:
> It’s unfortunate that this isn’t sharing with the identical list in the other
> file.

I actually made these two separate lists on purpose because if someone messes up
the list in the other file, the test becomes bogus. This is the "control case."
But maybe that doesn't matter?

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:30: }
On 2015/10/05 15:02:12, Mark Mentovai wrote:
> // namespace

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:45:
CHECK(temp_dir_.CreateUniqueTempDir());
On 2015/10/05 22:19:07, Robert Sesek wrote:
> No CHECK

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/test/data/safe_...
File chrome/test/data/safe_browsing/mach_o/Makefile (right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/test/data/safe_...
chrome/test/data/safe_browsing/mach_o/Makefile:69: codesign -f -s
$(KEYCHAIN_IDENTITY) --keychain $(PWD)/codesign.keychain \
On 2015/10/05 22:19:07, Robert Sesek wrote:
> Keep 80 columns. Here and throughout.

Done.

https://codereview.chromium.org/1363613004/diff/160001/chrome/test/data/safe_...
File chrome/test/data/safe_browsing/mach_o/ResourceRules (right):

https://codereview.chromium.org/1363613004/diff/160001/chrome/test/data/safe_...
chrome/test/data/safe_browsing/mach_o/ResourceRules:23: <real>1100</real>
On 2015/10/05 22:19:07, Robert Sesek wrote:
> Mixed tabs and spaces?

Done.

[Robert Sesek, 2015-10-08 19:20:07]

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:42:
new ClientIncidentReport_IncidentData_BinaryIntegrityIncident());
Can you use the copy ctor and remove line 43?

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:58:
NOTREACHED();
NOTREACHED() is only a DCHECK, so this needs a return after it for release mode.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc
(right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:130:
ASSERT_TRUE(incident_data->has_binary_integrity());
These can be EXPECT_ variants. The difference is that ASSERT_ stops the test
from continuing, while EXPECT_ keeps going.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:136:
ASSERT_TRUE(incident_data->binary_integrity().signature().signed_data_size() >
EXPECT_GT

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:45:
std::vector<ClientIncidentReport_IncidentData_BinaryIntegrityIncident>&
Non-constant references are forbidden by the styleguide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...

Change this to a pointer.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:126:
code_.InitializeInto()) != errSecSuccess)
nit: needs braces since multi-line

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:132: kSecCSDefaultFlags,
requirement_.InitializeInto()) != errSecSuccess)
nit: same, braces required

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc (right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:44: }  //
namespace
nit: remove? there's a closing namespace online 329

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:63:
base::ScopedCFTypeRef<CFStringRef> path_str(CFStringCreateWithCString(
SysUTF8ToCFStringRef

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:80:
sizeof(path_buf)))
nit: needs braces

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:111:
ASSERT_TRUE(evaluator.PerformEvaluation(results));
EXPECT  (and throughout where possible)

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:138:
ASSERT_EQ(1u, results.size());
This has to remain an ASSERT_ since you get results[0] below, but the others can
be EXPECT_.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:190: if
(base::mac::IsOSLionOrLater()) {
Generally prefer early returns for short-circuiting.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:292:
ASSERT_NE(main_exec, nullptr);
Similarly, these should be ASSERT_ since they're dereferenced below, but then
you can use EXPECT_ for verifying the data.

https://codereview.chromium.org/1363613004/diff/250001/chrome/common/safe_bro...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/common/safe_bro...
chrome/common/safe_browsing/csd.proto:211: message ExtendedAttr {
Document the message type too, specifically calling it out as an OS X-only
message.

https://codereview.chromium.org/1363613004/diff/250001/chrome/test/data/safe_...
File chrome/test/data/safe_browsing/mach_o/Makefile (right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/test/data/safe_...
chrome/test/data/safe_browsing/mach_o/Makefile:98:
of=$@/Contents/MacOS/test-bundle
nit: something's off about this line (80 cols)

https://codereview.chromium.org/1363613004/diff/250001/chrome/test/data/safe_...
File
chrome/test/data/safe_browsing/mach_o/modified-bundle-and-exec.app/Contents/Resources/codesign.cfg
(right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/test/data/safe_...
chrome/test/data/safe_browsing/mach_o/modified-bundle-and-exec.app/Contents/Resources/codesign.cfg:1:
# Copyright 2015 The Chromium Authors. All rights reserved.
Does this file need to be duplicated? (Same with other bundles)

[Greg K, 2015-10-08 19:40:37]

Patchset #5 (id:270001) has been deleted

[Greg K, 2015-10-09 17:12:01]

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:42:
new ClientIncidentReport_IncidentData_BinaryIntegrityIncident());
On 2015/10/08 19:20:06, Robert Sesek wrote:
> Can you use the copy ctor and remove line 43?

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:58:
NOTREACHED();
On 2015/10/08 19:20:06, Robert Sesek wrote:
> NOTREACHED() is only a DCHECK, so this needs a return after it for release
mode.

Done. I thought about aborting on ASSERT() but the problem is, as bizarre as
this case would be, the lack of ability to perform the background check doesn't
really mean the browser shouldn't be able to continue.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc
(right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:130:
ASSERT_TRUE(incident_data->has_binary_integrity());
On 2015/10/08 19:20:06, Robert Sesek wrote:
> These can be EXPECT_ variants. The difference is that ASSERT_ stops the test
> from continuing, while EXPECT_ keeps going.

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:136:
ASSERT_TRUE(incident_data->binary_integrity().signature().signed_data_size() >
On 2015/10/08 19:20:06, Robert Sesek wrote:
> EXPECT_GT

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:45:
std::vector<ClientIncidentReport_IncidentData_BinaryIntegrityIncident>&
On 2015/10/08 19:20:06, Robert Sesek wrote:
> Non-constant references are forbidden by the styleguide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...
> 
> Change this to a pointer.

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:126:
code_.InitializeInto()) != errSecSuccess)
On 2015/10/08 19:20:06, Robert Sesek wrote:
> nit: needs braces since multi-line

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:132: kSecCSDefaultFlags,
requirement_.InitializeInto()) != errSecSuccess)
On 2015/10/08 19:20:06, Robert Sesek wrote:
> nit: same, braces required

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc (right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:44: }  //
namespace
On 2015/10/08 19:20:06, Robert Sesek wrote:
> nit: remove? there's a closing namespace online 329

The one at the bottom was incorrectly commented. I fixed it to point out that it
closes the safe_browsing namespace.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:63:
base::ScopedCFTypeRef<CFStringRef> path_str(CFStringCreateWithCString(
On 2015/10/08 19:20:06, Robert Sesek wrote:
> SysUTF8ToCFStringRef

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:80:
sizeof(path_buf)))
On 2015/10/08 19:20:06, Robert Sesek wrote:
> nit: needs braces

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:111:
ASSERT_TRUE(evaluator.PerformEvaluation(results));
On 2015/10/08 19:20:06, Robert Sesek wrote:
> EXPECT  (and throughout where possible)

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:138:
ASSERT_EQ(1u, results.size());
On 2015/10/08 19:20:06, Robert Sesek wrote:
> This has to remain an ASSERT_ since you get results[0] below, but the others
can
> be EXPECT_.

Acknowledged.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:190: if
(base::mac::IsOSLionOrLater()) {
On 2015/10/08 19:20:06, Robert Sesek wrote:
> Generally prefer early returns for short-circuiting.

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:292:
ASSERT_NE(main_exec, nullptr);
On 2015/10/08 19:20:06, Robert Sesek wrote:
> Similarly, these should be ASSERT_ since they're dereferenced below, but then
> you can use EXPECT_ for verifying the data.

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/common/safe_bro...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/common/safe_bro...
chrome/common/safe_browsing/csd.proto:211: message ExtendedAttr {
On 2015/10/08 19:20:06, Robert Sesek wrote:
> Document the message type too, specifically calling it out as an OS X-only
> message.

Done.

https://codereview.chromium.org/1363613004/diff/250001/chrome/test/data/safe_...
File chrome/test/data/safe_browsing/mach_o/Makefile (right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/test/data/safe_...
chrome/test/data/safe_browsing/mach_o/Makefile:98:
of=$@/Contents/MacOS/test-bundle
On 2015/10/08 19:20:06, Robert Sesek wrote:
> nit: something's off about this line (80 cols)

Yes, that got messed up because Makefiles use real tabs. Fixed.

https://codereview.chromium.org/1363613004/diff/250001/chrome/test/data/safe_...
File
chrome/test/data/safe_browsing/mach_o/modified-bundle-and-exec.app/Contents/Resources/codesign.cfg
(right):

https://codereview.chromium.org/1363613004/diff/250001/chrome/test/data/safe_...
chrome/test/data/safe_browsing/mach_o/modified-bundle-and-exec.app/Contents/Resources/codesign.cfg:1:
# Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/10/08 19:20:06, Robert Sesek wrote:
> Does this file need to be duplicated? (Same with other bundles)

Nope, an empty file there will work just as well. Done.

[Robert Sesek, 2015-10-15 00:11:20]

Just a few more nits.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:42:
new ClientIncidentReport_IncidentData_BinaryIntegrityIncident(incident));
nit: 80 cols

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:84:
int i = 0;
size_t

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:88:
incident.set_file_basename(path.BaseName().AsUTF8Unsafe());
.value() instead of .AsUTF8Unsafe()

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:145:
base::ScopedCFTypeRef<CFErrorRef> errors(nullptr);
You don't need to say |nullptr| here.

[Greg K, 2015-10-16 19:17:58]

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:42:
new ClientIncidentReport_IncidentData_BinaryIntegrityIncident(incident));
On 2015/10/15 00:11:20, Robert Sesek wrote:
> nit: 80 cols

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:84:
int i = 0;
On 2015/10/15 00:11:20, Robert Sesek wrote:
> size_t

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:88:
incident.set_file_basename(path.BaseName().AsUTF8Unsafe());
On 2015/10/15 00:11:20, Robert Sesek wrote:
> .value() instead of .AsUTF8Unsafe()

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:145:
base::ScopedCFTypeRef<CFErrorRef> errors(nullptr);
On 2015/10/15 00:11:20, Robert Sesek wrote:
> You don't need to say |nullptr| here.

Done.

[Mark Mentovai, 2015-10-16 22:46:40]

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:42:
std::vector<std::pair<base::FilePath, std::string>>
The pair interface isn’t the most lovely because what’s .first and what’s
.second? This can be a vector<some_simple_struct> which lets the code that uses
it reference the fields by more meaningful names.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:42:
new ClientIncidentReport_IncidentData_BinaryIntegrityIncident(incident));
Greg Kerr wrote:
> On 2015/10/15 00:11:20, Robert Sesek wrote:
> > nit: 80 cols
> 
> Done.

Can you re-upload?

[Mark Mentovai, 2015-10-16 23:04:23]

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:41: output->assign([str
UTF8String]);
To get paths from NSString*s, use -fileSystemRepresentation instead of
-UTF8String. Same in the other branches.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:58:
has_requirement_(false),
requirement_str_() to be explicit about it (since that’s how it’s going to be
initialized anyway).

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:73: void
MacSignatureEvaluator::ReportAlteredFiles(
This method doesn’t access any member variables or call any member functions.
You can make it static.

private + static = often a good candidate for taking out of the class altogether
and making it into a helper function in an unnamed namespace right in the
implementation file. I think you can do that here.

The real advantage in this case is that you take any mention of “id” out of the
header, so that the non-Objective-C void* trick isn’t necessary.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:91: // TODO(kerrnel): if
Chrome ever opts into the OS X "kill" semantics, this
Blank line before this, otherwise the comment gets lost when you’re reading a
wall of mixed code and comments.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:96: path,
BinaryFeatureExtractor::kDefaultOptions,
I’d put kDefaultOptions on its own line, because all of the other arguments have
their own lines, and that makes this more obviously look like the 4-argument
call that it is.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:109:
xattr_msg->set_value(xattr_data.data(), xattr_data.size());
The xattr length may have changed in between the two getxattr() call. You should
save the return value from the second getxattr() and .resize() the vector with
that number. That deals with it shrinking. You probably don’t care about dealing
with it growing.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:143:
std::vector<ClientIncidentReport_IncidentData_BinaryIntegrityIncident>*
DCHECK vector emptiness on entry.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:155: CFStringRef keys[]
= {
const?

https://codereview.chromium.org/1363613004/diff/290001/chrome/common/safe_bro...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/1363613004/diff/290001/chrome/common/safe_bro...
chrome/common/safe_browsing/csd.proto:240: // a file. As GateKeeper respects
this signature, we look for it and collect
lowercase k

[Greg K, 2015-10-19 20:42:58]

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:42:
std::vector<std::pair<base::FilePath, std::string>>
On 2015/10/16 22:46:40, Mark Mentovai wrote:
> The pair interface isn’t the most lovely because what’s .first and what’s
> .second? This can be a vector<some_simple_struct> which lets the code that
uses
> it reference the fields by more meaningful names.

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:41: output->assign([str
UTF8String]);
On 2015/10/16 23:04:22, Mark Mentovai wrote:
> To get paths from NSString*s, use -fileSystemRepresentation instead of
> -UTF8String. Same in the other branches.

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:58:
has_requirement_(false),
On 2015/10/16 23:04:22, Mark Mentovai wrote:
> requirement_str_() to be explicit about it (since that’s how it’s going to be
> initialized anyway).

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:73: void
MacSignatureEvaluator::ReportAlteredFiles(
On 2015/10/16 23:04:22, Mark Mentovai wrote:
> This method doesn’t access any member variables or call any member functions.
> You can make it static.
> 
> private + static = often a good candidate for taking out of the class
altogether
> and making it into a helper function in an unnamed namespace right in the
> implementation file. I think you can do that here.
> 
> The real advantage in this case is that you take any mention of “id”&nbsp;out
of the
> header, so that the non-Objective-C void* trick isn’t necessary.

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:91: // TODO(kerrnel): if
Chrome ever opts into the OS X "kill" semantics, this
On 2015/10/16 23:04:22, Mark Mentovai wrote:
> Blank line before this, otherwise the comment gets lost when you’re reading a
> wall of mixed code and comments.

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:96: path,
BinaryFeatureExtractor::kDefaultOptions,
On 2015/10/16 23:04:22, Mark Mentovai wrote:
> I’d put kDefaultOptions on its own line, because all of the other arguments
have
> their own lines, and that makes this more obviously look like the 4-argument
> call that it is.

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:109:
xattr_msg->set_value(xattr_data.data(), xattr_data.size());
On 2015/10/16 23:04:22, Mark Mentovai wrote:
> The xattr length may have changed in between the two getxattr() call. You
should
> save the return value from the second getxattr() and .resize() the vector with
> that number. That deals with it shrinking. You probably don’t care about
dealing
> with it growing.

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:143:
std::vector<ClientIncidentReport_IncidentData_BinaryIntegrityIncident>*
On 2015/10/16 23:04:22, Mark Mentovai wrote:
> DCHECK vector emptiness on entry.

Done.

https://codereview.chromium.org/1363613004/diff/290001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:155: CFStringRef keys[]
= {
On 2015/10/16 23:04:22, Mark Mentovai wrote:
> const?

Done.

[Greg K, 2015-10-20 22:46:13]

kerrnel@chromium.org changed reviewers:
+ grt@chromium.org, thestig@chromium.org

[Greg K, 2015-10-20 22:46:14]

Since Robert and Mark's last feedback wasn't anything major, I'm going top move
this along to the OWNERS while I wait to see if they have any last comments. 

thestig@chromium.org: Please review changes in 

chrome/browser/safe_browsing/signature_evaluator* files and
chrome/common/safe_browsing/csd.proto, plus elsewhere as you see fit.

grt@chromium.org: Please review changes in 

chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer*
files, and anything else as you see fit.

[Robert Sesek, 2015-10-20 22:57:27]

https://codereview.chromium.org/1363613004/diff/310001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://codereview.chromium.org/1363613004/diff/310001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:23:
#if defined(OS_MACOSX)
Move this closer to where it's used by the functions?

https://codereview.chromium.org/1363613004/diff/310001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:28:
base::FilePath path_;
nit: public struct members don't get trailing _

[Lei Zhang, 2015-10-21 00:17:29]

thestig@chromium.org changed reviewers:
+ mattm@chromium.org

[Lei Zhang, 2015-10-21 00:18:16]

On 2015/10/20 22:46:14, Greg Kerr wrote:
> Since Robert and Mark's last feedback wasn't anything major, I'm going top
move
> this along to the OWNERS while I wait to see if they have any last comments. 
> 
> mailto:thestig@chromium.org: Please review changes in 
> 
> chrome/browser/safe_browsing/signature_evaluator* files and
> chrome/common/safe_browsing/csd.proto, plus elsewhere as you see fit.

I defer to mattm@ from chrome/browser/safe_browsing/OWNERS.

[mattm, 2015-10-21 02:06:34]

https://codereview.chromium.org/1363613004/diff/330001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc
(right):

https://codereview.chromium.org/1363613004/diff/330001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc:87:
incident_receiver->ClearIncidentForProcess(
This doesn't appear in the mac version. Should it?

https://codereview.chromium.org/1363613004/diff/330001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://codereview.chromium.org/1363613004/diff/330001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:39:
#if defined(OS_MACOSX)
Would it be possible to reconcile the win/mac code more? Or at least make the
split more clear and with less os-specific code in the files that aren't _win or
_mac?

https://codereview.chromium.org/1363613004/diff/330001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):

https://codereview.chromium.org/1363613004/diff/330001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:23: class
MacSignatureEvaluator {
There is also a bug about signature checking for download protection
(crbug.com/464083)  Could this be structured so that the signature checking is
usable for both?

[Greg K, 2015-10-27 01:06:18]

https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc
(right):

https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc:87:
incident_receiver->ClearIncidentForProcess(
On 2015/10/21 02:06:34, mattm wrote:
> This doesn't appear in the mac version. Should it?

I talked to some of the safe browsing engineers, and indeed it should. I missed
this because I didn't understood what it was doing but they clarified for me:
this is so that after an update, if the modified files are restored, it will
report about any new modifications in the future.

https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:39:
#if defined(OS_MACOSX)
On 2015/10/21 02:06:34, mattm wrote:
> Would it be possible to reconcile the win/mac code more? Or at least make the
> split more clear and with less os-specific code in the files that aren't _win
or
> _mac?

Yes, I've split the win and mac specific code into their respective files and
left the common functionality in the main file.

https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):

https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.h:23: class
MacSignatureEvaluator {
On 2015/10/21 02:06:34, mattm wrote:
> There is also a bug about signature checking for download protection
> (crbug.com/464083)  Could this be structured so that the signature checking is
> usable for both?

I talked to Robert. On OS X the notion of "trusted" doesn't tell us much: it
just means it was developer ID signed, which lots of unwanted software is. In
fact, this MacSignatureEvaluator actually calls into some of the code Robert
wrote for download protection. So we don't currently see a future where we'll be
using this class to check downloads.

[mattm, 2015-10-27 01:28:13]

https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):

https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
chrome/browser/safe_browsing/signature_evaluator_mac.h:23: class
MacSignatureEvaluator {
On 2015/10/27 01:06:18, Greg Kerr wrote:
> On 2015/10/21 02:06:34, mattm wrote:
> > There is also a bug about signature checking for download protection
> > (crbug.com/464083)  Could this be structured so that the signature checking
is
> > usable for both?
> 
> I talked to Robert. On OS X the notion of "trusted" doesn't tell us much: it
> just means it was developer ID signed, which lots of unwanted software is. In
> fact, this MacSignatureEvaluator actually calls into some of the code Robert
> wrote for download protection. So we don't currently see a future where we'll
be
> using this class to check downloads.

I see. I still think it could be useful, though. We do more than just check a
signed/not-signed bool. If we know the signature is valid, we  can check against
a whitelist
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...).
We also send the cert chain in the download service ping
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/safe...),
but that's not super useful if we don't know if it was a valid signature.

[Greg K, 2015-10-27 16:16:18]

On 2015/10/27 01:28:13, mattm wrote:
>
https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
> File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):
> 
>
https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
> chrome/browser/safe_browsing/signature_evaluator_mac.h:23: class
> MacSignatureEvaluator {
> On 2015/10/27 01:06:18, Greg Kerr wrote:
> > On 2015/10/21 02:06:34, mattm wrote:
> > > There is also a bug about signature checking for download protection
> > > (crbug.com/464083)  Could this be structured so that the signature
checking
> is
> > > usable for both?
> > 
> > I talked to Robert. On OS X the notion of "trusted" doesn't tell us much: it
> > just means it was developer ID signed, which lots of unwanted software is.
In
> > fact, this MacSignatureEvaluator actually calls into some of the code Robert
> > wrote for download protection. So we don't currently see a future where
we'll
> be
> > using this class to check downloads.
> 
> I see. I still think it could be useful, though. We do more than just check a
> signed/not-signed bool. If we know the signature is valid, we  can check
against
> a whitelist
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...).
> We also send the cert chain in the download service ping
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/safe...),
> but that's not super useful if we don't know if it was a valid signature.

Since we pull the entire code signature blob on OS X, the cert chain is being
sent. We can then verify the signature on our end. It might be useful to check
against a whitelist if the signature is valid, I'm not sure without talking to
Robert more, but this CL is already large and complicated so I would prefer
doing anything that expands the scope of the work in a follow up CL.

[Robert Sesek, 2015-10-27 20:33:56]

On 2015/10/27 16:16:18, Greg Kerr wrote:
> On 2015/10/27 01:28:13, mattm wrote:
> >
>
https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
> > File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
> > chrome/browser/safe_browsing/signature_evaluator_mac.h:23: class
> > MacSignatureEvaluator {
> > On 2015/10/27 01:06:18, Greg Kerr wrote:
> > > On 2015/10/21 02:06:34, mattm wrote:
> > > > There is also a bug about signature checking for download protection
> > > > (crbug.com/464083)  Could this be structured so that the signature
> checking
> > is
> > > > usable for both?
> > > 
> > > I talked to Robert. On OS X the notion of "trusted" doesn't tell us much:
it
> > > just means it was developer ID signed, which lots of unwanted software is.
> In
> > > fact, this MacSignatureEvaluator actually calls into some of the code
Robert
> > > wrote for download protection. So we don't currently see a future where
> we'll
> > be
> > > using this class to check downloads.
> > 
> > I see. I still think it could be useful, though. We do more than just check
a
> > signed/not-signed bool. If we know the signature is valid, we  can check
> against
> > a whitelist
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...).
> > We also send the cert chain in the download service ping
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/safe...),
> > but that's not super useful if we don't know if it was a valid signature.
> 
> Since we pull the entire code signature blob on OS X, the cert chain is being
> sent. We can then verify the signature on our end. It might be useful to check
> against a whitelist if the signature is valid, I'm not sure without talking to
> Robert more, but this CL is already large and complicated so I would prefer
> doing anything that expands the scope of the work in a follow up CL.

Right, we upload the entire chain for downloads already, so I'm not sure
checking them client-side is very important. Also, I think a little more
refactoring would need to be done to support signature checking of the in-memory
blobs (download files aren't ever written to disk in the DMG analyzer), which I
think is tangential to this CL. At the moment, we're most interested in
collecting incident reports of Chrome's own binary integrity.

[mattm, 2015-10-27 23:51:07]

On 2015/10/27 20:33:56, Robert Sesek wrote:
> On 2015/10/27 16:16:18, Greg Kerr wrote:
> > On 2015/10/27 01:28:13, mattm wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
> > > File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
> > > chrome/browser/safe_browsing/signature_evaluator_mac.h:23: class
> > > MacSignatureEvaluator {
> > > On 2015/10/27 01:06:18, Greg Kerr wrote:
> > > > On 2015/10/21 02:06:34, mattm wrote:
> > > > > There is also a bug about signature checking for download protection
> > > > > (crbug.com/464083)  Could this be structured so that the signature
> > checking
> > > is
> > > > > usable for both?
> > > > 
> > > > I talked to Robert. On OS X the notion of "trusted" doesn't tell us
much:
> it
> > > > just means it was developer ID signed, which lots of unwanted software
is.
> > In
> > > > fact, this MacSignatureEvaluator actually calls into some of the code
> Robert
> > > > wrote for download protection. So we don't currently see a future where
> > we'll
> > > be
> > > > using this class to check downloads.
> > > 
> > > I see. I still think it could be useful, though. We do more than just
check
> a
> > > signed/not-signed bool. If we know the signature is valid, we  can check
> > against
> > > a whitelist
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...).
> > > We also send the cert chain in the download service ping
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/safe...),
> > > but that's not super useful if we don't know if it was a valid signature.
> > 
> > Since we pull the entire code signature blob on OS X, the cert chain is
being
> > sent. We can then verify the signature on our end. It might be useful to
check
> > against a whitelist if the signature is valid, I'm not sure without talking
to
> > Robert more, but this CL is already large and complicated so I would prefer
> > doing anything that expands the scope of the work in a follow up CL.
> 
> Right, we upload the entire chain for downloads already, so I'm not sure
> checking them client-side is very important. Also, I think a little more
> refactoring would need to be done to support signature checking of the
in-memory
> blobs (download files aren't ever written to disk in the DMG analyzer), which
I
> think is tangential to this CL. At the moment, we're most interested in
> collecting incident reports of Chrome's own binary integrity.

Can you verify the signature remotely? Sure, you can verify that the chain is
valid, but can you verify that the binary was actually signed by that chain?


But yeah, we can go ahead with this CL without resolving that question.

Note: the CL description still says "DO NOT SUBMIT".

lgtm.

[Greg K, 2015-10-27 23:56:56]

On 2015/10/27 23:51:07, mattm wrote:
> On 2015/10/27 20:33:56, Robert Sesek wrote:
> > On 2015/10/27 16:16:18, Greg Kerr wrote:
> > > On 2015/10/27 01:28:13, mattm wrote:
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
> > > > File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/1363613004/diff/330001/chrome/browser/...
> > > > chrome/browser/safe_browsing/signature_evaluator_mac.h:23: class
> > > > MacSignatureEvaluator {
> > > > On 2015/10/27 01:06:18, Greg Kerr wrote:
> > > > > On 2015/10/21 02:06:34, mattm wrote:
> > > > > > There is also a bug about signature checking for download protection
> > > > > > (crbug.com/464083)  Could this be structured so that the signature
> > > checking
> > > > is
> > > > > > usable for both?
> > > > > 
> > > > > I talked to Robert. On OS X the notion of "trusted" doesn't tell us
> much:
> > it
> > > > > just means it was developer ID signed, which lots of unwanted software
> is.
> > > In
> > > > > fact, this MacSignatureEvaluator actually calls into some of the code
> > Robert
> > > > > wrote for download protection. So we don't currently see a future
where
> > > we'll
> > > > be
> > > > > using this class to check downloads.
> > > > 
> > > > I see. I still think it could be useful, though. We do more than just
> check
> > a
> > > > signed/not-signed bool. If we know the signature is valid, we  can check
> > > against
> > > > a whitelist
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...).
> > > > We also send the cert chain in the download service ping
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/safe...),
> > > > but that's not super useful if we don't know if it was a valid
signature.
> > > 
> > > Since we pull the entire code signature blob on OS X, the cert chain is
> being
> > > sent. We can then verify the signature on our end. It might be useful to
> check
> > > against a whitelist if the signature is valid, I'm not sure without
talking
> to
> > > Robert more, but this CL is already large and complicated so I would
prefer
> > > doing anything that expands the scope of the work in a follow up CL.
> > 
> > Right, we upload the entire chain for downloads already, so I'm not sure
> > checking them client-side is very important. Also, I think a little more
> > refactoring would need to be done to support signature checking of the
> in-memory
> > blobs (download files aren't ever written to disk in the DMG analyzer),
which
> I
> > think is tangential to this CL. At the moment, we're most interested in
> > collecting incident reports of Chrome's own binary integrity.
> 
> Can you verify the signature remotely? Sure, you can verify that the chain is
> valid, but can you verify that the binary was actually signed by that chain?
> 
> 
> But yeah, we can go ahead with this CL without resolving that question.
> 
> Note: the CL description still says "DO NOT SUBMIT".
> 
> lgtm.

Thanks. I understand why you want to check the signature locally but yeah, let's
punt that to another bug since that warrants an investigation and plan on its
own.

grt, can you please review the changes in
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer*
files, and anything else as you see fit?

[grt (UTC plus 2), 2015-10-28 17:49:51]

looks pretty good.

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:37:
void ClearIntegrityReports(IncidentReceiver* incident_receiver,
ClearBinaryIntegrityForFile?

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:7:
#include "base/base_paths.h"
unused

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:10:
#include "base/path_service.h"
unused

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:35:
VLOG(1) << "Signature verification failed: " << path.value();
DVLOG or no log at all

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:75:
VerifyBinaryIntegrityHelper(incident_receiver.get(),
    VerifyBinaryIntegrityHelper(incident_receiver.get(), p.path, p.requirement);

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.h
(right):

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.h:13:
#include "base/files/file_util.h"
base/files/file_path.h

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc
(right):

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:7:
#include <stdio.h>
are these needed?

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:10:
#include "base/bind.h"
unsed

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:17:
#include "base/test/scoped_path_override.h"
unused

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:59:
// Retrieve DIR_TEST_DATA here because it is based on DIR_EXE and we are
is this comment correct?

[Greg K, 2015-10-29 23:51:32]

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:37:
void ClearIntegrityReports(IncidentReceiver* incident_receiver,
On 2015/10/28 17:49:51, grt wrote:
> ClearBinaryIntegrityForFile?

Done.

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:7:
#include "base/base_paths.h"
On 2015/10/28 17:49:51, grt wrote:
> unused

Done.

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:10:
#include "base/path_service.h"
On 2015/10/28 17:49:51, grt wrote:
> unused

Done.

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:35:
VLOG(1) << "Signature verification failed: " << path.value();
On 2015/10/28 17:49:51, grt wrote:
> DVLOG or no log at all

Why no log at all? I actually want this log in production so that we can have
some direct insight into what happens in the actual signed builds. If you want,
I can do a follow up CL to delete this once it's deployed in canary, but at
least in the first few canary builds we ship I want to have some logging.

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:75:
VerifyBinaryIntegrityHelper(incident_receiver.get(),
On 2015/10/28 17:49:51, grt wrote:
>     VerifyBinaryIntegrityHelper(incident_receiver.get(), p.path,
p.requirement);

Done.

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.h
(right):

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.h:13:
#include "base/files/file_util.h"
On 2015/10/28 17:49:51, grt wrote:
> base/files/file_path.h

Done.

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc
(right):

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:7:
#include <stdio.h>
On 2015/10/28 17:49:51, grt wrote:
> are these needed?

Nope, good catch.

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:10:
#include "base/bind.h"
On 2015/10/28 17:49:51, grt wrote:
> unsed

Done.

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:17:
#include "base/test/scoped_path_override.h"
On 2015/10/28 17:49:51, grt wrote:
> unused

Done.

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:59:
// Retrieve DIR_TEST_DATA here because it is based on DIR_EXE and we are
On 2015/10/28 17:49:51, grt wrote:
> is this comment correct?

Not anymore, good catch.

[grt (UTC plus 2), 2015-10-30 15:47:04]

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/370001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:35:
VLOG(1) << "Signature verification failed: " << path.value();
On 2015/10/29 23:51:31, Greg Kerr wrote:
> On 2015/10/28 17:49:51, grt wrote:
> > DVLOG or no log at all
> 
> Why no log at all?

VLOG adds weight to shipping builds.

> I actually want this log in production so that we can have
> some direct insight into what happens in the actual signed builds.

Who would see the log? If it's just you for your manual testing and what you
want to know is if an incident is generated in a signed build when you manually
diddle the signature, look in chrome://histograms to see if the BINARY_INTEGRITY
bucket (2) of SBIRS.ReceivedIncident is hit. Or run the official Chrome in a
debugger and set a breakpoint on this function.

> If you want,
> I can do a follow up CL to delete this once it's deployed in canary, but at
> least in the first few canary builds we ship I want to have some logging.

If you can get the validation you want by any other means, please do so. Logging
should be used as a last resort.

https://codereview.chromium.org/1363613004/diff/390001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc
(right):

https://codereview.chromium.org/1363613004/diff/390001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:46:
void SetUp() override;
i forgot to mention: prefer a ctor over SetUp when possible (see
http://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/des...)

[Greg K, 2015-10-30 20:05:13]

https://codereview.chromium.org/1363613004/diff/390001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/390001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:33:
VLOG(1) << "Signature verification failed: " << path.value();
Removing this.

https://codereview.chromium.org/1363613004/diff/390001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc
(right):

https://codereview.chromium.org/1363613004/diff/390001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc:46:
void SetUp() override;
On 2015/10/30 15:47:04, grt wrote:
> i forgot to mention: prefer a ctor over SetUp when possible (see
>
http://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/des...)

Per discussion, SetUp() is used for the availability of the ASSERT_ macros.

[Greg K, 2015-11-02 18:34:22]

Description was changed from

==========
Implement anonymous, opt-in, collection of OS X binary integrity incidents.

This change implements anonymous collection of data about binary integrity
incidents on OS X. A binary integrity incident means that Chrome's application
bundle was modified on disk by some unauthorized source. This change collects
information about modified files. The checks are enabled on a sampling of user's
browsers and run at browser startup if enabled.

DO NOT SUBMIT

BUG=530314
==========

to

==========
Implement anonymous, opt-in, collection of OS X binary integrity incidents.

This change implements anonymous collection of data about binary integrity
incidents on OS X. A binary integrity incident means that Chrome's application
bundle was modified on disk by some unauthorized source. This change collects
information about modified files. The checks are enabled on a sampling of user's
browsers and run at browser startup if enabled.

BUG=530314
==========

[Robert Sesek, 2015-11-02 19:42:12]

LGTM w/ some indention nits. Could also just `git cl format` these away.

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc
(right):

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc:42:
incident(
nit: continuations are indented +4 spaces (so an additional +2 here)

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc
(right):

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.cc:31:
incident(new ClientIncidentReport_IncidentData_BinaryIntegrityIncident());
nit: +2

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.h
(right):

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac.h:41:

nit: extra blank line

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_win.h
(right):

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer_win.h:24:

nit: extra blank line

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:81:
signature->add_xattr();
nit: +2

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:112: contained_file =
incident->add_contained_file();
nit: +2

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc (right):

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:178:
contained_file = incident.contained_file(0);
nit: +2

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:209:
contained_file = incident.contained_file(0);
nit: +2

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:236:
contained_file = incident.contained_file(0);
nit: +2

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac_unittest.cc:312:
libsigned64->relative_path());
nit: align with the "

[grt (UTC plus 2), 2015-11-02 20:20:15]

Lgtm with nits

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:9:
#include <utility>
unused?

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.h (right):

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.h:11: #include <vector>
move to .mm file

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/signature_evaluator_mac.mm (right):

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:45: } else if (NSURL*
url = base::mac::ObjCCast<NSURL>(obj)) {
remove "else" here and below since each if() body returns. i.e.:
  if (foo) {
    blah;
    return true;
  }
  if (bar) {
    ...

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/signature_evaluator_mac.mm:81:
signature->add_xattr();
nit: indent

[Mark Mentovai, 2015-11-02 20:20:49]

LGTM after Robert’s nits.

[Greg K, 2015-11-02 23:13:00]

I cleaned up all the nits, and for all indentation nits, I just ran git cl
format.

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h
(right):

https://codereview.chromium.org/1363613004/diff/410001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.h:9:
#include <utility>
On 2015/11/02 20:20:15, grt wrote:
> unused?

Done.

[Greg K, 2015-11-02 23:20:51]

The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-02 23:22:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363613004/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363613004/430001

[commit-bot: I haz the power, 2015-11-02 23:28:23]

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

[commit-bot: I haz the power, 2015-11-02 23:28:24]

Dry run: This issue passed the CQ dry run.

[Greg K, 2015-11-02 23:31:34]

The CQ bit was checked by kerrnel@chromium.org

[Greg K, 2015-11-02 23:31:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org,
mark@chromium.org, rsesek@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1363613004/#ps430001
(title: "Ran git cl format")

[commit-bot: I haz the power, 2015-11-02 23:33:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363613004/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363613004/430001

[commit-bot: I haz the power, 2015-11-02 23:41:00]

Committed patchset #12 (id:430001)

[commit-bot: I haz the power, 2015-11-02 23:41:54]

Patchset 12 (id:??) landed as
https://crrev.com/2b08c52ad7d79322756e93b7fb4dfb0a00f402e4
Cr-Commit-Position: refs/heads/master@{#357469}