From c17d19bcc84dca215498960797c8ce7ccaebb4f7 Mon Sep 17 00:00:00 2001 From: Luigi Semenzato Date: Fri, 10 Jan 2014 16:26:08 -0800 Subject: 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/183053 Reviewed-by: Luigi Semenzato Commit-Queue: Luigi Semenzato Tested-by: Luigi Semenzato --- firmware/include/vboot_api.h | 23 +++++++++++ firmware/include/vboot_struct.h | 2 + firmware/lib/include/vboot_kernel.h | 10 ++++- firmware/lib/vboot_api_init.c | 2 + firmware/lib/vboot_api_kernel.c | 60 ++++++++++++++++++++++++--- firmware/stub/vboot_api_stub.c | 10 +++++ tests/vboot_api_kernel2_tests.c | 81 +++++++++++++++++++++++++++++++++++-- 7 files changed, 178 insertions(+), 10 deletions(-) diff --git a/firmware/include/vboot_api.h b/firmware/include/vboot_api.h index 214da753..f4a91347 100644 --- a/firmware/include/vboot_api.h +++ b/firmware/include/vboot_api.h @@ -221,6 +221,11 @@ typedef struct VbCommonParams { * 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 @@ enum VbKeyCode_t { 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 @@ enum VbKeyCode_t { * 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 e20b0aa9..7fa072f1 100644 --- a/firmware/include/vboot_struct.h +++ b/firmware/include/vboot_struct.h @@ -259,6 +259,8 @@ typedef struct VbKernelPreambleHeader { #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 48e12536..2804434f 100644 --- a/firmware/lib/include/vboot_kernel.h +++ b/firmware/lib/include/vboot_kernel.h @@ -44,6 +44,10 @@ void VbApiKernelFree(VbCommonParams *cparams); 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 @@ uint32_t VbTryLoadKernel(VbCommonParams *cparams, LoadKernelParams *p, * 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 ebafe2a7..58bc215f 100644 --- a/firmware/lib/vboot_api_init.c +++ b/firmware/lib/vboot_api_init.c @@ -76,6 +76,8 @@ VbError_t VbInit(VbCommonParams *cparams, VbInitParams *iparams) 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 ebbc9b04..57635aa8 100644 --- a/firmware/lib/vboot_api_kernel.c +++ b/firmware/lib/vboot_api_kernel.c @@ -130,26 +130,42 @@ uint32_t VbTryLoadKernel(VbCommonParams *cparams, LoadKernelParams *p, #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 @@ int VbUserConfirms(VbCommonParams *cparams, int space_means_no) 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 @@ VbError_t VbBootRecovery(VbCommonParams *cparams, LoadKernelParams *p) !(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 36d56032..1fb9d9b2 100644 --- a/firmware/stub/vboot_api_stub.c +++ b/firmware/stub/vboot_api_stub.c @@ -57,6 +57,16 @@ uint32_t VbExKeyboardRead(void) 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 21ea3063..9dd15ef3 100644 --- a/tests/vboot_api_kernel2_tests.c +++ b/tests/vboot_api_kernel2_tests.c @@ -34,9 +34,12 @@ static int vbexlegacy_called; 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 @@ static void ResetMocks(void) 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 VbExIsShutdownRequested(void) 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 @@ static void VbUserConfirmsTest(void) 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 @@ static void VbBootRecTest(void) 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 @@ static void VbBootRecTest(void) 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 @@ static void VbBootRecTest(void) 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"); -- cgit v1.2.1