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

Issue metadata

Status: Fixed
Owner:
Closed: Mar 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 732626



Sign in to add a comment

Eng tracking for Detachable Base Swap Detection

Project Member Reported by zalcorn@chromium.org, Dec 19

Issue description

Eng tracking for crbug/732626

Mattias, what's the current status here? If creating the UI is all that's left, please assign to rkc@.

Mocks for sign-in screen and in-session with approved strings are here: https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZV53zngRLxw3/files/MCE_Q_ba__iu2F01ppCjOYp0

 
Cc: drinkcat@chromium.org conradlo@chromium.org
Owner: r...@chromium.org
The firmware / system side of this is ready to the best of my knowledge. CC'ing drinkcat@ to confirm.

Assigning to rkc@ to coordinate the UI work.
Cc: mcirimele@chromium.org
Let me know if you need anything from UX. I'm marking this as done on my list :)
Labels: -Pri-2 -M-65 M-66 Pri-1
Owner: tbarzic@chromium.org
Toni let me know if any product/UX questions come up that I can help answer.
so far I have a couple of questions about the mocks for lock screen:
 1. the notification has a "Learn more" link - what is clicking the link expected to do, provided that we can't open a browser window on lock screen?
 2. the user might have a pin enabled on lock screen, in which case there might space for us to show the notification under the auth input

1. On sign-in screen, "Learn more" opens the article in guest mode. On lockscreen, let's hide "Learn more".
2. Error dialog shows above the PIN pad as in attached screenshot. This should be reusing the same error dialog UI as other sign-in/lockscreen errors.
Screenshot 2018-02-14 at 2.06.30 PM.png
534 KB View Download
Cc: -conradlo@chromium.org akahuang@chromium.org a....@samsung.com
I see hammerd exposes dbus API with signals about pairing/firmware update state.

Are there plans to add API for Chrome to query the base state? I'm worried about Chrome missing the signals from hammerd - e.g. if a signal is sent before Chrome starts, or while Chrome session is restarting/shutting down (for example on sign-out, or due to a crash).
Cc: -a....@samsung.com conradlo@chromium.org
Project Member

Comment 9 by bugdroid1@chromium.org, Feb 28

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

commit e5b1f57d6e90ffc0d322522a22df7b288eaf4be9
Author: Toni Barzic <tbarzic@google.com>
Date: Wed Feb 28 01:29:26 2018

Introduce HammerdClient - a client for hammerd dbus service

BUG= 796300 ,796342

Change-Id: I067155c431cdde40928e03cb9d0be94bb1fc73fd
Reviewed-on: https://chromium-review.googlesource.com/922267
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539623}
[modify] https://crrev.com/e5b1f57d6e90ffc0d322522a22df7b288eaf4be9/chromeos/BUILD.gn
[modify] https://crrev.com/e5b1f57d6e90ffc0d322522a22df7b288eaf4be9/chromeos/dbus/dbus_clients_common.cc
[modify] https://crrev.com/e5b1f57d6e90ffc0d322522a22df7b288eaf4be9/chromeos/dbus/dbus_clients_common.h
[modify] https://crrev.com/e5b1f57d6e90ffc0d322522a22df7b288eaf4be9/chromeos/dbus/dbus_thread_manager.cc
[modify] https://crrev.com/e5b1f57d6e90ffc0d322522a22df7b288eaf4be9/chromeos/dbus/dbus_thread_manager.h
[add] https://crrev.com/e5b1f57d6e90ffc0d322522a22df7b288eaf4be9/chromeos/dbus/fake_hammerd_client.cc
[add] https://crrev.com/e5b1f57d6e90ffc0d322522a22df7b288eaf4be9/chromeos/dbus/fake_hammerd_client.h
[add] https://crrev.com/e5b1f57d6e90ffc0d322522a22df7b288eaf4be9/chromeos/dbus/hammerd_client.cc
[add] https://crrev.com/e5b1f57d6e90ffc0d322522a22df7b288eaf4be9/chromeos/dbus/hammerd_client.h

Project Member

Comment 10 by bugdroid1@chromium.org, Mar 2

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

commit a783d843222fcb49a995a76cc0b114c28865dc8d
Author: Toni Barzic <tbarzic@google.com>
Date: Fri Mar 02 02:54:00 2018

Strings for detachable base related notifications

BUG=796342,  796300 

Change-Id: I839f1cf3bfc7dc5ae9f29a07835dbb89690fcf44
Reviewed-on: https://chromium-review.googlesource.com/944263
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540425}
[modify] https://crrev.com/a783d843222fcb49a995a76cc0b114c28865dc8d/ash/ash_strings.grd
[modify] https://crrev.com/a783d843222fcb49a995a76cc0b114c28865dc8d/chrome/app/chromeos_strings.grdp

