Merge pull request #397 from madhogs/wyrun-totp-lfs-fix

Reduce totp_face_lfs memory usage

movement/lib/TOTP/TOTP.h

@@ -4,7 +4,7 @@
 #include <inttypes.h>
 #include "time.h"
 
-typedef enum {
+typedef enum __attribute__ ((__packed__)) {
     SHA1,
     SHA224,
     SHA256,

movement/watch_faces/complication/totp_face_lfs.c

@@ -35,29 +35,41 @@
 
 #include "totp_face_lfs.h"
 
-#define MAX_TOTP_RECORDS 20
-#define MAX_TOTP_SECRET_SIZE 48
+#define MAX_TOTP_RECORDS 30
+#define MAX_TOTP_SECRET_SIZE 128
 #define TOTP_FILE "totp_uris.txt"
 
 const char* TOTP_URI_START = "otpauth://totp/";
 
 struct totp_record {
-    uint8_t *secret;
-    size_t secret_size;
     char label[2];
-    uint32_t period;
     hmac_alg algorithm;
+    uint8_t period;
+    uint8_t secret_size;
+
+    union {
+        uint8_t *secret;
+        struct {
+            uint16_t file_secret_offset;
+            uint16_t file_secret_length;
+        };
+    };
 };
 
+/* This is used if we're not storing all the secrets but instead
+ * calculating them on demand. Avoids malloc in normal operation.
+ */
+static uint8_t current_secret[MAX_TOTP_SECRET_SIZE];
+
 static struct totp_record totp_records[MAX_TOTP_RECORDS];
-static int num_totp_records = 0;
+static uint8_t num_totp_records = 0;
 
 static void init_totp_record(struct totp_record *totp_record) {
-    totp_record->secret_size = 0;
     totp_record->label[0] = 'A';
     totp_record->label[1] = 'A';
-    totp_record->period = 30;
     totp_record->algorithm = SHA1;
+    totp_record->period = 30;
+    totp_record->secret_size = 0;
 }
 
 static bool totp_face_lfs_read_param(struct totp_record *totp_record, char *param, char *value) {
@@ -69,14 +81,13 @@ static bool totp_face_lfs_read_param(struct totp_record *totp_record, char *para
         totp_record->label[0] = value[0];
         totp_record->label[1] = value[1];
     } else if (!strcmp(param, "secret")) {
-        if (UNBASE32_LEN(strlen(value)) > MAX_TOTP_SECRET_SIZE) {
+        totp_record->file_secret_length = strlen(value);
+        if (UNBASE32_LEN(totp_record->file_secret_length) > MAX_TOTP_SECRET_SIZE) {
             printf("TOTP secret too long: %s\n", value);
             return false;
         }
-        totp_record->secret = malloc(UNBASE32_LEN(strlen(value)));
-        totp_record->secret_size = base32_decode((unsigned char *)value, totp_record->secret);
+        totp_record->secret_size = base32_decode((unsigned char *)value, current_secret);
         if (totp_record->secret_size == 0) {
-            free(totp_record->secret);
             printf("TOTP can't decode secret: %s\n", value);
             return false;
         }
@@ -126,8 +137,8 @@ static void totp_face_lfs_read_file(char *filename) {
     }
 
     char line[256];
-    int32_t offset = 0;
-    while (filesystem_read_line(filename, line, &offset, 255) && strlen(line)) {
+    int32_t offset = 0, old_offset = 0;
+    while (old_offset = offset, filesystem_read_line(filename, line, &offset, 255) && strlen(line)) {
         if (num_totp_records == MAX_TOTP_RECORDS) {
             printf("TOTP max records: %d\n", MAX_TOTP_RECORDS);
             break;
@@ -155,7 +166,13 @@ static void totp_face_lfs_read_file(char *filename) {
         do {
             char *param_middle = strchr(param, '=');
             *param_middle = '\0';
-            error = error || !totp_face_lfs_read_param(&totp_records[num_totp_records], param, param_middle + 1);
+            if (totp_face_lfs_read_param(&totp_records[num_totp_records], param, param_middle + 1)) {
+                if (!strcmp(param, "secret")) {
+                    totp_records[num_totp_records].file_secret_offset = old_offset + (param_middle + 1 - line);
+                }
+            } else {
+                error = true;
+            }
         } while ((param = strtok_r(NULL, "&", &param_saveptr)));
 
         if (error) {
@@ -186,15 +203,42 @@ void totp_face_lfs_setup(movement_settings_t *settings, uint8_t watch_face_index
 #endif
 }
 
+static uint8_t *totp_face_lfs_get_file_secret(struct totp_record *record) {
+    char buffer[BASE32_LEN(MAX_TOTP_SECRET_SIZE) + 1];
+    int32_t file_secret_offset = record->file_secret_offset;
+
+    /* TODO filesystem_read_line is quite inefficient. Consider writing a new function,
+     * and keeping the file open?
+     */
+    if (!filesystem_read_line(TOTP_FILE, buffer, &file_secret_offset, record->file_secret_length + 1)) {
+        /* Shouldn't happen at this point. Return current_secret, which is misleading but will not cause a crash. */
+        printf("TOTP can't read expected secret from totp_uris.txt (failed readline)\n");
+        return current_secret;
+    }
+    if (base32_decode((unsigned char *)buffer, current_secret) != record->secret_size) {
+        printf("TOTP can't properly decode secret from totp_uris.txt; failed at offset %d; read to %d\n", buffer, record->file_secret_offset, file_secret_offset);
+    }
+    return current_secret;
+}
+
 static void totp_face_set_record(totp_lfs_state_t *totp_state, int i) {
+    struct totp_record *record;
+
     if (num_totp_records == 0 && i >= num_totp_records) {
         return;
     }
 
     totp_state->current_index = i;
-    TOTP(totp_records[i].secret, totp_records[i].secret_size, totp_records[i].period, totp_records[i].algorithm);
+    record = &totp_records[i];
+
+    TOTP(
+        totp_face_lfs_get_file_secret(record),
+        record->secret_size,
+        record->period,
+        record->algorithm
+    );
     totp_state->current_code = getCodeFromTimestamp(totp_state->timestamp);
-    totp_state->steps = totp_state->timestamp / totp_records[i].period;
+    totp_state->steps = totp_state->timestamp / record->period;
 }
 
 void totp_face_lfs_activate(movement_settings_t *settings, void *context) {
@@ -256,12 +300,7 @@ bool totp_face_lfs_loop(movement_event_t event, movement_settings_t *settings, v
             totp_face_display(totp_state);
             break;
         case EVENT_LIGHT_BUTTON_UP:
-            if (totp_state->current_index - 1 >= 0) {
-                totp_face_set_record(totp_state, totp_state->current_index - 1);
-            } else {
-                // Wrap around to the last record.
-                totp_face_set_record(totp_state, num_totp_records - 1);
-            }
+            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:

movement/watch_faces/complication/totp_face_lfs.h

@@ -35,7 +35,8 @@
  *   otpauth://totp/ACME%20Co:[email protected]?secret=HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ&issuer=ACME%20Co&algorithm=SHA1&digits=6&period=30
  *
  * This is also the same as what Aegis exports in plain-text format.
- * This face performs minimal sanitisation of input, however.
+ * This face performs minimal sanitisation of input, however, and you
+ * will only see errors if you use the simulator or view the serial console.
  *
  * At the moment, to get the records onto the filesystem, start a serial connection and do:
  *   echo otpauth://totp/Example:[email protected]?secret=JBSWY3DPEHPK3PXP&issuer=Example > totp_uris.txt