vboot: use recovery button as dev mode switch confirmation
We don't allow ENTER from a USB keyboard as the confirmation
in the switch from normal to developer mode.
For devices that have a physical recovery button, we require
a recovery button press instead. For other devices, we
require that ENTER be pressed on the internal keyboard.
This prevents an "evil keyboard" attack in which a USB keyboard
(or other USB device pretending to be a keyboard) sends a
control-D/ENTER sequence shortly after every boot (followed
by more evil keys). In that situation, when users power-on in
recovery mode, they will be forced to dev mode even if it
was not their intention. Further attacks are easy at
that point.
TESTING. On a panther device:
1. powered on with recovery button pressed -> booted in recovery mode
2. pressed control-D on external USB keyboard -> got to ToDev? screen
3. pressed ENTER -> system beeped
4. pressed recovery button -> system rebooted in DEV mode
... all as expected
Also:
1. powered on with recovery button pressed and HELD recovery button
2. pressed control-D -> system beeped
BUG=chrome-os-partner:21729
TEST=manual (see commit message)
BRANCH=none
CQ-DEPEND=CL:182420,CL:182946,CL:182357
Change-Id: Ib986d00d4567c2d447f8bbff0e5ccfec94596aa7
Reviewed-on: https://chromium-review.googlesource.com/182241
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>
Tested-by: Luigi Semenzato <semenzato@chromium.org>
Commit-Queue: Luigi Semenzato <semenzato@chromium.org>
diff --git a/firmware/include/vboot_api.h b/firmware/include/vboot_api.h
index 0ae6c76..92b9411 100644
--- a/firmware/include/vboot_api.h
+++ b/firmware/include/vboot_api.h
@@ -221,6 +221,11 @@
* HW write protect. Both must be set for flash write protection to work.
*/
#define VB_INIT_FLAG_SW_WP_ENABLED 0x00000800
+/*
+ * This platform does not have a physical recovery switch which, when present,
+ * can (and should) be used for additional physical presence checks.
+ */
+#define VB_INIT_FLAG_VIRTUAL_REC_SWITCH 0x00001000
/*
* Output flags for VbInitParams.out_flags. Used to indicate potential boot
@@ -699,6 +704,14 @@
VB_KEY_CTRL_ENTER = 0x104,
};
+/* Flags for additional information.
+ * TODO(semenzato): consider adding flags for modifiers instead of
+ * making up some of the key codes above.
+ */
+enum VbKeyFlags_t {
+ VB_KEY_FLAG_TRUSTED_KEYBOARD = 1 << 0,
+};
+
/**
* Read the next keypress from the keyboard buffer.
*
@@ -725,6 +738,16 @@
* sending an arrow key as the sequence of keys '\x1b', '[', '1', 'A'). */
uint32_t VbExKeyboardRead(void);
+/**
+ * Same as VbExKeyboardRead(), but return extra information.
+ */
+uint32_t VbExKeyboardReadWithFlags(uint32_t *flags_ptr);
+
+/**
+ * Return the current state of the switches specified in request_mask
+ */
+uint32_t VbExGetSwitches(uint32_t request_mask);
+
/*****************************************************************************/
/* Embedded controller (EC) */
diff --git a/firmware/include/vboot_struct.h b/firmware/include/vboot_struct.h
index e20b0aa..7fa072f 100644
--- a/firmware/include/vboot_struct.h
+++ b/firmware/include/vboot_struct.h
@@ -259,6 +259,8 @@
#define VBSD_EC_SLOW_UPDATE 0x00001000
/* Firmware software write protect was enabled at boot time */
#define VBSD_BOOT_FIRMWARE_SW_WP_ENABLED 0x00002000
+/* VbInit() was told that the recovery button is a virtual one */
+#define VBSD_BOOT_REC_SWITCH_VIRTUAL 0x00004000
/*
* Supported flags by header version. It's ok to add new flags while keeping
diff --git a/firmware/lib/include/vboot_kernel.h b/firmware/lib/include/vboot_kernel.h
index 48e1253..2804434 100644
--- a/firmware/lib/include/vboot_kernel.h
+++ b/firmware/lib/include/vboot_kernel.h
@@ -44,6 +44,10 @@
uint32_t VbTryLoadKernel(VbCommonParams *cparams, LoadKernelParams *p,
uint32_t get_info_flags);
+/* Flags for VbUserConfirms() */
+#define VB_CONFIRM_MUST_TRUST_KEYBOARD (1 << 0)
+#define VB_CONFIRM_SPACE_MEANS_NO (1 << 1)
+
/**
* Ask the user to confirm something.
*
@@ -52,9 +56,13 @@
* don't return until one of those keys is pressed, or until asked to shut
* down.
*
+ * Additionally, in some situations we don't accept confirmations from an
+ * untrusted keyboard (such as a USB device). In those cases, a recovery
+ * button press is needed for confirmation, instead of ENTER.
+ *
* Returns: 1=yes, 0=no, -1 = shutdown.
*/
-int VbUserConfirms(VbCommonParams *cparams, int space_means_no);
+int VbUserConfirms(VbCommonParams *cparams, uint32_t confirm_flags);
/**
* Handle a normal boot.
diff --git a/firmware/lib/vboot_api_init.c b/firmware/lib/vboot_api_init.c
index ebafe2a..58bc215 100644
--- a/firmware/lib/vboot_api_init.c
+++ b/firmware/lib/vboot_api_init.c
@@ -76,6 +76,8 @@
shared->flags |= VBSD_EC_SOFTWARE_SYNC;
if (iparams->flags & VB_INIT_FLAG_EC_SLOW_UPDATE)
shared->flags |= VBSD_EC_SLOW_UPDATE;
+ if (iparams->flags & VB_INIT_FLAG_VIRTUAL_REC_SWITCH)
+ shared->flags |= VBSD_BOOT_REC_SWITCH_VIRTUAL;
is_s3_resume = (iparams->flags & VB_INIT_FLAG_S3_RESUME ? 1 : 0);
diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index 4cf2a94..7f85dc7 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -130,26 +130,42 @@
#define CONFIRM_KEY_DELAY 20 /* Check confirm screen keys every 20ms */
-int VbUserConfirms(VbCommonParams *cparams, int space_means_no)
+int VbUserConfirms(VbCommonParams *cparams, uint32_t confirm_flags)
{
+ VbSharedDataHeader *shared =
+ (VbSharedDataHeader *)cparams->shared_data_blob;
uint32_t key;
+ uint32_t key_flags;
+ uint32_t button;
+ int rec_button_was_pressed = 0;
- VBDEBUG(("Entering %s(%d)\n", __func__, space_means_no));
+ VBDEBUG(("Entering %s(0x%x)\n", __func__, confirm_flags));
/* Await further instructions */
while (1) {
if (VbExIsShutdownRequested())
return -1;
- key = VbExKeyboardRead();
+ key = VbExKeyboardReadWithFlags(&key_flags);
+ button = VbExGetSwitches(VB_INIT_FLAG_REC_BUTTON_PRESSED);
switch (key) {
case '\r':
+ /* If we require a trusted keyboard for confirmation,
+ * but the keyboard may be faked (for instance, a USB
+ * device), beep and keep waiting.
+ */
+ if (confirm_flags & VB_CONFIRM_MUST_TRUST_KEYBOARD &&
+ !(key_flags & VB_KEY_FLAG_TRUSTED_KEYBOARD)) {
+ VbExBeep(120, 400);
+ break;
+ }
+
VBDEBUG(("%s() - Yes (1)\n", __func__));
return 1;
break;
case ' ':
VBDEBUG(("%s() - Space (%d)\n", __func__,
- space_means_no));
- if (space_means_no)
+ confirm_flags & VB_CONFIRM_SPACE_MEANS_NO));
+ if (confirm_flags & VB_CONFIRM_SPACE_MEANS_NO)
return 0;
break;
case 0x1b:
@@ -157,6 +173,20 @@
return 0;
break;
default:
+ /* If the recovery button is physical, and is pressed,
+ * this is also a YES, but must wait for release.
+ */
+ if (!(shared->flags & VBSD_BOOT_REC_SWITCH_VIRTUAL)) {
+ if (button) {
+ VBDEBUG(("%s() - Rec button pressed\n",
+ __func__));
+ rec_button_was_pressed = 1;
+ } else if (rec_button_was_pressed) {
+ VBDEBUG(("%s() - Rec button (1)\n",
+ __func__));
+ return 1;
+ }
+ }
VbCheckDisplayKey(cparams, key, &vnc);
}
VbExSleepMs(CONFIRM_KEY_DELAY);
@@ -505,12 +535,30 @@
!(shared->flags & VBSD_BOOT_DEV_SWITCH_ON) &&
(shared->flags & VBSD_BOOT_REC_SWITCH_ON) &&
VbExTrustEC()) {
+ if (!(shared->flags &
+ VBSD_BOOT_REC_SWITCH_VIRTUAL) &&
+ VbExGetSwitches(
+ VB_INIT_FLAG_REC_BUTTON_PRESSED)) {
+ /*
+ * Is the recovery button stuck? In
+ * any case we don't like this. Beep
+ * and ignore.
+ */
+ VBDEBUG(("%s() - ^D but rec switch "
+ "is pressed\n", __func__));
+ VbExBeep(120, 400);
+ continue;
+ }
+
/* Ask the user to confirm entering dev-mode */
VbDisplayScreen(cparams,
VB_SCREEN_RECOVERY_TO_DEV,
0, &vnc);
/* SPACE means no... */
- switch (VbUserConfirms(cparams, 1)) {
+ uint32_t vbc_flags =
+ VB_CONFIRM_SPACE_MEANS_NO |
+ VB_CONFIRM_MUST_TRUST_KEYBOARD;
+ switch (VbUserConfirms(cparams, vbc_flags)) {
case 1:
VBDEBUG(("%s() Enabling dev-mode...\n",
__func__));
diff --git a/firmware/stub/vboot_api_stub.c b/firmware/stub/vboot_api_stub.c
index e82be5b..913cac1 100644
--- a/firmware/stub/vboot_api_stub.c
+++ b/firmware/stub/vboot_api_stub.c
@@ -57,6 +57,16 @@
return 0;
}
+uint32_t VbExKeyboardReadWithFlags(uint32_t *flags_ptr)
+{
+ return 0;
+}
+
+uint32_t VbExGetSwitches(uint32_t mask)
+{
+ return 0;
+}
+
uint32_t VbExIsShutdownRequested(void)
{
return 0;
diff --git a/tests/vboot_api_kernel2_tests.c b/tests/vboot_api_kernel2_tests.c
index 21ea306..9dd15ef 100644
--- a/tests/vboot_api_kernel2_tests.c
+++ b/tests/vboot_api_kernel2_tests.c
@@ -34,9 +34,12 @@
static int trust_ec;
static int virtdev_set;
static uint32_t virtdev_retval;
-
static uint32_t mock_keypress[8];
+static uint32_t mock_keyflags[8];
static uint32_t mock_keypress_count;
+static uint32_t mock_switches[8];
+static uint32_t mock_switches_count;
+static int mock_switches_are_stuck;
static uint32_t screens_displayed[8];
static uint32_t screens_count = 0;
static uint32_t mock_num_disks[8];
@@ -81,8 +84,13 @@
screens_count = 0;
Memset(mock_keypress, 0, sizeof(mock_keypress));
+ Memset(mock_keyflags, 0, sizeof(mock_keyflags));
mock_keypress_count = 0;
+ Memset(mock_switches, 0, sizeof(mock_switches));
+ mock_switches_count = 0;
+ mock_switches_are_stuck = 0;
+
Memset(mock_num_disks, 0, sizeof(mock_num_disks));
mock_num_disks_count = 0;
}
@@ -101,8 +109,25 @@
uint32_t VbExKeyboardRead(void)
{
- if (mock_keypress_count < ARRAY_SIZE(mock_keypress))
+ return VbExKeyboardReadWithFlags(NULL);
+}
+
+uint32_t VbExKeyboardReadWithFlags(uint32_t *key_flags)
+{
+ if (mock_keypress_count < ARRAY_SIZE(mock_keypress)) {
+ if (key_flags != NULL)
+ *key_flags = mock_keyflags[mock_keypress_count];
return mock_keypress[mock_keypress_count++];
+ } else
+ return 0;
+}
+
+uint32_t VbExGetSwitches(uint32_t request_mask)
+{
+ if (mock_switches_are_stuck)
+ return mock_switches[0] & request_mask;
+ if (mock_switches_count < ARRAY_SIZE(mock_switches))
+ return mock_switches[mock_switches_count++] & request_mask;
else
return 0;
}
@@ -190,13 +215,47 @@
ResetMocks();
mock_keypress[0] = ' ';
shutdown_request_calls_left = 1;
- TEST_EQ(VbUserConfirms(&cparams, 1), 0, "Space means no");
+ TEST_EQ(VbUserConfirms(&cparams, VB_CONFIRM_SPACE_MEANS_NO), 0,
+ "Space means no");
ResetMocks();
mock_keypress[0] = ' ';
shutdown_request_calls_left = 1;
TEST_EQ(VbUserConfirms(&cparams, 0), -1, "Space ignored");
+ ResetMocks();
+ mock_keypress[0] = '\r';
+ mock_keyflags[0] = VB_KEY_FLAG_TRUSTED_KEYBOARD;
+ TEST_EQ(VbUserConfirms(&cparams, VB_CONFIRM_MUST_TRUST_KEYBOARD),
+ 1, "Enter with trusted keyboard");
+
+ ResetMocks();
+ mock_keypress[0] = '\r'; /* untrusted */
+ mock_keypress[1] = ' ';
+ TEST_EQ(VbUserConfirms(&cparams,
+ VB_CONFIRM_SPACE_MEANS_NO |
+ VB_CONFIRM_MUST_TRUST_KEYBOARD),
+ 0, "Untrusted keyboard");
+
+ ResetMocks();
+ mock_switches[0] = VB_INIT_FLAG_REC_BUTTON_PRESSED;
+ TEST_EQ(VbUserConfirms(&cparams,
+ VB_CONFIRM_SPACE_MEANS_NO |
+ VB_CONFIRM_MUST_TRUST_KEYBOARD),
+ 1, "Recovery button");
+
+ ResetMocks();
+ mock_keypress[0] = '\r';
+ mock_keypress[1] = 'y';
+ mock_keypress[2] = 'z';
+ mock_keypress[3] = ' ';
+ mock_switches[0] = VB_INIT_FLAG_REC_BUTTON_PRESSED;
+ mock_switches_are_stuck = 1;
+ TEST_EQ(VbUserConfirms(&cparams,
+ VB_CONFIRM_SPACE_MEANS_NO |
+ VB_CONFIRM_MUST_TRUST_KEYBOARD),
+ 0, "Recovery button stuck");
+
printf("...done.\n");
}
@@ -529,6 +588,20 @@
TEST_NEQ(screens_displayed[1], VB_SCREEN_RECOVERY_TO_DEV,
" todev screen");
+ /* Ctrl+D ignored because the physical recovery switch is still pressed
+ * and we don't like that.
+ */
+ ResetMocks();
+ shared->flags = VBSD_BOOT_REC_SWITCH_ON;
+ trust_ec = 1;
+ shutdown_request_calls_left = 100;
+ mock_keypress[0] = 0x04;
+ mock_switches[0] = VB_INIT_FLAG_REC_BUTTON_PRESSED;
+ TEST_EQ(VbBootRecovery(&cparams, &lkp), VBERROR_SHUTDOWN_REQUESTED,
+ "Ctrl+D ignored if phys rec button is still pressed");
+ TEST_NEQ(screens_displayed[1], VB_SCREEN_RECOVERY_TO_DEV,
+ " todev screen");
+
/* Ctrl+D then space means don't enable */
ResetMocks();
shared->flags = VBSD_HONOR_VIRT_DEV_SWITCH | VBSD_BOOT_REC_SWITCH_ON;
@@ -555,6 +628,7 @@
trust_ec = 1;
mock_keypress[0] = 0x04;
mock_keypress[1] = '\r';
+ mock_keyflags[1] = VB_KEY_FLAG_TRUSTED_KEYBOARD;
TEST_EQ(VbBootRecovery(&cparams, &lkp), VBERROR_TPM_REBOOT_REQUIRED,
"Ctrl+D todev confirm");
TEST_EQ(virtdev_set, 1, " virtual dev mode on");
@@ -567,6 +641,7 @@
trust_ec = 1;
mock_keypress[0] = 0x04;
mock_keypress[1] = '\r';
+ mock_keyflags[1] = VB_KEY_FLAG_TRUSTED_KEYBOARD;
virtdev_retval = VBERROR_SIMULATED;
TEST_EQ(VbBootRecovery(&cparams, &lkp), VBERROR_TPM_SET_BOOT_MODE_STATE,
"Ctrl+D todev failure");
