cookies not imported - follow up to 1545

[LaurenWags]

Description

Follow up to #1545

Using --upgrade-from-muon argument, cookies are not being manually imported.

Steps to Reproduce

1. Have an install of browser-laptop setup and configured (Release channel) with
   • bookmarks
   • history
   • cookies
   • saved passwords
2. Download/install the brave-core release candidate
3. Delete the brave-core profile (if one was created; sometimes installer will launch Brave after install)
4. Launch brave-core via CLI and make sure to pass --upgrade-from-muon
5. If prompted on macOS, allow access to your keychain.
6. See that cookies are not imported but bookmarks, history, passwords are.
7. Close b-c
8. Launch brave-core via CLI and make sure to pass --upgrade-from-muon
9. Cookies still not imported.
10. Attempt to manually import Cookies (on macOS, Brave > Import Bookmarks and Settings > choose Brave from drop list and only select cookies in the list). Allow keychain access if requested. Cookies are manually imported.

Actual result:

Cookies are not imported with --upgrade-from-muon

Expected result:

Cookies should be imported with --upgrade-from-muon

Reproduces how often:

easily

Brave version (brave://version info)

Brave	0.56.8 Chromium: 70.0.3538.77 (Official Build) (64-bit)
Revision	0f6ce0b0cd63a12cb4eccea3637b1bc9a29148d9-refs/branch-heads/3538@{#1039}
OS	Mac OS X

Reproducible on current release:

• Does it reproduce on brave-browser dev/beta builds?

Website problems only:

• Does the issue resolve itself when disabling Brave Shields?
• Is the issue reproducible on the latest version of Chrome?

Additional Information

Reproduced on Windows by @srirambv and @btlechowski
cc @garrettr @bsclifton

[garrettr]

This appears to affect saved passwords as well. When I reproduce with the latest build, I see the following DCHECK failure:

[17380:18179:1107/135923.202864:FATAL:os_crypt_mac.mm(103)] Check failed: g_key_creation_util.
0   libbase.dylib                       0x0000000103b085fe base::debug::StackTrace::StackTrace(unsigned long) + 174
1   libbase.dylib                       0x0000000103b086bd base::debug::StackTrace::StackTrace(unsigned long) + 29
2   libbase.dylib                       0x00000001036d9d3a base::debug::StackTrace::StackTrace() + 26
3   libbase.dylib                       0x0000000103747dbd logging::LogMessage::~LogMessage() + 461
4   libbase.dylib                       0x0000000103745955 logging::LogMessage::~LogMessage() + 21
5   libos_crypt.dylib                   0x000000013fc15744 (anonymous namespace)::GetEncryptionKey() + 2340
6   libos_crypt.dylib                   0x000000013fc18bc1 OSCrypt::DecryptString(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*) + 3105
7   libos_crypt.dylib                   0x000000013fc17926 OSCrypt::DecryptString16(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<unsigned short, base::string16_internals::string16_char_traits, std::__1::allocator<unsigned short> >*) + 310
8   libchrome_dll.dylib                 0x000000010efb61ac password_manager::LoginDatabase::DecryptedString(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<unsigned short, base::string16_internals::string16_char_traits, std::__1::allocator<unsigned short> >*) const + 44
9   libchrome_dll.dylib                 0x000000010efa2304 password_manager::LoginDatabase::InitPasswordFormFromStatement(autofill::PasswordForm*, sql::Statement const&) const + 644
10  libchrome_dll.dylib                 0x000000010efa0eb5 password_manager::LoginDatabase::StatementToForms(sql::Statement*, password_manager::PasswordStore::FormDigest const*, std::__1::vector<std::__1::unique_ptr<autofill::PasswordForm, std::__1::default_delete<autofill::PasswordForm> >, std::__1::allocator<std::__1::unique_ptr<autofill::PasswordForm, std::__1::default_delete<autofill::PasswordForm> > > >*) + 1541
11  libchrome_dll.dylib                 0x000000010efa9273 password_manager::LoginDatabase::GetAllLoginsWithBlacklistSetting(bool, std::__1::vector<std::__1::unique_ptr<autofill::PasswordForm, std::__1::default_delete<autofill::PasswordForm> >, std::__1::allocator<std::__1::unique_ptr<autofill::PasswordForm, std::__1::default_delete<autofill::PasswordForm> > > >*) + 1059
12  libchrome_dll.dylib                 0x000000010efa8e41 password_manager::LoginDatabase::GetAutofillableLogins(std::__1::vector<std::__1::unique_ptr<autofill::PasswordForm, std::__1::default_delete<autofill::PasswordForm> >, std::__1::allocator<std::__1::unique_ptr<autofill::PasswordForm, std::__1::default_delete<autofill::PasswordForm> > > >*) + 33
13  libchrome_dll.dylib                 0x00000001094dacd5 ChromeImporter::ImportPasswords(base::FilePath const&) + 789
14  libchrome_dll.dylib                 0x00000001094ce531 BraveImporter::StartImport(importer::SourceProfile const&, unsigned short, ImporterBridge*) + 529
<snip>

OSCrypt is used to decrypt both cookies and saved passwords, so if there is an issue with how we interact with oscrypt it would prevent the successful import of both data types.

[garrettr]

Likely related to https://chromium.googlesource.com/chromium/src/+/3ca917bd3ebea51bfcf7f8b80e39e217467153d4, which landed recently. That could explain why auto-importing cookies and passwords worked when it initially landed in brave/brave-core#729, but has stopped working recently—these changes would've come in from a recent Chromium rebase.

[garrettr]

The recent Chromium changes initialize oscrypt in ChromeBrowserMainPartsMac::PreProfileInit, which is called from PreMainMessageLoopRun. I think the correct solution here is to move brave::AutoImportMuon from PreMainMessageLoopRun to PostProfileInit.

[garrettr]

Tried this, didn't work. New theory: we need to call the new OSCrypt::Init in the utility process.

[garrettr]

I was testing this against 0.58.x (current master) yesterday; @bbondy pointed out there might be separate issues between 0.56.x and 0.58.x, especially if they were introduced in a recent Chromium rebase, and verifying/fixing this issue in 0.56.x should be first priority. Waiting on a build of 0.56.x now so I can repro/investigate/resolve.

[darkdh]

+// A utility which prevents overwriting the encryption key. This is temporary
+// pointer that is non-NULL between initialization and getting the encryption
+// key for the first time.
+os_crypt::EncryptionKeyCreationUtil* g_key_creation_util = nullptr;
+
 // This lock is used to make the GetEncrytionKey and
 // OSCrypt::GetRawEncryptionKey methods thread-safe.
 base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
@@ -76,9 +96,17 @@
     crypto::MockAppleKeychain keychain;
     password = keychain.GetEncryptionPassword();
   } else {
+#if defined(OS_IOS)
+    DCHECK(!g_key_creation_util);
+    g_key_creation_util = new os_crypt::EncryptionKeyCreationUtilIOS();
+#endif
+    DCHECK(g_key_creation_util);
     AppleKeychain keychain;
-    KeychainPassword encryptor_password(keychain);
+    KeychainPassword encryptor_password(
+        keychain,
+        std::unique_ptr<EncryptionKeyCreationUtil>(g_key_creation_util));
     password = encryptor_password.GetPassword();
+    g_key_creation_util = nullptr;
   }

DCHECK(g_key_creation_util) always fails if you are not on iOS and I'm not user if we can do OSCrypt::Init in utility process because it seems we don't have access to PrefService

[garrettr]

DCHECK(g_key_creation_util) always fails if you are not on iOS

@darkdh I don't think that's true, because it's initialized in OSCrypt::Init, which in turn is initialized before any code that uses OSCrypt—see https://chromium.googlesource.com/chromium/src/+/43615a9db0c2690640af16fd8c24cc014c046414%5E%21/.

[darkdh]

yeah, but for our importer hack, it cannot be initialized

[LaurenWags]

Verified passed on

Brave	0.56.14 Chromium: 70.0.3538.102 (Official Build) (64-bit)
Revision	4bbeebac88fdc09c97265e47c205868bbd190497-refs/branch-heads/3538@{#1077}
OS	Mac OS X

Verification Passed on

Brave	0.56.14 Chromium: 70.0.3538.102 (Official Build) (64-bit)
Revision	4bbeebac88fdc09c97265e47c205868bbd190497-refs/branch-heads/3538@{#1077}
OS	Windows