New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 741042 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Intermittent crash in D2D Auth

Project Member Reported by hansberry@chromium.org, Jul 11 2017

Issue description

I observed this during a Tether host scan (I can't tell if it's due to Tether or EasyUnlock though). This account has two Tether hosts, and one unlock_key. chrome logs attached as well. I've observed this happening in < 5% of host scans; it's quite rare.

Tim/Kyle, did something change recently in cryptauth/EasyUnlock that might have caused this crash?

Backtrace:

Thread 1 "chrome" received signal SIGSEGV, Segmentation fault.
0x3542e28c in cryptauth::(anonymous namespace)::OnSessionSymmetricKeyDerived(cryptauth::(anonymous namespace)::ValidateResponderAuthMessageContext, std::string const&) () at ../../components/cryptauth/device_to_device_initiator_operations.cc:153
(gdb) bt
#0  0x3542e28c in cryptauth::(anonymous namespace)::OnSessionSymmetricKeyDerived(cryptauth::(anonymous namespace)::ValidateResponderAuthMessageContext, std::string const&) () at ../../components/cryptauth/device_to_device_initiator_operations.cc:153
#1  0x3542e444 in base::internal::Invoker<base::internal::BindState<void (*)(cryptauth::(anonymous namespace)::ValidateResponderAuthMessageContext, std::string const&), cryptauth::(anonymous namespace)::ValidateResponderAuthMessageContext>, void (std::string const&)>::Run(base::internal::BindStateBase*, std::string const&) () at ../../base/bind_internal.h:164
#2  0x33166f86 in chromeos::(anonymous namespace)::EasyUnlockClientImpl::OnData(base::Callback<void (std::string const&), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&, dbus::Response*) () at ../../base/callback.h:80
#3  0x332470ac in dbus::ObjectProxy::RunResponseCallback(base::Callback<void (dbus::Response*), (base::internal::CopyMode)1, (base::internal::RepeatMode)1>, base::Callback<void (dbus::ErrorResponse*), (base::internal::CopyMode)1, (base::internal::RepeatMode)1>, base::TimeTicks, DBusMessage*) () at ../../base/callback.h:80
#4  0x332495c0 in base::internal::Invoker<base::internal::BindState<void (dbus::ObjectProxy::*)(base::Callback<void (dbus::Response*), (base::internal::CopyMode)1, (base::internal::RepeatMode)1>, base::Callback<void (dbus::ErrorResponse*), (base::internal::CopyMode)1, (base::internal::RepeatMode)1>, base::TimeTicks, DBusMessage*), scoped_refptr<dbus::ObjectProxy>, base::Callback<void (dbus::Response*), (base::internal::CopyMode)1, (base::internal::RepeatMode)1>, base::Callback<void (dbus::ErrorResponse*), (base::internal::CopyMode)1, (base::internal::RepeatMode)1>, base::TimeTicks, DBusMessage*>, void ()>::Run(base::internal::BindStateBase*) () at ../../base/bind_internal.h:209
#5  0x32adf560 in base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) () at ../../base/callback.h:91
#6  0x32a844dc in base::MessageLoop::RunTask(base::PendingTask*) () at ../../base/message_loop/message_loop.cc:422
#7  0x32a847b0 in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) () at ../../base/message_loop/message_loop.cc:433
#8  0x32a84a74 in base::MessageLoop::DoWork() () at ../../base/message_loop/message_loop.cc:540
#9  0x32a85d5c in base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) () at ../../base/message_loop/message_pump_libevent.cc:219
#10 0x32a9f054 in base::RunLoop::Run() () at ../../base/run_loop.cc:111
#11 0x327f42ce in ChromeBrowserMainParts::MainMessageLoopRun(int*) () at ../../chrome/browser/chrome_browser_main.cc:1962
#12 0x31969e64 in content::BrowserMainLoop::RunMainMessageLoopParts() () at ../../content/browser/browser_main_loop.cc:1147
#13 0x3196bca4 in content::BrowserMainRunnerImpl::Run() () at ../../content/browser/browser_main_runner.cc:142
#14 0x31966976 in content::BrowserMain(content::MainFunctionParams const&) () at ../../content/browser/browser_main.cc:46
#15 0x327d42a4 in content::ContentMainRunnerImpl::Run() () at ../../content/app/content_main_runner.cc:686
#16 0x327ec050 in service_manager::Main(service_manager::MainParams const&) () at ../../services/service_manager/embedder/main.cc:469
#17 0x327d3688 in content::ContentMain(content::ContentMainParams const&) () at ../../content/app/content_main.cc:19
#18 0x31538376 in ChromeMain () at ../../chrome/app/chrome_main.cc:134
#19 0xaec448b8 in __libc_start_main (main=0x0, argc=-1090743560, argv=0x0, init=<optimized out>, fini=0x35f50bfd <__libc_csu_fini>, rtld_fini=0xaf2bbf41 <_dl_fini>, stack_end=0xbefc9454) at libc-start.c:289
#20 0x31538218 in _start ()
 
