fix: fixed warnings and bugs raised by security audit

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

@@ -14,14 +14,6 @@ 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;

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();

src/common/time.c

@@ -66,7 +66,7 @@ int secondsToTm(long long t, tm *tm) {
 
     for (months = 0; days_in_month[months] <= remdays; months++) remdays -= days_in_month[months];
 
-    if (years + 100 > INT_MAX || years + 100 < INT_MIN) return -1;
+    if (years > INT_MAX - 100 || years < INT_MIN + 100) return -1;
 
     tm->tm_year = years + 100;
     tm->tm_mon = months + 2;
@@ -89,8 +89,11 @@ int secondsToTm(long long t, tm *tm) {
  * Helper function for prepending numbers that are
  * less than 10 with a '0', so that 5 results in 05.
  */
-int prefixWithZero(uint8_t *dst, int value) {
+int prefixWithZero(uint8_t *dst, size_t dstLength, int value) {
     if (value < 10) {
+        if (dstLength < 1) {
+            return -1;  // Prevent overflow
+        }
         memmove(dst, "0", 1);
         return 1;
     }
@@ -105,31 +108,31 @@ int timeToDisplayText(tm time, uint8_t *dst, size_t dstLength) {
     memmove(dst + offset, "-", 1);
     offset += 1;
 
-    offset += prefixWithZero(dst + offset, time.tm_mon + 1);
+    offset += prefixWithZero(dst + offset, dstLength - offset, time.tm_mon + 1);
     offset += numberToText(dst + offset, dstLength - offset, time.tm_mon + 1);
 
     memmove(dst + offset, "-", 1);
     offset += 1;
 
-    offset += prefixWithZero(dst + offset, time.tm_mday);
+    offset += prefixWithZero(dst + offset, dstLength - offset, time.tm_mday);
     offset += numberToText(dst + offset, dstLength - offset, time.tm_mday);
 
     memmove(dst + offset, " ", 1);
     offset += 1;
 
-    offset += prefixWithZero(dst + offset, time.tm_hour);
+    offset += prefixWithZero(dst + offset, dstLength - offset, time.tm_hour);
     offset += numberToText(dst + offset, dstLength - offset, time.tm_hour);
 
     memmove(dst + offset, ":", 1);
     offset += 1;
 
-    offset += prefixWithZero(dst + offset, time.tm_min);
+    offset += prefixWithZero(dst + offset, dstLength - offset, time.tm_min);
     offset += numberToText(dst + offset, dstLength - offset, time.tm_min);
 
     memmove(dst + offset, ":", 1);
     offset += 1;
 
-    offset += prefixWithZero(dst + offset, time.tm_sec);
+    offset += prefixWithZero(dst + offset, dstLength - offset, time.tm_sec);
     offset += bin2dec(dst + offset, dstLength - offset, time.tm_sec);
 
     return offset;

src/common/ui/display_bagl.c

@@ -9,7 +9,7 @@ UX_STEP_NOCB(ux_display_memo_step_nocb,
 
 UX_STEP_CB(ux_display_memo_step,
            bnnn_paging,
-           handleCborStep(),
+           sendSuccessNoIdle(),
            {"Memo", (char *)global.withDataBlob.cborContext.display});
 
 UX_FLOW(ux_display_memo, &ux_display_memo_step);

src/common/util.c

@@ -163,11 +163,6 @@ void getIdentityAccountDisplayNewPath(uint8_t *dst,
  * error that should be sent back to the callee.
  */
 void ensureNoError(cx_err_t errorCode) {
-    // TODO An improvement would be to stop using THROW for the control flow like this
-    // and to explicitly send back the error instead and then stop the flow.
-    // This implementation is a quick patch to the changes made to the Ledger SDK to
-    // mimic the old library functions that would do a similar throw.
-
     if (errorCode != CX_OK) {
         THROW(ERROR_FAILED_CX_OPERATION);
     }
@@ -311,9 +306,12 @@ void blsKeygen(const uint8_t *seed, size_t seedLength, uint8_t *dst, size_t dstL
 
     memcpy(ikm, seed, SEED_LENGTH);
     ikm[SEED_LENGTH] = 0;
-
+    cx_err_t error = 0;
     do {
-        cx_hash_sha256(salt, saltSize, salt, sizeof(salt));
+        error = cx_hash_sha256(salt, saltSize, salt, sizeof(salt));
+        if (error == 0) {
+            THROW(ERROR_FAILED_CX_OPERATION);
+        }
         saltSize = sizeof(salt);
         cx_hkdf_extract(CX_SHA256, ikm, sizeof(ikm), salt, sizeof(salt), prk);
         cx_hkdf_expand(CX_SHA256,

src/deployModule.c

@@ -8,16 +8,24 @@ static tx_state_t *tx_state = &global_tx_state;
 
 void handleDeployModule(uint8_t *cdata, uint8_t p1, uint8_t lc) {
     if (p1 == P1_INITIAL) {
-        cdata += parseKeyDerivationPath(cdata);
         cx_sha256_init(&tx_state->hash);
-        cdata += hashAccountTransactionHeaderAndKind(cdata, DEPLOY_MODULE);
+        size_t offset = parseKeyDerivationPath(cdata);
+        if (offset > lc) {
+            THROW(ERROR_BUFFER_OVERFLOW); // Ensure safe access
+        }
+        cdata += offset;
 
+        offset = hashAccountTransactionHeaderAndKind(cdata, DEPLOY_MODULE);
+        if (offset > lc) {
+            THROW(ERROR_BUFFER_OVERFLOW);  // Ensure safe access
+        }
+        cdata += offset;
         // hash the version and source length
         updateHash((cx_hash_t *)&tx_state->hash, cdata, 8);
         ctx_deploy_module->version = U4BE(cdata, 0);
         ctx_deploy_module->sourceLength = U4BE(cdata, 4);
         ctx_deploy_module->remainingSourceLength = ctx_deploy_module->sourceLength;
-        // TODO: Format the version
+
         numberToText((uint8_t *)ctx_deploy_module->versionDisplay,
                      sizeof(ctx_deploy_module->versionDisplay),
                      ctx_deploy_module->version);

src/getAppName.c

@@ -1,3 +1,5 @@
+#include <assert.h>  // _Static_assert
+
 #include "getAppName.h"
 #include "globals.h"
 

src/globals.h

@@ -78,6 +78,11 @@
  */
 #define MAX_APPNAME_LEN 64
 
+/**
+ * Key length of (Public Key || Verification Key || Account Key)
+ */
+#define KEY_LENGTH 32
+
 typedef enum {
     LEGACY_ID_CRED_SEC = 0,
     LEGACY_PRF_KEY = 1,

src/initContract.c

@@ -10,8 +10,19 @@ static tx_state_t *tx_state = &global_tx_state;
 void handleInitContract(uint8_t *cdata, uint8_t p1, uint8_t lc) {
     if (p1 == P1_INITIAL) {
         cx_sha256_init(&tx_state->hash);
-        cdata += parseKeyDerivationPath(cdata);
-        cdata += hashAccountTransactionHeaderAndKind(cdata, INIT_CONTRACT);
+
+        size_t offset = parseKeyDerivationPath(cdata);
+        if (offset > lc) {
+            THROW(ERROR_BUFFER_OVERFLOW); // Ensure safe access
+        }
+        cdata += offset;
+
+        offset = hashAccountTransactionHeaderAndKind(cdata, INIT_CONTRACT);
+        if (offset > lc) {
+            THROW(ERROR_BUFFER_OVERFLOW);  // Ensure safe access
+        }
+        cdata += offset;
+
         // hash the amount
         updateHash((cx_hash_t *)&tx_state->hash, cdata, 8);
         // extract the amount

src/signConfigureBaker.c

@@ -68,9 +68,18 @@ void handleSignConfigureBaker(uint8_t *cdata,
                               volatile unsigned int *flags,
                               bool isInitialCall) {
     if (P1_INITIAL == p1 && isInitialCall) {
-        cdata += parseKeyDerivationPath(cdata);
         cx_sha256_init(&tx_state->hash);
-        cdata += hashAccountTransactionHeaderAndKind(cdata, CONFIGURE_BAKER);
+        size_t offset = parseKeyDerivationPath(cdata);
+        if (offset > dataLength) {
+            THROW(ERROR_BUFFER_OVERFLOW); // Ensure safe access
+        }
+        cdata += offset;
+
+        offset = hashAccountTransactionHeaderAndKind(cdata, CONFIGURE_BAKER);
+        if (offset > dataLength) {
+            THROW(ERROR_BUFFER_OVERFLOW);  // Ensure safe access
+        }
+        cdata += offset;
         ctx_conf_baker->firstDisplay = true;
 
         // The initial 2 bytes tells us the fields we are receiving.

src/signConfigureDelegation.c

@@ -7,11 +7,17 @@ void handleSignConfigureDelegation(uint8_t *cdata,
                                    uint8_t dataLength,
                                    volatile unsigned int *flags) {
     int keyDerivationPathLength = parseKeyDerivationPath(cdata);
+    if (keyDerivationPathLength > dataLength) {
+        THROW(ERROR_BUFFER_OVERFLOW); // Ensure safe access
+    }
     cdata += keyDerivationPathLength;
 
     cx_sha256_init(&tx_state->hash);
     int accountTransactionHeaderAndKindLength =
         hashAccountTransactionHeaderAndKind(cdata, CONFIGURE_DELEGATION);
+    if (accountTransactionHeaderAndKindLength > dataLength) {
+        THROW(ERROR_BUFFER_OVERFLOW);  // Ensure safe access
+    }
     cdata += accountTransactionHeaderAndKindLength;
 
     // The initial 2 bytes tells us the fields we are receiving.