Merge pull request #6 from LedgerHQ/security-review

Security review

.github/workflows/codeql-analysis.yml

@@ -0,0 +1,50 @@
+name: "CodeQL"
+
+on:
+  push:
+    branches:
+      - develop
+  pull_request:
+    branches:
+      - develop
+    paths-ignore:
+      - '.github/workflows/*.yml'
+      - 'tests/*'
+
+jobs:
+  analyse:
+    name: Analyse
+    runs-on: ubuntu-latest
+    container:
+      image: ghcr.io/ledgerhq/ledger-app-builder/ledger-app-builder:latest
+    permissions:
+      actions: read
+      contents: read
+      security-events: write
+
+    strategy: 
+      matrix:
+        sdk: [ "$NANOS_SDK", "$NANOX_SDK", "$NANOSP_SDK" ]
+        #'cpp' covers C and C++
+        language: [ 'cpp' ]
+
+    steps:
+      - name: Adding GitHub workspace as safe directory
+        run: git config --global --add safe.directory $GITHUB_WORKSPACE
+
+      - name: Clone
+        uses: actions/checkout@v3
+
+      - name: Initialize CodeQL
+        uses: github/codeql-action/init@v2
+        with:
+          languages: ${{ matrix.language }}
+          queries: security-and-quality
+
+      # CodeQL will create the database during the compilation
+      - name: Build
+        run: |
+          make BOLOS_SDK=${{ matrix.sdk }}
+        
+      - name: Perform CodeQL Analysis
+        uses: github/codeql-action/analyze@v2

.gitignore

@@ -35,4 +35,10 @@ doc/latex
 # Bolos dev env
 dev-env
 
-**/node_modules
+**/node_modules
+
+# Fuzzer
+fuzzing/cmake-build-fuzz/
+fuzzing/cmake-build-fuzz-coverage/
+fuzzing/corpus/
+fuzzing/html-coverage/

Makefile

@@ -22,7 +22,11 @@ endif
 include $(BOLOS_SDK)/Makefile.defines
 
 APP_LOAD_PARAMS  = --curve secp256k1
-APP_LOAD_PARAMS += --appFlags 0x240
+ifeq ($(TARGET_NAME),TARGET_NANOX)
+APP_LOAD_PARAMS += --appFlags 0x200
+else
+APP_LOAD_PARAMS += --appFlags 0x000
+endif
 APP_LOAD_PARAMS += --path "48'/13'"
 APP_LOAD_PARAMS += $(COMMON_LOAD_PARAMS)
 

fuzzing/CMakeLists.txt

