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.

Sign up for GitHub

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

Jump to bottom

Add securer mode to totp_face_lfs (secrets only in memory) #421

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add securer mode to totp_face_lfs (secrets only in memory) #421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 55 additions & 5 deletions movement/watch_faces/complication/totp_face_lfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@

const char* TOTP_URI_START = "otpauth://totp/";

/* The totp_record struct has two modes of operation:
* - secmod = false: read from lfs (in which file_secret_offset
* points to an offset in totp_uris.txt)
* - secmod = true: read from memory
* (in which case secret contains the decoded secret)
*/
struct totp_record {
char label[2];
hmac_alg algorithm;
Expand Down Expand Up @@ -232,7 +238,7 @@ static void totp_face_set_record(totp_lfs_state_t *totp_state, int i) {
record = &totp_records[i];

TOTP(
totp_face_lfs_get_file_secret(record),
(totp_state->secmod ? record->secret : totp_face_lfs_get_file_secret(record)),
record->secret_size,
record->period,
record->algorithm
Expand All @@ -254,6 +260,7 @@ void totp_face_lfs_activate(movement_settings_t *settings, void *context) {
}
#endif

totp_state->secmod_request = false;
totp_state->timestamp = watch_utility_date_time_to_unix_time(watch_rtc_get_date_time(), movement_timezone_offsets[settings->bit.time_zone] * 60);
totp_face_set_record(totp_state, 0);
}
Expand All @@ -279,15 +286,37 @@ static void totp_face_display(totp_lfs_state_t *totp_state) {
watch_display_string(buf, 0);
}

static void totp_face_lfs_switch_to_secmod(totp_lfs_state_t *totp_state) {
int i;

for (i = 0; i < num_totp_records; ++i) {
totp_face_lfs_get_file_secret(&totp_records[i]);
totp_records[i].secret = malloc(totp_records[i].secret_size);
if (totp_records[i].secret == NULL) {
num_totp_records = i - 1;
// We can't easily undo at this point, because we destroyed
// the offset info (secret is in a union). So let's just junk
// the remaining records.
break;
}
memcpy(totp_records[i].secret, current_secret, totp_records[i].secret_size);
}
Comment on lines +291 to +303
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this impact memory usage? Copying every secret into memory has caused resets in the past, and the fix was to read them into memory one at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - there is a warning about this in the doco (i.e. that using secure mode is more likely to make you run out of memory, and whether it works well is a bit config dependent).

Personally, I've run 10 codes on my device without issue, but it obviously depends what other faces are loaded.


filesystem_rm(TOTP_FILE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unlikely to be a secure erase operation. For performance reasons, file systems typically don't overwrite file contents when erasing, they just remove the links to the file from the file system data structures, causing the file to become unreachable and the memory to become available for new files. The data will not be overwritten until new data is written to the same physical location.

For this reason, it may be wise to overwrite the entire file with zeros before issuing the rm command to the file system.

This is adequate for spinning disks but may not be completely effective for flash storage: copies of the file may still linger on the storage medium if wear leveling is employed.

I do not know if the hardware supports a secure erase operation but it's probably a good idea to also issue those commands if it does. Even if it does, it is unknown how effective it is, hardware manufacturers are known for screwing up features like this. The data might therefore still be recoverable despite all possible precautions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Yes, as I mentioned at the top, don't know enough about LFS to understand exactly what will happen here. I wouldn't feel confident in zero-ing it out as a method because I haven't looked at LFS's internal structure (I mean, if it was the same length... but as you mention, LFS could have some built-in wear levelling that means it will write it in a separate block). At least there's no hardware level magic here we have to worry about!

I'll change the top of the PR to make these caveats clearer.

totp_state->secmod = true;
}

bool totp_face_lfs_loop(movement_event_t event, movement_settings_t *settings, void *context) {
(void) settings;

totp_lfs_state_t *totp_state = (totp_lfs_state_t *)context;

switch (event.event_type) {
case EVENT_TICK:
totp_state->timestamp++;
totp_face_display(totp_state);
if (!totp_state->secmod_request) {
totp_state->timestamp++;
totp_face_display(totp_state);
}
break;
case EVENT_ACTIVATE:
totp_face_display(totp_state);
Expand All @@ -296,19 +325,40 @@ bool totp_face_lfs_loop(movement_event_t event, movement_settings_t *settings, v
movement_move_to_face(0);
break;
case EVENT_ALARM_BUTTON_UP:
totp_state->secmod_request = false;
totp_face_set_record(totp_state, (totp_state->current_index + 1) % num_totp_records);
totp_face_display(totp_state);
break;
case EVENT_LIGHT_BUTTON_UP:
totp_state->secmod_request = false;
totp_face_set_record(totp_state, (totp_state->current_index + num_totp_records - 1) % num_totp_records);
totp_face_display(totp_state);
break;
case EVENT_ALARM_BUTTON_DOWN:
case EVENT_ALARM_LONG_PRESS:
if (totp_state->secmod || num_totp_records < 1) {
break;
}

/* now long press 'Light' if you want secmod */
totp_state->secmod_request = true;
watch_display_string("LIIFSecmod", 0);
break;
case EVENT_ALARM_BUTTON_DOWN:
case EVENT_LIGHT_BUTTON_DOWN:
break;
case EVENT_LIGHT_LONG_PRESS:
movement_illuminate_led();
if (totp_state->secmod_request) {
totp_face_lfs_switch_to_secmod(totp_state);
totp_face_set_record(totp_state, 0);
totp_face_display(totp_state);
} else {
movement_illuminate_led();
}
totp_state->secmod_request = false;
break;
case EVENT_MODE_BUTTON_UP:
totp_state->secmod_request = false;
movement_default_loop_handler(event, settings);
break;
default:
movement_default_loop_handler(event, settings);
Expand Down
12 changes: 12 additions & 0 deletions movement/watch_faces/complication/totp_face_lfs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@
* If you have more than one secret key, press ALARM to cycle through them.
* Press LIGHT to cycle in the other direction or keep it pressed longer to
* activate the light.
*
* Finally, if you're especially paranoid, doing a long-hold of ALARM will bring
* up the option to move to 'secure mode' (LI IF SECMOD). To enable secure
* mode, do a long press of the LIGHT button. Secure mode removes totp_uris.txt
* from the filesystem, which means the codes will disappear when your
* watch loses power (i.e. when the battery runs out or you next disassemble it).
* HOWEVER, it will also use more memory. If you have a large number of TOTP
* codes, or many other faces loaded which allocate memory, this will drop any
* codes it can't allocate space for, and potentially cause other failures
* if other code requires memory.
*/

#include "movement.h"
Expand All @@ -59,6 +69,8 @@ typedef struct {
uint8_t steps;
uint32_t current_code;
uint8_t current_index;
bool secmod;
bool secmod_request;
} totp_lfs_state_t;

void totp_face_lfs_setup(movement_settings_t *settings, uint8_t watch_face_index, void ** context_ptr);
Expand Down
Loading