platformKeys.selectClientCertificates causing system crash with public-slot keys on Chrome 65.0.3325
Reported by
ldanx...@cisco.com,
May 3 2018
|
|||||||||||
Issue descriptionChrome OS Version: Chrome65.0.3325 Steps To Reproduce: (1) Call platformKeys.selectClientCertificates (2) Select certificate that has been imported using the "Import" button on chrome://settings/certificates (and thus is stored on the user's public slot). Expected Result: No crash Actual Result: Chrome device crashes How frequently does this problem reproduce? (Always, sometimes, hard to reproduce?) Always What is the impact to the user, and is there a workaround? If so, what is it? Import client certificates using "Import and Bind" instead of "Import".
,
May 4 2018
Also, does it crash on all devices or only on some devices? And does it only crash after some action (e.g. after importing a specific certificate)?
,
May 4 2018
It crashes consistently on two devices running 65.0.3325. I have another device set at the dev channel that does not exhibit the crash. I don't see anything recent in chrome://crashes. Is there some flag I need to enable? Also FYI, these 2 devices were put into developer mode. I don't know if that would cause this (it hasn't for me before on other devices).
,
May 4 2018
For crash reports, check in chrome://settings that "Automatically send usage statistics and crash reports to Google" is turned on. I don't think developer mode should matter.
,
May 4 2018
That setting is turned on. I don't see any new crash logs immediately after reproducing it. I see that there were some logs generated today, but don't know if they caught that issue. c47394dac846bd40 78df52d88b26234f 72df1f76e67e11f4
,
May 4 2018
Thanks, those look relevant. Interestingly, it seems to show up only in M65: https://crash.corp.google.com/browse?q=expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27chromeos%3A%3APlatformKeysService%3A%3ASelectTask%3A%3AFilterSelectionByPermission%27#samplereports That function is the same in M65 as today, but it was part of a merge back to M65: https://chromium.googlesource.com/chromium/src/+/0b20ab52f233798e4bf8a4ee5ebc4c8edcc38526 Maybe something about the merge doesn't quite work in M65? backtrace: 0x00005b6acbfafcd7 (chrome -platform_keys_service.cc:632 ) chromeos::PlatformKeysService::SelectTask::FilterSelectionByPermission() 0x00005b6acbfaf46e (chrome -platform_keys_service.cc:422 ) chromeos::PlatformKeysService::SelectTask::DoStep() 0x00005b6acbfaf9b4 (chrome -platform_keys_service.cc:599 ) chromeos::PlatformKeysService::SelectTask::UpdatePermission() 0x00005b6acbfaf45d (chrome -platform_keys_service.cc:418 ) chromeos::PlatformKeysService::SelectTask::DoStep() 0x00005b6acef82916 (chrome -callback.h:105 ) chromeos::PlatformKeysCertificateSelector::AcceptCertificate(std::__1::unique_ptr<net::ClientCertIdentity, std::__1::default_delete<net::ClientCertIdentity> >) 0x00005b6acf0118d4 (chrome -certificate_selector.cc:260 ) chrome::CertificateSelector::Accept() 0x00005b6ace341327 (chrome -dialog_client_view.cc:99 ) views::DialogClientView::ButtonPressed(views::Button*, ui::Event const&) 0x00005b6ace3079f9 (chrome -button.cc:233 ) views::Button::OnMouseReleased(ui::MouseEvent const&) 0x00005b6ace34ceae (chrome -ink_drop_host_view.cc:263 ) views::InkDropHostView::OnMouseEvent(ui::MouseEvent*) 0x00005b6acd597d71 (chrome -event_dispatcher.cc:191 ) ui::EventDispatcher::ProcessEvent(ui::EventTarget*, ui::Event*) 0x00005b6acd597b7b (chrome -event_dispatcher.cc:86 ) ui::EventDispatcherDelegate::DispatchEvent(ui::EventTarget*, ui::Event*) 0x00005b6ace366049 (chrome -root_view.cc:442 ) views::internal::RootView::OnMouseReleased(ui::MouseEvent const&) 0x00005b6ace33c78d (chrome -widget.cc:1218 ) views::Widget::OnMouseEvent(ui::MouseEvent*) 0x00005b6acd597d71 (chrome -event_dispatcher.cc:191 ) ui::EventDispatcher::ProcessEvent(ui::EventTarget*, ui::Event*) 0x00005b6acd597b7b (chrome -event_dispatcher.cc:86 ) ui::EventDispatcherDelegate::DispatchEvent(ui::EventTarget*, ui::Event*) 0x00005b6ad003a34e (chrome -event_processor.cc:57 ) ui::EventProcessor::OnEventFromSource(ui::Event*) 0x00005b6acd598303 (chrome -event_source.cc:73 ) ui::EventSource::SendEventToSink(ui::Event*) 0x00005b6acec600cd (chrome -ash_window_tree_host_platform.cc:124 ) ash::AshWindowTreeHostPlatform::DispatchEvent(ui::Event*) 0x00005b6acd598d6c (chrome -callback.h:65 ) ui::DispatchEventFromNativeUiEvent(void* const&, base::OnceCallback<void (ui::Event*)>) 0x00005b6acb05b2a1 (chrome -drm_window_host.cc:185 ) ui::DrmWindowHost::DispatchEvent(void* const&) 0x00005b6acb05b36c (chrome -drm_window_host.cc ) non-virtual thunk to ui::DrmWindowHost::DispatchEvent(void* const&) 0x00005b6acd590b62 (chrome -platform_event_source.cc:93 ) ui::PlatformEventSource::DispatchEvent(void*) 0x00005b6acd8f2ab2 (chrome -event_factory_evdev.cc:359 ) ui::EventFactoryEvdev::DispatchMouseButtonEvent(ui::MouseButtonEventParams const&) 0x00005b6ad13d22c4 (chrome -callback.h:65 ) base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) 0x00005b6ad13d3ac9 (chrome -message_loop.cc:399 ) base::MessageLoop::RunTask(base::PendingTask*) 0x00005b6ad13d46a8 (chrome -message_loop.cc:411 ) base::MessageLoop::DoWork() 0x00005b6ad13d4e4c (chrome -message_pump_libevent.cc:220 ) base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) 0x00005b6accbee854 (chrome -run_loop.cc:130 ) mojo::internal::Serializer<device::mojom::Geoposition_ErrorCode, device::mojom::Geoposition_ErrorCode>::Deserialize(int, device::mojom::Geoposition_ErrorCode*) 0x00005b6acc8a9407 (chrome -chrome_browser_main.cc:1973 ) ChromeBrowserMainParts::MainMessageLoopRun(int*) 0x00005b6acb4d45b3 (chrome -browser_main_loop.cc:1236 ) content::BrowserMainLoop::RunMainMessageLoopParts() 0x00005b6acb4d7461 (chrome -browser_main_runner.cc:145 ) content::BrowserMainRunnerImpl::Run() 0x00005b6acb4d0558 (chrome -browser_main.cc:46 ) content::BrowserMain(content::MainFunctionParams const&) 0x00005b6acc895a20 (chrome -content_main_runner.cc:717 ) content::ContentMainRunnerImpl::Run() 0x00005b6acc89ec1b (chrome -main.cc:456 ) service_manager::Main(service_manager::MainParams const&) 0x00005b6acc894610 (chrome -content_main.cc:19 ) content::ContentMain(content::ContentMainParams const&) 0x00005b6acadf7224 (chrome -chrome_main.cc:129 ) ChromeMain
,
May 5 2018
Thanks, I'll take a look asap. ldanxian@, could you please answer some additional questions so I can try to reproduce: - is the user this is happening with an affiliated user, or not affiliated (=managed by the same domain as the device)? - when you open the certificate manager from settings and go to "Your Certificates", does it contain Certificates? If yes, could you please check their details to see if they are on the "User' or "System" database? Thanks!
,
May 6 2018
From the crash report, this looks like it happens on the platformKeys.selectClientCertificatess call with "interactive"=true, after a certificate selection dialog is displayed and the user selects a certificate. Could you please confirm if this is the function triggering the crash (instead of enterprise.platformKeys.getCertificates suggested by the bug title)? If yes, could you please tell us what kind of certificate was selected and how it was installed, and post details about it from certificate manager (i.e. if it is in the User or System database, and if it is displayed as hardware-backed). First analysis: One possible way this could happen if the profile is not managed and GetKeyLocations returns no key locations for the selected client certificate. Specifically, what would happen then is: - In SelectTask::GotKeyLocations[1], CanUserGrantPermissionFor(..)[2] would return true, adding the cert to the list of certificates to select from. - The user selects the certificate. - In SelectTask::UpdatePermission[3], SetUserGrantedPermission[4] is a no-op. - In FilterSelectionByPermission[4], CanUseKeyForSigning[5] returns false This fails the CHECK[6] because it effectively means that certificate that was allowed for selection can not be used for signing. If this is what happens, the root cause would be that the key associated with a certificate returned from the user's ClientCertStore can not be found on the user's private slot by GetKeyLocationsWithDB[7] (the system slot should not be available as I'm assuming this would be an unmanaged profile). I am not sure yet what would cause this but will investigate further. Still, when this happens, Chrome OS should not offer the certificate for selection at all, instead of offering it and then crashing. I'll prepare a CL to do this part. [1] GotKeyLocations: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/platform_keys/platform_keys_service.cc?rcl=232f0079a6d8414bd65a9caccf9bd3fd1d96ad9c&l=516 [2] CanUserGrantPermissionFor: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/platform_keys/key_permissions.cc?rcl=232f0079a6d8414bd65a9caccf9bd3fd1d96ad9c&l=427 [3] UpdatePermission: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/platform_keys/platform_keys_service.cc?rcl=232f0079a6d8414bd65a9caccf9bd3fd1d96ad9c&l=597 [4] SetUserGrantedPermission: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/platform_keys/platform_keys_service.cc?rcl=232f0079a6d8414bd65a9caccf9bd3fd1d96ad9c&l=623 [5] CanUseKeyForSigning: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/platform_keys/key_permissions.cc?rcl=232f0079a6d8414bd65a9caccf9bd3fd1d96ad9c&l=197 [6] CHECK(!selected_cert_ || (filtered_certs->size() == 1 && filtered_certs->front() == selected_cert_)); I'm assuming selected_cert_ is set and filtered_certs is empty. [7] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/platform_keys/platform_keys_nss.cc?rcl=232f0079a6d8414bd65a9caccf9bd3fd1d96ad9c&l=770
,
May 6 2018
+emaxx@ FYI in case you have an idea. As stated in Comment #8, I'm *suspecting* a plausible explanation is an unmanaged user's ClientCertStoreChromeOS, effectively "CERT_FindUserCertsByUsage(CERT_GetDefaultCertDB(), certUsageSSLClient, PR_FALSE, PR_FALSE, password_delegate.get())", returning a certificate for which no key can be found on the user's private slot by "FindNSSKeyFromPublicKeyInfoInSlot" (or the private slot is not available at all for some reason).
,
May 6 2018
Hrm, now I'm wondering if the private key associated with the client certificte could be on the user's public slot (e.g. when importing through certificate manager, and using "Import" instead of "Import and Bind", which would currently not be supported by GetKeyLocationsByDB[1] at all, and could lead to the symptoms described in Comment #8. Matt, would you know if this can happen? [1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/platform_keys/platform_keys_nss.cc?rcl=232f0079a6d8414bd65a9caccf9bd3fd1d96ad9c&l=770
,
May 7 2018
So it looks like client certs imported through certificate manager with "Import" instead of "Import and Bind" do store their key on the public slot: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/certificates_handler.cc?rcl=7f65b0c0e07ba9263ad8b82d09d4ab267177cd1a&l=716 I could reproduce the CHECK-fail with an apitest and have prepared an attempt at a fix in https://chromium-review.googlesource.com/#/c/chromium/src/+/1047208 .
,
May 10 2018
Just to add that I am impacted by this bug. I use certificate-based authentication using Cisco AnyConnect client, recently the client no longer called the cert and when I manually tried to use my existing imported cert the screen would flicker and AnyConnect would crash. I have since used the import and bind option with no issues. What is the difference between 'import and bind' and 'import'
,
May 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/725eaa457dbd93a822240d06a4b317d63b958d18 commit 725eaa457dbd93a822240d06a4b317d63b958d18 Author: Pavol Marko <pmarko@chromium.org> Date: Thu May 10 21:00:15 2018 Treat public slot keys as stored on user token Keys that are stored on a user's public token are now recognized by GetKeyLocations. If no locations were found for a key, KeyPermissions::CanUserGrantPermissionsFor now returns false as a safeguard. A mechanism was introduced in nss_util.cc to simulate a separate private slot in Chrome OS browsertests. Bug: 839573 Test: browser_tests --gtest_filter=*PlatformKeysTest* Change-Id: If8b4cd4ef3a5763dd37a158426ec3cf87e1d10ae Reviewed-on: https://chromium-review.googlesource.com/1047208 Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Reviewed-by: David Benjamin <davidben@chromium.org> Reviewed-by: Maksim Ivanov <emaxx@chromium.org> Reviewed-by: Matt Mueller <mattm@chromium.org> Commit-Queue: Pavol Marko <pmarko@chromium.org> Cr-Commit-Position: refs/heads/master@{#557667} [modify] https://crrev.com/725eaa457dbd93a822240d06a4b317d63b958d18/chrome/browser/chromeos/login/webview_login_browsertest.cc [modify] https://crrev.com/725eaa457dbd93a822240d06a4b317d63b958d18/chrome/browser/chromeos/platform_keys/key_permissions.cc [modify] https://crrev.com/725eaa457dbd93a822240d06a4b317d63b958d18/chrome/browser/chromeos/platform_keys/platform_keys_nss.cc [modify] https://crrev.com/725eaa457dbd93a822240d06a4b317d63b958d18/chrome/browser/chromeos/policy/user_affiliation_browsertest.cc [modify] https://crrev.com/725eaa457dbd93a822240d06a4b317d63b958d18/chrome/browser/extensions/api/platform_keys/platform_keys_apitest_nss.cc [modify] https://crrev.com/725eaa457dbd93a822240d06a4b317d63b958d18/crypto/nss_util.cc [modify] https://crrev.com/725eaa457dbd93a822240d06a4b317d63b958d18/crypto/nss_util_internal.h
,
May 14 2018
,
May 14 2018
,
May 16 2018
Re Comment #13: The difference is where the certificate (and the private key) are stored. When imported using "Import and Bind", the certificate is stored in hardware-backed (using the TPM = Trusted Platform Module). The private key can not be extracted anymore. When imported using "Import", the certificate is stored in a file in the (encrypted) user data. While the user is signed in, it is in theory possible to extract the private key.
,
May 16 2018
Fixed on M-68 with the commit mentioned in Comment #14. Requesting merge to M-67.
,
May 16 2018
This bug requires manual review: We are only 12 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 16 2018
This is a large change to introduce this late into M67. It's not a M67 regression; it was reported against M65. Risk for not merging into M67? Also need more context on testing to ensure we don't creat new issues.
,
May 17 2018
Re Comment #20:
You're probably right that it's too intrusive for a merge.
If we don't merge, this particular usecase will continue to crash chrome on Chrome OS:
(1) Import a client certificate onto the public slot by navigating to
chrome://settings/certificates and using the 'Import' button on the
'Your Certificates' tab
(2) Invoke platformKeys.selectClientCertificates from an extension
(3) In the dialog box that pops up, select the certificate imported in (1).
However, there is a workaround: Using the 'Import and Bind' button instead of 'Import'.
,
May 17 2018
Hi, covering for Kevin. #21 are we removing the merge request and moving to M-68? If so, need to update the labels.
,
May 17 2018
Re Comment #22: Yes, I think that's reasonable. Removed the merge-related labels. |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by mattm@chromium.org
, May 3 2018