@@ -0,0 +1,99 @@
+cmake_minimum_required(VERSION 3.10)
+
+project(HiveFuzzer VERSION 0.0.1 LANGUAGES C)
+
+set(CMAKE_C_STANDARD 11)
+set(CMAKE_BUILD_TYPE RelWithDebInfo)
+
+# BOLOS SDK
+set(BOLOS_SDK $ENV{BOLOS_SDK})
+add_compile_definitions(APP_LOAD_PARAMS="--curve secp256k1")
+
+add_compile_definitions(
+  APPNAME="HIVE" 
+  HAVE_SECP256K1_CURVE
+  CX_CURVE_256K1=0x21
+  APPVERSION="1.0.1"
+  MAJOR_VERSION=1
+  MINOR_VERSION=0
+  PATCH_VERSION=1
+  OS_IO_SEPROXYHAL
+  HAVE_BAGL
+  HAVE_UX_FLOW
+  HAVE_SPRINTF
+  HAVE_IO_USB
+  HAVE_L4_USBLIB
+  IO_USB_MAX_ENDPOINTS=6
+  IO_HID_EP_LENGTH=64
+  HAVE_USB_APDU
+  USB_SEGMENT_SIZE=64
+  BLE_SEGMENT_SIZE=32
+  HAVE_WEBUSB
+  WEBUSB_URL_SIZE_B=0
+  WEBUSB_URL=""
+  IO_SEPROXYHAL_BUFFER_SIZE_B=128
+  HAVE_SHA224
+  HAVE_HASH HAVE_RIPEMD160
+  HAVE_ECC
+  BAGL_WIDTH=128
+  BAGL_HEIGHT=64)
+
+add_link_options(-Wl,--wrap,cx_ripemd160_init_no_throw -Wl,--wrap,pic -Wl,--wrap,os_longjmp -Wl,--wrap,cx_hash_no_throw -Wl,--wrap,cx_hash_get_size -Wl,--wrap,cx_ecdsa_sign) 
+
+include_directories(.
+        ../src/
+        "${BOLOS_SDK}/include"
+        "${BOLOS_SDK}/lib_bagl/include"
+        "${BOLOS_SDK}/lib_cxng/include"
+        "${BOLOS_SDK}/lib_stusb/include"
+        "${BOLOS_SDK}/lib_ux/include"
+)
+
+add_compile_options(-g -O3)
+
+# Build with code coverage generation
+if(CODE_COVERAGE)
+    if(CMAKE_C_COMPILER_ID MATCHES "(Apple)?[Cc]lang")
+        add_compile_options(-fprofile-instr-generate -fcoverage-mapping)
+        add_link_options(-fprofile-instr-generate -fcoverage-mapping)
+    elseif(CMAKE_C_COMPILER_ID MATCHES "GNU")
+        add_compile_options(-fprofile-arcs -ftest-coverage)
+        link_libraries(gcov)
+    else()
+        message(FATAL_ERROR "Unsupported compiler used with code coverage generation")
+    endif()
+endif()
+
+# Fuzzer target
+set(APP_SRC_DIR "../src")
+
+set(APP_SOURCES
+    ${APP_SRC_DIR}/globals.c
+    #${APP_SRC_DIR}/crypto.c
+    #${APP_SRC_DIR}/apdu/dispatcher.c
+    #${APP_SRC_DIR}/apdu/parser.c
+    #${APP_SRC_DIR}/handler/get_app_name.c
+    #${APP_SRC_DIR}/handler/get_public_key.c
+    #${APP_SRC_DIR}/handler/get_version.c
+    #${APP_SRC_DIR}/handler/sign_tx.c
+    ${APP_SRC_DIR}/transaction/decoders.c
+    ${APP_SRC_DIR}/transaction/parsers.c
+    ${APP_SRC_DIR}/transaction/transaction_parse.c
+    ${APP_SRC_DIR}/common/asn1.c
+    ${APP_SRC_DIR}/common/base58.c
+    ${APP_SRC_DIR}/common/bip32.c
+    ${APP_SRC_DIR}/common/buffer.c
+    ${APP_SRC_DIR}/common/format.c
+    ${APP_SRC_DIR}/common/read.c
+    ${APP_SRC_DIR}/common/wif.c
+)
+
+add_executable(fuzz_hive
+        fuzz_hive.c
+        mocks.c
+        ${APP_SOURCES}
+)
+
+target_include_directories(fuzz_hive PUBLIC ../src)
+target_compile_options(fuzz_hive PUBLIC -fsanitize=fuzzer,address,undefined -fno-sanitize-recover=undefined)
+target_link_options(fuzz_hive PUBLIC -fsanitize=fuzzer,address,undefined -fno-sanitize-recover=undefined)

fuzzing/build.sh

@@ -0,0 +1,14 @@
+#!/bin/bash
+set -e
+
+SCRIPTDIR="$(cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd)"
+BUILDDIR="$SCRIPTDIR/cmake-build-fuzz"
+
+# Compile fuzzer
+rm -rf "$BUILDDIR"
+mkdir "$BUILDDIR"
+cd "$BUILDDIR"
+
+cmake -DCMAKE_C_COMPILER=clang ..
+make clean
+make fuzz_hive

fuzzing/coverage.sh

