Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rest of Muon import fails after denying keychain access #2513

Closed
garrettr opened this issue Dec 12, 2018 · 2 comments · Fixed by brave/brave-core#1081
Closed

Rest of Muon import fails after denying keychain access #2513

garrettr opened this issue Dec 12, 2018 · 2 comments · Fixed by brave/brave-core#1081

Comments

@garrettr
Copy link
Contributor

garrettr commented Dec 12, 2018

Description

Separate issue for "Scenario M - Upgrade from muon but then deny keychain access in b-c" from #2438.

Steps to Reproduce

  1. Create browser-laptop profile with History, Bookmarks, Passwords, Cookies, Stats, Payments.
  2. Start brave-core and import all possible data types "Brave (old)", via either Import bookmarks and settings..." or the --upgrade-from-muon command line flag.
  3. At the keychain access prompt for "Brave Safe Storage," enter your password and click Allow or Always Allow.
  4. At the keychain access prompt for "Chromium Safe Storage" (notice this is being requested by a Helper process, aka the importer utility process), click Deny.

Actual result:

The following data types ARE imported: History, Bookmarks.
The following ARE NOT imported: Passwords, Cookies, Stats, Rewards, Open windows and tabs.

Expected result:

The following data types SHOULD be imported: History, Bookmarks, Stats, Rewards, Open windows and tabs.
The following data types SHOULD NOT be imported: Passwords, Cookies.

Passwords and Cookies are encrypted with a keychain password on some platforms (macOS, Linux) so if you see a keychain prompt and deny access, you should expect that these data types are not imported because there is no way to decrypt them. However, all other data types should be imported in this scenario.

Reproduces how often:

Easily reproduced.

Brave version (brave://version info)

Reproducible on current release:

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

Additional Information

The subset of data types that fail to import when keychain access is denied corresponds to the order of import functions as they are called in BraveImporter::StartImport, which suggests that an error in one of the functions that requires keychain access is crashing the importer utility process and thus preventing the rest of the import functions from executing.

@garrettr garrettr self-assigned this Dec 12, 2018
@garrettr
Copy link
Contributor Author

Manual testing with a debug build shows the utility process crashes with the following stack trace:

Received signal 11 SEGV_MAPERR 000000000000
0   libbase.dylib                       0x000000010f3bf5fe base::debug::StackTrace::StackTrace(unsigned long) + 174
1   libbase.dylib                       0x000000010f3bf6bd base::debug::StackTrace::StackTrace(unsigned long) + 29
2   libbase.dylib                       0x000000010ef90d3a base::debug::StackTrace::StackTrace() + 26
3   libbase.dylib                       0x000000010f3bf3f1 base::debug::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, void*) + 1393
4   libsystem_platform.dylib            0x00007fff778deb3d _sigtramp + 29
5   ???                                 0x0000000000000000 0x0 + 0
6   libos_crypt.dylib                   0x000000014dab497a (anonymous namespace)::GetEncryptionKey() + 3082
7   libos_crypt.dylib                   0x000000014dab7bc1 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
8   libos_crypt.dylib                   0x000000014dab6926 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
9   libchrome_dll.dylib                 0x0000000118c5215c 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
10  libchrome_dll.dylib                 0x0000000118c3e2b4 password_manager::LoginDatabase::InitPasswordFormFromStatement(autofill::PasswordForm*, sql::Statement const&) const + 644
11  libchrome_dll.dylib                 0x0000000118c3ce65 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
12  libchrome_dll.dylib                 0x0000000118c45223 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
13  libchrome_dll.dylib                 0x0000000118c44df1 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
14  libchrome_dll.dylib                 0x0000000113161575 ChromeImporter::ImportPasswords(base::FilePath const&) + 789
15  libchrome_dll.dylib                 0x000000011314758a BraveImporter::StartImport(importer::SourceProfile const&, unsigned short, ImporterBridge*) + 538
16  libchrome_dll.dylib                 0x000000011b402418 void base::internal::FunctorTraits<void (Importer::*)(importer::SourceProfile const&, unsigned short, ImporterBridge*), void>::Invoke<void (Importer::*)(importer::SourceProfile const&, unsigned short, ImporterBridge*), scoped_refptr<Importer>, importer::SourceProfile, unsigned short, ExternalProcessImporterBridge*>(void (Importer::*)(importer::SourceProfile const&, unsigned short, ImporterBridge*), scoped_refptr<Importer>&&, importer::SourceProfile&&, unsigned short&&, ExternalProcessImporterBridge*&&) + 184
17  libchrome_dll.dylib                 0x000000011b40229a void base::internal::InvokeHelper<false, void>::MakeItSo<void (Importer::*)(importer::SourceProfile const&, unsigned short, ImporterBridge*), scoped_refptr<Importer>, importer::SourceProfile, unsigned short, ExternalProcessImporterBridge*>(void (Importer::*&&)(importer::SourceProfile const&, unsigned short, ImporterBridge*), scoped_refptr<Importer>&&, importer::SourceProfile&&, unsigned short&&, ExternalProcessImporterBridge*&&) + 138
18  libchrome_dll.dylib                 0x000000011b4021dd void base::internal::Invoker<base::internal::BindState<void (Importer::*)(importer::SourceProfile const&, unsigned short, ImporterBridge*), scoped_refptr<Importer>, importer::SourceProfile, unsigned short, base::internal::RetainedRefWrapper<ExternalProcessImporterBridge> >, void ()>::RunImpl<void (Importer::*)(importer::SourceProfile const&, unsigned short, ImporterBridge*), std::__1::tuple<scoped_refptr<Importer>, importer::SourceProfile, unsigned short, base::internal::RetainedRefWrapper<ExternalProcessImporterBridge> >, 0ul, 1ul, 2ul, 3ul>(void (Importer::*&&)(importer::SourceProfile const&, unsigned short, ImporterBridge*), std::__1::tuple<scoped_refptr<Importer>, importer::SourceProfile, unsigned short, base::internal::RetainedRefWrapper<ExternalProcessImporterBridge> >&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) + 285
19  libchrome_dll.dylib                 0x000000011b402009 base::internal::Invoker<base::internal::BindState<void (Importer::*)(importer::SourceProfile const&, unsigned short, ImporterBridge*), scoped_refptr<Importer>, importer::SourceProfile, unsigned short, base::internal::RetainedRefWrapper<ExternalProcessImporterBridge> >, void ()>::RunOnce(base::internal::BindStateBase*) + 57

@garrettr garrettr changed the title Rest of import fails after denying keychain access Rest of "Brave (old)" import fails after denying keychain access Dec 13, 2018
@garrettr garrettr changed the title Rest of "Brave (old)" import fails after denying keychain access Rest of Muon import fails after denying keychain access Dec 13, 2018
@kjozwiak kjozwiak added this to the 0.58.x - Release milestone Dec 18, 2018
@LaurenWags
Copy link
Member

Verified passed with

Brave 0.58.15 Chromium: 71.0.3578.98 (Official Build) (64-bit)
Revision 15234034d19b85dcd9a03b164ae89d04145d8368-refs/branch-heads/3578@{#897}
OS Mac OS X
  • Verified STR from description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment