Reworked pin module

- Moved pin buffer to an internal buffer in pin.c
- Removed pin_t/pin_ctx from bolos_ux.h
- Adapted calls to keep pin buffer manipulation inside pin module

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;

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;
         };
     };
 

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

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.

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,19 +50,18 @@ 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);
     }
 
     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());
 }