From 88ff14719ffa50f70b6fc38df3b59dcab742daf5 Mon Sep 17 00:00:00 2001 From: Alvaro Denis Date: Wed, 15 Jan 2020 17:19:57 -0500 Subject: [PATCH 1/7] Fix chnagePin message Change pin should return canceled when a cancel command is recived not a pin mismatch ref #381 --- tiny-firmware/firmware/fsm_impl.c | 17 ++++-- tiny-firmware/firmware/fsm_impl.h | 2 +- tiny-firmware/firmware/protect.c | 92 ++++++++++++++++++++++--------- tiny-firmware/firmware/protect.h | 11 ++-- tiny-firmware/firmware/reset.c | 21 ++++--- tiny-firmware/firmware/reset.h | 2 +- 6 files changed, 98 insertions(+), 47 deletions(-) diff --git a/tiny-firmware/firmware/fsm_impl.c b/tiny-firmware/firmware/fsm_impl.c index e3aa50b..ad9f8cd 100644 --- a/tiny-firmware/firmware/fsm_impl.c +++ b/tiny-firmware/firmware/fsm_impl.c @@ -535,18 +535,23 @@ 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; + } } ErrCode_t msgWipeDeviceImpl(WipeDevice* msg) diff --git a/tiny-firmware/firmware/fsm_impl.h b/tiny-firmware/firmware/fsm_impl.h index 4c63ae6..194566d 100644 --- a/tiny-firmware/firmware/fsm_impl.h +++ b/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); diff --git a/tiny-firmware/firmware/protect.c b/tiny-firmware/firmware/protect.c index 8af30ac..068ea4b 100644 --- a/tiny-firmware/firmware/protect.c +++ b/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) { - fsm_sendFailure(FailureType_Failure_PinCancelled, NULL, 0); - return false; + char pin[10] = {0}; + { + PinMatrixAck pm = {0}; + _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,55 @@ 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 = {0}; + _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); + { + 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) diff --git a/tiny-firmware/firmware/protect.h b/tiny-firmware/firmware/protect.h index 5a413c7..edb0a9b 100644 --- a/tiny-firmware/firmware/protect.h +++ b/tiny-firmware/firmware/protect.h @@ -18,10 +18,11 @@ * along with this library. If not, see . */ -#ifndef __PROTECT_H__ -#define __PROTECT_H__ +#ifndef __PROTECT_HANDLER_H__ +#define __PROTECT_HANDLER_H__ #include "types.pb.h" +#include "tiny-firmware/firmware/error.h" #include bool protectButton(ButtonRequestType type, bool confirm_only); @@ -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__ diff --git a/tiny-firmware/firmware/reset.c b/tiny-firmware/firmware/reset.c index 34b18a6..ce32d9e 100644 --- a/tiny-firmware/firmware/reset.c +++ b/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) { diff --git a/tiny-firmware/firmware/reset.h b/tiny-firmware/firmware/reset.h index 679a2ad..82a8ad6 100644 --- a/tiny-firmware/firmware/reset.h +++ b/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 From d3715f6e78e3028b897e211cc1726cd436bae016 Mon Sep 17 00:00:00 2001 From: Alvaro Denis Date: Fri, 31 Jan 2020 17:14:43 -0500 Subject: [PATCH 2/7] bootloader - fix firmware upload, button request on confirmaion trough wire ref #381 --- tiny-firmware/bootloader/usb.c | 103 ++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 48 deletions(-) diff --git a/tiny-firmware/bootloader/usb.c b/tiny-firmware/bootloader/usb.c index ffd9472..3a6c084 100644 --- a/tiny-firmware/bootloader/usb.c +++ b/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; - } - - send_msg_success(dev); - flash_state = STATE_FLASHSTART; + } + } + 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(); + + // 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; } From 2e1901c007f03daa6b8ed2a8351a677fc3c4f3f0 Mon Sep 17 00:00:00 2001 From: Alvaro Denis Date: Wed, 12 Feb 2020 14:23:35 -0500 Subject: [PATCH 3/7] fix change pin tests and remove pin implementation ref #381 --- tiny-firmware/firmware/fsm_impl.c | 1 + tiny-firmware/tests/test_pin.c | 21 +++++++++++++-------- tiny-firmware/tests/test_pin.h | 6 +++--- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/tiny-firmware/firmware/fsm_impl.c b/tiny-firmware/firmware/fsm_impl.c index ad9f8cd..3a2d14f 100644 --- a/tiny-firmware/firmware/fsm_impl.c +++ b/tiny-firmware/firmware/fsm_impl.c @@ -541,6 +541,7 @@ ErrCode_t msgChangePinImpl(ChangePin* msg, ErrCode_t (*funcRequestPin)(PinMatrix if (removal) { storage_setPin(""); storage_update(); + return ErrOk; } ErrCode_t err = protectChangePinEx(funcRequestPin); switch (err) { diff --git a/tiny-firmware/tests/test_pin.c b/tiny-firmware/tests/test_pin.c index 8471429..d13b0b6 100644 --- a/tiny-firmware/tests/test_pin.c +++ b/tiny-firmware/tests/test_pin.c @@ -14,30 +14,35 @@ char* TEST_PIN1 = "123"; char* TEST_PIN2 = "246"; -const char* pin_reader_ok(PinMatrixRequestType pinReqType, const char* text) +ErrCode_t pin_reader_ok(PinMatrixRequestType pinReqType, const char* text, char *out_pin) { (void)text; (void)pinReqType; - return TEST_PIN1; + strcpy(out_pin, TEST_PIN1); + return ErrOk; } -const char* pin_reader_alt(PinMatrixRequestType pinReqType, const char* text) +ErrCode_t pin_reader_alt(PinMatrixRequestType pinReqType, const char* text, char *pin_out) { (void)text; (void)pinReqType; - return TEST_PIN2; + strcpy(pin_out, TEST_PIN2); + return ErrOk; } -const char* pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text) +ErrCode_t pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text, char *pin_out) { (void)text; switch (pinReqType) { case PinMatrixRequestType_PinMatrixRequestType_NewFirst: - return TEST_PIN1; + strcpy(pin_out, TEST_PIN1); + break; case PinMatrixRequestType_PinMatrixRequestType_NewSecond: - return "456"; + strcpy(pin_out, "456"); + break; default: break; } - return "789"; + strcpy(pin_out, "789"); + return ErrPinMismatch; } diff --git a/tiny-firmware/tests/test_pin.h b/tiny-firmware/tests/test_pin.h index df42300..2e2803a 100644 --- a/tiny-firmware/tests/test_pin.h +++ b/tiny-firmware/tests/test_pin.h @@ -14,8 +14,8 @@ extern char* TEST_PIN1; extern char* TEST_PIN2; -const char* pin_reader_ok(PinMatrixRequestType pinReqType, const char* text); +ErrCode_t pin_reader_ok(PinMatrixRequestType pinReqType, const char* text, char *out_pin); -const char* pin_reader_alt(PinMatrixRequestType pinReqType, const char* text); +ErrCode_t pin_reader_alt(PinMatrixRequestType pinReqType, const char* text, char *pin_out); -const char* pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text); +ErrCode_t pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text, char *pin_out); From 31745a238e31ee5f60003a4e0c03ea35c38b5a11 Mon Sep 17 00:00:00 2001 From: Alvaro Denis Date: Wed, 12 Feb 2020 14:47:33 -0500 Subject: [PATCH 4/7] fix format ref #381 --- tiny-firmware/bootloader/usb.c | 4 ++-- tiny-firmware/firmware/fsm_impl.c | 14 +++++++------- tiny-firmware/firmware/fsm_impl.h | 2 +- tiny-firmware/firmware/fsm_skycoin_impl.c | 3 ++- tiny-firmware/firmware/protect.c | 18 +++++++++--------- tiny-firmware/firmware/protect.h | 6 +++--- tiny-firmware/firmware/reset.c | 18 +++++++++--------- tiny-firmware/firmware/reset.h | 2 +- tiny-firmware/tests/test_pin.c | 6 +++--- tiny-firmware/tests/test_pin.h | 6 +++--- 10 files changed, 40 insertions(+), 39 deletions(-) diff --git a/tiny-firmware/bootloader/usb.c b/tiny-firmware/bootloader/usb.c index 3a6c084..cafab80 100644 --- a/tiny-firmware/bootloader/usb.c +++ b/tiny-firmware/bootloader/usb.c @@ -521,7 +521,7 @@ static void hid_rx_callback(usbd_device* dev, uint8_t ep) 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. @@ -533,7 +533,7 @@ static void hid_rx_callback(usbd_device* dev, uint8_t ep) layoutDialog(&bmp_icon_error, NULL, NULL, NULL, "Error installing ", "firmware.", NULL, "Unplug your Skywallet", "and try again.", NULL); return; } - + send_msg_success(dev); flash_state = STATE_FLASHSTART; return; diff --git a/tiny-firmware/firmware/fsm_impl.c b/tiny-firmware/firmware/fsm_impl.c index 3a2d14f..0db0482 100644 --- a/tiny-firmware/firmware/fsm_impl.c +++ b/tiny-firmware/firmware/fsm_impl.c @@ -545,13 +545,13 @@ ErrCode_t msgChangePinImpl(ChangePin* msg, ErrCode_t (*funcRequestPin)(PinMatrix } ErrCode_t err = protectChangePinEx(funcRequestPin); switch (err) { - case ErrPinRequired: - case ErrPinCancelled: - case ErrPinMismatch: - case ErrOk: - return err; - default: - return ErrUnexpectedMessage; + case ErrPinRequired: + case ErrPinCancelled: + case ErrPinMismatch: + case ErrOk: + return err; + default: + return ErrUnexpectedMessage; } } diff --git a/tiny-firmware/firmware/fsm_impl.h b/tiny-firmware/firmware/fsm_impl.h index 194566d..7a29a04 100644 --- a/tiny-firmware/firmware/fsm_impl.h +++ b/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, ErrCode_t(*)(PinMatrixRequestType, const char *, char *)); +ErrCode_t msgChangePinImpl(ChangePin* msg, ErrCode_t (*)(PinMatrixRequestType, const char*, char*)); ErrCode_t msgWipeDeviceImpl(WipeDevice* msg); diff --git a/tiny-firmware/firmware/fsm_skycoin_impl.c b/tiny-firmware/firmware/fsm_skycoin_impl.c index eda34ee..860c9a7 100644 --- a/tiny-firmware/firmware/fsm_skycoin_impl.c +++ b/tiny-firmware/firmware/fsm_skycoin_impl.c @@ -139,7 +139,8 @@ ErrCode_t msgSkycoinSignMessageImpl(SkycoinSignMessage* msg, ResponseSkycoinSign return ErrOk; } -static bool the_first_address_only(SkycoinAddress* msg) { +static bool the_first_address_only(SkycoinAddress* msg) +{ if (msg->has_bip44_addr) { return msg->bip44_addr.address_start_index == 0 && msg->bip44_addr.address_n == 1; } else { diff --git a/tiny-firmware/firmware/protect.c b/tiny-firmware/firmware/protect.c index 068ea4b..e0aa399 100644 --- a/tiny-firmware/firmware/protect.c +++ b/tiny-firmware/firmware/protect.c @@ -112,7 +112,7 @@ bool protectButton(ButtonRequestType type, bool confirm_only) return result; } -ErrCode_t requestPin(PinMatrixRequestType type, const char* text, char *out_pin) +ErrCode_t requestPin(PinMatrixRequestType type, const char* text, char* out_pin) { PinMatrixRequest resp; memset(&resp, 0, sizeof(PinMatrixRequest)); @@ -208,14 +208,14 @@ bool protectPin(bool use_cached) _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; + 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); diff --git a/tiny-firmware/firmware/protect.h b/tiny-firmware/firmware/protect.h index edb0a9b..2fc621c 100644 --- a/tiny-firmware/firmware/protect.h +++ b/tiny-firmware/firmware/protect.h @@ -21,8 +21,8 @@ #ifndef __PROTECT_HANDLER_H__ #define __PROTECT_HANDLER_H__ -#include "types.pb.h" #include "tiny-firmware/firmware/error.h" +#include "types.pb.h" #include bool protectButton(ButtonRequestType type, bool confirm_only); @@ -34,6 +34,6 @@ extern bool protectAbortedByInitialize; // Symbols exported for testing ErrCode_t protectChangePinEx(ErrCode_t (*)(PinMatrixRequestType, const char*, char*)); -ErrCode_t requestPin(PinMatrixRequestType type, const char* text, char *out_pin); +ErrCode_t requestPin(PinMatrixRequestType type, const char* text, char* out_pin); -#endif // __PROTECT_HANDLER_H__ +#endif // __PROTECT_HANDLER_H__ diff --git a/tiny-firmware/firmware/reset.c b/tiny-firmware/firmware/reset.c index ce32d9e..c688992 100644 --- a/tiny-firmware/firmware/reset.c +++ b/tiny-firmware/firmware/reset.c @@ -73,15 +73,15 @@ void reset_init_ex(bool display_random, uint32_t _strength, bool passphrase_prot 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; + 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); diff --git a/tiny-firmware/firmware/reset.h b/tiny-firmware/firmware/reset.h index 82a8ad6..38d8dbe 100644 --- a/tiny-firmware/firmware/reset.h +++ b/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, ErrCode_t (*funcRequestPin)(PinMatrixRequestType mt, const char* msg, char *out_pin)); +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 diff --git a/tiny-firmware/tests/test_pin.c b/tiny-firmware/tests/test_pin.c index d13b0b6..b3943c1 100644 --- a/tiny-firmware/tests/test_pin.c +++ b/tiny-firmware/tests/test_pin.c @@ -14,7 +14,7 @@ char* TEST_PIN1 = "123"; char* TEST_PIN2 = "246"; -ErrCode_t pin_reader_ok(PinMatrixRequestType pinReqType, const char* text, char *out_pin) +ErrCode_t pin_reader_ok(PinMatrixRequestType pinReqType, const char* text, char* out_pin) { (void)text; (void)pinReqType; @@ -22,7 +22,7 @@ ErrCode_t pin_reader_ok(PinMatrixRequestType pinReqType, const char* text, char return ErrOk; } -ErrCode_t pin_reader_alt(PinMatrixRequestType pinReqType, const char* text, char *pin_out) +ErrCode_t pin_reader_alt(PinMatrixRequestType pinReqType, const char* text, char* pin_out) { (void)text; (void)pinReqType; @@ -30,7 +30,7 @@ ErrCode_t pin_reader_alt(PinMatrixRequestType pinReqType, const char* text, char return ErrOk; } -ErrCode_t pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text, char *pin_out) +ErrCode_t pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text, char* pin_out) { (void)text; switch (pinReqType) { diff --git a/tiny-firmware/tests/test_pin.h b/tiny-firmware/tests/test_pin.h index 2e2803a..c5e3af9 100644 --- a/tiny-firmware/tests/test_pin.h +++ b/tiny-firmware/tests/test_pin.h @@ -14,8 +14,8 @@ extern char* TEST_PIN1; extern char* TEST_PIN2; -ErrCode_t pin_reader_ok(PinMatrixRequestType pinReqType, const char* text, char *out_pin); +ErrCode_t pin_reader_ok(PinMatrixRequestType pinReqType, const char* text, char* out_pin); -ErrCode_t pin_reader_alt(PinMatrixRequestType pinReqType, const char* text, char *pin_out); +ErrCode_t pin_reader_alt(PinMatrixRequestType pinReqType, const char* text, char* pin_out); -ErrCode_t pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text, char *pin_out); +ErrCode_t pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text, char* pin_out); From 6f12ead668aa637db47f69541fa6e0baba00ed53 Mon Sep 17 00:00:00 2001 From: Alvaro Denis Date: Thu, 27 Feb 2020 23:55:19 -0500 Subject: [PATCH 5/7] fix var initialization syntax ref #381 --- tiny-firmware/firmware/protect.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tiny-firmware/firmware/protect.c b/tiny-firmware/firmware/protect.c index e0aa399..7a0df41 100644 --- a/tiny-firmware/firmware/protect.c +++ b/tiny-firmware/firmware/protect.c @@ -204,7 +204,7 @@ bool protectPin(bool use_cached) usbTiny(0); char pin[10] = {0}; { - PinMatrixAck pm = {0}; + PinMatrixAck pm; _Static_assert(sizeof(pin) == sizeof(pm.pin), "invalid pin buffer size"); } switch (requestPin(PinMatrixRequestType_PinMatrixRequestType_Current, _("Please enter current PIN:"), pin)) { @@ -247,7 +247,7 @@ ErrCode_t protectChangePinEx(ErrCode_t (*funcRequestPin)(PinMatrixRequestType, c static CONFIDENTIAL char pin[10] = {0}; memset(pin, 0, sizeof(pin)); { - PinMatrixAck pm = {0}; + 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); From fef704ffb089ab825bdd64e9ed6f43e974539980 Mon Sep 17 00:00:00 2001 From: Alvaro Denis Date: Wed, 4 Mar 2020 18:42:45 -0500 Subject: [PATCH 6/7] add test for pin canceled ref #381 --- tiny-firmware/tests/test_fsm.c | 12 ++++++++++++ tiny-firmware/tests/test_pin.c | 13 +++++++++++++ tiny-firmware/tests/test_pin.h | 2 ++ 3 files changed, 27 insertions(+) diff --git a/tiny-firmware/tests/test_fsm.c b/tiny-firmware/tests/test_fsm.c index 272c4d3..bd6ddda 100644 --- a/tiny-firmware/tests/test_fsm.c +++ b/tiny-firmware/tests/test_fsm.c @@ -416,6 +416,17 @@ START_TEST(test_msgChangePinSecondRejected) } END_TEST +START_TEST(test_msgChangePinShouldReturnCanceledAccordingToPinReader) +{ + ChangePin msg = ChangePin_init_zero; + storage_wipe(); + + // Pin mismatch + ck_assert_int_eq(msgChangePinImpl(&msg, &pin_reader_canceled), ErrPinCancelled); + ck_assert_int_eq(storage_hasPin(), false); +} +END_TEST + START_TEST(test_msgSkycoinCheckMessageSignatureBip44Ok) { for (size_t wi = 0; wi < sizeof(wcs) / sizeof(*wcs); ++wi) { @@ -532,6 +543,7 @@ TCase* add_fsm_tests(TCase* tc) tcase_add_test(tc, test_msgEntropyAckChgMixerNotInternal); tcase_add_test(tc, test_msgChangePinSuccess); tcase_add_test(tc, test_msgChangePinSecondRejected); + tcase_add_test(tc, test_msgChangePinShouldReturnCanceledAccordingToPinReader); tcase_add_test(tc, test_msgChangePinEditSuccess); tcase_add_test(tc, test_msgChangePinRemoveSuccess); tcase_add_test(tc, test_isSha256DigestHex); diff --git a/tiny-firmware/tests/test_pin.c b/tiny-firmware/tests/test_pin.c index b3943c1..f7e012e 100644 --- a/tiny-firmware/tests/test_pin.c +++ b/tiny-firmware/tests/test_pin.c @@ -46,3 +46,16 @@ ErrCode_t pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text, ch strcpy(pin_out, "789"); return ErrPinMismatch; } + +ErrCode_t pin_reader_canceled(PinMatrixRequestType pinReqType, const char* text, char* pin_out) +{ + (void)text; + (void)pin_out; + switch (pinReqType) { + case PinMatrixRequestType_PinMatrixRequestType_NewFirst: + case PinMatrixRequestType_PinMatrixRequestType_NewSecond: + return ErrPinCancelled; + default: + return ErrInvalidArg; + } +} diff --git a/tiny-firmware/tests/test_pin.h b/tiny-firmware/tests/test_pin.h index c5e3af9..dabc10e 100644 --- a/tiny-firmware/tests/test_pin.h +++ b/tiny-firmware/tests/test_pin.h @@ -19,3 +19,5 @@ ErrCode_t pin_reader_ok(PinMatrixRequestType pinReqType, const char* text, char* ErrCode_t pin_reader_alt(PinMatrixRequestType pinReqType, const char* text, char* pin_out); ErrCode_t pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text, char* pin_out); + +ErrCode_t pin_reader_canceled(PinMatrixRequestType pinReqType, const char* text, char* pin_out); From 5f32cc517c04fe1ed78f2e6732f35f751f2ff659 Mon Sep 17 00:00:00 2001 From: Alvaro Denis Date: Wed, 4 Mar 2020 20:59:01 -0500 Subject: [PATCH 7/7] add more tests for pin canceled ref #381 --- tiny-firmware/firmware/protect.c | 5 +++++ tiny-firmware/tests/test_fsm.c | 30 +++++++++++++++++++++++++----- tiny-firmware/tests/test_pin.c | 21 +++++++++++++++++++-- tiny-firmware/tests/test_pin.h | 4 +++- 4 files changed, 52 insertions(+), 8 deletions(-) diff --git a/tiny-firmware/firmware/protect.c b/tiny-firmware/firmware/protect.c index 7a0df41..0a20f1a 100644 --- a/tiny-firmware/firmware/protect.c +++ b/tiny-firmware/firmware/protect.c @@ -267,6 +267,11 @@ ErrCode_t protectChangePinEx(ErrCode_t (*funcRequestPin)(PinMatrixRequestType, c strlcpy(pin_compare, pin, sizeof(pin_compare)); 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))) { diff --git a/tiny-firmware/tests/test_fsm.c b/tiny-firmware/tests/test_fsm.c index bd6ddda..d5f05ba 100644 --- a/tiny-firmware/tests/test_fsm.c +++ b/tiny-firmware/tests/test_fsm.c @@ -416,14 +416,33 @@ START_TEST(test_msgChangePinSecondRejected) } END_TEST -START_TEST(test_msgChangePinShouldReturnCanceledAccordingToPinReader) +START_TEST(test_msgChangePinShouldReturnCanceledAccordingToPinReaderNewPin) { ChangePin msg = ChangePin_init_zero; storage_wipe(); - - // Pin mismatch - ck_assert_int_eq(msgChangePinImpl(&msg, &pin_reader_canceled), ErrPinCancelled); ck_assert_int_eq(storage_hasPin(), false); + ck_assert_int_eq(msgChangePinImpl(&msg, &pin_reader_new_canceled), ErrPinCancelled); + ck_assert_int_eq(storage_hasPin(), false); +} +END_TEST + +START_TEST(test_msgChangePinShouldReturnCanceledAccordingToPinReaderButPreserveAPreviuosOne) +{ + ErrCode_t (*pin_readers[2])(PinMatrixRequestType, const char*, char*) = { + &pin_reader_new_canceled, &pin_reader_confirm_canceled}; + for (size_t i = 0; i < sizeof(pin_readers) / sizeof(*pin_readers); ++i) { + ChangePin msg = ChangePin_init_zero; + storage_wipe(); + ck_assert_int_eq(storage_hasPin(), false); + + ck_assert_int_eq(msgChangePinImpl(&msg, &pin_reader_ok), ErrOk); + ck_assert_int_eq(storage_hasPin(), true); + ck_assert_str_eq(storage_getPin(), TEST_PIN1); + + ck_assert_int_eq(msgChangePinImpl(&msg, pin_readers[i]), ErrPinCancelled); + ck_assert_int_eq(storage_hasPin(), true); + ck_assert_str_eq(storage_getPin(), TEST_PIN1); + } } END_TEST @@ -543,7 +562,8 @@ TCase* add_fsm_tests(TCase* tc) tcase_add_test(tc, test_msgEntropyAckChgMixerNotInternal); tcase_add_test(tc, test_msgChangePinSuccess); tcase_add_test(tc, test_msgChangePinSecondRejected); - tcase_add_test(tc, test_msgChangePinShouldReturnCanceledAccordingToPinReader); + tcase_add_test(tc, test_msgChangePinShouldReturnCanceledAccordingToPinReaderNewPin); + tcase_add_test(tc, test_msgChangePinShouldReturnCanceledAccordingToPinReaderButPreserveAPreviuosOne); tcase_add_test(tc, test_msgChangePinEditSuccess); tcase_add_test(tc, test_msgChangePinRemoveSuccess); tcase_add_test(tc, test_isSha256DigestHex); diff --git a/tiny-firmware/tests/test_pin.c b/tiny-firmware/tests/test_pin.c index f7e012e..82fd36f 100644 --- a/tiny-firmware/tests/test_pin.c +++ b/tiny-firmware/tests/test_pin.c @@ -47,14 +47,31 @@ ErrCode_t pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text, ch return ErrPinMismatch; } -ErrCode_t pin_reader_canceled(PinMatrixRequestType pinReqType, const char* text, char* pin_out) +ErrCode_t pin_reader_new_canceled(PinMatrixRequestType pinReqType, const char* text, char* pin_out) { (void)text; - (void)pin_out; switch (pinReqType) { case PinMatrixRequestType_PinMatrixRequestType_NewFirst: + return ErrPinCancelled; + case PinMatrixRequestType_PinMatrixRequestType_Current: + case PinMatrixRequestType_PinMatrixRequestType_NewSecond: + strcpy(pin_out, TEST_PIN1); + return ErrOk; + default: + return ErrInvalidArg; + } +} + +ErrCode_t pin_reader_confirm_canceled(PinMatrixRequestType pinReqType, const char* text, char* pin_out) +{ + (void)text; + switch (pinReqType) { case PinMatrixRequestType_PinMatrixRequestType_NewSecond: return ErrPinCancelled; + case PinMatrixRequestType_PinMatrixRequestType_Current: + case PinMatrixRequestType_PinMatrixRequestType_NewFirst: + strcpy(pin_out, TEST_PIN1); + return ErrOk; default: return ErrInvalidArg; } diff --git a/tiny-firmware/tests/test_pin.h b/tiny-firmware/tests/test_pin.h index dabc10e..7dcc1d6 100644 --- a/tiny-firmware/tests/test_pin.h +++ b/tiny-firmware/tests/test_pin.h @@ -20,4 +20,6 @@ ErrCode_t pin_reader_alt(PinMatrixRequestType pinReqType, const char* text, char ErrCode_t pin_reader_wrong(PinMatrixRequestType pinReqType, const char* text, char* pin_out); -ErrCode_t pin_reader_canceled(PinMatrixRequestType pinReqType, const char* text, char* pin_out); +ErrCode_t pin_reader_new_canceled(PinMatrixRequestType pinReqType, const char* text, char* pin_out); + +ErrCode_t pin_reader_confirm_canceled(PinMatrixRequestType pinReqType, const char* text, char* pin_out);