Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Soft-locking for Trezor T #958

Merged
merged 64 commits into from Jun 4, 2020
Merged

Soft-locking for Trezor T #958

merged 64 commits into from Jun 4, 2020

Conversation

matejcik
Copy link
Contributor

@matejcik matejcik commented Apr 22, 2020

implements #949, also includes messages and some preliminaries for #912

Summary

  • Functions like Initialize, GetFeatures, Ping etc. were moved out of "homescreen app" into apps.base.
  • apps.homescreen was refactored into a separate "lockscreen" (used in boot.py and for softlock) and "homescreen", with common avatar rendering.
  • Default handling in workflow was changed and should be a little easier to understand again. apps.base.set_homescreen makes sure to start the homescreen appropriate for the current situation.
  • the above two points should ensure that locking works just as well with recovery homescreen, but this is only very lightly tested
  • We lock the storage when soft-locked, and use storage lock state to figure out if we should be soft-locked. This actually simplifies the implementation: storage lock is the source of truth for all of the different moving parts.
  • ClearSession was renamed to LockDevice and enables soft-lock on TT. This causes the test suite to fail!

Most of the behavior is implemented through extracting get_workflow_handler out of wire. Registered handlers are looked up through a callback. When the device is soft-locked, this callback will wrap everything other than Initialize and GetFeatures so that it asks for PIN first.

If we wanted, we could also use this mechanism for recovery homescreen (to block all messages unrelated to recovery), or for other homescreens (#114 comes to mind - this paves the road for various unattended modes).
In the future, we might even want to tie together the selected homescreen with the handler lookup.

TBD

  • FIDO2 behavior. Currently it does not recognize softlock at all.
  • Tests. Due to LockDevice having an effect, we will need to modify the test suite. It will involve reviewing all uses of client.clear_session(). Maybe clear_session should not be called by default, too?
  • new tests for the softlock behavior
  • user-interaction locking, if we want it. It's simple in terms of code, I have a working prototype, but just putting a button on the homescreen is ugly. Some UI love would be appreciated.
  • should we allow soft-lock when PIN is not set but SD protect is enabled? As noted in the summary doc, this makes sense conceptually (storage is locked, sessions might be encrypted in the future (Encrypt session cache when device is soft-locked #957)). But it defies user expectations: while the device is displaying "Locked", any message from the host will transparently unlock it with no interaction from the user
    -> soft-lock is only enabled when PIN is set
  • dimming the lock screen, which has proven more complicated than I expected, due to how the layout engine works will be done separately in Dim Trezor T screen while locked #974
  • check that values of Features are returned properly wrt storage lock status. There are differences in what T1 returns that make sense.

@matejcik matejcik added core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature labels Apr 22, 2020
@andrewkozlik
Copy link
Contributor

andrewkozlik commented Apr 22, 2020

user-interaction locking, if we want it. It's simple in terms of code, I have a working prototype, but just putting a button on the homescreen is ugly. Some UI love would be appreciated.

Yeah, I was wondering what the plans are around this, because I couldn't see where to put the button. I had two ideas:

  1. We can do it similar to T1, basically a hidden feature. Holding the finger anywhere on the homescreen for a few seconds brings up a lock device confirmation dialog. Another alternative is a swipe-down action. However, I don't like these sort of hidden features.
  2. Tapping the homescreen brings up a menu with all sorts of options like lock, change PIN, wipe device, i.e. firmware: Simple on-device management #61.

@tsusanka tsusanka added this to the 2020-06 milestone Apr 23, 2020
@matejcik
Copy link
Contributor Author

matejcik commented Apr 27, 2020

here is a full list of Features fields, marked by whether T1 sends them when locked or not.

not sent

These fields are either not sent by T1, or only sent in bootloader:

optional bool bootloader_mode = 5;          // is device in bootloader mode?
optional bool firmware_present = 18;        // is valid firmware loaded?
optional uint32 fw_major = 22;              // reported firmware version if in bootloader mode
optional uint32 fw_minor = 23;              // reported firmware version if in bootloader mode
optional uint32 fw_patch = 24;              // reported firmware version if in bootloader mode
optional string fw_vendor = 25;             // reported firmware vendor if in bootloader mode
optional bytes fw_vendor_keys = 26;         // reported firmware vendor keys (their hash)

only sent when unlocked

optional bool imported = 15;                // was storage imported from an external source?
optional uint32 flags = 20;                 // device flags (equals to Storage.flags)
optional bool needs_backup = 19;            // does storage need backup? (equals to Storage.needs_backup)
optional bool unfinished_backup = 27;       // report unfinished backup (equals to Storage.unfinished_backup)
optional bool no_backup = 28;               // report no backup (equals to Storage.no_backup)
optional bool wipe_code_protection = 34;    // is wipe code protection enabled

needs_backup, no_backup and unfinished_backup is always sent, but the value is always False when the device is locked. This seems to be a bug; the fields should not be sent.

always sent

optional string vendor = 1;                 // name of the manufacturer, e.g. "trezor.io"
optional uint32 major_version = 2;          // major version of the firmware/bootloader, e.g. 1
optional uint32 minor_version = 3;          // minor version of the firmware/bootloader, e.g. 0
optional uint32 patch_version = 4;          // patch version of the firmware/bootloader, e.g. 0
optional string device_id = 6;              // device's unique identifier
optional bool pin_protection = 7;           // is device protected by PIN?
optional bool passphrase_protection = 8;    // is node/mnemonic encrypted using passphrase?
optional string language = 9;               // device language
optional string label = 10;                 // device description label
optional bool initialized = 12;             // does device contain seed?
optional bytes revision = 13;               // SCM revision of firmware
optional bytes bootloader_hash = 14;        // hash of the bootloader
optional bool pin_cached = 16;              // is PIN already cached in session?
optional string model = 21;                 // device hardware model
repeated Capability capabilities = 30;      // list of supported capabilities
optional bytes session_id = 35;

Personally I find it strange that pin_protection and passphrase_protection are always reported, but I guess we will keep it that way for backwards compatibility.


On the TT as currently implemented, the needs_backup, no_backup and unfinished_backup fields are always false when the device is locked -- seemingly by virtue of having the storage locked. flags field is always 0 when device is locked too.

I would propose the following fields to be hidden on the TT when device is locked:

optional bool needs_backup = 19;            // does storage need backup? (equals to Storage.needs_backup)
optional uint32 flags = 20;                 // device flags (equals to Storage.flags)
optional bool unfinished_backup = 27;       // report unfinished backup (equals to Storage.unfinished_backup)
optional bool no_backup = 28;               // report no backup (equals to Storage.no_backup)
optional bool recovery_mode = 29;           // is recovery mode in progress
optional BackupType backup_type = 31;       // type of device backup (BIP-39 / SLIP-39 basic / SLIP-39 advanced)
optional bool sd_protection = 33;           // is SD Protect enabled
optional bool wipe_code_protection = 34;    // is wipe code protection enabled
optional bool passphrase_always_on_device = 36;  // device enforces passphrase entry on Trezor

in addition to T1 fields, sd_card_present should be safe to send while locked.

@matejcik
Copy link
Contributor Author

It turns out that on TT with locked storage, we cannot tell if the device was initialized. So we'll have to add initialized to the set of fields that are not sent when locked.

@matejcik
Copy link
Contributor Author

matejcik commented May 4, 2020

Did a quick review of @andrewkozlik's code. LGTM on the surface, but I don't have a deep enough understanding of the FIDO2 implementation to be the judge of that. Can someone else review that? @onvej-sl perhaps?

@matejcik matejcik marked this pull request as ready for review May 6, 2020 12:05
Copy link
Contributor

@tsusanka tsusanka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 🎉. Few comments, other than that LGTM.

There are few todos, which you are probably aware of (legacy's fsm_msgEndSession, trezorlib.clear_session and wire/__init__.py for example).

And +1 for adding the input_signal in the layout directly via create_tasks instead of providing it in ctx.wait every time. It is way clearer!

core/SConscript.firmware Show resolved Hide resolved
core/src/apps/base.py Show resolved Hide resolved
core/src/apps/common/request_pin.py Outdated Show resolved Hide resolved
core/src/apps/webauthn/fido2.py Show resolved Hide resolved
python/src/trezorlib/debuglink.py Outdated Show resolved Hide resolved
@matejcik matejcik force-pushed the matejcik/softlock branch 2 times, most recently from 404695e to b06b29f Compare May 15, 2020 14:55
core/src/apps/base.py Outdated Show resolved Hide resolved
@matejcik
Copy link
Contributor Author

with an unexpectedly minor changeset, autolock is now implemented

@matejcik
Copy link
Contributor Author

A problem with this approach is that pending actions will never return when the autolock engages. We would need to send an explicit exception to the workflow tasks.

@tsusanka tsusanka modified the milestones: 2020-06, 2020-07 May 21, 2020
@matejcik matejcik force-pushed the matejcik/softlock branch 2 times, most recently from b06b29f to 4548aee Compare May 22, 2020 09:15
@prusnak prusnak removed their request for review May 28, 2020 12:55
so that pytest doesn't think it is a testcase
The original wait_layout was unreliable, because there are no guarantees
re order of arrival of the respective events. Still, TT's event handling
is basically deterministic, so as long as the host sent its messages
close enough to each other, the order worked out.

This is no longer the case with the introduction of loop.spawn: TT's
behavior is still deterministic, but now ButtonAck is processed *before*
the corresponding wait_layout, so the waiting side waits forever.

In the new process, the host must first register to receive layout
events, and then receives all of them (so the number of calls to
wait_layout must match the number of layout changes).

DebugLinkWatchLayout message must be version-gated, because of an
unfortunate collection of bugs in previous versions wrt unknown message
handling; and this interests us because upgrade-tests are using
wait_layout feature.
this makes sense, really: close_others() requests UI exclusivity, and
that is something that generally happens at the same places we emit a
ButtonRequest
This will have unintended consequences if you call a wirelink function
on the debulink interface. TT allows this ... and will behave badly.
This prevents a race condition where sometimes an Initialize message
could arrive before the homescreen was fully booted -- and Recovery
homescreen would cancel it as part of its bootup sequence.
This avoids problems with large timeouts causing the scheduler queue to
think the time counter has overflown, and ordering the autolock task before
immediate tasks.

The maximum reasonable time difference is 0x20000000, which in
microseconds is ~8 minutes, but in milliseconds a more reasonable ~6
days.
@matejcik matejcik merged commit 7579ac5 into master Jun 4, 2020
@matejcik matejcik deleted the matejcik/softlock branch June 4, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants