From c93d70d7d4aab422f800715ef2d82c4a7f7cba6c Mon Sep 17 00:00:00 2001 From: Italo Sampaio <100376888+italo-sampaio@users.noreply.github.com> Date: Tue, 26 Nov 2024 16:21:25 -0300 Subject: [PATCH 01/12] Fixes coverage report for feature/sgx branch (#224) - Triggers the coverage workflow for pushes to master and feature/sgx branches - Adds optional exec argument unit tests scripts - Some additional fixes to unit tests --- .github/workflows/coverage.yml | 2 +- firmware/coverage/gen-coverage | 16 +++++------ firmware/src/common/test/run-all.sh | 30 ++++++++++++-------- firmware/src/hal/common/test/run-all.sh | 30 ++++++++++++-------- firmware/src/hal/sgx/test/run-all.sh | 29 ++++++++++++------- firmware/src/hal/x86/test/run-all.sh | 29 ++++++++++++------- firmware/src/ledger/signer/test/run-all.sh | 30 ++++++++++++-------- firmware/src/ledger/ui/test/run-all.sh | 31 +++++++++++++-------- firmware/src/powhsm/test/btctx/test_btctx.c | 6 ++-- firmware/src/powhsm/test/run-all.sh | 30 ++++++++++++-------- firmware/src/powhsm/test/srlp/test_srlp.c | 5 +++- firmware/src/sgx/test/run-all.sh | 30 ++++++++++++-------- 12 files changed, 170 insertions(+), 98 deletions(-) diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 8a688477..8169f121 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -2,7 +2,7 @@ name: "Code coverage" on: push: - branches: [ "master" ] + branches: [ "master", "feature/sgx" ] jobs: coverage: diff --git a/firmware/coverage/gen-coverage b/firmware/coverage/gen-coverage index e8d4e6cc..70859ef8 100755 --- a/firmware/coverage/gen-coverage +++ b/firmware/coverage/gen-coverage @@ -10,13 +10,13 @@ if [[ $1 == "exec" ]]; then find $REPOROOT/firmware -name "*.gcno" -o -name "*.gcda" | xargs rm -f # Run unit tests with coverage generation - COVERAGE=y $REPOROOT/firmware/src/common/test/run-all.sh - COVERAGE=y $REPOROOT/firmware/src/powhsm/test/run-all.sh - COVERAGE=y $REPOROOT/firmware/src/sgx/test/run-all.sh - COVERAGE=y $REPOROOT/firmware/src/ledger/ui/test/run-all.sh - COVERAGE=y $REPOROOT/firmware/src/ledger/signer/test/run-all.sh - COVERAGE=y $REPOROOT/firmware/src/tcpsigner/test/run-all.sh - COVERAGE=y $REPOROOT/firmware/src/hal/sgx/test/run-all.sh + # The `exec` argument is used for all scripts, since we are running them inside a docker container + COVERAGE=y $REPOROOT/firmware/src/common/test/run-all.sh exec + COVERAGE=y $REPOROOT/firmware/src/powhsm/test/run-all.sh exec + COVERAGE=y $REPOROOT/firmware/src/sgx/test/run-all.sh exec + COVERAGE=y $REPOROOT/firmware/src/ledger/ui/test/run-all.sh exec + COVERAGE=y $REPOROOT/firmware/src/ledger/signer/test/run-all.sh exec + COVERAGE=y $REPOROOT/firmware/src/hal/sgx/test/run-all.sh exec # Run tcpsigner test suite pushd $REPOROOT/firmware/src/tcpsigner > /dev/null @@ -37,7 +37,7 @@ if [[ $1 == "exec" ]]; then # Remove unwanted coverage info (test files, tcpsigner, x86 HAL implementation, mock files) lcov --remove $BASEDIR/coverage.info "*/test_*.c" --output-file $BASEDIR/coverage.info lcov --remove $BASEDIR/coverage.info "*/tcpsigner/src/*" --output-file $BASEDIR/coverage.info - lcov --remove $BASEDIR/coverage.info "*/hal/src/x86/*" --output-file $BASEDIR/coverage.info + lcov --remove $BASEDIR/coverage.info "*/hal/x86/src/*" --output-file $BASEDIR/coverage.info lcov --remove $BASEDIR/coverage.info "*/mock_*.c" --output-file $BASEDIR/coverage.info # Generate report and summary genhtml $BASEDIR/coverage.info --output $BASEDIR/output -p $SRCDIR -t "powHSM firmware" diff --git a/firmware/src/common/test/run-all.sh b/firmware/src/common/test/run-all.sh index 0ac28ed5..f5efe428 100755 --- a/firmware/src/common/test/run-all.sh +++ b/firmware/src/common/test/run-all.sh @@ -1,13 +1,21 @@ #!/bin/bash -BASEDIR=$(dirname $0) -TESTDIRS="bigdigits_helper ints memutil" -TESTDIRS=${1:-"$TESTDIRS"} -for d in $TESTDIRS; do - echo "******************************" - echo "Testing $d..." - echo "******************************" - cd "$BASEDIR/$d" - make clean test || exit $? - cd - > /dev/null -done +if [[ $1 == "exec" ]]; then + BASEDIR=$(realpath $(dirname $0)) + TESTDIRS="bigdigits_helper ints memutil" + for d in $TESTDIRS; do + echo "******************************" + echo "Testing $d..." + echo "******************************" + cd "$BASEDIR/$d" + make clean test || exit $? + cd - > /dev/null + done + exit 0 +else + # Script directory + REPOROOT=$(realpath $(dirname $0)/../../../../) + SCRIPT=$(realpath $0 --relative-to=$REPOROOT) + + $REPOROOT/docker/mware/do-notty-nousb /hsm2 "./$SCRIPT exec" +fi diff --git a/firmware/src/hal/common/test/run-all.sh b/firmware/src/hal/common/test/run-all.sh index 4278c458..650caf44 100755 --- a/firmware/src/hal/common/test/run-all.sh +++ b/firmware/src/hal/common/test/run-all.sh @@ -1,13 +1,21 @@ #!/bin/bash -BASEDIR=$(dirname $0) -TESTDIRS="sha256" -TESTDIRS=${1:-"$TESTDIRS"} -for d in $TESTDIRS; do - echo "******************************" - echo "Testing $d..." - echo "******************************" - cd "$BASEDIR/$d" - make clean test || exit $? - cd - > /dev/null -done +if [[ $1 == "exec" ]]; then + BASEDIR=$(realpath $(dirname $0)) + TESTDIRS="sha256" + for d in $TESTDIRS; do + echo "******************************" + echo "Testing $d..." + echo "******************************" + cd "$BASEDIR/$d" + make clean test || exit $? + cd - > /dev/null + done + exit 0 +else + # Script directory + REPOROOT=$(realpath $(dirname $0)/../../../../../) + SCRIPT=$(realpath $0 --relative-to=$REPOROOT) + + $REPOROOT/docker/mware/do-notty-nousb /hsm2 "./$SCRIPT exec" +fi diff --git a/firmware/src/hal/sgx/test/run-all.sh b/firmware/src/hal/sgx/test/run-all.sh index c7ce071e..d059b586 100755 --- a/firmware/src/hal/sgx/test/run-all.sh +++ b/firmware/src/hal/sgx/test/run-all.sh @@ -1,12 +1,21 @@ #!/bin/bash -ROOTDIR=$(dirname $0)/../../../../.. -TESTDIR=$(realpath $(dirname $0) --relative-to $ROOTDIR) -TESTDIRS="nvmem secret_store seed" -TESTDIRS=${1:-"$TESTDIRS"} -for d in $TESTDIRS; do - echo "******************************" - echo "Testing $d..." - echo "******************************" - $ROOTDIR/docker/mware/do-notty-nousb /hsm2/$TESTDIR/$d "make clean test" || exit $? -done +if [[ $1 == "exec" ]]; then + BASEDIR=$(realpath $(dirname $0)) + TESTDIRS="nvmem secret_store seed" + for d in $TESTDIRS; do + echo "******************************" + echo "Testing $d..." + echo "******************************" + cd "$BASEDIR/$d" + make clean test || exit $? + cd - > /dev/null + done + exit 0 +else + # Script directory + REPOROOT=$(realpath $(dirname $0)/../../../../..) + SCRIPT=$(realpath $0 --relative-to=$REPOROOT) + + $REPOROOT/docker/mware/do-notty-nousb /hsm2 "./$SCRIPT exec" +fi diff --git a/firmware/src/hal/x86/test/run-all.sh b/firmware/src/hal/x86/test/run-all.sh index 54e1f7d0..0628d598 100755 --- a/firmware/src/hal/x86/test/run-all.sh +++ b/firmware/src/hal/x86/test/run-all.sh @@ -1,12 +1,21 @@ #!/bin/bash -ROOTDIR=$(dirname $0)/../../../../.. -TESTDIR=$(realpath $(dirname $0) --relative-to $ROOTDIR) -TESTDIRS="bip32 endian hmac_sha256 hmac_sha512 keccak256" -TESTDIRS=${1:-"$TESTDIRS"} -for d in $TESTDIRS; do - echo "******************************" - echo "Testing $d..." - echo "******************************" - $ROOTDIR/docker/mware/do-notty-nousb /hsm2/$TESTDIR/$d "make clean test" || exit $? -done +if [[ $1 == "exec" ]]; then + BASEDIR=$(realpath $(dirname $0)) + TESTDIRS="bip32 endian hmac_sha256 hmac_sha512 keccak256" + for d in $TESTDIRS; do + echo "******************************" + echo "Testing $d..." + echo "******************************" + cd "$BASEDIR/$d" + make clean test || exit $? + cd - > /dev/null + done + exit 0 +else + # Script directory + REPOROOT=$(realpath $(dirname $0)/../../../../..) + SCRIPT=$(realpath $0 --relative-to=$REPOROOT) + + $REPOROOT/docker/mware/do-notty-nousb /hsm2 "./$SCRIPT exec" +fi diff --git a/firmware/src/ledger/signer/test/run-all.sh b/firmware/src/ledger/signer/test/run-all.sh index de927848..7631c557 100755 --- a/firmware/src/ledger/signer/test/run-all.sh +++ b/firmware/src/ledger/signer/test/run-all.sh @@ -1,13 +1,21 @@ #!/bin/bash -BASEDIR=$(dirname $0) -TESTDIRS="signer_ux" -TESTDIRS=${1:-"$TESTDIRS"} -for d in $TESTDIRS; do - echo "******************************" - echo "Testing $d..." - echo "******************************" - cd "$BASEDIR/$d" - make clean test || exit $? - cd - > /dev/null -done +if [[ $1 == "exec" ]]; then + BASEDIR=$(realpath $(dirname $0)) + TESTDIRS="signer_ux" + for d in $TESTDIRS; do + echo "******************************" + echo "Testing $d..." + echo "******************************" + cd "$BASEDIR/$d" + make clean test || exit $? + cd - > /dev/null + done + exit 0 +else + # Script directory + REPOROOT=$(realpath $(dirname $0)/../../../../../) + SCRIPT=$(realpath $0 --relative-to=$REPOROOT) + + $REPOROOT/docker/mware/do-notty-nousb /hsm2 "./$SCRIPT exec" +fi diff --git a/firmware/src/ledger/ui/test/run-all.sh b/firmware/src/ledger/ui/test/run-all.sh index 2be0d676..f7fd121f 100755 --- a/firmware/src/ledger/ui/test/run-all.sh +++ b/firmware/src/ledger/ui/test/run-all.sh @@ -1,13 +1,22 @@ #!/bin/bash -BASEDIR=$(dirname $0) -TESTDIRS="attestation bootloader onboard pin signer_authorization ui_comm ui_heartbeat unlock ux_handlers" -TESTDIRS=${1:-"$TESTDIRS"} -for d in $TESTDIRS; do - echo "******************************" - echo "Testing $d..." - echo "******************************" - cd "$BASEDIR/$d" - make clean test || exit $? - cd - > /dev/null -done +if [[ $1 == "exec" ]]; then + BASEDIR=$(realpath $(dirname $0)) + TESTDIRS="attestation bootloader onboard pin signer_authorization ui_comm ui_heartbeat unlock ux_handlers" + for d in $TESTDIRS; do + echo "******************************" + echo "Testing $d..." + echo "******************************" + cd "$BASEDIR/$d" + make clean test || exit $? + cd - > /dev/null + done + exit 0 +else + # Script directory + REPOROOT=$(realpath $(dirname $0)/../../../../../) + SCRIPT=$(realpath $0 --relative-to=$REPOROOT) + + $REPOROOT/docker/mware/do-notty-nousb /hsm2 "./$SCRIPT exec" +fi + diff --git a/firmware/src/powhsm/test/btctx/test_btctx.c b/firmware/src/powhsm/test/btctx/test_btctx.c index 0bea267d..8b372376 100644 --- a/firmware/src/powhsm/test/btctx/test_btctx.c +++ b/firmware/src/powhsm/test/btctx/test_btctx.c @@ -54,12 +54,14 @@ int read_hex_file(const char* file_name, unsigned char** buffer, size_t* len) { fread(tmp, 2, 1, f); read_hex(tmp, 2, *buffer + off); } - fclose(f); - if (ferror(f)) { return -1; } + if (fclose(f)) { + return -1; + } + return 0; } diff --git a/firmware/src/powhsm/test/run-all.sh b/firmware/src/powhsm/test/run-all.sh index 3eb6b08e..dbe1dc1d 100755 --- a/firmware/src/powhsm/test/run-all.sh +++ b/firmware/src/powhsm/test/run-all.sh @@ -1,13 +1,21 @@ #!/bin/bash -BASEDIR=$(dirname $0) -TESTDIRS="btcscript btctx difficulty srlp svarint trie" -TESTDIRS=${1:-"$TESTDIRS"} -for d in $TESTDIRS; do - echo "******************************" - echo "Testing $d..." - echo "******************************" - cd "$BASEDIR/$d" - make clean test || exit $? - cd - > /dev/null -done +if [[ $1 == "exec" ]]; then + BASEDIR=$(realpath $(dirname $0)) + TESTDIRS="btcscript btctx difficulty srlp svarint trie" + for d in $TESTDIRS; do + echo "******************************" + echo "Testing $d..." + echo "******************************" + cd "$BASEDIR/$d" + make clean test || exit $? + cd - > /dev/null + done + exit 0 +else + # Script directory + REPOROOT=$(realpath $(dirname $0)/../../../../) + SCRIPT=$(realpath $0 --relative-to=$REPOROOT) + + $REPOROOT/docker/mware/do-notty-nousb /hsm2 "./$SCRIPT exec" +fi diff --git a/firmware/src/powhsm/test/srlp/test_srlp.c b/firmware/src/powhsm/test/srlp/test_srlp.c index b05a2b96..dba0d1ee 100644 --- a/firmware/src/powhsm/test/srlp/test_srlp.c +++ b/firmware/src/powhsm/test/srlp/test_srlp.c @@ -204,12 +204,15 @@ int read_block_file(const char* file_name, char** buffer, size_t* len) { *buffer = malloc(*len); fread(*buffer, *len, 1, f); - fclose(f); if (ferror(f)) { return -1; } + if (fclose(f)) { + return -1; + } + return 0; } diff --git a/firmware/src/sgx/test/run-all.sh b/firmware/src/sgx/test/run-all.sh index ea5c2acc..e69b07d7 100755 --- a/firmware/src/sgx/test/run-all.sh +++ b/firmware/src/sgx/test/run-all.sh @@ -1,13 +1,21 @@ #!/bin/bash -BASEDIR=$(dirname $0) -TESTDIRS="system" -TESTDIRS=${1:-"$TESTDIRS"} -for d in $TESTDIRS; do - echo "******************************" - echo "Testing $d..." - echo "******************************" - cd "$BASEDIR/$d" - make clean test || exit $? - cd - > /dev/null -done +if [[ $1 == "exec" ]]; then + BASEDIR=$(realpath $(dirname $0)) + TESTDIRS="system" + for d in $TESTDIRS; do + echo "******************************" + echo "Testing $d..." + echo "******************************" + cd "$BASEDIR/$d" + make clean test || exit $? + cd - > /dev/null + done + exit 0 +else + # Script directory + REPOROOT=$(realpath $(dirname $0)/../../../../) + SCRIPT=$(realpath $0 --relative-to=$REPOROOT) + + $REPOROOT/docker/mware/do-notty-nousb /hsm2 "./$SCRIPT exec" +fi From f45141f85f5f2ad2415b8bede34a416a6687c30d Mon Sep 17 00:00:00 2001 From: Italo Sampaio <100376888+italo-sampaio@users.noreply.github.com> Date: Mon, 2 Dec 2024 09:47:06 -0300 Subject: [PATCH 02/12] Install SGX powHSM as a systemd service (#226) --- dist/sgx/hsm/{run => start} | 7 +++- dist/sgx/hsm/stop | 4 ++ dist/sgx/scripts/hsmsgx.service | 19 +++++++++ dist/sgx/scripts/install_service | 67 ++++++++++++++++++++++++++++++++ dist/sgx/scripts/setup | 16 ++++++++ dist/sgx/setup-new-powhsm | 20 +++++++++- 6 files changed, 131 insertions(+), 2 deletions(-) rename dist/sgx/hsm/{run => start} (81%) create mode 100755 dist/sgx/hsm/stop create mode 100644 dist/sgx/scripts/hsmsgx.service create mode 100755 dist/sgx/scripts/install_service diff --git a/dist/sgx/hsm/run b/dist/sgx/hsm/start similarity index 81% rename from dist/sgx/hsm/run rename to dist/sgx/hsm/start index 44d972af..163454d0 100755 --- a/dist/sgx/hsm/run +++ b/dist/sgx/hsm/start @@ -4,6 +4,8 @@ BINDIR=$(realpath $(dirname $0)) WORKDIR=$(realpath $BINDIR/..) DOCKER_IMAGE=powhsmsgx:runner +source $BINDIR/.env + QUIET="" echo -e "\e[96mBuilding docker image $DOCKER_IMAGE (this will take a few minutes)..." if [[ "$2" != "-v" ]]; then @@ -16,10 +18,13 @@ echo DOCKER_CNT=powhsmsgx-runner DOCKER_USER="$(id -u):$(id -g)" +HOSTNAME="SGX" +NETWORK=${NETWORK:-net_sgx} PORT=7777 DOCKER_PORT="$PORT:$PORT" -docker run -ti --rm --name $DOCKER_CNT --user $DOCKER_USER -v $WORKDIR:/hsm \ +docker run --rm --name $DOCKER_CNT --user $DOCKER_USER -v $WORKDIR:/hsm \ + --hostname $HOSTNAME --network $NETWORK \ --device=/dev/sgx_enclave:/dev/sgx_enclave \ --device=/dev/sgx_provision:/dev/sgx_provision \ -w /hsm -p$DOCKER_PORT $DOCKER_IMAGE \ diff --git a/dist/sgx/hsm/stop b/dist/sgx/hsm/stop new file mode 100755 index 00000000..65d5355b --- /dev/null +++ b/dist/sgx/hsm/stop @@ -0,0 +1,4 @@ +#!/bin/bash + +DOCKER_CNT=powhsmsgx-runner +docker stop $DOCKER_CNT diff --git a/dist/sgx/scripts/hsmsgx.service b/dist/sgx/scripts/hsmsgx.service new file mode 100644 index 00000000..18c7f7e9 --- /dev/null +++ b/dist/sgx/scripts/hsmsgx.service @@ -0,0 +1,19 @@ +[Unit] +Description=SGX powHSM +Wants=network.target +After=syslog.target network-online.target docker.service +Requires=docker.service + +[Service] +Type=simple +WorkingDirectory=$HSM_INSTALL_DIR +User=hsm +Group=hsm +ExecStart=$HSM_INSTALL_DIR/bin/start +ExecStop=$HSM_INSTALL_DIR/bin/stop +Restart=on-failure +RestartSec=10 +KillMode=mixed + +[Install] +WantedBy=multi-user.target \ No newline at end of file diff --git a/dist/sgx/scripts/install_service b/dist/sgx/scripts/install_service new file mode 100755 index 00000000..47bfb7db --- /dev/null +++ b/dist/sgx/scripts/install_service @@ -0,0 +1,67 @@ +#!/bin/bash + +# Require superuser +if ! [ "$(id -u)" == "0" ]; then + echo -e "\e[1;31mPlease run with sudo.\e[0m" + exit 1 +fi + +if [ -z "$1" ]; then + echo -e "\e[1;31mUsage: $0 \e[0m" + exit 1 +fi + +SERVICE_UNIT=$(realpath $1) +if [ ! -f "$SERVICE_UNIT" ]; then + echo "\e[1;31mService file not found: $SERVICE_UNIT\e[0m" + exit 1 +fi + +# Extract the installation directory from the service file +INSTALL_DIR=$(grep -oP 'WorkingDirectory=\K.*' $SERVICE_UNIT) +if [ -z "$INSTALL_DIR" ]; then + echo -e "\e[1;31mCould not extract installation directory from service file.\e[0m" + exit 1 +fi + +echo -e "\e[1;32mCreating hsm user and group...\e[0m" +if ! id -u hsm >/dev/null 2>&1; then + useradd -rm -s /bin/bash hsm || exit $? + usermod -aG docker hsm || exit $? +else + echo -e "\e[1;33mUser 'hsm' already exists. Skipping user creation.\e[0m" +fi + +DEFAULT_NETWORK="net_sgx" +while true; do + echo -e "\e[1;32mEnter the name of the docker network to be created: [$DEFAULT_NETWORK]\e[0m" + read -p "> " NETWORK + if [ -z "$NETWORK" ]; then + NETWORK=$DEFAULT_NETWORK + fi + echo -e "\e[1;33mThe docker network will be named '$NETWORK'. Proceed? [Y/n]\e[0m" + read -p "> " proceed + if [[ "Y" == "$proceed" ]] || [[ "y" == "$proceed" ]] || [ -z "$proceed" ]; then + break + fi +done + +echo -e "\e[1;32mCreating $NETWORK network...\e[0m" +docker network rm $NETWORK 2> /dev/null +docker network create $NETWORK &> /dev/null +echo "NETWORK=$NETWORK" >> $INSTALL_DIR/.env || exit $? + +echo -e "\e[1;32mSetting permisions...\e[0m" +chown -R root:hsm $INSTALL_DIR || exit $? +chmod 664 $INSTALL_DIR/*.dat $INSTALL_DIR/.env || exit $? + +echo -e "\e[1;32mCreating service...\e[0m" +cp $SERVICE_UNIT /etc/systemd/system/hsmsgx.service +systemctl daemon-reload || exit $? +echo -e "\e[1;32mEnabling service...\e[0m" +systemctl enable hsmsgx.service || exit $? +echo -e "\e[1;32mEStarting service...\e[0m" +systemctl start hsmsgx.service || exit $? +echo -e "\e[1;32mService started.\e[0m" +echo -e "\e[1;32mTo check the status of the service, run 'systemctl status hsmsgx.service'.\e[0m" +exit 0 diff --git a/dist/sgx/scripts/setup b/dist/sgx/scripts/setup index b118e26f..2170614d 100755 --- a/dist/sgx/scripts/setup +++ b/dist/sgx/scripts/setup @@ -35,6 +35,12 @@ EXPORT_DIR="$ROOT_DIR/export" PUBLIC_KEY_FILE="$EXPORT_DIR/public-keys.txt" PUBLIC_KEY_FILE_JSON="$EXPORT_DIR/public-keys.json" +# HSM scripts directory +SCRIPTS_DIR=$ROOT_DIR/scripts + +# Directory where the finalized systemd service unit will be saved +SERVICE_DIR=$ROOT_DIR/service + function checkHsmBinaries() { # Check for HSM binary files FILES="$HSMBIN_DIR/hsmsgx $HSMBIN_DIR/hsmsgx_enclave.signed" @@ -96,6 +102,15 @@ function selectInstallationDir() { done } +function createServiceUnit() { + rm -rf $SERVICE_DIR + mkdir $SERVICE_DIR + + cp $SCRIPTS_DIR/hsmsgx.service $SERVICE_DIR + # Replace the $HSM_INSTALL_DIR token in the script with the actual installation directory + sed -i "s|\$HSM_INSTALL_DIR|$INSTALL_DIR|g" $SERVICE_DIR/hsmsgx.service +} + function installPowHsm() { mkdir $REAL_INSTALL_DIR/bin cp -R $HSMBIN_DIR/* $REAL_INSTALL_DIR/bin @@ -134,6 +149,7 @@ checkForPinFile checkHsmBinaries expandBinaries selectInstallationDir +createServiceUnit echo echo -e "\e[1;32mInstalling the powHSM...\e[0m" installPowHsm diff --git a/dist/sgx/setup-new-powhsm b/dist/sgx/setup-new-powhsm index 1398782a..67dd81ba 100755 --- a/dist/sgx/setup-new-powhsm +++ b/dist/sgx/setup-new-powhsm @@ -1,3 +1,21 @@ #!/bin/bash -$(dirname $0)/scripts/run_with_docker ./scripts/setup $1 +# Require superuser, since we need to install a service in the host +if ! [ "$(id -u)" == "0" ]; then + echo -e "\e[1;32mPlease run with sudo.\e[0m" + exit 1 +fi + +ROOT_DIR=$(realpath $(dirname $0)) +$ROOT_DIR/scripts/run_with_docker ./scripts/setup $1 +if [ $? -ne 0 ]; then + echo -e "\e[1;31m Error during the powhsm setup, aborting \e[0m" + exit 1 +fi + +$ROOT_DIR/scripts/install_service $ROOT_DIR/service/hsmsgx.service +if [ $? -ne 0 ]; then + echo -e "\e[1;31m Error during the powhsm service installation, aborting \e[0m" + exit 1 +fi +echo -e "\e[1;32mHSM SGX setup done.\e[0m" From 5de65aab2bbc00ad22d88e3d1a4fb9cba6ba80d1 Mon Sep 17 00:00:00 2001 From: Ariel Mendelzon Date: Wed, 4 Dec 2024 06:34:35 +1300 Subject: [PATCH 03/12] Version 5.3.2 ALPHA release (#227) - Bumped version to 5.3.2 - Updated version references in firmware, middleware and unit tests - Updated CHANGELOG --- CHANGELOG.md | 10 ++++++++++ firmware/src/ledger/ui/src/defs.h | 2 +- firmware/src/ledger/ui/test/onboard/test_onboard.c | 4 ++-- firmware/src/powhsm/src/defs.h | 2 +- middleware/ledger/protocol.py | 4 ++-- middleware/tests/ledger/test_protocol.py | 2 +- middleware/tests/ledger/test_protocol_v1.py | 2 +- 7 files changed, 18 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 32e2bdcc..a74724b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changelog +## [5.3.2 ALPHA] - 04/12/2024 + +### Features/enhancements + +- SGX distribution: installing powHSM as a system service + +### Fixes + +- Fixed code coverage report + ## [5.3.1 ALPHA] - 14/11/2024 ### Fixes diff --git a/firmware/src/ledger/ui/src/defs.h b/firmware/src/ledger/ui/src/defs.h index b4577ec9..9f7ab309 100644 --- a/firmware/src/ledger/ui/src/defs.h +++ b/firmware/src/ledger/ui/src/defs.h @@ -31,6 +31,6 @@ // Version and patchlevel #define VERSION_MAJOR 0x05 #define VERSION_MINOR 0x03 -#define VERSION_PATCH 0x01 +#define VERSION_PATCH 0x02 #endif // __DEFS_H diff --git a/firmware/src/ledger/ui/test/onboard/test_onboard.c b/firmware/src/ledger/ui/test/onboard/test_onboard.c index 933e13b2..bcc8e555 100644 --- a/firmware/src/ledger/ui/test/onboard/test_onboard.c +++ b/firmware/src/ledger/ui/test/onboard/test_onboard.c @@ -313,11 +313,11 @@ void test_is_onboarded() { G_device_onboarded = true; assert(5 == is_onboarded()); - ASSERT_APDU("\x80\x01\x05\x03\x01"); + ASSERT_APDU("\x80\x01\x05\x03\x02"); G_device_onboarded = false; assert(5 == is_onboarded()); - ASSERT_APDU("\x80\x00\x05\x03\x01"); + ASSERT_APDU("\x80\x00\x05\x03\x02"); } int main() { diff --git a/firmware/src/powhsm/src/defs.h b/firmware/src/powhsm/src/defs.h index ac150a22..9465f26a 100644 --- a/firmware/src/powhsm/src/defs.h +++ b/firmware/src/powhsm/src/defs.h @@ -30,6 +30,6 @@ // Version and patchlevel #define VERSION_MAJOR 0x05 #define VERSION_MINOR 0x03 -#define VERSION_PATCH 0x01 +#define VERSION_PATCH 0x02 #endif // __DEFS_H diff --git a/middleware/ledger/protocol.py b/middleware/ledger/protocol.py index 3b683345..4141cda5 100644 --- a/middleware/ledger/protocol.py +++ b/middleware/ledger/protocol.py @@ -38,8 +38,8 @@ class HSM2ProtocolLedger(HSM2Protocol): # Current manager supported versions for HSM UI and HSM SIGNER (<=) - UI_VERSION = HSM2FirmwareVersion(5, 3, 1) - APP_VERSION = HSM2FirmwareVersion(5, 3, 1) + UI_VERSION = HSM2FirmwareVersion(5, 3, 2) + APP_VERSION = HSM2FirmwareVersion(5, 3, 2) # Amount of time to wait to make sure the app is opened OPEN_APP_WAIT = 1 # second diff --git a/middleware/tests/ledger/test_protocol.py b/middleware/tests/ledger/test_protocol.py index 25a076e2..08079d58 100644 --- a/middleware/tests/ledger/test_protocol.py +++ b/middleware/tests/ledger/test_protocol.py @@ -49,7 +49,7 @@ def setUp(self): self.dongle.disconnect = Mock() self.dongle.is_onboarded = Mock(return_value=True) self.dongle.get_current_mode = Mock(return_value=HSM2Dongle.MODE.SIGNER) - self.dongle.get_version = Mock(return_value=HSM2FirmwareVersion(5, 3, 1)) + self.dongle.get_version = Mock(return_value=HSM2FirmwareVersion(5, 3, 2)) self.dongle.get_signer_parameters = Mock(return_value=Mock( min_required_difficulty=123)) self.protocol = HSM2ProtocolLedger(self.pin, self.dongle) diff --git a/middleware/tests/ledger/test_protocol_v1.py b/middleware/tests/ledger/test_protocol_v1.py index 91978469..b0a4d0d1 100644 --- a/middleware/tests/ledger/test_protocol_v1.py +++ b/middleware/tests/ledger/test_protocol_v1.py @@ -47,7 +47,7 @@ def setUp(self): self.dongle.disconnect = Mock() self.dongle.is_onboarded = Mock(return_value=True) self.dongle.get_current_mode = Mock(return_value=HSM2Dongle.MODE.SIGNER) - self.dongle.get_version = Mock(return_value=HSM2FirmwareVersion(5, 3, 1)) + self.dongle.get_version = Mock(return_value=HSM2FirmwareVersion(5, 3, 2)) self.dongle.get_signer_parameters = Mock(return_value=Mock( min_required_difficulty=123)) self.protocol = HSM1ProtocolLedger(self.pin, self.dongle) From 53e25077c67701d033186012da3c05917fd88a5f Mon Sep 17 00:00:00 2001 From: Ariel Mendelzon Date: Tue, 14 Jan 2025 07:54:16 +1300 Subject: [PATCH 04/12] Fix middleware docker image build (#252) --- docker/mware/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/mware/Dockerfile b/docker/mware/Dockerfile index b9bad088..8c7055a2 100644 --- a/docker/mware/Dockerfile +++ b/docker/mware/Dockerfile @@ -11,7 +11,7 @@ RUN apt-get update && \ # Python package prerequisites RUN apt-get install -y \ libsecp256k1-dev=0.2.0-2 \ - libudev-dev=252.31-1~deb12u1 \ + libudev-dev=252.33-1~deb12u1 \ libusb-1.0-0-dev=2:1.0.26-1 \ libffi-dev=3.4.4-1 \ libjpeg-dev=1:2.1.5-2 From 37ddeea38f9404bb835cc55f6f06b764806c85f9 Mon Sep 17 00:00:00 2001 From: Ariel Mendelzon Date: Thu, 16 Jan 2025 05:53:08 +1300 Subject: [PATCH 05/12] Fixed C linting to include sgx code (#261) - Including sgx code in lint-c/format-c scripts - Fixed reported sgx linting errors --- firmware/src/sgx/src/trusted/ecall.c | 1 - firmware/src/sgx/src/trusted/sync.c | 3 ++- firmware/src/sgx/src/trusted/system.c | 18 +++++++-------- firmware/src/sgx/src/trusted/system.h | 7 +++--- .../src/sgx/src/untrusted/enclave_provider.c | 15 ++++++++---- .../src/sgx/src/untrusted/enclave_provider.h | 8 +++---- .../src/sgx/src/untrusted/enclave_proxy.c | 14 +++++------ firmware/src/sgx/src/untrusted/io.h | 6 ++--- .../src/sgx/src/untrusted/keyvalue_store.c | 23 ++++++++----------- .../src/sgx/src/untrusted/keyvalue_store.h | 16 ++++++------- firmware/src/sgx/src/untrusted/log.c | 8 +++---- firmware/src/sgx/src/untrusted/log.h | 4 ++-- firmware/src/sgx/src/untrusted/main.c | 7 +++--- lint-c | 3 ++- 14 files changed, 67 insertions(+), 66 deletions(-) diff --git a/firmware/src/sgx/src/trusted/ecall.c b/firmware/src/sgx/src/trusted/ecall.c index cf5c89d4..9d6bb2b0 100644 --- a/firmware/src/sgx/src/trusted/ecall.c +++ b/firmware/src/sgx/src/trusted/ecall.c @@ -44,4 +44,3 @@ unsigned int ecall_system_process_apdu(unsigned int rx) { SYNC_RELEASE_LOCK(); return result; } - diff --git a/firmware/src/sgx/src/trusted/sync.c b/firmware/src/sgx/src/trusted/sync.c index 078c9eee..179dd58b 100644 --- a/firmware/src/sgx/src/trusted/sync.c +++ b/firmware/src/sgx/src/trusted/sync.c @@ -28,7 +28,8 @@ static bool G_locked = false; bool sync_try_aqcuire_lock() { - if (G_locked) return false; + if (G_locked) + return false; G_locked = true; return true; } diff --git a/firmware/src/sgx/src/trusted/system.c b/firmware/src/sgx/src/trusted/system.c index 0f5d4a1b..9bb65808 100644 --- a/firmware/src/sgx/src/trusted/system.c +++ b/firmware/src/sgx/src/trusted/system.c @@ -90,13 +90,13 @@ static unsigned int do_unlock(unsigned int rx) { SET_APDU_OP(1); return TX_NO_DATA(); } - + if (APDU_DATA_SIZE(rx) == 0) { THROW(ERR_INVALID_DATA_SIZE); } - SET_APDU_OP( - access_unlock((char*)APDU_DATA_PTR, APDU_DATA_SIZE(rx)) ? 1 : 0); + SET_APDU_OP(access_unlock((char*)APDU_DATA_PTR, APDU_DATA_SIZE(rx)) ? 1 + : 0); return TX_NO_DATA(); } @@ -158,16 +158,17 @@ unsigned int system_process_apdu(unsigned int rx) { return hsm_process_apdu(rx); } -bool system_init(unsigned char *msg_buffer, size_t msg_buffer_size) { +bool system_init(unsigned char* msg_buffer, size_t msg_buffer_size) { // Setup the shared APDU buffer if (msg_buffer_size != EXPECTED_APDU_BUFFER_SIZE) { LOG("Expected APDU buffer size to be %u but got %lu\n", - EXPECTED_APDU_BUFFER_SIZE, msg_buffer_size); + EXPECTED_APDU_BUFFER_SIZE, + msg_buffer_size); return false; } apdu_buffer = msg_buffer; apdu_buffer_size = msg_buffer_size; - + // Initialize modules LOG("Initializing modules...\n"); if (!sest_init()) { @@ -206,9 +207,8 @@ bool system_init(unsigned char *msg_buffer, size_t msg_buffer_size) { } nvmem_init(); - if (!nvmem_register_block("bcstate", - &N_bc_state_var, - sizeof(N_bc_state_var))) { + if (!nvmem_register_block( + "bcstate", &N_bc_state_var, sizeof(N_bc_state_var))) { LOG("Error registering bcstate block\n"); return false; } diff --git a/firmware/src/sgx/src/trusted/system.h b/firmware/src/sgx/src/trusted/system.h index e571e136..8d922a9b 100644 --- a/firmware/src/sgx/src/trusted/system.h +++ b/firmware/src/sgx/src/trusted/system.h @@ -27,7 +27,7 @@ /** * @brief Initializes the system module - * + * * @param msg_buffer the APDU buffer to use * @param msg_buffer_size the size of the APDU buffer in bytes * @@ -37,12 +37,11 @@ bool system_init(unsigned char *msg_buffer, size_t msg_buffer_size); /** * @brief Process an APDU message - * + * * @param rx number of received bytes - * + * * @returns number of bytes to transmit */ unsigned int system_process_apdu(unsigned int rx); - #endif // __TRUSTED_SYSTEM_H diff --git a/firmware/src/sgx/src/untrusted/enclave_provider.c b/firmware/src/sgx/src/untrusted/enclave_provider.c index 20ac295c..5866ee94 100644 --- a/firmware/src/sgx/src/untrusted/enclave_provider.c +++ b/firmware/src/sgx/src/untrusted/enclave_provider.c @@ -22,7 +22,6 @@ * IN THE SOFTWARE. */ - #include #include "hsm_u.h" @@ -36,7 +35,8 @@ #define CREATE_ENCLAVE_FLAGS OE_ENCLAVE_FLAG_SIMULATE #endif -// Global pointer to the enclave. This should be the only global pointer to the enclave +// Global pointer to the enclave. This should be the only global pointer to the +// enclave static char* G_enclave_path = NULL; static oe_enclave_t* G_enclave = NULL; @@ -51,13 +51,18 @@ bool epro_init(char* enclave_path) { oe_enclave_t* epro_get_enclave() { if (NULL == G_enclave) { - oe_enclave_t *enclave = NULL; + oe_enclave_t* enclave = NULL; LOG("Creating HSM enclave...\n"); oe_result_t result = oe_create_hsm_enclave(G_enclave_path, OE_ENCLAVE_TYPE_AUTO, - CREATE_ENCLAVE_FLAGS, NULL, 0, &enclave); + CREATE_ENCLAVE_FLAGS, + NULL, + 0, + &enclave); if (OE_OK != result) { - LOG("Failed to create enclave: oe_result=%u (%s)\n", result, oe_result_str(result)); + LOG("Failed to create enclave: oe_result=%u (%s)\n", + result, + oe_result_str(result)); return NULL; } diff --git a/firmware/src/sgx/src/untrusted/enclave_provider.h b/firmware/src/sgx/src/untrusted/enclave_provider.h index 37a31f91..3adf6f17 100644 --- a/firmware/src/sgx/src/untrusted/enclave_provider.h +++ b/firmware/src/sgx/src/untrusted/enclave_provider.h @@ -29,16 +29,16 @@ /** * @brief Initializes the enclave provider with the given enclave binary path - * + * * @returns Whether initialization succeeded */ bool epro_init(char* enclave_path); /** - * @brief Returns a pointer to the HSM enclave. This function should always - * return a valid pointer to the enclave, which can be used to perform + * @brief Returns a pointer to the HSM enclave. This function should always + * return a valid pointer to the enclave, which can be used to perform * ecall operations. - * + * * @returns A valid pointer to the HSM enclave, or NULL if an error occurred */ oe_enclave_t* epro_get_enclave(); diff --git a/firmware/src/sgx/src/untrusted/enclave_proxy.c b/firmware/src/sgx/src/untrusted/enclave_proxy.c index 6e402b56..53bb4c44 100644 --- a/firmware/src/sgx/src/untrusted/enclave_proxy.c +++ b/firmware/src/sgx/src/untrusted/enclave_proxy.c @@ -22,8 +22,8 @@ * ECALLS */ -bool eprx_system_init(unsigned char *msg_buffer, size_t msg_buffer_size) { - oe_enclave_t *enclave = epro_get_enclave(); +bool eprx_system_init(unsigned char* msg_buffer, size_t msg_buffer_size) { + oe_enclave_t* enclave = epro_get_enclave(); if (enclave == NULL) { LOG("Failed to retrieve the enclave. " "Unable to call system_init().\n"); @@ -31,14 +31,14 @@ bool eprx_system_init(unsigned char *msg_buffer, size_t msg_buffer_size) { } bool result; - oe_result_t oe_result = ecall_system_init(enclave, &result, - msg_buffer, msg_buffer_size); + oe_result_t oe_result = + ecall_system_init(enclave, &result, msg_buffer, msg_buffer_size); CHECK_ECALL_RESULT(oe_result, "Failed to call system_init()", false); return result; } unsigned int eprx_system_process_apdu(unsigned int rx) { - oe_enclave_t *enclave = epro_get_enclave(); + oe_enclave_t* enclave = epro_get_enclave(); if (enclave == NULL) { LOG("Failed to retrieve the enclave. " "Unable to call system_process_command().\n"); @@ -48,7 +48,8 @@ unsigned int eprx_system_process_apdu(unsigned int rx) { unsigned int result; oe_result_t oe_result = ecall_system_process_apdu(enclave, &result, rx); - CHECK_ECALL_RESULT(oe_result, "Failed to call ecall_system_process_apdu()", false); + CHECK_ECALL_RESULT( + oe_result, "Failed to call ecall_system_process_apdu()", false); return result; } @@ -85,4 +86,3 @@ bool ocall_kvstore_remove(char* key) { log_clear_prefix(); return retval; } - diff --git a/firmware/src/sgx/src/untrusted/io.h b/firmware/src/sgx/src/untrusted/io.h index 405fe86a..cc2d0394 100644 --- a/firmware/src/sgx/src/untrusted/io.h +++ b/firmware/src/sgx/src/untrusted/io.h @@ -34,12 +34,12 @@ extern unsigned char io_apdu_buffer[APDU_BUFFER_SIZE]; /** - * @brief Initializes the I/O module. Starts a TCP server at the given host and + * @brief Initializes the I/O module. Starts a TCP server at the given host and * port. - * + * * @param port the port on which to listen for connections * @param host the interface to bind to - * + * */ bool io_init(int port, const char *host); diff --git a/firmware/src/sgx/src/untrusted/keyvalue_store.c b/firmware/src/sgx/src/untrusted/keyvalue_store.c index b743a7ec..a1058253 100644 --- a/firmware/src/sgx/src/untrusted/keyvalue_store.c +++ b/firmware/src/sgx/src/untrusted/keyvalue_store.c @@ -30,10 +30,9 @@ #define KVSTORE_SUFFIX ".dat" static char* filename_for(char* key) { - size_t filename_size = strlen(KVSTORE_PREFIX) + - strlen(KVSTORE_SUFFIX) + - strlen(key); - char* filename = malloc(filename_size+1); + size_t filename_size = + strlen(KVSTORE_PREFIX) + strlen(KVSTORE_SUFFIX) + strlen(key); + char* filename = malloc(filename_size + 1); strcpy(filename, ""); strcat(filename, KVSTORE_PREFIX); strcat(filename, key); @@ -45,7 +44,8 @@ static FILE* open_file_for(char* key, char* mode, size_t* file_size) { char* filename = filename_for(key); struct stat fst; stat(filename, &fst); - if (file_size) *file_size = fst.st_size; + if (file_size) + *file_size = fst.st_size; FILE* file = fopen(filename, mode); free(filename); return file; @@ -64,10 +64,7 @@ bool kvstore_save(char* key, uint8_t* data, size_t data_size) { return false; } - if (fwrite(data, - sizeof(data[0]), - data_size, - file) != data_size) { + if (fwrite(data, sizeof(data[0]), data_size, file) != data_size) { LOG("Error writing secret payload for key <%s>\n", key); fclose(file); return false; @@ -109,10 +106,7 @@ size_t kvstore_get(char* key, uint8_t* data_buf, size_t buffer_size) { return 0; } - if (fread(data_buf, - sizeof(data_buf[0]), - file_size, - file) != file_size) { + if (fread(data_buf, sizeof(data_buf[0]), file_size, file) != file_size) { LOG("Could not read payload for key <%s>\n", key); fclose(file); return 0; @@ -125,7 +119,8 @@ size_t kvstore_get(char* key, uint8_t* data_buf, size_t buffer_size) { bool kvstore_remove(char* key) { char* filename = filename_for(key); int result = remove(filename); - if (result) LOG("Error removing file for key <%s>\n", key); + if (result) + LOG("Error removing file for key <%s>\n", key); free(filename); return !result; } diff --git a/firmware/src/sgx/src/untrusted/keyvalue_store.h b/firmware/src/sgx/src/untrusted/keyvalue_store.h index 359d8010..872a60a5 100644 --- a/firmware/src/sgx/src/untrusted/keyvalue_store.h +++ b/firmware/src/sgx/src/untrusted/keyvalue_store.h @@ -27,40 +27,40 @@ /** * @brief Tell whether a given key currently exists - * + * * @param key the key to check for - * + * * @returns whether the key exists */ bool kvstore_exists(char* key); /** * @brief Save the given data to the given key - * + * * @param key the key to save the data to * @param data the buffer containing the data to write * @param data_size the data size in bytes - * + * * @returns whether saving succeeded */ bool kvstore_save(char* key, uint8_t* data, size_t data_size); /** * @brief Read the given key into the given buffer - * + * * @param key the key to read from * @param data_buf the buffer to read the data to * @param buffer_size the buffer size in bytes - * + * * @returns the number of bytes read, or ZERO upon error */ size_t kvstore_get(char* key, uint8_t* data_buf, size_t buffer_size); /** * @brief Remove any data associated with the given key - * + * * @param key the key to remove - * + * * @returns whether key removal was successful */ bool kvstore_remove(char* key); diff --git a/firmware/src/sgx/src/untrusted/log.c b/firmware/src/sgx/src/untrusted/log.c index f8aa9e47..0d4ea99c 100644 --- a/firmware/src/sgx/src/untrusted/log.c +++ b/firmware/src/sgx/src/untrusted/log.c @@ -27,7 +27,7 @@ #include "log.h" -static char* log_prefix = (char*)NULL; +static char *log_prefix = (char *)NULL; void LOG(const char *format, ...) { va_list args; @@ -57,10 +57,10 @@ void LOG_HEX(const char *prefix, const void *buffer, const size_t size) { printf("\n"); } -void log_set_prefix(const char* prefix) { - log_prefix = (char*)prefix; +void log_set_prefix(const char *prefix) { + log_prefix = (char *)prefix; } void log_clear_prefix() { - log_prefix = (char*)NULL; + log_prefix = (char *)NULL; } \ No newline at end of file diff --git a/firmware/src/sgx/src/untrusted/log.h b/firmware/src/sgx/src/untrusted/log.h index 99f16a1d..14850e33 100644 --- a/firmware/src/sgx/src/untrusted/log.h +++ b/firmware/src/sgx/src/untrusted/log.h @@ -43,10 +43,10 @@ void LOG_HEX(const char *prefix, const void *buffer, const size_t size); /** * @brief Set a prefix for all logs - * + * * @param prefix the prefix to use for logs */ -void log_set_prefix(const char* prefix); +void log_set_prefix(const char *prefix); /** * @brief Clear any prefix set for logs diff --git a/firmware/src/sgx/src/untrusted/main.c b/firmware/src/sgx/src/untrusted/main.c index 56dfc1fd..d5b36adc 100644 --- a/firmware/src/sgx/src/untrusted/main.c +++ b/firmware/src/sgx/src/untrusted/main.c @@ -45,8 +45,7 @@ static struct argp_option options[] = { {"bind", 'b', "ADDRESS", 0, "Address to bind to", 0}, {"port", 'p', "PORT", 0, "Port to listen on", 0}, - {0} -}; + {0}}; // Argument definitions for argp struct arguments { @@ -91,7 +90,9 @@ static struct argp argp = { parse_opt, "ENCLAVE_PATH", "SGX powHSM", - NULL, NULL, NULL, + NULL, + NULL, + NULL, }; static void finalise_with(int exit_code) { diff --git a/lint-c b/lint-c index 65270971..6f35653f 100755 --- a/lint-c +++ b/lint-c @@ -12,11 +12,12 @@ if [[ $1 == "exec" ]]; then fi SRC_DIR="firmware/src" - SEARCH_DIRS="$SRC_DIR/ledger/signer $SRC_DIR/ledger/ui $SRC_DIR/tcpsigner $SRC_DIR/common $SRC_DIR/hal" + SEARCH_DIRS="$SRC_DIR/ledger/signer $SRC_DIR/ledger/ui $SRC_DIR/tcpsigner $SRC_DIR/common $SRC_DIR/hal $SRC_DIR/sgx" find $SEARCH_DIRS -name "*.[ch]" | \ egrep -v "(bigdigits|bigdtypes|keccak256)\.[ch]$" | \ egrep -v "firmware/src/ledger/ui/src/glyphs.[ch]" | \ + egrep -v "firmware/src/sgx/src/(trusted|untrusted)/hsm_([tu]|args).[ch]" | \ xargs clang-format-10 --style=file $CLANG_ARGS else # Script directory From 607394975d94493df355a417331ec13340332f15 Mon Sep 17 00:00:00 2001 From: Italo Sampaio <100376888+italo-sampaio@users.noreply.github.com> Date: Fri, 17 Jan 2025 10:20:21 -0300 Subject: [PATCH 06/12] Sets uninitialized socket file descriptors to -1 (#248) Sets the value of uninitialized file descriptors on io.c to -1 instead of 0. --- firmware/src/sgx/src/untrusted/io.c | 45 ++++++++++++++--------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/firmware/src/sgx/src/untrusted/io.c b/firmware/src/sgx/src/untrusted/io.c index b5dd84b4..deb02a78 100644 --- a/firmware/src/sgx/src/untrusted/io.c +++ b/firmware/src/sgx/src/untrusted/io.c @@ -43,6 +43,13 @@ int serverfd; int connfd; struct sockaddr_in servaddr, cliaddr; +static void close_and_reset_fd(int *fd) { + if (fd && (*fd != -1)) { + close(*fd); + *fd = -1; + } +} + static int start_server(int port, const char *host) { int sockfd; struct hostent *hostinfo; @@ -50,14 +57,14 @@ static int start_server(int port, const char *host) { if (hostinfo == NULL) { LOG("Host not found.\n"); - return 0; + return -1; } // socket create and verification sockfd = socket(AF_INET, SOCK_STREAM, 0); if (sockfd == -1) { LOG("Socket creation failed...\n"); - return 0; + return -1; } explicit_bzero(&servaddr, sizeof(servaddr)); @@ -65,12 +72,12 @@ static int start_server(int port, const char *host) { if (setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &(int){1}, sizeof(int)) < 0) { LOG("Socket option setting failed failed\n"); - return 0; + return -1; } if (setsockopt(sockfd, SOL_TCP, TCP_NODELAY, &(int){1}, sizeof(int)) < 0) { LOG("Socket option setting failed failed\n"); - return 0; + return -1; } // Set address and port @@ -81,13 +88,13 @@ static int start_server(int port, const char *host) { // Binding newly created socket to given IP and verification if ((bind(sockfd, (struct sockaddr *)&servaddr, sizeof(servaddr))) != 0) { LOG("Socket bind failed...\n"); - return 0; + return -1; } // Now server is ready to listen and verification if ((listen(sockfd, 5)) != 0) { LOG("Listen failed...\n"); - return 0; + return -1; } LOG("Server listening...\n"); @@ -107,20 +114,14 @@ static bool accept_connection() { } bool io_init(int port, const char *host) { - connfd = 0; + connfd = -1; serverfd = start_server(port, host); - return serverfd; + return (serverfd != -1); } void io_finalise() { - if (connfd) { - close(connfd); - connfd = 0; - } - if (serverfd) { - close(serverfd); - serverfd = 0; - } + close_and_reset_fd(&connfd); + close_and_reset_fd(&serverfd); } unsigned short io_exchange(unsigned short tx) { @@ -129,7 +130,7 @@ unsigned short io_exchange(unsigned short tx) { int readlen; while (true) { - if (!connfd) { + if (connfd == -1) { if (!accept_connection()) { LOG("Error accepting client connection\n"); return 0; @@ -146,13 +147,13 @@ unsigned short io_exchange(unsigned short tx) { tx_net = htonl(tx_net); if (send(connfd, &tx_net, sizeof(tx_net), MSG_NOSIGNAL) == -1) { LOG("Connection closed by the client\n"); - connfd = 0; + close_and_reset_fd(&connfd); continue; } // Write APDU if (send(connfd, io_apdu_buffer, tx, MSG_NOSIGNAL) == -1) { LOG("Connection closed by the client\n"); - connfd = 0; + close_and_reset_fd(&connfd); continue; } LOG_HEX("I/O =>", io_apdu_buffer, tx); @@ -172,8 +173,7 @@ unsigned short io_exchange(unsigned short tx) { "Disconnected\n", readlen, rx); - close(connfd); - connfd = 0; + close_and_reset_fd(&connfd); continue; } LOG_HEX("I/O <=", io_apdu_buffer, rx); @@ -196,7 +196,6 @@ unsigned short io_exchange(unsigned short tx) { readlen, sizeof(rx_net)); } - close(connfd); - connfd = 0; + close_and_reset_fd(&connfd); } } From bebff196a51fb87e5a3284105bd7572ce367561d Mon Sep 17 00:00:00 2001 From: Italo Sampaio <100376888+italo-sampaio@users.noreply.github.com> Date: Fri, 17 Jan 2025 16:06:24 -0300 Subject: [PATCH 07/12] Removes .gitignore from distribution builds (#264) --- build-dist-ledger | 1 + build-dist-sgx | 1 + 2 files changed, 2 insertions(+) diff --git a/build-dist-ledger b/build-dist-ledger index c62d0297..22959902 100755 --- a/build-dist-ledger +++ b/build-dist-ledger @@ -35,6 +35,7 @@ echo -e "\e[32mBuilding into \e[93m$DEST_DIR\e[32m with checkpoint \e[93m$CHECKP echo -e "\e[33mCopying files and creating directories...\e[0m" rm -rf $DEST_DIR cp -Rf $ROOT_DIR/dist/ledger $DEST_DIR +rm $DEST_DIR/.gitignore rm -rf $FIRMWARE_DIR mkdir -p $FIRMWARE_DIR diff --git a/build-dist-sgx b/build-dist-sgx index 31052197..61f347a2 100755 --- a/build-dist-sgx +++ b/build-dist-sgx @@ -33,6 +33,7 @@ echo -e "\e[32mBuilding into \e[93m$DEST_DIR\e[32m with checkpoint \e[93m$CHECKP echo -e "\e[33mCopying files and creating directories...\e[0m" rm -rf $DEST_DIR cp -Rf $ROOT_DIR/dist/sgx $DEST_DIR +rm $DEST_DIR/.gitignore rm -rf $BIN_DIR mkdir -p $BIN_DIR From 21f3b80d219c1de46bdaf38e2e034a02e3bf7944 Mon Sep 17 00:00:00 2001 From: Italo Sampaio <100376888+italo-sampaio@users.noreply.github.com> Date: Mon, 20 Jan 2025 10:00:32 -0300 Subject: [PATCH 08/12] Fixes signature for finalise function (#249) Fixes the signature of finalise function so that it conforms with the expected signal handler function signature --- firmware/src/sgx/src/untrusted/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/firmware/src/sgx/src/untrusted/main.c b/firmware/src/sgx/src/untrusted/main.c index d5b36adc..a2d2c1af 100644 --- a/firmware/src/sgx/src/untrusted/main.c +++ b/firmware/src/sgx/src/untrusted/main.c @@ -103,7 +103,9 @@ static void finalise_with(int exit_code) { exit(exit_code); } -static void finalise() { +static void finalise(int signum) { + (void) signum; // Suppress unused parameter warning + finalise_with(0); } From 6359eed20745b7e4f403e50deecf106952d6689b Mon Sep 17 00:00:00 2001 From: Italo Sampaio <100376888+italo-sampaio@users.noreply.github.com> Date: Mon, 20 Jan 2025 11:46:39 -0300 Subject: [PATCH 09/12] Fixes formatting error identified by C linter (#266) --- firmware/src/sgx/src/untrusted/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firmware/src/sgx/src/untrusted/main.c b/firmware/src/sgx/src/untrusted/main.c index a2d2c1af..222c7add 100644 --- a/firmware/src/sgx/src/untrusted/main.c +++ b/firmware/src/sgx/src/untrusted/main.c @@ -104,7 +104,7 @@ static void finalise_with(int exit_code) { } static void finalise(int signum) { - (void) signum; // Suppress unused parameter warning + (void)signum; // Suppress unused parameter warning finalise_with(0); } From ed9b51b12c915256588439f8f4d9173a0a6b28f2 Mon Sep 17 00:00:00 2001 From: Italo Sampaio <100376888+italo-sampaio@users.noreply.github.com> Date: Mon, 20 Jan 2025 17:35:54 -0300 Subject: [PATCH 10/12] Sanitizes key for kvstore (#247) - Sanitizes the key before using it for file operations. - Added unit tests for keyvalue_store module --- .../src/sgx/src/untrusted/keyvalue_store.c | 39 ++- .../src/sgx/src/untrusted/keyvalue_store.h | 3 + firmware/src/sgx/test/common/common.mk | 4 +- firmware/src/sgx/test/keyvalue_store/Makefile | 38 +++ .../test/keyvalue_store/test_keyvalue_store.c | 282 ++++++++++++++++++ firmware/src/sgx/test/run-all.sh | 2 +- 6 files changed, 363 insertions(+), 5 deletions(-) create mode 100644 firmware/src/sgx/test/keyvalue_store/Makefile create mode 100644 firmware/src/sgx/test/keyvalue_store/test_keyvalue_store.c diff --git a/firmware/src/sgx/src/untrusted/keyvalue_store.c b/firmware/src/sgx/src/untrusted/keyvalue_store.c index a1058253..b68d6ce2 100644 --- a/firmware/src/sgx/src/untrusted/keyvalue_store.c +++ b/firmware/src/sgx/src/untrusted/keyvalue_store.c @@ -23,19 +23,52 @@ */ #include -#include "hsm_u.h" +#include +#include +#include #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(KVSTORE_PREFIX) + strlen(KVSTORE_SUFFIX) + 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; } diff --git a/firmware/src/sgx/src/untrusted/keyvalue_store.h b/firmware/src/sgx/src/untrusted/keyvalue_store.h index 872a60a5..f54de20b 100644 --- a/firmware/src/sgx/src/untrusted/keyvalue_store.h +++ b/firmware/src/sgx/src/untrusted/keyvalue_store.h @@ -25,6 +25,9 @@ #ifndef __KEYVALUE_STORE_H #define __KEYVALUE_STORE_H +#include +#include + /** * @brief Tell whether a given key currently exists * diff --git a/firmware/src/sgx/test/common/common.mk b/firmware/src/sgx/test/common/common.mk index fbf7a6b1..7d43cd52 100644 --- a/firmware/src/sgx/test/common/common.mk +++ b/firmware/src/sgx/test/common/common.mk @@ -1,5 +1,6 @@ TESTCOMMONDIR = ../common SGXTRUSTEDDIR = ../../src/trusted +SGXUNTRUSTEDDIR = ../../src/untrusted HALINCDIR = ../../../hal/include HALSGXSRCDIR = ../../../hal/sgx/src/trusted POWHSMSRCDIR = ../../../powhsm/src @@ -7,13 +8,14 @@ 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 diff --git a/firmware/src/sgx/test/keyvalue_store/Makefile b/firmware/src/sgx/test/keyvalue_store/Makefile new file mode 100644 index 00000000..705a9078 --- /dev/null +++ b/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) diff --git a/firmware/src/sgx/test/keyvalue_store/test_keyvalue_store.c b/firmware/src/sgx/test/keyvalue_store/test_keyvalue_store.c new file mode 100644 index 00000000..483d5ed3 --- /dev/null +++ b/firmware/src/sgx/test/keyvalue_store/test_keyvalue_store.c @@ -0,0 +1,282 @@ +/** + * 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 +#include +#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; +} \ No newline at end of file diff --git a/firmware/src/sgx/test/run-all.sh b/firmware/src/sgx/test/run-all.sh index e69b07d7..e54a9360 100755 --- a/firmware/src/sgx/test/run-all.sh +++ b/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..." From 353c4cf1962d7802847dab835f0fab75cc60feb3 Mon Sep 17 00:00:00 2001 From: Ariel Mendelzon Date: Wed, 22 Jan 2025 01:19:03 +1300 Subject: [PATCH 11/12] Added APDU buffer pointer validation to SGX enclave init sequence (#267) - Using oe_is_outside_enclave to validate the APDU buffer in system_init - Added and updated unit tests cases --- firmware/src/sgx/src/trusted/system.c | 9 +++ firmware/src/sgx/test/common/common.mk | 3 +- .../src/sgx/test/common/openenclave/enclave.h | 27 ++++++++ .../test/keyvalue_store/test_keyvalue_store.c | 69 ++++++++++--------- firmware/src/sgx/test/system/test_system.c | 36 +++++++++- 5 files changed, 108 insertions(+), 36 deletions(-) create mode 100644 firmware/src/sgx/test/common/openenclave/enclave.h diff --git a/firmware/src/sgx/src/trusted/system.c b/firmware/src/sgx/src/trusted/system.c index 9bb65808..114ee4bd 100644 --- a/firmware/src/sgx/src/trusted/system.c +++ b/firmware/src/sgx/src/trusted/system.c @@ -1,4 +1,5 @@ #include +#include #include "hal/constants.h" #include "hal/communication.h" @@ -166,6 +167,14 @@ bool system_init(unsigned char* msg_buffer, size_t msg_buffer_size) { msg_buffer_size); return false; } + + // Validate that the APDU buffer is entirely outside the enclave + // memory space + if (!oe_is_outside_enclave(msg_buffer, msg_buffer_size)) { + LOG("APDU buffer memory area not outside the enclave\n"); + return false; + } + apdu_buffer = msg_buffer; apdu_buffer_size = msg_buffer_size; diff --git a/firmware/src/sgx/test/common/common.mk b/firmware/src/sgx/test/common/common.mk index 7d43cd52..3b10a292 100644 --- a/firmware/src/sgx/test/common/common.mk +++ b/firmware/src/sgx/test/common/common.mk @@ -6,7 +6,8 @@ HALSGXSRCDIR = ../../../hal/sgx/src/trusted POWHSMSRCDIR = ../../../powhsm/src COMMONDIR = ../../../common/src -CFLAGS = -iquote $(TESTCOMMONDIR) +CFLAGS = -Wall -Wextra -Werror -Wno-unused-parameter -Wno-unused-function +CFLAGS += -I $(TESTCOMMONDIR) CFLAGS += -iquote $(SGXTRUSTEDDIR) CFLAGS += -iquote $(SGXUNTRUSTEDDIR) CFLAGS += -iquote $(HALINCDIR) diff --git a/firmware/src/sgx/test/common/openenclave/enclave.h b/firmware/src/sgx/test/common/openenclave/enclave.h new file mode 100644 index 00000000..87b7f344 --- /dev/null +++ b/firmware/src/sgx/test/common/openenclave/enclave.h @@ -0,0 +1,27 @@ +/** + * 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 + +bool oe_is_outside_enclave(const void *ptr, size_t size); diff --git a/firmware/src/sgx/test/keyvalue_store/test_keyvalue_store.c b/firmware/src/sgx/test/keyvalue_store/test_keyvalue_store.c index 483d5ed3..87ff9ae6 100644 --- a/firmware/src/sgx/test/keyvalue_store/test_keyvalue_store.c +++ b/firmware/src/sgx/test/keyvalue_store/test_keyvalue_store.c @@ -89,24 +89,23 @@ void test_save_retrieve() { 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"}}; + char* data; + } input_data[] = {{"a-key", "some piece of data"}, + {"another-key", "another piece of data"}, + {"yet-another-key", "yet another piece of data"}, + {"the-last-key", "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)); + (uint8_t*)input_data[i].data, + strlen(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)); + (uint8_t*)input_data[i].data, + strlen(input_data[i].data)); } } @@ -116,11 +115,11 @@ void test_kvstore_exists() { struct { char* key; - uint8_t* data; + char* 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"}, + {"first-key", "some piece of data"}, + {"second-key", "another piece of data"}, + {"third-key", "yet another piece of data"}, }; size_t num_existing_keys = sizeof(existing_keys) / sizeof(existing_keys[0]); @@ -134,8 +133,8 @@ void test_kvstore_exists() { 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)); + (uint8_t*)existing_keys[i].data, + strlen(existing_keys[i].data)); } for (size_t i = 0; i < num_existing_keys; i++) { @@ -153,23 +152,23 @@ void test_save_remove() { struct { char* key; - uint8_t* data; + char* 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}, + {"first-key", "some piece of data", false}, + {"second-key", "another piece of data", true}, + {"third-key", "yet another piece of data", true}, + {"fourth-key", "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)); + (uint8_t*)input_data[i].data, + strlen(input_data[i].data)); assert_key_value(input_data[i].key, - input_data[i].data, - strlen((char*)input_data[i].data)); + (uint8_t*)input_data[i].data, + strlen(input_data[i].data)); } // Remove selected keys @@ -185,8 +184,8 @@ void test_save_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)); + (uint8_t*)input_data[i].data, + strlen(input_data[i].data)); } } } @@ -197,7 +196,7 @@ void test_filename() { struct { char* key; - uint8_t* data; + char* data; char* filename; } input_data[] = { {"first-key", "data for the first key", "kvstore-first-key.dat"}, @@ -232,7 +231,7 @@ void test_sanitize_key() { struct { char* key; char* filename; - uint8_t* data; + char* data; } input_data[] = { {"onlyletters", "kvstore-onlyletters.dat", "data1"}, {"123456", "kvstore-123456.dat", "data2"}, @@ -257,18 +256,20 @@ void test_sanitize_key() { // 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)); + 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, - input_data[i].data, + (uint8_t*)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)); + assert_key_value(input_data[i].key, + (uint8_t*)input_data[i].data, + strlen(input_data[i].data)); } } diff --git a/firmware/src/sgx/test/system/test_system.c b/firmware/src/sgx/test/system/test_system.c index b0c0b760..aec1b7c3 100644 --- a/firmware/src/sgx/test/system/test_system.c +++ b/firmware/src/sgx/test/system/test_system.c @@ -64,6 +64,7 @@ typedef struct mock_calls_counter { int nvmem_init_count; int nvmem_register_block_count; int sest_init_count; + int oe_is_outside_enclave_count; } mock_calls_counter_t; typedef struct nvmem_register_block_args { @@ -106,6 +107,7 @@ typedef struct mock_force_fail { bool endorsement_init; bool nvmem_register_block; bool sest_init; + bool oe_is_outside_enclave; } mock_force_fail_t; typedef struct mock_data { @@ -166,6 +168,11 @@ try_context_t* G_try_last_open_context = &G_try_last_open_context_var; unsigned char G_io_apdu_buffer[IO_APDU_BUFFER_SIZE]; // Mock implementation of dependencies +bool oe_is_outside_enclave(const void* ptr, size_t size) { + MOCK_CALL(oe_is_outside_enclave); + return true; +} + void hsm_init() { NUM_CALLS(hsm_init)++; } @@ -351,6 +358,7 @@ void test_init_success() { printf("Test system_init success...\n"); assert(system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer))); + assert(NUM_CALLS(oe_is_outside_enclave) == 1); assert(NUM_CALLS(sest_init) == 1); assert(NUM_CALLS(access_init) == 1); assert(NUM_CALLS(seed_init) == 1); @@ -381,6 +389,23 @@ void test_init_fails_invalid_buf_size() { printf("Test system_init fails with invalid buffer size...\n"); assert(!system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer) - 1)); + ASSERT_NOT_CALLED(oe_is_outside_enclave); + ASSERT_NOT_CALLED(sest_init); + ASSERT_NOT_CALLED(access_init); + ASSERT_NOT_CALLED(seed_init); + ASSERT_NOT_CALLED(communication_init); + ASSERT_NOT_CALLED(endorsement_init); + ASSERT_NOT_CALLED(nvmem_init); + teardown(); +} + +void test_init_fails_invalid_buf_memarea() { + setup(); + printf("Test system_init fails with invalid buffer memory area...\n"); + + FORCE_FAIL(oe_is_outside_enclave, true); + assert(!system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer))); + assert(NUM_CALLS(oe_is_outside_enclave) == 1); ASSERT_NOT_CALLED(sest_init); ASSERT_NOT_CALLED(access_init); ASSERT_NOT_CALLED(seed_init); @@ -396,6 +421,7 @@ void test_init_fails_when_sest_init_fails() { FORCE_FAIL(sest_init, true); assert(!system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer))); + assert(NUM_CALLS(oe_is_outside_enclave) == 1); assert(NUM_CALLS(sest_init) == 1); ASSERT_NOT_CALLED(access_init); ASSERT_NOT_CALLED(seed_init); @@ -411,6 +437,7 @@ void test_init_fails_when_access_init_fails() { FORCE_FAIL(access_init, true); assert(!system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer))); + assert(NUM_CALLS(oe_is_outside_enclave) == 1); assert(NUM_CALLS(sest_init) == 1); assert(NUM_CALLS(access_init) == 1); ASSERT_NOT_CALLED(seed_init); @@ -426,6 +453,7 @@ void test_init_fails_when_seed_init_fails() { FORCE_FAIL(seed_init, true); assert(!system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer))); + assert(NUM_CALLS(oe_is_outside_enclave) == 1); assert(NUM_CALLS(sest_init) == 1); assert(NUM_CALLS(access_init) == 1); assert(NUM_CALLS(seed_init) == 1); @@ -441,6 +469,7 @@ void test_init_fails_when_communication_init_fails() { FORCE_FAIL(communication_init, true); assert(!system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer))); + assert(NUM_CALLS(oe_is_outside_enclave) == 1); assert(NUM_CALLS(sest_init) == 1); assert(NUM_CALLS(access_init) == 1); assert(NUM_CALLS(seed_init) == 1); @@ -456,6 +485,7 @@ void test_init_fails_when_endorsement_init_fails() { FORCE_FAIL(endorsement_init, true); assert(!system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer))); + assert(NUM_CALLS(oe_is_outside_enclave) == 1); assert(NUM_CALLS(sest_init) == 1); assert(NUM_CALLS(access_init) == 1); assert(NUM_CALLS(seed_init) == 1); @@ -471,7 +501,7 @@ void test_init_fails_when_nvmem_register_block_fails() { FORCE_NVMEM_FAIL_ON_KEY("bcstate"); assert(!system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer))); - assert(NUM_CALLS(sest_init) == 1); + assert(NUM_CALLS(oe_is_outside_enclave) == 1); assert(NUM_CALLS(sest_init) == 1); assert(NUM_CALLS(access_init) == 1); assert(NUM_CALLS(seed_init) == 1); @@ -487,6 +517,8 @@ void test_init_fails_when_nvmem_register_block_fails() { FORCE_NVMEM_FAIL_ON_KEY("bcstate_updating"); assert(!system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer))); + assert(NUM_CALLS(oe_is_outside_enclave) == 2); + assert(NUM_CALLS(sest_init) == 2); assert(NUM_CALLS(access_init) == 2); assert(NUM_CALLS(seed_init) == 2); assert(NUM_CALLS(communication_init) == 2); @@ -513,6 +545,7 @@ void test_init_fails_when_nvmem_load_fails() { FORCE_FAIL(nvmem_load, true); assert(!system_init(G_io_apdu_buffer, sizeof(G_io_apdu_buffer))); + assert(NUM_CALLS(oe_is_outside_enclave) == 1); assert(NUM_CALLS(sest_init) == 1); assert(NUM_CALLS(access_init) == 1); assert(NUM_CALLS(seed_init) == 1); @@ -958,6 +991,7 @@ void test_invalid_cmd_not_handled() { int main() { test_init_success(); test_init_fails_invalid_buf_size(); + test_init_fails_invalid_buf_memarea(); test_init_fails_when_sest_init_fails(); test_init_fails_when_access_init_fails(); test_init_fails_when_seed_init_fails(); From 5e63f11fab2dd3c59f100ced8d65c494ca7e6f57 Mon Sep 17 00:00:00 2001 From: Italo Sampaio <100376888+italo-sampaio@users.noreply.github.com> Date: Tue, 21 Jan 2025 10:25:52 -0300 Subject: [PATCH 12/12] Moves finalise logic out of signal handler (#268) Signal handler now only sets a flag that is checked in main --- firmware/src/sgx/src/untrusted/main.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/firmware/src/sgx/src/untrusted/main.c b/firmware/src/sgx/src/untrusted/main.c index 222c7add..25a0e4df 100644 --- a/firmware/src/sgx/src/untrusted/main.c +++ b/firmware/src/sgx/src/untrusted/main.c @@ -54,6 +54,9 @@ struct arguments { char *enclave_path; }; +// Global flag to indicate that the application should stop +static sig_atomic_t G_stop_requested = 0; + // Argp individual option parsing function static error_t parse_opt(int key, char *arg, struct argp_state *state) { struct arguments *arguments = state->input; @@ -106,7 +109,9 @@ static void finalise_with(int exit_code) { static void finalise(int signum) { (void)signum; // Suppress unused parameter warning - finalise_with(0); + // Note: Do not add any finalise logic directly here, just set the flag + // and let the main loop handle it + G_stop_requested = 1; } static void set_signal_handlers() { @@ -157,6 +162,10 @@ int main(int argc, char **argv) { unsigned int tx = 0; while (true) { + if (G_stop_requested) { + break; + } + rx = io_exchange(tx); if (rx) { @@ -164,9 +173,11 @@ int main(int argc, char **argv) { } } - LOG("Exited main loop unexpectedly\n"); + finalise_with(0); + return 0; main_error: + LOG("Exited main loop unexpectedly\n"); finalise_with(1); return 1; }