Project Member

Comment 11 by bugdroid1@chromium.org, Mar 2

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

commit d6238312f455742e2fafb9208d976eb8daeba2d6
Author: Toni Barzic <tbarzic@google.com>
Date: Fri Mar 02 18:26:13 2018

Add DetachableBaseHandler to ash/

Tracks HammerdClient and PowerManagerClient to detect if a
detachable base (hammer) is connected/paired with the device.
It keeps track of the last connected base per user - this
information will be used to notify the user when the attached
base is changed.
DetachableBaseHandler does not track the active users, or session
state - it depends on its clients to determine when a base is
considered used/trusted by a user.

BUG= 796300 ,796342

Change-Id: Iaa3e8114dfb65a6e5654b166c4bc74522b30c80e
Reviewed-on: https://chromium-review.googlesource.com/930182
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540560}
[modify] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/BUILD.gn
[add] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/detachable_base/DEPS
[add] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/detachable_base/OWNERS
[add] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/detachable_base/detachable_base_handler.cc
[add] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/detachable_base/detachable_base_handler.h
[add] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/detachable_base/detachable_base_handler_unittest.cc
[add] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/detachable_base/detachable_base_observer.h
[add] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/detachable_base/detachable_base_pairing_status.h
[modify] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/public/cpp/ash_pref_names.cc
[modify] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/public/cpp/ash_pref_names.h
[modify] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/shell.cc
[modify] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/ash/shell.h
[modify] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/chrome/browser/chromeos/login/DEPS
[modify] https://crrev.com/d6238312f455742e2fafb9208d976eb8daeba2d6/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Mar 7

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

commit be20db5ab2108cfd77d84c6f9b68d6845cca3d30
Author: Toni Barzic <tbarzic@google.com>
Date: Wed Mar 07 03:02:42 2018

Use upstart to kick off hammerd when detachable base handler is started

The goal is to get hammerd to run pairing challenge, and send the
appropriate dbus pairing signals (indicating whether the base was
paired and authenticated, or whether it requires a firmware update),
in order to initialize Chrome's state.

Note: given that the hammerd job is short lived - it is usually run
when a detachable base is detached, and exists once pairing has been
completed - it cannot easily expose a dbus API to retrieve this state
when Chrome needs it (which might be a good alternative).

BUG=796342,  796300 ,  818057 

Change-Id: I40e71318f6d35d96aaf295033964864e3dc74f88
Reviewed-on: https://chromium-review.googlesource.com/941724
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541301}
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/ash/detachable_base/DEPS
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/ash/detachable_base/detachable_base_handler.cc
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/ash/detachable_base/detachable_base_handler_unittest.cc
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/chromeos/dbus/fake_upstart_client.cc
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/chromeos/dbus/fake_upstart_client.h
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/chromeos/dbus/upstart_client.cc
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/chromeos/dbus/upstart_client.h

Project Member

Comment 13 by bugdroid1@chromium.org, Mar 8

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

commit 550419f83c761e9d3aa2575665ca87af84b45698
Author: Toni Barzic <tbarzic@chromium.org>
Date: Thu Mar 08 03:56:21 2018

Show a notification when a detachable base change is detected for a user

Adds DetachableBaseNotificationController that observes
DetachableBaseHandler in order to detect when a detachable base
different than the last base used by the active user changes. When a
detachable base change is detected, it shows a system notification.
The notification serves a security purpose - it warns the user that
the detachable base they are using has changed, and that the base could
be compromised (to steal their keystrokes).

Note that the notification is shown only if the session is unblocked
at the time - lock screen and login screen will implement their own UI
to notify the user about the base change.

If/when the session is unblocked, the attached base is set as the last
one used by the user to prevent the notification from showing up next
time the user attaches the same base.

Bug:  796300 
Change-Id: I7ff8fdf3e02e5833df2a709169fdd076285f3a4b
Reviewed-on: https://chromium-review.googlesource.com/952477
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541717}
[modify] https://crrev.com/550419f83c761e9d3aa2575665ca87af84b45698/ash/BUILD.gn
[add] https://crrev.com/550419f83c761e9d3aa2575665ca87af84b45698/ash/detachable_base/detachable_base_notification_controller.cc
[add] https://crrev.com/550419f83c761e9d3aa2575665ca87af84b45698/ash/detachable_base/detachable_base_notification_controller.h
[add] https://crrev.com/550419f83c761e9d3aa2575665ca87af84b45698/ash/detachable_base/detachable_base_notification_controller_unittest.cc
[modify] https://crrev.com/550419f83c761e9d3aa2575665ca87af84b45698/ash/shell.cc
[modify] https://crrev.com/550419f83c761e9d3aa2575665ca87af84b45698/ash/shell.h

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 10

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

