Vulnerability fixes

Fixed:
- Missing input value validation
- Potential missing secret wiping from stack after use
- String manipulation
- Pointer arithmetic on void pointer
- Trivial swich-case statements were removed
- Replaced certain other memset calls

Author: relatko

src/diffieHellman.c

@@ -370,6 +370,8 @@ __noinline_due_to_stack__ size_t dh_decode(bip44_path_t* pathSpec,
     END_TRY;
 
     TRACE("Finishing decription, written:%d, lastCharacter:%d", written, buffer[written - 1]);
-    // Calculate redulting length based on the last decoded value
+    // Calculate resulting length based on the last decoded value
+    ASSERT(written != 0);
+    ASSERT(written >= buffer[written - 1]);
     return written - buffer[written - 1];
 }

src/eos_utils.c

@@ -179,7 +179,7 @@ bool b58enc(uint8_t *bin, uint32_t binsz, char *b58, uint32_t *b58sz) {
     size = (binsz - zcount) * 138 / 100 + 1;
     uint8_t buf[MAX_B58ENC_LENGTH];
     ASSERT(size <= MAX_B58ENC_LENGTH);
-    memset(buf, 0, sizeof(buf));
+    explicit_bzero(buf, sizeof(buf));
 
     for (i = zcount, high = size - 1; i < binsz; ++i, high = j) {
         for (carry = bin[i], j = size - 1; (j > high) || carry; --j) {
@@ -223,7 +223,7 @@ uint32_t compressed_public_key_to_wif(const uint8_t *publicKey,
     ASSERT(outLength >= 40);
 
     uint8_t temp[37];
-    memset(temp, 0, sizeof(temp));
+    explicit_bzero(temp, sizeof(temp));
     memcpy(temp, publicKey, 33);
 
     uint8_t check[20];
@@ -232,7 +232,7 @@ uint32_t compressed_public_key_to_wif(const uint8_t *publicKey,
     cx_hash(&riprip.header, CX_LAST, temp, 33, check, sizeof(check));
     memcpy(temp + 33, check, 4);
 
-    memset(out, 0, outLength);
+    explicit_bzero(out, outLength);
     out[0] = 'F';
     out[1] = 'I';
     out[2] = 'O';

src/getPublicKey.c

@@ -14,24 +14,6 @@ static ins_get_key_context_t* ctx = &(instructionState.getKeyContext);
 // it should be set to this value at the beginning and after a UI state machine is finished
 static int UI_STEP_NONE = 0;
 
-static inline void CHECK_STAGE(get_key_stage_t expected) {
-    VALIDATE(ctx->stage == expected, ERR_INVALID_STATE);
-}
-
-static void advanceStage() {
-    TRACE("Advancing from stage: %d", ctx->stage);
-
-    switch (ctx->stage) {
-        case GET_KEY_STAGE_INIT:
-            ctx->stage = GET_KEY_STAGE_NONE;
-            ui_idle();  // we are done with this key export
-            break;
-
-        default:
-            ASSERT(false);
-    }
-}
-
 // ============================== Derivation and UI state machine ==============================
 
 enum {
@@ -74,10 +56,10 @@ static void getPublicKey_ui_runStep() {
 
         ctx->responseReadyMagic = 0;  // just for safety
         ui_displayBusy();             // needs to happen after I/O
-
+        
         TRACE("Export done.");
 
-        advanceStage();
+        ui_idle();  // we are done with this key export
     }
     UI_STEP_END(GET_KEY_UI_STEP_INVALID);
 }
@@ -128,10 +110,8 @@ void getPublicKey_handleAPDU(uint8_t p1,
     VALIDATE(p2 == P2_UNUSED, ERR_INVALID_REQUEST_PARAMETERS);
 
     explicit_bzero(ctx, SIZEOF(*ctx));
-    ctx->stage = GET_KEY_STAGE_INIT;
     ctx->ui_step = UI_STEP_NONE;
 
-    CHECK_STAGE(GET_KEY_STAGE_INIT);
     ASSERT(wireDataSize < BUFFER_SIZE_PARANOIA);
 
     {

src/getPublicKey.h

@@ -8,18 +8,12 @@
 
 #define MAX_PUBLIC_KEYS 1000
 
-typedef enum {
-    GET_KEY_STAGE_NONE = 0,
-    GET_KEY_STAGE_INIT = 20,
-} get_key_stage_t;
-
 typedef enum {
     P1_SHOW_PUBKEY = 1,
     P1_DO_NOT_SHOW_PUBKEY = 2,
 } get_key_p1_t;
 
 typedef struct {
-    get_key_stage_t stage;
     get_key_p1_t show_or_not;
 
     bip44_path_t pathSpec;

src/signTransaction.c

@@ -175,7 +175,7 @@ __noinline_due_to_stack__ void signTx_handleInitAPDU(uint8_t p2,
         switch (network) {
 #define CASE(NETWORK, CHAIN_STRING)                                 \
     case NETWORK: {                                                 \
-        snprintf(ctx->value, MAX_DISPLAY_KEY_LENGTH, CHAIN_STRING); \
+        snprintf(ctx->value, MAX_DISPLAY_VALUE_LENGTH, CHAIN_STRING); \
         break;                                                      \
     }
             CASE(NETWORK_MAINNET, "Mainnet");
@@ -204,19 +204,13 @@ __noinline_due_to_stack__ void signTx_handleInitAPDU(uint8_t p2,
         policy = policyForSignTxInit(&ctx->wittnessPath);
         TRACE("Policy: %d", (int) policy);
         ENSURE_NOT_DENIED(policy);
-        {
-            // select UI steps
-            switch (policy) {
-#define CASE(POLICY, UI_STEP)   \
-    case POLICY: {              \
-        ctx->ui_step = UI_STEP; \
-        break;                  \
-    }
-                CASE(POLICY_SHOW_BEFORE_RESPONSE, HANDLE_SIMPLE_STEP_DISPLAY_DETAILS);
-                default:
-                    THROW(ERR_NOT_IMPLEMENTED);
-#undef CASE
-            }
+        // select UI step
+        if (policy == POLICY_SHOW_BEFORE_RESPONSE) {
+            ctx->ui_step = HANDLE_SIMPLE_STEP_DISPLAY_DETAILS;
+        }
+        else {
+            THROW(ERR_NOT_IMPLEMENTED);
+
         }
     }
 
@@ -296,7 +290,7 @@ __noinline_due_to_stack__ void signTx_handleShowMessageAPDU(
     struct {
         uint8_t displayValueLen;
         uint8_t displayValue[MAX_DISPLAY_VALUE_LENGTH];
-    }* constData2 = (void*) constDataBuffer + 1 + constData->displayKeyLen;
+    }* constData2 = (void*) (constDataBuffer + 1 + constData->displayKeyLen);
     VALIDATE(constData2->displayValueLen < MAX_DISPLAY_VALUE_LENGTH - 1, ERR_INVALID_DATA);
     VALIDATE(constSize == 2 + constData->displayKeyLen + constData2->displayValueLen,
              ERR_INVALID_DATA);
@@ -848,19 +842,12 @@ __noinline_due_to_stack__ void signTx_handleEndDHEncodingAPDU(
             policy = policyForSignTxDHEnd();
             TRACE("Policy: %d", (int) policy);
             ENSURE_NOT_DENIED(policy);
-            {
-                // select UI steps
-                switch (policy) {
-#define CASE(POLICY, UI_STEP)   \
-    case POLICY: {              \
-        ctx->ui_step = UI_STEP; \
-        break;                  \
-    }
-                    CASE(POLICY_PROMPT_BEFORE_RESPONSE, HANDLE_DH_END_STEP_CONFIRM);
-                    default:
-                        THROW(ERR_NOT_IMPLEMENTED);
-#undef CASE
-                }
+            // select UI step
+            if (policy == POLICY_PROMPT_BEFORE_RESPONSE) {
+                ctx->ui_step = HANDLE_DH_END_STEP_CONFIRM;
+            }
+            else {
+                THROW(ERR_NOT_IMPLEMENTED);
             }
         }
     }
@@ -952,19 +939,12 @@ __noinline_due_to_stack__ void signTx_handleFinishAPDU(
         policy = policyForSignTxFinish();
         TRACE("Policy: %d", (int) policy);
         ENSURE_NOT_DENIED(policy);
-        {
-            // select UI steps
-            switch (policy) {
-#define CASE(POLICY, UI_STEP)   \
-    case POLICY: {              \
-        ctx->ui_step = UI_STEP; \
-        break;                  \
-    }
-                CASE(POLICY_PROMPT_BEFORE_RESPONSE, HANDLE_FINISH_STEP_DISPLAY_DETAILS);
-                default:
-                    THROW(ERR_NOT_IMPLEMENTED);
-#undef CASE
-            }
+        // select UI step
+        if (policy == POLICY_PROMPT_BEFORE_RESPONSE) {
+            ctx->ui_step = HANDLE_FINISH_STEP_DISPLAY_DETAILS;
+        }
+        else {
+            THROW(ERR_NOT_IMPLEMENTED);
         }
     }
 
@@ -1032,7 +1012,7 @@ __noinline_due_to_stack__ void signTx_handleFinishAPDU(
             }
         }
         FINALLY {
-            memset(&privateKey, 0, sizeof(privateKey));
+            explicit_bzero(&privateKey, sizeof(privateKey));
         }
     }
     END_TRY;