Implement Keychain reauthorization

chrome/browser/mac/keychain_reauthorize.cc

Index: chrome/browser/mac/keychain_reauthorize.cc
===================================================================
--- chrome/browser/mac/keychain_reauthorize.cc	(revision 0)
+++ chrome/browser/mac/keychain_reauthorize.cc	(revision 0)
@@ -0,0 +1,282 @@
+// Copyright (c) 2012 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/mac/keychain_reauthorize.h"
+
+#include <Security/Security.h>
+
+#include <algorithm>
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/mac/foundation_util.h"
+#include "base/mac/scoped_cftyperef.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/stringprintf.h"
+#include "base/sys_string_conversions.h"
+#include "chrome/browser/mac/security_wrappers.h"
+
+namespace chrome {
+namespace browser {
+namespace mac {
+
+void KeychainReauthorize() {
+  // See the designated requirement for a signed released build:
+  // codesign -d -r- "Google Chrome.app"
+  //
+  // Export the certificates from a signed released build:
+  // codesign -v --extract-certificates=/tmp/cert. "Google Chrome.app"
+  // (The extracted leaf certificate is at /tmp/cert.0; intermediates and root
+  // are at successive numbers.)
+  //
+  // Show some information about the exported certificates:
+  // openssl x509 -inform DER -in /tmp/cert.0 -noout -text -fingerprint
+  // (The "SHA1 Fingerprint" value printed by -fingerprint should match the
+  // hash used in a codesign designated requirement after allowing for obvious
+  // formatting differences.)
+
+  const char* const kIdentifierMatches[] = {
+#if defined(GOOGLE_CHROME_BUILD)
+    "com.google.Chrome",
+    "com.google.Chrome.canary",
+#else
+    "org.chromium.Chromium",
+#endif
+  };
+
+  const char* const kLeafCertificateHashMatches[] = {
+    // Only official released builds of Google Chrome have ever been signed
+    // (with a certificate that anyone knows about or cares about).
+#if defined(GOOGLE_CHROME_BUILD)
+    // This is the new certificate that has not yet been used to sign Chrome,
+    // but will be. Once used, the reauthorization code will become obsolete
+    // until it's needed for some other purpose in the future.
+    // Subject: UID=EQHXZ8M8AV, CN=Developer ID Application: Google Inc.,
+    //     OU=EQHXZ8M8AV, O=Google Inc., C=US
+    // Issuer: CN=Developer ID Certification Authority,
+    //     OU=Apple Certification Authority, O=Apple Inc., C=US
+    // Validity: 2012-04-26 14:10:10 UTC to 2017-04-27 14:10:10 UTC
+    // "85cee8254216185620ddc8851c7a9fc4dfe120ef",
+
+    // This certificate was used on 2011-12-20 and 2011-12-21, but the one
+    // below was restored afterwards as an interim fix to the Keychain

[stuartmorgan, 2012/05/10 06:50:54]

"the one below" is ambiguous, especially since it could easily be the same thing
as "this certificate" given the length of the comment.

+    // authorization problem.

[stuartmorgan, 2012/05/10 06:50:54]

Worth a bug ref? I'm guessing there are about three of us who know what this is
referring to.

+    // Subject: C=US, ST=California, L=Mountain View, O=Google Inc,
+    //     OU=Digital ID Class 3 - Java Object Signing, CN=Google Inc
+    // Issuer: C=US, O=VeriSign, Inc., OU=VeriSign Trust Network,
+    //     OU=Terms of use at https://www.verisign.com/rpa (c)10,
+    //     CN=VeriSign Class 3 Code Signing 2010 CA
+    // Validity: 2011-11-14 00:00:00 UTC to 2014-11-13 23:59:59 UTC
+    "06c92bec3bbf32068cb9208563d004169448ee21",
+
+    // This certificate has been used since 2010-07-19, except for the brief
+    // period when the certificate above was used.
+    // Subject: C=US, ST=California, L=Mountain View, O=Google Inc,
+    //     OU=Digital ID Class 3 - Java Object Signing, CN=Google Inc
+    // Issuer: C=US, O=VeriSign, Inc., OU=VeriSign Trust Network,
+    //    OU=Terms of use at https://www.verisign.com/rpa (c)09,
+    //    CN=VeriSign Class 3 Code Signing 2009-2 CA
+    // Validity: 2010-02-22 00:00:00 UTC to 2012-02-22 23:59:59 UTC
+    "9481882581d8178db8b1649c0eaa4f9eb11288f0",
+
+    // This certificate was used for all public Chrome releases prior to
+    // 2010-07-19.
+    // Subject: C=US, ST=California, L=Mountain View, O=Google Inc,
+    //     OU=Digital ID Class 3 - Netscape Object Signing, CN=Google Inc
+    // Issuer: C=US, O=VeriSign, Inc., OU=VeriSign Trust Network,
+    //     OU=Terms of use at https://www.verisign.com/rpa (c)04,
+    //     CN=VeriSign Class 3 Code Signing 2004 CA
+    // Validity: 2007-06-19 00:00:00 UTC to 2010-06-18 23:59:59 UTC
+    "fe5008fe0da7a2033816752d6eafe95214f5a7e1",
+#endif
+  };
+
+  std::vector<std::string> requirement_matches;
+  requirement_matches.reserve(arraysize(kIdentifierMatches) *
+                              ARRAYSIZE_UNSAFE(kLeafCertificateHashMatches));
+
+  for (size_t identifier_index = 0;
+       identifier_index < arraysize(kIdentifierMatches);
+       ++identifier_index) {
+    for (size_t leaf_certificate_hash_index = 0;
+         leaf_certificate_hash_index <
+             ARRAYSIZE_UNSAFE(kLeafCertificateHashMatches);
+         ++leaf_certificate_hash_index) {
+      requirement_matches.push_back(base::StringPrintf(
+          "identifier \"%s\" and certificate leaf = H\"%s\"",
+          kIdentifierMatches[identifier_index],
+          kLeafCertificateHashMatches[leaf_certificate_hash_index]));
+    }
+  }
+
+  ScopedSecKeychainSetUserInteractionAllowed user_interaction_allowed(FALSE);
+
+  // Apple's documentation (Keychain Services Reference, Constants/Mac OS X
+  // Keychain Services API Constants/Keychain Item Class Constants) says to
+  // use CSSM_DL_DB_RECORD_ALL_KEYS, but that doesn't work.
+  // CSSM_DL_DB_RECORD_ANY (as used by SecurityTool's keychain-dump) does
+  // work.
+  base::mac::ScopedCFTypeRef<SecKeychainSearchRef> search(
+    CrSKeychainSearchCreateFromAttributes(NULL,
+                                          CSSM_DL_DB_RECORD_ANY,
+                                          NULL));
+
+  base::mac::ScopedCFTypeRef<SecTrustedApplicationRef> this_application;
+
+  std::vector<CrSKeychainItemAndAccess> items;
+
+  base::mac::ScopedCFTypeRef<SecKeychainItemRef> item;
+  while (item.reset(CrSKeychainSearchCopyNext(search)), item) {
+    if (!CrSKeychainItemTestAccess(item)) {
+      continue;
+    }

[stuartmorgan, 2012/05/10 06:50:54]

Chromium style discourages single-line braces; it would be nice if all this code
followed that since it's not in a file that already has the other style.

+
+    base::mac::ScopedCFTypeRef<SecAccessRef> access(
+        CrSKeychainItemCopyAccess(item));
+    base::mac::ScopedCFTypeRef<CFArrayRef> acl_list(
+        CrSAccessCopyACLList(access));
+    if (!acl_list) {
+      continue;
+    }
+
+    bool acl_list_modified = false;
+
+    CFIndex acl_count = CFArrayGetCount(acl_list);
+    for (CFIndex acl_index = 0; acl_index < acl_count; ++acl_index) {

[stuartmorgan, 2012/05/10 06:50:54]

The layers of if statements in a double for-loop is pretty opaque. I think this
block would benefit a lot from an introductory comment explaining roughly what
it's doing.

In fact, could you reasonably extract most or all of this loop into a well-named
helper function that returns a bool for having modified or something similar? If
it's feasible (it's hard to see how many arguments such a function would need),
that could help readability of the function quite a bit.

+      SecACLRef acl = base::mac::CFCast<SecACLRef>(
+          CFArrayGetValueAtIndex(acl_list, acl_index));
+      scoped_ptr<CrSACLSimpleContents> acl_simple_contents(
+          CrSACLCopySimpleContents(acl));
+      if (!acl_simple_contents.get() ||
+          !acl_simple_contents->application_list) {
+        continue;
+      }
+
+      CFMutableArrayRef application_list_mutable = NULL;
+      bool added_this_application = false;
+
+      CFIndex application_count =
+          CFArrayGetCount(acl_simple_contents->application_list);
+      for (CFIndex application_index = 0;
+           application_index < application_count;
+           ++application_index) {
+        SecTrustedApplicationRef application =
+            base::mac::CFCast<SecTrustedApplicationRef>(
+                CFArrayGetValueAtIndex(acl_simple_contents->application_list,
+                                       application_index));
+        base::mac::ScopedCFTypeRef<SecRequirementRef> requirement(
+            CrSTrustedApplicationCopyRequirement(application));
+        base::mac::ScopedCFTypeRef<CFStringRef> requirement_string_cf(
+            CrSRequirementCopyString(requirement, kSecCSDefaultFlags));
+        if (!requirement_string_cf) {
+          continue;
+        }
+
+        std::string requirement_string =
+            base::SysCFStringRefToUTF8(requirement_string_cf);
+        if (std::find(requirement_matches.begin(),
+                      requirement_matches.end(),
+                      requirement_string) != requirement_matches.end()) {
+          if (!application_list_mutable) {
+            application_list_mutable =
+                CFArrayCreateMutableCopy(NULL,
+                                         application_count,
+                                         acl_simple_contents->application_list);
+            acl_simple_contents->application_list.reset(
+                application_list_mutable);
+          }
+
+          if (!this_application) {
+            this_application.reset(CrSTrustedApplicationCreateFromPath(NULL));

[stuartmorgan, 2012/05/10 06:50:54]

Is it really expensive enough to do this (and unlikely enough to happen at some
point) that it's worth doing lazily instead of just up-front? It's a small
thing, but it's extra complexity in an already-complex function.

+          }
+
+          if (!added_this_application) {

[stuartmorgan, 2012/05/10 06:50:54]

I find
  if (!foo)
  else
to generally be less readable than the reverse.

[Mark Mentovai, 2012/05/14 21:28:45]

stuartmorgan wrote:
In this case, !added_this_application always happens before
added_this_application, and I think it makes sense to structure it according to
“the thing that happens first come first in the file.”

+            CFArraySetValueAtIndex(application_list_mutable,
+                                   application_index,
+                                   this_application);
+            added_this_application = true;
+          } else {
+            // Even though it's more bookkeeping to walk a list in the forward
+            // direction when there are removals, it's done here anyway to
+            // keep this_application at the position of the first match.
+            CFArrayRemoveValueAtIndex(application_list_mutable,
+                                      application_index);
+            --application_index;
+            --application_count;
+          }
+        }
+      }
+
+      if (application_list_mutable) {
+        if (!CrSACLSetSimpleContents(acl, *acl_simple_contents.get())) {
+          continue;
+        }
+
+        acl_list_modified = true;
+      }
+    }
+
+    if (acl_list_modified) {
+      items.push_back(CrSKeychainItemAndAccess(item, access));
+    }
+  }
+
+  for (std::vector<CrSKeychainItemAndAccess>::const_iterator iterator =
+           items.begin();
+       iterator != items.end();
+       ++iterator) {
+    SecKeychainItemRef old_item = iterator->item();
+    base::mac::ScopedCFTypeRef<SecKeychainRef> keychain(
+        CrSKeychainItemCopyKeychain(old_item));
+
+    ScopedCrSKeychainItemAttributesAndData old_attributes_and_data(
+        CrSKeychainItemCopyAttributesAndData(keychain, old_item));
+    if (!old_attributes_and_data.get()) {
+      continue;
+    }
+
+    // SecKeychainItemCreateFromContent fails if any attribute is zero-length,
+    // but old_attributes_and_data can contain zero-length attributes. Create
+    // a new attribute list devoid of zero-length attributes.

[stuartmorgan, 2012/05/10 06:50:54]

Could this whole step be a helper function?

+    UInt32 old_attribute_count =
+        old_attributes_and_data.attribute_list()->count;
+    std::vector<SecKeychainAttribute> new_attributes;
+    new_attributes.reserve(old_attribute_count);
+    for (UInt32 old_attribute_index = 0;
+         old_attribute_index < old_attribute_count;
+         ++old_attribute_index) {
+      SecKeychainAttribute* attribute =
+          &old_attributes_and_data.attribute_list()->attr[old_attribute_index];
+      if (attribute->length) {
+        new_attributes.push_back(*attribute);
+      }
+    }
+
+    SecKeychainAttributeList new_attribute_list;
+    new_attribute_list.count = new_attributes.size();
+    new_attribute_list.attr =
+        new_attribute_list.count ? &new_attributes[0] : NULL;
+
+    CrSKeychainItemAttributesAndData new_attributes_and_data =
+        *old_attributes_and_data.get();
+    new_attributes_and_data.attribute_list = &new_attribute_list;
+
+    // Delete the item last, to give everything else above a chance to bail
+    // out early, and to ensure that the old item is still present while it
+    // may still be used by the above code.
+    if (!CrSKeychainItemDelete(old_item)) {
+      continue;
+    }
+
+    base::mac::ScopedCFTypeRef<SecKeychainItemRef> new_item(
+        CrSKeychainItemCreateFromContent(new_attributes_and_data,
+                                         keychain,
+                                         iterator->access()));
+  }
+}
+
+}  // namespace mac
+}  // namespace browser
+}  // namespace chrome