vboot: use recovery button as dev mode switch confirmationstabilize-5339.B

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>

7 files changed, 178 insertions, 10 deletions

diff --git a/firmware/include/vboot_api.h b/firmware/include/vboot_api.h
index 0ae6c76b..92b9411d 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 4cf2a946..7f85dc75 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 e82be5b2..913cac15 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");