Merge pull request #66 from blooo-io/fix/LDG-678-audit-fix-v1

Fix/ldg 678 audit fix v1

.github/workflows/build_and_functional_tests.yml

@@ -16,7 +16,7 @@ on:
         type: choice
         required: true
         default: 'Raise an error (default)'
-        description: CI behavior if the test snaphots are different than expected.
+        description: CI behavior if the test snapshots are different than expected.
         options:
           - 'Raise an error (default)'
           - 'Open a PR'

.github/workflows/codeql_checks.yml

@@ -17,20 +17,21 @@ jobs:
   analyse:
     name: Analyse
     strategy:
+      fail-fast: false
       matrix:
         sdk: ["$NANOX_SDK", "$NANOSP_SDK", "$STAX_SDK", "$FLEX_SDK"]
-        #'cpp' covers C and C++
+        # 'cpp' covers C and C++
         language: ["cpp"]
     runs-on: ubuntu-latest
     container:
       image: ghcr.io/ledgerhq/ledger-app-builder/ledger-app-builder-legacy:latest
 
     steps:
       - name: Clone
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
 
       - name: Initialize CodeQL
-        uses: github/codeql-action/init@v2
+        uses: github/codeql-action/init@v3
         with:
           languages: ${{ matrix.language }}
           queries: security-and-quality
@@ -41,4 +42,4 @@ jobs:
           make BOLOS_SDK=${{ matrix.sdk }}
 
       - name: Perform CodeQL Analysis
-        uses: github/codeql-action/analyze@v2
+        uses: github/codeql-action/analyze@v3

.github/workflows/coding_style_checks.yml

@@ -4,7 +4,7 @@ name: Run coding style check through reusable workflow
 #
 # The presence of this workflow is mandatory as a minimal level of linting is required.
 # You are however free to modify the content of the .clang-format file and thus the coding style of your application.
-# We simply ask you to not diverge too much from the linting of the Concordium application.
+# We simply ask you to not diverge too much from the linting of the Boilerplate application.
 
 on:
   workflow_dispatch:
@@ -20,6 +20,6 @@ jobs:
     name: Check linting using the reusable workflow
     uses: LedgerHQ/ledger-app-workflows/.github/workflows/reusable_lint.yml@v1
     with:
-      source: './src'
-      extensions: 'h,c'
-      version: 11
+      source: "./src"
+      extensions: "h,c"
+      version: 12

.github/workflows/documentation_generation.yml

@@ -18,12 +18,12 @@ jobs:
 
     steps:
       - name: Clone
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
 
       - name: HTML documentation
         run: doxygen .doxygen/Doxyfile
 
-      - uses: actions/upload-artifact@v3
+      - uses: actions/upload-artifact@v4
         with:
           name: documentation
           path: doc/html

.github/workflows/misspellings_checks.yml

@@ -18,12 +18,12 @@ jobs:
     name: Check misspellings
     runs-on: ubuntu-latest
     steps:
-    - name: Clone
-      uses: actions/checkout@v3
+      - name: Clone
+        uses: actions/checkout@v4
 
-    - name: Check misspellings
-      uses: codespell-project/actions-codespell@v1
-      with:
-        builtin: clear,rare
-        check_filenames: true
-        skip: "*.ai,submission_graphics/*,./.git"
+      - name: Check misspellings
+        uses: codespell-project/actions-codespell@v2
+        with:
+          builtin: clear,rare
+          check_filenames: true
+          skip: "*.ai,submission_graphics/*,./.git"

.github/workflows/unit_tests.yml

@@ -17,12 +17,11 @@ jobs:
       image: ghcr.io/ledgerhq/ledger-app-builder/ledger-app-builder-lite:latest
 
     steps:
-
       - name: Clone
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
 
       - name: Clone SDK
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
         with:
           repository: ledgerHQ/ledger-secure-sdk
           path: sdk
@@ -42,19 +41,21 @@ jobs:
           lcov --directory . -b "$(realpath build/)" --remove coverage.info '*/unit-tests/*' -o coverage.info && \
           genhtml coverage.info -o coverage
 
-      - uses: actions/upload-artifact@v3
+      - uses: actions/upload-artifact@v4
         with:
           name: code-coverage
           path: unit-tests/coverage
 
+      - name: Install codecov dependencies
+        run: apk update && apk add curl gpg
+
       - name: Upload to codecov.io
-        uses: codecov/codecov-action@v3
+        uses: codecov/codecov-action@v5
+        env:
+          CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
         with:
-          token: ${{ secrets.CODECOV_TOKEN }}
           files: ./unit-tests/coverage.info
           flags: unittests
           name: codecov-app-concordium
           fail_ci_if_error: true
           verbose: true
-        env:
-          CODECOV_DEBUG: true

Makefile

@@ -30,7 +30,7 @@ APPNAME = "Concordium"
 # Application version
 APPVERSION_M = 5
 APPVERSION_N = 0
-APPVERSION_P = 1
+APPVERSION_P = 2
 APPVERSION = "$(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P)"
 
 DEFINES += APPVERSION=\"$(APPVERSION)\"