Forgot to attach chrome logs:
d2d-crash.log
560 KB View Download
Owner: tengs@chromium.org
Status: Assigned (was: Available)
Tim, this crash originates from EasyUnlockClient. Can you look into it?
It seems the |secure_message_delegate| is nil. I don't know exactly why.

This not necessary triggered by EasyUnlock, as the |secure_message_delegate| is used by both Magic Tether and EasyUnlock to establish the secure channel. The delegate performs the crypto operations.
Both EasyUnlock and Tether utilize CryptAuthService to create a new SecureMessageDelegate, and here is the implementation:

https://cs.chromium.org/chromium/src/chrome/browser/cryptauth/chrome_cryptauth_service.cc?q=CreateSecureMessageDelegate

Can you determine how EasyUnlockClientImpl::OnData() passes the delegate along?
The delegate is part of a |context| passed to the delegate itself on every crypto operation. 

The same |context| was just used before to derive the symmetric key, and the delegate was not nil then:
https://cs.chromium.org/chromium/src/components/cryptauth/device_to_device_initiator_operations.cc?l=138

Comment 6 by tengs@chromium.org, Jul 11 2017

Owner: hansberry@chromium.org
From a brief look at the logs, it looks like this is caused by a race condition of timing out the connection attempt while authentication is in progress.

I see the following in the logs:


[12137:12137:0711/113711.473872:INFO:device_to_device_authenticator.cc(237)] Received [Responder Auth] message, payload_size=325
.
.
.
[12137:12137:0711/113711.509895:INFO:ble_connection_manager.cc(462)] Connection attempt timeout - Device ID "CAESR...0qr4="
[12137:12137:0711/113711.509985:VERBOSE1:bluetooth_discovery_session.cc(45)] Stopping device discovery session.
[12137:12137:0711/113711.510195:VERBOSE1:device_event_log_impl.cc(158)] [11:37:11.510] Bluetooth: EVENT: bluetooth_adapter_bluez.cc:1427 RemoveDiscoverySession
[12137:12137:0711/113711.510365:INFO:ble_advertiser.cc(276)] Stopping advertising to device CAESR...0qr4=
[12137:12137:0711/113711.510419:INFO:ble_advertiser.cc(72)] Calling Unregister
[12137:12137:0711/113711.510537:VERBOSE1:bluetooth_le_advertisement_service_provider.cc(84)] Cleaning up Bluetooth Advertisement: /org/chromium/bluetooth_advertisement/dac3bacfdd6d47fbaa3f4d28188038a9
[12137:12137:0711/113711.510599:INFO:ble_advertiser.cc(283)] Remaining adverts: 0
[12137:12137:0711/113711.510693:INFO:ble_connection_manager.cc(499)] Status change - Device ID: "CAESR...0qr4=": [connecting] => [disconnected]



This seems to correspond to the stack trace in ValidateResponderAuthMessage. The |secure_message_delegate| that Gustavo is referring to in #3 is owned by DeviceToDeviceAuthenticator, which would be destroyed when the session is invalidated.

This crash occurs in the callback of a DBus message response (EasyUnlockClient talks to the easyunlock deamon in ChromeOS), which would call into the now invalidated SecureMessageContext.

At least, this is what I think is happening.

Owner: khorimoto@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2693aa8e650f3819cbf455d628622d694d9d35a3

commit 2693aa8e650f3819cbf455d628622d694d9d35a3
Author: Kyle Horimoto <khorimoto@google.com>
Date: Thu Jul 27 17:08:01 2017

