Unifying pin policy

- Added pin_policy library implementing both Ledger and SGX pin policies
- Updated SGX's trusted HAL access module to use the pin policy library for password validation
- Updated the Ledger UI to use the pin policy library for pin validation
- Added pin_policy unit tests
- Updated failing unit tests

firmware/src/common/src/pin_policy.c

@@ -0,0 +1,51 @@
+/**
+ * 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 "pin_policy.h"
+
+// Helper macros for pin validation
+#define IS_IN_RANGE(c, begin, end) (((c) >= (begin)) && ((c) <= (end)))
+#define IS_ALPHA(c) (IS_IN_RANGE(c, 'a', 'z') || IS_IN_RANGE(c, 'A', 'Z'))
+#define IS_NUM(c) IS_IN_RANGE(c, '0', '9')
+#define IS_ALPHANUM(c) (IS_ALPHA(c) || IS_NUM(c))
+
+bool pin_policy_is_valid_pin(const char* pin, size_t pin_length) {
+    // Pin should be exactly MAX_PIN_LENGTH characters long
+    if (pin_length != MAX_PIN_LENGTH) {
+        return false;
+    }
+
+    // Pin should:
+    // - Be entirely alphanumeric and;
+    // - Contain at least one alphabetic character
+    bool hasAlpha = false;
+    for (size_t i = 0; i < pin_length; i++) {
+        if (!IS_ALPHANUM(pin[i])) {
+            return false;
+        }
+        hasAlpha |= IS_ALPHA(pin[i]);
+    }
+
+    return hasAlpha;
+}

firmware/src/common/src/pin_policy.h

@@ -0,0 +1,42 @@
+/**
+ * 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 __PIN_POLICY_H
+#define __PIN_POLICY_H
+
+#include <stdlib.h>
+#include <stdbool.h>
+
+#define MAX_PIN_LENGTH 8
+
+/*
+ * Tells whether the given pin is valid under the pin policy
+ *
+ * @param pin        the buffer where the pin to validate is stored
+ * @param pin_length the pin length
+ * @returns true     if pin is valid, false otherwise
+ */
+bool pin_policy_is_valid_pin(const char* pin, size_t pin_length);
+
+#endif // __PIN_POLICY_H

firmware/src/common/test/pin_policy/Makefile

@@ -0,0 +1,39 @@
+# 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 ../common.mk
+
+PROG = test.out
+OBJS = pin_policy.o test_pin_policy.o
+
+all: $(PROG)
+
+$(PROG): $(OBJS)
+	$(CC) $(COVFLAGS) -o $@ $^
+
+.PHONY: clean test
+
+clean:
+	rm -f $(PROG) *.o $(COVFILES)
+
+test: all
+	./$(PROG)

firmware/src/common/test/pin_policy/test_pin_policy.c

@@ -0,0 +1,95 @@
+/**
+ * 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 <stdio.h>
+#include <stdlib.h>
+#include <assert.h>
+#include <string.h>
+
+#include "pin_policy.h"
+
+#define IS_VALID true
+#define IS_NOT_VALID false
+
+// Other helpers
+void assert_pin(char *pin, bool expected) {
+    size_t pin_length = strlen(pin);
+    assert(pin_policy_is_valid_pin(pin, pin_length) == expected);
+}
+
+void test_validate_ok() {
+    printf("Test validate pin OK...\n");
+
+    assert_pin("abcdefgh", IS_VALID);
+    assert_pin("8b23ef1s", IS_VALID);
+    assert_pin("MN22P3S9", IS_VALID);
+    assert_pin("MN22p3s9", IS_VALID);
+}
+
+void test_validate_numeric_pin() {
+    printf("Test validate pin with only numbers...\n");
+
+    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_validate_pin_too_long() {
+    printf("Test validate pin too long...\n");
+
+    // Long pins are accepted, but internally capped to PIN_LENGTH
+    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_validate_pin_too_short() {
+    printf("Test validate pin too short...\n");
+
+    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_validate_pin_non_alpha() {
+    printf("Test validate pin non alpha chars...\n");
+
+    assert_pin("a1-@.;", IS_NOT_VALID);
+    assert_pin("!@#$^&*", IS_NOT_VALID);
+    assert_pin("(),./;']", IS_NOT_VALID);
+    assert_pin("abcdefg", IS_NOT_VALID);
+}
+
+int main() {
+    test_validate_ok();
+    test_validate_numeric_pin();
+    test_validate_pin_too_long();
+    test_validate_pin_too_short();
+    test_validate_pin_non_alpha();
+
+    return 0;
+}

firmware/src/common/test/run-all.sh

@@ -2,7 +2,7 @@
 
 if [[ $1 == "exec" ]]; then
     BASEDIR=$(realpath $(dirname $0))
-    TESTDIRS="bigdigits_helper ints memutil"
+    TESTDIRS="bigdigits_helper ints memutil pin_policy"
     for d in $TESTDIRS; do
         echo "******************************"
         echo "Testing $d..."

firmware/src/hal/sgx/src/trusted/access.c

@@ -30,9 +30,8 @@
 #include "hal/log.h"
 
 #include "secret_store.h"
+#include "pin_policy.h"
 
-#define MIN_PASSWORD_LENGTH 4
-#define MAX_PASSWORD_LENGTH 10
 #define MAX_RETRIES 3
 
 #define SEST_PASSWORD_KEY "password"
@@ -42,7 +41,7 @@
 static bool G_wiped;
 static bool G_locked;
 static uint8_t G_available_retries;
-static char G_password[MAX_PASSWORD_LENGTH];
+static char G_password[MAX_PIN_LENGTH];
 static uint8_t G_password_length;
 static access_wiped_callback_t G_wiped_callback;
 
@@ -64,8 +63,7 @@ bool access_init(access_wiped_callback_t wiped_callback) {
     }
 
     // Make sure password is sound
-    if (G_password_length < MIN_PASSWORD_LENGTH ||
-        G_password_length > MAX_PASSWORD_LENGTH) {
+    if (!pin_policy_is_valid_pin(G_password, G_password_length)) {
         LOG("Detected invalid password\n");
         return false;
     }
@@ -153,8 +151,7 @@ bool access_set_password(char* password, uint8_t password_length) {
         return false;
     }
 
-    if (password_length < MIN_PASSWORD_LENGTH ||
-        password_length > MAX_PASSWORD_LENGTH) {
+    if (!pin_policy_is_valid_pin(password, password_length)) {
         LOG("Invalid password\n");
         return false;
     }

firmware/src/ledger/ui/src/pin.c

@@ -28,15 +28,9 @@
 #include "os.h"
 #include "ui_err.h"
 #include "pin.h"
+#include "pin_policy.h"
 
-// Helper macros for pin validation
-#define IS_IN_RANGE(c, begin, end) (((c) >= (begin)) && ((c) <= (end)))
-#define IS_ALPHA(c) (IS_IN_RANGE(c, 'a', 'z') || IS_IN_RANGE(c, 'A', 'Z'))
-#define IS_NUM(c) IS_IN_RANGE(c, '0', '9')
-#define IS_ALPHANUM(c) (IS_ALPHA(c) || IS_NUM(c))
-
-#define PIN_LENGTH 8
-#define PIN_BUFFER_LENGTH (PIN_LENGTH + 2)
+#define PIN_BUFFER_LENGTH (MAX_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
@@ -59,7 +53,7 @@ unsigned int update_pin_buffer(volatile unsigned int rx) {
     }
 
     unsigned char index = APDU_OP();
-    if ((index >= 0) && (index <= PIN_LENGTH)) {
+    if ((index >= 0) && (index <= MAX_PIN_LENGTH)) {
         G_pin_buffer[index] = APDU_AT(DATA);
         G_pin_buffer[index + 1] = 0;
     }
@@ -92,28 +86,14 @@ unsigned int set_pin() {
 }
 
 /*
- * Validates that the pin curently saved to the internal buffer has exactly
- * PIN_LENGTH alphanumeric characters with at least one alphabetic character.
+ * Validates that the pin curently saved to the internal buffer
+ * is valid according to the current pin policy
+ * (see pin_policy for details)
  *
- * @ret     true if pin is valid, false otherwise
+ * @returns 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;
+    return pin_policy_is_valid_pin((const char *)GET_PIN(), GET_PIN_LENGTH());
 }
 
 /*

firmware/src/ledger/ui/test/mock/common.mk

@@ -30,4 +30,6 @@ CFLAGS  = -iquote $(THISMOCKDIR) -iquote $(COMMONDIR) -iquote $(MOCKDIR)
 CFLAGS += -iquote $(SRCDIR) -iquote $(HALINCDIR) -iquote $(HALSRCDIR)
 CFLAGS += -DHSM_PLATFORM_X86
 
+VPATH += $(SRCDIR):$(MOCKDIR):$(COMMONDIR)
+
 include ../../../../../coverage/coverage.mk

firmware/src/ledger/ui/test/pin/Makefile

@@ -23,22 +23,13 @@
 include ../mock/common.mk
 
 PROG = test.out
-OBJS = mock.o pin.o test_pin.o
+OBJS = mock.o pin.o test_pin.o pin_policy.o
 
 all: $(PROG)
 
 $(PROG): $(OBJS)
 	$(CC) $(COVFLAGS) -o $@ $^
 
-test_pin.o: test_pin.c
-	$(CC) $(CFLAGS) -c -o $@ $^
-
-pin.o: $(SRCDIR)/pin.c
-	$(CC) $(CFLAGS) $(COVFLAGS) -c -o $@ $^
-
-mock.o: $(MOCKDIR)/mock.c
-	$(CC) $(CFLAGS) -c -o $@ $^
-
 .PHONY: clean test
 
 clean: