Skip to content

Commit aaa8bd9

Browse files
committed
test(I6): disabling AdvancedMode must revoke the signer, not suspend it
I6 previously MEASURED and documented a gap instead of closing it: turning AdvancedMode off left the loaded provider in RAM, and the test's own docstring said so -- a user who disables AdvancedMode to revoke a provider has not revoked it, only suspended it. Re-enabling the policy costs one button press whose screen names the policy and never names the signer it silently re-arms. That also contradicted docs/security/clearsign-provider-tier.md, which lists "disabling AdvancedMode" among the events that clear identities. One of the two had to move, and the doc was right: 7.15 is safe without any key-management programme precisely because trust dies on its own, and a revocation that only suspends is not one. The firmware side is four lines in fsm_msgApplyPolicies (firmware PR). This flips the assertion to match: after the policy round-trip the signer must be GONE, and the bare-message expected-response list (one ButtonRequest, one Success) proves trust cannot be restored by a policy toggle at all -- coming back costs a fresh LoadClearsignSigner consent, which is the screen that names the alias and fingerprint. Renamed to say what it now asserts. The atlas entry follows.
1 parent bb1d77a commit aaa8bd9

2 files changed

Lines changed: 40 additions & 44 deletions

File tree

scripts/generate-test-report.py

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2523,25 +2523,25 @@ def _arg_shown(a):
25232523
'Enable Policy: Experimental (marker, four commits)',
25242524
'Enable Policy: AdvancedMode (re-armed after the reboot to isolate the slot)']),
25252525
('I6', 'test_msg_session_trust_lifetime',
2526-
'test_disabling_advanced_mode_makes_signer_inert_not_erased',
2527-
'Disabling AdvancedMode suspends the signer, it does not revoke it',
2528-
'MEASURED, and it contradicts the shorthand that disabling AdvancedMode clears loaded '
2529-
'signers. Turning the policy off does make the signer unusable - every consumer in '
2530-
'signed_metadata.c refuses a runtime slot while the policy is off, so metadata fails closed. '
2531-
'But nothing erases the slot: storage_setPolicy() flips a bit and only session_clear() calls '
2532-
'signed_metadata_clear_signers(). Sending the bare ApplyPolicies to turn the policy back on '
2533-
'brings the old signer straight back to VERIFIED, and the expected-response list asserts '
2534-
'exactly one ButtonRequest for that - the "Trust CI Test ... NOT verified by KeepKey" consent '
2535-
'is provably NOT re-shown. The host API hides this because apply_policy() follows every '
2536-
'policy change with Initialize, and it is the Initialize that clears the slot (I3). Release '
2537-
'consequence: a user who disables AdvancedMode to drop a provider has suspended it, not '
2538-
'revoked it, and the screen that re-arms it names the policy but never the signer it silently '
2539-
'reinstates.',
2526+
'test_disabling_advanced_mode_revokes_the_signer',
2527+
'Disabling AdvancedMode revokes the signer, it does not suspend it',
2528+
'With the policy off, revoking and suspending are indistinguishable: every consumer in '
2529+
'signed_metadata.c refuses a runtime slot while AdvancedMode is off, so metadata fails '
2530+
'closed either way. The difference shows on the way back. Suspending would mean '
2531+
're-enabling the policy silently re-arms a provider the user never re-loaded, on a '
2532+
'confirmation screen that names the policy and never names the signer - so a user who '
2533+
'disabled AdvancedMode to drop a provider would not have dropped it. '
2534+
'fsm_msgApplyPolicies therefore calls signed_metadata_clear_signers() on disable. The '
2535+
're-enable is sent as the bare ApplyPolicies with an exact expected-response list - one '
2536+
'ButtonRequest and a Success - so the absence of a trust screen there is proof, not '
2537+
'observation: trust cannot be restored by a policy toggle at all. Coming back costs a '
2538+
'fresh LoadClearsignSigner consent, the screen that names the alias and fingerprint.',
25402539
['Enable Policy: AdvancedMode',
25412540
"Load Clearsigner: Trust 'CI Test' (fingerprint) ... NOT verified by KeepKey",
25422541
'Disable Policy: AdvancedMode',
25432542
'Home screen at the refusal - the metadata message fails closed with no screen',
2544-
'Enable Policy: AdvancedMode - the ONLY confirm shown on re-arming; no second trust screen']),
2543+
'Enable Policy: AdvancedMode - the only confirm on re-arming, and the signer does NOT '
2544+
'come back with it']),
25452545
]),
25462546
('L', 'Bitcoin-Only Variant', '7.15.0',
25472547
'KK_BITCOIN_ONLY=ON builds a second shipping product out of the same tree: coins.def keeps '

tests/test_msg_session_trust_lifetime.py

Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -433,30 +433,27 @@ def test_signer_dropped_by_power_cycle(self):
433433
CLASSIFICATION_MALFORMED,
434434
"the signer came back after a power cycle — it was written to flash")
435435

436-
def test_disabling_advanced_mode_makes_signer_inert_not_erased(self):
437-
"""MEASURED behaviour, and it is NOT "disabling AdvancedMode clears the
438-
signer".
439-
440-
Turning the policy off does make the signer unusable: every consumer in
441-
signed_metadata.c (signed_metadata_process, _verify_attestation,
442-
_signer_fingerprint) refuses a runtime slot while AdvancedMode is off,
443-
so the metadata message fails closed. But nothing erases the slot —
444-
storage_setPolicy() only flips a policy bit, and only session_clear()
445-
calls signed_metadata_clear_signers(). Turn the policy back on and the
446-
old signer verifies again, with NO second trust screen: the expected
447-
response list below is exactly one ApplyPolicies ButtonRequest and a
448-
Success, so the "Trust 'CI Test' (…) NOT verified by KeepKey" consent is
449-
provably not re-shown.
450-
451-
Why the host path looks otherwise: ProtocolMixin.apply_policy() follows
452-
every policy change with Initialize, and it is that Initialize — not the
453-
policy change — that clears the signer (test_signer_dropped_by_initialize).
454-
A host that sends the bare message gets the behaviour asserted here.
455-
456-
Consequence to weigh at release: a user who disables AdvancedMode to
457-
revoke a provider has not revoked it, only suspended it. Re-enabling
458-
the policy costs one button press whose screen names the policy and
459-
never names the signer it silently re-arms.
436+
def test_disabling_advanced_mode_revokes_the_signer(self):
437+
"""Turning the policy off DROPS the provider, it does not suspend it.
438+
439+
Every consumer in signed_metadata.c already refuses a runtime slot
440+
while AdvancedMode is off, so with the policy off the two behaviours
441+
are indistinguishable — the metadata fails closed either way. The
442+
difference only shows on the way back.
443+
444+
Suspending would mean re-enabling the policy silently re-arms a
445+
provider the user never re-loaded, on a confirmation screen that names
446+
the policy and never names the signer. A user who disabled
447+
AdvancedMode to drop a provider would not have dropped it. So
448+
fsm_msgApplyPolicies calls signed_metadata_clear_signers() on disable,
449+
and coming back costs a fresh LoadClearsignSigner consent — the screen
450+
that names the alias and fingerprint, which is the screen that should
451+
appear whenever trust begins.
452+
453+
The re-enable is sent as the bare message with the exact expected
454+
response list: one ApplyPolicies ButtonRequest and a Success. No trust
455+
screen appears there, which is the point — trust cannot be restored by
456+
a policy toggle at all.
460457
"""
461458
self._arm_session()
462459

@@ -474,11 +471,10 @@ def test_disabling_advanced_mode_makes_signer_inert_not_erased(self):
474471
self._apply_policy_raw("AdvancedMode", True)
475472

476473
self._assertClassification(
477-
CLASSIFICATION_VERIFIED,
478-
"the signer did NOT survive the policy toggle. That is stricter "
479-
"than the code path allows today, so something changed: re-read "
480-
"storage_setPolicy() and signed_metadata_clear_signers() before "
481-
"loosening this assertion")
474+
CLASSIFICATION_MALFORMED,
475+
"the signer survived disabling AdvancedMode — re-enabling the "
476+
"policy re-armed a provider the user never re-loaded, on a screen "
477+
"that never named it")
482478

483479

484480
if __name__ == '__main__':

0 commit comments

Comments
 (0)