Reauthorize Keychain to Replace Developer ID Certificate

chrome/browser/mac/keychain_reauthorize.mm

+// Copyright 2017 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"
+
+#import <Foundation/Foundation.h>
+#include <Security/Security.h>
+
+#include <string.h>
+
+#include "base/logging.h"
+#include "base/mac/foundation_util.h"
+#include "base/mac/scoped_cftyperef.h"
+#include "base/metrics/histogram.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/scoped_generic.h"
+#include "components/os_crypt/keychain_password_mac.h"
+#include "crypto/apple_keychain.h"
+
+namespace chrome {
+
+namespace {
+
+// Reauthorizes the Safe Storage keychain item, which protects the randomly
+// generated password that encrypts the user's saved passwords. This reads out
+// the keychain item, deletes it, and re-adds it to the keychain. This works
+// because the keychain uses an apps designated requirement as the default ACL

[Mark Mentovai, 2017/03/01 22:00:30]

“default ACL for”→“ACL for reading”

[Mark Mentovai, 2017/03/01 22:00:31]

app’s

[Greg K, 2017/03/02 01:28:27]

Done.

[Greg K, 2017/03/02 01:28:27]

Done.

+// for an item. We are signing Chrome with the current certificate (so it can

[Mark Mentovai, 2017/03/01 22:00:31]

Don’t say “we.” Also, the bit about signing will become stale, because signing
happens outside of this codebase. So this should just say that Chrome will be
signed with a designated requirement that allows the item to be read by Chromes
signed with both the old and new certificates.

[Greg K, 2017/03/02 01:28:27]

Done.

+// access the item), but using a designated requirement that accepts the current
+// and new certificates as valid.
+bool KeychainReauthorize() {
+  base::ScopedCFTypeRef<SecKeychainItemRef> storage_item;
+  UInt32 pw_length = 0;
+  void* password_data = nullptr;
+
+  crypto::AppleKeychain keychain;
+  OSStatus error = keychain.FindGenericPassword(
+      NULL, strlen(KeychainPassword::service_name),

[Mark Mentovai, 2017/03/01 22:00:31]

Use nullptr now in new code. Hooray for C++11!

[Greg K, 2017/03/02 01:28:27]

Done.

+      KeychainPassword::service_name, strlen(KeychainPassword::account_name),

[Mark Mentovai, 2017/03/01 22:00:31]

If git cl format formatted this, then it’s fine because I guess I’m required to
say so, but if it didn’t format this, then I’ll say that it’s ugly.

[Greg K, 2017/03/02 01:28:27]

Done.

+      KeychainPassword::account_name, &pw_length, &password_data,
+      storage_item.InitializeInto());
+
+  std::string password =

[Mark Mentovai, 2017/03/01 22:00:30]

One of the sanitizers may not like this if !password_data, and !password_data
seems to be a possibility based on line 46.

[Greg K, 2017/03/02 01:28:27]

Done.

+      std::string(static_cast<char*>(password_data), pw_length);
+  if (password_data) {
+    memset(password_data, 0xAA, pw_length);

[Mark Mentovai, 2017/03/01 22:00:31]

This is good. memset_s() would be even better. But you never bother scrambling
the std::string version of password, so it kind of doesn’t even matter that
you’ve done this. You should scramble any copies that you make.

[Greg K, 2017/03/02 01:28:27]

memset_s requires some LIBC extension, but let's definitely scramble the string.

+    keychain.ItemFreeContent(nullptr, password_data);
+  }
+
+  if (error != noErr)
+    return false;

[Mark Mentovai, 2017/03/01 22:00:30]

Wanna say something about this?

[Greg K, 2017/03/02 01:28:27]

Done.

+
+  if (keychain.ItemDelete(storage_item) != noErr) {
+    LOG(ERROR) << "KeychainReauthorize failed. Cannot delete item.";

[Mark Mentovai, 2017/03/01 22:00:31]

Aha! Whenever you’re logging something about an OSStatus like you are here, you
can use OSSTATUS_LOG from base/mac/mac_logging.h. The log message will include a
description of what went wrong. It’s amazing!

+    return false;
+  }
+
+  error = keychain.AddGenericPassword(

[Mark Mentovai, 2017/03/01 22:00:31]

I’m pretty sure that the answer is no, but we can’t Find-Add without a Delete in
the middle, right?

[Greg K, 2017/03/02 01:28:27]

Definitely good to test, but it failed.

+      NULL, strlen(KeychainPassword::service_name),
+      KeychainPassword::service_name, strlen(KeychainPassword::account_name),
+      KeychainPassword::account_name, password.size(), password.data(), NULL);
+
+  if (error != noErr) {
+    LOG(ERROR) << "Failed to re-add storage password.";
+    return false;
+  }
+  return true;
+}
+
+}  // namespace
+
+void KeychainReauthorizeIfNeeded(NSString* pref_key, int max_tries) {
+  NSUserDefaults* user_defaults = [NSUserDefaults standardUserDefaults];
+  [user_defaults synchronize];

[Mark Mentovai, 2017/03/01 22:00:31]

I don’t believe that this one will do anything, because you haven’t written
anything yet.

[Greg K, 2017/03/02 01:28:27]

Done.

+  int pref_value = [user_defaults integerForKey:pref_key];
+
+  if (pref_value >= max_tries)
+    return;
+
+  BOOL success_value =
+      [user_defaults boolForKey:[pref_key stringByAppendingString:@"Success"]];
+  if (success_value)
+    return;
+
+  if (pref_value > 0) {
+    // Logs the number of previous tries that didn't complete.
+    if (base::mac::AmIBundled()) {
+      UMA_HISTOGRAM_COUNTS("OSX.KeychainReauthorizeIfNeeded", pref_value);
+    } else {
+      UMA_HISTOGRAM_COUNTS("OSX.KeychainReauthorizeIfNeededAtUpdate",
+                           pref_value);
+    }
+  }
+
+  bool success = KeychainReauthorize();
+
+  ++pref_value;

[Mark Mentovai, 2017/03/01 22:00:31]

The comment in the header was explicit that this would happen BEFORE the
reauthorize attempt. It’s nice to do it before, because if KeychainReauthorize()
turns out to be crashy, just launch Chrome a few more times to recover.

[Greg K, 2017/03/02 01:28:27]

That makes sense, I didn't think about that.

+  [user_defaults setInteger:pref_value forKey:pref_key];
+  [user_defaults synchronize];
+
+  if (!success)
+    return;
+
+  NSString* success_pref_key = [pref_key stringByAppendingString:@"Success"];

[Mark Mentovai, 2017/03/01 22:00:31]

You already computed this above, on line 82. You should only do it once.

[Greg K, 2017/03/02 01:28:27]

Done.

+  [user_defaults setBool:YES forKey:success_pref_key];
+  [user_defaults synchronize];
+
+  // Logs the try number (1, 2) that succeeded.
+  if (base::mac::AmIBundled()) {
+    UMA_HISTOGRAM_COUNTS("OSX.KeychainReauthorizeIfNeededSuccess", pref_value);
+  } else {
+    UMA_HISTOGRAM_COUNTS("OSX.KeychainReauthorizeIfNeededAtUpdateSuccess",
+                         pref_value);
+  }
+}
+
+}  // namespace chrome