otp fixes

Change-Id: Ida18226344c58c83222721cedf26622072c4d67c

1 files changed, 242 insertions, 128 deletions

diff --git a/apps/plugins/otp.c b/apps/plugins/otp.c
index 9c835ed..3f9dd49 100644
--- a/apps/plugins/otp.c
+++ b/apps/plugins/otp.c
@@ -58,19 +58,33 @@ struct account_t {
     int sec_len;
 };
 
-static int max_accts = 0;
-
 /* in plugin buffer */
 static struct account_t *accounts = NULL;
 
+/* global variables */
+
+static int max_accts = 0; // dynamic, depends on plugin buffer size
 static int next_slot = 0;
 
 /* in SECONDS, asked for on first run */
 static int time_offs = 0;
-
 static bool encrypted = false;
 
-static char enc_password[PASS_MAX + 1];
+static char enc_password[PASS_MAX + 1]; // encryption password
+static char data_buf[MAX(MAX_NAME, MAX(SECRET_MAX * 2, sizeof(struct account_t)))];
+static char temp_sec[SECRET_MAX];
+
+static void wipe_buf(void *ptr, size_t len)
+{
+    rb->memset(ptr, 0, len);
+}
+
+static void erase_sensitive_info(void)
+{
+    wipe_buf(accounts, sizeof(struct account_t) * max_accts);
+    wipe_buf(enc_password, sizeof(enc_password));
+    wipe_buf(temp_sec, sizeof(temp_sec));
+}
 
 static void acct_menu(const char *title, void (*cb)(int acct));
 
@@ -113,6 +127,8 @@ static time_t get_utc(void)
 
 static int TOTP(unsigned char *secret, size_t sec_len, uint64_t step, int digits)
 {
+    if(!step)
+        return -1;
     uint64_t tm = get_utc() / step;
     return HOTP(secret, sec_len, tm, digits);
 }
@@ -145,78 +161,78 @@ static bool acct_exists(const char *name)
 // limitations under the License.
 
 static int base32_decode(uint8_t *result, int bufSize, const uint8_t *encoded) {
-  int buffer = 0;
-  int bitsLeft = 0;
-  int count = 0;
-  for (const uint8_t *ptr = encoded; count < bufSize && *ptr; ++ptr) {
-    uint8_t ch = *ptr;
-    if (ch == ' ' || ch == '\t' || ch == '\r' || ch == '\n' || ch == '-') {
-      continue;
-    }
-    buffer <<= 5;
-
-    // Deal with commonly mistyped characters
-    if (ch == '0') {
-      ch = 'O';
-    } else if (ch == '1') {
-      ch = 'L';
-    } else if (ch == '8') {
-      ch = 'B';
-    }
+    int buffer = 0;
+    int bitsLeft = 0;
+    int count = 0;
+    for (const uint8_t *ptr = encoded; count < bufSize && *ptr; ++ptr) {
+        uint8_t ch = *ptr;
+        if (ch == ' ' || ch == '\t' || ch == '\r' || ch == '\n' || ch == '-') {
+            continue;
+        }
+        buffer <<= 5;
+
+        // Deal with commonly mistyped characters
+        if (ch == '0') {
+            ch = 'O';
+        } else if (ch == '1') {
+            ch = 'L';
+        } else if (ch == '8') {
+            ch = 'B';
+        }
 
-    // Look up one base32 digit
-    if ((ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z')) {
-      ch = (ch & 0x1F) - 1;
-    } else if (ch >= '2' && ch <= '7') {
-      ch -= '2' - 26;
-    } else {
-      return -1;
-    }
+        // Look up one base32 digit
+        if ((ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z')) {
+            ch = (ch & 0x1F) - 1;
+        } else if (ch >= '2' && ch <= '7') {
+            ch -= '2' - 26;
+        } else {
+            return -1;
+        }
 
-    buffer |= ch;
-    bitsLeft += 5;
-    if (bitsLeft >= 8) {
-      result[count++] = buffer >> (bitsLeft - 8);
-      bitsLeft -= 8;
+        buffer |= ch;
+        bitsLeft += 5;
+        if (bitsLeft >= 8) {
+            result[count++] = buffer >> (bitsLeft - 8);
+            bitsLeft -= 8;
+        }
+    }
+    if (count < bufSize) {
+        result[count] = '\000';
     }
-  }
-  if (count < bufSize) {
-    result[count] = '\000';
-  }
-  return count;
+    return count;
 }
 
 static int base32_encode(const uint8_t *data, int length, uint8_t *result,
                          int bufSize) {
-  if (length < 0 || length > (1 << 28)) {
-    return -1;
-  }
-  int count = 0;
-  if (length > 0) {
-    int buffer = data[0];
-    int next = 1;
-    int bitsLeft = 8;
-    while (count < bufSize && (bitsLeft > 0 || next < length)) {
-      if (bitsLeft < 5) {
-        if (next < length) {
-          buffer <<= 8;
-          buffer |= data[next++] & 0xFF;
-          bitsLeft += 8;
-        } else {
-          int pad = 5 - bitsLeft;
-          buffer <<= pad;
-          bitsLeft += pad;
+    if (length < 0 || length > (1 << 28)) {
+        return -1;
+    }
+    int count = 0;
+    if (length > 0) {
+        int buffer = data[0];
+        int next = 1;
+        int bitsLeft = 8;
+        while (count < bufSize && (bitsLeft > 0 || next < length)) {
+            if (bitsLeft < 5) {
+                if (next < length) {
+                    buffer <<= 8;
+                    buffer |= data[next++] & 0xFF;
+                    bitsLeft += 8;
+                } else {
+                    int pad = 5 - bitsLeft;
+                    buffer <<= pad;
+                    bitsLeft += pad;
+                }
+            }
+            int index = 0x1F & (buffer >> (bitsLeft - 5));
+            bitsLeft -= 5;
+            result[count++] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"[index];
         }
-      }
-      int index = 0x1F & (buffer >> (bitsLeft - 5));
-      bitsLeft -= 5;
-      result[count++] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"[index];
     }
-  }
-  if (count < bufSize) {
-    result[count] = '\000';
-  }
-  return count;
+    if (count < bufSize) {
+        result[count] = '\000';
+    }
+    return count;
 }
 
 /***********************************************************************
@@ -270,6 +286,7 @@ static void aes_ctr_nextblock(struct aes_ctr_ctx *ctx)
     ctx->bytes_left = 16;
 }
 
+/* should be safe to operate in-place */
 static void aes_ctr_process(struct aes_ctr_ctx *ctx, const unsigned char *in, unsigned char *out, size_t len)
 {
     while(len--)
@@ -282,16 +299,12 @@ static void aes_ctr_process(struct aes_ctr_ctx *ctx, const unsigned char *in, un
 
 static void aes_ctr_destroy(struct aes_ctr_ctx *ctx)
 {
-    rb->memset(ctx, 0, sizeof(ctx));
-    rb->memset(ctx, 0xff, sizeof(ctx));
-    rb->memset(ctx, 0, sizeof(ctx));
+    wipe_buf(ctx, sizeof(*ctx));
 #ifdef HAVE_ADJUSTABLE_CPU_FREQ
     rb->cpu_boost(false);
 #endif
 }
 
-static unsigned char in_buf[sizeof(struct account_t)];
-
 static bool read_accts(void)
 {
     int fd = rb->open(ACCT_FILE, O_RDONLY);
@@ -321,12 +334,22 @@ static bool read_accts(void)
             uint64_t nonce;
             rb->read(fd, &nonce, sizeof(nonce));
 
-            /* read the encrypted header */
-            rb->read(fd, in_buf, 4);
+            /* read in the hash, which is encrypted */
+            /* in order to detect proper decryption, we must decrypt
+             * the hash and account data, and ensure that the
+             * calculated hash matches the given hash */
+            char hash_enc[20];
+            rb->read(fd, hash_enc, sizeof(hash_enc));
+
+            /* also read the encrypted data into memory */
+            while(next_slot < max_accts)
+            {
+                if(rb->read(fd, accounts + next_slot, sizeof(struct account_t)) != sizeof(struct account_t))
+                    break;
+                ++next_slot;
+            }
 
-            /* and the hash */
-            char hash_orig[20], hash[20];
-            rb->read(fd, hash_orig, sizeof(hash_orig));
+            rb->close(fd);
 
             for(int i = 0; i < 3; ++i)
             {
@@ -349,34 +372,33 @@ static bool read_accts(void)
 
                 aes_ctr_init(&aes_ctx, key, nonce);
 
-                /* decrypt the magic to see if the password works */
-                aes_ctr_process(&aes_ctx, in_buf, buf, 4);
-                if(rb->memcmp(buf, magic, 4))
-                {
-                    rb->splashf(HZ * 4, "Wrong password!");
-                    continue;
-                }
+                char hash_given[20];
+                aes_ctr_process(&aes_ctx, hash_enc, hash_given, sizeof(hash_given));
 
-                aes_ctr_process(&aes_ctx, hash_orig, hash_orig, sizeof(hash));
+                aes_ctr_process(&aes_ctx, (const unsigned char*)accounts, (char*)accounts, sizeof(struct account_t) * next_slot);
 
-                while(next_slot < max_accts)
-                {
-                    if(rb->read(fd, in_buf, sizeof(in_buf)) != sizeof(struct account_t))
-                        break;
-                    aes_ctr_process(&aes_ctx, in_buf, (unsigned char*)(accounts + next_slot), sizeof(struct account_t));
-                    ++next_slot;
-                }
+                char hash_calculated[20];
+                sha1_buffer((const char*)accounts, sizeof(struct account_t) * next_slot, hash_calculated);
 
                 aes_ctr_destroy(&aes_ctx);
 
-                rb->close(fd);
+                if(rb->memcmp(hash_calculated, hash_given, 20) != 0)
+                {
+                    /* failed attempt, we restore the account data
+                     * which was decrypted in-place by encrypting it
+                     * again with the given parameters */
+                    aes_ctr_init(&aes_ctx, key, nonce);
+                    aes_ctr_process(&aes_ctx, hash_enc, hash_given, sizeof(hash_given));
+                    aes_ctr_process(&aes_ctx, (const unsigned char*)accounts, (char*)accounts, sizeof(struct account_t) * next_slot);
+                    aes_ctr_destroy(&aes_ctx);
+                    rb->splash(HZ, "Wrong password!");
+                    continue;
+                }
 
+                /* successful decryption */
                 return true;
             }
 
-            /* failure */
-            rb->close(fd);
-
             exit(PLUGIN_ERROR);
         }
     }
@@ -394,8 +416,6 @@ static bool read_accts(void)
     return true;
 }
 
-static char out_buf[sizeof(struct account_t)];
-
 static void save_accts(void)
 {
     int fd = rb->open(ACCT_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0600);
@@ -405,7 +425,8 @@ static void save_accts(void)
     rb->write(fd, &time_offs, sizeof(time_offs));
     rb->write(fd, &encrypted, sizeof(encrypted));
 
-    assert(sizeof(out_buf) == sizeof(struct account_t));
+    assert(sizeof(data_buf) >= sizeof(struct account_t));
+    assert(sizeof(data_buf) >= 20); // needs to hold an SHA-1 hash
 
     if(encrypted)
     {
@@ -414,14 +435,14 @@ static void save_accts(void)
         /* generate/write the nonce */
         uint64_t nonce = *rb->current_tick;
 #if CONFIG_RTC
-        nonce |= get_utc() << 32;
+        nonce |= (uint64_t)get_utc() << 32;
 #endif
 
         rb->write(fd, &nonce, sizeof(nonce));
 
         /* the HMAC-SHA-1 of the password and nonce is truncated to
          * 128 bits to form the AES key. a salted MD5 would work, but
-         * an HMAC is more readily available and suited to this
+         * an HMAC is more readily available and better suited to this
          * purpose */
         char key[20];
 
@@ -430,23 +451,16 @@ static void save_accts(void)
         struct aes_ctr_ctx aes_ctx;
         aes_ctr_init(&aes_ctx, key, nonce);
 
-        const char *magic = "OTP2";
-
-        /* write the encrypted "magic" value so the decryptor knows
-         * the password is valid */
-        aes_ctr_process(&aes_ctx, magic, out_buf, 4);
-        rb->write(fd, out_buf, 4);
-
-        /* write the encrypted SHA-1 of the data to detect
-         * corruption */
-        sha1_buffer(accounts, sizeof(struct account_t) * next_slot, out_buf);
-        aes_ctr_process(&aes_ctx, out_buf, out_buf, 20);
-        rb->write(fd, out_buf, 20);
+        /* write the encrypted SHA-1 of the data to ensure integrity
+         * and alert the decrypting end of validity */
+        sha1_buffer((const char*)accounts, sizeof(struct account_t) * next_slot, data_buf);
+        aes_ctr_process(&aes_ctx, data_buf, data_buf, 20);
+        rb->write(fd, data_buf, 20);
 
         for(int i = 0; i < next_slot; ++i)
         {
-            aes_ctr_process(&aes_ctx, (unsigned char*)(accounts + i), out_buf, sizeof(struct account_t));
-            rb->write(fd, out_buf, sizeof(out_buf));
+            aes_ctr_process(&aes_ctx, (unsigned char*)(accounts + i), data_buf, sizeof(struct account_t));
+            rb->write(fd, data_buf, sizeof(struct account_t));
         }
 
         aes_ctr_destroy(&aes_ctx);
@@ -812,10 +826,6 @@ static bool danger_confirm(void)
     }
 }
 
-char data_buf[MAX(MAX_NAME, SECRET_MAX * 2)];
-char temp_sec[SECRET_MAX];
-size_t old_len;
-
 static void edit_menu(int acct)
 {
     rb->splashf(HZ, "Editing account `%s'.", accounts[acct].name);
@@ -908,8 +918,9 @@ static void edit_menu(int acct)
             rb->splash(HZ, "Success.");
             break;
         case 4: // secret
+        {
             /* save the old secret */
-            old_len = accounts[acct].sec_len;
+            size_t old_len = accounts[acct].sec_len;
             rb->memcpy(temp_sec, accounts[acct].secret, accounts[acct].sec_len);
 
             /* encode */
@@ -933,6 +944,7 @@ static void edit_menu(int acct)
             rb->splash(HZ, "Success.");
 
             break;
+        }
 #ifdef CONFIG_RTC
         case 5:
         {
@@ -1246,12 +1258,28 @@ static void encrypt_menu(void)
         break;
     }
     case 1:
+    {
         if(menu == &encrypt_menu_1)
         {
+            char temp_pass[sizeof(enc_password)];
+            temp_pass[0] = '\0';
+
+            rb->splash(HZ * 2, "Enter current password:");
+
+            if(rb->kbd_input(temp_pass, sizeof(temp_pass)) < 0)
+                break;
+
+            if(rb->strcmp(enc_password, temp_pass))
+            {
+                rb->splash(HZ * 2, "Wrong password!");
+                break;
+            }
+
             rb->splash(HZ * 2, "Success.");
             encrypted = false;
         }
         /* fall through */
+    }
     case 2:
     default:
         break;
@@ -1327,15 +1355,99 @@ static void show_help(void)
 #endif
 
     static char *help_text[] = { "One-Time Password Manager", "",
+                                 "",
                                  "Introduction", "",
-                                 "This", "plugin", "allows", "you", "to", "generate", "one-time", "passwords", "to", "provide", "a", "second", "factor", "of", "authentication", "for", "services", "that", "support", "it.",
-                                 "It", "suppports", "both", "event-based", "(HOTP),", "and", "time-based", "(TOTP)", "password", "schemes.",
-                                 "In", "order", "to", "ensure", "proper", "functioning", "of", "time-based", "passwords", "ensure", "that", "the", "clock", "is", "accurate", "to", "within", "30", "seconds", "of", "actual", "time."
-                                 "Note", "that", "some", "devices", "lack", "a", "real-time", "clock,", "so", "time-based", "passwords", "are", "not", "supported", "on", "those", "targets." };
-
+                                 "This", "plugin", "allows", "you", "to", "generate", "one-time", "passwords", "as", "a", "second", "factor", "of", "authentication", "for", "online", "services", "which", "support", "it,", "such", "as", "GitHub", "and", "Google.",
+                                 "This", "plugin", "supports", "both", "counter-based", "(HOTP),", "and", "time-based", "(TOTP)", "password", "schemes.",
+                                 "",
+                                 "",
+                                 "Time Zone Configuration", "",
+                                 "On", "the", "first", "run", "of", "the", "plugin,", "you", "are", "asked", "for", "the", "time", "zone", "to", "which", "your", "system", "clock", "is", "set.",
+                                 "If", "you", "need", "to", "change", "this", "setting", "later,", "it", "is", "available", "under", "the", "'Advanced'", "menu", "option.",
+                                 "",
+                                 "",
+                                 "Account Setup", "",
+                                 "To", "add", "a", "new", "account,", "choose", "the", "'Import", "Account(s)'", "menu", "option.",
+                                 "There", "are", "two", "ways", "to", "import", "an", "account,", "either", "from", "a", "file", "containing", "account", "information", "in", "URI", "format,", "or", "manual", "entry.",
+                                 "",
+                                 "",
+                                 "URI Import", "",
+                                 "This", "method", "of", "adding", "an", "account", "reads", "a", "list", "of", "URIs", "from", "a", "file.",
+                                 "It", "expects", "each", "URI", "to", "be", "on", "a", "line", "by", "itself", "in", "the", "following", "format:", "",
+                                 "",
+                                 "otpauth://[hotp", "OR", "totp]/[account", "name]?secret=[Base32", "secret][&counter=X][&period=X][&digits=X]", "",
+                                 "",
+                                 "An", "example", "is", "shown", "below,", "provisioning", "a", "TOTP", "key", "for", "an", "account", "called", "``bob'':", "",
+                                 "",
+                                 "otpauth://totp/bob?secret=JBSWY3DPEHPK3PXP", "",
+                                 "",
+                                 "Any", "other", "URI", "options", "are", "not", "supported", "and", "will", "be", "ignored.",
+                                 "",
+                                 "Most", "services", "will", "provide", "a", "scannable", "QR", "code", "that", "encodes", "a", "OTP", "URI.",
+                                 "In", "order", "to", "use", "those,", "first", "scan", "the", "QR", "code", "separately", "and", "save", "the", "URI", "to", "a", "file", "on", "your", "device.",
+                                 "If", "necessary,", "rewrite", "the", "URI", "so", "it", "is", "in", "the", "format", "shown", "above.",
+                                 "For", "example,", "GitHub's", "URI", "has", "a", "slash", "after", "the", "provider.",
+                                 "In", "order", "for", "this", "URI", "to", "be", "properly", "parsed,", "you", "must", "rewrite", "the", "account", "name", "so", "that", "it", "does", "not", "contain", "a", "slash.",
+                                 "",
+                                 "",
+                                 "Manual Import", "",
+                                 "If", "direct", "URI", "import", "is", "not", "possible,", "the", "plugin", "supports", "the", "manual", "entry", "of", "data", "associated", "with", "an", "account.",
+                                 "After", "you", "select", "the", "'Manual", "Entry'", "option,", "it", "will", "prompt", "you", "for", "an", "account", "name.",
+                                 "You", "may", "type", "anything", "you", "wish,", "but", "it", "should", "be", "memorable.",
+                                 "It", "will", "then", "prompt", "you", "for", "the", "Base32-encoded", "secret.",
+                                 "Most", "services", "will", "provide", "this", "to", "you", "directly,", "but", "some", "may", "only", "provide", "you", "with", "a", "QR", "code.",
+                                 "In", "these", "cases,", "you", "must", "scan", "the", "QR", "code", "separately,", "and", "then", "enter", "the", "string", "following", "the", "'secret='", "parameter", "on", "your", "Rockbox", "device", "manually.",
+                                 "",
+                                 "On", "devices", "with", "a", "real-time", "clock,", "the", "plugin", "will", "ask", "whether", "the", "account", "is", "a", "time-based", "account", "(TOTP).",
+                                 "If", "you", "answer", "'yes'", "to", "this", "question,", "it", "will", "ask", "for", "further", "information", "regarding", "the", "account.",
+                                 "Usually", "it", "is", "safe", "to", "accept", "the", "defaults", "here.",
+                                 "However,", "if", "your", "device", "lacks", "a", "real-time", "clock,", "the", "plugin's", "functionality", "will", "be", "restricted", "to", "HMAC-based", "(HOTP)", "accounts", "only.",
+                                 "If", "this", "is", "the", "case,", "the", "plugin", "will", "prompt", "you", "for", "information", "regarding", "the", "HOTP", "setup.",
+                                 "Again,", "it", "is", "usually", "safe", "to", "accept", "the", "defaults.",
+                                 "",
+                                 "",
+                                 "Account Export", "",
+                                 "This", "plugin", "allows", "you", "to", "export", "account", "data", "to", "a", "file", "for", "backup", "and", "transfer", "purposes.",
+                                 "This", "option", "is", "located", "under", "the", "'Advanced'", "menu.",
+                                 "It", "will", "prompt", "for", "for", "a", "filename,", "and", "will", "write", "all", "your", "account", "data", "to", "the", "specified", "file.",
+                                 "This", "file", "can", "be", "imported", "by", "this", "plugin", "using", "the", "'From", "URI", "List'", "option", "when", "importing.",
+                                 "",
+                                 "",
+                                 "Encryption", "",
+                                 "This", "plugin", "supports", "the", "optional", "encryption", "of", "account", "data", "while", "stored", "on", "disk.",
+                                 "This", "feature", "is", "located", "under", "the", "'Advanced'", "menu", "option.",
+                                 "Upon", "enabling", "this", "feature,", "you", "must", "enter", "an", "encryption", "password", "that", "will", "need", "to", "be", "entered", "each", "time", "the", "plugin", "starts", "up.",
+                                 "It", "is", "recommended", "that", "you", "use", "a", "strong,", "alphanumeric", "password", "of", "at", "least", "8", "characters", "in", "order", "to", "frustrate", "attempts", "to", "crack", "the", "encryption.",
+                                 "Be", "sure", "not", "to", "forget", "this", "password.",
+                                 "In", "the", "event", "that", "the", "password", "is", "lost,", "it", "is", "nearly", "impossible", "to", "recover", "your", "account", "data.",
+                                 "",
+                                 "",
+                                 "Implementation Details", "",
+                                 "Account", "data", "is", "encrypted", "with", "128-bit", "AES", "encryption", "in", "counter", "mode,", "keyed", "with", "the", "truncated", "HMAC-SHA-1", "of", "your", "password", "and", "a", "nonce.",
+                                 "The", "nonce", "is", "generated", "from", "the", "system's", "current", "tick", "and", "the", "real-time", "clock,", "if", "available.",
+                                 "",
+                                 "",
+                                 "Troubleshooting", "",
+#if CONFIG_RTC
+                                 "If", "time-based", "passwords", "and", "not", "working", "properly,", "ensure", "that", "your", "system", "clock", "is", "accurate", "to", "within", "30", "seconds", "of", "the", "authenticating", "server's", "clock,", "and", "that", "the", "proper", "time", "zone", "is", "configured", "within", "the", "plugin.",
+                                 "Be", "sure", "to", "account", "for", "Daylight", "Savings", "Time,", "if", "applicable.",
+                                 "",
+#else
+                                 "Please", "note", "that", "your", "device", "lacks", "a", "real-time", "clock,", "and", "thus", "time-based", "passwords", "are", "not", "supported.",
+                                 "",
+#endif
+    };
     struct style_text style[] = {
-        {0,  TEXT_CENTER | TEXT_UNDERLINE},
-        {2,  C_RED},
+        { 0, TEXT_CENTER | TEXT_UNDERLINE },
+        { 3, C_RED },
+        { 43, C_RED },
+        { 84, C_RED },
+        { 120, C_RED },
+        { 273, C_RED },
+        { 461, C_RED },
+        { 523, C_RED },
+        { 619, C_RED },
+        { 660, C_RED },
         LAST_STYLE_ITEM
     };
 
@@ -1367,7 +1479,7 @@ static bool wait_for_usb(void)
             }
             else if(button)
             {
-                /* check if a key's being held down */
+                /* check if a key is being held down */
 
                 if(oldbutton == 0)
                 {
@@ -1530,6 +1642,8 @@ enum plugin_status plugin_start(const void* parameter)
     accounts = rb->plugin_get_buffer(&bufsz);
     max_accts = bufsz / sizeof(struct account_t);
 
+    atexit(erase_sensitive_info);
+
     if(!read_accts())
 #if CONFIG_RTC
     {