From db887e63d708925ad759e3504a6bc9ceef4aeb8f Mon Sep 17 00:00:00 2001 From: Drashna Jaelre Date: Sat, 14 May 2022 15:07:08 -0700 Subject: [PATCH] Enhancement and fixes of "Secure" feature (#16958) --- quantum/process_keycode/process_secure.c | 10 +- quantum/quantum.c | 13 ++ quantum/quantum_keycodes.h | 1 + quantum/secure.c | 15 ++ quantum/secure.h | 12 + tests/secure/config.h | 32 +++ tests/secure/test.mk | 20 ++ tests/secure/test_secure.cpp | 278 +++++++++++++++++++++++ 8 files changed, 379 insertions(+), 2 deletions(-) create mode 100644 tests/secure/config.h create mode 100644 tests/secure/test.mk create mode 100644 tests/secure/test_secure.cpp diff --git a/quantum/process_keycode/process_secure.c b/quantum/process_keycode/process_secure.c index 827ace597a8..3224104c99b 100644 --- a/quantum/process_keycode/process_secure.c +++ b/quantum/process_keycode/process_secure.c @@ -7,7 +7,9 @@ bool preprocess_secure(uint16_t keycode, keyrecord_t *record) { if (secure_is_unlocking()) { - if (!record->event.pressed) { + // !pressed will trigger on any already held keys (such as layer keys), + // and cause the request secure check to prematurely fail. + if (record->event.pressed) { secure_keypress_event(record->event.key.row, record->event.key.col); } @@ -33,7 +35,11 @@ bool process_secure(uint16_t keycode, keyrecord_t *record) { secure_is_locked() ? secure_unlock() : secure_lock(); return false; } + if (keycode == SECURE_REQUEST) { + secure_request_unlock(); + return false; + } } #endif return true; -} \ No newline at end of file +} diff --git a/quantum/quantum.c b/quantum/quantum.c index b54b46760c8..ac3e2d90b4d 100644 --- a/quantum/quantum.c +++ b/quantum/quantum.c @@ -571,3 +571,16 @@ const char *get_u16_str(uint16_t curr_num, char curr_pad) { last_pad = curr_pad; return get_numeric_str(buf, sizeof(buf), curr_num, curr_pad); } + +#if defined(SECURE_ENABLE) +void secure_hook_quantum(secure_status_t secure_status) { + // If keys are being held when this is triggered, they may not be released properly + // this can result in stuck keys, mods and layers. To prevent that, manually + // clear these, when it is triggered. + + if (secure_status == SECURE_PENDING) { + clear_keyboard(); + layer_clear(); + } +} +#endif diff --git a/quantum/quantum_keycodes.h b/quantum/quantum_keycodes.h index 2f8ee2322e8..40355d799af 100644 --- a/quantum/quantum_keycodes.h +++ b/quantum/quantum_keycodes.h @@ -601,6 +601,7 @@ enum quantum_keycodes { SECURE_LOCK, SECURE_UNLOCK, SECURE_TOGGLE, + SECURE_REQUEST, CAPS_WORD, diff --git a/quantum/secure.c b/quantum/secure.c index 00048bd6dd5..f07f6af2cb2 100644 --- a/quantum/secure.c +++ b/quantum/secure.c @@ -23,17 +23,24 @@ static secure_status_t secure_status = SECURE_LOCKED; static uint32_t unlock_time = 0; static uint32_t idle_time = 0; +static void secure_hook(secure_status_t secure_status) { + secure_hook_quantum(secure_status); + secure_hook_kb(secure_status); +} + secure_status_t secure_get_status(void) { return secure_status; } void secure_lock(void) { secure_status = SECURE_LOCKED; + secure_hook(secure_status); } void secure_unlock(void) { secure_status = SECURE_UNLOCKED; idle_time = timer_read32(); + secure_hook(secure_status); } void secure_request_unlock(void) { @@ -41,6 +48,7 @@ void secure_request_unlock(void) { secure_status = SECURE_PENDING; unlock_time = timer_read32(); } + secure_hook(secure_status); } void secure_activity_event(void) { @@ -85,3 +93,10 @@ void secure_task(void) { } #endif } + +__attribute__((weak)) bool secure_hook_user(secure_status_t secure_status) { + return true; +} +__attribute__((weak)) bool secure_hook_kb(secure_status_t secure_status) { + return secure_hook_user(secure_status); +} diff --git a/quantum/secure.h b/quantum/secure.h index 04507fd5b13..bb2ba50f316 100644 --- a/quantum/secure.h +++ b/quantum/secure.h @@ -65,3 +65,15 @@ void secure_keypress_event(uint8_t row, uint8_t col); /** \brief Handle various secure subsystem background tasks */ void secure_task(void); + +/** \brief quantum hook called when changing secure status device + */ +void secure_hook_quantum(secure_status_t secure_status); + +/** \brief user hook called when changing secure status device + */ +bool secure_hook_user(secure_status_t secure_status); + +/** \brief keyboard hook called when changing secure status device + */ +bool secure_hook_kb(secure_status_t secure_status); diff --git a/tests/secure/config.h b/tests/secure/config.h new file mode 100644 index 00000000000..3cfbc6cb14f --- /dev/null +++ b/tests/secure/config.h @@ -0,0 +1,32 @@ +/* Copyright 2021 Stefan Kerkmann + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#pragma once + +#include "test_common.h" + +// clang-format off +#define SECURE_UNLOCK_SEQUENCE \ + { \ + {0, 1}, \ + {0, 2}, \ + {0, 3}, \ + {0, 4} \ + } +// clang-format on + +#define SECURE_UNLOCK_TIMEOUT 20 +#define SECURE_IDLE_TIMEOUT 50 diff --git a/tests/secure/test.mk b/tests/secure/test.mk new file mode 100644 index 00000000000..ea406493bed --- /dev/null +++ b/tests/secure/test.mk @@ -0,0 +1,20 @@ +# Copyright 2021 Stefan Kerkmann +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# -------------------------------------------------------------------------------- +# Keep this file, even if it is empty, as a marker that this folder contains tests +# -------------------------------------------------------------------------------- + +SECURE_ENABLE = yes diff --git a/tests/secure/test_secure.cpp b/tests/secure/test_secure.cpp new file mode 100644 index 00000000000..87055ebb7fd --- /dev/null +++ b/tests/secure/test_secure.cpp @@ -0,0 +1,278 @@ +/* Copyright 2021 Stefan Kerkmann + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#include "gtest/gtest.h" +#include "keyboard_report_util.hpp" +#include "test_common.hpp" + +using testing::_; +using testing::AnyNumber; +using testing::InSequence; + +class Secure : public TestFixture { + public: + void SetUp() override { + secure_lock(); + } + // Convenience function to tap `key`. + void TapKey(KeymapKey key) { + key.press(); + run_one_scan_loop(); + key.release(); + run_one_scan_loop(); + } + + // Taps in order each key in `keys`. + template + void TapKeys(Ts... keys) { + for (KeymapKey key : {keys...}) { + TapKey(key); + } + } +}; + +TEST_F(Secure, test_lock) { + TestDriver driver; + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); + + EXPECT_FALSE(secure_is_unlocked()); + secure_unlock(); + EXPECT_TRUE(secure_is_unlocked()); + run_one_scan_loop(); + EXPECT_TRUE(secure_is_unlocked()); + secure_lock(); + EXPECT_FALSE(secure_is_unlocked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + +TEST_F(Secure, test_unlock_timeout) { + TestDriver driver; + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); + + EXPECT_FALSE(secure_is_unlocked()); + secure_unlock(); + EXPECT_TRUE(secure_is_unlocked()); + idle_for(SECURE_IDLE_TIMEOUT+1); + EXPECT_FALSE(secure_is_unlocked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + +TEST_F(Secure, test_unlock_request) { + TestDriver driver; + auto key_mo = KeymapKey(0, 0, 0, MO(1)); + auto key_a = KeymapKey(0, 1, 0, KC_A); + auto key_b = KeymapKey(0, 2, 0, KC_B); + auto key_c = KeymapKey(0, 3, 0, KC_C); + auto key_d = KeymapKey(0, 4, 0, KC_D); + + set_keymap({key_mo, key_a, key_b, key_c, key_d}); + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); + + EXPECT_TRUE(secure_is_locked()); + secure_request_unlock(); + EXPECT_TRUE(secure_is_unlocking()); + TapKeys(key_a, key_b, key_c, key_d); + EXPECT_TRUE(secure_is_unlocked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + +TEST_F(Secure, test_unlock_request_fail) { + TestDriver driver; + auto key_e = KeymapKey(0, 0, 0, KC_E); + auto key_a = KeymapKey(0, 1, 0, KC_A); + auto key_b = KeymapKey(0, 2, 0, KC_B); + auto key_c = KeymapKey(0, 3, 0, KC_C); + auto key_d = KeymapKey(0, 4, 0, KC_D); + + set_keymap({key_e, key_a, key_b, key_c, key_d}); + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(AnyNumber()); + { // Expect the following reports in this order. + InSequence s; + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_A))); + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_B))); + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_C))); + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_D))); + } + EXPECT_TRUE(secure_is_locked()); + secure_request_unlock(); + EXPECT_TRUE(secure_is_unlocking()); + TapKeys(key_e, key_a, key_b, key_c, key_d); + EXPECT_FALSE(secure_is_unlocked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + +TEST_F(Secure, test_unlock_request_timeout) { + TestDriver driver; + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); + + EXPECT_FALSE(secure_is_unlocked()); + secure_request_unlock(); + EXPECT_TRUE(secure_is_unlocking()); + idle_for(SECURE_UNLOCK_TIMEOUT+1); + EXPECT_FALSE(secure_is_unlocking()); + EXPECT_FALSE(secure_is_unlocked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + + +TEST_F(Secure, test_unlock_request_fail_mid) { + TestDriver driver; + auto key_e = KeymapKey(0, 0, 0, KC_E); + auto key_a = KeymapKey(0, 1, 0, KC_A); + auto key_b = KeymapKey(0, 2, 0, KC_B); + auto key_c = KeymapKey(0, 3, 0, KC_C); + auto key_d = KeymapKey(0, 4, 0, KC_D); + + set_keymap({key_e, key_a, key_b, key_c, key_d}); + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(AnyNumber()); + { // Expect the following reports in this order. + InSequence s; + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_C))); + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_D))); + } + EXPECT_FALSE(secure_is_unlocked()); + secure_request_unlock(); + EXPECT_TRUE(secure_is_unlocking()); + TapKeys(key_a, key_b, key_e, key_c, key_d); + EXPECT_FALSE(secure_is_unlocking()); + EXPECT_FALSE(secure_is_unlocked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + +TEST_F(Secure, test_unlock_request_fail_out_of_order) { + TestDriver driver; + auto key_e = KeymapKey(0, 0, 0, KC_E); + auto key_a = KeymapKey(0, 1, 0, KC_A); + auto key_b = KeymapKey(0, 2, 0, KC_B); + auto key_c = KeymapKey(0, 3, 0, KC_C); + auto key_d = KeymapKey(0, 4, 0, KC_D); + + set_keymap({key_e, key_a, key_b, key_c, key_d}); + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(AnyNumber()); + { // Expect the following reports in this order. + InSequence s; + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_B))); + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_C))); + } + EXPECT_FALSE(secure_is_unlocked()); + secure_request_unlock(); + EXPECT_TRUE(secure_is_unlocking()); + TapKeys(key_a, key_d, key_b, key_c); + EXPECT_TRUE(secure_is_locked()); + EXPECT_FALSE(secure_is_unlocking()); + EXPECT_FALSE(secure_is_unlocked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + +TEST_F(Secure, test_unlock_request_on_layer) { + TestDriver driver; + auto key_mo = KeymapKey(0, 0, 0, MO(1)); + auto key_a = KeymapKey(0, 1, 0, KC_A); + auto key_b = KeymapKey(0, 2, 0, KC_B); + auto key_c = KeymapKey(0, 3, 0, KC_C); + auto key_d = KeymapKey(0, 4, 0, KC_D); + + set_keymap({key_mo, key_a, key_b, key_c, key_d}); + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); + + EXPECT_TRUE(secure_is_locked()); + key_mo.press(); + run_one_scan_loop(); + secure_request_unlock(); + key_mo.release(); + run_one_scan_loop(); + EXPECT_TRUE(secure_is_unlocking()); + TapKeys(key_a, key_b, key_c, key_d); + EXPECT_TRUE(secure_is_unlocked()); + EXPECT_FALSE(layer_state_is(1)); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + +TEST_F(Secure, test_unlock_request_mid_stroke) { + TestDriver driver; + auto key_e = KeymapKey(0, 0, 0, KC_E); + auto key_a = KeymapKey(0, 1, 0, KC_A); + auto key_b = KeymapKey(0, 2, 0, KC_B); + auto key_c = KeymapKey(0, 3, 0, KC_C); + auto key_d = KeymapKey(0, 4, 0, KC_D); + + set_keymap({key_e, key_a, key_b, key_c, key_d}); + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_E))); + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())); + EXPECT_TRUE(secure_is_locked()); + key_e.press(); + run_one_scan_loop(); + secure_request_unlock(); + key_e.release(); + run_one_scan_loop(); + EXPECT_TRUE(secure_is_unlocking()); + TapKeys(key_a, key_b, key_c, key_d); + EXPECT_TRUE(secure_is_unlocked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + +TEST_F(Secure, test_unlock_request_mods) { + TestDriver driver; + auto key_lsft = KeymapKey(0, 0, 0, KC_LSFT); + auto key_a = KeymapKey(0, 1, 0, KC_A); + auto key_b = KeymapKey(0, 2, 0, KC_B); + auto key_c = KeymapKey(0, 3, 0, KC_C); + auto key_d = KeymapKey(0, 4, 0, KC_D); + + set_keymap({key_lsft, key_a, key_b, key_c, key_d}); + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(key_lsft.report_code))); + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())); + EXPECT_TRUE(secure_is_locked()); + key_lsft.press(); + run_one_scan_loop(); + secure_request_unlock(); + key_lsft.release(); + run_one_scan_loop(); + EXPECT_TRUE(secure_is_unlocking()); + TapKeys(key_a, key_b, key_c, key_d); + EXPECT_TRUE(secure_is_unlocked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +}