Make the TOTP face use the filesystem for secret storage #95

wryun · 2022-10-13T00:35:28Z

Messed around with using LFS more nicely, then decided to stop getting distracted since the memory usage from reading the entire file in was acceptable on startup.

Only limited testing on simulator so far, pending feedback on approach. Don't merge as is.

Why this is good:

don't have to bake TOTP keys into code
understands normal totp format
understands files exported from Aegis

Why this is bad:

should we be encrypting? Probably no point (unless one can't easily extract the firmware? i.e. that it's a lot easier to pull this file that the code?)
'parsing' code is a bit sketchy
not clear whether we want the watch to be doing so much work (could have a script to 'upload' the file into a form that's closer or identical to the in-memory representation)
with base32.c, there's a bunch more code (could easily cut the encode function from the base32 library, I guess)

No encryption. Unclear if this is useful.

movement/watch_faces/complication/totp_face.c

wryun · 2022-10-13T00:44:21Z

movement/watch_faces/complication/totp_face.c

            }
-            TOTP(keys + totp_state->current_key_offset, key_sizes[totp_state->current_index], timesteps[totp_state->current_index]);
+            totp_state->current_index = (index + 1) % num_totp_records;
+            TOTP(totp_records[totp_state->current_index].secret, totp_records[totp_state->current_index].secret_size, totp_records[totp_state->current_index].period);


TODO: line looks a bit long. Probably should just set 'index' as above. Hmm, and feels like I should reset steps to force getCodeFromTimestamp to happen again.

rules.mk

joeycastillo · 2022-10-15T14:21:23Z

Thanks for this! It's a good start. I'm going to get in there and make some changes, though.

My main issues center around the extra malloc's and free's that are happening here. My vision for Movement is that watch faces generally shouldn't call malloc except the once, in setup, to claim space for their watch face state. At this time all watch faces have one call to malloc, and no call to free; this effectively means all memory is statically allocated at boot and sticks around in exactly the same place until the watch battery dies.

I think there is a refactor that will allow us to do that here in the TOTP face, but I'd prefer to hack on it myself (which will also give me a chance to review the PR in depth).

wryun · 2022-10-16T04:33:55Z

Thanks! I'll be interested to see how you get around not using malloc. The only thing I can really think of is to read the file into a static buffer and then just point to the secrets in that buffer, but that'll end up wasting quite a bit of space after the init (vs malloc/freeing just in the init)...

EDIT: or I guess reading line at a time into a static buffer, then over-catering the secret inside the struct (usually secrets are 10 bytes, but I don't believe there's an upper limit; I think the longest I have is 42 bytes).

wryun · 2022-10-19T03:25:57Z

When I finally tried to flash this to my watch, there were a couple of issues:

the serial connection was a bit problematic because of the way 'read' is working. Using picocom gave me character at a time into 'read' (i.e. couldn't execute any commands, since there's essentially no line buffering); the Arduino IDE's serial monitor allowed me to send almost a line in a quick chunk, but over 50 bytes got chopped off in the middle. i.e. it's pretty hard to get the secrets across.
when you get the secrets across, you get a different sequence to the emulator. I've tried to be pretty careful about making sure the time was set properly, yet I persistently get the wrong numbers. I was getting the correct numbers before this PR though when I hardcoded my secrets. Unfortunately, this is hard to debug, because 'cat' doesn't work either across the serial connection for larger files (you just get the first 50 or so bytes). EDIT: fixed. This was due to the base32 code I found on the internet assuming chars were signed (i.e. worked on x86 in the simulator/emulator, failed on the watch).

Random base32 code copied off internet assumed chars were signed. Not on ARM, folks. Also improved error output (\n).

joeycastillo · 2022-10-23T21:26:25Z

OKAY! I've made the changes we discussed: static allocation of all the secrets (it uses up a little over a kilobyte of RAM, which could worry me but not right now) and it reads the file line by line into a fixed-length buffer (max line length is 255). I also renamed the watch face to totp_face_lfs; it occurs to me that some folks will still want to do it the old way, compiling their secrets into the firmware, so we should keep both watch faces available.

Feel free to play around with this and let me know if you see any issues; when you're ready, mark as ready for review and I can merge it in :)

tahnok · 2022-10-24T00:57:09Z

movement/filesystem.c

+        err = lfs_file_read(&lfs, &file, buf, min(length, file_size - offset));
+        if (err < 0) return false;
+        for(int i = 0; i < length; i++) {
+            if (buf[i] == '\n') {
+                buf[i] = 0;
+                break;
+            }
+        }


I feel like there's a subtle bug here where you don't get null terminated string if you read length and don't hit a new line or if your file is less than length and it doesn't end in a newline

Good call; it works as long as you have a buffer one byte longer than the maximum length you're requesting, but it's probably safer to just max out at length - 1.

wryun

Looks good.

Some thoughts; haven't run the code yet. Happy to make suggested changes if they seem ok.

movement/filesystem.c

wryun · 2022-10-26T01:59:38Z

movement/filesystem.c

+        if (err < 0) return false;
+        err = lfs_file_seek(&lfs, &file, offset, LFS_SEEK_SET);
+        if (err < 0) return false;
+        err = lfs_file_read(&lfs, &file, buf, min(length, file_size - offset));


To fix the issue tahnok mentioned, one approach would be to read length - 1 here...

Yep, this seems like the move :)

Instead of reading to length - 1, I clarified the documentation to state that you should provide a buffer of length + 1, and that the last character will be a null terminator; this is how we were currently using the function anyway, and it feels more obvious than reading one character less than we were asked for.

wryun · 2022-10-26T02:03:11Z

movement/filesystem.c

+        if (err < 0) return false;
+        err = lfs_file_read(&lfs, &file, buf, min(length, file_size - offset));
+        if (err < 0) return false;
+        for(int i = 0; i < length; i++) {


Or:

char *end_of_line = strchr(buf, '\n'); if (end_of_line) *end_of_line = '\0';

I think. But if the function signature is changed to return the offset, then I guess there's more pay-off in doing our own loop.

wryun · 2022-10-26T02:05:24Z

movement/watch_faces/complication/totp_face_lfs.c

+        totp_record->label[0] = value[0];
+        totp_record->label[1] = value[1];
+    } else if (!strcmp(param, "secret")) {
+        totp_record->secret_size = base32_decode((unsigned char *)value, totp_record->secret);


I feel like checking BASE32_LEN is the responsible thing to do here... (i.e. make sure it's <= 48, now that the max secret size is fixed)

This is also a good call, I can make this change.

movement/watch_faces/complication/totp_face_lfs.c

wryun · 2022-10-26T02:14:54Z

Re keeping the old TOTP face, is there a reason to prefer that approach aside from a bit of 'security through obscurity' for the secrets?

joeycastillo · 2022-10-26T02:44:46Z

Re keeping the old TOTP face, is there a reason to prefer that approach aside from a bit of 'security through obscurity' for the secrets?

The main reason is that there are already blog posts out there documenting the current workflow, and I am hesitant to break things for folks who are already using the existing functionality as provided at launch. I think both faces present valid options for folks who want to use this function, and there's no harm in including both; watch faces don't take up any flash or RAM unless included in the list.

wryun · 2022-10-27T01:57:06Z

I've made most of the changes discussed except the filesystem API one. Haven't resolved in case you want to go through the comments.

wryun · 2022-10-27T02:02:47Z

Also, totp_face_lfs doesn't follow the normal naming convention (totp_lfs_face?). Not sure if that's intentional or not.

joeycastillo · 2022-10-31T06:50:58Z

Okay! I made the changes we discussed; take a look and if you think I haven't mangled it up too badly, I can merge it in :)

Upon further reflection, I think you were correct to use malloc instead of a fixed maximum secret size, so I restored that logic. I had gotten spooked because the call was happening in activate instead of setup, but since it checks against the static number of codes, it is effectively only called once. Just to be sure, I used a pair of #ifdefs so that the file is normally read in setup, and only read in activate on the simulator; this ensures that the mallocs in that function will only get called once at boot.

Also, totp_face_lfs doesn't follow the normal naming convention (totp_lfs_face?). Not sure if that's intentional or not.

This doesn't bother me; it's a variant of a face, I could see e.g. temperature_face_thermistor and temperature_face_i2c; as long as the meaning is clear and nothing in the header steps on another watch face's namespace, it should be fine.

wryun

Thanks - LGTM aside from my mistake with BASE32_LEN vs UNBASE32_LEN.

wryun · 2022-10-31T09:06:23Z

movement/watch_faces/complication/totp_face_lfs.c

+            printf("TOTP secret too long: %s\n", value);
+            return false;
+        }
+        totp_record->secret = malloc(BASE32_LEN(strlen(value)));


Sorry, my mistake earlier - this should be UNBASE32_LEN (as above).

bademux · 2022-10-31T18:02:34Z

Hello, is it ready for testing?
Will get grateful for quick manual how to configure it with FS

wryun · 2022-11-01T04:26:30Z

@bademux unfortunately, due to issues with the serial interface to the Sensor Watch, your best bet is to test it in the simulator (I do have it on my watch, but it wasn't fun getting to that point; the easiest thing to do is probably hack a watch face into the firmware to write the necessary files, then remove that face!). There are instructions at the top of the the face for how to use it:

Sensor-Watch/movement/watch_faces/complication/totp_face_lfs.c

Line 22 in 9ad4184

    
            * At the moment, to get the records onto the filesystem, start a serial connection and do:

wryun · 2022-11-01T04:29:51Z

(I believe it's basically ready to merge, aside from the issue with BASE32_LEN - which just causes it to use a bit more space than it should)

joeycastillo

Right on, I think we're there! Thanks for all your hard work on this one @wryun :)

bademux · 2022-11-01T09:31:56Z

@wryun Is the serial console something that can be temporary hackfixed from client side (some app that will send totp uri in a special way),
or it have to be fixed in the firmware @joeycastillo.
Thanks for the great software and hardware!

joeycastillo · 2022-11-01T13:40:01Z

It’s something that will need a firmware fix, @wryun if you can characterize the issue with some specifics, can you open an issue? I know it has to do with my USB code, and might require someone with a better knowledge of tinyUSB to dig into it (or I’ll take a look eventually)

wryun · 2022-11-01T18:57:18Z

Thanks @joeycastillo ! I've raised #117

As mentioned there, there's a relatively straightforward issue in movement.c, but I suspect there are also deeper problems with how TinyUSB works. But I may be misremembering; I think I got dissuaded from doing the simple fix after hitting other issues.

wryun added 2 commits October 13, 2022 10:58

Initial draft of TOTP using filesystem

f198e1e

No encryption. Unclear if this is useful.

Minor fixes, and added support for appending to filesystem.c

e980455

wryun commented Oct 13, 2022

View reviewed changes

movement/watch_faces/complication/totp_face.c Outdated Show resolved Hide resolved

wryun commented Oct 13, 2022

View reviewed changes

movement/watch_faces/complication/totp_face.c Outdated Show resolved Hide resolved

wryun commented Oct 13, 2022

View reviewed changes

rules.mk Outdated Show resolved Hide resolved

Minor cleanups

b7455c0

wryun marked this pull request as draft October 19, 2022 03:50

wryun and others added 6 commits October 19, 2022 20:35

Fix signedness issue on ARM

491cf97

Random base32 code copied off internet assumed chars were signed. Not on ARM, folks. Also improved error output (\n).

add function for reading file one line at a time

cdcf8ec

TOTP: make secrets statically allocated

3f21e06

TOTP: read secrets file line by line

df0a68c

TOTP: rename to reflect that this version uses the filesystem

95e12a6

TOTP: let's keep both versions of this watch face

e0cd246

tahnok reviewed Oct 24, 2022

View reviewed changes

wryun commented Oct 26, 2022

View reviewed changes

wryun marked this pull request as ready for review October 27, 2022 01:57

Minor fix-ups and refactored so it display immediate on TOTP change

afc8989

wryun force-pushed the totp-use-lfs branch from ccd6bd6 to afc8989 Compare October 27, 2022 02:02

joeycastillo added 3 commits October 31, 2022 01:17

make offset an in/out parameter

3371702

filesystem_read_line: clarify requirements for buffer length

aa47c73

add clearer message for 2FA error condition

8c84ff9

only read file in activate when running in emulator; otherwise use setup

9ad4184

wryun commented Oct 31, 2022

View reviewed changes

fix length of memory to allocate for secret

3596eac

joeycastillo approved these changes Nov 1, 2022

View reviewed changes

joeycastillo merged commit b7a461d into joeycastillo:main Nov 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the TOTP face use the filesystem for secret storage #95

Make the TOTP face use the filesystem for secret storage #95

wryun commented Oct 13, 2022

wryun Oct 13, 2022

joeycastillo commented Oct 15, 2022

wryun commented Oct 16, 2022 •

edited

Loading

wryun commented Oct 19, 2022 •

edited

Loading

joeycastillo commented Oct 23, 2022

tahnok Oct 24, 2022

joeycastillo Oct 26, 2022

wryun left a comment

wryun Oct 26, 2022

joeycastillo Oct 26, 2022

joeycastillo Oct 31, 2022

wryun Oct 26, 2022

wryun Oct 26, 2022

joeycastillo Oct 26, 2022

wryun commented Oct 26, 2022

joeycastillo commented Oct 26, 2022 •

edited

Loading

wryun commented Oct 27, 2022

wryun commented Oct 27, 2022

joeycastillo commented Oct 31, 2022

wryun left a comment

wryun Oct 31, 2022

bademux commented Oct 31, 2022

wryun commented Nov 1, 2022

wryun commented Nov 1, 2022

joeycastillo left a comment

bademux commented Nov 1, 2022

joeycastillo commented Nov 1, 2022

wryun commented Nov 1, 2022

Make the TOTP face use the filesystem for secret storage #95

Make the TOTP face use the filesystem for secret storage #95

Conversation

wryun commented Oct 13, 2022

Choose a reason for hiding this comment

joeycastillo commented Oct 15, 2022

wryun commented Oct 16, 2022 • edited Loading

wryun commented Oct 19, 2022 • edited Loading

joeycastillo commented Oct 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wryun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wryun commented Oct 26, 2022

joeycastillo commented Oct 26, 2022 • edited Loading

wryun commented Oct 27, 2022

wryun commented Oct 27, 2022

joeycastillo commented Oct 31, 2022

wryun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bademux commented Oct 31, 2022

wryun commented Nov 1, 2022

wryun commented Nov 1, 2022

joeycastillo left a comment

Choose a reason for hiding this comment

bademux commented Nov 1, 2022

joeycastillo commented Nov 1, 2022

wryun commented Nov 1, 2022

wryun commented Oct 16, 2022 •

edited

Loading

wryun commented Oct 19, 2022 •

edited

Loading

joeycastillo commented Oct 26, 2022 •

edited

Loading