[CrOS Tether] Eliminate crash in D2D-Auth protocol.

This fix makes it possible to instantiate
DeviceToDeviceInitiatorOperations instances and uses WeakPtrs
within the class to ensure that it is not possible to segfault by
attempting to call a function on a deleted object.

Bug: 672263,  741042 
Change-Id: Idcb13aa4772d6e227a335556bd4bf6b3cb4d3f4c
Reviewed-on: https://chromium-review.googlesource.com/583563
Reviewed-by: Tim Song <tengs@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489989}
[modify] https://crrev.com/2693aa8e650f3819cbf455d628622d694d9d35a3/components/cryptauth/BUILD.gn
[modify] https://crrev.com/2693aa8e650f3819cbf455d628622d694d9d35a3/components/cryptauth/device_to_device_authenticator.cc
[modify] https://crrev.com/2693aa8e650f3819cbf455d628622d694d9d35a3/components/cryptauth/device_to_device_authenticator.h
[modify] https://crrev.com/2693aa8e650f3819cbf455d628622d694d9d35a3/components/cryptauth/device_to_device_authenticator_unittest.cc
[rename] https://crrev.com/2693aa8e650f3819cbf455d628622d694d9d35a3/components/cryptauth/device_to_device_initiator_helper.cc
[add] https://crrev.com/2693aa8e650f3819cbf455d628622d694d9d35a3/components/cryptauth/device_to_device_initiator_helper.h
[delete] https://crrev.com/9191b87bd02343147139fb3f3e320d8bec5b8999/components/cryptauth/device_to_device_initiator_operations.h
[modify] https://crrev.com/2693aa8e650f3819cbf455d628622d694d9d35a3/components/cryptauth/device_to_device_operations_unittest.cc

Labels: Merge-Request-61
Labels: -Merge-Request-61 Merge-Approved-61
Approving merge to M61 Chrome OS.
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 27 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3dbc8112cfdb0ff5a155b9a894d14b3d4813e089

commit 3dbc8112cfdb0ff5a155b9a894d14b3d4813e089
Author: Kyle Horimoto <khorimoto@google.com>
Date: Thu Jul 27 21:34:30 2017

[CrOS Tether] Eliminate crash in D2D-Auth protocol.

This fix makes it possible to instantiate
DeviceToDeviceInitiatorOperations instances and uses WeakPtrs
within the class to ensure that it is not possible to segfault by
attempting to call a function on a deleted object.

TBR=khorimoto@google.com

(cherry picked from commit 2693aa8e650f3819cbf455d628622d694d9d35a3)

Bug: 672263,  741042 
Change-Id: Idcb13aa4772d6e227a335556bd4bf6b3cb4d3f4c
Reviewed-on: https://chromium-review.googlesource.com/583563
Reviewed-by: Tim Song <tengs@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#489989}
Reviewed-on: https://chromium-review.googlesource.com/590750
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#94}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/3dbc8112cfdb0ff5a155b9a894d14b3d4813e089/components/cryptauth/BUILD.gn
[modify] https://crrev.com/3dbc8112cfdb0ff5a155b9a894d14b3d4813e089/components/cryptauth/device_to_device_authenticator.cc
[modify] https://crrev.com/3dbc8112cfdb0ff5a155b9a894d14b3d4813e089/components/cryptauth/device_to_device_authenticator.h
[modify] https://crrev.com/3dbc8112cfdb0ff5a155b9a894d14b3d4813e089/components/cryptauth/device_to_device_authenticator_unittest.cc
[rename] https://crrev.com/3dbc8112cfdb0ff5a155b9a894d14b3d4813e089/components/cryptauth/device_to_device_initiator_helper.cc
[add] https://crrev.com/3dbc8112cfdb0ff5a155b9a894d14b3d4813e089/components/cryptauth/device_to_device_initiator_helper.h
[delete] https://crrev.com/17ad12406183efb7d6ece31ff78f787aae97a3a5/components/cryptauth/device_to_device_initiator_operations.h
[modify] https://crrev.com/3dbc8112cfdb0ff5a155b9a894d14b3d4813e089/components/cryptauth/device_to_device_operations_unittest.cc

Status: Fixed (was: Started)

Comment 13 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment