From f1fd1e0609e1f4fa7ff3220655b1255a96629f5a Mon Sep 17 00:00:00 2001 From: italo sampaio Date: Mon, 17 Oct 2022 15:00:48 -0300 Subject: [PATCH 1/3] Factor out UI modules unlock and pin - Factor out unlock and pin modules - Add unit tests for unlock and pin --- ledger/src/ui/src/bolos_ux.c | 64 ++++++++++----------- ledger/src/ui/src/bolos_ux.h | 1 + ledger/src/ui/src/pin.c | 65 ++++++++++++++++----- ledger/src/ui/src/pin.h | 75 ++++++++++++++++++++++++- ledger/src/ui/src/unlock.c | 36 ++++++++++++ ledger/src/ui/src/unlock.h | 40 +++++++++++++ ledger/src/ui/test/mock/apdu.h | 1 + ledger/src/ui/test/mock/os.c | 60 ++++++++++++++++++++ ledger/src/ui/test/mock/os.h | 53 +++++++++++++++++ ledger/src/ui/test/pin/Makefile | 8 ++- ledger/src/ui/test/pin/test_pin.c | 73 +++++++++++++++++------- ledger/src/ui/test/unlock/Makefile | 53 +++++++++++++++++ ledger/src/ui/test/unlock/test_unlock.c | 66 ++++++++++++++++++++++ 13 files changed, 524 insertions(+), 71 deletions(-) create mode 100644 ledger/src/ui/src/unlock.c create mode 100644 ledger/src/ui/src/unlock.h create mode 120000 ledger/src/ui/test/mock/apdu.h create mode 100644 ledger/src/ui/test/mock/os.c create mode 100644 ledger/src/ui/test/mock/os.h create mode 100644 ledger/src/ui/test/unlock/Makefile create mode 100644 ledger/src/ui/test/unlock/test_unlock.c diff --git a/ledger/src/ui/src/bolos_ux.c b/ledger/src/ui/src/bolos_ux.c index b1012363..29803d26 100644 --- a/ledger/src/ui/src/bolos_ux.c +++ b/ledger/src/ui/src/bolos_ux.c @@ -52,16 +52,13 @@ #include "attestation.h" #include "signer_authorization.h" #include "memutil.h" +#include "unlock.h" // Onboarded with the UI flag const unsigned char N_onboarded_ui[1] = {0}; // PIN buffer used for authenticated operations -unsigned char G_pin_buffer[PIN_LENGTH + 2]; -// Skip the prepended length of pin buffer -#define G_PIN_BUFFER_PAYLOAD (G_pin_buffer + 1) -// Unify pin buffer length -#define G_PIN_BUFFER_LEN() strlen((const char *)G_PIN_BUFFER_PAYLOAD) +unsigned char G_pin_buffer[MAX_PIN_LENGTH + 2]; #ifdef OS_IO_SEPROXYHAL @@ -243,6 +240,9 @@ 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) + // Operation being currently executed static unsigned char curr_cmd; @@ -265,6 +265,7 @@ static void reset_if_starting(unsigned char cmd) { G_bolos_ux_context.words_buffer_length = 0; reset_attestation(&attestation_ctx); reset_signer_authorization(&sigaut_ctx); + reset_pin(&pin_ctx); } } @@ -321,11 +322,9 @@ static void sample_main(void) { break; case RSK_PIN_CMD: // Send pin_buffer reset_if_starting(RSK_META_CMD_UIOP); - pin = APDU_AT(2); - if ((pin >= 0) && (pin <= PIN_LENGTH)) { - G_pin_buffer[pin] = APDU_AT(3); - G_pin_buffer[pin + 1] = 0; - } + tx = pin_cmd(&pin_ctx); + // Save pin for authenticated operations + get_pin_ctx(&pin_ctx, G_pin_buffer); THROW(APDU_OK); break; case RSK_IS_ONBOARD: // Wheter it's onboarded or not @@ -347,8 +346,9 @@ static void sample_main(void) { nvm_write( (void *)PIC(N_onboarded_ui), (void *)&aux, sizeof(aux)); + set_pin_ctx(&pin_ctx, G_pin_buffer); #ifndef DEBUG_BUILD - if (!is_pin_valid(G_PIN_BUFFER_PAYLOAD)) { + if (!is_pin_valid(&pin_ctx)) { THROW(ERR_INVALID_PIN); } #endif @@ -392,14 +392,18 @@ static void sample_main(void) { sizeof(G_bolos_ux_context.words_buffer)); // Set PIN os_perso_set_pin( - 0, G_PIN_BUFFER_PAYLOAD, G_PIN_BUFFER_LEN()); + 0, + pin_ctx.pin_buffer.payload, + strlen((const char *)pin_ctx.pin_buffer.payload)); // Finalize onboarding os_perso_finalize(); os_global_pin_invalidate(); SET_APDU_AT(1, 2); - SET_APDU_AT(2, - os_global_pin_check(G_PIN_BUFFER_PAYLOAD, - G_PIN_BUFFER_LEN())); + SET_APDU_AT( + 2, + os_global_pin_check( + pin_ctx.pin_buffer.payload, + strlen((const char *)pin_ctx.pin_buffer.payload))); // Clear pin buffer explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); // Turn the onboarding flag on to mark onboarding @@ -414,21 +418,13 @@ static void sample_main(void) { case RSK_NEWPIN: reset_if_starting(RSK_META_CMD_UIOP); + set_pin_ctx(&pin_ctx, G_pin_buffer); #ifndef DEBUG_BUILD - if (!is_pin_valid(G_PIN_BUFFER_PAYLOAD)) { + if (!is_pin_valid(&pin_ctx)) { THROW(ERR_INVALID_PIN); } #endif - // Set PIN - os_perso_set_pin( - 0, G_PIN_BUFFER_PAYLOAD, G_PIN_BUFFER_LEN()); - // check PIN - os_global_pin_invalidate(); - SET_APDU_AT(1, 2); - SET_APDU_AT(2, - os_global_pin_check(G_PIN_BUFFER_PAYLOAD, - G_PIN_BUFFER_LEN())); - tx = 3; + tx = new_pin_cmd(&pin_ctx); // Clear pin buffer explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); THROW(APDU_OK); @@ -464,16 +460,13 @@ static void sample_main(void) { reset_if_starting(RSK_META_CMD_UIOP); // RSK_UNLOCK_CMD does not send the prepended length, // so we kept this call backwards compatible, using - // G_pin_buffer instead of G_PIN_BUFFER_PAYLOAD - SET_APDU_AT( - 2, - os_global_pin_check( - G_pin_buffer, strlen((const char *)G_pin_buffer))); - tx = 5; - THROW(APDU_OK); + // pin_ctx.pin_raw instead of pin_ctx.pin_buffer.payload + set_pin_ctx(&pin_ctx, G_pin_buffer); + tx = unlock(&pin_ctx); // The pin value will also be used in // BOLOS_UX_CONSENT_APP_ADD command, so we can't wipe the // pin buffer here + THROW(APDU_OK); break; case RSK_END_CMD: // return to dashboard reset_if_starting(RSK_END_CMD); @@ -652,8 +645,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 - os_global_pin_check(G_pin_buffer, - strlen((const char *)G_pin_buffer)); + set_pin_ctx(&pin_ctx, G_pin_buffer); + os_global_pin_check(pin_ctx.pin_raw, + strlen((const char *)pin_ctx.pin_raw)); G_bolos_ux_context.exit_code = BOLOS_UX_OK; explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); break; diff --git a/ledger/src/ui/src/bolos_ux.h b/ledger/src/ui/src/bolos_ux.h index 2622e928..172c50c1 100644 --- a/ledger/src/ui/src/bolos_ux.h +++ b/ledger/src/ui/src/bolos_ux.h @@ -160,6 +160,7 @@ typedef struct bolos_ux_context { union { att_t attestation; sigaut_t sigaut; + pin_t pin; }; }; diff --git a/ledger/src/ui/src/pin.c b/ledger/src/ui/src/pin.c index ffbaccdb..a05f6020 100644 --- a/ledger/src/ui/src/pin.c +++ b/ledger/src/ui/src/pin.c @@ -24,6 +24,8 @@ #include +#include "apdu.h" +#include "os.h" #include "err.h" #include "pin.h" @@ -33,29 +35,66 @@ #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 null-terminated string representing the pin to validate - * @ret true if pin is valid, false otherwise - */ -bool is_pin_valid(unsigned char *pin) { +bool is_pin_valid(pin_t* pin_ctx) { // PIN_LENGTH is the only length accepted - size_t length = strnlen((const char *)pin, PIN_LENGTH + 1); - if (length != PIN_LENGTH) { + size_t length = + strnlen((const char*)pin_ctx->pin_buffer.payload, MAX_PIN_LENGTH + 1); + if (length != MAX_PIN_LENGTH) { return false; } // Check if PIN is alphanumeric bool hasAlpha = false; - for (int i = 0; i < PIN_LENGTH; i++) { - if (!IS_ALPHANUM(pin[i])) { + for (int i = 0; i < MAX_PIN_LENGTH; i++) { + if (!IS_ALPHANUM(pin_ctx->pin_buffer.payload[i])) { return false; } - if (hasAlpha || IS_ALPHA(pin[i])) { + if (hasAlpha || IS_ALPHA(pin_ctx->pin_buffer.payload[i])) { hasAlpha = true; } } return hasAlpha; +} + +void get_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer) { + memcpy(pin_buffer, pin_ctx->pin_raw, sizeof(pin_ctx->pin_raw)); +} + +void set_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer) { + memcpy(pin_ctx->pin_raw, pin_buffer, sizeof(pin_ctx->pin_raw)); +} + +void reset_pin(pin_t* pin_ctx) { + explicit_bzero(pin_ctx, sizeof(pin_t)); +} + +unsigned int pin_cmd(pin_t* pin_ctx) { + unsigned char index = APDU_AT(2); + if ((index >= 0) && (index <= MAX_PIN_LENGTH)) { + pin_ctx->pin_raw[index] = APDU_AT(3); + pin_ctx->pin_raw[index + 1] = 0; + } + + return 3; +} + +unsigned int new_pin_cmd(pin_t* pin_ctx) { +#ifndef DEBUG_BUILD + if (!is_pin_valid(pin_ctx)) { + THROW(ERR_INVALID_PIN); + } +#endif + // Set PIN + os_perso_set_pin(0, + pin_ctx->pin_buffer.payload, + strlen((const char*)pin_ctx->pin_buffer.payload)); + // 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(pin_ctx->pin_buffer.payload, + strlen((const char*)pin_ctx->pin_buffer.payload))); + return output_index; } \ No newline at end of file diff --git a/ledger/src/ui/src/pin.h b/ledger/src/ui/src/pin.h index 0f5ac3b7..fcb04990 100644 --- a/ledger/src/ui/src/pin.h +++ b/ledger/src/ui/src/pin.h @@ -27,8 +27,79 @@ #include -#define PIN_LENGTH 8 +#define MAX_PIN_LENGTH 8 +#define MIN_PIN_LENGTH 4 -bool is_pin_valid(unsigned char *pin); +// Pin context +typedef struct { + union { + struct { + unsigned char length; + unsigned char payload[MAX_PIN_LENGTH + 1]; + } pin_buffer; + unsigned char pin_raw[MAX_PIN_LENGTH + 2]; + }; +} pin_t; + +/* + * Reset the given pin context + * + * @arg[in] pin_ctx pin context + */ +void reset_pin(pin_t* pin_ctx); + +// ----------------------------------------------------------------------- +// RSK protocol implementation +// ----------------------------------------------------------------------- + +/* + * Implements RSK PIN command. + * + * Receives one byte at a time and updates the pin context, adding a null byte + * at the end. + * + * @arg[in] pin_ctx pin context + * @ret number of transmited bytes to the host + */ +unsigned int pin_cmd(pin_t* pin_ctx); + +/* + * Implements RSK NEW PIN command. + * + * Sets the device pin. + * + * @arg[in] pin_ctx pin context + * @ret number of transmited bytes to the host + */ +unsigned int new_pin_cmd(pin_t* pin_ctx); + +// ----------------------------------------------------------------------- +// Pin manipulation utilities +// ----------------------------------------------------------------------- + +/* + * 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); + +/* + * Retrieves the pin currently saved on pin_ctx to pin_buffer + * + * @arg[in] pin_ctx pin context + * @arg[out] pin_buffer output buffer where the pin should be copied + */ +void get_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer); + +/* + * Saves the pin in pin_buffer to pin_ctx. + * + * @arg[out] pin_ctx pin context + * @arg[in] pin_buffer input buffer that holds the pin + */ +void set_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer); #endif \ No newline at end of file diff --git a/ledger/src/ui/src/unlock.c b/ledger/src/ui/src/unlock.c new file mode 100644 index 00000000..e0a4b793 --- /dev/null +++ b/ledger/src/ui/src/unlock.c @@ -0,0 +1,36 @@ +/** + * The MIT License (MIT) + * + * Copyright (c) 2021 RSK Labs Ltd + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include "apdu.h" +#include "os.h" +#include "string.h" +#include "unlock.h" + +unsigned int unlock(pin_t *pin_ctx) { + unsigned char output_index = OP; + SET_APDU_AT(output_index++, + os_global_pin_check(pin_ctx->pin_raw, + strlen((const char *)pin_ctx->pin_raw))); + return output_index; +} diff --git a/ledger/src/ui/src/unlock.h b/ledger/src/ui/src/unlock.h new file mode 100644 index 00000000..b9ebffd8 --- /dev/null +++ b/ledger/src/ui/src/unlock.h @@ -0,0 +1,40 @@ +/** + * The MIT License (MIT) + * + * Copyright (c) 2021 RSK Labs Ltd + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#ifndef __UNLOCK +#define __UNLOCK + +#include "pin.h" + +/* + * Implements RSK UNLOCK command. + * + * Unlocks the device. + * + * @arg[in] pin_ctx pin context + * @ret number of transmited bytes to the host + */ +unsigned int unlock(pin_t *pin_ctx); + +#endif diff --git a/ledger/src/ui/test/mock/apdu.h b/ledger/src/ui/test/mock/apdu.h new file mode 120000 index 00000000..946c51db --- /dev/null +++ b/ledger/src/ui/test/mock/apdu.h @@ -0,0 +1 @@ +../../../common/src/apdu.h \ No newline at end of file diff --git a/ledger/src/ui/test/mock/os.c b/ledger/src/ui/test/mock/os.c new file mode 100644 index 00000000..68419ada --- /dev/null +++ b/ledger/src/ui/test/mock/os.c @@ -0,0 +1,60 @@ +/** + * The MIT License (MIT) + * + * Copyright (c) 2021 RSK Labs Ltd + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include "os.h" +#include "string.h" + +/** + * Mocks pin currently loaded to device + */ +unsigned char current_pin[10]; + +/** + * APDU buffer + */ +unsigned char G_io_apdu_buffer[IO_APDU_BUFFER_SIZE]; + +unsigned int os_global_pin_check(unsigned char *pin_buffer, + unsigned char pin_length) { + return !strncmp( + (const char *)pin_buffer, (const char *)current_pin, pin_length); +} + +void explicit_bzero(void *s, size_t len) { + memset(s, '\0', len); +} + +void os_perso_set_pin(unsigned int identity, + unsigned char *pin, + unsigned int length) { + // Do nothing +} + +void os_global_pin_invalidate(void) { + // Do nothing +} + +void mock_set_pin(unsigned char *pin, size_t n) { + memcpy(current_pin, pin, n); +} \ No newline at end of file diff --git a/ledger/src/ui/test/mock/os.h b/ledger/src/ui/test/mock/os.h new file mode 100644 index 00000000..deb74c04 --- /dev/null +++ b/ledger/src/ui/test/mock/os.h @@ -0,0 +1,53 @@ +/** + * The MIT License (MIT) + * + * Copyright (c) 2021 RSK Labs Ltd + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include + +/** + * Utility macros + */ +#define UNUSED(x) (void)x +#define THROW(e) return e + +/** + * Mock APDU buffer + */ +#define IO_APDU_BUFFER_SIZE 85 +extern unsigned char G_io_apdu_buffer[IO_APDU_BUFFER_SIZE]; + +/** + * Mock calls for os API + */ +unsigned int os_global_pin_check(unsigned char *pin_buffer, + unsigned char pin_length); +void os_perso_set_pin(unsigned int identity, + unsigned char *pin, + unsigned int length); +void os_global_pin_invalidate(void); + +/** + * Other mocks + */ +void explicit_bzero(void *s, size_t len); +void mock_set_pin(unsigned char *pin, size_t n); diff --git a/ledger/src/ui/test/pin/Makefile b/ledger/src/ui/test/pin/Makefile index 3906a2aa..864df04c 100644 --- a/ledger/src/ui/test/pin/Makefile +++ b/ledger/src/ui/test/pin/Makefile @@ -21,10 +21,11 @@ # SOFTWARE. SRCDIR = ../../src -CFLAGS = -I $(SRCDIR) -I ./ +MOCKDIR = ../mock +CFLAGS = -I $(SRCDIR) -I $(MOCKDIR) -I ./ PROG = test.out -OBJS = pin.o test_pin.o +OBJS = os.o pin.o test_pin.o all: $(PROG) @@ -37,6 +38,9 @@ test_pin.o: test_pin.c pin.o: $(SRCDIR)/pin.c $(CC) $(CFLAGS) -c -o $@ $^ +os.o: $(MOCKDIR)/os.c + $(CC) $(CFLAGS) -c -o $@ $^ + .PHONY: clean test clean: diff --git a/ledger/src/ui/test/pin/test_pin.c b/ledger/src/ui/test/pin/test_pin.c index de2e2c85..d1f38b58 100644 --- a/ledger/src/ui/test/pin/test_pin.c +++ b/ledger/src/ui/test/pin/test_pin.c @@ -29,48 +29,83 @@ #include "pin.h" +void set_payload(pin_t *pin_ctx, unsigned char *payload, size_t n) { + memcpy(pin_ctx->pin_buffer.payload, payload, n); +} + void test_ok() { printf("Test OK...\n"); + pin_t pin_ctx; + reset_pin(&pin_ctx); - assert(is_pin_valid((unsigned char*)"abcdefgh")); - assert(is_pin_valid((unsigned char*)"8b23ef1s")); - assert(is_pin_valid((unsigned char*)"MN22P3S9")); - assert(is_pin_valid((unsigned char*)"MN22P3S9")); + set_payload(&pin_ctx, (unsigned char *)"abcdefgh", strlen("abcdefgh")); + assert(is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"8b23ef1s", strlen("8b23ef1s")); + assert(is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"MN22P3S9", strlen("MN22P3S9")); + assert(is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"MN22P3S9", strlen("MN22P3S9")); + assert(is_pin_valid(&pin_ctx)); } void test_numeric_pin() { printf("Test pin with only numbers...\n"); + pin_t pin_ctx; + reset_pin(&pin_ctx); - assert(!is_pin_valid((unsigned char*)"1234")); - assert(!is_pin_valid((unsigned char*)"123456")); - assert(!is_pin_valid((unsigned char*)"12345678")); - assert(!is_pin_valid((unsigned char*)"1234567890")); + set_payload(&pin_ctx, (unsigned char *)"1234", strlen("1234")); + assert(!is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"123456", strlen("123456")); + assert(!is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"12345678", strlen("12345678")); + assert(!is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"1234567890", strlen("1234567890")); + assert(!is_pin_valid(&pin_ctx)); } void test_pin_too_long() { printf("Test pin buffer too long...\n"); + pin_t pin_ctx; + reset_pin(&pin_ctx); - assert(!is_pin_valid((unsigned char*)"abcdefghi")); - assert(!is_pin_valid((unsigned char*)"8b23ef1s85")); - assert(!is_pin_valid((unsigned char*)"MN22P3S9P20")); - assert(!is_pin_valid((unsigned char*)"MNOPQRSTQDAS")); + set_payload(&pin_ctx, (unsigned char *)"abcdefghi", strlen("abcdefghi")); + assert(!is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"8b23ef1s85", strlen("8b23ef1s85")); + assert(!is_pin_valid(&pin_ctx)); + set_payload( + &pin_ctx, (unsigned char *)"MN22P3S9P20", strlen("MN22P3S9P20")); + assert(!is_pin_valid(&pin_ctx)); + set_payload( + &pin_ctx, (unsigned char *)"MNOPQRSTQDAS", strlen("MNOPQRSTQDAS")); + assert(!is_pin_valid(&pin_ctx)); } void test_pin_too_short() { printf("Test pin buffer too short...\n"); + pin_t pin_ctx; + reset_pin(&pin_ctx); - assert(!is_pin_valid((unsigned char*)"abcdefg")); - assert(!is_pin_valid((unsigned char*)"8b23ef")); - assert(!is_pin_valid((unsigned char*)"MN22P")); - assert(!is_pin_valid((unsigned char*)"MNOP")); + set_payload(&pin_ctx, (unsigned char *)"abcdefg", strlen("abcdefg")); + assert(!is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"8b23ef", strlen("8b23ef")); + assert(!is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"MN22P", strlen("MN22P")); + assert(!is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"MNOP", strlen("MNOP")); + assert(!is_pin_valid(&pin_ctx)); } void test_pin_non_alpha() { printf("Test pin non alpha chars...\n"); + pin_t pin_ctx; + reset_pin(&pin_ctx); - assert(!is_pin_valid((unsigned char*)"a1-@.;")); - assert(!is_pin_valid((unsigned char*)"!@#$^&*")); - assert(!is_pin_valid((unsigned char*)"(),./;']")); + set_payload(&pin_ctx, (unsigned char *)"a1-@.;", strlen("a1-@.;")); + assert(!is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"!@#$^&*", strlen("!@#$^&*")); + assert(!is_pin_valid(&pin_ctx)); + set_payload(&pin_ctx, (unsigned char *)"(),./;']", strlen("(),./;']")); + assert(!is_pin_valid(&pin_ctx)); } int main() { diff --git a/ledger/src/ui/test/unlock/Makefile b/ledger/src/ui/test/unlock/Makefile new file mode 100644 index 00000000..e0024619 --- /dev/null +++ b/ledger/src/ui/test/unlock/Makefile @@ -0,0 +1,53 @@ +# The MIT License (MIT) +# +# Copyright (c) 2021 RSK Labs Ltd +# +# Permission is hereby granted, free of charge, to any person obtaining a copy of +# this software and associated documentation files (the "Software"), to deal in +# the Software without restriction, including without limitation the rights to +# use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies +# of the Software, and to permit persons to whom the Software is furnished to do +# so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. + +SRCDIR = ../../src +MOCKDIR = ../mock +CFLAGS = -I $(SRCDIR) -I $(MOCKDIR) + +PROG = test.out +OBJS = os.o pin.o unlock.o test_unlock.o + +all: $(PROG) + +$(PROG): $(OBJS) + $(CC) -o $@ $^ + +test_unlock.o: test_unlock.c + $(CC) $(CFLAGS) -c -o $@ $^ + +unlock.o: $(SRCDIR)/unlock.c + $(CC) $(CFLAGS) -c -o $@ $^ + +pin.o: $(SRCDIR)/pin.c + $(CC) $(CFLAGS) -c -o $@ $^ + +os.o: $(MOCKDIR)/os.c + $(CC) $(CFLAGS) -c -o $@ $^ + +.PHONY: clean test + +clean: + rm -f $(PROG) ./*.o + +test: all + ./$(PROG) diff --git a/ledger/src/ui/test/unlock/test_unlock.c b/ledger/src/ui/test/unlock/test_unlock.c new file mode 100644 index 00000000..2b0e52df --- /dev/null +++ b/ledger/src/ui/test/unlock/test_unlock.c @@ -0,0 +1,66 @@ +/** + * The MIT License (MIT) + * + * Copyright (c) 2021 RSK Labs Ltd + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include +#include +#include +#include + +#include "apdu.h" +#include "os.h" +#include "unlock.h" + +void test_ok() { + printf("Test OK...\n"); + pin_t pin_ctx; + reset_pin(&pin_ctx); + set_pin_ctx(&pin_ctx, (unsigned char *)"1234567a\0\0"); + + unsigned int tx = unlock(&pin_ctx); + + assert(tx == 3); + assert(APDU_OP() == 1); +} + +void test_wrong_pin() { + printf("Test wrong pin...\n"); + pin_t pin_ctx; + reset_pin(&pin_ctx); + set_pin_ctx(&pin_ctx, (unsigned char *)"wrong-pin\0"); + + unsigned int tx = unlock(&pin_ctx); + + assert(tx == 3); + assert(APDU_OP() == 0); +} + +int main() { + // Set device pin + mock_set_pin((unsigned char *)"1234567a", strlen("1234567a")); + + test_ok(); + test_wrong_pin(); + + return 0; +} From 2c30729fd45b8c077eb04d41345af73a086ea4ea Mon Sep 17 00:00:00 2001 From: italo sampaio Date: Tue, 25 Oct 2022 16:36:59 -0300 Subject: [PATCH 2/3] Changes after code review - Changed pin context to hold just a pointer to the pin buffer - Changed function names to more declarative versions - Removed unused pin length macros - Added missing comments - Refactored pin and unlock unit tests for clarity --- ledger/src/ui/src/bolos_ux.c | 39 ++++------ ledger/src/ui/src/bolos_ux.h | 5 +- ledger/src/ui/src/pin.c | 76 +++++++++++++------ ledger/src/ui/src/pin.h | 46 +++++------- ledger/src/ui/src/unlock.c | 12 ++- ledger/src/ui/test/pin/test_pin.c | 98 +++++++++++-------------- ledger/src/ui/test/unlock/test_unlock.c | 10 +-- 7 files changed, 143 insertions(+), 143 deletions(-) diff --git a/ledger/src/ui/src/bolos_ux.c b/ledger/src/ui/src/bolos_ux.c index 29803d26..81bb32dc 100644 --- a/ledger/src/ui/src/bolos_ux.c +++ b/ledger/src/ui/src/bolos_ux.c @@ -58,7 +58,7 @@ const unsigned char N_onboarded_ui[1] = {0}; // PIN buffer used for authenticated operations -unsigned char G_pin_buffer[MAX_PIN_LENGTH + 2]; +unsigned char G_pin_buffer[PIN_LENGTH + 2]; #ifdef OS_IO_SEPROXYHAL @@ -265,7 +265,7 @@ static void reset_if_starting(unsigned char cmd) { G_bolos_ux_context.words_buffer_length = 0; reset_attestation(&attestation_ctx); reset_signer_authorization(&sigaut_ctx); - reset_pin(&pin_ctx); + reset_pin_ctx(&pin_ctx); } } @@ -322,9 +322,8 @@ static void sample_main(void) { break; case RSK_PIN_CMD: // Send pin_buffer reset_if_starting(RSK_META_CMD_UIOP); - tx = pin_cmd(&pin_ctx); - // Save pin for authenticated operations - get_pin_ctx(&pin_ctx, G_pin_buffer); + init_pin_ctx(&pin_ctx, G_pin_buffer); + tx = update_pin_buffer(&pin_ctx); THROW(APDU_OK); break; case RSK_IS_ONBOARD: // Wheter it's onboarded or not @@ -346,7 +345,7 @@ static void sample_main(void) { nvm_write( (void *)PIC(N_onboarded_ui), (void *)&aux, sizeof(aux)); - set_pin_ctx(&pin_ctx, G_pin_buffer); + init_pin_ctx(&pin_ctx, G_pin_buffer); #ifndef DEBUG_BUILD if (!is_pin_valid(&pin_ctx)) { THROW(ERR_INVALID_PIN); @@ -391,19 +390,17 @@ static void sample_main(void) { explicit_bzero(G_bolos_ux_context.words_buffer, sizeof(G_bolos_ux_context.words_buffer)); // Set PIN - os_perso_set_pin( - 0, - pin_ctx.pin_buffer.payload, - strlen((const char *)pin_ctx.pin_buffer.payload)); + os_perso_set_pin(0, + PIN_CTX_PAYLOAD(&pin_ctx), + PIN_CTX_PAYLOAD_LEN(&pin_ctx)); // Finalize onboarding os_perso_finalize(); os_global_pin_invalidate(); SET_APDU_AT(1, 2); SET_APDU_AT( 2, - os_global_pin_check( - pin_ctx.pin_buffer.payload, - strlen((const char *)pin_ctx.pin_buffer.payload))); + os_global_pin_check(PIN_CTX_PAYLOAD(&pin_ctx), + PIN_CTX_PAYLOAD_LEN(&pin_ctx))); // Clear pin buffer explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); // Turn the onboarding flag on to mark onboarding @@ -417,14 +414,13 @@ static void sample_main(void) { break; case RSK_NEWPIN: reset_if_starting(RSK_META_CMD_UIOP); - - set_pin_ctx(&pin_ctx, G_pin_buffer); + init_pin_ctx(&pin_ctx, G_pin_buffer); #ifndef DEBUG_BUILD if (!is_pin_valid(&pin_ctx)) { THROW(ERR_INVALID_PIN); } #endif - tx = new_pin_cmd(&pin_ctx); + tx = set_device_pin(&pin_ctx); // Clear pin buffer explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); THROW(APDU_OK); @@ -458,10 +454,7 @@ static void sample_main(void) { break; case RSK_UNLOCK_CMD: // Unlock reset_if_starting(RSK_META_CMD_UIOP); - // RSK_UNLOCK_CMD does not send the prepended length, - // so we kept this call backwards compatible, using - // pin_ctx.pin_raw instead of pin_ctx.pin_buffer.payload - set_pin_ctx(&pin_ctx, G_pin_buffer); + init_pin_ctx(&pin_ctx, G_pin_buffer); tx = unlock(&pin_ctx); // The pin value will also be used in // BOLOS_UX_CONSENT_APP_ADD command, so we can't wipe the @@ -645,9 +638,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 - set_pin_ctx(&pin_ctx, G_pin_buffer); - os_global_pin_check(pin_ctx.pin_raw, - strlen((const char *)pin_ctx.pin_raw)); + init_pin_ctx(&pin_ctx, G_pin_buffer); + os_global_pin_check(pin_ctx.pin_buffer, + strlen((const char *)pin_ctx.pin_buffer)); G_bolos_ux_context.exit_code = BOLOS_UX_OK; explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); break; diff --git a/ledger/src/ui/src/bolos_ux.h b/ledger/src/ui/src/bolos_ux.h index 172c50c1..e7d42a97 100644 --- a/ledger/src/ui/src/bolos_ux.h +++ b/ledger/src/ui/src/bolos_ux.h @@ -164,10 +164,7 @@ typedef struct bolos_ux_context { }; }; -#define MAX_PIN_LENGTH 8 -#define MIN_PIN_LENGTH 4 - char pin_buffer[MAX_PIN_LENGTH + - 1]; // length prepended for custom pin length + char pin_buffer[PIN_LENGTH + 1]; // length prepended for custom pin length // filled up during os_ux syscall when called by user or bolos. bolos_ux_params_t parameters; diff --git a/ledger/src/ui/src/pin.c b/ledger/src/ui/src/pin.c index a05f6020..d7d6aa51 100644 --- a/ledger/src/ui/src/pin.c +++ b/ledger/src/ui/src/pin.c @@ -35,20 +35,27 @@ #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 size_t length = - strnlen((const char*)pin_ctx->pin_buffer.payload, MAX_PIN_LENGTH + 1); - if (length != MAX_PIN_LENGTH) { + strnlen((const char*)PIN_CTX_PAYLOAD(pin_ctx), PIN_LENGTH + 1); + if (length != PIN_LENGTH) { return false; } // Check if PIN is alphanumeric bool hasAlpha = false; - for (int i = 0; i < MAX_PIN_LENGTH; i++) { - if (!IS_ALPHANUM(pin_ctx->pin_buffer.payload[i])) { + for (int i = 0; i < PIN_LENGTH; i++) { + if (!IS_ALPHANUM(PIN_CTX_PAYLOAD(pin_ctx)[i])) { return false; } - if (hasAlpha || IS_ALPHA(pin_ctx->pin_buffer.payload[i])) { + if (hasAlpha || IS_ALPHA(PIN_CTX_PAYLOAD(pin_ctx)[i])) { hasAlpha = true; } } @@ -56,45 +63,66 @@ bool is_pin_valid(pin_t* pin_ctx) { return hasAlpha; } -void get_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer) { - memcpy(pin_buffer, pin_ctx->pin_raw, sizeof(pin_ctx->pin_raw)); -} - -void set_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer) { - memcpy(pin_ctx->pin_raw, pin_buffer, sizeof(pin_ctx->pin_raw)); +/* + * 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; } -void reset_pin(pin_t* pin_ctx) { +/* + * 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)); } -unsigned int pin_cmd(pin_t* pin_ctx) { +/* + * Implements RSK PIN command. + * + * Receives one byte at a time and updates the pin context, adding a null byte + * at the end. + * + * @arg[in] pin_ctx pin context + * @ret number of transmited bytes to the host + */ +unsigned int update_pin_buffer(pin_t* pin_ctx) { unsigned char index = APDU_AT(2); - if ((index >= 0) && (index <= MAX_PIN_LENGTH)) { - pin_ctx->pin_raw[index] = APDU_AT(3); - pin_ctx->pin_raw[index + 1] = 0; + if ((index >= 0) && (index <= PIN_LENGTH)) { + pin_ctx->pin_buffer[index] = APDU_AT(3); + pin_ctx->pin_buffer[index + 1] = 0; } return 3; } -unsigned int new_pin_cmd(pin_t* pin_ctx) { +/* + * Implements RSK NEW PIN command. + * + * Sets the device pin. + * + * @arg[in] pin_ctx pin context + * @ret number of transmited bytes to the host + */ +unsigned int set_device_pin(pin_t* pin_ctx) { #ifndef DEBUG_BUILD if (!is_pin_valid(pin_ctx)) { THROW(ERR_INVALID_PIN); } #endif // Set PIN - os_perso_set_pin(0, - pin_ctx->pin_buffer.payload, - strlen((const char*)pin_ctx->pin_buffer.payload)); + os_perso_set_pin(0, PIN_CTX_PAYLOAD(pin_ctx), PIN_CTX_PAYLOAD_LEN(pin_ctx)); // 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(pin_ctx->pin_buffer.payload, - strlen((const char*)pin_ctx->pin_buffer.payload))); + SET_APDU_AT(output_index++, + os_global_pin_check(PIN_CTX_PAYLOAD(pin_ctx), + PIN_CTX_PAYLOAD_LEN(pin_ctx))); return output_index; } \ No newline at end of file diff --git a/ledger/src/ui/src/pin.h b/ledger/src/ui/src/pin.h index fcb04990..8beacb2d 100644 --- a/ledger/src/ui/src/pin.h +++ b/ledger/src/ui/src/pin.h @@ -27,26 +27,32 @@ #include -#define MAX_PIN_LENGTH 8 -#define MIN_PIN_LENGTH 4 +#define PIN_LENGTH 8 // Pin context typedef struct { - union { - struct { - unsigned char length; - unsigned char payload[MAX_PIN_LENGTH + 1]; - } pin_buffer; - unsigned char pin_raw[MAX_PIN_LENGTH + 2]; - }; + unsigned char* pin_buffer; } pin_t; +// Helper macros for pin context manipulation +#define PIN_CTX_PAYLOAD(pin_ctx) ((unsigned char*)((pin_ctx)->pin_buffer + 1)) +#define PIN_CTX_PAYLOAD_LEN(pin_ctx) \ + strlen((const char*)PIN_CTX_PAYLOAD(pin_ctx)) + /* * Reset the given pin context * * @arg[in] pin_ctx pin context */ -void reset_pin(pin_t* pin_ctx); +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 @@ -61,7 +67,7 @@ void reset_pin(pin_t* pin_ctx); * @arg[in] pin_ctx pin context * @ret number of transmited bytes to the host */ -unsigned int pin_cmd(pin_t* pin_ctx); +unsigned int update_pin_buffer(pin_t* pin_ctx); /* * Implements RSK NEW PIN command. @@ -71,7 +77,7 @@ unsigned int pin_cmd(pin_t* pin_ctx); * @arg[in] pin_ctx pin context * @ret number of transmited bytes to the host */ -unsigned int new_pin_cmd(pin_t* pin_ctx); +unsigned int set_device_pin(pin_t* pin_ctx); // ----------------------------------------------------------------------- // Pin manipulation utilities @@ -86,20 +92,4 @@ unsigned int new_pin_cmd(pin_t* pin_ctx); */ bool is_pin_valid(pin_t* pin_ctx); -/* - * Retrieves the pin currently saved on pin_ctx to pin_buffer - * - * @arg[in] pin_ctx pin context - * @arg[out] pin_buffer output buffer where the pin should be copied - */ -void get_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer); - -/* - * Saves the pin in pin_buffer to pin_ctx. - * - * @arg[out] pin_ctx pin context - * @arg[in] pin_buffer input buffer that holds the pin - */ -void set_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer); - #endif \ No newline at end of file diff --git a/ledger/src/ui/src/unlock.c b/ledger/src/ui/src/unlock.c index e0a4b793..34aa31d5 100644 --- a/ledger/src/ui/src/unlock.c +++ b/ledger/src/ui/src/unlock.c @@ -27,10 +27,18 @@ #include "string.h" #include "unlock.h" +/* + * Implements RSK UNLOCK command. + * + * 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 char output_index = OP; SET_APDU_AT(output_index++, - os_global_pin_check(pin_ctx->pin_raw, - strlen((const char *)pin_ctx->pin_raw))); + os_global_pin_check(pin_ctx->pin_buffer, + strlen((const char *)pin_ctx->pin_buffer))); return output_index; } diff --git a/ledger/src/ui/test/pin/test_pin.c b/ledger/src/ui/test/pin/test_pin.c index d1f38b58..110d3e0f 100644 --- a/ledger/src/ui/test/pin/test_pin.c +++ b/ledger/src/ui/test/pin/test_pin.c @@ -29,87 +29,73 @@ #include "pin.h" +#define IS_VALID true +#define NOT_VALID false + void set_payload(pin_t *pin_ctx, unsigned char *payload, size_t n) { - memcpy(pin_ctx->pin_buffer.payload, payload, n); + memcpy(PIN_CTX_PAYLOAD(pin_ctx), payload, n); +} + +void assert_pin(char *pin, bool expected) { + unsigned char pin_length = (char)strlen(pin); + unsigned char *pin_buffer = malloc(pin_length + 2); + pin_buffer[0] = pin_length; + strcpy((char *)pin_buffer + 1, pin); + + pin_t pin_ctx; + init_pin_ctx(&pin_ctx, pin_buffer); + assert(is_pin_valid(&pin_ctx) == expected); + free(pin_buffer); } void test_ok() { printf("Test OK...\n"); - pin_t pin_ctx; - reset_pin(&pin_ctx); - - set_payload(&pin_ctx, (unsigned char *)"abcdefgh", strlen("abcdefgh")); - assert(is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"8b23ef1s", strlen("8b23ef1s")); - assert(is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"MN22P3S9", strlen("MN22P3S9")); - assert(is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"MN22P3S9", strlen("MN22P3S9")); - assert(is_pin_valid(&pin_ctx)); + + assert_pin("abcdefgh", IS_VALID); + assert_pin("8b23ef1s", IS_VALID); + assert_pin("MN22P3S9", IS_VALID); + assert_pin("MN22p3s9", IS_VALID); } void test_numeric_pin() { printf("Test pin with only numbers...\n"); - pin_t pin_ctx; - reset_pin(&pin_ctx); - - set_payload(&pin_ctx, (unsigned char *)"1234", strlen("1234")); - assert(!is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"123456", strlen("123456")); - assert(!is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"12345678", strlen("12345678")); - assert(!is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"1234567890", strlen("1234567890")); - assert(!is_pin_valid(&pin_ctx)); + + assert_pin("1234", NOT_VALID); + assert_pin("123456", NOT_VALID); + assert_pin("12345678", NOT_VALID); + assert_pin("1234567890", NOT_VALID); } void test_pin_too_long() { printf("Test pin buffer too long...\n"); - pin_t pin_ctx; - reset_pin(&pin_ctx); - - set_payload(&pin_ctx, (unsigned char *)"abcdefghi", strlen("abcdefghi")); - assert(!is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"8b23ef1s85", strlen("8b23ef1s85")); - assert(!is_pin_valid(&pin_ctx)); - set_payload( - &pin_ctx, (unsigned char *)"MN22P3S9P20", strlen("MN22P3S9P20")); - assert(!is_pin_valid(&pin_ctx)); - set_payload( - &pin_ctx, (unsigned char *)"MNOPQRSTQDAS", strlen("MNOPQRSTQDAS")); - assert(!is_pin_valid(&pin_ctx)); + + assert_pin("abcdefghi", NOT_VALID); + assert_pin("8b23ef1s85", NOT_VALID); + assert_pin("MN22P3S9P20", NOT_VALID); + assert_pin("MNOPQRSTQDAS", NOT_VALID); } void test_pin_too_short() { printf("Test pin buffer too short...\n"); - pin_t pin_ctx; - reset_pin(&pin_ctx); - - set_payload(&pin_ctx, (unsigned char *)"abcdefg", strlen("abcdefg")); - assert(!is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"8b23ef", strlen("8b23ef")); - assert(!is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"MN22P", strlen("MN22P")); - assert(!is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"MNOP", strlen("MNOP")); - assert(!is_pin_valid(&pin_ctx)); + + assert_pin("abcdefg", NOT_VALID); + assert_pin("8b23ef", NOT_VALID); + assert_pin("MN22P", NOT_VALID); + assert_pin("MNOP", NOT_VALID); } void test_pin_non_alpha() { printf("Test pin non alpha chars...\n"); - pin_t pin_ctx; - reset_pin(&pin_ctx); - - set_payload(&pin_ctx, (unsigned char *)"a1-@.;", strlen("a1-@.;")); - assert(!is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"!@#$^&*", strlen("!@#$^&*")); - assert(!is_pin_valid(&pin_ctx)); - set_payload(&pin_ctx, (unsigned char *)"(),./;']", strlen("(),./;']")); - assert(!is_pin_valid(&pin_ctx)); + + assert_pin("a1-@.;", NOT_VALID); + assert_pin("!@#$^&*", NOT_VALID); + assert_pin("(),./;']", NOT_VALID); + assert_pin("abcdefg", NOT_VALID); } int main() { test_ok(); + test_numeric_pin(); test_pin_too_long(); test_pin_too_short(); diff --git a/ledger/src/ui/test/unlock/test_unlock.c b/ledger/src/ui/test/unlock/test_unlock.c index 2b0e52df..22cb14eb 100644 --- a/ledger/src/ui/test/unlock/test_unlock.c +++ b/ledger/src/ui/test/unlock/test_unlock.c @@ -33,24 +33,22 @@ void test_ok() { printf("Test OK...\n"); + pin_t pin_ctx; - reset_pin(&pin_ctx); - set_pin_ctx(&pin_ctx, (unsigned char *)"1234567a\0\0"); + init_pin_ctx(&pin_ctx, (unsigned char *)"1234567a\0\0"); unsigned int tx = unlock(&pin_ctx); - assert(tx == 3); assert(APDU_OP() == 1); } void test_wrong_pin() { printf("Test wrong pin...\n"); + pin_t pin_ctx; - reset_pin(&pin_ctx); - set_pin_ctx(&pin_ctx, (unsigned char *)"wrong-pin\0"); + init_pin_ctx(&pin_ctx, (unsigned char *)"wrong-pin\0"); unsigned int tx = unlock(&pin_ctx); - assert(tx == 3); assert(APDU_OP() == 0); } From f7e491ffabfa06d611c12465c0573da201620b5a Mon Sep 17 00:00:00 2001 From: italo sampaio Date: Thu, 27 Oct 2022 15:35:56 -0300 Subject: [PATCH 3/3] More changes after code review --- ledger/src/ui/src/bolos_ux.c | 22 ++++++++----------- ledger/src/ui/src/bolos_ux.h | 2 -- ledger/src/ui/src/pin.c | 31 +++++++++++++++----------- ledger/src/ui/src/pin.h | 12 ++++++----- ledger/src/ui/src/unlock.c | 6 +++++- ledger/src/ui/src/unlock.h | 3 ++- ledger/src/ui/test/mock/os.c | 2 ++ ledger/src/ui/test/pin/test_pin.c | 36 +++++++++++++++---------------- 8 files changed, 62 insertions(+), 52 deletions(-) diff --git a/ledger/src/ui/src/bolos_ux.c b/ledger/src/ui/src/bolos_ux.c index 81bb32dc..cce8554f 100644 --- a/ledger/src/ui/src/bolos_ux.c +++ b/ledger/src/ui/src/bolos_ux.c @@ -58,7 +58,7 @@ const unsigned char N_onboarded_ui[1] = {0}; // PIN buffer used for authenticated operations -unsigned char G_pin_buffer[PIN_LENGTH + 2]; +unsigned char G_pin_buffer[PIN_BUFFER_LENGTH]; #ifdef OS_IO_SEPROXYHAL @@ -258,8 +258,6 @@ static void reset_if_starting(unsigned char cmd) { curr_cmd = cmd; explicit_bzero(G_bolos_ux_context.words_buffer, sizeof(G_bolos_ux_context.words_buffer)); - explicit_bzero(G_bolos_ux_context.pin_buffer, - sizeof(G_bolos_ux_context.pin_buffer)); explicit_bzero(G_bolos_ux_context.string_buffer, sizeof(G_bolos_ux_context.string_buffer)); G_bolos_ux_context.words_buffer_length = 0; @@ -323,7 +321,7 @@ static void sample_main(void) { 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(&pin_ctx); + tx = update_pin_buffer(rx, &pin_ctx); THROW(APDU_OK); break; case RSK_IS_ONBOARD: // Wheter it's onboarded or not @@ -390,17 +388,15 @@ static void sample_main(void) { explicit_bzero(G_bolos_ux_context.words_buffer, sizeof(G_bolos_ux_context.words_buffer)); // Set PIN - os_perso_set_pin(0, - PIN_CTX_PAYLOAD(&pin_ctx), - PIN_CTX_PAYLOAD_LEN(&pin_ctx)); + os_perso_set_pin( + 0, GET_PIN(&pin_ctx), GET_PIN_LENGTH(&pin_ctx)); // Finalize onboarding os_perso_finalize(); os_global_pin_invalidate(); SET_APDU_AT(1, 2); - SET_APDU_AT( - 2, - os_global_pin_check(PIN_CTX_PAYLOAD(&pin_ctx), - PIN_CTX_PAYLOAD_LEN(&pin_ctx))); + SET_APDU_AT(2, + os_global_pin_check(GET_PIN(&pin_ctx), + GET_PIN_LENGTH(&pin_ctx))); // Clear pin buffer explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); // Turn the onboarding flag on to mark onboarding @@ -420,7 +416,7 @@ static void sample_main(void) { THROW(ERR_INVALID_PIN); } #endif - tx = set_device_pin(&pin_ctx); + tx = set_device_pin(rx, &pin_ctx); // Clear pin buffer explicit_bzero(G_pin_buffer, sizeof(G_pin_buffer)); THROW(APDU_OK); @@ -455,7 +451,7 @@ static void sample_main(void) { 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(rx, &pin_ctx); // The pin value will also be used in // BOLOS_UX_CONSENT_APP_ADD command, so we can't wipe the // pin buffer here diff --git a/ledger/src/ui/src/bolos_ux.h b/ledger/src/ui/src/bolos_ux.h index e7d42a97..b81f7159 100644 --- a/ledger/src/ui/src/bolos_ux.h +++ b/ledger/src/ui/src/bolos_ux.h @@ -164,8 +164,6 @@ typedef struct bolos_ux_context { }; }; - char pin_buffer[PIN_LENGTH + 1]; // length prepended for custom pin length - // filled up during os_ux syscall when called by user or bolos. bolos_ux_params_t parameters; diff --git a/ledger/src/ui/src/pin.c b/ledger/src/ui/src/pin.c index d7d6aa51..cf8f6d8e 100644 --- a/ledger/src/ui/src/pin.c +++ b/ledger/src/ui/src/pin.c @@ -44,18 +44,16 @@ */ bool is_pin_valid(pin_t* pin_ctx) { // PIN_LENGTH is the only length accepted - size_t length = - strnlen((const char*)PIN_CTX_PAYLOAD(pin_ctx), PIN_LENGTH + 1); - if (length != PIN_LENGTH) { + 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(PIN_CTX_PAYLOAD(pin_ctx)[i])) { + if (!IS_ALPHANUM(GET_PIN(pin_ctx)[i])) { return false; } - if (hasAlpha || IS_ALPHA(PIN_CTX_PAYLOAD(pin_ctx)[i])) { + if (hasAlpha || IS_ALPHA(GET_PIN(pin_ctx)[i])) { hasAlpha = true; } } @@ -88,13 +86,19 @@ void reset_pin_ctx(pin_t* pin_ctx) { * Receives one byte at a time and updates the pin context, adding a null byte * 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(pin_t* pin_ctx) { - unsigned char index = APDU_AT(2); +unsigned int update_pin_buffer(volatile unsigned int rx, pin_t* pin_ctx) { + // 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(3); + pin_ctx->pin_buffer[index] = APDU_AT(DATA); pin_ctx->pin_buffer[index + 1] = 0; } @@ -106,23 +110,26 @@ unsigned int update_pin_buffer(pin_t* pin_ctx) { * * Sets the device pin. * + * @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 set_device_pin(pin_t* pin_ctx) { +unsigned int set_device_pin(volatile unsigned int rx, pin_t* pin_ctx) { + // NEW_PIN command does not use any input from apdu buffer + UNUSED(rx); + #ifndef DEBUG_BUILD if (!is_pin_valid(pin_ctx)) { THROW(ERR_INVALID_PIN); } #endif // Set PIN - os_perso_set_pin(0, PIN_CTX_PAYLOAD(pin_ctx), PIN_CTX_PAYLOAD_LEN(pin_ctx)); + os_perso_set_pin(0, GET_PIN(pin_ctx), GET_PIN_LENGTH(pin_ctx)); // 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(PIN_CTX_PAYLOAD(pin_ctx), - PIN_CTX_PAYLOAD_LEN(pin_ctx))); + os_global_pin_check(GET_PIN(pin_ctx), GET_PIN_LENGTH(pin_ctx))); return output_index; } \ No newline at end of file diff --git a/ledger/src/ui/src/pin.h b/ledger/src/ui/src/pin.h index 8beacb2d..0eb270bd 100644 --- a/ledger/src/ui/src/pin.h +++ b/ledger/src/ui/src/pin.h @@ -28,6 +28,7 @@ #include #define PIN_LENGTH 8 +#define PIN_BUFFER_LENGTH (PIN_LENGTH + 2) // Pin context typedef struct { @@ -35,9 +36,8 @@ typedef struct { } pin_t; // Helper macros for pin context manipulation -#define PIN_CTX_PAYLOAD(pin_ctx) ((unsigned char*)((pin_ctx)->pin_buffer + 1)) -#define PIN_CTX_PAYLOAD_LEN(pin_ctx) \ - strlen((const char*)PIN_CTX_PAYLOAD(pin_ctx)) +#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 @@ -64,20 +64,22 @@ void init_pin_ctx(pin_t* pin_ctx, unsigned char* pin_buffer); * Receives one byte at a time and updates the pin context, adding a null byte * 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(pin_t* pin_ctx); +unsigned int update_pin_buffer(volatile unsigned int rx, pin_t* pin_ctx); /* * Implements RSK NEW PIN command. * * Sets the device pin. * + * @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 set_device_pin(pin_t* pin_ctx); +unsigned int set_device_pin(volatile unsigned int rx, pin_t* pin_ctx); // ----------------------------------------------------------------------- // Pin manipulation utilities diff --git a/ledger/src/ui/src/unlock.c b/ledger/src/ui/src/unlock.c index 34aa31d5..e8e3592f 100644 --- a/ledger/src/ui/src/unlock.c +++ b/ledger/src/ui/src/unlock.c @@ -32,10 +32,14 @@ * * Unlocks the device. * + * @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 unlock(pin_t *pin_ctx) { +unsigned int unlock(volatile unsigned int rx, pin_t *pin_ctx) { + // Unlock command does not use any input from apdu buffer + UNUSED(rx); + unsigned char output_index = OP; SET_APDU_AT(output_index++, os_global_pin_check(pin_ctx->pin_buffer, diff --git a/ledger/src/ui/src/unlock.h b/ledger/src/ui/src/unlock.h index b9ebffd8..e631d1f1 100644 --- a/ledger/src/ui/src/unlock.h +++ b/ledger/src/ui/src/unlock.h @@ -32,9 +32,10 @@ * * Unlocks the device. * + * @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 unlock(pin_t *pin_ctx); +unsigned int unlock(volatile unsigned int rx, pin_t *pin_ctx); #endif diff --git a/ledger/src/ui/test/mock/os.c b/ledger/src/ui/test/mock/os.c index 68419ada..1590a831 100644 --- a/ledger/src/ui/test/mock/os.c +++ b/ledger/src/ui/test/mock/os.c @@ -43,6 +43,8 @@ unsigned int os_global_pin_check(unsigned char *pin_buffer, void explicit_bzero(void *s, size_t len) { memset(s, '\0', len); + /* Compiler barrier. */ + asm volatile("" ::: "memory"); } void os_perso_set_pin(unsigned int identity, diff --git a/ledger/src/ui/test/pin/test_pin.c b/ledger/src/ui/test/pin/test_pin.c index 110d3e0f..d4933b0d 100644 --- a/ledger/src/ui/test/pin/test_pin.c +++ b/ledger/src/ui/test/pin/test_pin.c @@ -30,10 +30,10 @@ #include "pin.h" #define IS_VALID true -#define NOT_VALID false +#define IS_NOT_VALID false void set_payload(pin_t *pin_ctx, unsigned char *payload, size_t n) { - memcpy(PIN_CTX_PAYLOAD(pin_ctx), payload, n); + memcpy(GET_PIN(pin_ctx), payload, n); } void assert_pin(char *pin, bool expected) { @@ -60,37 +60,37 @@ void test_ok() { void test_numeric_pin() { printf("Test pin with only numbers...\n"); - assert_pin("1234", NOT_VALID); - assert_pin("123456", NOT_VALID); - assert_pin("12345678", NOT_VALID); - assert_pin("1234567890", NOT_VALID); + assert_pin("1234", IS_NOT_VALID); + assert_pin("123456", IS_NOT_VALID); + assert_pin("12345678", IS_NOT_VALID); + assert_pin("1234567890", IS_NOT_VALID); } void test_pin_too_long() { printf("Test pin buffer too long...\n"); - assert_pin("abcdefghi", NOT_VALID); - assert_pin("8b23ef1s85", NOT_VALID); - assert_pin("MN22P3S9P20", NOT_VALID); - assert_pin("MNOPQRSTQDAS", NOT_VALID); + assert_pin("abcdefghi", IS_NOT_VALID); + assert_pin("8b23ef1s85", IS_NOT_VALID); + assert_pin("MN22P3S9P20", IS_NOT_VALID); + assert_pin("MNOPQRSTQDAS", IS_NOT_VALID); } void test_pin_too_short() { printf("Test pin buffer too short...\n"); - assert_pin("abcdefg", NOT_VALID); - assert_pin("8b23ef", NOT_VALID); - assert_pin("MN22P", NOT_VALID); - assert_pin("MNOP", NOT_VALID); + assert_pin("abcdefg", IS_NOT_VALID); + assert_pin("8b23ef", IS_NOT_VALID); + assert_pin("MN22P", IS_NOT_VALID); + assert_pin("MNOP", IS_NOT_VALID); } void test_pin_non_alpha() { printf("Test pin non alpha chars...\n"); - assert_pin("a1-@.;", NOT_VALID); - assert_pin("!@#$^&*", NOT_VALID); - assert_pin("(),./;']", NOT_VALID); - assert_pin("abcdefg", NOT_VALID); + assert_pin("a1-@.;", IS_NOT_VALID); + assert_pin("!@#$^&*", IS_NOT_VALID); + assert_pin("(),./;']", IS_NOT_VALID); + assert_pin("abcdefg", IS_NOT_VALID); } int main() {