Sanitizes key for kvstore

- Sanitizes the key before using it for file operations.
- Added unit tests for keyvalue_store module

firmware/src/sgx/src/untrusted/keyvalue_store.c

@@ -23,20 +23,53 @@
  */
 
 #include <sys/stat.h>
-#include "hsm_u.h"
+#include <ctype.h>
+#include <stdio.h>
+#include <string.h>
 #include "log.h"
+#include "keyvalue_store.h"
 
 #define KVSTORE_PREFIX "./kvstore-"
 #define KVSTORE_SUFFIX ".dat"
+#define KVSTORE_MAX_KEY_LEN 150
+
+
+// Sanitizes a key by allowing only [a-zA-Z0-9]. If one or more invalid characters are
+// found, Replace them with a single hyphen.
+static void sanitize_key(char* key, char *sanitized_key) {
+    if (!key || !sanitized_key) return;
+
+    size_t key_len = strlen(key);
+
+    // Truncate key if it's too long
+    if (key_len > KVSTORE_MAX_KEY_LEN) {
+        key_len = KVSTORE_MAX_KEY_LEN;
+    }
+
+    bool prev_char_valid = false;
+    size_t sanitized_key_len = 0;
+    for (size_t i = 0; i < key_len; i++) {
+        if (isalnum(key[i])) {
+            sanitized_key[sanitized_key_len++] = key[i];
+            prev_char_valid = true;
+        } else if (prev_char_valid) {
+            sanitized_key[sanitized_key_len++] = '-';
+            prev_char_valid = false;
+        }
+    }
+    sanitized_key[sanitized_key_len] = '\0';
+}
 
 static char* filename_for(char* key) {
+    char sanitized_key[KVSTORE_MAX_KEY_LEN + 1];
+    sanitize_key(key, sanitized_key);
     size_t filename_size = strlen(KVSTORE_PREFIX) + 
                            strlen(KVSTORE_SUFFIX) + 
-                           strlen(key);
+                           strlen(sanitized_key);
     char* filename = malloc(filename_size+1);
     strcpy(filename, "");
     strcat(filename, KVSTORE_PREFIX);
-    strcat(filename, key);
+    strcat(filename, sanitized_key);
     strcat(filename, KVSTORE_SUFFIX);
     return filename;
 }

firmware/src/sgx/src/untrusted/keyvalue_store.h

@@ -25,6 +25,9 @@
 #ifndef __KEYVALUE_STORE_H
 #define __KEYVALUE_STORE_H
 
+#include <stdbool.h>
+#include <stdint.h>
+
 /**
  * @brief Tell whether a given key currently exists
  * 

firmware/src/sgx/test/common/common.mk

@@ -1,19 +1,21 @@
 TESTCOMMONDIR = ../common
 SGXTRUSTEDDIR = ../../src/trusted
+SGXUNTRUSTEDDIR = ../../src/untrusted
 HALINCDIR = ../../../hal/include
 HALSGXSRCDIR = ../../../hal/sgx/src/trusted
 POWHSMSRCDIR = ../../../powhsm/src
 COMMONDIR = ../../../common/src
 
 CFLAGS  = -iquote $(TESTCOMMONDIR)
 CFLAGS += -iquote $(SGXTRUSTEDDIR)
+CFLAGS += -iquote $(SGXUNTRUSTEDDIR)
 CFLAGS += -iquote $(HALINCDIR)
 CFLAGS += -iquote $(HALSGXSRCDIR)
 CFLAGS += -iquote $(POWHSMSRCDIR)
 CFLAGS += -iquote $(COMMONDIR)
 CFLAGS += -DHSM_PLATFORM_SGX
 
-VPATH += $(SGXTRUSTEDDIR):$(COMMONDIR)
+VPATH += $(SGXTRUSTEDDIR):$(SGXUNTRUSTEDDIR):$(COMMONDIR)
 
 include ../../../../coverage/coverage.mk
 

firmware/src/sgx/test/keyvalue_store/Makefile

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

firmware/src/sgx/test/keyvalue_store/test_keyvalue_store.c

@@ -0,0 +1,255 @@
+/**
+ * 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 <assert.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include "keyvalue_store.h"
+
+// Test helpers
+void setup() {
+    system("rm -f ./kvstore-*.dat");
+}
+
+void assert_key_exists(char* key, bool exists) {
+    assert(kvstore_exists(key) == exists);
+}
+
+void assert_key_value(char* key, uint8_t* data, size_t data_size) {
+    uint8_t retrieved_data[BUFSIZ];
+    size_t retrieved_size = kvstore_get(key, retrieved_data, sizeof(retrieved_data));
+    assert(retrieved_size == data_size);
+    assert(memcmp(retrieved_data, data, retrieved_size) == 0);
+}
+
+void save_and_assert_success(char* key, uint8_t* data, size_t data_size) {
+    assert(kvstore_save(key, data, data_size));
+    assert_key_exists(key, true);
+}
+
+void remove_and_assert_success(char* key) {
+    assert(kvstore_remove(key));
+    assert_key_exists(key, false);
+}
+
+void assert_file_exists(char* filename, bool exists) {
+    FILE* file = fopen(filename, "rb");
+    if (exists) {
+        assert(file != NULL);
+    } else {
+        assert(file == NULL);
+    }
+    if (file) {
+        fclose(file);
+    }
+}
+
+void assert_file_contents(char* filename, uint8_t* data, size_t data_size) {
+    FILE* file = fopen(filename, "rb");
+    assert(file != NULL);
+
+    uint8_t file_data[BUFSIZ];
+    size_t file_size = fread(file_data, sizeof(file_data[0]), sizeof(file_data), file);
+    assert(file_size == data_size);
+    assert(memcmp(file_data, data, data_size) == 0);
+
+    fclose(file);
+}
+
+// Test cases
+void test_save_retrieve() {
+    printf("Test save and retrieve...\n");
+    setup();
+
+    struct {
+        char* key;
+        uint8_t* data;
+    } input_data[] = {
+        {"a-key", (uint8_t*)"some piece of data"},
+        {"another-key", (uint8_t*)"another piece of data"},
+        {"yet-another-key", (uint8_t*)"yet another piece of data"},
+        {"the-last-key", (uint8_t*)"the last piece of data"}
+    };
+    size_t num_inputs = sizeof(input_data) / sizeof(input_data[0]);
+
+    for (size_t i = 0; i < num_inputs; i++) {
+        save_and_assert_success(input_data[i].key, input_data[i].data, strlen((char*)input_data[i].data));
+    }
+
+    for (size_t i = 0; i < num_inputs; i++) {
+        assert_key_value(input_data[i].key, input_data[i].data, strlen((char*)input_data[i].data));
+    }
+}
+
+void test_kvstore_exists() {
+    printf("Test kvstore_exists...\n");
+    setup();
+
+    struct {
+        char* key;
+        uint8_t* data;
+    } existing_keys[] = {
+        {"first-key", (uint8_t*)"some piece of data"},
+        {"second-key", (uint8_t*)"another piece of data"},
+        {"third-key", (uint8_t*)"yet another piece of data"},
+    };
+    size_t num_existing_keys = sizeof(existing_keys) / sizeof(existing_keys[0]);
+
+    char* non_existing_keys[] = {
+        "non-existing-key-1",
+        "non-existing-key-2",
+        "non-existing-key-3",
+    };
+    size_t num_non_existing_keys = sizeof(non_existing_keys) / sizeof(non_existing_keys[0]);
+
+    for (size_t i = 0; i < num_existing_keys; i++) {
+        save_and_assert_success(existing_keys[i].key, existing_keys[i].data, strlen((char*)existing_keys[i].data));
+    }
+
+    for (size_t i = 0; i < num_existing_keys; i++) {
+        assert_key_exists(existing_keys[i].key, true);
+    }
+
+    for (size_t i = 0; i < num_non_existing_keys; i++) {
+        assert_key_exists(non_existing_keys[i], false);
+    }
+}
+
+void test_save_remove() {
+    printf("Test save and remove...\n");
+    setup();
+
+    struct {
+        char* key;
+        uint8_t* data;
+        bool remove;
+    } input_data[] = {
+        {"first-key", (uint8_t*)"some piece of data", false},
+        {"second-key", (uint8_t*)"another piece of data", true},
+        {"third-key", (uint8_t*)"yet another piece of data", true},
+        {"fourth-key", (uint8_t*)"the last piece of data", false},
+    };
+    size_t num_inputs = sizeof(input_data) / sizeof(input_data[0]);
+
+    for (size_t i = 0; i < num_inputs; i++) {
+        save_and_assert_success(input_data[i].key, input_data[i].data, strlen((char*)input_data[i].data));
+        assert_key_value(input_data[i].key, input_data[i].data, strlen((char*)input_data[i].data));
+    }
+
+    // Remove selected keys
+    for (size_t i = 0; i < num_inputs; i++) {
+        if (input_data[i].remove) {
+            remove_and_assert_success(input_data[i].key);
+        }
+    }
+
+    // Assert that the selected keys were removed and the others still exist
+    for (size_t i = 0; i < num_inputs; i++) {
+        if (input_data[i].remove) {
+            assert_key_exists(input_data[i].key, false);
+        } else {
+            assert_key_value(input_data[i].key, input_data[i].data, strlen((char*)input_data[i].data));
+        }
+    }
+}
+
+void test_filename() {
+    printf("Test filename for key...\n");
+    setup();
+
+    struct {
+        char* key;
+        uint8_t* data;
+        char* filename;
+    } input_data[] = {
+        {"first-key", "data for the first key", "kvstore-first-key.dat"},
+        {"second-key", "data for the second key", "kvstore-second-key.dat"},
+        {"third-key", "data for the third key", "kvstore-third-key.dat"},
+        {"fourth-key", "data for the fourth key", "kvstore-fourth-key.dat"},
+    };
+    size_t num_inputs = sizeof(input_data) / sizeof(input_data[0]);
+
+    // Make sure none of the files exist
+    for (size_t i = 0; i < num_inputs; i++) {
+        assert_file_exists(input_data[i].filename, false);
+    }
+
+    // Save data to each key and assert that the file name and contents are correct
+    for (size_t i = 0; i < num_inputs; i++) {
+        save_and_assert_success(input_data[i].key, (uint8_t*)input_data[i].data, strlen(input_data[i].data));
+        assert_file_exists(input_data[i].filename, true);
+        assert_file_contents(input_data[i].filename, (uint8_t*)input_data[i].data, strlen(input_data[i].data));
+    }
+}
+
+void test_sanitize_key() {
+    printf("Test sanitize key...\n");
+    setup();
+
+    struct {
+        char* key;
+        char* filename;
+        uint8_t* data;
+    } input_data[] = {
+        {"onlyletters", "kvstore-onlyletters.dat", "data1"},
+        {"123456", "kvstore-123456.dat", "data2"},
+        {"lettersandnumbers123", "kvstore-lettersandnumbers123.dat", "data3"},
+        {"letters-and-numbers-with-hyphen-123", "kvstore-letters-and-numbers-with-hyphen-123.dat", "data4"},
+        {"key containing spaces", "kvstore-key-containing-spaces.dat", "data5"},
+        {"key containing special characters!@#$%^&*()", "kvstore-key-containing-special-characters-.dat", "data6"},
+        {"../../../../../etc/passwd", "kvstore-etc-passwd.dat", "data7"},
+        {"some@#£_&-(_./file#£+-:;name", "kvstore-some-file-name.dat", "data8"},
+    };
+    size_t num_inputs = sizeof(input_data) / sizeof(input_data[0]);
+
+    // Make sure none of the files exist
+    for (size_t i = 0; i < num_inputs; i++) {
+        assert_file_exists(input_data[i].filename, false);
+    }
+
+    // Save data to each key and assert that the file name and contents are correct
+    for (size_t i = 0; i < num_inputs; i++) {
+        save_and_assert_success(input_data[i].key, input_data[i].data, strlen(input_data[i].data));
+        assert_file_exists(input_data[i].filename, true);
+        assert_file_contents(input_data[i].filename, input_data[i].data, strlen(input_data[i].data));
+    }
+
+    // Ensure data can be retrieved with the original key
+    for (size_t i = 0; i < num_inputs; i++) {
+        assert_key_value(input_data[i].key, input_data[i].data, strlen(input_data[i].data));
+    }
+}
+
+
+int main() {
+    test_save_retrieve();
+    test_kvstore_exists();
+    test_save_remove();
+    test_filename();
+    test_sanitize_key();
+    return 0;
+}

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

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