New issue
Advanced search Search tips

Issue 839573 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

platformKeys.selectClientCertificates causing system crash with public-slot keys on Chrome 65.0.3325

Reported by ldanx...@cisco.com, May 3 2018

Issue description

Chrome 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".



 

Comment 1 by mattm@chromium.org, May 3 2018

Components: Internals>Network>Certificate Platform>Extensions>API
If you have crash reporting enabled, could you provide crash id(s)?

Is this a new crash? If so, what is the last version it did not crash on?
Cc: pmarko@chromium.org
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)?

Comment 3 by ldanx...@cisco.com, 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).

Comment 4 by mattm@chromium.org, 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.

Comment 5 by ldanx...@cisco.com, 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

Comment 6 by mattm@chromium.org, May 4 2018

Cc: -pmarko@chromium.org
Labels: Stability-Crash
Owner: pmarko@chromium.org
Status: Assigned (was: Unconfirmed)
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
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!
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
Cc: emaxx@chromium.org
+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).
Cc: mattm@chromium.org
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
Status: Started (was: Assigned)
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 .

Comment 12 Deleted

Comment 13 by mham...@gmail.com, 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'
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Summary: platformKeys.selectClientCertificates causing system crash with public-slot keys on Chrome 65.0.3325 (was: platformKeys.getCertificates causing system crash on Chrome 65.0.3325)
Description: Show this description
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.
Labels: -Pri-3 Merge-Request-67 M-67 M-68 Pri-2
Status: Fixed (was: Started)
Fixed on M-68 with the commit mentioned in Comment #14.
Requesting merge to M-67.
Project Member

Comment 19 by sheriffbot@chromium.org, May 16 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
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
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.
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'.
Hi, covering for Kevin. #21 are we removing the merge request and moving to M-68? If so, need to update the labels.
Labels: -Hotlist-Merge-Review -M-67 -Merge-Review-67
Re Comment #22: Yes, I think that's reasonable.
Removed the merge-related labels.

Sign in to add a comment