From 139a53a484702caf30747ae0823534343c57395d Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 12:10:57 -0700 Subject: [PATCH 01/21] Add hooks for Secure feature --- quantum/quantum.c | 13 +++++++++++++ quantum/secure.c | 3 +++ quantum/secure.h | 12 ++++++++++++ 3 files changed, 28 insertions(+) diff --git a/quantum/quantum.c b/quantum/quantum.c index 673ea91b11f9..8b0068bf3733 100644 --- a/quantum/quantum.c +++ b/quantum/quantum.c @@ -553,3 +553,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) +__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); +} + +void secure_hook_quantum(secure_status_t secure_status) { + secure_hook_kb(secure_status); +} +#endif diff --git a/quantum/secure.c b/quantum/secure.c index 00048bd6dd55..1514390a354f 100644 --- a/quantum/secure.c +++ b/quantum/secure.c @@ -29,11 +29,13 @@ secure_status_t secure_get_status(void) { void secure_lock(void) { secure_status = SECURE_LOCKED; + secure_hook_quantum(secure_status); } void secure_unlock(void) { secure_status = SECURE_UNLOCKED; idle_time = timer_read32(); + secure_hook_quantum(secure_status); } void secure_request_unlock(void) { @@ -41,6 +43,7 @@ void secure_request_unlock(void) { secure_status = SECURE_PENDING; unlock_time = timer_read32(); } + secure_hook_quantum(secure_status); } void secure_activity_event(void) { diff --git a/quantum/secure.h b/quantum/secure.h index 04507fd5b139..bb2ba50f3166 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); From f3eae10681819655f6d0663f88b409de050c589f Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 12:12:07 -0700 Subject: [PATCH 02/21] Fix issue with stuck keys when secure request is triggered Since the pending request blocks all keystrokes, this can/will cause keys to get stuck if they were being held when the request function was called. To prevent this, clear all pressed keys and clear layers to ensure that no keys get stuck. --- quantum/secure.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/quantum/secure.c b/quantum/secure.c index 1514390a354f..ab694a6781f0 100644 --- a/quantum/secure.c +++ b/quantum/secure.c @@ -3,6 +3,7 @@ #include "secure.h" #include "timer.h" +#include "action_layer.h" #ifndef SECURE_UNLOCK_TIMEOUT # define SECURE_UNLOCK_TIMEOUT 5000 @@ -42,6 +43,11 @@ void secure_request_unlock(void) { if (secure_status == SECURE_LOCKED) { secure_status = SECURE_PENDING; unlock_time = timer_read32(); + // 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. + clear_keyboard(); + layer_clear(); } secure_hook_quantum(secure_status); } From b5a1d9ff2e3641e604e4b2c4a61c54a576cd04e1 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 12:14:00 -0700 Subject: [PATCH 03/21] Fix issues with premature failure of secure locking request Again, with the pending request, if keys are already being held, they will trigger the !pressed event on release, ending the check prematurely. Such as having a request key being on a layer. To prevent this from happening, using the pressed event works better. (If needed, a preprocessor option can be added to this) --- quantum/process_keycode/process_secure.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/quantum/process_keycode/process_secure.c b/quantum/process_keycode/process_secure.c index 827ace597a8c..41c764620767 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); } @@ -36,4 +38,4 @@ bool process_secure(uint16_t keycode, keyrecord_t *record) { } #endif return true; -} \ No newline at end of file +} From a3da2ca9334c838116c38b8427f7407caec8fdbb Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 13:44:33 -0700 Subject: [PATCH 04/21] changes based on feedback --- quantum/quantum.c | 13 ------------- quantum/secure.c | 18 +++++++++++++++++- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/quantum/quantum.c b/quantum/quantum.c index 8b0068bf3733..673ea91b11f9 100644 --- a/quantum/quantum.c +++ b/quantum/quantum.c @@ -553,16 +553,3 @@ 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) -__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); -} - -void secure_hook_quantum(secure_status_t secure_status) { - secure_hook_kb(secure_status); -} -#endif diff --git a/quantum/secure.c b/quantum/secure.c index ab694a6781f0..6a2256c39274 100644 --- a/quantum/secure.c +++ b/quantum/secure.c @@ -3,7 +3,7 @@ #include "secure.h" #include "timer.h" -#include "action_layer.h" + #ifndef SECURE_UNLOCK_TIMEOUT # define SECURE_UNLOCK_TIMEOUT 5000 @@ -39,6 +39,9 @@ void secure_unlock(void) { secure_hook_quantum(secure_status); } +void clear_keyboard(void); +void layer_clear(void); + void secure_request_unlock(void) { if (secure_status == SECURE_LOCKED) { secure_status = SECURE_PENDING; @@ -94,3 +97,16 @@ void secure_task(void) { } #endif } + +#if defined(SECURE_ENABLE) +__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); +} + +__attribute__((weak)) void secure_hook_quantum(secure_status_t secure_status) { + secure_hook_kb(secure_status); +} +#endif From bc3773b18d0983407a0c528dde48704571f3196f Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 13:44:57 -0700 Subject: [PATCH 05/21] add tests for secure --- tests/secure/config.h | 24 +++++ tests/secure/test.mk | 20 +++++ tests/secure/test_secure.cpp | 170 +++++++++++++++++++++++++++++++++++ 3 files changed, 214 insertions(+) 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/tests/secure/config.h b/tests/secure/config.h new file mode 100644 index 000000000000..12403b4c6b71 --- /dev/null +++ b/tests/secure/config.h @@ -0,0 +1,24 @@ +/* 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" + +#define SECURE_UNLOCK_SEQUENCE \ + { \ + { 0, 1 }, { 0, 2 }, { 0, 3 }, { 0, 4 } \ + } diff --git a/tests/secure/test.mk b/tests/secure/test.mk new file mode 100644 index 000000000000..ea406493bede --- /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 000000000000..c0767e69172f --- /dev/null +++ b/tests/secure/test_secure.cpp @@ -0,0 +1,170 @@ +/* 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_unlock) { + TestDriver driver; + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); + + secure_unlock(); + EXPECT_TRUE(secure_is_unlocked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + +TEST_F(Secure, test_lock) { + TestDriver driver; + + // Allow any number of empty reports. + EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); + + secure_unlock(); + run_one_scan_loop(); + secure_lock(); + EXPECT_TRUE(secure_is_locked()); + + 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); + + 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_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); + + 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())); + 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())); + 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); +} From efbbc135400296892256d879965dca55bd05aebe Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 16:30:28 -0700 Subject: [PATCH 06/21] Move board clear calls --- quantum/quantum.c | 5 +++++ quantum/secure.c | 8 -------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/quantum/quantum.c b/quantum/quantum.c index 673ea91b11f9..15acf84dc4d0 100644 --- a/quantum/quantum.c +++ b/quantum/quantum.c @@ -214,6 +214,11 @@ bool process_record_quantum(keyrecord_t *record) { #if defined(SECURE_ENABLE) if (!preprocess_secure(keycode, record)) { + // 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. + clear_keyboard(); + layer_clear(); return false; } #endif diff --git a/quantum/secure.c b/quantum/secure.c index 6a2256c39274..971d3a94e41c 100644 --- a/quantum/secure.c +++ b/quantum/secure.c @@ -39,18 +39,10 @@ void secure_unlock(void) { secure_hook_quantum(secure_status); } -void clear_keyboard(void); -void layer_clear(void); - void secure_request_unlock(void) { if (secure_status == SECURE_LOCKED) { secure_status = SECURE_PENDING; unlock_time = timer_read32(); - // 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. - clear_keyboard(); - layer_clear(); } secure_hook_quantum(secure_status); } From b93392c9e6fcae123d473a1ea777cbf34301cab7 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 16:31:18 -0700 Subject: [PATCH 07/21] Appease lint --- quantum/secure.c | 1 - 1 file changed, 1 deletion(-) diff --git a/quantum/secure.c b/quantum/secure.c index 971d3a94e41c..1ac8a9880414 100644 --- a/quantum/secure.c +++ b/quantum/secure.c @@ -4,7 +4,6 @@ #include "secure.h" #include "timer.h" - #ifndef SECURE_UNLOCK_TIMEOUT # define SECURE_UNLOCK_TIMEOUT 5000 #endif From 813c0c7c79a5c653e7ac786f39ae129511ab0174 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 16:31:58 -0700 Subject: [PATCH 08/21] Appease lint in weirdest way possible --- tests/secure/config.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/secure/config.h b/tests/secure/config.h index 12403b4c6b71..b432eece5c6c 100644 --- a/tests/secure/config.h +++ b/tests/secure/config.h @@ -18,7 +18,9 @@ #include "test_common.h" -#define SECURE_UNLOCK_SEQUENCE \ - { \ - { 0, 1 }, { 0, 2 }, { 0, 3 }, { 0, 4 } \ +#define SECURE_UNLOCK_SEQUENCE \ + { \ + {0, 1}, {0, 2}, {0, 3}, { \ + 0, 4 \ + } \ } From 1d6fc0a35d7e6ecd698db024168f02ccc7318da1 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 17:04:29 -0700 Subject: [PATCH 09/21] Only clear on key up --- quantum/quantum.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/quantum/quantum.c b/quantum/quantum.c index 15acf84dc4d0..a1ea0f68837c 100644 --- a/quantum/quantum.c +++ b/quantum/quantum.c @@ -217,8 +217,10 @@ bool process_record_quantum(keyrecord_t *record) { // 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. - clear_keyboard(); - layer_clear(); + if (!record->event.pressed) { + clear_keyboard(); + layer_clear(); + } return false; } #endif From 41659c306462dced3ed11275ba0b4db511a7fb80 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 18:31:46 -0700 Subject: [PATCH 10/21] Add sequence fail check --- tests/secure/test_secure.cpp | 87 +++++++++++++++++++++++++++++++++--- 1 file changed, 81 insertions(+), 6 deletions(-) diff --git a/tests/secure/test_secure.cpp b/tests/secure/test_secure.cpp index c0767e69172f..56a0d45084ff 100644 --- a/tests/secure/test_secure.cpp +++ b/tests/secure/test_secure.cpp @@ -14,7 +14,6 @@ * along with this program. If not, see . */ - #include "gtest/gtest.h" #include "keyboard_report_util.hpp" #include "test_common.hpp" @@ -92,6 +91,83 @@ TEST_F(Secure, test_unlock_request) { 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))); + } + 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_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))); + } + secure_request_unlock(); + EXPECT_TRUE(secure_is_unlocking()); + TapKeys(key_a, key_b, key_e, key_c, key_d); + 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))); + } + secure_request_unlock(); + EXPECT_TRUE(secure_is_unlocking()); + TapKeys(key_a, key_d, key_b, key_c); + 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)); @@ -143,14 +219,13 @@ TEST_F(Secure, test_unlock_request_mid_stroke) { 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); + 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}); From 78f884a5bf6fdba2d81e9d99072bc9bd45c677c1 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 18:55:14 -0700 Subject: [PATCH 11/21] Add timeout cases --- tests/secure/config.h | 3 +++ tests/secure/test_secure.cpp | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/tests/secure/config.h b/tests/secure/config.h index b432eece5c6c..64c389f85e0e 100644 --- a/tests/secure/config.h +++ b/tests/secure/config.h @@ -24,3 +24,6 @@ 0, 4 \ } \ } + +#define SECURE_UNLOCK_TIMEOUT 20 +#define SECURE_IDLE_TIMEOUT 50 diff --git a/tests/secure/test_secure.cpp b/tests/secure/test_secure.cpp index 56a0d45084ff..05bf98da6d3b 100644 --- a/tests/secure/test_secure.cpp +++ b/tests/secure/test_secure.cpp @@ -70,6 +70,19 @@ TEST_F(Secure, test_lock) { 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); + + secure_unlock(); + idle_for(SECURE_UNLOCK_TIMEOUT+1); + EXPECT_FALSE(secure_is_locked()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + TEST_F(Secure, test_unlock_request) { TestDriver driver; auto key_mo = KeymapKey(0, 0, 0, MO(1)); @@ -118,6 +131,21 @@ TEST_F(Secure, test_unlock_request_fail) { 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); + + secure_request_unlock(); + EXPECT_TRUE(secure_is_unlocking()); + idle_for(SECURE_IDLE_TIMEOUT+1); + EXPECT_FALSE(secure_is_locked() && secure_is_unlocking()); + + testing::Mock::VerifyAndClearExpectations(&driver); +} + + TEST_F(Secure, test_unlock_request_fail_mid) { TestDriver driver; auto key_e = KeymapKey(0, 0, 0, KC_E); From ebacefee046226ad8166533941d839e3146f4722 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 18:56:32 -0700 Subject: [PATCH 12/21] Add separate cases --- tests/secure/test_secure.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/secure/test_secure.cpp b/tests/secure/test_secure.cpp index 05bf98da6d3b..fa0380ba5b68 100644 --- a/tests/secure/test_secure.cpp +++ b/tests/secure/test_secure.cpp @@ -140,7 +140,8 @@ TEST_F(Secure, test_unlock_request_timeout) { secure_request_unlock(); EXPECT_TRUE(secure_is_unlocking()); idle_for(SECURE_IDLE_TIMEOUT+1); - EXPECT_FALSE(secure_is_locked() && secure_is_unlocking()); + EXPECT_FALSE(secure_is_locked()); + EXPECT_FALSE(secure_is_unlocking()); testing::Mock::VerifyAndClearExpectations(&driver); } From c14cdb3a878f4d6e3cff1361f05bb261e8b17a32 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 19:01:16 -0700 Subject: [PATCH 13/21] Add extra expected checks --- tests/secure/test_secure.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/secure/test_secure.cpp b/tests/secure/test_secure.cpp index fa0380ba5b68..596e9bc2a9ba 100644 --- a/tests/secure/test_secure.cpp +++ b/tests/secure/test_secure.cpp @@ -63,6 +63,7 @@ TEST_F(Secure, test_lock) { EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); secure_unlock(); + EXPECT_TRUE(secure_is_unlocked()); run_one_scan_loop(); secure_lock(); EXPECT_TRUE(secure_is_locked()); @@ -140,8 +141,8 @@ TEST_F(Secure, test_unlock_request_timeout) { secure_request_unlock(); EXPECT_TRUE(secure_is_unlocking()); idle_for(SECURE_IDLE_TIMEOUT+1); - EXPECT_FALSE(secure_is_locked()); EXPECT_FALSE(secure_is_unlocking()); + EXPECT_FALSE(secure_is_unlocked()); testing::Mock::VerifyAndClearExpectations(&driver); } @@ -167,6 +168,7 @@ TEST_F(Secure, test_unlock_request_fail_mid) { 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); @@ -192,6 +194,7 @@ TEST_F(Secure, test_unlock_request_fail_out_of_order) { secure_request_unlock(); EXPECT_TRUE(secure_is_unlocking()); TapKeys(key_a, key_d, key_b, key_c); + EXPECT_FALSE(secure_is_unlocking()); EXPECT_FALSE(secure_is_unlocked()); testing::Mock::VerifyAndClearExpectations(&driver); From c91a0f32cf0c2ca37c330d3fa7d1f0fce2738124 Mon Sep 17 00:00:00 2001 From: Drashna Jaelre Date: Thu, 28 Apr 2022 20:01:01 -0700 Subject: [PATCH 14/21] Apply suggestions from code review Co-authored-by: Nick Brassel --- tests/secure/test_secure.cpp | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/tests/secure/test_secure.cpp b/tests/secure/test_secure.cpp index 596e9bc2a9ba..8d43f7998b70 100644 --- a/tests/secure/test_secure.cpp +++ b/tests/secure/test_secure.cpp @@ -44,29 +44,19 @@ class Secure : public TestFixture { } }; -TEST_F(Secure, test_unlock) { - TestDriver driver; - - // Allow any number of empty reports. - EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0); - - secure_unlock(); - EXPECT_TRUE(secure_is_unlocked()); - - testing::Mock::VerifyAndClearExpectations(&driver); -} - 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_TRUE(secure_is_locked()); + EXPECT_FALSE(secure_is_unlocked()); testing::Mock::VerifyAndClearExpectations(&driver); } @@ -77,9 +67,11 @@ TEST_F(Secure, test_unlock_timeout) { // 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_UNLOCK_TIMEOUT+1); - EXPECT_FALSE(secure_is_locked()); + EXPECT_FALSE(secure_is_unlocked()); testing::Mock::VerifyAndClearExpectations(&driver); } @@ -97,6 +89,7 @@ TEST_F(Secure, test_unlock_request) { // 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); @@ -124,6 +117,7 @@ TEST_F(Secure, test_unlock_request_fail) { 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); @@ -138,6 +132,7 @@ TEST_F(Secure, test_unlock_request_timeout) { // 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_IDLE_TIMEOUT+1); @@ -165,6 +160,7 @@ TEST_F(Secure, test_unlock_request_fail_mid) { 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); @@ -191,9 +187,11 @@ TEST_F(Secure, test_unlock_request_fail_out_of_order) { 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()); @@ -213,6 +211,7 @@ TEST_F(Secure, test_unlock_request_on_layer) { // 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(); @@ -239,6 +238,7 @@ TEST_F(Secure, test_unlock_request_mid_stroke) { // 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(); @@ -264,6 +264,7 @@ TEST_F(Secure, test_unlock_request_mods) { // 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(); From b03eecc2b2964800843e39cb69d6dee0c2102c32 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Thu, 28 Apr 2022 20:24:52 -0700 Subject: [PATCH 15/21] Fix timout checks --- tests/secure/test_secure.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/secure/test_secure.cpp b/tests/secure/test_secure.cpp index 8d43f7998b70..87055ebb7fd6 100644 --- a/tests/secure/test_secure.cpp +++ b/tests/secure/test_secure.cpp @@ -70,7 +70,7 @@ TEST_F(Secure, test_unlock_timeout) { EXPECT_FALSE(secure_is_unlocked()); secure_unlock(); EXPECT_TRUE(secure_is_unlocked()); - idle_for(SECURE_UNLOCK_TIMEOUT+1); + idle_for(SECURE_IDLE_TIMEOUT+1); EXPECT_FALSE(secure_is_unlocked()); testing::Mock::VerifyAndClearExpectations(&driver); @@ -135,7 +135,7 @@ TEST_F(Secure, test_unlock_request_timeout) { EXPECT_FALSE(secure_is_unlocked()); secure_request_unlock(); EXPECT_TRUE(secure_is_unlocking()); - idle_for(SECURE_IDLE_TIMEOUT+1); + idle_for(SECURE_UNLOCK_TIMEOUT+1); EXPECT_FALSE(secure_is_unlocking()); EXPECT_FALSE(secure_is_unlocked()); From ea31998d47874b93f32bf4cb7701ca7e3f144ac5 Mon Sep 17 00:00:00 2001 From: Drashna Jaelre Date: Thu, 28 Apr 2022 22:05:38 -0700 Subject: [PATCH 16/21] Sure, why not. Co-authored-by: Nick Brassel --- tests/secure/config.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/secure/config.h b/tests/secure/config.h index 64c389f85e0e..3cfbc6cb14f4 100644 --- a/tests/secure/config.h +++ b/tests/secure/config.h @@ -18,12 +18,15 @@ #include "test_common.h" -#define SECURE_UNLOCK_SEQUENCE \ - { \ - {0, 1}, {0, 2}, {0, 3}, { \ - 0, 4 \ - } \ +// 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 From 671b3f93b8d3cb92e14dcfa93416b7a4436f5ab9 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Sat, 7 May 2022 18:55:56 -0700 Subject: [PATCH 17/21] Remove preprocessor from secure.c --- quantum/secure.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/quantum/secure.c b/quantum/secure.c index 1ac8a9880414..7afa493dd59b 100644 --- a/quantum/secure.c +++ b/quantum/secure.c @@ -89,7 +89,6 @@ void secure_task(void) { #endif } -#if defined(SECURE_ENABLE) __attribute__((weak)) bool secure_hook_user(secure_status_t secure_status) { return true; } @@ -100,4 +99,3 @@ __attribute__((weak)) bool secure_hook_kb(secure_status_t secure_status) { __attribute__((weak)) void secure_hook_quantum(secure_status_t secure_status) { secure_hook_kb(secure_status); } -#endif From ff4e05d7271459ee4a7755781a6d67ec4c73d332 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Sat, 7 May 2022 19:16:38 -0700 Subject: [PATCH 18/21] Fix hooks --- quantum/quantum.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/quantum/quantum.c b/quantum/quantum.c index a1ea0f68837c..957ca23e91d8 100644 --- a/quantum/quantum.c +++ b/quantum/quantum.c @@ -214,13 +214,6 @@ bool process_record_quantum(keyrecord_t *record) { #if defined(SECURE_ENABLE) if (!preprocess_secure(keycode, record)) { - // 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 (!record->event.pressed) { - clear_keyboard(); - layer_clear(); - } return false; } #endif @@ -560,3 +553,18 @@ 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(); + } + + secure_hook_kb(secure_status); +} +#endif From 68c04ee9e3bf5fbeaaf81d9a5741b62f986d2276 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Sat, 7 May 2022 19:20:41 -0700 Subject: [PATCH 19/21] Add sequre request keycode --- quantum/process_keycode/process_secure.c | 4 ++++ quantum/quantum_keycodes.h | 1 + 2 files changed, 5 insertions(+) diff --git a/quantum/process_keycode/process_secure.c b/quantum/process_keycode/process_secure.c index 41c764620767..3224104c99bd 100644 --- a/quantum/process_keycode/process_secure.c +++ b/quantum/process_keycode/process_secure.c @@ -35,6 +35,10 @@ 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; diff --git a/quantum/quantum_keycodes.h b/quantum/quantum_keycodes.h index c7b4ea593feb..3da5f69936ed 100644 --- a/quantum/quantum_keycodes.h +++ b/quantum/quantum_keycodes.h @@ -600,6 +600,7 @@ enum quantum_keycodes { SECURE_LOCK, SECURE_UNLOCK, SECURE_TOGGLE, + SECURE_REQUEST, // Start of custom keycode range for keyboards and keymaps - always leave at the end SAFE_RANGE From aa26b27b525ba254ca13aaf5aba6981ceb422425 Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Sat, 7 May 2022 19:27:47 -0700 Subject: [PATCH 20/21] rework hooks --- quantum/quantum.c | 2 -- quantum/secure.c | 15 ++++++++------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/quantum/quantum.c b/quantum/quantum.c index 957ca23e91d8..fb97457307a0 100644 --- a/quantum/quantum.c +++ b/quantum/quantum.c @@ -564,7 +564,5 @@ void secure_hook_quantum(secure_status_t secure_status) { clear_keyboard(); layer_clear(); } - - secure_hook_kb(secure_status); } #endif diff --git a/quantum/secure.c b/quantum/secure.c index 7afa493dd59b..f07f6af2cb28 100644 --- a/quantum/secure.c +++ b/quantum/secure.c @@ -23,19 +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_quantum(secure_status); + secure_hook(secure_status); } void secure_unlock(void) { secure_status = SECURE_UNLOCKED; idle_time = timer_read32(); - secure_hook_quantum(secure_status); + secure_hook(secure_status); } void secure_request_unlock(void) { @@ -43,7 +48,7 @@ void secure_request_unlock(void) { secure_status = SECURE_PENDING; unlock_time = timer_read32(); } - secure_hook_quantum(secure_status); + secure_hook(secure_status); } void secure_activity_event(void) { @@ -95,7 +100,3 @@ __attribute__((weak)) bool secure_hook_user(secure_status_t secure_status) { __attribute__((weak)) bool secure_hook_kb(secure_status_t secure_status) { return secure_hook_user(secure_status); } - -__attribute__((weak)) void secure_hook_quantum(secure_status_t secure_status) { - secure_hook_kb(secure_status); -} From 91a360db15af4b0b0f5f05f96155802dcdf42e0a Mon Sep 17 00:00:00 2001 From: Drashna Jael're Date: Sat, 7 May 2022 19:29:57 -0700 Subject: [PATCH 21/21] Fix lint issues --- quantum/quantum.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/quantum/quantum.c b/quantum/quantum.c index fb97457307a0..8e9e406c7ea5 100644 --- a/quantum/quantum.c +++ b/quantum/quantum.c @@ -561,8 +561,8 @@ void secure_hook_quantum(secure_status_t secure_status) { // clear these, when it is triggered. if (secure_status == SECURE_PENDING) { - clear_keyboard(); - layer_clear(); + clear_keyboard(); + layer_clear(); } } #endif