diff --git a/ledger/src/ui/src/bolos_ux.c b/ledger/src/ui/src/bolos_ux.c index 899e59f6..0c5ffab4 100644 --- a/ledger/src/ui/src/bolos_ux.c +++ b/ledger/src/ui/src/bolos_ux.c @@ -55,9 +55,6 @@ #include "memutil.h" #include "unlock.h" -// PIN buffer used for authenticated operations -unsigned char G_pin_buffer[PIN_BUFFER_LENGTH]; - #ifdef OS_IO_SEPROXYHAL #define ARRAYLEN(array) (sizeof(array) / sizeof(array[0])) @@ -238,9 +235,6 @@ void io_seproxyhal_display(const bagl_element_t *element) { // Signer authorization context shorthand #define sigaut_ctx (G_bolos_ux_context.sigaut) -// Pin context shorthand -#define pin_ctx (G_bolos_ux_context.pin) - // Onboard context shorthand #define onboard_ctx (G_bolos_ux_context.onboard) @@ -259,7 +253,6 @@ static void reset_if_starting(unsigned char cmd) { curr_cmd = cmd; reset_attestation(&attestation_ctx); reset_signer_authorization(&sigaut_ctx); - reset_pin_ctx(&pin_ctx); reset_onboard_ctx(&onboard_ctx); } } @@ -311,8 +304,7 @@ static void sample_main(void) { break; case RSK_PIN_CMD: // Send pin_buffer reset_if_starting(RSK_META_CMD_UIOP); - init_pin_ctx(&pin_ctx, G_pin_buffer); - tx = update_pin_buffer(rx, &pin_ctx); + tx = update_pin_buffer(rx); THROW(APDU_OK); break; case RSK_IS_ONBOARD: // Wheter it's onboarded or not @@ -322,23 +314,14 @@ static void sample_main(void) { break; case RSK_WIPE: //--- wipe and onboard device --- reset_if_starting(RSK_META_CMD_UIOP); - init_pin_ctx(&pin_ctx, G_pin_buffer); - tx = onboard_device(&onboard_ctx, &pin_ctx); - // Clear pin buffer - explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); + tx = onboard_device(&onboard_ctx); + clear_pin(); THROW(APDU_OK); break; case RSK_NEWPIN: reset_if_starting(RSK_META_CMD_UIOP); - init_pin_ctx(&pin_ctx, G_pin_buffer); -#ifndef DEBUG_BUILD - if (!is_pin_valid(&pin_ctx)) { - THROW(ERR_INVALID_PIN); - } -#endif - tx = set_device_pin(&pin_ctx); - // Clear pin buffer - explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); + tx = set_pin(); + clear_pin(); THROW(APDU_OK); break; case RSK_ECHO_CMD: // echo @@ -368,8 +351,7 @@ static void sample_main(void) { break; case RSK_UNLOCK_CMD: // Unlock reset_if_starting(RSK_META_CMD_UIOP); - init_pin_ctx(&pin_ctx, G_pin_buffer); - tx = unlock(&pin_ctx); + tx = unlock(); // The pin value will also be used in // BOLOS_UX_CONSENT_APP_ADD command, so we can't wipe the // pin buffer here @@ -552,11 +534,9 @@ void bolos_ux_main(void) { // PIN is invalidated so we must check it again. The pin value // used here is the same as in RSK_UNLOCK_CMD, so we also // don't have a prepended length byte - init_pin_ctx(&pin_ctx, G_pin_buffer); - os_global_pin_check(pin_ctx.pin_buffer, - strlen((const char *)pin_ctx.pin_buffer)); + validate_pin(false); G_bolos_ux_context.exit_code = BOLOS_UX_OK; - explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); + clear_pin(); break; } else { G_bolos_ux_context.exit_code = BOLOS_UX_CANCEL; diff --git a/ledger/src/ui/src/bolos_ux.h b/ledger/src/ui/src/bolos_ux.h index a1012673..74cdb464 100644 --- a/ledger/src/ui/src/bolos_ux.h +++ b/ledger/src/ui/src/bolos_ux.h @@ -141,12 +141,7 @@ typedef struct bolos_ux_context { union { att_t attestation; sigaut_t sigaut; - // pin and onboard are used in the same context, so they can't share - // the same memory region - struct { - pin_t pin; - onboard_t onboard; - }; + onboard_t onboard; }; }; diff --git a/ledger/src/ui/src/onboard.c b/ledger/src/ui/src/onboard.c index c6df27cf..5539fbb0 100644 --- a/ledger/src/ui/src/onboard.c +++ b/ledger/src/ui/src/onboard.c @@ -48,11 +48,10 @@ void reset_onboard_ctx(onboard_t *onboard_ctx) { * * Wipes and onboards the device. * - * @arg[in] pin_ctx pin context * @arg[out] onboard_ctx onboard context * @ret number of transmited bytes to the host */ -unsigned int onboard_device(onboard_t *onboard_ctx, pin_t *pin_ctx) { +unsigned int onboard_device(onboard_t *onboard_ctx) { volatile unsigned char onboarded_flag = 0; // Reset the onboarding flag to mark onboarding @@ -62,7 +61,7 @@ unsigned int onboard_device(onboard_t *onboard_ctx, pin_t *pin_ctx) { sizeof(onboarded_flag)); #ifndef DEBUG_BUILD - if (!is_pin_valid(pin_ctx)) { + if (!is_pin_valid()) { THROW(ERR_INVALID_PIN); } #endif @@ -100,14 +99,13 @@ unsigned int onboard_device(onboard_t *onboard_ctx, pin_t *pin_ctx) { sizeof(onboard_ctx->words_buffer)); // Set PIN - os_perso_set_pin(0, GET_PIN(pin_ctx), GET_PIN_LENGTH(pin_ctx)); + set_device_pin(); // Finalize onboarding os_perso_finalize(); os_global_pin_invalidate(); unsigned char output_index = CMDPOS; SET_APDU_AT(output_index++, 2); - SET_APDU_AT(output_index++, - os_global_pin_check(GET_PIN(pin_ctx), GET_PIN_LENGTH(pin_ctx))); + SET_APDU_AT(output_index++, validate_pin(true)); // Turn the onboarding flag on to mark onboarding // has been done using the UI diff --git a/ledger/src/ui/src/onboard.h b/ledger/src/ui/src/onboard.h index 70b17db7..30a92778 100644 --- a/ledger/src/ui/src/onboard.h +++ b/ledger/src/ui/src/onboard.h @@ -52,11 +52,10 @@ void reset_onboard_ctx(onboard_t *onboard_ctx); * * Wipes and onboards the device. * - * @arg[in] pin_ctx pin context * @arg[out] onboard_ctx onboard context * @ret number of transmited bytes to the host */ -unsigned int onboard_device(onboard_t *onboard_ctx, pin_t *pin_ctx); +unsigned int onboard_device(onboard_t *onboard_ctx); /* * Implement the RSK SEED command. diff --git a/ledger/src/ui/src/pin.c b/ledger/src/ui/src/pin.c index f243fe06..72eecf0c 100644 --- a/ledger/src/ui/src/pin.c +++ b/ledger/src/ui/src/pin.c @@ -35,50 +35,13 @@ #define IS_NUM(c) IS_IN_RANGE(c, '0', '9') #define IS_ALPHANUM(c) (IS_ALPHA(c) || IS_NUM(c)) -/* - * Validates that the pin has exactly PIN_LENGTH alphanumeric characters - * with at least one alphabetic character. - * - * @arg[in] pin_ctx pin context (with prepended length) - * @ret true if pin is valid, false otherwise - */ -bool is_pin_valid(pin_t* pin_ctx) { - // PIN_LENGTH is the only length accepted - if (GET_PIN_LENGTH(pin_ctx) != PIN_LENGTH) { - return false; - } - // Check if PIN is alphanumeric - bool hasAlpha = false; - for (int i = 0; i < PIN_LENGTH; i++) { - if (!IS_ALPHANUM(GET_PIN(pin_ctx)[i])) { - return false; - } - if (hasAlpha || IS_ALPHA(GET_PIN(pin_ctx)[i])) { - hasAlpha = true; - } - } - - return hasAlpha; -} - -/* - * Reset the given pin context to point to a target buffer - * - * @arg[out] pin_ctx pin context - * @arg[in] pin_buffer pin buffer to which the pin context should point - */ -void init_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer) { - pin_ctx->pin_buffer = pin_buffer; -} - -/* - * Reset the given pin context - * - * @arg[in] pin_ctx pin context - */ -void reset_pin_ctx(pin_t* pin_ctx) { - explicit_bzero(pin_ctx, sizeof(pin_t)); -} +#define PIN_LENGTH 8 +#define PIN_BUFFER_LENGTH (PIN_LENGTH + 2) +// Internal PIN buffer used for authenticated operations +unsigned char G_pin_buffer[PIN_BUFFER_LENGTH]; +// Helper macros for pin manipulation when prepended length is used +#define GET_PIN() ((unsigned char *)(G_pin_buffer + 1)) +#define GET_PIN_LENGTH() strlen((const char *)GET_PIN()) /* * Implements RSK PIN command. @@ -87,10 +50,9 @@ void reset_pin_ctx(pin_t* pin_ctx) { * at the end. * * @arg[in] rx number of received bytes from the Host - * @arg[in] pin_ctx pin context * @ret number of transmited bytes to the host */ -unsigned int update_pin_buffer(volatile unsigned int rx, pin_t* pin_ctx) { +unsigned int update_pin_buffer(volatile unsigned int rx) { // Should receive 1 byte per call if (APDU_DATA_SIZE(rx) != 1) { THROW(PROT_INVALID); @@ -98,8 +60,8 @@ unsigned int update_pin_buffer(volatile unsigned int rx, pin_t* pin_ctx) { unsigned char index = APDU_OP(); if ((index >= 0) && (index <= PIN_LENGTH)) { - pin_ctx->pin_buffer[index] = APDU_AT(DATA); - pin_ctx->pin_buffer[index + 1] = 0; + G_pin_buffer[index] = APDU_AT(DATA); + G_pin_buffer[index + 1] = 0; } return 3; @@ -108,24 +70,79 @@ unsigned int update_pin_buffer(volatile unsigned int rx, pin_t* pin_ctx) { /* * Implements RSK NEW PIN command. * - * Sets the device pin. + * Sets and checks the device pin. * - * @arg[in] pin_ctx pin context - * @ret number of transmited bytes to the host + * @ret number of transmited bytes to the host */ -unsigned int set_device_pin(pin_t* pin_ctx) { +unsigned int set_pin() { #ifndef DEBUG_BUILD - if (!is_pin_valid(pin_ctx)) { + if (!is_pin_valid()) { THROW(ERR_INVALID_PIN); } #endif // Set PIN - os_perso_set_pin(0, GET_PIN(pin_ctx), GET_PIN_LENGTH(pin_ctx)); + os_perso_set_pin(0, GET_PIN(), GET_PIN_LENGTH()); // check PIN os_global_pin_invalidate(); unsigned char output_index = CMDPOS; SET_APDU_AT(output_index++, 2); SET_APDU_AT(output_index++, - os_global_pin_check(GET_PIN(pin_ctx), GET_PIN_LENGTH(pin_ctx))); + os_global_pin_check(GET_PIN(), GET_PIN_LENGTH())); return output_index; +} + +/* + * Validates that the pin curently saved to the internal buffer has exactly + * PIN_LENGTH alphanumeric characters with at least one alphabetic character. + * + * @ret true if pin is valid, false otherwise + */ +bool is_pin_valid() { + // PIN_LENGTH is the only length accepted + if (GET_PIN_LENGTH() != PIN_LENGTH) { + return false; + } + // Check if PIN is alphanumeric + bool hasAlpha = false; + for (int i = 0; i < PIN_LENGTH; i++) { + if (!IS_ALPHANUM(GET_PIN()[i])) { + return false; + } + if (hasAlpha || IS_ALPHA(GET_PIN()[i])) { + hasAlpha = true; + } + } + + return hasAlpha; +} + +/* + * Fills the internal pin buffer with zeroes + */ +void clear_pin() { + explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); +} + +/* + * Validates the pin currently saved to the internal pin buffer + * + * @arg[in] prepended_length true if the internal buffer includes a prepended + * length byte, false otherwise + * @ret 1 if pin validated successfully, 0 otherwise + */ +unsigned int validate_pin(bool prepended_length) { + if (prepended_length) { + return os_global_pin_check(GET_PIN(), GET_PIN_LENGTH()); + } else { + return os_global_pin_check(G_pin_buffer, + strlen((const char *)G_pin_buffer)); + } +} + +/* + * Sets the pin currently saved to the internal pin buffer as the device's pin. + * This function assumes the pin is saved with a prepended length byte. + */ +void set_device_pin() { + os_perso_set_pin(0, GET_PIN(), GET_PIN_LENGTH()); } \ No newline at end of file diff --git a/ledger/src/ui/src/pin.h b/ledger/src/ui/src/pin.h index 226eef65..2ba801fb 100644 --- a/ledger/src/ui/src/pin.h +++ b/ledger/src/ui/src/pin.h @@ -27,33 +27,6 @@ #include -#define PIN_LENGTH 8 -#define PIN_BUFFER_LENGTH (PIN_LENGTH + 2) - -// Pin context -typedef struct { - unsigned char* pin_buffer; -} pin_t; - -// Helper macros for pin context manipulation -#define GET_PIN(pin_ctx) ((unsigned char*)((pin_ctx)->pin_buffer + 1)) -#define GET_PIN_LENGTH(pin_ctx) strlen((const char*)GET_PIN(pin_ctx)) - -/* - * Reset the given pin context - * - * @arg[in] pin_ctx pin context - */ -void reset_pin_ctx(pin_t* pin_ctx); - -/* - * Reset the given pin context to point to a target buffer - * - * @arg[out] pin_ctx pin context - * @arg[in] pin_buffer pin buffer to which the pin context should point - */ -void init_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer); - // ----------------------------------------------------------------------- // RSK protocol implementation // ----------------------------------------------------------------------- @@ -65,32 +38,49 @@ void init_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer); * at the end. * * @arg[in] rx number of received bytes from the Host - * @arg[in] pin_ctx pin context * @ret number of transmited bytes to the host */ -unsigned int update_pin_buffer(volatile unsigned int rx, pin_t* pin_ctx); +unsigned int update_pin_buffer(volatile unsigned int rx); /* * Implements RSK NEW PIN command. * - * Sets the device pin. + * Sets and checks the device pin. * - * @arg[in] pin_ctx pin context - * @ret number of transmited bytes to the host + * @ret number of transmited bytes to the host */ -unsigned int set_device_pin(pin_t* pin_ctx); +unsigned int set_pin(); // ----------------------------------------------------------------------- // Pin manipulation utilities // ----------------------------------------------------------------------- /* - * Validates that the pin has exactly PIN_LENGTH alphanumeric characters - * with at least one alphabetic character. + * Validates that the pin curently saved to the internal buffer has exactly + * PIN_LENGTH alphanumeric characters with at least one alphabetic character. * - * @arg[in] pin_ctx pin context (with prepended length) * @ret true if pin is valid, false otherwise */ -bool is_pin_valid(pin_t* pin_ctx); +bool is_pin_valid(); + +/* + * Fills the internal pin buffer with zeroes + */ +void clear_pin(); + +/* + * Validates the pin currently saved to the internal pin buffer + * + * @arg[in] prepended_length true if the internal buffer includes a prepended + * length byte, false otherwise + * @ret 1 if pin validated successfully, 0 otherwise + */ +unsigned int validate_pin(bool prepended_length); + +/* + * Sets the pin currently saved to the internal pin buffer as the device's pin. + * This function assumes the pin is saved with a prepended length byte. + */ +void set_device_pin(); #endif \ No newline at end of file diff --git a/ledger/src/ui/src/unlock.c b/ledger/src/ui/src/unlock.c index 34aa31d5..8195d9d1 100644 --- a/ledger/src/ui/src/unlock.c +++ b/ledger/src/ui/src/unlock.c @@ -32,13 +32,10 @@ * * Unlocks the device. * - * @arg[in] pin_ctx pin context * @ret number of transmited bytes to the host */ -unsigned int unlock(pin_t *pin_ctx) { +unsigned int unlock() { unsigned char output_index = OP; - SET_APDU_AT(output_index++, - os_global_pin_check(pin_ctx->pin_buffer, - strlen((const char *)pin_ctx->pin_buffer))); + SET_APDU_AT(output_index++, validate_pin(false)); return output_index; } diff --git a/ledger/src/ui/src/unlock.h b/ledger/src/ui/src/unlock.h index b9ebffd8..4b0e0f2e 100644 --- a/ledger/src/ui/src/unlock.h +++ b/ledger/src/ui/src/unlock.h @@ -32,9 +32,8 @@ * * Unlocks the device. * - * @arg[in] pin_ctx pin context * @ret number of transmited bytes to the host */ -unsigned int unlock(pin_t *pin_ctx); +unsigned int unlock(); #endif