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

chrome/browser/safe_browsing/signature_evaluator_mac.mm

+// 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 "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 {

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

Blank line after this.

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

Done.

+// 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
+// currently.

[Mark Mentovai, 2015/10/05 15:02:11]

Provide a reference to where in Security.framework they’re defined, so that we
can check against changes in the future.

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

Done.

+const char* xattrs[] = {

[Mark Mentovai, 2015/10/05 15:02:11]

Make this more const.

[Greg K, 2015/10/07 22:54:29]

Done.

+    "com.apple.cs.CodeDirectory",    "com.apple.cs.CodeSignature",

[Mark Mentovai, 2015/10/05 15:02:11]

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.

[Greg K, 2015/10/07 22:54:29]

Done.

+    "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(NSObject* obj, std::string* output) {

[Mark Mentovai, 2015/10/05 15:02:11]

id for a generic Objective-C object?

[Mark Mentovai, 2015/10/05 15:02:11]

Naming: GetPath, capital G.

[Greg K, 2015/10/07 22:54:29]

Done.

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

Done.

+  if ([obj isKindOfClass:[NSString class]]) {

[Mark Mentovai, 2015/10/05 15:02:11]

Seems like a job for ObjCCast.

[Greg K, 2015/10/07 22:54:29]

Done.

+    output->assign([static_cast<NSString*>(obj) UTF8String]);
+    return true;
+  } else if ([obj isKindOfClass:[NSURL class]]) {
+    output->assign([[static_cast<NSURL*>(obj) path] UTF8String]);
+    return true;
+  } else if ([obj isKindOfClass:[NSBundle class]]) {
+    output->assign([[static_cast<NSBundle*>(obj) bundlePath] UTF8String]);
+    return true;
+  }
+
+  return false;
+}
+
+}  // namespace
+
+MacSignatureEvaluator::MacSignatureEvaluator(
+    const base::FilePath& signed_object_path)
+    : path_(signed_object_path),
+      has_requirement_(false),
+      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::report_altered_files(
+    NSObject* detail,
+    ClientIncidentReport_IncidentData_OSXBinaryIntegrityIncident*
+        main_incident) {
+  if ([detail isKindOfClass:[NSArray class]]) {

[Mark Mentovai, 2015/10/05 15:02:11]

ObjCCast

[Greg K, 2015/10/07 22:54:29]

Done.

+    NSArray* arr = static_cast<NSArray*>(detail);
+    for (id obj in arr)
+      report_altered_files(obj, main_incident);
+  } else {
+    std::string path_str;
+    if (!getPathFromNSObject(detail, &path_str))
+      return;
+
+    base::FilePath path(path_str);
+    ClientIncidentReport_IncidentData_BinaryIntegrityIncident* sub_incident =
+        main_incident->add_sub_incident();
+    sub_incident->set_file_basename(path.BaseName().AsUTF8Unsafe());
+    scoped_refptr<BinaryFeatureExtractor> bfe = new BinaryFeatureExtractor();
+    // TODO(kerrnel): if Chrome ever opts into the OS X "kill" semantics, this
+    // call has to change. `ExtractImageFeatures` maps the file, which will

[Mark Mentovai, 2015/10/05 15:02:11]

What if it maps it without making the pages executable?

[Greg K, 2015/10/07 22:54:29]

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. 

+    // 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,
+            sub_incident->mutable_image_headers(),
+            sub_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, NULL, 0, 0, 0);

[Mark Mentovai, 2015/10/05 15:02:11]

nullptr

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

Done.

+        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 =
+                sub_incident->mutable_signature()->add_xattr();
+            xattr_msg->set_key(attr);
+            xattr_msg->set_value(xattr_data.data(), xattr_data.size());
+          }
+        }
+      }
+    }
+  }
+}
+
+bool MacSignatureEvaluator::Initialize() {
+  base::scoped_nsobject<NSString> url_str([[NSString alloc]

[Mark Mentovai, 2015/10/05 15:02:11]

SysUTF8ToNSString()? Elsewhere too.

[Greg K, 2015/10/07 22:54:29]

Done.

+      initWithCString:path_.value().c_str()
+             encoding:NSUTF8StringEncoding]);
+
+  base::scoped_nsobject<NSURL> code_url(
+      [[NSURL alloc] initFileURLWithPath:url_str]);
+  if (!code_url)
+    return false;
+
+  if (SecStaticCodeCreateWithPath(base::mac::NSToCFCast(code_url.get()),
+                                  kSecCSDefaultFlags,
+                                  code_.InitializeInto()) != errSecSuccess)
+    return false;
+
+  if (has_requirement_) {
+    base::scoped_nsobject<NSString> req_str([[NSString alloc]
+        initWithCString:requirement_str_.c_str()
+               encoding:NSUTF8StringEncoding]);
+    if (!req_str)
+      return false;
+    if (SecRequirementCreateWithString(
+            base::mac::NSToCFCast(req_str.get()), kSecCSDefaultFlags,
+            requirement_.InitializeInto()) != errSecSuccess)
+      return false;
+  }
+
+  return true;
+}
+
+bool MacSignatureEvaluator::PerformEvaluation(
+    ClientIncidentReport_IncidentData_OSXBinaryIntegrityIncident* incident) {
+  base::ScopedCFTypeRef<CFErrorRef> errors(nullptr);
+  OSStatus err = SecStaticCodeCheckValidityWithErrors(
+      code_, kSecCSCheckAllArchitectures, requirement_,
+      errors.InitializeInto());
+  if (err != errSecSuccess) {
+    incident->set_file_basename(path_.BaseName().value());
+    incident->set_sec_error(err);
+    // This adds the signature of the main binary to the sub incidents list.
+    base::scoped_nsobject<NSString> code_str([[NSString alloc]
+        initWithCString:path_.value().c_str()
+               encoding:NSUTF8StringEncoding]);
+    if (code_str)
+      report_altered_files(code_str, incident);
+    if (errors) {

[Mark Mentovai, 2015/10/05 15:02:11]

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?

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

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

+      NSDictionary* info = [base::mac::CFToNSCast(errors.get()) userInfo];
+      if (id detail = [info
+              objectForKey:base::mac::CFToNSCast(kSecCFErrorResourceAltered)])
+        report_altered_files(detail, incident);

[Mark Mentovai, 2015/10/05 15:02:11]

{} since the condition got long and wrapped weirdly.

[Greg K, 2015/10/07 22:54:29]

Done.

+      if (id detail = [info

[Mark Mentovai, 2015/10/05 15:02:11]

Loop over a list of interesting keys?

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

Done.

+              objectForKey:base::mac::CFToNSCast(kSecCFErrorResourceMissing)])
+        report_altered_files(detail, incident);
+    }
+  }
+  return err == errSecSuccess;

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

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.

[Greg K, 2015/10/07 22:54:29]

Done.

+}
+
+}  // namespace safe_browsing