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

chrome/browser/safe_browsing/signature_evaluator_mac.mm

Index: chrome/browser/safe_browsing/signature_evaluator_mac.mm
diff --git a/chrome/browser/safe_browsing/signature_evaluator_mac.mm b/chrome/browser/safe_browsing/signature_evaluator_mac.mm
new file mode 100644
index 0000000000000000000000000000000000000000..90032818ed76cf8de72be7d57d8024fd871dd3c8
--- /dev/null
+++ b/chrome/browser/safe_browsing/signature_evaluator_mac.mm
@@ -0,0 +1,166 @@
+// Copyright (c) 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/safe_browsing/signature_evaluator_mac.h"
+
+#include <CoreFoundation/CoreFoundation.h>
+#include <Foundation/Foundation.h>
+#include <Security/Security.h>
+#include <sys/xattr.h>
+
+#include "base/mac/foundation_util.h"
+#include "base/mac/mac_util.h"
+#include "base/mac/scoped_cftyperef.h"
+#include "base/mac/scoped_nsobject.h"
+#include "base/strings/sys_string_conversions.h"
+#include "chrome/common/safe_browsing/binary_feature_extractor.h"
+#include "chrome/common/safe_browsing/csd.pb.h"
+#include "chrome/common/safe_browsing/mach_o_image_reader_mac.h"
+
+namespace safe_browsing {
+
+namespace {
+
+// OS X code signing data can be stored in extended attributes as well. This is
+// a list of the extended attributes slots currently used in Security.framework,
+// from codesign.h (see the kSecCS_* constants).
+const char* const xattrs[] = {
+  "com.apple.cs.CodeDirectory",
+  "com.apple.cs.CodeSignature",
+  "com.apple.cs.CodeRequirements",
+  "com.apple.cs.CodeResources",
+  "com.apple.cs.CodeApplication",
+  "com.apple.cs.CodeEntitlements",
+};
+
+// Convenience function to get the appropriate path from a variety of NSObject
+// types.
+bool GetPathFromNSObject(id obj, std::string* output) {
+  if (NSString* str = base::mac::ObjCCast<NSString>(obj)) {
+    output->assign([str UTF8String]);

[Mark Mentovai, 2015/10/16 23:04:22]

To get paths from NSString*s, use -fileSystemRepresentation instead of
-UTF8String. Same in the other branches.

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

Done.

+    return true;
+  } else if (NSURL* url = base::mac::ObjCCast<NSURL>(obj)) {
+    output->assign([[url path] UTF8String]);
+    return true;
+  } else if (NSBundle* bundle = base::mac::ObjCCast<NSBundle>(obj)) {
+    output->assign([[bundle bundlePath] UTF8String]);
+    return true;
+  }
+  return false;
+}
+
+}  // namespace
+
+MacSignatureEvaluator::MacSignatureEvaluator(
+    const base::FilePath& signed_object_path)
+    : path_(signed_object_path),
+      has_requirement_(false),

[Mark Mentovai, 2015/10/16 23:04:22]

requirement_str_() to be explicit about it (since that’s how it’s going to be
initialized anyway).

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

Done.

+      code_(nullptr),
+      requirement_(nullptr) {}
+
+MacSignatureEvaluator::MacSignatureEvaluator(
+    const base::FilePath& signed_object_path,
+    const std::string& requirement)
+    : path_(signed_object_path),
+      requirement_str_(requirement),
+      has_requirement_(true),
+      code_(nullptr),
+      requirement_(nullptr) {}
+
+MacSignatureEvaluator::~MacSignatureEvaluator() {}
+
+void MacSignatureEvaluator::ReportAlteredFiles(

[Mark Mentovai, 2015/10/16 23:04:22]

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.

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

Done.

+    id detail,
+    int32_t err_code,
+    std::vector<ClientIncidentReport_IncidentData_BinaryIntegrityIncident>*
+        results) {
+  if (NSArray* arr = base::mac::ObjCCast<NSArray>(detail)) {
+    for (id obj in arr)
+      ReportAlteredFiles(obj, err_code, results);
+  } else {
+    std::string path_str;
+    if (!GetPathFromNSObject(detail, &path_str))
+      return;
+
+    base::FilePath path(path_str);
+    ClientIncidentReport_IncidentData_BinaryIntegrityIncident incident;
+    incident.set_file_basename(path.BaseName().AsUTF8Unsafe());

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

.value() instead of .AsUTF8Unsafe()

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

Done.

+    incident.set_sec_error(err_code);
+    scoped_refptr<BinaryFeatureExtractor> bfe = new BinaryFeatureExtractor();
+    // TODO(kerrnel): if Chrome ever opts into the OS X "kill" semantics, this

[Mark Mentovai, 2015/10/16 23:04:22]

Blank line before this, otherwise the comment gets lost when you’re reading a
wall of mixed code and comments.

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

Done.

+    // call has to change. `ExtractImageFeatures` maps the file, which will
+    // cause Chrome to be killed before it can report on the invalid file.
+    // This call will need to read(2) the binary into a buffer.
+    if (!bfe->ExtractImageFeatures(
+            path, BinaryFeatureExtractor::kDefaultOptions,

[Mark Mentovai, 2015/10/16 23:04:22]

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.

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

Done.

+            incident.mutable_image_headers(),
+            incident.mutable_signature()->mutable_signed_data())) {
+      // If this is not a mach-o file, search inside the extended attributes.
+      for (const char* attr : xattrs) {
+        ssize_t rc = getxattr(path.value().c_str(), attr, nullptr, 0, 0, 0);
+        if (rc >= 0) {
+          std::vector<uint8_t> xattr_data(rc);
+          if (getxattr(path.value().c_str(), attr, &xattr_data[0],
+                       xattr_data.size(), 0, 0) >= 0) {
+            ClientDownloadRequest_ExtendedAttr* xattr_msg =
+                incident.mutable_signature()->add_xattr();
+            xattr_msg->set_key(attr);
+            xattr_msg->set_value(xattr_data.data(), xattr_data.size());

[Mark Mentovai, 2015/10/16 23:04:22]

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.

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

Done.

+          }
+        }
+      }
+    }
+    results->push_back(incident);
+  }
+}
+
+bool MacSignatureEvaluator::Initialize() {
+  base::scoped_nsobject<NSURL> code_url([[NSURL alloc]
+      initFileURLWithPath:base::SysUTF8ToNSString(path_.value())]);
+  if (!code_url)
+    return false;
+
+  if (SecStaticCodeCreateWithPath(base::mac::NSToCFCast(code_url.get()),
+                                  kSecCSDefaultFlags,
+                                  code_.InitializeInto()) != errSecSuccess) {
+    return false;
+  }
+
+  if (has_requirement_) {
+    if (SecRequirementCreateWithString(
+            base::mac::NSToCFCast(base::SysUTF8ToNSString(requirement_str_)),
+            kSecCSDefaultFlags, requirement_.InitializeInto()) !=
+        errSecSuccess) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
+bool MacSignatureEvaluator::PerformEvaluation(
+    std::vector<ClientIncidentReport_IncidentData_BinaryIntegrityIncident>*

[Mark Mentovai, 2015/10/16 23:04:22]

DCHECK vector emptiness on entry.

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

Done.

+        results) {
+  base::ScopedCFTypeRef<CFErrorRef> errors(nullptr);

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

You don't need to say |nullptr| here.

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

Done.

+  OSStatus err = SecStaticCodeCheckValidityWithErrors(
+      code_, kSecCSCheckAllArchitectures, requirement_,
+      errors.InitializeInto());
+  if (err == errSecSuccess)
+    return true;
+  // This adds the signature of the main binary to the incident reports.
+  ReportAlteredFiles(base::SysUTF8ToNSString(path_.value()), err, results);
+  if (errors) {
+    NSDictionary* info = [base::mac::CFToNSCast(errors.get()) userInfo];
+    CFStringRef keys[] = {

[Mark Mentovai, 2015/10/16 23:04:22]

const?

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

Done.

+        kSecCFErrorResourceAltered, kSecCFErrorResourceMissing,
+    };
+    for (CFStringRef key : keys) {
+      if (id detail = [info objectForKey:base::mac::CFToNSCast(key)])
+        ReportAlteredFiles(detail, err, results);
+    }
+  }
+  return false;
+}
+
+}  // namespace safe_browsing