Repository navigation
fix: require a TC Code to enable TC Flash Hash at Boot - #966
Thanatos9404 wants to merge 2 commits into
Conversation
TC Flash Hash at Boot only runs when a Tamper Check Code is stored, so turning it on without one had no effect and gave a false sense of protection. The setting now refuses True when no TC Code is set, reusing the existing "Set a tamper check code first" warning, and stays False. Turning it off is always allowed, so a value loaded from older settings can still be cleared. Fixes selfcustody#813
|
cc @tadeubas |
|
Host-side Tested ACK on fb40181, compared with 2937a29. Confirmed that enabling without a TC Code is rejected, normal toggling with a code works, and a previously stored True can still be disabled without a code. The regression test fails on the base and passes here. All 34 settings tests and 17 additional edge cases passed, including repeated attempts, touch controls, persistence, cancellation, and enabling after code creation. Full suite: 1,197 passed, 1 expected failure. No blocking findings. No firmware build or physical-device testing; GitHub CI still requires maintainer approval. LGTM form my side. |
| elif setting.attr == "boot_flash_hash": | ||
| # Without a TC Code there is nothing to verify at boot | ||
| if new_category and not self.ctx.tc_code_enabled: | ||
| self.flash_error(t("Set a tamper check code first")) | ||
| Settings().security.boot_flash_hash = False |
There was a problem hiding this comment.
Could you check and verify this finding?
Some Swipes and long-presses leave the value unchanged but still trigger it (:387, :397). If the saved value is True with no TC Code, such a press shows "Set a tamper check code first" and turns the option off, although the user never tried to enable it. The resulting state is the right one; only the message is misleading. Worth to check if is necessary to compare against current_category and/or be left as is.
There was a problem hiding this comment.
Yes, the finding was right, and I fixed it in dcdf382. category_setting now passes new_category != current_category to _category_change_special_cases, so the error and the reset only happen when a press actually changes the value. A swipe or long-press on a stored True with no TC Code is now a no-op, and test_unchanged_press_keeps_stored_boot_flash_hash_quiet covers that on all devices.
| assert mock_file.write_data == b"" | ||
|
|
||
|
|
||
| def test_enable_boot_flash_hash_without_tc_code(m5stickv, mocker): |
There was a problem hiding this comment.
tests done with m5stickv only. IMO is good to test with all devices in current global context.
| A [Tamper Check Code](#tamper-check-code) must be set first. Otherwise, Krux | ||
| shows a warning and keeps this option disabled. | ||
|
|
There was a problem hiding this comment.
| A [Tamper Check Code](#tamper-check-code) must be set first. Otherwise, Krux | |
| shows a warning and keeps this option disabled. | |
| A [Tamper Check Code](#tamper-check-code) must be set first. Otherwise, Krux shows an error and keeps this option disabled. | |
Presses that leave the value unchanged no longer show the TC Code error. Tests now run on every device.
|
Thanks for the review. Pushed dcdf382: the TC Code error now only shows when the value actually changes, so swipes and long-presses on a stored True no longer trigger it. I applied the docs wording as suggested. The boot_flash_hash tests now use the multiple_devices fixture (all 7 devices), plus a new test for the unchanged-press case. |
What is this PR for?
Fixes #813
Settings > Security > TC Flash Hash at Boot could be set to
Trueeven when no Tamper Check Code was stored. At boot,tc_code_verification()returns early when there is no TC Code file, so the option silently did nothing while the menu showed it as enabled.Now, if the user tries to switch it to
Truewithout a TC Code, Krux flashes the existing "Set a tamper check code first" error (the same one Tools > Flash Tools > TC Flash Hash already shows) and the value staysFalse. It usesctx.tc_code_enabled, whichboot.pyandenter_modify_tc_code()already keep up to date.Some details:
Falseis always allowed, so aTruethat was saved before the TC Code went away (for example settings kept on the SD card after erasing user's data) can still be cleared.This only touches the settings page logic and adds one sentence to the settings docs. It does not change the TC Code storage, hashing or the boot flow.
Changes made to:
Did you build the code and tested on device?
Not tested on a device. Tested with the test suite:
uv run pytest --cache-clear tests/pages/test_settings_page.py: 34 passed. The newtest_enable_boot_flash_hash_without_tc_codefails on currentdevelopand passes with this change. The other two new tests (enable with a TC Code, disable a stored value without a TC Code) cover the cases that should keep working.uv run poe test-simple: 1197 passed, 1 xfaileduv run poe lint-src: 10.00/10black --check src tests: unchangeduv run poe i18n validateanduv run poe vulture: OKWhat is the purpose of this pull request?