src/common/base58check.c

@@ -22,13 +22,8 @@
 
 #define MAX_ENC_INPUT_SIZE 120
 
-unsigned char const BASE58ALPHABET[] = {'1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C',
-                                        'D', 'E', 'F', 'G', 'H', 'J', 'K', 'L', 'M', 'N', 'P', 'Q',
-                                        'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z', 'a', 'b', 'c',
-                                        'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'm', 'n', 'o', 'p',
-                                        'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'};
-
 #define ADDRESS_LENGTH 32
+#define HASH_LENGTH    32
 
 int base58check_encode(const unsigned char *in, size_t length, unsigned char *out, size_t *outlen) {
     if (length != ADDRESS_LENGTH) {
@@ -47,9 +42,16 @@ int base58check_encode(const unsigned char *in, size_t length, unsigned char *ou
 
     // Calculate SHA256(SHA256(version + in)), and append the first 4 bytes to the (version + in)
     // bytes.
-    uint8_t hash[32];
-    cx_hash_sha256(buffer, ADDRESS_LENGTH + 1, hash, sizeof(hash));
-    cx_hash_sha256(hash, sizeof(hash), hash, sizeof(hash));
+    uint8_t hash[HASH_LENGTH];
+    cx_err_t error = 0;
+    error = cx_hash_sha256(buffer, ADDRESS_LENGTH + 1, hash, sizeof(hash));
+    if (error == 0) {
+        THROW(ERROR_FAILED_CX_OPERATION);
+    }
+    error = cx_hash_sha256(hash, sizeof(hash), hash, sizeof(hash));
+    if (error == 0) {
+        THROW(ERROR_FAILED_CX_OPERATION);
+    }
     memmove(&buffer[1 + ADDRESS_LENGTH], hash, 4);
 
     return base58_encode(buffer, 1 + ADDRESS_LENGTH + 4, (char *)out, *outlen);

src/common/getPublicKey.c

@@ -12,7 +12,7 @@ instructionContext global;
  * the APDU buffer to be returned to the caller.
  */
 void sendPublicKey(bool compare) {
-    uint8_t publicKey[32];
+    uint8_t publicKey[KEY_LENGTH];
     getPublicKey(publicKey);
 
     // tx is holding the offset in the buffer we have written to. It is a convention to call this tx

src/common/handler.c

@@ -10,57 +10,74 @@ int handler(uint8_t INS,
             bool isInitialCall) {
     switch (INS) {
         case INS_GET_PUBLIC_KEY:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleGetPublicKey(cdata, p1, p2, flags);
             break;
         case INS_VERIFY_ADDRESS:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleVerifyAddress(cdata, p1, flags);
             break;
         case INS_SIGN_TRANSFER:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleSignTransfer(cdata, flags);
             break;
         case INS_SIGN_TRANSFER_WITH_MEMO:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleSignTransferWithMemo(cdata, p1, lc, flags, isInitialCall);
             break;
         case INS_SIGN_TRANSFER_WITH_SCHEDULE:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleSignTransferWithSchedule(cdata, p1, flags, isInitialCall);
             break;
         case INS_SIGN_TRANSFER_WITH_SCHEDULE_AND_MEMO:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleSignTransferWithScheduleAndMemo(cdata, p1, lc, flags, isInitialCall);
             break;
         case INS_CREDENTIAL_DEPLOYMENT:
-            handleSignCredentialDeployment(cdata, p1, p2, flags, isInitialCall);
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
+            handleSignCredentialDeployment(cdata, p1, p2, lc, flags, isInitialCall);
             break;
         case INS_EXPORT_PRIVATE_KEY:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleExportPrivateKey(cdata, p1, p2, flags);
             break;
         case INS_TRANSFER_TO_PUBLIC:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleSignTransferToPublic(cdata, p1, lc, flags, isInitialCall);
             break;
         case INS_REGISTER_DATA:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleSignRegisterData(cdata, p1, lc, flags, isInitialCall);
             break;
         case INS_PUBLIC_INFO_FOR_IP:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleSignPublicInformationForIp(cdata, p1, flags, isInitialCall);
             break;
         case INS_CONFIGURE_BAKER:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleSignConfigureBaker(cdata, p1, lc, flags, isInitialCall);
             break;
         case INS_CONFIGURE_DELEGATION:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleSignConfigureDelegation(cdata, lc, flags);
             break;
         case INS_SIGN_UPDATE_CREDENTIAL:
-            handleSignUpdateCredential(cdata, p1, p2, flags, isInitialCall);
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
+            handleSignUpdateCredential(cdata, p1, p2, lc, flags, isInitialCall);
             break;
         case INS_GET_APP_NAME:
             return handleGetAppName();
             break;
         case INS_DEPLOY_MODULE:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleDeployModule(cdata, p1, lc);
             break;
         case INS_INIT_CONTRACT:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleInitContract(cdata, p1, lc);
             break;
         case INS_UPDATE_CONTRACT:
+            LEDGER_ASSERT(cdata != NULL, "NULL cdata");
             handleUpdateContract(cdata, p1, lc);
             break;
         default:

src/common/numberHelpers.c

@@ -61,7 +61,7 @@ size_t decimalDigitsDisplay(uint8_t *dst,
     size_t length = lengthOfNumber(decimalPart);
     int zeroFillLength = decimalDigitsLength - length;
 
-    if (dstLength - zeroFillLength < 0) {
+    if (zeroFillLength < 0 || dstLength < (size_t)zeroFillLength) {
         THROW(ERROR_BUFFER_OVERFLOW);
     }
 
@@ -185,13 +185,16 @@ size_t fractionToPercentageDisplay(uint8_t *dst, size_t dstLength, uint32_t numb
  * to relate to in the GUI.
  */
 size_t amountToGtuDisplay(uint8_t *dst, size_t dstLength, uint64_t microGtuAmount) {
+    if (dstLength < 4) return 0;  // Prevent overflow
     memmove(dst, "CCD ", 4);
     size_t offset = decimalNumberToDisplay(dst + 4, dstLength, microGtuAmount, 1000000, 6) + 4;
     dst[offset] = '\0';
     return offset + 1;
 }
 
 void toPaginatedHex(uint8_t *byteArray, const uint64_t len, char *asHex, const size_t asHexSize) {
+    LEDGER_ASSERT(byteArray != NULL, "NULL byteArray");
+
     static uint8_t const hex[] = "0123456789abcdef";
 
     if (asHexSize < len * 2 + len / 16 + 1) {

src/common/sign.c

@@ -1,5 +1,16 @@
 #include "globals.h"
 
+// CBOR encoding constants
+#define CBOR_SHORT_COUNT_MAX   23  // Values below this are direct length
+#define CBOR_ONE_BYTE_LENGTH   24  // Uses 1 additional byte for length
+#define CBOR_TWO_BYTE_LENGTH   25  // Uses 2 additional bytes for length
+#define CBOR_FOUR_BYTE_LENGTH  26  // Uses 4 additional bytes for length
+#define CBOR_EIGHT_BYTE_LENGTH 27  // Uses 8 additional bytes for length
+#define CBOR_INDEFINITE_LENGTH 31  // Indicates indefinite length encoding (unsupported)
+
+// Mask and bit shifts
+#define CBOR_SHORT_COUNT_MASK 0x1F  // 5 lower bits
+
 static tx_state_t *tx_state = &global_tx_state;
 static cborContext_t *ctx = &global.withDataBlob.cborContext;
 
@@ -14,22 +25,14 @@ void buildAndSignTransactionHash() {
     sendSuccess(sizeof(signedHash));
 }
 
-void handleCborStep(void) {
-    if (ctx->cborLength < 0) {
-        THROW(ERROR_INVALID_STATE);
-    } else {
-        sendSuccessNoIdle();  // Request more data from the computer.
-    }
-}
-
 void readCborInitial(uint8_t *cdata, uint8_t dataLength) {
     uint8_t header = cdata[0];
     cdata += 1;
     ctx->cborLength -= 1;
     // the first byte of an cbor encoding contains the type (3 high bits) and the shortCount (5
     // lower bits);
     ctx->majorType = header >> 5;
-    uint8_t shortCount = header & 0x1f;
+    uint8_t shortCount = header & CBOR_SHORT_COUNT_MASK;
 
     // Calculate length of cbor payload
     // sizeLength: number of bytes (beside the header) used to indicate the payload length
@@ -39,23 +42,22 @@ void readCborInitial(uint8_t *cdata, uint8_t dataLength) {
 
     ctx->displayUsed = 0;
 
-    if (shortCount < 24) {
+    if (shortCount <= CBOR_SHORT_COUNT_MAX) {
         // shortCount is the length, no extra bytes are used.
-        sizeLength = 0;
         length = shortCount;
-    } else if (shortCount == 24) {
+    } else if (shortCount == CBOR_ONE_BYTE_LENGTH) {
         length = cdata[0];
         sizeLength = 1;
-    } else if (shortCount == 25) {
+    } else if (shortCount == CBOR_TWO_BYTE_LENGTH) {
         length = U2BE(cdata, 0);
         sizeLength = 2;
-    } else if (shortCount == 26) {
+    } else if (shortCount == CBOR_FOUR_BYTE_LENGTH) {
         length = U4BE(cdata, 0);
         sizeLength = 4;
-    } else if (shortCount == 27) {
+    } else if (shortCount == CBOR_EIGHT_BYTE_LENGTH) {
         length = U8BE(cdata, 0);
         sizeLength = 8;
-    } else if (shortCount == 31) {
+    } else if (shortCount == CBOR_INDEFINITE_LENGTH) {
         THROW(ERROR_UNSUPPORTED_CBOR);
     } else {
         THROW(ERROR_INVALID_PARAM);

src/common/sign.h

@@ -22,6 +22,4 @@ void readCborInitial(uint8_t *cdata, uint8_t dataLength);
  */
 void readCborContent(uint8_t *cdata, uint8_t dataLength);
 
-void handleCborStep(void);
-
 void buildAndSignTransactionHash();