@@ -0,0 +1,23 @@
+#!/usr/bin/env bash
+# Generate code coverage reports from fuzzing results
+
+set -e
+
+SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+BUILDDIR="$SCRIPTDIR/cmake-build-fuzz-coverage"
+CORPUSDIR="$SCRIPTDIR/corpus"
+HTMLCOVDIR="$SCRIPTDIR/html-coverage"
+
+# Compile the fuzzer with code coverage support
+rm -rf "$BUILDDIR" "$HTMLCOVDIR"
+mkdir "$BUILDDIR"
+cd "$BUILDDIR"
+cmake -DCMAKE_C_COMPILER=clang -DCODE_COVERAGE=1 -B"$BUILDDIR" ..
+cmake --build "$BUILDDIR" --target fuzz_hive
+
+# Run the fuzzer on the corpus files
+export LLVM_PROFILE_FILE="$BUILDDIR/fuzz_hive.profraw"
+"$BUILDDIR/fuzz_hive" "$CORPUSDIR"/*
+llvm-profdata merge --sparse "$LLVM_PROFILE_FILE" -o "$BUILDDIR/fuzz_hive.profdata"
+llvm-cov show "$BUILDDIR/fuzz_hive" -instr-profile="$BUILDDIR/fuzz_hive.profdata" -show-line-counts-or-regions -output-dir="$HTMLCOVDIR" -format=html
+llvm-cov report "$BUILDDIR/fuzz_hive" -instr-profile="$BUILDDIR/fuzz_hive.profdata"

fuzzing/fuzz_hive.c

@@ -0,0 +1,71 @@
+#include "transaction/transaction_parse.h"
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include "globals.h"
+/*
+#include "apdu/parser.h"
+#include "apdu/dispatcher.h"
+#include "cx.h"
+*/
+
+int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
+    if (Size < 5) {
+        //mimic buf_len < OFFSET_CDATA in apdu_parser()
+        return 1;
+    }
+    BEGIN_TRY {
+        TRY {
+            uint8_t lc;
+
+            lc = Data[0];
+
+            explicit_bzero(&G_context, sizeof(G_context));
+            G_context.req_type = CONFIRM_TRANSACTION;
+            G_context.state = STATE_NONE;
+
+            G_context.tx_info.raw_tx_len = Size - 1 > MAX_TRANSACTION_LEN ? MAX_TRANSACTION_LEN : Size - 1;
+            memcpy(G_context.tx_info.raw_tx, Data + 1, G_context.tx_info.raw_tx_len);
+            buffer_t tx_buffer = {.offset = 0, .ptr = &G_context.tx_info.raw_tx, .size = lc};
+
+            transaction_parse(&tx_buffer);
+
+            lc = Data[0];
+
+            explicit_bzero(&G_context, sizeof(G_context));
+            G_context.req_type = CONFIRM_HASH;
+            G_context.state = STATE_NONE;
+
+            G_context.tx_info.raw_tx_len = Size - 1 > MAX_TRANSACTION_LEN ? MAX_TRANSACTION_LEN : Size - 1;
+            memcpy(G_context.tx_info.raw_tx, Data + 1, G_context.tx_info.raw_tx_len);
+            buffer_t hash_buffer = {.offset = 0, .ptr = &G_context.tx_info.raw_tx, .size = lc};
+
+            hash_parse(&hash_buffer);
+        }
+        CATCH_OTHER(e) {
+            return 0;
+        }
+        FINALLY {
+        }
+        END_TRY;
+    }
+
+    return 0;
+}
+
+/*
+int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
+    command_t cmd;
+    memset(&cmd, 0, sizeof(cmd));
+    if (!apdu_parser(&cmd, Data, Size)) {
+        return 0;
+    }
+
+    // Dispatch structured APDU command to handler
+    if (apdu_dispatcher(&cmd) < 0) {
+        return 0;
+    }
+    return 0;
+}
+*/

fuzzing/mocks.c

@@ -0,0 +1,36 @@
+#include "cx.h"
+
+void __wrap_os_longjmp(unsigned int exception) {
+    longjmp(try_context_get()->jmp_buf, exception);
+}
+
+try_context_t *current_context = NULL;
+try_context_t *try_context_get(void) {
+    return current_context;
+}
+
+try_context_t *try_context_set(try_context_t *ctx) {
+    try_context_t *previous_ctx = current_context;
+    current_context = ctx;
+    return previous_ctx;
+}
+
+void *__wrap_pic(void *link_address) {
+    return link_address;
+}
+
+cx_err_t __wrap_cx_hash_no_throw(cx_hash_t *hash, uint32_t mode, const uint8_t *in, size_t len, uint8_t *out, size_t out_len) {
+    return CX_OK;
+}
+
+size_t __wrap_cx_hash_get_size(int fd) {
+    return 32;
+}
+
+cx_err_t __wrap_cx_ripemd160_init_no_throw(cx_ripemd160_t *hash) {
+    return CX_OK;
+}
+
+int __wrap_cx_ecdsa_sign ( const cx_ecfp_private_key_t * pvkey, int mode, cx_md_t hashID, const unsigned char * hash, unsigned int hash_len, unsigned char * sig, unsigned int sig_len, unsigned int * info ) {
+    return CX_OK;
+}

fuzzing/run.sh

@@ -0,0 +1,9 @@
+#!/usr/bin/env bash
+
+set -e
+
+SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+BUILDDIR="$SCRIPTDIR/cmake-build-fuzz"
+CORPUSDIR="$SCRIPTDIR/corpus"
+
+"$BUILDDIR"/fuzz_hive "$CORPUSDIR" "$@" > /dev/null

src/common/bip32.c

@@ -32,7 +32,7 @@ bool bip32_path_read(const uint8_t *in, size_t in_len, uint32_t *out, size_t out
     size_t offset = 0;
 
     for (size_t i = 0; i < out_len; i++) {
-        if (offset > in_len) {
+        if (offset + 4 > in_len) {
             return false;
         }
         out[i] = read_u32_be(in, offset);

src/common/wif.c

@@ -54,9 +54,9 @@ bool wif_from_compressed_public_key(uint8_t compressed_key[static PUBKEY_COMPRES
     memset(out, 0, out_len);
     strncpy(out, "STM", out_len);
 
-    if (base58_encode(temp, sizeof(temp), out + 3, out_len) == -1) {
+    if (base58_encode(temp, sizeof(temp), out + 3, out_len - 3) == -1) {
         return false;
     }
 
     return true;
-}
+}

src/glyphs.h

@@ -1,3 +1,4 @@
+#pragma once
 #ifndef GLYPH_hive_logo_BPP
         #define GLYPH_hive_logo_WIDTH 16
         #define GLYPH_hive_logo_HEIGHT 16

src/handler/get_public_key.c

@@ -46,14 +46,23 @@ int handler_get_public_key(buffer_t *cdata, bool display) {
         return io_send_sw(SW_WRONG_BIP32_PATH);
     }
 
-    // derive private key according to BIP32 path
-    crypto_derive_private_key(&private_key, G_context.pk_info.chain_code, G_context.bip32_path, G_context.bip32_path_len);
+    BEGIN_TRY {
+        TRY {
+            // derive private key according to BIP32 path
+            crypto_derive_private_key(&private_key, G_context.pk_info.chain_code, G_context.bip32_path, G_context.bip32_path_len);
 
-    // generate corresponding public key
-    crypto_init_public_key(&private_key, &public_key, G_context.pk_info.raw_public_key);
-
-    // reset private key
-    explicit_bzero(&private_key, sizeof(private_key));
+            // generate corresponding public key
+            crypto_init_public_key(&private_key, &public_key, G_context.pk_info.raw_public_key);
+        }
+        CATCH_OTHER(e) {
+            THROW(e);
+        }
+        FINALLY {
+            // reset private key
+            explicit_bzero(&private_key, sizeof(private_key));
+        }
+    }
+    END_TRY;
 
     wif_from_public_key(G_context.pk_info.raw_public_key, sizeof(G_context.pk_info.raw_public_key), G_context.pk_info.wif, sizeof(G_context.pk_info.wif));