Allow whitelisted bundles to load in Chrome

chrome/common/mac/cfbundle_blocker.mm

Index: chrome/common/mac/cfbundle_blocker.mm
===================================================================
--- chrome/common/mac/cfbundle_blocker.mm	(revision 105082)
+++ chrome/common/mac/cfbundle_blocker.mm	(working copy)
@@ -60,13 +60,6 @@
     // Everything in the suffix list has a trailing slash so as to only block
     // loading things contained in these directories.
     NSString* const blocked_suffixes[] = {
-      // SIMBL - http://code.google.com/p/simbl/source/browse/src/SIMBL.{h,m}.
-      // It attempts to inject itself via an AppleScript event.
-      // http://code.google.com/p/simbl/source/browse/SIMBL%20Agent/SIMBLAgent.m
-      // Blocking input managers and scripting additions may already be enough
-      // to prevent SIMBL plugins from loading.
-      @"Application Support/SIMBL/Plugins/",
-
 #if !defined(__LP64__)
       // Contextual menu manager plug-ins are unavailable to 64-bit processes.
       // http://developer.apple.com/library/mac/releasenotes/Cocoa/AppKitOlderNotes.html#NSMenu
@@ -146,10 +139,111 @@
   return false;
 }
 
-// Returns true if bundle_id identifies a bundle that is allowed to be loaded
+struct AllowedBundle {
+  // The bundle identifier to permit. These are matched with a case-sensitive
+  // literal comparison. "Children" of the declared bundle ID are permitted:
+  // if bundle_id here is @"org.chromium", it would match both @"org.chromium"
+  // and @"org.chromium.Chromium".
+  NSString* bundle_id;
+
+  // If bundle_id should only be permitted as of a certain minimum version,
+  // this string defines that version, which will be compared to the bundle's
+  // version with a numeric comparison. If bundle_id may be permitted at any
+  // version, set minimum_version to nil.
+  NSString* minimum_version;
+};
+
+// Returns true if |bundle| identifies a bundle that is allowed to be loaded
 // even when found in a blocked directory.
-bool IsBundleIDAllowed(NSString* bundle_id) {
-  return [bundle_id isEqualToString:@"com.google.osax.Google_Authenticator_BT"];
+bool IsBundleAllowed(CFBundleRef bundle) {
+  // The list of bundles that are allowed to load. Before adding an entry to
+  // this list, be sure that it's well-behaved. Specifically, anything that
+  // uses mach_override
+  // (https://github.com/rentzsch/mach_star/tree/master/mach_override) must
+  // use version 51ae3d199463fa84548f466d649f0821d579fdaf (July 22, 2011) or
+  // newer. Entries should include the name of the product, URL, and the name
+  // and e-mail address of someone responsible for the product's engineering.
+  // To add items to this list, file a bug at http://new.crbug.com/ using the
+  // "Defect on Mac OS" template, and provide the bundle ID (or IDs) and
+  // minimum CFBundleVersion that's safe for Chrome to load, along with the
+  // necessary product and contact information. Whitelisted bundles in this
+  // list may be removed if they are found to cause instability or otherwise
+  // behave badly. With proper contact information, Chrome developers may try
+  // to contact maintainers to resolve any problems.
+  const AllowedBundle kAllowedBundles[] = {
+    // Google Authenticator BT
+    // dmaclach

[Scott Hess - ex-Googler, 2011/10/12 19:34:34]

Expanding this like other email addresses won't kill anyone, and makes sense in
case of external contributors needing to ask questions.  Sorry dave.

+    { @"com.google.osax.Google_Authenticator_BT", nil },
+
+    // Default Folder X, http://www.stclairsoft.com/DefaultFolderX/
+    // Jon Gotow <gotow@stclairsoft.com>
+    { @"com.stclairsoft.DefaultFolderX", @"4.4.3" },
+
+    // MySpeed, http://www.enounce.com/myspeed
+    // Edward Bianchi <ejbianchi@enounce.com>
+    { @"com.enounce.MySpeed.osax", @"1201" },
+
+    // SIMBL (fork), https://github.com/albertz/simbl
+    // Albert Zeyer <albzey@googlemail.com>
+    { @"net.culater.SIMBL", nil },
+
+    // Smart Scroll, http://marcmoini.com/sx_en.html
+    // Marc Moini <marc@a9ff.com>
+    { @"com.marcmoini.SmartScroll", @"3.9" },
+  };
+
+  CFStringRef identifier_cf = CFBundleGetIdentifier(bundle);
+  NSString* identifier_ns = base::mac::CFToNSCast(identifier_cf);

[Scott Hess - ex-Googler, 2011/10/12 19:34:34]

It might be worthwhile to pull everything outside of CFBundleGetIdentifier()
into a separate function, which could be unit-tested.  Once you've fetched the
bundle id to pass in, I bet fetching the version wouldn't make things materially
slower.

+
+  for (size_t index = 0; index < arraysize(kAllowedBundles); ++index) {
+    const AllowedBundle& allowed_bundle = kAllowedBundles[index];
+    NSString* allowed_bundle_id = allowed_bundle.bundle_id;
+
+    // Try a direct equality test.
+    bool bundle_id_match = [identifier_ns isEqualToString:allowed_bundle_id];
+
+    if (!bundle_id_match) {
+      // Permit bundle identifiers that are "children" of the allowed
+      // identifier.

[Scott Hess - ex-Googler, 2011/10/12 19:34:34]

Would it be reasonable to make "children" be a different id than a straight
match?  Maybe @"com.example.myapp" would match that exactly, but
@"com.example.myapp.*" would match children?

I wasn't going to suggest this, but AFAICT it would only add a line or two of
code to implement.  [I only expected two or three lines, but it _is_ objective-c
:-).]

[Mark Mentovai, 2011/10/12 23:48:50]

shess wrote:
At least two of the things in the list now need to match both the base ID and
children, so with wildcards, they’d turn into two entries. The other option
would be to have a third field in the struct, either a “bool allow_children” or
an “enum { kExactMatch, KOnlyAllowChildren, kExactMatchOrChildren}”. I think
these are more than a line or two of code, though.

+      NSUInteger bundle_id_length = [identifier_ns length];
+      NSUInteger allowed_bundle_id_length = [allowed_bundle_id length];
+      if (bundle_id_length > allowed_bundle_id_length &&
+          [identifier_ns characterAtIndex:allowed_bundle_id_length] == '.') {
+        NSComparisonResult result =
+            [identifier_ns compare:allowed_bundle_id
+                           options:NSLiteralSearch
+                             range:NSMakeRange(0, allowed_bundle_id_length)];

[Scott Hess - ex-Googler, 2011/10/12 19:34:34]

-hasPrefix:?  Unless NSLiteralSearch implies correctness differences, I guess. 
Of course this busts my above suggestion, but you might just ignore that anyhow.

Actually, you could pull a -hasPrefix: test up where -isEqualToString: is, and
then qualify success by requiring either matching lengths or next char is a '.'.

+        bundle_id_match = result == NSOrderedSame;
+      }
+    }
+
+    if (bundle_id_match) {
+      NSString* minimum_version = allowed_bundle.minimum_version;
+      if (!minimum_version) {
+        // If the rule didn't declare any version requirement, the bundle is
+        // allowed to load.
+        return true;
+      }
+
+      NSDictionary* bundle_dictionary =
+          base::mac::CFToNSCast(CFBundleGetInfoDictionary(bundle));
+      NSString* version = [bundle_dictionary objectForKey:
+          base::mac::CFToNSCast(kCFBundleVersionKey)];
+      if (![version isKindOfClass:[NSString class]]) {
+        // If there wasn't any version but one was required, the bundle isn't
+        // allowed to load.
+        return false;
+      }
+
+      // A numeric search is appropriate for comparing version numbers.
+      NSComparisonResult result = [version compare:minimum_version
+                                           options:NSNumericSearch];

[Scott Hess - ex-Googler, 2011/10/12 19:34:34]

OMG.  I mean, good job, Apple!

[Mark Mentovai, 2011/10/12 23:48:50]

shess wrote:
I hear that it might not have worked properly in 10.4, but we don’t care.

The new unit test will catch unexpected behavior.

+      return result != NSOrderedAscending;
+    }
+  }
+
+  // Nothing matched.
+  return false;
 }
 
 typedef Boolean (*_CFBundleLoadExecutableAndReturnError_Type)(CFBundleRef,
@@ -176,7 +270,7 @@
   CFStringRef identifier_cf = CFBundleGetIdentifier(bundle);
   NSString* identifier_ns = base::mac::CFToNSCast(identifier_cf);
 
-  if (IsBundlePathBlocked(path_ns) && !IsBundleIDAllowed(identifier_ns)) {
+  if (IsBundlePathBlocked(path_ns) && !IsBundleAllowed(bundle)) {
     NSString* identifier_ns_print = identifier_ns ? identifier_ns : @"(nil)";
 
     LOG(INFO) << "Blocking attempt to load bundle "