commit baaec481b96981d00f14d7caf55ada6186df706e
Author: Toni Barzic <tbarzic@chromium.org>
Date: Sat Mar 10 00:51:04 2018

Show an error bubble on lock screen when user attaches different base

Makes LockContentsView observe DetachableBaseHandler, and display an
error bubble when it detects that the user has attached a base
different than the one they used last.
The bubble has the same style as the auth error bubble, but is not
supposed to be dismissable by the user (so untrusted user can't remove
the error bubble while the device is unattended).

Updates LoginBubble class to support non-dismissable error bubbles -
introduces kFlagPersistent flag that can be passed to ShowErrorBubble.
If the flag is set, the bubble will not be closed on key, mouse or
gesture events.

BUG= 796300 

Change-Id: I9723ba61450d03bdedb395462994d0b3546e5db0
Reviewed-on: https://chromium-review.googlesource.com/941726
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542298}
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/BUILD.gn
[add] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/fake_login_detachable_base_model.cc
[add] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/fake_login_detachable_base_model.h
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/lock_contents_view.cc
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/lock_contents_view.h
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/lock_contents_view_unittest.cc
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/lock_debug_view.cc
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/lock_debug_view.h
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/lock_screen.cc
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/lock_screen_sanity_unittest.cc
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/login_bubble.cc
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/login_bubble.h
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/login_bubble_unittest.cc
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/login_data_dispatcher.cc
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/login_data_dispatcher.h
[add] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/login_detachable_base_model.cc
[add] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/login/ui/login_detachable_base_model.h
[modify] https://crrev.com/baaec481b96981d00f14d7caf55ada6186df706e/ash/metrics/login_metrics_recorder_unittest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Mar 13

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

commit 9fdc367608792c731925837e00a3e10f81ec7406
Author: Toni Barzic <tbarzic@chromium.org>
Date: Tue Mar 13 23:56:21 2018

LockDebugView UI for detachable base debugging

Bug:  796300 
Change-Id: Ib76be47f5a9727ec5d7f5995a83e1f671811b9f1
Reviewed-on: https://chromium-review.googlesource.com/954608
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542954}
[modify] https://crrev.com/9fdc367608792c731925837e00a3e10f81ec7406/ash/login/ui/lock_debug_view.cc
[modify] https://crrev.com/9fdc367608792c731925837e00a3e10f81ec7406/ash/login/ui/lock_debug_view.h
[modify] https://crrev.com/9fdc367608792c731925837e00a3e10f81ec7406/ash/login/ui/lock_screen.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Mar 14

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

commit d2432cb89ade6b1f60154937f1dcc5e06a969730
Author: Toni Barzic <tbarzic@chromium.org>
Date: Wed Mar 14 22:23:34 2018

Add detachable base change notification to login webui

Adds logic to display "persistent" error bubble (i.e. error bubble
that remains shown if user clicks, scrolls, types outside of the
bubble) that warns the user when a detachable base change / invalid
detachable base is detected. The warning warns the user that the
detachable base is different than he one they used last, and that
they should proceed with caution as the base might be malicious.

SigninScreenHandler is updated to observer ash::DetachableBaseHandler
state (currently non-mash only), and notify the UI when the error
message should be shown or hidden as the detachable base pairing
status, and the focused user pod change.

BUG= 796300 

Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I2813af6ac9473554509308194b3ae04331d013b9
Reviewed-on: https://chromium-review.googlesource.com/956802
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543219}
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/chrome/browser/resources/chromeos/login/md_lock.js
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/chrome/browser/resources/chromeos/login/md_login.js
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/chrome/browser/resources/chromeos/login/md_screen_container.html
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/chrome/browser/resources/chromeos/login/oobe.js
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/chrome/browser/ui/webui/chromeos/login/DEPS
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/ui/login/account_picker/md_screen_account_picker.css
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/ui/login/account_picker/md_screen_account_picker.js
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/ui/login/account_picker/md_user_pod_row.js
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/ui/login/bubble.js
[modify] https://crrev.com/d2432cb89ade6b1f60154937f1dcc5e06a969730/ui/login/display_manager.js

Labels: -M-66 M-67
Status: Fixed (was: Assigned)

Sign in to add a comment