invalid response on cancel fixes #381

tiny-firmware/bootloader/usb.c

@@ -178,6 +178,7 @@ static int hid_control_request(usbd_device* dev, struct usb_setup_data* req, uin
 enum {
     STATE_READY,
     STATE_OPEN,
+    STATE_ABOUT_TO_FLASHSTART,
     STATE_FLASHSTART,
     STATE_FLASHING,
     STATE_CHECK,
@@ -481,59 +482,65 @@ static void hid_rx_callback(usbd_device* dev, uint8_t ep)
     if (flash_state == STATE_OPEN) {
         if (msg_id == 0x0006) { // FirmwareErase message (id 6)
             if (!brand_new_firmware) {
-                layoutDialog(&bmp_icon_question, "Abort", "Continue", NULL, "Install new", "firmware?", NULL, "Never do this without", "your recovery card!", NULL);
-                do {
-                    delay(100000);
-                    buttonUpdate();
-                } while (!button.YesUp && !button.NoUp);
+                flash_state = STATE_ABOUT_TO_FLASHSTART;
+                send_msg_buttonrequest_firmwarecheck(dev);
+                return;
             }
-            if (brand_new_firmware || button.YesUp) {
-                // check whether current firmware is signed
-                if (!brand_new_firmware && SIG_OK == signatures_ok(NULL)) {
-                    old_was_unsigned = false;
-                    // backup metadata
-                    backup_metadata(meta_backup);
-                } else {
-                    old_was_unsigned = true;
-                }
-                flash_wait_for_last_operation();
-                flash_clear_status_flags();
-                flash_unlock();
-                // erase metadata area
-                for (int i = FLASH_META_SECTOR_FIRST; i <= FLASH_META_SECTOR_LAST; i++) {
-                    layoutProgress("ERASING ... Please wait", 1000 * (i - FLASH_META_SECTOR_FIRST) / (FLASH_CODE_SECTOR_LAST - FLASH_META_SECTOR_FIRST));
-                    flash_erase_sector(i, FLASH_CR_PROGRAM_X32);
-                }
-                // erase code area
-                for (int i = FLASH_CODE_SECTOR_FIRST; i <= FLASH_CODE_SECTOR_LAST; i++) {
-                    layoutProgress("ERASING ... Please wait", 1000 * (i - FLASH_META_SECTOR_FIRST) / (FLASH_CODE_SECTOR_LAST - FLASH_META_SECTOR_FIRST));
-                    flash_erase_sector(i, FLASH_CR_PROGRAM_X32);
-                }
-                layoutProgress("INSTALLING ... Please wait", 0);
-                flash_wait_for_last_operation();
-                flash_lock();
-
-                // check that metadata was succesfully erased
-                // flash status register should show now error and
-                // the config block should contain only \xff.
-                uint8_t hash[32];
-                sha256_Raw((unsigned char*)FLASH_META_START, FLASH_META_LEN, hash);
-                if ((FLASH_SR & (FLASH_SR_PGAERR | FLASH_SR_PGPERR | FLASH_SR_PGSERR | FLASH_SR_WRPERR)) != 0 || memcmp(hash, "\x2d\x86\x4c\x0b\x78\x9a\x43\x21\x4e\xee\x85\x24\xd3\x18\x20\x75\x12\x5e\x5c\xa2\xcd\x52\x7f\x35\x82\xec\x87\xff\xd9\x40\x76\xbc", 32) != 0) {
-                    send_msg_failure(dev);
-                    flash_state = STATE_END;
-                    layoutDialog(&bmp_icon_error, NULL, NULL, NULL, "Error installing ", "firmware.", NULL, "Unplug your Skywallet", "and try again.", NULL);
-                    return;
-                }
+        }
+    }
+    if (flash_state == STATE_ABOUT_TO_FLASHSTART) {
+        if (!brand_new_firmware) {
+            layoutDialog(&bmp_icon_question, "Abort", "Continue", NULL, "Install new", "firmware?", NULL, "Never do this without", "your recovery card!", NULL);
+            do {
+                delay(100000);
+                buttonUpdate();
+            } while (!button.YesUp && !button.NoUp);
+        }
+        if (brand_new_firmware || button.YesUp) {
+            // check whether current firmware is signed
+            if (!brand_new_firmware && SIG_OK == signatures_ok(NULL)) {
+                old_was_unsigned = false;
+                // backup metadata
+                backup_metadata(meta_backup);
+            } else {
+                old_was_unsigned = true;
+            }
+            flash_wait_for_last_operation();
+            flash_clear_status_flags();
+            flash_unlock();
+            // erase metadata area
+            for (int i = FLASH_META_SECTOR_FIRST; i <= FLASH_META_SECTOR_LAST; i++) {
+                layoutProgress("ERASING ... Please wait", 1000 * (i - FLASH_META_SECTOR_FIRST) / (FLASH_CODE_SECTOR_LAST - FLASH_META_SECTOR_FIRST));
+                flash_erase_sector(i, FLASH_CR_PROGRAM_X32);
+            }
+            // erase code area
+            for (int i = FLASH_CODE_SECTOR_FIRST; i <= FLASH_CODE_SECTOR_LAST; i++) {
+                layoutProgress("ERASING ... Please wait", 1000 * (i - FLASH_META_SECTOR_FIRST) / (FLASH_CODE_SECTOR_LAST - FLASH_META_SECTOR_FIRST));
+                flash_erase_sector(i, FLASH_CR_PROGRAM_X32);
+            }
+            layoutProgress("INSTALLING ... Please wait", 0);
+            flash_wait_for_last_operation();
+            flash_lock();
 
-                send_msg_success(dev);
-                flash_state = STATE_FLASHSTART;
+            // check that metadata was succesfully erased
+            // flash status register should show now error and
+            // the config block should contain only \xff.
+            uint8_t hash[32];
+            sha256_Raw((unsigned char*)FLASH_META_START, FLASH_META_LEN, hash);
+            if ((FLASH_SR & (FLASH_SR_PGAERR | FLASH_SR_PGPERR | FLASH_SR_PGSERR | FLASH_SR_WRPERR)) != 0 || memcmp(hash, "\x2d\x86\x4c\x0b\x78\x9a\x43\x21\x4e\xee\x85\x24\xd3\x18\x20\x75\x12\x5e\x5c\xa2\xcd\x52\x7f\x35\x82\xec\x87\xff\xd9\x40\x76\xbc", 32) != 0) {
+                send_msg_failure(dev);
+                flash_state = STATE_END;
+                layoutDialog(&bmp_icon_error, NULL, NULL, NULL, "Error installing ", "firmware.", NULL, "Unplug your Skywallet", "and try again.", NULL);
                 return;
             }
-            send_msg_failure(dev);
-            flash_state = STATE_END;
-            layoutDialog(&bmp_icon_warning, NULL, NULL, NULL, "Firmware installation", "aborted.", NULL, "You may now", "unplug your Skywallet.", NULL);
+
+            send_msg_success(dev);
+            flash_state = STATE_FLASHSTART;
             return;
         }
+        send_msg_failure(dev);
+        flash_state = STATE_END;
+        layoutDialog(&bmp_icon_warning, NULL, NULL, NULL, "Firmware installation", "aborted.", NULL, "You may now", "unplug your Skywallet.", NULL);
         return;
     }
 

tiny-firmware/firmware/fsm_impl.c

@@ -535,18 +535,24 @@ ErrCode_t msgPingImpl(Ping* msg)
     return ErrOk;
 }
 
-ErrCode_t msgChangePinImpl(ChangePin* msg, const char* (*funcRequestPin)(PinMatrixRequestType, const char*))
+ErrCode_t msgChangePinImpl(ChangePin* msg, ErrCode_t (*funcRequestPin)(PinMatrixRequestType, const char*, char*))
 {
     bool removal = msg->has_remove && msg->remove;
     if (removal) {
         storage_setPin("");
         storage_update();
-    } else {
-        if (!protectChangePinEx(funcRequestPin)) {
-            return ErrPinMismatch;
-        }
+        return ErrOk;
+    }
+    ErrCode_t err = protectChangePinEx(funcRequestPin);
+    switch (err) {
+    case ErrPinRequired:
+    case ErrPinCancelled:
+    case ErrPinMismatch:
+    case ErrOk:
+        return err;
+    default:
+        return ErrUnexpectedMessage;
     }
-    return ErrOk;
 }
 
 ErrCode_t msgWipeDeviceImpl(WipeDevice* msg)

tiny-firmware/firmware/fsm_impl.h

@@ -192,7 +192,7 @@ ErrCode_t msgGetFeaturesImpl(Features* resp);
 
 ErrCode_t msgPingImpl(Ping* msg);
 
-ErrCode_t msgChangePinImpl(ChangePin* msg, const char* (*)(PinMatrixRequestType, const char*));
+ErrCode_t msgChangePinImpl(ChangePin* msg, ErrCode_t (*)(PinMatrixRequestType, const char*, char*));
 
 ErrCode_t msgWipeDeviceImpl(WipeDevice* msg);
 

tiny-firmware/firmware/protect.c

@@ -112,7 +112,7 @@ bool protectButton(ButtonRequestType type, bool confirm_only)
     return result;
 }
 
-const char* requestPin(PinMatrixRequestType type, const char* text)
+ErrCode_t requestPin(PinMatrixRequestType type, const char* text, char* out_pin)
 {
     PinMatrixRequest resp;
     memset(&resp, 0, sizeof(PinMatrixRequest));
@@ -128,16 +128,23 @@ const char* requestPin(PinMatrixRequestType type, const char* text)
             PinMatrixAck* pma = (PinMatrixAck*)msg_tiny;
             pinmatrix_done(pma->pin); // convert via pinmatrix
             usbTiny(0);
-            return pma->pin;
+            memcpy(out_pin, pma->pin, sizeof(pma->pin));
+            return ErrOk;
         }
         if (msg_tiny_id == MessageType_MessageType_Cancel || msg_tiny_id == MessageType_MessageType_Initialize) {
             pinmatrix_done(0);
             if (msg_tiny_id == MessageType_MessageType_Initialize) {
                 protectAbortedByInitialize = true;
+                msg_tiny_id = 0xFFFF;
+                usbTiny(0);
+                // TODO what does means Initialize here?
+                return ErrOk;
+            }
+            if (msg_tiny_id == MessageType_MessageType_Cancel) {
+                msg_tiny_id = 0xFFFF;
+                usbTiny(0);
+                return ErrPinCancelled;
             }
-            msg_tiny_id = 0xFFFF;
-            usbTiny(0);
-            return 0;
         }
 #if DEBUG_LINK
         if (msg_tiny_id == MessageType_MessageType_DebugLinkGetState) {
@@ -195,11 +202,20 @@ bool protectPin(bool use_cached)
         wait--;
     }
     usbTiny(0);
-    const char* pin;
-    pin = requestPin(PinMatrixRequestType_PinMatrixRequestType_Current, _("Please enter current PIN:"));
-    if (!pin) {
+    char pin[10] = {0};
+    {
+        PinMatrixAck pm;
+        _Static_assert(sizeof(pin) == sizeof(pm.pin), "invalid pin buffer size");
+    }
+    switch (requestPin(PinMatrixRequestType_PinMatrixRequestType_Current, _("Please enter current PIN:"), pin)) {
+    case ErrOk:
+        break;
+    case ErrPinCancelled:
         fsm_sendFailure(FailureType_Failure_PinCancelled, NULL, 0);
         return false;
+    default:
+        fsm_sendFailure(FailureType_Failure_UnexpectedMessage, NULL, 0);
+        return false;
     }
     if (!storage_increasePinFails(fails)) {
         fsm_sendFailure(FailureType_Failure_PinInvalid, NULL, 0);
@@ -221,33 +237,60 @@ bool protectChangePin()
     return protectChangePinEx(NULL);
 }
 
-bool protectChangePinEx(const char* (*funcRequestPin)(PinMatrixRequestType, const char*))
+ErrCode_t protectChangePinEx(ErrCode_t (*funcRequestPin)(PinMatrixRequestType, const char*, char*))
 {
     static CONFIDENTIAL char pin_compare[17];
+    memset(pin_compare, 0, sizeof(pin_compare));
     if (funcRequestPin == NULL) {
         funcRequestPin = requestPin;
     }
-
-    const char* pin = funcRequestPin(PinMatrixRequestType_PinMatrixRequestType_NewFirst, _("Please enter new PIN:"));
-
-    if (!pin) {
-        return false;
+    static CONFIDENTIAL char pin[10] = {0};
+    memset(pin, 0, sizeof(pin));
+    {
+        PinMatrixAck pm;
+        _Static_assert(sizeof(pin) == sizeof(pm.pin), "invalid pin buffer size");
+    }
+    ErrCode_t err = funcRequestPin(PinMatrixRequestType_PinMatrixRequestType_NewFirst, _("Please enter new PIN:"), pin);
+    if (err != ErrOk) {
+        memset(pin_compare, 0, sizeof(pin_compare));
+        memset(pin, 0, sizeof(pin));
+        return err;
+    }
+    {
+        char empty_pin[sizeof(pin)] = {0};
+        if (!memcmp(pin, empty_pin, sizeof(pin))) {
+            memset(pin_compare, 0, sizeof(pin_compare));
+            memset(pin, 0, sizeof(pin));
+            return ErrPinRequired;
+        }
     }
-
     strlcpy(pin_compare, pin, sizeof(pin_compare));
-
-    pin = funcRequestPin(PinMatrixRequestType_PinMatrixRequestType_NewSecond, _("Please re-enter new PIN:"));
-
-    const bool result = pin && *pin && (strncmp(pin_compare, pin, sizeof(pin_compare)) == 0);
-
-    if (result) {
+    memset(pin, 0, sizeof(pin));
+    err = funcRequestPin(PinMatrixRequestType_PinMatrixRequestType_NewSecond, _("Please re-enter new PIN:"), pin);
+    if (err != ErrOk) {
+        memset(pin_compare, 0, sizeof(pin_compare));
+        memset(pin, 0, sizeof(pin));
+        return err;
+    }
+    {
+        char empty_pin[sizeof(pin)] = {0};
+        if (!memcmp(pin, empty_pin, sizeof(pin))) {
+            memset(pin_compare, 0, sizeof(pin_compare));
+            memset(pin, 0, sizeof(pin));
+            return ErrPinRequired;
+        }
+    }
+    if (strncmp(pin_compare, pin, sizeof(pin_compare)) == 0) {
         storage_setPin(pin_compare);
         storage_update();
+    } else {
+        memset(pin_compare, 0, sizeof(pin_compare));
+        memset(pin, 0, sizeof(pin));
+        return ErrPinMismatch;
     }
-
-    memzero(pin_compare, sizeof(pin_compare));
-
-    return result;
+    memset(pin_compare, 0, sizeof(pin_compare));
+    memset(pin, 0, sizeof(pin));
+    return ErrOk;
 }
 
 bool protectPassphrase(void)

tiny-firmware/firmware/protect.h

@@ -18,9 +18,10 @@
  * along with this library.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __PROTECT_H__
-#define __PROTECT_H__
+#ifndef __PROTECT_HANDLER_H__
+#define __PROTECT_HANDLER_H__
 
+#include "tiny-firmware/firmware/error.h"
 #include "types.pb.h"
 #include <stdbool.h>
 
@@ -32,7 +33,7 @@ bool protectPassphrase(void);
 extern bool protectAbortedByInitialize;
 
 // Symbols exported for testing
-bool protectChangePinEx(const char* (*)(PinMatrixRequestType, const char*));
-const char* requestPin(PinMatrixRequestType type, const char* text);
+ErrCode_t protectChangePinEx(ErrCode_t (*)(PinMatrixRequestType, const char*, char*));
+ErrCode_t requestPin(PinMatrixRequestType type, const char* text, char* out_pin);
 
-#endif
+#endif // __PROTECT_HANDLER_H__

tiny-firmware/firmware/reset.c

@@ -41,7 +41,7 @@ void reset_init(bool display_random, uint32_t _strength, bool passphrase_protect
     reset_init_ex(display_random, _strength, passphrase_protection, pin_protection, language, label, _skip_backup, NULL);
 }
 
-void reset_init_ex(bool display_random, uint32_t _strength, bool passphrase_protection, bool pin_protection, const char* language, const char* label, bool _skip_backup, const char* (*funcRequestPin)(PinMatrixRequestType, const char*))
+void reset_init_ex(bool display_random, uint32_t _strength, bool passphrase_protection, bool pin_protection, const char* language, const char* label, bool _skip_backup, ErrCode_t (*funcRequestPin)(PinMatrixRequestType, const char*, char*))
 {
     if (funcRequestPin == NULL) {
         funcRequestPin = requestPin;
@@ -70,13 +70,20 @@ void reset_init_ex(bool display_random, uint32_t _strength, bool passphrase_prot
             return;
         }
     }
-
-    if (pin_protection && !protectChangePinEx(funcRequestPin)) {
-        fsm_sendFailure(FailureType_Failure_PinMismatch, NULL, 0);
-        layoutHome();
-        return;
+    if (pin_protection) {
+        ErrCode_t err = protectChangePinEx(funcRequestPin);
+        switch (err) {
+        case ErrOk:
+            break;
+        case ErrPinMismatch:
+            fsm_sendFailure(FailureType_Failure_PinMismatch, NULL, 0);
+            layoutHome();
+            return;
+        default:
+            fsm_sendFailure(FailureType_Failure_UnexpectedMessage, NULL, 0);
+            return;
+        }
     }
-
     storage_setPassphraseProtection(passphrase_protection);
     storage_setLanguage(language);
     if (label != NULL && strcmp("", label) != 0) {

tiny-firmware/firmware/reset.h

@@ -40,6 +40,6 @@ uint32_t reset_get_int_entropy(uint8_t* entropy);
 const char* reset_get_word(void);
 
 // Functions exported or testing purposes
-void reset_init_ex(bool display_random, uint32_t _strength, bool passphrase_protection, bool pin_protection, const char* language, const char* label, bool _skip_backup, const char* (*funcRequestPin)(PinMatrixRequestType mt, const char* msg));
+void reset_init_ex(bool display_random, uint32_t _strength, bool passphrase_protection, bool pin_protection, const char* language, const char* label, bool _skip_backup, ErrCode_t (*funcRequestPin)(PinMatrixRequestType mt, const char* msg, char* out_pin));
 
 #endif